Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from May 21, 2019

Conversation

Projects
None yet
9 participants
@marcuschangarm
Copy link
Contributor

commented Mar 6, 2019

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

This comment has been minimized.

Copy link
Member

commented Mar 6, 2019

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

@ciarmcom ciarmcom requested review from ARMmbed/mbed-os-maintainers Mar 6, 2019

@deepikabhavnani

This comment has been minimized.

Copy link
Contributor

commented Mar 6, 2019

@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

This comment has been minimized.

Copy link
Contributor Author

commented Mar 6, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Mar 6, 2019

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

This comment has been minimized.

Copy link
Member

commented Mar 6, 2019

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

This comment has been minimized.

Copy link
Contributor Author

commented Mar 6, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Mar 6, 2019

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-arm

This comment has been minimized.

Copy link
Contributor

commented Mar 7, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Mar 28, 2019

so I basically approve.

👍

@cmonr

cmonr approved these changes Mar 28, 2019

@cmonr cmonr requested a review from ARMmbed/mbed-os-core Mar 28, 2019

@0xc0170

This comment has been minimized.

Copy link
Member

commented Apr 12, 2019

@ARMmbed/mbed-os-core Please review

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

@0xc0170

This comment has been minimized.

Copy link
Member

commented Apr 15, 2019

Scheduling Ci while finalizing reviews

@mbed-ci

This comment has been minimized.

Copy link

commented Apr 15, 2019

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
uint32_t start = ticker_read(ticker);
while ((ticker_read(ticker) - start) < (uint32_t)(ms * 1000));
#else
wait_us(ms * 1000)

This comment has been minimized.

Copy link
@0xc0170

0xc0170 Apr 16, 2019

Member

missing ; - see the build failures

This comment has been minimized.

Copy link
@marcuschangarm

marcuschangarm Apr 16, 2019

Author Contributor

Whoops! 😄

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

@marcuschangarm marcuschangarm force-pushed the marcuschangarm:fix-no-rtos-wait branch from b2f0969 to c43a55c Apr 16, 2019

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

@adbridge

This comment has been minimized.

Copy link
Contributor

commented Apr 25, 2019

ci started

@mbed-ci

This comment has been minimized.

Copy link

commented Apr 25, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Apr 26, 2019

@marcuschangarm please take a look at the test failure

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

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

@0xc0170

This comment has been minimized.

Copy link
Member

commented May 3, 2019

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

@adbridge

This comment has been minimized.

Copy link
Contributor

commented May 7, 2019

Restarted ci

@mbed-ci

This comment has been minimized.

Copy link

commented May 7, 2019

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

This comment has been minimized.

Copy link
Member

commented May 12, 2019

CI restarted

@0xc0170

This comment has been minimized.

Copy link
Member

commented May 15, 2019

@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

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

Use LP tickers for waiting in no RTOS builds when available
For bare metal builds, use the lp_ticker for calls to wait_ms.

@marcuschangarm marcuschangarm force-pushed the marcuschangarm:fix-no-rtos-wait branch from c43a55c to 6aca51f May 16, 2019

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

@adbridge

This comment has been minimized.

Copy link
Contributor

commented May 17, 2019

CI started

@mbed-ci

This comment has been minimized.

Copy link

commented May 17, 2019

Test run: SUCCESS

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

@0xc0170 0xc0170 requested a review from kjbracey-arm May 20, 2019

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

@kjbracey-arm
Copy link
Contributor

left a comment

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

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

26 checks passed

continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jenkins-ci/build-ARM Success
Details
jenkins-ci/build-GCC_ARM Success
Details
jenkins-ci/build-IAR Success
Details
jenkins-ci/cloud-client-test Success
Details
jenkins-ci/dynamic-memory-usage Success
Details
jenkins-ci/exporter Success
Details
jenkins-ci/greentea-test Success
Details
jenkins-ci/mbed2-build-ARM Success
Details
jenkins-ci/mbed2-build-GCC_ARM Success
Details
jenkins-ci/mbed2-build-IAR Success
Details
jenkins-ci/unittests Success
Details
travis-ci/astyle Success!
Details
travis-ci/docs Success!
Details
travis-ci/doxy-spellcheck Success!
Details
travis-ci/events Success! Runtime is 8513 cycles.
Details
travis-ci/gitattributestest Success!
Details
travis-ci/include_check Success!
Details
travis-ci/licence_check Success!
Details
travis-ci/littlefs Success! Code size is 8448B.
Details
travis-ci/psa-autogen Success!
Details
travis-ci/tools-py2.7 Success!
Details
travis-ci/tools-py3.5 Success!
Details
travis-ci/tools-py3.6 Success!
Details
travis-ci/tools-py3.7 Success!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.