Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
UART: Add wrapper around RIOT's UART-interface #39
base: main
Are you sure you want to change the base?
UART: Add wrapper around RIOT's UART-interface #39
Changes from 27 commits
17995a0
dd1372a
9dad81a
5e3c472
fc644cf
88ff4a6
c6fbd33
79e8e23
78d412a
fa26004
4aa6841
9be601c
cfbe7c1
9bff3de
793614b
c6ef6d5
c7cc626
b1f76f0
c3738ca
581729d
0b965d2
11014dd
b81ee74
4d917bf
2896752
6b40cd9
fd5d9b3
fd70931
0c1f2dd
055fa14
66de5f0
1b87e04
a98b8e3
6de785c
0fc9d44
3ff8dd0
a9231a0
c3afaf4
7aff5f7
5a6ea25
cc63b4f
83856a1
b6eada9
bbfa1c3
cb8257f
e0cd794
0b0c400
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 where we still feel the shock of the leakpocalypse -- sadly, having a
&a
callback and a Drop implementation isn't good enough. Say I write this code:then when data is received,
f
will be called even though it has long been deallocated.There are two (mutually non-exclusive) ways around this:
.new()
onUartDeviceStatus<'static>
, thus taking a&'static FnMut
.fn(...) -> !
), and drop gets called reliably.I suggest to use the first approach, unless your application needs the second, in which case I'd suggest doing both. (Existing APIs in riot-wrappers often only do the second, but I've come to consider that impractical in many situations).
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'm not sure if I'm misinformed here, but if I use a
&'static mut F
as parameter, then it's not possible anymore that the closure captures variables because you have to do something like this:This is because
static
can only be function pointers afaik.I also thought about taking ownership of the closure, but that does't work either because if we take the address of the closure and then return it with the
Self
, the address of the closure changed. All other wrappers avoid this by putting the closure on the heap e.g. withBox
, but we don't have this option here because of the#![no_std]
world.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.
Yeah, a
'static
can not be such a closure -- that's what the scoped API would be for.It's not only about not being able to
Box
something away. The thread's initial example would also not be sound if we had Box -- the Box would aBox<dyn FnMut() + 'a>
, with the'a
ending when the function returns, but the function would need to demand+ 'static
to warrant passing it into the unsafe C function. The box would help with the issue of the callback not moving when stored, but as we need the drop issue solved anyway (which is most easily done with the body-runs-in-callback approach), we don't have the moving reference problem.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 have updated my old solution and have followed the scoped approach. I think this is a good compromise between comfort and safety. As long as the user doesn't get the idea to use something like
core::mem::forget
, he should be on the safe side. Furthermore I have now implemented an extra new method to inizialize theUART
using a static callback.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, we can't rely on the user to not
core::mem::forget
when the consequences are unsound (use-after-free).From the current state (which is good but does not go all the way yet), we'd need to:
impl<'scope> UartDevice<'scope> { ... }
to a block that isimpl UartDevice<'static> { ... }
, because that's the scope-less situation that is sound(One could also leave out the drop as it's implied -- what matters is that it happens reliably at this point, and that while the scope can safely forget the &mut, the self_ will be dropped reliably before all the references to 'scope it contains become invalid).
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 thought about the problem for a bit. I still like my version better because in normal rust you can do something like this:
This creates a memory leak without even using
forget
. You can do all sorts of things causing bad behavior in safe rust if you really wish. I think atm there is a good compromise between safety and ease of use. Furthermore, the normal use case would be that the user creates theUART
once on startup, then it stays forever, and the destructor is never called (at least in my experience). And I also find it a bit awkward and unintuitive, that the main function runs inside theUART
-Wrapper.But that is only my opinion. What do you think?
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.
(Reminder to never type longer text in browser windows... let's hope this second version is better)
Safe Rust code must never allow unsafe behavior. No matter how exotic or even malicious the safe code is: If it leads to undefined behavior (use-after-free, double-free, unaligned access, read while modify, or using uninitialized memory), it's always the fault of the crate that did the unsafe thing. (There are some corner cases, such as opening /proc/self/mem or overwriting libc symbols, but they're more the fault of the OS or linker).
Leaking memory is safe in Rust -- it's not undefined behavior, but more importantly, as was found out late during Rust's 1.0 phase, there are just too many ways in which one can leak memory that static analysis would not cover. Thus, our code must be safe even when memory is (intentionally or accidentally) leaked.
There are some APIs in riot-wrappers that need to deal with this already; I hope I've caught all by now that I've written before I learned the above. Using them is not too bad, I think, and falls in three categories:
Use in a main function.
Yeah, it's wrapping main in possibly several layers of callbacks. But the compiler can inline expand them, and drop the cleanup code if the innermost function is an endless loop (which it often is), so it's not too bad. Indent-wise, a good pattern is to have a main function that sets up all these scopes, and then calls a
fn inner_main(thread_scope, ...) -> ! { ... }
that gets rid of all the indentation.Use in startup functions.
Startup functions can make use of static_cell as in the stdio_coap startup code to store their forever data. A downside you mentioned is that closures can't be named and thus not be stored in statics -- true, but these can be worked around using functions. Things get convoluted and type names truly horrible, so a viable alternative is:
Use workarounds:
Use with nightly. The naming issues of closures and the horrible type names around them can both be done away using the
type_alias_impl_trait
(TAIT) nightly feature. Then, the type names become simple, and anything can be stored statically easily.Given the current progress on async functions, I hope that TAIT is stabilized this year.
Put it in a box and use Box::leak to get a &'static.
RIOT does support dynamic memory management, and while it's generally frowned upon, some dynamic allocation at startup is sometimes accepted as being a practical compromise.
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.
Ok that makes sense. I have to say that I think I'm at the end of my knowledge here. What would you say is the best way to implement it?
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.
The code outline is still at #39 (comment). Implementing either of the two options there would be sufficient to make this PR mergable. (I'll eventually want to have both, but do whichever works better with your use case).
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.
Fixed :)