Use `thread_sleep_for` to sleep

[hugueskamba]

This function allows the application to be built wit the bare metal profile without requiring the rtos-api.

[kjbracey]

Justification for the change isn't correct - both calls do the same thing, including RTOS-vs-non-RTOS.

I would expect a C++ app to use the C++ call, normally. If you're trying a bare-metal build, doing requires: { "bare-metal", "rtos-api" } gives you the C++ call.

The C call is for convenience for C code (that was previously using wait_ms, so it doesn't have to become C++ just for this), or use inside the drivers or platform directory, just in case the rtos directory isn't there.

[hugueskamba]

Justification for the change isn't correct - both calls do the same thing, including RTOS-vs-non-RTOS.

I would expect a C++ app to use the C++ call, normally. If you're trying a bare-metal build, doing requires: { "bare-metal", "rtos-api" } gives you the C++ call.

The C call is for convenience for C code (that was previously using wait_ms, so it doesn't have to become C++ just for this), or use inside the drivers or platform directory, just in case the rtos directory isn't there.

@kjbracey-arm I thought the C call was there to be able to build with the full OS or with the bare metal profile. I was not aware of requires: { "bare-metal", "rtos-api" } in order to use the RTOS API as I thought the bare metal profile always excluded the rtos dir.

So are you saying that this PR is not necessary?

[kjbracey]

I thought the bare metal profile always excluded the rtos dir.

Previously it did, but since https://github.com/ARMmbed/mbed-os/pull/10104, you can get a subset of the RTOS API without the RTOS.

For the "thread sleep" call, it's just a matter of spelling, but the RTOS API also gives you higher-level functions like EventFlags::wait_xxx that are important for real-world code (eg waking your app from an interrupt) that are otherwise not easy to express bare metal.

So are you saying that this PR is not necessary?

Not for the reasons given, at least.

I think an update is required to https://os.mbed.com/docs/mbed-os/v5.14/reference/bare-metal-apis.html.

[hugueskamba]

I thought the bare metal profile always excluded the rtos dir.

Previously it did, but since ARMmbed/mbed-os#10104, you can get a subset of the RTOS API without the RTOS.

For the "thread sleep" call, it's just a matter of spelling, but the RTOS API also gives you higher-level functions like EventFlags::wait_xxx that are important for real-world code (eg waking your app from an interrupt) that are otherwise not easy to express bare metal.

So are you saying that this PR is not necessary?

Not for the reasons given, at least.

I think an update is required to https://os.mbed.com/docs/mbed-os/v5.14/reference/bare-metal-apis.html.

FYI @AnotherButler @iriark01

[kjbracey]

The "requires" mechanism as a whole doesn't seem to be really documented in the main docs anyway - I only see it in the "migrating from Mbed OS 2" tutorial. I recall seeing some discussion about this a few weeks back - maybe someone is already looking at it?

[hugueskamba]

The "requires" mechanism as a whole doesn't seem to be really documented in the main docs anyway - I only see it in the "migrating from Mbed OS 2" tutorial. I recall seeing some discussion about this a few weeks back - maybe someone is already looking at it?

FYI @AnotherButler @iriark01 again.

@kjbracey-arm I have updated the PR description.

[kjbracey]

@kjbracey-arm I have updated the PR description.

The remaining question is "why do you want to exclude rtos-api"? I don't think there's a strong reason to do that.

In the PR, I did consider for a while having bare-metal include rtos-api by default, so it was always there, and main reason I shied away from it was for backwards compatibility problems - knowing that people tend to have /rtos in a .mbedignore. That's also the reason why thread_sleep_for is fully implemented in the /platform directory.

If making an app, there's no real reason not to require rtos-api. It adds no overhead for calls you don't use, and gives you a lot of tools to work with. And it's not too large, so if you're trying to minimise build time, it's not that big a deal.

A counter-argument is that if blinky is supposed to be the most-minimal-possible app, so should we be requiring something beyond the absolute bare minimum?

But it does feel a bit like using printf rather than cout << in a C++ "hello world". Yes, it's more minimal, but is it C++? Is using a C API rather than C++ "Mbed OS"?

[kjbracey]

TL;DR - I'm basically neutral on this PR, now the description has been updated - don't have a particular preference for either style.

[hugueskamba]

@kjbracey-arm If one follows the instructions in the getting started guide (https://os.mbed.com/docs/mbed-os/v5.14/quick-start/compiling-the-code.html), they will not be able to build with the bare metal profile as suggested on that page. This PR was required to allow that.

[AnotherButler]

Will adding "RTOS" to the list of bare metal APIs (https://os.mbed.com/docs/mbed-os/v5.14/reference/bare-metal-apis.html) solve this issue, or is there more to it?

[kjbracey]

@AnotherButler

It's a subset of RTOS APIs. Some aren't applicable if you are in a single-thread environment, and a couple haven't been implemented, but could be in future.

You can see a list in the commit message here: https://github.com/ARMmbed/mbed-os/pull/10104/commits/83b329cb71d53b3f50961c647590c6bffe36da4a

Listing those 4 classes (plus osThreadSetFlags) would suffice, for the first "what's available in bare metal issue".

The thing about the "requires" mechanism in the build system backing the bare metal build is a bit broader.

[evedon]

I am catching this thread quite late. For the documentation change, given that we have a bare metal blinky example then we should just point to that. See comment in https://github.com/ARMmbed/mbed-os/issues/11570#issuecomment-543758777

[amq]

@kjbracey-arm would it make sense to change everything in this commit https://github.com/ARMmbed/mbed-os/commit/a522dcfa0acc5ab22f092abab57cc39f9b2ae87d from ThisThread::sleep_for to thread_sleep_for?

My use case: I want to build an app with SD and with mbed-os/rtos/* in .mbedignore. This will save around 7 KB.

[kjbracey]

@amq Not really. There should be negligible size difference between the C and C++ API. They do exactly the same thing.

The fact that you're asking the question kind of highlights a reason not to have made this change - giving the false impression that somehow using thread_sleep_for is going to save resources.

This change was only justified because the docs said to add requires: [ "bare-metal" ] rather than requires: [ "bare-metal", "rtos-api" ]. (That requires addition is what you should now be doing rather than using .mbedignore).

The bare metal form of the RTOS API doesn't add any overhead - you only pay for the calls you use, so there's no real reason to not ask for it, apart from saving a teeny bit of compilation time.

[amq]

@kjbracey-arm the problem in my case is that SD builds just fine with mbed-os/rtos/* in .mbedignore, but I can't build it with bare-metal (and/or rtos-api). It fails right on

[ERROR] Attempt to override undefined parameter 'sd.CMD_TIMEOUT' in 'application[*]'

[kjbracey]

The "requires" line switches the entire build to "opt-in". You have to list everything you want to use in that line - so you'd need to add "sd", I guess.

Any folder with a mbed_lib.json is excluded unless the application requires it (directly, or indirectly via another library).

[amq]

Ahh, thank you, worked perfectly!