-
Notifications
You must be signed in to change notification settings - Fork 618
os/pm: Add PMIOC_STOP to stop pm and show state in pm_procfs #6979
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
ebae5c7 to
a7ae0e3
Compare
apps/examples/power/power_main.c
Outdated
| pid = task_create("stop_pm_test", 100, 1024, stop_pm_test, argv + 1); | ||
| if (pid < 0) { | ||
| printf("Fail to create stop_pm_test task(errno %d)\n", get_errno()); | ||
| is_running = true; | ||
| return -1; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we don't need stop_pm_test task
it is requred only calling stop ioctl.
if(ioctl(fd, PMIOC_STOP, 0) < 0) {
printf("Fail to pm start(errno %d)\n", get_errno());
close(fd);
return -1;
}There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And how about make each testing function like below? (start / stop)
we can move log of start and stop to actually start and end entry point.
static int pm_start(void)
{
if(ioctl(fd, PMIOC_START, 0) < 0) ~~
}
static int pm_stop(void)
{
if(ioctl(fd, PMIOC_STOP, 0) < 0) ~~
}
....
if (strncmp(argv[1], "start", 6) == 0) {
...
is_running = true;
printf("######################### PM LONG TERM TEST START #########################\n");
pm_stop();
} else if (strncmp(argv[1], "stop", 5) == 0) {
...
pm_stop();
printf("######################### PM LONG TERM TEST END #########################\n");
is_running = true;
}| void pm_stop(void) | ||
| { | ||
| g_pmglobals.is_running = false; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me this stop fucntion.
os/include/tinyara/fs/ioctl.h
Outdated
| #define PMIOC_METRICS _PMIOC(0x0007) | ||
| #define PMIOC_SUSPEND_COUNT _PMIOC(0x0008) | ||
| #define PMIOC_START _PMIOC(0x0009) | ||
| #define PMIOC_STOP _PMIOC(0x0010) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
0x10 is not a next value of 0x09.
Please change this value to 0x0A
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed. Thank you.
| { | ||
| enum pm_state_e pm_state; | ||
|
|
||
| readprint("PM %s\n\n", (g_pmglobals.is_running) ? "RUNNING" : "STOPPED"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
procfs is not only for information.
sometimes application should parse data from procfs, so it shoul memcpy data to buffer.
Hence if you want reveal running state, then you should create additional directory, not here.
|
|
||
| void pm_stop(void) | ||
| { | ||
| g_pmglobals.is_running = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pm_ioctl doesn't use semaphore, so I think this is not 'thread-safe'
apps/examples/power/power_main.c
Outdated
|
|
||
| fd = open(PM_DRVPATH, O_WRONLY); | ||
| if (fd < 0) { | ||
| printf("Fail to open pm start(errno %d)", get_errno()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's open pm driver, not open pm start.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed. Thank you.
And I modified other open error message too.
apps/examples/power/power_main.c
Outdated
| } | ||
|
|
||
| if(ioctl(fd, PMIOC_STOP, 0) < 0) { | ||
| printf("Fail to pm start(errno %d)\n", get_errno()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks fail to stop pm, not start
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
apps/examples/power/power_main.c
Outdated
| return -1; | ||
| } | ||
|
|
||
| if(ioctl(fd, PMIOC_STOP, 0) < 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
space.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed this line and also in start_pm_test
apps/examples/power/power_main.c
Outdated
|
|
||
| is_running = false; | ||
|
|
||
| pid = task_create("stop_pm_test", 100, 1024, stop_pm_test, argv + 1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you let me know why you give argv + 1?
In help message, the stop command does not have any following argument.
And the task creation for start also give it, but is it right with +1? not +2?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, stop_pm_test doesn't need any argument.
So I fixed argv + 1 to NULL.
argv + 1 is the correct value to use because argv is a pointer to the first element of the command argument.
When the command line is something like:
power start -t
the argv array is initialized as:
argv[0] = "power"
argv[1] = "start"
argv[2] = "-t"
argv[3] = NULL
To pass only the part after the program name(i.e.,{"start", "-t"}), need to give the pointer that starts at argv[1]. So argv + 1 is right.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@seokhun-eom24 You already check the argv[1] with start.
if (strncmp(argv[1], "start", 6) == 0) {
if (is_running) {
printf("power test is already running\n");
return 0;
}
is_running = true;
pid = task_create("start_pm_test", 100, 1024, start_pm_test, argv + 1);
The task is created for start. Why do you need to give start argument to the task?
apps/examples/power/power_main.c
Outdated
| pid = task_create("stop_pm_test", 100, 1024, stop_pm_test, argv + 1); | ||
| if (pid < 0) { | ||
| printf("Fail to create stop_pm_test task(errno %d)\n", get_errno()); | ||
| is_running = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you set the variable to false after this creation, what happen?
I mean
do you need to set it to false always and then revert it with failure case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In start_pm_test, is_running should be set first and start task,
but as you said in stop_pm_test reset is_running can handled after create task.
So I modified the flow to reset after create task.
| * | ||
| ****************************************************************************/ | ||
|
|
||
| void pm_stop(void) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file is pm_initialize. Let's add this with new file.
same for pm_start
Currently, pm driver only support start function with PMIOC_START. So I added pm stop PMIOC_STOP to stop pm functionality. Signed-off-by: seokhun-eom <[email protected]>
In current power example code when user input "power start" command,
"PM LONG TERM TEST END" log is printed immediately after "PM LONG TERM TEST START" log like below.
```
TASH>>power start
TASH>>######################### PM LONG TERM TEST START #########################
######################## PM LONG TERM TEST END #########################
```
To fix this issue, add stop_pm_test function and call it when user input "power stop" command.
This function calls PMIOC_STOP to stop pm functionality.
Signed-off-by: seokhun-eom <[email protected]>
Fix typo fucntion to function. Fix function brace convention to enter after function name. Signed-off-by: seokhun-eom <[email protected]>
Modify pm_procfs to print whether pm is RUNNING or STOPPED. Example ``` TASH>>cat state PM STOPPED * NORMAL SLEEP TASH>>power start TASH>>######################### PM LONG TERM TEST START ######################### TASH>>cat state PM RUNNING * NORMAL SLEEP TASH>>power stop TASH>>######################### PM LONG TERM TEST END ######################### TASH>>cat state PM STOPPED * NORMAL SLEEP ``` Signed-off-by: seokhun-eom <[email protected]>
a7ae0e3 to
40a7baa
Compare
| return 0; | ||
| } | ||
|
|
||
| pid = task_create("stop_pm_test", 100, 1024, stop_pm_test, NULL); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any reason why we should do start and stop test in separate tasks?
| return -1; | ||
| } | ||
|
|
||
| if (ioctl(fd, PMIOC_STOP, 0) < 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code can be called first than "start_pm_test" task termination
like
call resume(10)
######################### PM LONG TERM TEST END #########################
call suspend(10)
|
LGTM |
1) Add PMIOC_STOP to stop pm.
Currently, pm driver only support start function with PMIOC_START.
So I added pm stop PMIOC_STOP to stop pm functionality.
2) Add pm stop example code
In current power example code when user input "power start" command,
"PM LONG TERM TEST END" log is printed immediately after "PM LONG TERM TEST START" log like below.
To fix this issue, add stop_pm_test function and call it when user input "power stop" command.
This function calls PMIOC_STOP to stop pm functionality.
3) Fix typo and convention in pm_start
Fix typo fucntion to function.
Fix function brace convention to enter after function name.
4) Add pm running state in pm_procfs
Modify pm_procfs to print whether pm is RUNNING or STOPPED.
Example