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 all commits
9722813
5ace203
530e441
14f0d08
3d7b15b
5fb2e12
e1dd2c0
ac0d680
a39a7a7
1a0d07c
b62e14e
788d633
7e2badb
eec16de
398cf41
77290d9
2adc6ac
a3ee384
6ca86f3
2942733
bf83576
06ba80e
2788caa
a545c7b
e0e99a6
820b42c
74e68fd
be20cea
3a9e145
d957c9f
e45c297
7aa931b
762a639
748fe93
cd066aa
12fda60
973d70a
59f3d78
03595f9
9763bf3
2b63e71
455291e
16a67e9
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.
Sorry, just found a soundness hole in this:
A user might do something like this:
The two classical workarounds that occur to me off my head are using Pin (which we don't fully need but resolves the issue because they can't be swapped), or making 'scope part of Self (where for _without_rx and _static that'd be 'static), and then Main is
for<'a> Main: FnOnce(&mut UartDevice<'a>)
so it becomes a brand, and this module will by construction ensure that there are never two UartDevice that can be swapped.A scope was even in the type back in #39 (comment), which from how I view it now was right, but we just didn't manage to figure out why it was. The new_scoped etc. types could still live on the
<'static>
version of this, which should make it unambiguous for both the user and the compiler which to pick.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.
Hmm, which version would you recommend? I have to admit, I've reached the end of my knowledge here.
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 think the version currently on
chrysn-impl_uart_wrapper
would work. That the example still works unmodified even though I altered the lifetimes accordingly is a good sign.[edit: Oups, that one so far does not apply the branding; I'm redoing 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.
Fixed now: The proposed 479d663 introduces a lifetime on a UART, which states that if any callback has been configured, that callback is valid for that lifetime. Constructors that don't set a callback (or the one that sets one for a shorter time) are implemented on the
'static
because that's what corresponds to "no callback has been registered (therefore all registered callbacks live for the longest possible time)".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.
When is this safe to call?
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.
Hmm, I'm not so sure about this. Is it an option to consume the object here via taking
self
, so that the is destructed, and the user can use the pins as normal GPIO?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.
If the object is consumed, it can't be reinited with the same settings later; it's an option, but I don't think that's the intention of deinit_pins. That seems to be more of a "let's pause this for a moment, do funky stuff with the UART, and return to using it later".
If we're not sure (I am not either), maybe let's remove that API for now.
If we keep this and the other removals in a separate commit when squashing, it'll be easy to selectively revert it.