Add sleep manager API #4912

0xc0170 · 2017-08-15T10:56:25Z

This goes to feature-hal-spec, based on the sleep manager work that is here #4882 . Review only the last commit (at the moment SHA 34f9f43). This goes to feature-hal-spec branch = work in progress

To move further with sleep, I am sending this early to get feedback.

The goal is to have deepsleep locks for those drivers that depend on high speed clocks (it can be that some drivers for a target might support deep sleep but this is not usual, plus speed is limited often).

@bulislaw @c1728p9 @pan-

c1728p9 · 2017-08-16T15:17:37Z

drivers/CAN.cpp

@@ -115,9 +115,11 @@ int CAN::filter(unsigned int id, unsigned int mask, CANFormat format, int handle
 void CAN::attach(Callback<void()> func, IrqType type) {


Attach could also be called to replace a callback of the same type. If this is the case then deep sleep shouldn't be locked a second time.

c1728p9 · 2017-08-16T15:19:52Z

drivers/SerialBase.cpp

@@ -73,9 +76,11 @@ void SerialBase::attach(Callback<void()> func, IrqType type) {
    // Disable interrupts when attaching interrupt handler
    core_util_critical_section_enter();
    if (func) {
+        sleep_manager_lock_deep_sleep();


Attach could also be called to replace a callback of the same type. If this is the case then deep sleep shouldn't be locked a second time.

c1728p9 · 2017-08-16T15:24:38Z

drivers/Ticker.cpp

@@ -26,6 +26,7 @@ void Ticker::detach() {
    core_util_critical_section_enter();
    remove();
    _function = 0;
+    sleep_manager_unlock_deep_sleep();


Detach could be called while already detached (like during object destruction) which could cause an underflow.

c1728p9 · 2017-08-16T15:26:01Z

drivers/Timer.cpp

@@ -41,6 +42,7 @@ void Timer::stop() {
    core_util_critical_section_enter();
    _time += slicetime();
    _running = 0;
+    sleep_manager_unlock_deep_sleep();


Unlock should only be called if the timer was running.

0xc0170 · 2017-08-17T07:57:24Z

@c1728p9 Added fixes for the last review.

c1728p9 · 2017-08-17T14:44:18Z

drivers/I2C.cpp

@@ -125,6 +125,7 @@ void I2C::unlock() {
 int I2C::transfer(int address, const char *tx_buffer, int tx_length, char *rx_buffer, int rx_length, const event_callback_t& callback, int event, bool repeated)
 {
    lock();
+    sleep_manager_lock_deep_sleep();
    if (i2c_active(&_i2c)) {


If I2C is already active, you aren't starting a new transfer. You should probably either unlock before returning or add the lock until after the active check.

c1728p9 · 2017-08-17T14:49:03Z

drivers/CAN.cpp

        _irq[(CanIrqType)type] = func;
        can_irq_set(&_can, (CanIrqType)type, 1);
    } else {
+        sleep_manager_unlock_deep_sleep();


Same problem could occur with the else case - it is valid to call attach with null multiple times in a row. Maybe as a general strategy, each interrupt type could be locked when donothing is replaced, and unlocked when donothing is set. A similar change should also be made for SerialBase.

0xc0170 · 2017-08-18T09:08:21Z

@c1728p9 Fixed, I'll rebase once all approved to clean the history

0xc0170 · 2017-08-21T08:34:45Z

This now should be feature complete (testing ongoing, plus reviews so expect me to rebase). I rebased it , so the first comment is not relevant anymore - this is now adding these:

add sleep manager API
deep sleep locks in drivers (Serial, SPI, etc)
rtos idle loop locks deep sleep (backward compability) for now

It should be good to go - being backward compatible plus adding new feature that we will use with upcoming tickless addition.

This is still against feature hal spec branch. I'll send tests as separate patch soon.

Quickly tested with rtos tests for K64F and GCC ARM - all OK

0xc0170 · 2017-08-21T08:39:17Z

retest uvisor

0xc0170 · 2017-08-22T12:43:56Z

/morph test

mbed-bot · 2017-08-22T12:44:08Z

Result: FAILURE

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

/morph test

0xc0170 · 2017-08-22T12:49:45Z

/morph test

mbed-bot · 2017-08-22T15:23:12Z

Result: FAILURE

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

/morph test

Output

mbed Build Number: 1077

Build Prep failed!

studavekar · 2017-08-22T15:27:02Z

/morph test

mbed-bot · 2017-08-22T19:11:13Z

Result: SUCCESS

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

/morph test

Output

mbed Build Number: 1078

All builds and test passed!

0xc0170 · 2017-08-23T13:32:23Z

I added DeepSleepLock RAII object . One thing to consider - it is currently just lock/unlock on aquicistion/release. But to be usable also with async, we might need to add also lock/unlock methods. This however might require more additions to be completed ? For instance, should it track pair lock/unlock, thus when destroying, it would clean up ?

Please review

nvlsianpu · 2017-08-25T13:00:54Z

drivers/CAN.h

@@ -24,6 +24,7 @@
 #include "platform/Callback.h"
 #include "platform/PlatformMutex.h"
 #include "platform/NonCopyable.h"
+#include "platform/mbed_sleep.h"


Is this intentionally propagation of "mbed_sleep.h" inclusion to any module which using the driver? If not this should be moved to the cpp file of the driver instead. This remark is up to all drivers headers in this patch.

@nvlsianpu thanks for the review, the PR updated , all should be fixed.

nvlsianpu · 2017-08-28T08:58:22Z

hal/mbed_sleep_manager.c

+
+bool sleep_manager_can_deep_sleep(void)
+{
+    core_util_critical_section_enter();


Critical section is not required for read of natively supported integer (uint16). It only extend call execution time.

nvlsianpu · 2017-08-28T09:14:50Z

platform/DeepSleepLock.h

+  * void f() {
+  *     // some code here
+  *     {
+  *         DeepSleepLock lock;


Dose such a object will not been cutted out as unused object?

It looks like it is guaranteed by cpp object life cycle behaviour. cpp reference

0xc0170 · 2017-08-30T08:55:12Z

/morph test

mbed-bot · 2017-08-30T09:18:23Z

Result: ABORTED

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

/morph test

Output

mbed Build Number: 1142

Build failed!

As attach provides API to change a callback, lock/unlock should only happen if we are doing the first/last callback (start-stop).

attach/detach can be multiple invoked. Therefore lock/unlock deep sleep only for the very first time it is invoked (when callbacks are actually changed).

RAII object for disabling, then restoring the deep sleep mode

This commits contains two tests: - race condition - deep sleep locking/unlocking

For backward compability, we invoke just sleep mode. Adding critical section to complete lock/unlock deepsleep section.

As Callback currently does not have fully functional comparision (see ARMmbed#5017), we workaround by doing null check.

0xc0170 · 2017-09-07T14:41:11Z

Rebased (removed HAL docs changes, will come as separate PR), tested with nucloe f429, all good.

studavekar · 2017-09-07T15:02:42Z

/morph test

studavekar · 2017-09-07T20:51:24Z

Jobs seems have completed with results : http://mbed-ci-master-2.austin.arm.com:8081/job/pr_pipeline/1247/

theotherjimmy · 2017-09-07T21:35:47Z

Awesome! LGTM.

c1728p9 · 2017-09-07T21:50:06Z

drivers/Ticker.h

@@ -102,6 +103,10 @@ class Ticker : public TimerEvent, private NonCopyable<Ticker> {
     *  @param t the time between calls in micro-seconds
     */
    void attach_us(Callback<void()> func, us_timestamp_t t) {
+        // lock only for the initial callback setup
+        if (!_function) {
+            sleep_manager_lock_deep_sleep();


How does the sleep manager get unlocked?

Generally, sleep_manager_unlock_deep_sleep, but you're asking for the specific unlock for this call to lock right?

c1728p9 · 2017-09-07T21:53:51Z

drivers/Timer.cpp

@@ -40,6 +41,9 @@ void Timer::start() {
 void Timer::stop() {
    core_util_critical_section_enter();
    _time += slicetime();
+    if (_running) {
+        sleep_manager_unlock_deep_sleep();


You might want to unlock this on object destruction.

c1728p9 · 2017-09-07T22:04:33Z

platform/DeepSleepLock.h

+
+    /** Mark the start of a locked deep sleep section
+     */
+    void lock()


Is there any reason to expose lock and unlock? This brings back the risks of forgetting to balance locks and unlocks.

c1728p9

Looks good to me. There are a couple of things that should be addressed after this is merged.

sg- · 2017-09-08T20:45:53Z

Nice change removing the donothing. Looks like there is one straggler left in the codebase. https://github.com/ARMmbed/mbed-os/search?utf8=%E2%9C%93&q=callback%28donothing%29&type=

0xc0170 · 2017-09-09T07:39:25Z

Nice change removing the donothing. Looks like there is one straggler left in the codebase. https://github.com/ARMmbed/mbed-os/search?utf8=%E2%9C%93&q=callback%28donothing%29&type=

It was left there as donothing replacement in this PR was required for making new API working (compare if a callback was attached or not). Thanks for checking ! Yes, I'll send PR shortly to align also InterruptIn

jeromecoutant · 2017-09-20T08:14:54Z

Hi
In tests-mbed_drivers-lp_timeout and tests-mbed_hal-lp_ticker, we are still using deepsleep function which is deprecated.
Maybe you should update these tests ?
Thx

0xc0170 · 2017-09-20T08:19:24Z

In tests-mbed_drivers-lp_timeout and tests-mbed_hal-lp_ticker, we are still using deepsleep function which is deprecated.
Maybe you should update these tests ?

Yes, I'll do

nvlsianpu · 2017-10-11T08:12:34Z

Apparently looks like this path broke nrf52_Dk (nrf52840 as well) spi (tests-api-spi). Commit cb4e9b3 introduced such a change. Even if i comment off sleep instruction it still failed.

nvlsianpu · 2017-10-11T10:41:57Z

@0xc0170, @pan- ^^

0xc0170 · 2017-10-11T10:55:27Z

@nvlsianpu I do not fully get even if you comment out sleep, then it still fails? Can you create an issue with details, I would like to reproduce it locally .

nvlsianpu · 2017-10-11T11:20:04Z

@0xc0170 #5297

0xc0170 added needs: preceding PR needs: review labels Aug 15, 2017

0xc0170 force-pushed the dev_sleep_drivers branch from a85b351 to 08ba260 Compare August 15, 2017 13:07

0xc0170 mentioned this pull request Aug 16, 2017

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

Closed

c1728p9 reviewed Aug 16, 2017

View reviewed changes

0xc0170 force-pushed the dev_sleep_drivers branch 2 times, most recently from 6eb12e8 to 18ed12c Compare August 17, 2017 08:48

c1728p9 reviewed Aug 17, 2017

View reviewed changes

0xc0170 mentioned this pull request Aug 18, 2017

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

Closed

0xc0170 force-pushed the dev_sleep_drivers branch from 92ddba2 to e0048cb Compare August 20, 2017 15:46

0xc0170 changed the title ~~Drivers: add sleep locks (feature-hal-spec branch)~~ Add sleep manager API Aug 21, 2017

0xc0170 removed the needs: preceding PR label Aug 21, 2017

0xc0170 mentioned this pull request Aug 21, 2017

Add tickless feature #4943

Closed

0xc0170 force-pushed the dev_sleep_drivers branch from 8a3cb67 to 65c075b Compare August 23, 2017 13:30

nvlsianpu suggested changes Aug 28, 2017

View reviewed changes

0xc0170 force-pushed the dev_sleep_drivers branch from 65c075b to ac3b9d4 Compare August 29, 2017 15:03

nvlsianpu approved these changes Aug 29, 2017

View reviewed changes

0xc0170 added 6 commits September 7, 2017 15:35

Drivers: locking sleep fix for attach/detach

e57f0bc

As attach provides API to change a callback, lock/unlock should only happen if we are doing the first/last callback (start-stop).

drivers: fix attach sleep locking

5b33d25

attach/detach can be multiple invoked. Therefore lock/unlock deep sleep only for the very first time it is invoked (when callbacks are actually changed).

platform: add DeepSleepLock

bc70442

RAII object for disabling, then restoring the deep sleep mode

test: add sleep manager tests

c5a41d8

This commits contains two tests: - race condition - deep sleep locking/unlocking

idle loop: add deepsleep locks

39f31c3

For backward compability, we invoke just sleep mode. Adding critical section to complete lock/unlock deepsleep section.

SerialBase and CAN: fix Callbacks comparision

f6c34a2

As Callback currently does not have fully functional comparision (see ARMmbed#5017), we workaround by doing null check.

0xc0170 force-pushed the dev_sleep_drivers branch from 35dfd55 to f6c34a2 Compare September 7, 2017 14:36

theotherjimmy added ready for merge and removed needs: CI labels Sep 7, 2017

c1728p9 reviewed Sep 7, 2017

View reviewed changes

c1728p9 approved these changes Sep 7, 2017

View reviewed changes

theotherjimmy merged commit e12f116 into ARMmbed:master Sep 7, 2017

theotherjimmy removed the ready for merge label Sep 7, 2017

0xc0170 deleted the dev_sleep_drivers branch September 9, 2017 07:42

ccli8 mentioned this pull request Sep 12, 2017

Fail to enter hal_deepsleep when LowPowerTimer is used #5076

Closed

nvlsianpu mentioned this pull request Oct 11, 2017

nRF52 SPI had been broken by Add sleep manager API PR #5297

Closed

		@@ -115,9 +115,11 @@ int CAN::filter(unsigned int id, unsigned int mask, CANFormat format, int handle
		void CAN::attach(Callback<void()> func, IrqType type) {

Add sleep manager API #4912

Add sleep manager API #4912

Uh oh!

Conversation

0xc0170 commented Aug 15, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

0xc0170 commented Aug 17, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

0xc0170 commented Aug 18, 2017

Uh oh!

0xc0170 commented Aug 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

0xc0170 commented Aug 21, 2017

Uh oh!

0xc0170 commented Aug 22, 2017

Uh oh!

mbed-bot commented Aug 22, 2017

Uh oh!

0xc0170 commented Aug 22, 2017

Uh oh!

mbed-bot commented Aug 22, 2017

Output

Uh oh!

studavekar commented Aug 22, 2017

Uh oh!

mbed-bot commented Aug 22, 2017

Output

Uh oh!

0xc0170 commented Aug 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

0xc0170 commented Aug 30, 2017

Uh oh!

mbed-bot commented Aug 30, 2017

Output

Uh oh!

0xc0170 commented Sep 7, 2017

Uh oh!

studavekar commented Sep 7, 2017

Uh oh!

studavekar commented Sep 7, 2017

Uh oh!

theotherjimmy commented Sep 7, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

c1728p9 Sep 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

c1728p9 left a comment

Choose a reason for hiding this comment

Uh oh!

0xc0170 commented Aug 21, 2017 •

edited

Loading

0xc0170 commented Aug 23, 2017 •

edited

Loading

c1728p9 Sep 7, 2017 •

edited

Loading

nvlsianpu commented Oct 11, 2017 •

edited

Loading