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 timer should let deep sleep happen #11522

Merged
merged 2 commits into from Sep 19, 2019

Conversation

@LMESTM
Copy link
Contributor

commented Sep 19, 2019

Description

Pull request type

[ x] Fix
[ ] Refactor
[ ] Target update
[ ] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change

Reviewers

@kjbracey-arm @0xc0170 @jeromecoutant

Release Notes

When next SysTimer wake-up is scheduler far enough, always consider
that deep sleep may be entered and program an early wake-up.

So that even if deep sleep is only allowed some time later, it can be
entered. If not doing this, then the deep sleep would be prevented by
SysTimer itself and may not be entered at all.

This has been proved to happen in a simple blinly example.

Proposed fixes to #11509

Has been tested with simple blinky example, but should be reviewed and tested by mbed-os core team.

@LMESTM LMESTM force-pushed the LMESTM:SysTtimer_Should_Let_DeepSleep_Happen branch 2 times, most recently from 29e44c4 to f8a8845 Sep 19, 2019
When next SysTimer wake-up is scheduler far enough, always consider
that deep sleep may be entered and program an early wake-up.

So that even if deep sleep is only allowed some time later, it can be
entered. If not doing this, then the deep sleep would be prevented by
SysTimer itself and may not be entered at all.

This has been proved to happen in a simple blinly example.
@LMESTM LMESTM force-pushed the LMESTM:SysTtimer_Should_Let_DeepSleep_Happen branch from f8a8845 to cd3105b Sep 19, 2019
@ciarmcom ciarmcom requested review from 0xc0170, kjbracey-arm and ARMmbed/mbed-os-maintainers Sep 19, 2019
@ciarmcom

This comment has been minimized.

Copy link
Member

commented Sep 19, 2019

@ciarmcom ciarmcom requested a review from ARMmbed/mbed-os-core Sep 19, 2019
@kjbracey-arm

This comment has been minimized.

Copy link
Contributor

commented Sep 19, 2019

Thanks for the investigation.

I've been studying this issue/PR, and considering alternatives. This does work, but does mean we always get double wake-up on boards with latency, even when not deep sleeping.

That's not the end of the world, so maybe it's the way to go, but I was trying a different approach, which was to prepare the timer on the basis of the current sleep_manager_can_deep_sleep, and then recheck just before sleeping. If deep sleep had been unlocked while preparing, and we were set for non-early wake, go back to readjust.

That doesn't work, because changing the timer re-locks the sleep. We get stuck in an endless timer reprogramming loop.

I'm not very happy with the HAL timer drivers locking deep sleep. Seems like a layer violation, and I'm worried it may cause more upsets. Is there any alternative? My assumption was always that the unlock race there would be rare to non-existent, not something that would occur regularly due to a HAL timer interaction with the lock.

So. You need the programming to be complete before entering your deep sleep mode? And that completion generates an interrupt (in which you currently unlock deep sleep?) Could you just have a HAL-local lock - if the "lp timer programming progress" flag is set, hal_deepsleep calls hal_sleep instead? (Similar to what you do with serial, but you can do shallow sleep rather than returning, because you know there will be an interrupt).

@LMESTM

This comment has been minimized.

Copy link
Contributor Author

commented Sep 19, 2019

@kjbracey-arm

Thanks for the investigation.

I've been studying this issue/PR, and considering alternatives. This does work, but does mean we always get double wake-up on boards with latency, even when not deep sleeping.

During the extra latency we would be in sleep / WFI which is the same anyway we would have when we can't deep sleep .. so power loss would be insignificant

So. You need the programming to be complete before entering your deep sleep mode? And that completion generates an interrupt (in which you currently unlock deep sleep?) Could you just have a HAL-local lock - if the "lp timer programming progress" flag is set, hal_deepsleep calls hal_sleep instead? (Similar to what you do with serial, but you can do shallow sleep rather than returning, because you know there will be an interrupt).

If doing this, we would actually make all cpu stats wrong. We would tell the system we're in deep sleep when we're not and that could be pretty confusing in the end.

If we return from deep sleep like we do for serial (something we should get rid of one day ...) then we will continuously be executing code in and out of deep sleep (mbed power management layer checks deep sleep is allower, enters deep sleep, u we actually exit immediately, then again .... ) - here the power less can be significant because CPU is active instead of WFI ...

@kjbracey-arm

This comment has been minimized.

Copy link
Contributor

commented Sep 19, 2019

This does work, but does mean we always get double wake-up on boards with latency, even when not deep sleeping.

During the extra latency we would be in sleep / WFI which is the same anyway we would have when we can't deep sleep .. so power loss would be insignificant

But you're potentially doubling the run-time - a simple LED blink is going to wake twice for every transition. Isn't doubling run-time significant?

Admittedly it's an increase for the intermediate not-deep-sleeping power state, not the actually minimum deep sleep, so maybe it's not that significant.

(Feedback like this is welcome, as I've done quite a lot of work on the sleep stuff, but very much from a theoretical angle - I don't have an intuitive feel for the power/time ratios).

If doing this, we would actually make all cpu stats wrong. We would tell the system we're in deep sleep when we're not and that could be pretty confusing in the end.

True. That's a good enough argument to dissuade me.

I've also noticed another issue in that when going around again for sleep_prepare, the timer reprogramming is a no-op if already set, and that doesn't give it a chance to reconsider sleep_manager_can_deep_sleep for adjustment anyway.

So this patch wins for simplicity. I'll review.

if (MBED_CONF_TARGET_DEEP_SLEEP_LATENCY > 0 &&
ticks_to_sleep > MBED_CONF_TARGET_DEEP_SLEEP_LATENCY &&
sleep_manager_can_deep_sleep()) {
ticks_to_sleep > MBED_CONF_TARGET_DEEP_SLEEP_LATENCY) {

This comment has been minimized.

Copy link
@kjbracey-arm

kjbracey-arm Sep 19, 2019

Contributor

I think you can instead replace the sleep_manager_can_deep_sleep() with !_deep_sleep_locked. If we know we've taken the lock because we're using us_ticker, no need to do the early wake.

The comments need a bit of rework I think. The "race" no longer exists. Something like:

/* Consider whether we will need early or precise wake-up */

/* If there is deep sleep latency, but we still have enough time,
 * and we haven't blocked deep sleep ourselves, allow for that latency by requesting early wake-up.
 * Actual sleep may or may not be deep, depending on other actors.
 */

/* Otherwise, set up to wake at the precise time.
 * If there is a deep sleep latency, ensure that we're holding the lock so the sleep
 * is shallow. (If there is no deep sleep latency, we're fine with it being deep).
 */

This comment has been minimized.

Copy link
@LMESTM

LMESTM Sep 19, 2019

Author Contributor

PR updated with your changes (and your github name added for reference)

@LMESTM

This comment has been minimized.

Copy link
Contributor Author

commented Sep 19, 2019

@0xc0170 I would really like this PR to be considered for 15.4 release - unless 15.4 is not deployed to the online compiler, mbed studio and so on ... I'd like to avoid having a release where deep sleep is not entered at all after all the efforts we've been through before summer time

@LMESTM

This comment has been minimized.

Copy link
Contributor Author

commented Sep 19, 2019

Admittedly it's an increase for the intermediate not-deep-sleeping power state, not the actually minimum deep sleep, so maybe it's not that significant.
(Feedback like this is welcome, as I've done quite a lot of work on the sleep stuff, but very much from a theoretical angle - I don't have an intuitive feel for the power/time ratios).

I'll run a power measurement on this concrete example and share the results here so that we have concrete data to discuss with.

@0xc0170 0xc0170 added needs: CI and removed needs: work labels Sep 19, 2019
@0xc0170

This comment has been minimized.

Copy link
Member

commented Sep 19, 2019

CI started

@adbridge

This comment has been minimized.

Copy link
Contributor

commented Sep 19, 2019

If it passes CI in the next couple of hours then we can try and get this into RC3

@0xc0170 0xc0170 changed the title Sys ttimer should let deep sleep happen Sys timer should let deep sleep happen Sep 19, 2019
@mbed-ci

This comment has been minimized.

Copy link

commented Sep 19, 2019

Test run: FAILED

Summary: 1 of 4 test jobs failed
Build number : 1
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_build-ARM
@0xc0170

This comment has been minimized.

Copy link
Member

commented Sep 19, 2019

I'll restart tests, @LMESTM can you fix astyle first?

Suggested-by: @kjbracey-arm

Replace the sleep_manager_can_deep_sleep() with !_deep_sleep_locked.
Indeed, if we know we've taken the lock because we're using us_ticker,
no need to do the early wake.

Updated comments accordingly.
@LMESTM LMESTM force-pushed the LMESTM:SysTtimer_Should_Let_DeepSleep_Happen branch from ce647bd to 9858b16 Sep 19, 2019
@LMESTM

This comment has been minimized.

Copy link
Contributor Author

commented Sep 19, 2019

I'll restart tests, @LMESTM can you fix astyle first?

Sorry about that - this is now fixed !

@0xc0170

This comment has been minimized.

Copy link
Member

commented Sep 19, 2019

CI started

@LMESTM

This comment has been minimized.

Copy link
Contributor Author

commented Sep 19, 2019

@kjbracey-arm some data measured today on NUCLEO_L073RZ using mbed-os Blinky example

Deep sleep current ~6µA
Sleep current ~3500µA
Active current ~8000µA

Then average MCU power (well average current here) can be considered as :
( (Deep Sleep current * %deep_sleep) + (sleep current * %sleep) + (run current * %run) ) / 100

the fewer wake-ups the better. But the extra wake-up cost when Sleep mode only is possible seems to be low ... the big saving comes when you start entering Deep Sleep

@korjaa

This comment has been minimized.

Copy link
Contributor

commented Sep 19, 2019

I checked out this branch from mbed-os and ran a minimal blinky example.

#include "mbed.h"
#include "mbed_stats.h"

static DigitalOut led(LED1);

int main() {
   while (1) {
       led = 1;
       led = 0;
       wait_ms(2000);
   }
}

My current measurement setup is very unsophisticated and the absolute values are most likely off a lot. I wouldn't trust them too much. However, this fix does seem to work for me also and the device does enter deep sleep.
image

Also I'm unable to spot the double "shallow sleep" with this setup. I'm assuming it's so short compared to the deep sleep?

@mbed-ci

This comment has been minimized.

Copy link

commented Sep 19, 2019

Test run: SUCCESS

Summary: 11 of 11 test jobs passed
Build number : 2
Build artifacts

@adbridge adbridge added ready for merge and removed needs: CI labels Sep 19, 2019
@adbridge adbridge merged commit d0b5ba6 into ARMmbed:master Sep 19, 2019
25 checks passed
25 checks passed
continuous-integration/jenkins/pr-head This commit looks good
Details
jenkins-ci/build-ARM Success
Details
jenkins-ci/build-GCC_ARM Success
Details
jenkins-ci/build-IAR Success
Details
jenkins-ci/cloud-client-test Success
Details
jenkins-ci/dynamic-memory-usage RTOS ROM(+0 bytes) RAM(-72 bytes)
Details
jenkins-ci/exporter Success
Details
jenkins-ci/greentea-test Success
Details
jenkins-ci/mbed2-build-ARM Success
Details
jenkins-ci/mbed2-build-GCC_ARM Success
Details
jenkins-ci/mbed2-build-IAR Success
Details
jenkins-ci/unittests Success
Details
travis-ci/astyle Success!
Details
travis-ci/docs Success!
Details
travis-ci/doxy-spellcheck Success!
Details
travis-ci/events Success! Runtime is 8639 cycles.
Details
travis-ci/gitattributestest Success!
Details
travis-ci/include_check Success!
Details
travis-ci/licence_check Success!
Details
travis-ci/littlefs Success! Code size is 8464B.
Details
travis-ci/psa-autogen Success!
Details
travis-ci/tools-py2.7 Success!
Details
travis-ci/tools-py3.5 Success!
Details
travis-ci/tools-py3.6 Success!
Details
travis-ci/tools-py3.7 Success!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.