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

Sleep rework, RTOS API for bare metal, wait deprecations #10104

Merged
merged 17 commits into from Jul 17, 2019

Conversation

Projects
None yet
@kjbracey-arm
Copy link
Contributor

commented Mar 14, 2019

Description

A chunk of work bringing together connected themes.

  • Decouple timed sleep code from RTOS idle
  • Some refinements to that sleep code
  • Separate C++ RTOS API from actual RTX implementation
  • Implement bare metal subset of RTOS API using timed sleep code
  • Deprecate wait APIs that actually sleep - only remaining ones spin and are safe from weird contexts.
  • Small number of C RTOS APIs to allow basic sleep from C.

Details:

  • rtos/mbed_lib.json moved down to rtos/TARGET_CORTEX, still with name: "rtos" so it's still checked by MBED_CONF_RTOS_PRESENT.
  • new rtos/mbed_lib.json has name: "rtos-api", so checked by MBED_CONF_RTOS_API_PRESENT, and accessible as rtos/Semaphore.h still.
  • Basic C sleep and get_ms_count APIs in platform/mbed_thread.h, in case C++ RTOS API isn't present, or we need to sleep from C.
  • Couple of new compatibility headers to smooth over RTOS/non-RTOS difference, eg include mbed_rtos_types.h instead of cmsis_os2.h (cmsis_os2.h is not there in bare-metal, just the C++ RTOS APIs).

Points for discussion:

  • Directory structure, naming
  • Final detail of naming/deprecations for wait APIs - deprecate all and split into sleep and delay?

TODO: Make event loop use the RTOS API for its sleep done
TODO: Recheck exact changes made to sleep code, for discussion with Russ (GitHub diff isn't useful)

With this PR, you can take blinky, add "requires" : [ "bare-metal" ] to its mbed_app.json change its wait_ms to ThisThread::sleep_for, and you've now got a bare metal blinky that properly deep sleeps, with all the latency compensation stuff you get in the RTOS. (For backwards compatibility, wait_ms remains non-sleeping in non-RTOS builds).

Pull request type

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

Reviewers

@c1728p9, @sg-, @geky, @theotherjimmy, @bridadan, @bulislaw

Release Notes

  • Subset of RTOS API now available in bare metal builds
  • Full power save functionality now available in bare metal builds via RTOS API
  • Consistent sleep vs wait API naming - wait() and wait_ms() deprecated

Migration notes

  • Use of wait and wait_ms should be reviewed to check whether sleeping is appropriate or not in the context the call occurs - if so, change to ThisThread::sleep_for/thread_sleep_for, else change to wait_us.

@ciarmcom ciarmcom requested review from bridadan, bulislaw, c1728p9, geky, sg-, theotherjimmy and ARMmbed/mbed-os-maintainers Mar 14, 2019

@ciarmcom

This comment has been minimized.

@theotherjimmy

This comment has been minimized.

Copy link
Contributor

commented Mar 14, 2019

@kjbracey-arm Love the concept. I'm thinking that adding "rtos-api" to bare metal is pretty safe so long as all of it optimizes away when unused.

@ithinuel

This comment has been minimized.

Copy link
Member

commented Mar 14, 2019

@kjbracey-arm see that core_util_atomic_fetch_and_* and core_util_atomic_fetch_or_* as added in this work.
There start to be a significant amount of core_util_atomic_ method. Wouldn't that make sense to have a single one with a callback as its argument and let the user decide what it needs to do in between ldrex and strex ?
We could still provide core_util_atomic_fetch_and_* as macros, that would probably save us from maintaining a lot of ldrex-strex block with slight variation in each of them.
IIRC the compiler should also be able to inline the function on its own whenever it is possible.

@kjbracey-arm

This comment has been minimized.

Copy link
Contributor Author

commented Mar 15, 2019

Wouldn't that make sense to have a single one with a callback as its argument and let the user decide what it needs to do in between ldrex and strex ?

Doesn't work in general, unfortunately. The ARM architecture doesn't let you just run arbitrary code between LDREX and STREX - any intervening memory writes (such as storing stuff to the stack) could make the STREX fail, potentially causing a deadlock.

Thus the C code as it currently stands is already technically dodgy, as we can't be sure the compiler won't be putting any stores in, and a compiler warning about the dodginess is being suppressed with a pragma. LDREX/STREX should only really be used in assembler - the C intrinsics are deprecated. Putting in a callback would greatly increase the chance of failure at low optimisation levels as it would be inclined to push a return address to the stack.

So, if I were to do anything, I would be taking the LDREX/STREX intrinsics out, converting either to inline assembler or complete op compiler intrinsics. Compilers do these days have intrinsics for these ops, but afaict, they insist on them being SMP-compatible, thus adding DMB instructions we don't need.

Certainly it would make sense to reduce boilerplate by using macros to generate the code chunks.

The C/C++ general pattern for arbitrary ops, which doesn't suffer from the LDREX/STREX problem is:

  type oldValue =atomic_load(x);
  type newValue;
  do {
       newValue = whatever_operation(oldValue);
  } while (!atomic_compare_exchange_weak(x, &oldValue, newValue);

There the LDREX/STREX pair is contained in the exchange, and you can have your code as complex as you like in the loop. I guess that construct could be wrapped into a "callback" pattern, but I don't fancy inventing a new style different from the standard.

@kjbracey-arm

This comment has been minimized.

Copy link
Contributor Author

commented Mar 15, 2019

I'm thinking that adding "rtos-api" to bare metal is pretty safe so long as all of it optimizes away when unused.

Should do. The only static things are the "thread flags" for the main thread, and the SysTimer, and they should be stripped if no-one uses them.

There is a code cost of actually doing the sleep - when you change blinky from wait_ms to ThisThread::sleep_for does increase code size - you're pulling in the power management stuff and hal_sleep and hal_deepsleep.

Kernel::get_ms_count() pulls in the SysTimer, but it's only started on first call, not boot, so again nothing dragged in if not used.

In a bootloader type build or secure library, you'd presumably never call sleep or anything time related, so it should all stay out.

@kjbracey-arm kjbracey-arm force-pushed the kjbracey-arm:sleep_api branch 3 times, most recently Mar 15, 2019

@kjbracey-arm

This comment has been minimized.

Copy link
Contributor Author

commented Mar 15, 2019

Junked separate "rtos-impl" directory - no significant file restructuring in this version. Originally I thought I was unable to nest "rtos-impl" inside "rtos", but it was just operator error.

@0xc0170

This comment has been minimized.

Copy link
Member

commented Mar 15, 2019

Break this up into more PRs? RTOS for bare metal could be separated, but here to show justification for the sleep decoupling

👍 Once we get concept agreed, split would help to few PRs (fixes can go separately - first 4 commits are good fixes on their own).

Directory structure, naming

+1 for internal namespace and even platform/internal for separation (debating when someone is breaking user space or not, often leads to - "this is internal function, no harm done"). If I didn't know this is internal, anyone else would?

I am interested in learning about RTOS API for baremetal builds - what subset we want to provide? What is 3b7ddc5ceab29ffc96006c3cc58361f7be313f7f achieving ? RTOS API for baremetal - two worlds unite? It's large change, commit msg might help to understand the goal.

Do we have design document for this RTOS API? (would do some reading before I dig into implementation details)

@kjbracey-arm kjbracey-arm force-pushed the kjbracey-arm:sleep_api branch Mar 15, 2019

@kjbracey-arm

This comment has been minimized.

Copy link
Contributor Author

commented Mar 15, 2019

No design doc as yet, this is kind of the proposal. I've filled in the message for the RTOS API commit, which should help.

The wait/sleep clarification is associated with https://jira.arm.com/browse/ISGDEVPREQ-1083

@theotherjimmy

This comment has been minimized.

Copy link
Contributor

commented Mar 15, 2019

@kjbracey-arm In the future be aware that nested libraries are designed to function independently WRT "requires". If you notice a corner case where this does not work, let me know.

@kjbracey-arm kjbracey-arm referenced this pull request Mar 18, 2019

Merged

Assembler atomics #10147

@kjbracey-arm

This comment has been minimized.

Copy link
Contributor Author

commented Mar 18, 2019

Started splitting a bunch of the minor fixes into separate PRs.

@kjbracey-arm kjbracey-arm force-pushed the kjbracey-arm:sleep_api branch to f7f6d3b Mar 18, 2019

@cmonr

This comment has been minimized.

Copy link
Contributor

commented Mar 18, 2019

Started splitting a bunch of the minor fixes into separate PRs.

That explains it.

@mbed-ci

This comment has been minimized.

Copy link

commented Jul 12, 2019

Test run: FAILED

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

Failed test jobs:

  • jenkins-ci/mbed-os-ci_cloud-client-test
  • jenkins-ci/mbed-os-ci_dynamic-memory-usage

kjbracey-arm and others added some commits Jan 18, 2019

RTOS API for bare metal
Provide partial RTOS API for bare metal builds - things that
can be done in a single threaded environment.

Allows more code to work in both RTOS and bare metal builds without
change, and in particular gives easy access to the ability to
efficiently wait for something occurring in interrupt.

Available in bare-metal:
* ThisThread
* osThreadFlagsSet to set flags on main thread (can be set from IRQ)
* EventFlags (can be set from IRQ)
* Semaphores (can be released from IRQ)
* Mutex (dummy implementation)

Not useful:
* ConditionVariable (could only be signalled from 2nd thread)
* RtosTimer (calls in a second thread context)
* Thread

Unimplemented:
* Mail, Queue, MemoryPool

Possible future work:
* ConditionVariableCS to act as IRQ signalled ConditionVariable
Make events use RTOS API
Switch from CMSIS-RTOS to mbed C++ API, which are available in bare
metal build.

Other minor tidies, like removing unnecessary volatile.
SysTimer: don't always hold deep sleep lock
Revert back to older behaviour where we hold deep sleep lock only while
timing a sleep. Previous version was a speed optimisation, but broke
some tests.
rtos: NULL -> nullptr
Some build errors seen with NULL not being defined. Rather than
add cstddef includes, switch to C++11 nullptr.
equeue - avoid Kernel::get_ms_count from IRQ
Kernel::get_ms_count is documented as not working from IRQ.

In RTOS builds it can return misleading answers - see
ARM-software/CMSIS_5#625

In non-RTOS builds, it can trigger an assert, as it upsets the
sleep logic.

Modified code is still not ideal - could be improved further if
there was a fast path for "post now" that didn't bother looking
at timers (both at post time and dispatch time).

@kjbracey-arm kjbracey-arm force-pushed the kjbracey-arm:sleep_api branch from ab49061 to ed4bf52 Jul 15, 2019

@kjbracey-arm

This comment has been minimized.

Copy link
Contributor Author

commented Jul 15, 2019

CI restarted

@mbed-ci

This comment has been minimized.

Copy link

commented Jul 15, 2019

Test run: FAILED

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

Failed test jobs:

  • jenkins-ci/mbed-os-ci_cloud-client-test
  • jenkins-ci/mbed-os-ci_dynamic-memory-usage
events: Don't use OsTimer in SysTick builds
Previous fix assumed OsTimer is in use - it is for tickless RTOS builds
and non-RTOS builds, but non-tickless RTOS builds avoid it altogether
and use the SysTick peripheral.

Restore previous behaviour for those builds, and just always read the
tick count - there is no downside to this when ticking.

While in the code, make equeue_tick initialisation explicit. This was
problem if the OsTimer is not yet initialised when th
done while debugging - turns out not to be part of the fix, but it
speeds up the runtime code.
@kjbracey-arm

This comment has been minimized.

Copy link
Contributor Author

commented Jul 16, 2019

CI restarted - last fix broke non-tickless builds like the K66F in CI.

@mbed-ci

This comment has been minimized.

Copy link

commented Jul 16, 2019

Test run: SUCCESS

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

@kjbracey-arm

This comment has been minimized.

Copy link
Contributor Author

commented Jul 16, 2019

Lingering "change requested" was from @bulislaw, think it's resolved. Could you approve?

@bulislaw
Copy link
Member

left a comment

Looks good, but please try to submit smaller PRs in the future.

@SeppoTakalo SeppoTakalo merged commit 9875338 into ARMmbed:master Jul 17, 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 RTOS ROM(+0 bytes) RAM(+0 bytes)
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 8673 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.