Use LP tickers for waiting in no RTOS builds when available #9960

marcuschangarm · 2019-03-06T15:42:55Z

Description

For bare metal builds, use the lp_ticker for calls to wait_ms.

Pull request type

[x] Fix
[ ] Refactor
[ ] Target update
[ ] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change

Reviewers

Release Notes

For bare metal builds, use the lp_ticker for calls to wait_ms instead of us ticker if lp_ticker is available.

ciarmcom · 2019-03-06T16:00:46Z

@marcuschangarm, thank you for your changes.
@ARMmbed/mbed-os-core @ARMmbed/mbed-os-maintainers please review.

deepikabhavnani · 2019-03-06T16:41:33Z

@marcuschangarm - This will enable both lpticker and usticker by default.
Also using lpticker will change the functional behavior of wait - as precision will be in msec and not usec, will be considered breaking change.

Faced similar challenges with RTOS version - #8189

@kjbracey-arm @bulislaw - Can provide more input on this for bare metal version.

marcuschangarm · 2019-03-06T17:02:53Z

This will enable both lpticker and usticker by default.

Only if the user actually calls wait_us. Also, presumably the cost of enabling the lp ticker is negligible if you already have the us ticker running.

Also using lpticker will change the functional behavior of wait - as precision will be in msec and not usec, will be considered breaking change.

We can keep the float version as-is. I'm more concerned about having to use the us ticker by default for wait_ms on systems that supports lp ticker.

deepikabhavnani · 2019-03-06T17:12:32Z

Only if the user actually calls wait_us

wait_us is called from multiple places in mbed-os code base

We can keep the float version as-is.

Limitation is not of float, lpticker precision is in msec

bulislaw · 2019-03-06T17:22:19Z

This will enable both lpticker and usticker by default.

Only if the user actually calls wait_us. Also, presumably the cost of enabling the lp ticker is negligible if you already have the us ticker running.

Could we check what's the difference?

Also using lpticker will change the functional behavior of wait - as precision will be in msec and not usec, will be considered breaking change.

We can keep the float version as-is. I'm more concerned about having to use the us ticker by default for wait_ms on systems that supports lp ticker.

I'm also afraid we are going further and further apart between RTOS and non-RTOS implementations, wait_ms with RTOS will use RTX osDelay. The way it works is a bit weird and timing is absolutely not guaranteed (I think it's something like osDelay(5) for 1ms tick will sleep for 4-5ms). It's good idea to use LPTicker rather than usticker whenever possible, but we should clean up the "wait" calls and make them behave in a predictable way between both OS variants.

marcuschangarm · 2019-03-06T17:47:57Z

Limitation is not of float, lpticker precision is in msec

Exactly, I would expect msec precision from an API that takes msec as input and usec precision from an API that takes usec as input.

deepikabhavnani · 2019-03-06T17:50:32Z

Exactly, I would expect msec precision from an API that takes msec as input and usec precision from an API that takes usec as input.

100% with you on this, just that existing implementation was not like that :-( hence functionality change.
Forgot to mention - Problem is with wait - msec or usec.

kjbracey · 2019-03-07T07:55:06Z

wait_us is called from multiple places in mbed-os code base

True, but I believe all instances in the core code are because they specifically need an "IRQ-safe busywait" rather than "sleep", not because they want precision. Not sure about stuff in targets.

This ties in with JIRA ISGDEVPREQ-1083, in which I propose fundamentally separating the wait APIs from the sleep ones - wait calls would be the same busy waits in RTOS and non-RTOS. I hadn't proposed retaining wait_ms (as opposed to sleep) as part of that, but maybe it is needed in specialised circumstances. Up to now, that call was just going to be removed, on the basis that any busy-waiting milliseconds delay is not to be encouraged.

What most normal users of wait_ms really want to be doing is using ThisThread::sleep_for(), and a non-RTOS implementation of that can stop the CPU, sleep the HAL and wait for an interrupt, achieving much better power than this busy wait. That's something we need to get on and implement.

There was recent talk about changing wait_us to have a non-DEVICE_USTICKER fallback to wait_ns for secure code, justified on not having any timers at all in a secure image. Conceivably that should be also done manually in core code like mbed_die that uses timers to avoid ever starting up or pulling in any HAL ticker code.

For a non-RTOS build, I think having at least a low power timer running is going to be almost a given - you need something to be your timebase. Emulation of rtos calls especially Kernel::get_ms_count() would also need it. I think we should concentrate on eliminating unnecessary use of the microsecond timer, and assume the low power is what will be normally active.

But more care is needed in the "always present" bits to avoid pulling any timers into bootloaders or secure libraries that otherwise need no timing at all - either don't try for blinky LED on mbed_die or use wait_ns CPU spins.

So, this PR is an incremental improvement, but if we're going to go ahead with wider changes proposed in ISGDEVPREQ-1083, both changes are redundant, as both affected calls are to be deprecated. Still, no reason to block this, so I basically approve.

cmonr · 2019-03-28T18:47:59Z

so I basically approve.

👍

0xc0170 · 2019-04-12T11:45:08Z

@ARMmbed/mbed-os-core Please review

0xc0170 · 2019-04-15T13:44:14Z

Scheduling Ci while finalizing reviews

mbed-ci · 2019-04-15T17:56:04Z

Test run: FAILED

Summary: 6 of 7 test jobs failed
Build number : 1
Build artifacts

Failed test jobs:

jenkins-ci/mbed-os-ci_mbed2-build-ARM
jenkins-ci/mbed-os-ci_mbed2-build-GCC_ARM
jenkins-ci/mbed-os-ci_build-IAR
jenkins-ci/mbed-os-ci_mbed2-build-IAR
jenkins-ci/mbed-os-ci_build-ARM
jenkins-ci/mbed-os-ci_build-GCC_ARM

0xc0170 · 2019-04-16T07:49:59Z

platform/mbed_wait_api_no_rtos.c

+    uint32_t start = ticker_read(ticker);
+    while ((ticker_read(ticker) - start) < (uint32_t)(ms * 1000));
+#else
+    wait_us(ms * 1000)


missing ; - see the build failures

Whoops! 😄

adbridge · 2019-04-25T15:16:49Z

ci started

mbed-ci · 2019-04-25T20:52:28Z

Test run: FAILED

Summary: 1 of 11 test jobs failed
Build number : 4
Build artifacts

Failed test jobs:

jenkins-ci/mbed-os-ci_greentea-test

adbridge · 2019-04-26T10:00:55Z

@marcuschangarm please take a look at the test failure

0xc0170 · 2019-05-03T12:19:46Z

I restarted the test, the latest test logs have no failure, and hook showed error that should be fixed now on master.

adbridge · 2019-05-07T15:03:07Z

Restarted ci

mbed-ci · 2019-05-07T19:11:11Z

Test run: FAILED

Summary: 2 of 11 test jobs failed
Build number : 5
Build artifacts

Failed test jobs:

jenkins-ci/mbed-os-ci_dynamic-memory-usage
jenkins-ci/mbed-os-ci_greentea-test

0xc0170 · 2019-05-12T19:15:47Z

CI restarted

0xc0170 · 2019-05-15T07:56:23Z

@marcuschangarm Can you rebase this PR please? the error in dynamic memory job is configuration error that I've noticed in another Pr that is also weeks old (assuming the CI job was updated - configuration changed). Rebase should fix it

For bare metal builds, use the lp_ticker for calls to wait_ms.

adbridge · 2019-05-17T08:48:51Z

CI started

mbed-ci · 2019-05-17T12:57:17Z

Test run: SUCCESS

Summary: 11 of 11 test jobs passed
Build number : 6
Build artifacts

kjbracey

Still fine with this - I will need to rebase my #10104. (I think it does the same thing differently, will double-check).

ciarmcom requested review from a team March 6, 2019 16:00

ciarmcom added the needs: review label Mar 6, 2019

cmonr approved these changes Mar 28, 2019

View reviewed changes

cmonr added the release-version: 5.13.0-rc1 label Mar 28, 2019

cmonr requested a review from a team March 28, 2019 18:49

0xc0170 approved these changes Apr 12, 2019

View reviewed changes

0xc0170 added needs: CI and removed needs: review labels Apr 15, 2019

0xc0170 requested changes Apr 16, 2019

View reviewed changes

0xc0170 added needs: work and removed needs: CI labels Apr 16, 2019

cmonr added needs: CI and removed needs: work labels Apr 16, 2019

0xc0170 approved these changes Apr 17, 2019

View reviewed changes

adbridge added needs: work and removed needs: CI labels Apr 26, 2019

0xc0170 added needs: CI and removed needs: work labels May 3, 2019

0xc0170 added needs: work and removed needs: CI labels May 15, 2019

Use LP tickers for waiting in no RTOS builds when available

6aca51f

For bare metal builds, use the lp_ticker for calls to wait_ms.

adbridge added needs: CI and removed needs: work labels May 17, 2019

0xc0170 requested a review from kjbracey May 20, 2019 09:31

0xc0170 added needs: review and removed needs: CI labels May 20, 2019

kjbracey approved these changes May 20, 2019

View reviewed changes

0xc0170 added ready for merge and removed needs: review labels May 20, 2019

0xc0170 merged commit 5be51a5 into ARMmbed:master May 21, 2019

0xc0170 removed the ready for merge label May 21, 2019

Use LP tickers for waiting in no RTOS builds when available #9960

Use LP tickers for waiting in no RTOS builds when available #9960

Uh oh!

Conversation

marcuschangarm commented Mar 6, 2019 • edited by 0xc0170 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Pull request type

Reviewers

Release Notes

Uh oh!

ciarmcom commented Mar 6, 2019

Uh oh!

deepikabhavnani commented Mar 6, 2019

Uh oh!

marcuschangarm commented Mar 6, 2019

Uh oh!

deepikabhavnani commented Mar 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bulislaw commented Mar 6, 2019

Uh oh!

marcuschangarm commented Mar 6, 2019

Uh oh!

deepikabhavnani commented Mar 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kjbracey commented Mar 7, 2019

Uh oh!

cmonr commented Mar 28, 2019

Uh oh!

0xc0170 commented Apr 12, 2019

Uh oh!

0xc0170 commented Apr 15, 2019

Uh oh!

mbed-ci commented Apr 15, 2019

Test run: FAILED

Uh oh!

0xc0170 Apr 16, 2019

Choose a reason for hiding this comment

Uh oh!

marcuschangarm Apr 16, 2019

Choose a reason for hiding this comment

Uh oh!

adbridge commented Apr 25, 2019

Uh oh!

mbed-ci commented Apr 25, 2019

Test run: FAILED

Uh oh!

adbridge commented Apr 26, 2019

Uh oh!

0xc0170 commented May 3, 2019

Uh oh!

adbridge commented May 7, 2019

Uh oh!

mbed-ci commented May 7, 2019

Test run: FAILED

Uh oh!

0xc0170 commented May 12, 2019

Uh oh!

0xc0170 commented May 15, 2019

Uh oh!

adbridge commented May 17, 2019

Uh oh!

mbed-ci commented May 17, 2019

Test run: SUCCESS

Uh oh!

kjbracey left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

marcuschangarm commented Mar 6, 2019 •

edited by 0xc0170

Loading

deepikabhavnani commented Mar 6, 2019 •

edited

Loading

deepikabhavnani commented Mar 6, 2019 •

edited

Loading