Sleep: add sleep manager API (feature-hal-spec branch) #4882

0xc0170 · 2017-08-09T14:20:51Z

Note: this goes to the feature branch feature-hal-spec (shall go to the next minor release at minimum). I rebased it on top of the latest master. We can then use this for the other work, and build on top of it or update it according to feedback.

Sleep manager provides API to lock/unlock deepsleep. This API allows a user to
control deep sleep.

This API should be done via atomic operations (to be IRQ/thread safe).

Add wake-up times for sleep and deep sleep. Sleep should wake up within 10 us,
deep sleep within 5 milliseconds (sufficient time to restore clock if required).

Deprecates deepsleep that is being replaced by auto function from sleep manager.

There are tests to test this changeset, currently private, will be shared later.

cc @c1728p9 @geky @pan- @bulislaw

amq · 2017-08-10T11:07:10Z

How does this relate to the automatic sleep mode selection on some targets, such as EFM32?

0xc0170 · 2017-08-10T11:35:57Z

How does this relate to the automatic sleep mode selection on some targets, such as EFM32?

Still can be used. We are going to provide a locking deepsleep in drivers (with attaching interrupts like SPI, I2C, etc). This will come as another PR, more details will follow.
Some changes might be needed in targets sleep/deepsleep implementations - in this case, sleep should not go to em2 or lower (if high freq gets disabled for instance, or not any interrupt can wake up a target). Then deepsleep could do em2 or lower (it's up to a target how to do it, some have it implemented in hw, some might need some sw logic to figure out which deep sleep mode they can enter).

Please review sleep/deepsleep hal docs if it is clear what each one should/not should do.

0xc0170 · 2017-08-11T10:55:19Z

Rebased, fixed an error if sleep in the device_has is not defined

0xc0170 · 2017-08-14T06:37:19Z

Please review (note that this goes to the feature branch thus not feature complete yet)

@bcostm @adustm @LMESTM @jeromecoutant @jeremybrodt @stevew817 @akselsm @mmahadevan108 @TomoYamanaka @nvlsianpu @anangl @andreaslarssonublox @RobMeades @Archcady @ccli8 @khj098765 @jacobjohnson-ON

RobMeades

Looks good to me.

mmahadevan108 · 2017-08-14T16:18:13Z

hal/mbed_sleep_manager.c

+{
+    core_util_critical_section_enter();
+// debug profile should keep debuggers attached, no deep sleep allowed
+#ifndef NDEBUG


Should this change to #ifdef DEBUG

NDEBUG is standard:
http://en.cppreference.com/w/c/error/assert

Shouldn't the increment happen inside the critical section? So that nothing happens (another increment) between the lines of 34 and 35?

ccli8

The sleep manager strictly asks lock/unlock in pair. But in practice, it is prone to invoke lock or unlock multiple times and redundantly.

0xc0170 · 2017-08-15T10:47:51Z

The sleep manager strictly asks lock/unlock in pair. But in practice, it is prone to invoke lock or unlock multiple times and redundantly.

@ccli8 What would you suggest?

ccli8 · 2017-08-15T11:22:14Z

What would you suggest?

@0xc0170 I suggest lock bitmap rather than lock counter to maintain lock or not.

uint32_t sleep_manager_acquire_deep_sleep_lock_id(void);
void sleep_manager_release_deep_sleep_lock_id(uint32_t lock_id);
void sleep_manager_lock_deep_sleep(uint32_t lock_id);
void sleep_manager_unlock_deep_sleep(uint32_t lock_id);

For a HAL to lock deep sleep, it first invokes sleep_manager_acquire_deep_sleep_lock_id to get a lock ID, which corresponds to one bit of the lock bitmap. Then it can call lock/unlock not in pair to change the bit value corresponding to the got lock ID. Redundant calls to lock/unlock won't cause trouble.

0xc0170 · 2017-08-15T12:54:33Z

tIsn't this overkill and makes it harder to use? Look at the general use case - you lock and unlock in specific cases (mostly when high frequency clocks are involved) for some period of time (start-end of the transfer for instance).

For instance for SPI, it would be part of transfer() to lock the deep sleep mode. In case of any event, it would unlock it. I assume with this API it would rather require lock id in the ctor and release it in dtor. Would 2 different SPI objects require different id or use the same?

What others think?

ccli8 · 2017-08-16T05:29:17Z

For instance for SPI, it would be part of transfer() to lock the deep sleep mode. In case of any event, it would unlock it. I assume with this API it would rather require lock id in the ctor and release it in dtor. Would 2 different SPI objects require different id or use the same?

@0xc0170 2 different SPI objects require 2 different id for this idea. This is to limit scope of redundant lock/unlock calls.

Other question. How does the sleep manager co-work with SysTick, which RTOS relies on and us_ticker, which Ticker, Timer, Timout, etc. rely on. As system enters deep sleep, SysTick and us_ticker which both rely on high-freq clock will also stop. How to handle registered interrupts and maintain tick count for these tickers?

RobMeades · 2017-08-16T08:31:33Z

Personally I would prefer the simpler approach, lock()/unlock(), similar to a mutex, an API everyone will understand, even if it carries an element of redundancy. The more complex API requires the user to pass around the ID value within their code and I fear it will be used incorrectly.

bulislaw

I agree, simpler lock/unlock should be enough for this case, it fits mbed OS mission more and it addresses most of cases anyway.

bulislaw · 2017-08-16T09:04:10Z

hal/sleep_api.h

 * system clock to the core is stopped until a reset or an interrupt occurs. This eliminates
 * dynamic power used by the processor, memory systems and buses. The processor, peripheral and
 * memory state are maintained, and the peripherals continue to work and can generate interrupts.
 *
 * The processor can be woken up by any internal peripheral interrupt or external pin interrupt.
 *
+ * The wake-up time should be less than 10 us.


'should' in a language of specs means it's optional (recommended). Let's use shall or must here. Could we isolate and make it point on it's own requirements wake up sources. We should also add that implementation of sleep is HW dependent as long as the requirements are met.

'should' in a language of specs means it's optional (recommended)

That was my intention, must is a strong in this case. @AnotherButler?

Could we isolate and make it point on it's own requirements wake up sources.

I don't follow this one, please elaborate.

Well we are writing a specification, if we give a recommendation we can't rely on the implementation's behavior.

Could we isolate and make it point on it's own requirements wake up sources.
I don't follow this one, please elaborate.

Ignore that, it is clearly mentioned after the main description body, I missed it somehow.

Is this something we're telling the developer to set, or is this something we set? "Should" implies it may not be. In general, we don't say things like "The code should compile." Instead we say, "The code compiles" because we don't want to imply our code doesn't work.

I would say we are trying to describe a hard requirement, which will be asserted with a test.

@AnotherButler we changed it to shall, is that correct as it is requirement ?

bulislaw · 2017-08-16T09:06:49Z

hal/sleep_api.h

 * system clock to the core is stopped until a reset or an interrupt occurs. This eliminates
 * dynamic power used by the processor, memory systems and buses. The processor, peripheral and
 * memory state are maintained, and the peripherals continue to work and can generate interrupts.
 *
 * The processor can be woken up by any internal peripheral interrupt or external pin interrupt.
 *
+ * The wake-up time should be less than 10 us.
+ * 
 * @note


Do we still want that here? Or should it be moved to the sleep manager?

I would like to remove this note as it's specific to one target, and should be documented elsewhere. Where shall we move it (or better question - where we would expect to have documented sleep modes for a target - is it all in the code, that this note would be moved to lpc targets that have localfilesystem, same for what modes, or developer target page) ?

Yeah I would say developer target page is the right place for that.

bulislaw · 2017-08-16T09:08:01Z

hal/sleep_api.h

 *
+ * The wake-up time should be less than 5 ms.


should -> must/shall

bulislaw · 2017-08-16T09:09:35Z

platform/mbed_sleep.h

@@ -20,24 +20,62 @@
 #define MBED_SLEEP_H

 #include "sleep_api.h"
+#include "mbed_toolchain.h"
+#include <stdbool.h>

 #ifdef __cplusplus
 extern "C" {
 #endif



Could we get some general description of the model and intended use here.

bulislaw · 2017-08-16T09:11:33Z

platform/mbed_sleep.h


 #ifdef __cplusplus
 extern "C" {
 #endif

+/** Lock the deep sleep mode


I wouldn't know why and when I should use it from this description.

bulislaw · 2017-08-16T09:18:39Z

platform/mbed_sleep.h

 __INLINE static void deepsleep(void)
 {
 #if !(defined(FEATURE_UVISOR) && defined(TARGET_UVISOR_SUPPORTED))
-#ifndef MBED_DEBUG
 #if DEVICE_SLEEP
    hal_deepsleep();


shouldn't that be implemented using sleep manager as well?

I left it as it is, it's deprecated, no changes for this anymore, use sleep to get sleep manager in. deepsleep still invokes deepsleep as it was (no change)

But it goes around our lock/unlock logic so it can cause troubles. I would say the API should stay the same (just marked as deprecated), but which exactly sleep is taken should be decided by the new logic.

bulislaw · 2017-08-16T09:23:25Z

hal/mbed_sleep_manager.c

+{
+    core_util_critical_section_enter();
+// debug profile should keep debuggers attached, no deep sleep allowed
+#ifndef NDEBUG


why are we changing the behavior again? it's confusing if we keep doing every second mbed OS release what do you want to achieve? Using NDEBUG we won't sleep in both debug and develop I don't think it's something we want. I would say we should go to sleep in both develop and release. We should be using #ifndef MBED_DEBUG as before.

yes, we should use MBED_DEBUG . will fix

bulislaw · 2017-08-16T09:28:29Z

hal/mbed_sleep_manager.c

+{
+    core_util_critical_section_enter();
+// debug profile should keep debuggers attached, no deep sleep allowed
+#ifndef NDEBUG


Shouldn't the increment happen inside the critical section? So that nothing happens (another increment) between the lines of 34 and 35?

bulislaw · 2017-08-16T09:28:51Z

hal/mbed_sleep_manager.c

+        error("Deep sleep lock would underflow (< 0)");
+    }
+    core_util_critical_section_exit();
+    core_util_atomic_decr_u16(&deep_sleep_lock, 1);


Switch the lines 45 and 46 around?

I wanted to use only atomic operations, that critical section there is for the check to not underflow. I could move it, and just use regular decrement. As it is, for that architecture that has atomic operations, this would decrease critical section. Open for suggestions

Yeah I know, but in this case someone else can increase/decrease after your check but before you actually do anything with it, leading to under/over flow.

there is a weak spot, I'll move it down, and might not to use even that atomic operations then

bulislaw · 2017-08-16T09:34:08Z

hal/mbed_sleep_manager.c

+
+void sleep_manager_sleep_auto(void)
+{
+    core_util_critical_section_enter();


Are you sure it's going to work as intended? If we go to a shallow sleep and the interrupts are blocked we won't be wakening up.

It will that is a feature of cortex.

0xc0170 · 2017-08-16T09:56:11Z

Other question. How does the sleep manager co-work with SysTick, which RTOS relies on and us_ticker, which Ticker, Timer, Timout, etc. rely on. As system enters deep sleep, SysTick and us_ticker which both rely on high-freq clock will also stop. How to handle registered interrupts and maintain tick count for these tickers?

@ccli8 This will come as another feature - tickless (on top of sleep manager, low power ticker updates). RTOS relies on mostly Systick (in some cases other ticker if Systick is not available). RTX5 provides tickless support, and we connect this to low power ticker to keep the ticks while we sleep (deepsleep has a requirement to have lp ticker running).

0xc0170 · 2017-08-16T10:25:52Z

Rebased after the review, all should responded

ccli8 · 2017-08-16T10:43:02Z

This will come as another feature - tickless (on top of sleep manager, low power ticker updates). RTOS relies on mostly Systick (in some cases other ticker if Systick is not available). RTX5 provides tickless support, and we connect this to low power ticker to keep the ticks while we sleep (deepsleep has a requirement to have lp ticker running).

@0xc0170 Does tickless also support us_ticker? Besides, what's the schedule to release tickless?

0xc0170 · 2017-08-16T11:33:33Z

@0xc0170 Does tickless also support us_ticker?

us ticker - no, it depends on the high frequency clock to provide very accurate ticks (1us) in the most cases, therefore for us ticker to keep the time during the deep sleep mode is not guaranteed. Compare to lp ticker, that one should guarantee the functionality across any sleep mode (sleep and deep sleep). This feature will be part of another patch that is coming, more details will follow.

Besides, what's the schedule to release tickless?

It shall for upcoming minor release of Mbed OS.

I rebased again, removed the notes from sleep API that are related to the original mbed board. The target page for lpc1768 was updated (if anyone finds better place or any edit, please do).

LMESTM

Few comments / questions, but I agree with the overall change

LMESTM · 2017-08-16T13:55:58Z

hal/mbed_sleep_manager.c

+
+#if DEVICE_SLEEP
+
+// deep sleep locking counter. A target is allowed to sleep if counter != 0


isn't it "allowed to sleep is counter ==0" (not allowed if counter != 0)

LMESTM · 2017-08-16T14:00:36Z

hal/sleep_api.h

- *  The mbed interface semihosting is disconnected as part of going to sleep, and can not be restored.
- * Flash re-programming and the USB serial port will remain active, but the mbed program will no longer be
- * able to access the LocalFileSystem
+ * The wake-up time shall be less than 10 us.


Where does 10us limit comes from ? Is is a realistic value for exiting WFI whatever the clock speed ?

@LMESTM Do you have any use case that would not fit ? From the analysis , the latency introduced by sleep is around 2us (on top of that would be: going to sleep + interrupt latency)

@0xc0170 - I don't have a requirement. I'm thinking that people interested in saving power may lower the core and bus frequencies, which will have a direct impact on this timing (2us will easilly become 10us or more). Anyway I feel that 10us is more an order of range rather than a strict requirement - the actual limit actually depends on low level HW interfaces and devices we communicate with, so I don't have an absolute value for this. If you'd want to be more generic, then the drivers would pass the wake-up time requirement to the sleep manager as a parameter, then the more strict requirement of all drivers would be passed to MCU to decide how deep it can sleep ... But I'm fine with the generic approach and the "10us order of range" ....

For shallow sleep I'd like to keep a strict limit, I'm more open about the deep sleep, but usually if something is not defined (or explicitly mention to be not defined) it tend to go all over the place. @c1728p9 @0xc0170 do we actually care about the deep sleep wake up time?

I'm thinking that people interested in saving power may lower the core and bus frequencies

@LMESTM I would expect this feature for the deepsleep, the timeperiods are longer, more power savings could be introduced. Considering that sleep is default one, and a target goes to this mode very often - when there is nothing to do - idle loop (latency introduced by lowering clocks might not be worth).

the drivers would pass the wake-up time requirement to the sleep manager as a parameter,

I had a prototype with sleep(timestamp_t sleep_time) . The argument sleep_time was a hint for a target to know for how long it would sleep and based on that would choose the sleep mode. We simplified it to as it is now, having it without any hint (no changes in API). A target just needs to wake-up within certain time that it is defined.
Having the upper limit for deepsleep defined is for an application and tests - this is the upper limit of deepsleep latency that an app developer should be aware of and account with.

This is the first proposal, open for feedback.

I am also more open about the defined times for deep sleep. From various manuals we looked at, the most common wake-up time from deep sleep to sleep with external crystal is around 3ms (1-2ms for clocks to reconfigure, it varies quite a lot - some has it just hunderds of microseconds, some up 1/2 ms).

Ok for me !

One note - idle loop can be reconfigured via attaching own. We are providing default implementation for tickless. Thus a user still can do further power savings (reconfiguring clocks for instance - this is currently not in the scope), same for having shut off mode (another mode that some users might want to further reduce power).

LMESTM · 2017-08-16T14:03:13Z

platform/mbed_sleep.h

+ */
+bool sleep_manager_can_deep_sleep(void);
+
+/** Enter auto selected sleep mode. It chooses the sleep or deeepslee modes based


typo: deeepslee => deepsleep

LMESTM · 2017-08-16T14:03:59Z

platform/mbed_sleep.h

- *  The mbed interface semihosting is disconnected as part of going to sleep, and can not be restored.
- * Flash re-programming and the USB serial port will remain active, but the mbed program will no longer be
- * able to access the LocalFileSystem
+ * The wake-up time shall be less than 5 us.


Previous comment mentions 10us vs 5us here

0xc0170 · 2017-08-17T08:03:20Z

@LMESTM thank you for the review, I updated the PR

Sleep manager provides API to lock/unlock deepsleep. This API allows a user to control deep sleep. This API should be done via atomic operations (to be IRQ/thread safe). Add wake-up times for sleep and deep sleep. Sleep should wake up within 10 us, deep sleep within 10 milliseconds (sufficient time to restore clock if required).

If a target do not support sleep (no DEVICE_SLEEP defined), we provide empty deep sleep locking.

0xc0170 · 2017-08-17T09:55:27Z

retest uvisor

0xc0170 · 2017-08-17T14:13:58Z

retest uvisor

0xc0170 · 2017-08-18T08:00:53Z

@tommikas Can you help to identify the problem with jenkins CI? I do not see any error related to this patch?

0xc0170 · 2017-08-18T08:06:52Z

/morph test

mbed-bot · 2017-08-18T09:40:37Z

Result: FAILURE

Your command has finished executing! Here's what you wrote!

/morph test

Output

mbed Build Number: 1052

Test failed!

tommikas · 2017-08-18T10:53:22Z

@0xc0170 For some reason the application is starting but it's not responding to commands sent in. As a result all the tests fail. I'll see if I could find time to have a better look at it.

tommikas · 2017-08-18T11:36:22Z

Looks like it's breaking serial RX somehow. When sending something the RX interrupt isn't firing most of the time, and when it does fire the data is corrupted.

0xc0170 · 2017-08-18T12:50:53Z

Looks like it's breaking serial RX somehow. When sending something the RX interrupt isn't firing most of the time, and when it does fire the data is corrupted.

Will look at it, and try to reproduce locally

0xc0170 · 2017-08-18T16:51:21Z

Looking at the above post I realized that without drivers support, thus should come all together.

Thanks for the reviews, I'll rework #4912 (that will replace this pull request, is it based on top of this one anyway). Continue discusion there please

0xc0170 added the needs: review label Aug 10, 2017

0xc0170 force-pushed the dev_sleep_manager branch from 86f3fad to 92e9859 Compare August 11, 2017 10:54

RobMeades approved these changes Aug 14, 2017

View reviewed changes

mmahadevan108 approved these changes Aug 14, 2017

View reviewed changes

ccli8 reviewed Aug 15, 2017

View reviewed changes

0xc0170 mentioned this pull request Aug 15, 2017

Add sleep manager API #4912

Merged

bulislaw reviewed Aug 16, 2017

View reviewed changes

0xc0170 force-pushed the dev_sleep_manager branch 6 times, most recently from 18c060b to 93d0d2c Compare August 16, 2017 10:25

0xc0170 force-pushed the dev_sleep_manager branch from 93d0d2c to b73df87 Compare August 16, 2017 11:24

0xc0170 mentioned this pull request Aug 16, 2017

Ticker issues with NRF52 platform, especially while using event queue call_every timer function. #4893

Closed

LMESTM approved these changes Aug 16, 2017

View reviewed changes

0xc0170 force-pushed the dev_sleep_manager branch from b73df87 to 5946d67 Compare August 16, 2017 14:12

0xc0170 added 2 commits August 17, 2017 09:12

Sleep: add deepsleep locks for !DEVICE_SLEEP

543660f

If a target do not support sleep (no DEVICE_SLEEP defined), we provide empty deep sleep locking.

0xc0170 force-pushed the dev_sleep_manager branch from 568c620 to 543660f Compare August 17, 2017 08:13

0xc0170 closed this Aug 18, 2017

0xc0170 removed the needs: review label Aug 18, 2017


		#if DEVICE_SLEEP

		// deep sleep locking counter. A target is allowed to sleep if counter != 0

Sleep: add sleep manager API (feature-hal-spec branch) #4882

Sleep: add sleep manager API (feature-hal-spec branch) #4882

Conversation

0xc0170 commented Aug 9, 2017

amq commented Aug 10, 2017

0xc0170 commented Aug 10, 2017 • edited

0xc0170 commented Aug 11, 2017

0xc0170 commented Aug 14, 2017 • edited

RobMeades left a comment

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

ccli8 left a comment

Choose a reason for hiding this comment

0xc0170 commented Aug 15, 2017

ccli8 commented Aug 15, 2017

0xc0170 commented Aug 15, 2017 • edited

ccli8 commented Aug 16, 2017

RobMeades commented Aug 16, 2017

bulislaw left a comment

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xc0170 Aug 16, 2017 • edited

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xc0170 Aug 16, 2017 • edited

Choose a reason for hiding this comment

0xc0170 commented Aug 16, 2017 • edited

0xc0170 commented Aug 16, 2017

ccli8 commented Aug 16, 2017

0xc0170 commented Aug 16, 2017

LMESTM left a comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xc0170 Aug 17, 2017 • edited

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

Choose a reason for hiding this comment

0xc0170 commented Aug 17, 2017

0xc0170 commented Aug 17, 2017

0xc0170 commented Aug 17, 2017

0xc0170 commented Aug 18, 2017

0xc0170 commented Aug 18, 2017

mbed-bot commented Aug 18, 2017

Output

tommikas commented Aug 18, 2017

tommikas commented Aug 18, 2017

0xc0170 commented Aug 18, 2017

0xc0170 commented Aug 18, 2017

0xc0170 commented Aug 10, 2017 •

edited

0xc0170 commented Aug 14, 2017 •

edited

0xc0170 commented Aug 15, 2017 •

edited

0xc0170 Aug 16, 2017 •

edited

0xc0170 Aug 16, 2017 •

edited

0xc0170 commented Aug 16, 2017 •

edited

0xc0170 Aug 17, 2017 •

edited