tests/periph_pm: make usage more intuitive and move shell commands to sys/ #11731

benemorius · 2019-06-22T03:33:08Z

Contribution description

Using tests/periph_pm can be unintuitive if you aren't already very familiar with the PM subsystem. I identified and addressed two specific causes and I think this greatly improves the situation.

there is no way to see which modes are currently blocked or unblocked
unblocking an already-unblocked mode triggers an assertion failure and kernel panic which can make the user/tester/developer think there's some serious problem when really there is no problem

This PR addresses 1) by adding a new command pm show to view blockers and 2) by adding a check to print a friendly error message when unblocking an unblocked mode.

Testing procedure

> pm show
mode 0 blockers: 1 
mode 1 blockers: 0 
mode 2 blockers: 0 
mode 3 blockers: 0 
> unblock 2
Mode 2 is already unblocked.
1 >

Issues/PRs references

I think some confusion earlier in #7897 can be avoided with these changes.

Updates

Print current PM blockers on startup (execute pm show). In case we boot into an unresponsive shell, we at least get some direction for troubleshooting, as this tells us which power mode must have been entered.
Increased pm_set() verbosity. It may not be obvious to the user that running shell command set %i (which calls pm_set(i)) is forcing the CPU into the specified power mode, and that such manual forcing is not necessarily an orthodox thing to do. I tried to increase the verbosity somewhat so the process is clearer. Also, this verbosity provides an event (some printf output) which indicates the moment when the CPU woke up, whereas previously we didn't know when it had woken up except by typing into the shell to check for a response. In some cases, the CPU might always wake immediately (or just sooner than expected) after set %i. This should reveal those cases.

aabadie · 2019-06-22T20:34:09Z

Nice contribution! I like the idea of having access to the pm blocker status from the shell.

The new pm command made me think to an alternative: how about having only one pm command in the shell with subcommands for each actions (reboote, set, block, unblock, etc) and add a new status (or show) to print the current pm blocker status ? What do you think ?

benemorius · 2019-06-22T23:49:41Z

Thanks. Yes I also think it seems like a good improvement to merge the commands into one. I only didn't for brevity.

I have even thought that these commands would be useful to move outside of tests/periph_pm. For instance I was testing power modes while running various examples/ and I repeatedly tried to use these commands by accident.

I've been postponing most of the shell command changes I've wanted to make because I'd like to benefit effort-wise from (the concept of) #9538, but since we only need simple parsing here I don't mind so much.

However, I suppose this topic is at risk of rapidly deviating from what I hoped might be a relatively quick merge (ultimately I am trying to push #7897 forward along with subsequent PM efforts). I would be content to aim for a minimal PR for now, provided that consideration is given to forward compatibility with future improvements. Probably I will be looking at PM on EFM32 and EFR32 after this, so I will still have plenty of opportunity and context to make improvements here during that.

benemorius · 2019-06-23T05:32:32Z

@benemorius

provided that consideration is given to forward compatibility with future improvements

For example the current command pm is forward-compatible equivalent with a future pm show command because pm show could be the default behavior when a future pm command is called with no arguments.

aabadie · 2019-06-23T09:50:29Z

I would be content to aim for a minimal PR for now, provided that consideration is given to forward compatibility with future improvements.

Having all subcommands under pm was just a suggestion. It's great if you had the same idea. I'm fine doing this in follow-up PRs.

the current command pm is forward-compatible equivalent with a future pm show command because pm show could be the default behavior when a future pm command is called with no arguments

Calling pm alone should print the basic usage of the command with available subcommands. So it won't be forward-compatible in this case.

Maybe you could rename the new pm command in status (or show as you prefer) in this PR. This will smooth the future merge in a single command.

benemorius · 2019-07-03T20:10:33Z

I changed the command to pm show and moved it out of tests/periph_pm into sys/shell/commands so it can be used also to test that power modes behave correctly in various examples.

Also the command is run automatically when tests/periph_pm starts in case the shell isn't responsive:

main(): This is RIOT! (Version: 2019.07-devel-929-g881951-HEAD)
This application allows you to test the CPU power management.
The available power modes are 0 - 3. Lower-numbered power modes
save more power, but may require an event/interrupt to wake up
the CPU. Reset the CPU if needed.
mode 0 blockers: 0 
mode 1 blockers: 0 
mode 2 blockers: 0 
mode 3 blockers: 0 
lowest allowed mode: 0
>

roberthartung · 2019-07-30T17:31:49Z

Will have a look tomorrow :)

roberthartung · 2019-07-30T17:36:31Z

We should think about how this behaves if we have a low-power uart on a board that can wakeup the system. I know that there was a PR/Issue for that #7947 and #11310 - will have to look what they do :)

benemorius · 2019-07-30T20:07:52Z

We should think about how this behaves if we have a low-power uart on a board that can wakeup the system

In that case pm show will show the blocker for that and you can verify the expected behavior by manually unblocking it and observing that uart rx stops working. Also depending on the uart driver it may be possible that you can type a character at just the right time (while pm_blocker is being read) to see a blocker that is only active during frame reception.

Also if the uart_write() implementation is nonblocking, it's possible for pm_blocker to be sampled while the uart is actively transmitting or a DMA transfer is in progress and then those blockers will be shown. I don't think there are any uart drivers like that in master yet, but I've observed this case in a local branch with DMA buffered transmit and it works as expected.

stale · 2020-01-31T21:05:46Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want me to ignore this issue, please mark it with the "State: don't stale" label. Thank you for your contributions.

roberthartung · 2020-02-17T09:27:41Z

I will give this another test so we can merge asap!

aabadie

There are minor things that could still be improved in this PR. See my suggestions below.

Otherwise, I think that the definition of pm_blocker_t could be moved to pm_layered.h (and related TODOs could be removed).

sys/shell/commands/sc_pm.c

tests/periph_pm/main.c

benemorius · 2020-03-01T00:27:07Z

Sorry for the delay. I moved pm_blocker_t to pm_layered.h

benemorius · 2020-03-22T17:57:46Z

I think I've addressed all your comments @aabadie

gschorcht

Some comments from my side:

Why did you define the commands block, unblock and set commands twice, once in tests/periph_pm and once in sc_pm.c? They should be placed in sc_pm and tests/periph_pm should use them as a module.
We should have all PM commands in sc_pm.c available. Ok, the reboot command is a standard shell command, but off should be also available if feature periph_pm is provided. The unblock_rtc is only for testing and is the only command that could stay in tests/periph_pm from my point of view.
Indeed, a question is whether the pm command should be included whenever the feature periph_pm is provided or the module pm_layered is used. It depends on the purpose. If it is to be a standard command, it should be included whenever the feature periph_pm is provided or module pm_layered is used. However, if it is only intended for testing, it should only be included explicitly. In any case, it will increase the the ROM size.

sys/shell/commands/sc_pm.c

benemorius · 2020-03-27T22:01:43Z

@gschorcht

Oops I knew something mundane was missing from this. I hadn't bothered to deduplicate that yet. I added a fixup.
Ok I added pm off
I think you addressed this in sys/shell: add pm command for power management #13679 right? Would you be content to address this part in a followup PR?

gschorcht · 2020-03-29T13:54:46Z

3. Indeed, a question is whether the pm command should be included whenever the feature periph_pm is provided or the module pm_layered is used. It depends on the purpose. If it is to be a standard command, it should be included whenever the feature periph_pm is provided or module pm_layered is used. However, if it is only intended for testing, it should only be included explicitly. In any case, it will increase the the ROM size.

3. I think you addressed this in #13679 right?

Yes.

Would you be content to address this part in a followup PR?

I would like to have it considered in this PR for the following reasons:

The commands strongly affect the system state and in the worst case can lead to an unusable system. Therefore, an application developer might not want to expose these commands in his application.
The commands are pulled in automatically with module shell_commands when feature pm_layered is provided. So it increases the code size and RAM requirements even if they are not used or required.

Therefore, I would suggest to provide the command by a separate pseudomodule pm_cmd which also might be pulled in automatically if feature pm_layered is provided. The difference to your approach would be that it could be disabled explicitly.

@aabadie How do you think about that?

aabadie · 2020-04-07T12:56:36Z

I'm wondering whether it would be better to pull it in when periph_pm is enabled and to embed pm_layered related commands in #ifdef MODULE_PM_LAYERED ... #endif so that at least the off command is available without pm_layered

I think it's a good idea.

So basically use:

in sys/shell/commands/Makefile

ifneq (,$(filter periph_pm,$(USEMODULE)))
  SRC += sc_pm.c
endif

in sys/shell/commands/sc_pm.c

#ifdef MODULE_PM_LAYERED
#include "pm_layered.h"

extern volatile pm_blocker_t pm_blocker; /* sys/pm_layered/pm.c */
#endif /* MODULE_PM_LAYERED */


static void _print_usage(void) {
    puts("Usage:");
#ifdef MODULE_PM_LAYERED
    puts("\tpm show: display current blockers for each power mode");
    puts("\tpm set <mode>: manually set power mode (lasts until WFI returns)");
    puts("\tpm block <mode>: manually block power mode");
    puts("\tpm unblock <mode>: manually unblock power mode");
#endif /* MODULE_PM_LAYERED */
    puts("\tpm off: call pm_off()");
}

#ifdef MODULE_PM_LAYERED
static int check_mode(int argc, char **argv)
{
[...]

#endif /* MODULE_PM_LAYERED */

static int cmd_off(char *arg)
{
    (void)arg;

    pm_off();

    return 0;
}

[...] etc

@benemorius, would mind updating this PR once more ? After that we should be good to go. Thanks!

aabadie · 2020-04-28T07:42:43Z

@benemorius @gschorcht, let's move forward with this one. Is it ok for both of you if I take over this PR and open a new one adding the requested changes ?

aabadie · 2020-04-28T11:12:03Z

I think you meant @benemorius

Yes, and I don't see where is the typo :)

aabadie · 2020-04-28T14:58:02Z

I finally went for the force-push option in your branch @benemorius.
After addressing the current comments, I found other minor things and now it should be good.
I also tested this PR locally and it works as expected.

Maybe someone could give a final round ? @fjmolinas ?

fjmolinas

Looks good, I think there is no reason for unclock_rtc to not be moved as well, but that can be in a followup. For what I can see @aabadie just implemented the final nitpicks that had already been agreed on, ACK.

USEMODULE=rtt_rtc PROGRAMMER=jlink BOARD=openmote-b make -C tests/periph_pm flash term

2020-04-28 17:13:41,402 # help
2020-04-28 17:13:41,403 # Command              Description
2020-04-28 17:13:41,403 # ---------------------------------------
2020-04-28 17:13:41,405 # unblock_rtc          temporarily unblock power mode
2020-04-28 17:13:41,406 # reboot               Reboot the node
2020-04-28 17:13:41,418 # version              Prints current RIOT_VERSION
2020-04-28 17:13:41,419 # pm                   interact with layered PM subsystem
2020-04-28 17:13:41,420 # rtc                  control RTC peripheral interface
> rtc
2020-04-28 17:13:44,266 #  rtc
2020-04-28 17:13:44,266 # usage: rtc <command> [arguments]
2020-04-28 17:13:44,267 # commands:
2020-04-28 17:13:44,267 # 	poweron		power the interface on
2020-04-28 17:13:44,268 # 	poweroff	power the interface off
2020-04-28 17:13:44,268 # 	clearalarm	deactivate the current alarm
2020-04-28 17:13:44,282 # 	getalarm	print the currently alarm time
2020-04-28 17:13:44,284 # 	setalarm YYYY-MM-DD HH:MM:SS
2020-04-28 17:13:44,285 # 			set an alarm for the specified time
2020-04-28 17:13:44,286 # 	gettime		print the current time
2020-04-28 17:13:44,287 # 	settime YYYY-MM-DD HH:MM:SS
2020-04-28 17:13:44,287 # 			set the current time
unblock_rtc 1 3
2020-04-28 17:14:08,041 #  unblock_rtc 1 3
2020-04-28 17:14:08,042 # Unblocking power mode 1 for 3 seconds.
pm show
2020-04-28 17:14:14,136 #  pm show
2020-04-28 17:14:14,137 # mode 0 blockers: 1 
2020-04-28 17:14:14,138 # mode 1 blockers: 1 
2020-04-28 17:14:14,152 # mode 2 blockers: 1 
2020-04-28 17:14:14,152 # mode 3 blockers: 1 
2020-04-28 17:14:14,153 # Lowest allowed mode: 4
unblock_rtc 1 3
2020-04-28 17:14:15,960 #  unblock_rtc 1 3
2020-04-28 17:14:15,961 # Unblocking power mode 1 for 3 seconds.
pm show 3
2020-04-28 17:14:16,936 #  pm show
2020-04-28 17:14:16,937 # mode 0 blockers: 1 
2020-04-28 17:14:16,952 # mode 1 blockers: 0 
2020-04-28 17:14:16,952 # mode 2 blockers: 1 
2020-04-28 17:14:16,953 # mode 3 blockers: 1 
2020-04-28 17:14:16,953 # Lowest allowed mode: 4
pm show
2020-04-28 17:14:19,656 #  pm show
2020-04-28 17:14:19,657 # mode 0 blockers: 1 
2020-04-28 17:14:19,672 # mode 1 blockers: 1 
2020-04-28 17:14:19,672 # mode 2 blockers: 1 
2020-04-28 17:14:19,673 # mode 3 blockers: 1 
2020-04-28 17:14:19,673 # Lowest allowed mode: 4
pm unblock 2
2020-04-28 17:14:28,808 #  pm unblock 2
2020-04-28 17:14:28,809 # Unblocking power mode 2.
pm show
2020-04-28 17:14:31,975 #  pm show
2020-04-28 17:14:31,977 # mode 0 blockers: 1 
2020-04-28 17:14:31,977 # mode 1 blockers: 1 
2020-04-28 17:14:31,978 # mode 2 blockers: 0 
2020-04-28 17:14:31,979 # mode 3 blockers: 1 
2020-04-28 17:14:31,979 # Lowest allowed mode: 4
unblock_rtc 3 3
2020-04-28 17:14:41,034 #  unblock_rtc 3 3
help
2020-04-28 17:14:45,433 # Unblocking power mode 3 ���x
                                                      ��f�����`��?��������help
2020-04-28 17:14:45,436 # Command              Description
2020-04-28 17:14:45,438 # ---------------------------------------
2020-04-28 17:14:45,439 # unblock_rtc          temporarily unblock power mode
2020-04-28 17:14:45,449 # reboot               Reboot the node
2020-04-28 17:14:45,450 # version              Prints current RIOT_VERSION
2020-04-28 17:14:45,451 # pm                   interact with layered PM subsystem
2020-04-28 17:14:45,466 # rtc                  control RTC peripheral interface
pm show
2020-04-28 17:14:51,385 #  pm show
2020-04-28 17:14:51,386 # mode 0 blockers: 1 
2020-04-28 17:14:51,388 # mode 1 blockers: 1 
2020-04-28 17:14:51,389 # mode 2 blockers: 0 
2020-04-28 17:14:51,389 # mode 3 blockers: 1 
2020-04-28 17:14:51,390 # Lowest allowed mode: 4
> pm show
2020-04-28 17:14:55,225 #  pm show
2020-04-28 17:14:55,225 # mode 0 blockers: 1 
2020-04-28 17:14:55,225 # mode 1 blockers: 1 
2020-04-28 17:14:55,241 # mode 2 blockers: 0 
2020-04-28 17:14:55,242 # mode 3 blockers: 1 
2020-04-28 17:14:55,242 # Lowest allowed mode: 4

aabadie · 2020-04-28T15:45:50Z

Some boards doesn't have enough memory now. I'll exclude them from the CI.

aabadie · 2020-04-28T16:52:05Z

And go!

benemorius force-pushed the tests-periph_pm-ux branch from 5161072 to 3a851bd Compare July 3, 2019 19:51

benemorius mentioned this pull request Jul 4, 2019

[Tracking] kinetis: Implement low power modes #11789

Open

benemorius changed the title ~~tests/periph_pm: make usage more intuitive~~ tests/periph_pm: make usage more intuitive and move shell commands to sys/ Jul 10, 2019

miri64 requested review from MichelRottleuthner and roberthartung July 30, 2019 17:30

miri64 added Area: tests Area: tests and testing framework Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation labels Jul 30, 2019

stale bot added the State: stale State: The issue / PR has no activity for >185 days label Jan 31, 2020

smlng added State: don't stale State: Tell state-bot to ignore this issue and removed State: stale State: The issue / PR has no activity for >185 days labels Feb 3, 2020

aabadie requested changes Feb 17, 2020

View reviewed changes

benemorius requested review from kaspar030 and MrKevinWeiss as code owners February 29, 2020 23:13

aabadie mentioned this pull request Mar 22, 2020

sys/shell: add pm command for power management #13679

Closed

5 tasks

gschorcht reviewed Mar 22, 2020

View reviewed changes

sys/shell/commands/sc_pm.c Outdated Show resolved Hide resolved

benemorius force-pushed the tests-periph_pm-ux branch from f79e3ec to edc21ef Compare March 27, 2020 21:58

aabadie force-pushed the tests-periph_pm-ux branch 2 times, most recently from c21ab6e to 591e43a Compare April 28, 2020 14:51

benemorius requested review from cgundogan, haukepetersen, jia200x, leandrolanzieri, miri64 and PeterKietzmann as code owners April 28, 2020 14:51

benemorius added 4 commits April 28, 2020 16:51

sys/shell/commands: add pm show command to view current blockers

7c82ac6

tests/periph_pm: print block/unblock error instead of assertion failure

100390c

tests/periph_pm: print current PM blockers on startup

90579e8

tests/periph_pm: be more verbose about what pm_set() is doing

aa97e7b

aabadie force-pushed the tests-periph_pm-ux branch from 591e43a to 9659300 Compare April 28, 2020 14:51

sys/shell/commands: pm: add set mode block mode unblock mode

325ab42

aabadie force-pushed the tests-periph_pm-ux branch from 9659300 to 325ab42 Compare April 28, 2020 14:56

fjmolinas approved these changes Apr 28, 2020

View reviewed changes

aabadie added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Apr 28, 2020

aabadie approved these changes Apr 28, 2020

View reviewed changes

tests: add new boards in low memory blacklists

804cc8c

benemorius requested a review from smlng as a code owner April 28, 2020 16:35

aabadie merged commit f22529e into RIOT-OS:master Apr 28, 2020

miri64 added this to the Release 2020.07 milestone Jun 24, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tests/periph_pm: make usage more intuitive and move shell commands to sys/ #11731

tests/periph_pm: make usage more intuitive and move shell commands to sys/ #11731

benemorius commented Jun 22, 2019 •

edited

Loading

aabadie commented Jun 22, 2019

benemorius commented Jun 22, 2019

benemorius commented Jun 23, 2019

aabadie commented Jun 23, 2019

benemorius commented Jul 3, 2019

roberthartung commented Jul 30, 2019

roberthartung commented Jul 30, 2019

benemorius commented Jul 30, 2019

stale bot commented Jan 31, 2020

roberthartung commented Feb 17, 2020

aabadie left a comment

benemorius commented Mar 1, 2020 •

edited

Loading

benemorius commented Mar 22, 2020

gschorcht left a comment

benemorius commented Mar 27, 2020

gschorcht commented Mar 29, 2020 •

edited

Loading

aabadie commented Apr 7, 2020 •

edited

Loading

aabadie commented Apr 28, 2020

aabadie commented Apr 28, 2020

aabadie commented Apr 28, 2020

fjmolinas left a comment

aabadie commented Apr 28, 2020

aabadie commented Apr 28, 2020

tests/periph_pm: make usage more intuitive and move shell commands to sys/ #11731

tests/periph_pm: make usage more intuitive and move shell commands to sys/ #11731

Conversation

benemorius commented Jun 22, 2019 • edited Loading

Contribution description

Testing procedure

Issues/PRs references

Updates

aabadie commented Jun 22, 2019

benemorius commented Jun 22, 2019

benemorius commented Jun 23, 2019

aabadie commented Jun 23, 2019

benemorius commented Jul 3, 2019

roberthartung commented Jul 30, 2019

roberthartung commented Jul 30, 2019

benemorius commented Jul 30, 2019

stale bot commented Jan 31, 2020

roberthartung commented Feb 17, 2020

aabadie left a comment

Choose a reason for hiding this comment

benemorius commented Mar 1, 2020 • edited Loading

benemorius commented Mar 22, 2020

gschorcht left a comment

Choose a reason for hiding this comment

benemorius commented Mar 27, 2020

gschorcht commented Mar 29, 2020 • edited Loading

aabadie commented Apr 7, 2020 • edited Loading

aabadie commented Apr 28, 2020

aabadie commented Apr 28, 2020

aabadie commented Apr 28, 2020

fjmolinas left a comment

Choose a reason for hiding this comment

aabadie commented Apr 28, 2020

aabadie commented Apr 28, 2020

benemorius commented Jun 22, 2019 •

edited

Loading

benemorius commented Mar 1, 2020 •

edited

Loading

gschorcht commented Mar 29, 2020 •

edited

Loading

aabadie commented Apr 7, 2020 •

edited

Loading