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 thread_sleep_for to sleep #193

Merged

Conversation

hugueskamba
Copy link
Contributor

@hugueskamba hugueskamba commented Oct 11, 2019

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

@kjbracey
Copy link

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
Copy link
Contributor Author

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
Copy link

kjbracey commented Oct 14, 2019

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.

@hugueskamba
Copy link
Contributor Author

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

@hugueskamba hugueskamba changed the title Call thread_sleep_for to sleep Use thread_sleep_for to sleep Oct 14, 2019
@kjbracey
Copy link

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?

This function allows the application to be built wit the bare metal
profile without requiring `rtos-api`.
@hugueskamba hugueskamba force-pushed the hk-call-correct-sleep-function branch from 0fd1e7e to 81a8ba3 Compare October 14, 2019 08:10
@hugueskamba
Copy link
Contributor Author

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
Copy link

kjbracey commented Oct 14, 2019

@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
Copy link

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 hugueskamba merged commit 80f1888 into ARMmbed:master Oct 14, 2019
@hugueskamba hugueskamba deleted the hk-call-correct-sleep-function branch October 14, 2019 12:59
@hugueskamba
Copy link
Contributor Author

@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
Copy link
Contributor

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
Copy link

kjbracey commented Oct 18, 2019

@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: ARMmbed/mbed-os@83b329c

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
Copy link
Collaborator

evedon commented Oct 18, 2019

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 ARMmbed/mbed-os#11570 (comment)

@amq
Copy link

amq commented Nov 5, 2019

@kjbracey-arm would it make sense to change everything in this commit ARMmbed/mbed-os@a522dcf 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
Copy link

kjbracey commented Nov 5, 2019

@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
Copy link

amq commented Nov 5, 2019

@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
Copy link

kjbracey commented Nov 5, 2019

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
Copy link

amq commented Nov 5, 2019

Ahh, thank you, worked perfectly!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants