Use RTOS wait primitive where possible

[stevew817]

Description

When an RTOS is present, use the thread wait primitive for millisecond resolution and above. This will lead to vastly improved power consumption on targets with tickless support.

Pull request type

[ ] Fix
[ ] Refactor
[ ] New target
[x] Feature
[ ] Breaking change

[0xc0170]

@bulislaw @c1728p9 Please review as well

[c1728p9]

@stevew817 do you have rough numbers on how much power this saves?

[geky]

Isn't this already handled by the logic in wait_us?
https://github.com/ARMmbed/mbed-os/pull/6969/files#diff-5a106edf0944ae7ba2d1bf95153fb0a0R43

[felix-ht]

Wasn't the old function - wait_us doing the same thing, but with some added checks? see line 43

[felix-ht]

Looking at the old function there are some differences. That is if you enter a delay that has some us component. The old function would do a thread wait and then a busy wait:
wait_us(1500) would call Thread::wait(1); and then do a busy wait for the remaining 500 us

In this sense wait_s(float s) is quite "non deterministic". A slight change in the input to wait_s() (eg 0.002 vs 0.001999) can have a big input to the power consumption. This might also be the issue that @stevew817 was actually trying to address.

[felix-ht]

this breaks the interface of wait(float s) as you can no longer wait sub ms time deltas

[geky]

@Oteph made a good point about sub-millisecond waits breaking the float wait function.

Is that a part of the goal of this PR?

[stevew817]

@geky @Oteph
Have you seen anyone actually use 4 or more digits of precision on a float that indicates seconds? You're then getting awfully close to the limit for losing precision on the float (which is 6 decimal places IIRC, so in the us domain), which means you suddenly end up with an added busy-wait instead of a pure ms-based wait, even if you think your calculations should lead to a round ms number.
IMHO, if you want microsecond precision - use wait_us. If you don't require that level of precision, use the _ms or regular wait functions. The us version will allow you to wait up to 4.294.967.295 us, which is roughly 71 minutes. If you require microsecond precision on a 71-minute wait, I'd argue you're doing something very wrong, since oscillator drift and busy-waiting skew then also come into play, in addition to the computational overhead of starting the wait.

I'll back out this change if the vision of the mbed OS team doesn't match mine, but at least consider my argument.

[bulislaw]

I'm there with you, unfortunately we are committed to backward compatibility for API and behavior between major versions of Mbed OS. wait is one of the areas that need unification and rethinking for next major mbed version.

[felix-ht]

Wait_us calls thread::wait (no busy wait), if the input is a multiple of 1000, which it is if the input is in milliseconds. So the behavior of your implementation is the same as in the current one besides that you are not surrounding it with sleep_manager_lock_deep_sleep - sleep_manager_unlock_deep_sleep.

[cmonr]

Closing this PR since looking at the feedback, we're not going to be modifying the wait API at this time.

An issue can still be made separate from this PR so that it can be tracked.

[deepikabhavnani]

@stevew817 @c1728p9 @geky @kjbracey-arm @janjongboom - Please share your thoughts on wait API update in line to this PR.

diff --git a/platform/mbed_wait_api_rtos.cpp b/platform/mbed_wait_api_rtos.cpp
index f24f58cbd..2ad2ae691 100644
--- a/platform/mbed_wait_api_rtos.cpp
+++ b/platform/mbed_wait_api_rtos.cpp
@@ -26,12 +26,23 @@

 void wait(float s)
 {
-    wait_us(s * 1000000.0f);
+    // If wait is more than 2 msec
+    if ((s * 1000.0f) > 1.9f) {
+        wait_ms((uint32_t)s * 1000.0f);
+    } else {
+        wait_us(s * 1000000.0f);
+    }
 }

If the RTOS is present and delay is greater than 2 milliseconds, it will enter deep sleep and save power same as Thread::wait(). For delay in sub-millisecond precision behavior will be same as stated here https://github.com/ARMmbed/mbed-os/blob/master/platform/mbed_wait_api.h#L60

2 msec was selected to avoid issue of 1ms delay resulting into no wait. Other suggestion was to have 10 msec

wait_ms - As per the PR, If the RTOS is present, this API will enter deep sleep and save power same
as Thread::wait() as oppose to API description here - https://github.com/ARMmbed/mbed-os/blob/master/platform/mbed_wait_api.h#L71

[kjbracey]

@deepikabhavnani Your most significant suggestion here is about changing the implementation of wait_ms to be just ThisThread::sleep (formerly Thread::wait), right? (Otherwise the above patch to wait(float) wouldn't do anything). That's the potential backwards-compatibility biggie.

Personally, I'd be happy to make that change, as it makes sense for a wait call to have precision equal to its granularity, and there is a route for applications to cope with the change - if they really want micro-second precision, they can say wait_us(2000), and that would work on old and new versions of Mbed OS. (And if they wanted nanosecond precision, it would be nice to give them wait_ns(20000000)).

But I'm more cavalier about backwards-compatbility than many.

Alternatively, could have a new function, documented to only be millisecond precision, but not sure what on earth to call it if it has to sit among wait(float), backwards-compatible wait_ms and wait_us.

[janjongboom]

+100 on the change, I think we should do this as soon as possible; but no comments on the quality of the patch.

[deepikabhavnani]

Alternatively, could have a new function, documented to only be millisecond precision

We can deprecate wait_ms and wait_us and add wait_msec and wait_usec with new behavior

[geky]

+1 for the change, though does it need to be backwards incompatible?

It this switch is internal and the called by all three APIs that should keep our API backwards compatible while still removing the deepsleep lock.

[kjbracey]

It this switch is internal and the called by all three APIs that should keep our API backwards compatible while still removing the deepsleep lock.

I'm not convinced that having 3 calls that are totally equivalent and only vary mode by the magnitude of value passed is the way to go for this sort of fundamental API.

If making changes, I'd rather go the opposite direction - have distinct specific calls for

• thread sleep (or low-res timer if non-RTOS) [ms]
• high-res timer sleep [us]
• cpu spin sleep [ns]

Maybe do have one convenience wrapper on top of those such as wait(float) that is auto-thresholding by value, for people who don't want to worry too much about the formal characteristics of the API, but I think you do need to be able to properly request particular characteristics like "thread sleep or not" and "hardware sleep or not" via the API you explicitly use.

More on the other PR.