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

Ensure us_ticker is initialized before it is used #5194

Merged
merged 2 commits into from Oct 2, 2017

Conversation

Projects
None yet
6 participants
@c1728p9
Contributor

c1728p9 commented Sep 25, 2017

Update platform code to use the ticker common layer rather than using HAL us ticker directly. This both ensures that the underlying ticker is properly initialized and that the value read is in microseconds with full 32-bit range.

@c1728p9 c1728p9 force-pushed the c1728p9:fix_sleep branch Sep 25, 2017

// Use the RTOS to wait for millisecond delays if possible
int ms = us / 1000;
if ((ms > 0) && core_util_are_interrupts_enabled()) {
sleep_manager_lock_deep_sleep();

This comment has been minimized.

@c1728p9

c1728p9 Sep 25, 2017

Contributor

The current implementation of wait doesn't allow deep sleep, since the us ticker must be running. Two possible solutions to this are:

  • Make distinction between busy wait and suspended wait (encourage use of Thread::wait again)
  • Use Thread::wait for ms part and busy wait for remainder (start time would need to be read after Thread::wait)

Anyone have a preference or other ideas on how to handle this?

This comment has been minimized.

@0xc0170

0xc0170 Sep 26, 2017

Member

The current implementation of wait doesn't allow deep sleep, since the us ticker must be running.

tickers are always running (only initialize function present), are we getting API to stop/resume them?

Make distinction between busy wait and suspended wait (encourage use of Thread::wait again)

From a user perspective, what API would be preferable ?

If I consider the second one (ms part and busy), what about these:

wait_us() - Busy wait with higher accurancy (microseconds) always blocking, no Thread:wait involved. We are specifically requesting high precision and be blocking. Smaller periods (limits to 32bit microseconds).
wait_ms() + wait() (this would not allow us accuracy?) - lower accuracy (task switch allowed), plus tickless possible. Bigger periods.

This comment has been minimized.

@kjbracey-arm

kjbracey-arm Sep 28, 2017

Contributor

Largely agree with this proposal - see #5216.

Only caveat is that it is something of an API change - wait_ms will wait up to that number of milliseconds, being potentially low by up to a millisecond, rather than waiting at least that number of milliseconds.

cf confusion when RTX changed a similar function: http://www.keil.com/support/docs/3766.htm and linked threads.

Although in this case the change would be towards conventional/expected behaviour, rather than away from.

@c1728p9

This comment has been minimized.

Contributor

c1728p9 commented Sep 25, 2017

BLE and LWIP still use us_ticker_read directly. These are not used for delaying though, so for proper operation they would need to hold the sleep lock while powered up. This may be ok for lwip since the device likely won't be able to go into deep sleep when a network connection is in use. This may present a bigger power for BLE since they are typically very low power. @pan- what are your thoughts on this?

Two possible solutions:

  • Replace use of microsecond ticker with the low power ticker (Won't work for devices without lp ticker)
  • Replace use of the microsecond ticker with osKernelGetTickCount() with the assertion that mbed-os's tickrate is always 1ms (Won't work for mbed 2 devices)

Let me know what you think of these solutions, or if you can think of another solution.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Sep 26, 2017

@pan-

This comment has been minimized.

Member

pan- commented Sep 26, 2017

BLE and LWIP still use us_ticker_read directly. These are not used for delaying though, so for proper operation they would need to hold the sleep lock while powered up.

us_ticker_read is used in partner port (Beetle and Maxim). Sooner or latter those ports will use the more generic Cordio port which don't rely on us_ticker. In the meantime, until the Cordio update is effective, the existing code can be patched to use the lp ticker if available or osKernelGetTickCount as a last resort.

@0xc0170 0xc0170 referenced this pull request Sep 28, 2017

Merged

Avoid wait_ms() spin #5216

@c1728p9

This comment has been minimized.

Contributor

c1728p9 commented Sep 28, 2017

/morph test

@c1728p9

This comment has been minimized.

Contributor

c1728p9 commented Sep 28, 2017

This PR should fix the initialization problems so should be merged if everyone is happy with it and it passes testing. The issue regarding sleep should probably be fixed in a separate PR.

@mbed-bot

This comment has been minimized.

mbed-bot commented Sep 28, 2017

Result: FAILURE

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

/morph test

Initialize the HAL ticker on first read
Initialize the ticker on the first call to ticker_read* if the
ticker has not been initialized already.

@0xc0170 0xc0170 added needs: CI and removed needs: review labels Sep 29, 2017

Remove direct use of us ticker from platform
Update platform code to use the ticker common layer rather than using
HAL us ticker directly. This both ensures that the underlying ticker
is properly initialized and that the value read is in microseconds with
full 32-bit range.

@0xc0170 0xc0170 force-pushed the c1728p9:fix_sleep branch to 114d60c Sep 29, 2017

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Sep 29, 2017

This PR should fix the initialization problems so should be merged if everyone is happy with it and it passes testing. The issue regarding sleep should probably be fixed in a separate PR.

👍

I rebased it to resolve conflict with clock() function. Restarting CI now

/morph test

@mbed-bot

This comment has been minimized.

mbed-bot commented Sep 29, 2017

Result: SUCCESS

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

/morph test

Output

mbed Build Number: 1436

All builds and test passed!

@theotherjimmy theotherjimmy merged commit cd8e9e6 into ARMmbed:master Oct 2, 2017

4 checks passed

Cam-CI uvisor Build & Test Success
Details
ci/morph-test Job has completed
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment