Platform: Add sleep/deepsleep user facing functions #3607

bulislaw · 2017-01-18T14:06:54Z

Add sleep/deepsleep functions to platform layer which are replacing HAL
functions with the same name, rename existing symbols in HAL layer
to hal_sleep/hal_deepsleep. This way sleep functions
are always available, even if target doesn't implement them, which makes
the code using sleep clearer. It also enables us to make decision on in
which builds (debug/release) the sleep will be enabled.

Status

READY

Related PRs

#3566

CC: @0xc0170 @sg- @betzw

bridadan · 2017-01-18T16:09:21Z

@bulislaw This seems a bit misleading to me, though I may not understand all the implications of this. Without this patch if I call sleep and the target doesn't implement it, I'll get a compiler error with an undefined function. This gives me at least an indication that the target doesn't support it.

Perhaps we could add a #warning or #error in the case where the target doesn't support sleep or it's using NDEBUG? That way the user has some indication that the target isn't actually sleeping?

betzw · 2017-01-19T07:44:29Z

platform/sleep.h

+__INLINE void sleep(void)
+{
+#ifdef NDEBUG
+#ifdef DEVICE_SLEEP


Maybe #if DEVICE_SLEEP is slightly better as more inline with other usages of this macro.

betzw · 2017-01-19T07:44:53Z

platform/sleep.h

+__INLINE void deepsleep(void)
+{
+#ifdef NDEBUG
+#ifdef DEVICE_SLEEP


Maybe #if DEVICE_SLEEP is slightly better as more inline with other usages of this macro.

betzw · 2017-01-19T07:46:25Z

Does this mean that I should remove the #if DEVICE_SLEEP from PR #3566?!?

bulislaw · 2017-01-19T09:20:51Z

@bridadan

@bulislaw This seems a bit misleading to me, though I may not understand all the implications of this. Without this patch if I call sleep and the target doesn't implement it, I'll get a compiler error with an undefined function. This gives me at least an indication that the target doesn't support it.

That was partially my point, I think usage of sleep shouldn't be surrounded in flag checking which is pointless. Also going to sleep it's an optimization rather then an functional action. I was wondering on adding a bool return value that indicates if the platform actually went to sleep, but decided that it's not like user can do anything useful with that.

Perhaps we could add a #warning or #error in the case where the target doesn't support sleep or it's using NDEBUG? That way the user has some indication that the target isn't actually sleeping?

Yeah I guess #warning would be good.

@betzw

Does this mean that I should remove the #if DEVICE_SLEEP from PR #3566?!?

Yeah no need to have it twice. I'll replace #ifdef with #if according to your comments.

Add sleep/deepsleep functions to platform layer which are replacing HAL functions with the same name, rename existing symbols in HAL layer to hal_sleep/hal_deepsleep. This way sleep functions are always available, even if target doesn't implement them, which makes the code using sleep clearer. It also enables us to make decision on in which builds (debug/release) the sleep will be enabled.

bulislaw · 2017-01-19T10:24:35Z

@0xc0170 please review.

0xc0170 · 2017-01-19T10:22:09Z

targets/TARGET_NORDIC/TARGET_MCU_NRF51822/sleep.c

 {
 // ensure debug is disconnected if semihost is enabled....
 NRF_POWER->TASKS_LOWPWR = 1;
 // wait for interrupt
 __WFE();
 }

-MBED_WEAK void deepsleep(void)
+MBED_WEAK void hal_deepsleep(void)


We might want to remove weak linkage, as it's not applied to any other implementations.

0xc0170 · 2017-01-19T10:22:43Z

platform/sleep.h

+ * Flash re-programming and the USB serial port will remain active, but the mbed program will no longer be
+ * able to access the LocalFileSystem
+ */
+__INLINE void sleep(void)


functions in these header should be defined with static specifier.

Why static?

0xc0170 · 2017-01-19T10:30:51Z

platform/sleep.h

+#warning Sleep is not supported on this platform.
+#else /* DEVICE_SLEEP */
+#ifndef NDEBUG
+#warning Sleep is disabled for debug builds.


Is this warning active by default (default profile)? means if I use mbed compile -m K64F -t GCC_ARM I would get always this warning? If that's the case, I would comment out this code straight away just to remove this annoyance.

As I recall only small-build defines NDEBUG symbol. Is default profile debug build?

It's not like our builds are clean of warnings. And yes default build is debug, which is probably the correct behavior for mbed-os. So you think that the documentation note (which i will submit separately to the handbook) is enough of a info?

bulislaw · 2017-01-19T11:57:19Z

To summarize our discussion and changes :

Warnings removed as they will be distracting in the default profiles
static added to sleep functions
MBED_WEAK removed from NRF51
Profiles will be rethink and renamed in near future.

bulislaw · 2017-01-19T13:01:28Z

/morph test

mbed-bot · 2017-01-19T13:59:54Z

Result: SUCCESS

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

/morph test

Output

mbed Build Number: 1429

All builds and test passed!

bridadan · 2017-01-19T16:03:59Z

I'm actually going to suggest we run a full nightly on this, just because there are tests that call sleep that are only ran during the nightly.

/morph test-nightly

mbed-bot · 2017-01-19T18:15:25Z

Result: SUCCESS

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

/morph test-nightly

Output

mbed Build Number: 1431

All builds and test passed!

bridadan

This is a breaking change though, yes? Previously calling sleep in the default build profile would actually put the target to sleep. Now this won't be the case. Just double checking this is an intended API break.

betzw · 2017-01-20T06:08:12Z

@bridadan as far as I remember this (breaking) change has been requested/suggested by @sg-, so maybe he is the right person to clarify if he intended what has been done here ...

bulislaw · 2017-01-20T09:28:10Z

Yes the platform won't go to sleep for default & debug profile, but I would say it's not really a breaking change as sleep is an optimization and more of a express of a intent for user, which should be opaque. Ideally platform should go to sleep whenever it can without being explicitly told to (and accidentally @betzw PR achieves just that). This PR clarifies the API and hides ugly #ifdefs from the code. Also I took the action to rethink our profiles (what the heck is default profile anyway) and come up with something like debug/release/small with some set characteristics.

bridadan

Thanks for clarifying @bulislaw and @betzw, seems ok to me 👍

betzw · 2017-01-24T11:30:19Z

What will be the next steps for this PR & PR #3566?

bulislaw · 2017-01-24T11:56:24Z

@sg- @0xc0170 can we merge that?

betzw · 2017-01-26T07:30:34Z

@bulislaw seems as we if have the "ready to merge" go from @0xc0170.
As I do not have write access permissions could you pls. merge this PR and take care also of PR #3566 to get merged?!?

cc @screamerbg

bulislaw · 2017-01-26T07:45:55Z

I don't have the powers, someone will merge it soon.

The `sleep` function as been changed into `hal_sleep` by ARMmbed#3607. Unfortunately the call to `sleep` in the hal_patch for the NRF51822 has not been updated to `hal_sleep`. The result was a link time error for targets based on NRF51822_LEGACY compiling with the mbed OS 5 tree.

The `sleep` function as been changed into `hal_sleep` by #3607. Unfortunately the call to `sleep` in the hal_patch for the NRF51822 has not been updated to `hal_sleep`. The result was a link time error for targets based on NRF51822_LEGACY compiling with the mbed OS 5 tree.

bulislaw added the needs: review label Jan 18, 2017

bulislaw force-pushed the feature_hal_sleep branch from 53eec5f to fb1ea6a Compare January 18, 2017 14:07

betzw reviewed Jan 19, 2017

View reviewed changes

bulislaw force-pushed the feature_hal_sleep branch from fb1ea6a to 6a045a4 Compare January 19, 2017 09:39

bulislaw requested a review from 0xc0170 January 19, 2017 10:24

0xc0170 requested changes Jan 19, 2017

View reviewed changes

betzw mentioned this pull request Jan 19, 2017

Put system to sleep when going idle #3566

Merged

bulislaw added 3 commits January 19, 2017 11:53

sleep: remove warnings for default build

1e73a66

sleep: make sleep/deepsleep static

8a4a3ca

NRF51822: Remove MBED_WEAK from hal_sleep functions

f2c00c7

0xc0170 approved these changes Jan 19, 2017

View reviewed changes

bridadan added needs: CI and removed needs: review labels Jan 19, 2017

bridadan reviewed Jan 19, 2017

View reviewed changes

bridadan added needs: review and removed needs: CI labels Jan 19, 2017

bridadan approved these changes Jan 20, 2017

View reviewed changes

betzw approved these changes Jan 25, 2017

View reviewed changes

0xc0170 added ready for merge and removed needs: review labels Jan 25, 2017

sg- added the release-version: 5.4.0 label Jan 26, 2017

0xc0170 removed the ready for merge label Jan 26, 2017

0xc0170 merged commit ade6722 into master Jan 26, 2017

pan- mentioned this pull request Feb 6, 2017

[Proposal] Simplify build profiles: debug and release only #3693

Merged

pan- mentioned this pull request Mar 9, 2017

[NRF51822] Fix reference to sleep in hal_patch override #3913

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Platform: Add sleep/deepsleep user facing functions #3607

Platform: Add sleep/deepsleep user facing functions #3607

bulislaw commented Jan 18, 2017

bridadan commented Jan 18, 2017

betzw Jan 19, 2017

betzw Jan 19, 2017

betzw commented Jan 19, 2017

bulislaw commented Jan 19, 2017

bulislaw commented Jan 19, 2017

0xc0170 Jan 19, 2017

0xc0170 Jan 19, 2017

bulislaw Jan 19, 2017

0xc0170 Jan 19, 2017

bulislaw Jan 19, 2017

bulislaw commented Jan 19, 2017

bulislaw commented Jan 19, 2017

mbed-bot commented Jan 19, 2017

bridadan commented Jan 19, 2017

mbed-bot commented Jan 19, 2017

bridadan left a comment

betzw commented Jan 20, 2017

bulislaw commented Jan 20, 2017

bridadan left a comment

betzw commented Jan 24, 2017

bulislaw commented Jan 24, 2017

betzw commented Jan 26, 2017

bulislaw commented Jan 26, 2017

Platform: Add sleep/deepsleep user facing functions #3607

Platform: Add sleep/deepsleep user facing functions #3607

Conversation

bulislaw commented Jan 18, 2017

Status

Related PRs

bridadan commented Jan 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

betzw commented Jan 19, 2017

bulislaw commented Jan 19, 2017

bulislaw commented Jan 19, 2017

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

bulislaw commented Jan 19, 2017

bulislaw commented Jan 19, 2017

mbed-bot commented Jan 19, 2017

Output

bridadan commented Jan 19, 2017

mbed-bot commented Jan 19, 2017

Output

bridadan left a comment

Choose a reason for hiding this comment

betzw commented Jan 20, 2017

bulislaw commented Jan 20, 2017

bridadan left a comment

Choose a reason for hiding this comment

betzw commented Jan 24, 2017

bulislaw commented Jan 24, 2017

betzw commented Jan 26, 2017

bulislaw commented Jan 26, 2017