Skip to content

Improve RTOS behavior with deep sleep #8223

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Oct 27, 2018
Merged

Improve RTOS behavior with deep sleep #8223

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions rtos/TARGET_CORTEX/mbed_rtx_idle.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,13 @@ uint32_t OS_Tick_GetInterval (void) {
static void default_idle_hook(void)
{
uint32_t ticks_to_sleep = osKernelSuspend();
const bool block_deep_sleep = ticks_to_sleep <= MBED_CONF_TARGET_DEEP_SLEEP_LATENCY;

if (block_deep_sleep) {
sleep_manager_lock_deep_sleep();
} else {
ticks_to_sleep -= MBED_CONF_TARGET_DEEP_SLEEP_LATENCY;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

}
os_timer->suspend(ticks_to_sleep);

bool event_pending = false;
Expand All @@ -106,6 +113,11 @@ static void default_idle_hook(void)
// Ensure interrupts get a chance to fire
__ISB();
}

if (block_deep_sleep) {
sleep_manager_unlock_deep_sleep();
}

osKernelResume(os_timer->resume());
}

Expand Down
4 changes: 4 additions & 0 deletions targets/targets.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

"value": 0
},
"boot-stack-size": {
"help": "Define the boot stack size in bytes. This value must be a multiple of 8",
"value": "0x1000"
Expand Down