Skip to content
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

sys/pm: Fix behavior of fallback pm_off() implementation #13978

Merged
merged 1 commit into from
May 1, 2020

Conversation

maribu
Copy link
Member

@maribu maribu commented Apr 29, 2020

Contribution description

  • pm_off() should prevent other threads from getting executed after it is called, as it should turn off the MCU completely according to its doc
    • Previously, the fallback implementation would still allow other threads to continue working
    • Simply disabling IRQs and never enabling them again should make sure the MCU behaves like it would be completely off
  • pm_off() should reduce the power consumption as much as possible
    • Previously, when IRQs came after the call to pm_set_lowest() in the fallback implementation of pm_off(), while(1) {} got executed at full power consumption
    • Just calling pm_set(0); in a loop while make sure that lowest power mode is restored if the MCU wakes up again.
    • The check if the lowest power mode is available is skipped, as no code gets executed afterwards anyway

Testing procedure

The fallback implementation of pm_off() should now prevent any code from being executed after it is called.

I'll add a test application to this PR later on. Update: tests/periph_pm is fully sufficient for testing this.

Issues/PRs references

Split out of #13973 (but improved afterwards)

- pm_off() should prevent other threads from getting executed after it is
  called, as it should turn off the MCU completely according to its doc
    - Previously, the fallback implementation would still allow other threads
      to continue working
    - Simply disabling IRQs and never enabling them again should make sure
      the MCU behaves like it would be completely off
- pm_off() should reduce the power consumption as much as possible
    - Previously, when IRQs came after the call to pm_set_lowest() in the
      fallback implementation of pm_off(), `while(1) {}` got executed at full
      power consumption
    - Just calling `pm_set(0);` in a loop while make sure that lowest power mode
      is restored if the MCU wakes up again.
    - The check if the lowest power mode is available is skipped, as no code
      gets executed afterwards anyway
@maribu maribu added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Area: pm Area: (Low) power management labels Apr 29, 2020
@maribu
Copy link
Member Author

maribu commented Apr 29, 2020

I'll add a test application to this PR later on.

After a second look into tests/periph_pm I changed my mind: tests/periph_pm already contains everything needed. The callback for BTN0 should no longer be reachable if pm_off() is called (using the pm shell command).

@waehlisch
Copy link
Member

@maribu, can you improve the title of this PR?

@maribu maribu changed the title https://github.com/RIOT-OS/RIOT/pull/13976 sys/pm: Fix behavior of fallback pm_off() implementation Apr 29, 2020
@maribu
Copy link
Member Author

maribu commented Apr 29, 2020

Btw: The implementation of pm_off() in drivers/periph_common/pm.c is now almost identical with the one here.

@maribu
Copy link
Member Author

maribu commented Apr 29, 2020

Maybe @benpicco could test this using his mcb2388 board? However, in order to reproduce the issue (which in its full beauty can only occur if no off mode is implemented) the following patch needs to be (temporary) applied:

diff --git a/cpu/lpc2387/periph/pm.c b/cpu/lpc2387/periph/pm.c
index 2afbb92509..5fe853fdb6 100644
--- a/cpu/lpc2387/periph/pm.c
+++ b/cpu/lpc2387/periph/pm.c
@@ -33,7 +33,7 @@ void pm_set(unsigned mode)
         case 0:
             /* Everything except battery backup is powered down */
             DEBUG_PUTS("pm_set(): setting Deep Power Down mode.");
-            PCON |= PM_DEEP_POWERDOWN;
+            PCON |= PM_IDLE;
             break;
         case 1:
             /* PLL & Flash are powered down */

Note: The race condition fixed in this PR affects every user of pm_layered, including those implementing deep power down mode.

@benpicco
Copy link
Contributor

benpicco commented Apr 29, 2020

Maybe @benpicco could test this using his mcb2388 board?

Yup that works:

2020-04-29 17:43:23,081 # mode 0 blockers: 1 
2020-04-29 17:43:23,082 # mode 1 blockers: 1 
2020-04-29 17:43:23,082 # mode 2 blockers: 1 
2020-04-29 17:43:23,082 # Lowest allowed mode: 3
2020-04-29 17:43:23,083 # using BTN0 as wake-up source
# button press is registered
> 2020-04-29 17:43:37,862 #  BTN0 pressed.
2020-04-29 17:43:47,657 # pm off
# button presses are ignored

Whereas on master

2020-04-29 17:45:14,518 # mode 0 blockers: 1 
2020-04-29 17:45:14,519 # mode 1 blockers: 1 
2020-04-29 17:45:14,520 # mode 2 blockers: 1 
2020-04-29 17:45:14,520 # Lowest allowed mode: 3
2020-04-29 17:45:14,521 # using BTN0 as wake-up source
> 2020-04-29 17:45:30,662 #  BTN0 pressed.
2020-04-29 17:45:33,447 # pm off
2020-04-29 17:45:35,415 # BTN0 pressed.
2020-04-29 17:45:35,767 # BTN0 pressed.
2020-04-29 17:45:36,072 # BTN0 pressed.

@benpicco benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Apr 29, 2020
@cgundogan cgundogan added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Apr 29, 2020
@benpicco benpicco merged commit ee7ec7e into RIOT-OS:master May 1, 2020
@maribu maribu deleted the fix_pm_off branch May 11, 2020 13:37
@miri64 miri64 added this to the Release 2020.07 milestone Jun 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: pm Area: (Low) power management CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants