Improve RTOS behavior with deep sleep #8223

c1728p9 · 2018-09-21T22:09:47Z

Description

Only deep sleep when the wakeup time is more than 10ms in the future. This ensures that RTOS events are handled at the correct time. Note - when deep sleep is allow interrupt latency may still be as high as 10ms.

Pull request type

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

c1728p9 · 2018-09-21T22:10:17Z

This may negatively increase power consumption so I'm marking this as do not merge until further investigation can be done

jeromecoutant · 2018-09-24T08:26:53Z

Hi

Proposition in targets.json file:

"Target": {
    "config": {
        "deep-sleep-latency": {
            "help": "time in ms required to go to and wake up from deep sleep (max 10)",
            "value": 0
        },

Then, it is up to each target to define it ?

kjbracey · 2018-09-24T08:29:03Z

Seems like something like this is needed, given the HAL sleep spec. Implementation seems reasonable to me.

Next possible refinement would be to allow a target to specify its number.

jeromecoutant

Verified with tests
(note that this current patch version doesn't compile :-))

c1728p9 · 2018-10-03T22:02:55Z

Rebased on to master and added config value with a default of 0. Removing "do not merge" since this PR should not effect power consumption in the default configuration.

kjbracey · 2018-10-04T06:49:44Z

GitHub description needs update from commit message

jeromecoutant · 2018-10-09T15:36:59Z

@c1728p9 it seems there is some conflict
@ARMmbed/mbed-os-maintainers can you start CI ?
Thx

cmonr · 2018-10-09T15:52:34Z

@jeromecoutant It doesn't makes sense to start CI if the PR needs to be rebased, which will require another run of CI anyways.

kjbracey · 2018-10-10T11:16:18Z

rtos/TARGET_CORTEX/mbed_rtx_idle.cpp

+    if (block_deep_sleep) {
+        sleep_manager_lock_deep_sleep();
+    } else {
+        ticks_to_sleep -= MBED_CONF_TARGET_DEEP_SLEEP_LATENCY;


Technically this subtraction should be just "wake latency".

If sleep time was 3ms and wake time was 5ms, you'd want

7ms: normal sleep, 7ms timer
8ms: normal sleep, 8ms timer
9ms: deep sleep, 4ms timer
10ms: deep sleep, 5ms timer

I think you might want separate "sleep" and "wake" numbers to do that.

But then, is 1ms in deep sleep better than 9ms in normal sleep? Scope for another fudge factor... Maybe you'd want to not go deep until "actual deep sleep time >= 50% of desired total" or something.

Hi @kjbracey-arm the intent here was to reduce typical latency by using the requirement defined in the sleep specification - https://github.com/ARMmbed/mbed-os/blob/master/hal/sleep_api.h#L42

This is an optimization for latency not for power savings which is why I didn't add additional logic to determine if it would be optimal for power consumption to go into deep sleep. This could definitely be done, but is outside the scope of this PR.

I think you might want separate "sleep" and "wake" numbers to do that.

Have you seen devices which take a large amount of time to enter deep sleep? I'm use to a wakeup delay since crystals and plls need time to stabalize and lock, but I don't think I have seen a case where the time to enter sleep was a concern.

Okay, I think that answers my main point - I was thinking sleep and wake time might be comparable, which but if it's typically almost all wake, this is fine.

The power saving thing is something else, indeed.

The 10ms latency is something I think we'll have to be occasionally conscious of; I'd not really registered the implications. There may be external devices that keep working perfectly fine in deep sleep, except that they wouldn't cope with 10ms IRQ response latency from the CPU, so drivers may need to hold the lock purely for that reason.

jeromecoutant · 2018-10-16T16:00:38Z

bump...

cmonr · 2018-10-17T14:09:32Z

@c1728p9 Could you address @kjbracey-arm's comments, and rebase so that we can get CI going?

mattbrown015 · 2018-10-19T10:42:50Z

targets/targets.json

@@ -20,6 +20,10 @@
            "network-default-interface-type": {
                "help": "Default network interface type. Typical options: null, ETHERNET, WIFI, CELLULAR, MESH",
                "value": null
+            },
+            "deep-sleep-latency": {
+	            "help": "Time in ms required to go to and wake up from deep sleep (max 10)",


I know it's a nitpick but I think you've got some tabs in this change. The only 2 tabs in the file.

Only deep sleep when the wakeup time is more than MBED_CONF_TARGET_DEEP_SLEEP_LATENCY ms in the future. This ensures that RTOS events are handled at the correct time. Note - when deep sleep is allow interrupt latency may still be as high as 10ms.

c1728p9 · 2018-10-24T20:03:58Z

Fixed witespace and rebased to master

cmonr · 2018-10-25T01:58:08Z

@mattbrown015 @LMESTM Please feel free to take another look at this PR.

/morph build

mbed-ci · 2018-10-25T02:50:57Z

Build : SUCCESS

Build number : 3456
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/8223/

Triggering tests

/morph test
/morph export-build
/morph mbed2-build

mbed-ci · 2018-10-25T03:46:53Z

Exporter Build : SUCCESS

Build number : 3077
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/exporter/8223/

mbed-ci · 2018-10-25T04:21:34Z

Test : SUCCESS

Build number : 3247
Test logs :http://mbed-os-logs.s3-website-us-west-1.amazonaws.com/?prefix=logs/8223/3247

kjbracey · 2018-10-25T09:46:21Z

I still approve this, but would welcome @c1728p9's thoughts on my comments about separate wake/sleep times.

0xc0170 · 2018-10-25T10:09:44Z

targets/targets.json

@@ -21,6 +21,10 @@
                "help": "Default network interface type. Typical options: null, ETHERNET, WIFI, CELLULAR, MESH",
                "value": null
            },
+            "deep-sleep-latency": {
+                "help": "Time in ms required to go to and wake up from deep sleep (max 10)",


I assume MBED_CONF_TARGET_DEEP_SLEEP_LATENCY is the worst case scenario (the main depending factor here are the clocks that are running and need to be reenabled - can change in the runtime - in the most cases now it wont).

Max number is defined in the help text as 10, but not checked anywhere?

@0xc0170 the max recommended value is 10 because deep sleep shouldn't take longer than 10ms to wake - https://github.com/ARMmbed/mbed-os/blob/master/hal/sleep_api.h#L42

Setting it to a value higher than 10 will cause higher power consumption but won't negatively effect the system otherwise.

I understood, just checking if we set it to 11 , is this allowed and just impacts the system (no compile time error). I believe so.

0xc0170 · 2018-10-26T12:56:49Z

/morph build

mbed-ci · 2018-10-26T13:55:09Z

Build : SUCCESS

Build number : 3479
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/8223/

Triggering tests

/morph test
/morph export-build
/morph mbed2-build

mbed-ci · 2018-10-26T20:57:08Z

Test : SUCCESS

Build number : 3267
Test logs :http://mbed-os-logs.s3-website-us-west-1.amazonaws.com/?prefix=logs/8223/3267

mbed-ci · 2018-10-26T22:41:30Z

Exporter Build : SUCCESS

Build number : 3099
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/exporter/8223/

mattbrown015 · 2018-10-29T09:53:14Z

Hi @jeromecoutant,

Would you recommend changing this to something other than 0 for my STM32L4?

The STM32L4 datasheet says it will wake from Stop 2 mode in microseconds. In another thread you said you were setting this to 5 ms but is that because you were using a difference device that's not as good at waking up?

Or, is it because the wake up time in the datasheet doesn't include all the overhead required to really get mbed-os running again (enabling clocks, etc.)?

Thanks,
Matt

jeromecoutant · 2018-11-08T13:12:25Z

Hi

mbed-os/targets/TARGET_STM/sleep.c

Line 227 in 0fe6369

ForceClockOutofDeepSleep();

After STOP2 mode, we need to reconfigure PLL, this takes time.
As we got several issues around this, we added some clock initialisation code and some tempo, this takes also time...

About the value to apply, tests are on going on my side...
About power consumption argument, again, I am not sure... When you know that deepsleep takes time, and then power to update registers, maybe it is not so bad to stay in sleep for small delay ?

And finally, maybe this fix is no more useful after #8189 ...?
@c1728p9 what is your thinking about this PR and #8189?

jeromecoutant · 2018-11-08T13:12:51Z

@LMESTM

c1728p9 · 2018-11-08T22:56:37Z

And finally, maybe this fix is no more useful after #8189 ...?

@jeromecoutant this PR should still improve average wakeup latency even if it doesn't help with worst case latency.

kjbracey · 2018-11-29T13:31:30Z

rtos/TARGET_CORTEX/mbed_rtx_idle.cpp

+    if (block_deep_sleep) {
+        sleep_manager_lock_deep_sleep();
+    } else {
+        ticks_to_sleep -= MBED_CONF_TARGET_DEEP_SLEEP_LATENCY;


@c1728p9 - was discussing this with @sg-, and it occurred to me that this is being unduly pessimistic - you're subtracting the deep sleep latency from the wakeup time even if you're not going to be deep sleeping anyway, so in shallow operation there will be lots of unnecessary double firing.

May have to make the critical section wrapper bigger to synchronise a read of sleep_manager_can_deep_sleep. Maybe we could partially compensate by stripping the sleep_manager_lock_deep_sleep_internal() down - it doesn't need its critical section.

c1728p9 added the do not merge label Sep 21, 2018

c1728p9 requested review from jeromecoutant, LMESTM and kjbracey September 21, 2018 22:10

jeromecoutant mentioned this pull request Sep 25, 2018

STM32 LPTICKER : Fix tickless and LPTICKER_DELAY_TICKS #8242

Merged

jeromecoutant approved these changes Sep 25, 2018

View reviewed changes

jeromecoutant mentioned this pull request Oct 2, 2018

All STM should have tickless mode enabled #7153

Closed

c1728p9 force-pushed the deep_sleep_wakeup_fix branch from 3d1259a to 9a51d4e Compare October 3, 2018 22:00

c1728p9 removed the do not merge label Oct 3, 2018

kjbracey approved these changes Oct 4, 2018

View reviewed changes

cmonr added the needs: work label Oct 9, 2018

kjbracey reviewed Oct 10, 2018

View reviewed changes

jeromecoutant mentioned this pull request Oct 19, 2018

STM32: #8286 Causes Tickless RTC Mode App to Hang #8473

Closed

mattbrown015 reviewed Oct 19, 2018

View reviewed changes

Improve RTOS behavior with deep sleep

50316d0

Only deep sleep when the wakeup time is more than MBED_CONF_TARGET_DEEP_SLEEP_LATENCY ms in the future. This ensures that RTOS events are handled at the correct time. Note - when deep sleep is allow interrupt latency may still be as high as 10ms.

c1728p9 force-pushed the deep_sleep_wakeup_fix branch from 9a51d4e to 50316d0 Compare October 24, 2018 20:03

cmonr added needs: CI and removed needs: work labels Oct 25, 2018

cmonr added the release-version: 5.11.0-rc1 label Oct 25, 2018

0xc0170 reviewed Oct 25, 2018

View reviewed changes

cmonr added needs: review and removed needs: CI labels Oct 25, 2018

0xc0170 added needs: CI and removed needs: review labels Oct 26, 2018

cmonr merged commit 4798a91 into ARMmbed:master Oct 27, 2018

cmonr removed the needs: CI label Oct 27, 2018

jeromecoutant mentioned this pull request Nov 19, 2018

STM32 : Add default deep sleep latency of 3ms #8798

Merged

kjbracey reviewed Nov 29, 2018

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve RTOS behavior with deep sleep #8223

Improve RTOS behavior with deep sleep #8223

c1728p9 commented Sep 21, 2018

c1728p9 commented Sep 21, 2018

jeromecoutant commented Sep 24, 2018

kjbracey commented Sep 24, 2018

jeromecoutant left a comment

c1728p9 commented Oct 3, 2018

kjbracey commented Oct 4, 2018

jeromecoutant commented Oct 9, 2018

cmonr commented Oct 9, 2018

kjbracey Oct 10, 2018

kjbracey Oct 10, 2018

c1728p9 Oct 25, 2018

kjbracey Oct 26, 2018

jeromecoutant commented Oct 16, 2018

cmonr commented Oct 17, 2018

mattbrown015 Oct 19, 2018

c1728p9 commented Oct 24, 2018

cmonr commented Oct 25, 2018

mbed-ci commented Oct 25, 2018

mbed-ci commented Oct 25, 2018

mbed-ci commented Oct 25, 2018

kjbracey commented Oct 25, 2018

0xc0170 Oct 25, 2018

c1728p9 Oct 25, 2018

0xc0170 Oct 26, 2018

0xc0170 commented Oct 26, 2018

mbed-ci commented Oct 26, 2018

mbed-ci commented Oct 26, 2018

mbed-ci commented Oct 26, 2018

mattbrown015 commented Oct 29, 2018 •

edited

Loading

jeromecoutant commented Nov 8, 2018

jeromecoutant commented Nov 8, 2018

c1728p9 commented Nov 8, 2018

kjbracey Nov 29, 2018

Improve RTOS behavior with deep sleep #8223

Improve RTOS behavior with deep sleep #8223

Conversation

c1728p9 commented Sep 21, 2018

Description

Pull request type

c1728p9 commented Sep 21, 2018

jeromecoutant commented Sep 24, 2018

kjbracey commented Sep 24, 2018

jeromecoutant left a comment

Choose a reason for hiding this comment

c1728p9 commented Oct 3, 2018

kjbracey commented Oct 4, 2018

jeromecoutant commented Oct 9, 2018

cmonr commented Oct 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeromecoutant commented Oct 16, 2018

cmonr commented Oct 17, 2018

Choose a reason for hiding this comment

c1728p9 commented Oct 24, 2018

cmonr commented Oct 25, 2018

mbed-ci commented Oct 25, 2018

Triggering tests

mbed-ci commented Oct 25, 2018

mbed-ci commented Oct 25, 2018

kjbracey commented Oct 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xc0170 commented Oct 26, 2018

mbed-ci commented Oct 26, 2018

Triggering tests

mbed-ci commented Oct 26, 2018

mbed-ci commented Oct 26, 2018

mattbrown015 commented Oct 29, 2018 • edited Loading

jeromecoutant commented Nov 8, 2018

jeromecoutant commented Nov 8, 2018

c1728p9 commented Nov 8, 2018

Choose a reason for hiding this comment

mattbrown015 commented Oct 29, 2018 •

edited

Loading