-
Notifications
You must be signed in to change notification settings - Fork 115
Split libtock into core and high level library #164
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regarding your open question: If we can build libcore
using cargo
(see https://www.github.com/tock/tock/issues/1682) then we should be able to enable the panic_immediate_abort feature (see rust-lang/rust#55011), which should help us remove a lot of `panic!()'s machinery. That would remove the call to the panic handler entirely, so it becomes a moot point.
If panic handling is customizable then I think it should be customized through panic handler crates (as is already the case for other platforms). We would need to add a cargo
feature to libtock-core
to prevent libtock-core
from providing its panic handler.
Okay, but that's probably something for the remote future, right? |
Do you mean adding a crate only containing the panic handler here (and turning off the panic handler in libtock-core)? |
Oh, I misunderstood your question as referring to the future. Yes, my comment was out of scope for this PR. |
That's what I meant. However, when I look around I think my assertion that panic handler crates were in use on other platforms was incorrect. I'm not strongly opinionated on whether panic behavior is controlled by swapping crates or whether it is controlled through cargo features, other than I think we need a "no libtock-rs panic handler" feature so applications can overwrite things themselves. I would prefer to use interchangable panic handler crates rather than cargo features to control behavior. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great! Do you have any idea what the split looks like size wise?
@@ -1,35 +1,23 @@ | |||
#![feature(asm, lang_items, naked_functions)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this mean the new libtock-rs doesn't need the nightly toolchain?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately not. Its dependencies need it, still. In libtock-core
it should be in principle possible to remove the necessity for the nightly compiler (as in https://github.com/rust-embedded/cortex-m-rt), however I am not sure whether it is worth the effort, while the tock kernel needs its.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume this is fixed?
It is hard to answer: using
I think the last point has some potential for optimization, if one restricts to not delay in paralllel futures. The current workarounds for drawbacks of the timer interface are quite expensive. However, I think we currently don't have objective criteria to judge about the size of libtock-rs applications. I started to create some basic tooling for that and have the vision of including that into the CI of libtock: https://github.com/torfmaster/libtock-size-utils (e.g. store some snapshot sizes, and fail if some threshold is hit). |
It should be mentioned that those are the approximate values for a trivial example. We observed that the impact of futures or the parallel sleep driver on the text size is significantly lower in more complex examples. This is kind of expected due to synergetic effects. |
54b9f4c
to
b2680dd
Compare
What is this PR still waiting on? |
Apart from some minor fixes we still need to create a solution for the panic handler, i.e. add a feature flag or an external panic crate. |
I recommend adding a cargo feature flag to libtock-core that -- when enabled -- removes the panic handler. |
I'll do that+add the original panic handler for libtock-rs behind a feature flag. |
bors try |
I already prepared that. It's just that it's hard to test. |
Adding the original panic handler inside libtock-rs behind a feature flag may be a test. |
tryBuild succeeded |
I pushed something, s.t. you can see where this is getting at but I am still trying to find a solution to include the feature flags into our build pipeline. |
bors try |
tryBuild succeeded |
I pushed a commit addressing all comments but the panic_handler issue. |
I added an example including a custom error handler and build this in CI. An alternative would be to put a regular panic example into the core crate and restore the original error handler of libtock-rs as a default feature. |
I found a slightly different solution which doesn't add a new example and tests both features. |
I would recommend a readme in the core folder that describes what is (and what isn't) in core. Mainly for future readers to understand what the conceptual split is. |
I added a (minimal) readme and remove the now duplicated example with a custom panic handler. bors try |
tryBuild succeeded |
bors r+ |
Build succeeded |
Summary
In order to address #160 we decided to do a first draft of splitting the library into parts.
The idea is that there are two layers:
(names tbd). Libtock-core contains the panic handler, the entry point, the allocator and the syscall interfaces. libtock-rs uses libtock-core and the interface stays as usual.
Possible Extensions
We deleted the "blink" code from the panic handler. This saves more than 1kbytes in the text segment. If this feature is still wanted we could implement the following:
In the long run the (default) panic handler could exit the app and send debug information to the kernel which could print it for example. This could be implemented once the kernel functionality is in place.
Open questions
The current implementation of the panic handler more or less silently fails if there is a panic. Panics can be identified by enabling the "trace_syscall" option in the kernel. How should this be tackled?
Should there be a default panic handling or should it be customizable and if yes how should it be customized?