UART: Add wrapper around RIOT's UART-interface

[kbarning]

Here is my attempt to create a wrapper for RIOT's UART-interface :)

[chrysn]

Thanks for the PR. I've cleared it for a first CI run, but haven't yet had time to look at it. Given this needs a uart_t at construction, it might be good to look at #37 in parallel.

[chrysn]

Thanks for adding this. It looks good over-all, and very comprehensive. I have several details and questions annotated across the source. Quite a few of them might easily be explained by inspiration from existing code; I'd still hope to not continue on the bad examples I've set in the past.

[kbarning]

Thanks for adding this. It looks good over-all, and very comprehensive. I have several details and questions annotated across the source. Quite a few of them might easily be explained by inspiration from existing code; I'd still hope to not continue on the bad examples I've set in the past.

First of all, thank you for this comprehensive review. I will try to resolve your issues in the next few days :)

[chrysn]

CI tests are failing because RIOT's version of the riot-sys crate wasn't updated to the latest nightly yet -- but that's on the RIOT repository side of things.

[chrysn]

When all open issues are addressed (I think it's only the matter of safety and scoping), as with the DAC PR, I'll want to have a test around to ensure that the code is actually built during CI. Let me know whether you want to give that a try in the style of c9cf3f2, otherwise I'll do it as part of the final review.

[chrysn]

I think this is approaching completion.

Given the many fixes and merges, please rebase onto current master and squash into one or some logically structured commits.

Would you add some small runnable test?

[Teufelchen1]

@kbarning hey! I accidentally started a competing PR, I overlooked your work. Do you intend of picking this PR up again?

[kbarning]

@kbarning hey! I accidentally started a competing PR, I overlooked your work. Do you intend of picking this PR up again?

Hey, I'm still waiting for an answer from @chrysn. I've implemented everything that he suggested to me so far, but I'm still waiting for approval.

[chrysn]

Sorry for having let this sit unduly long, especially after you've brought it all this way – and not having remembered that this is sitting when Teufelchen started up his PR didn't help make the situation better, yet here we are.

Before I start looking through both PRs to see whether there's one that clearly outperforms (on usability, safe usability, features or clarity), would the two of you like to have a look over each other's PRs and/or a short exchange on whether there is one you both prefer or aspects missing from both?

[Teufelchen1]

Okay, I gave it a thought and I believe this PR is better, in the sense that it is much more mature (documentation, api completeness, etc.).

What is missing compared to my draft is the implementation of embedded_io::Write / embedded_io::ErrorKind that chrysn wished for - But that can be retro fitted easily. I recommend doing that in a follow up PR (maybe I can do that), so this work here can finally get merged.

This PR needs a rebase. I did a shitty one locally to test this PR, it was annoying but not too bad. The code still works as intended.

@kbarning I can imagine you want this done sooner than later, please rebase and squash. I'll poke @chrysn to review it in a timely manner :)

(I can also offer to rebase & squash for you, if you like)

[chrysn]

Final round of comments while going through this again after the duplicate PR situation is being resolved.

Most of them should be easy to apply. The soundness one will need another round of review, but given that there was a lifetime in the type in earlier iterations, it should be managable.

[chrysn]

Sorry, just found a soundness hole in this:

A user might do something like this:

new_scoped(…, |_char| (), |self_| {
    let racy = 0;
    let shortlived_callback = |_char| { *racy = char };
    new_scoped(…, shortlived_callback, |self2_| {
        core::mem::swap(self_, self2_);
    });
    // at drop time it's effectively self_ that is dropped
    do_something_with(&mut racy);
    // wait for  input
    // goes to self2_, calls shortlived_callback, writes into racy
});

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.

[kbarning]

Hmm, which version would you recommend? I have to admit, I've reached the end of my knowledge here.

[chrysn]

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…]

[chrysn]

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)".

[chrysn]

The better example would be to use static_cell or maybe crate::mutex::Mutex, or even println!("Received {:02x}", new_data);, but unless you feel super motivate to update this, we can leave that as is for now.

[kbarning]

I have added the println! in the examples :)

[chrysn]

Then could almost just be fn cb(new_data: &[u8) { println!(...) }, no need for a mut static that'll need unsafe and makes the linter scream.

Almost, because we don't get a &mut cb, which hints at that user_callback should be F and not &'scope mut F … which again we can't do because we don't have a size limit.

We may need to iterate over that later on, but that'll work better when we see actual use cases. So let's have this in for now. I've just updated the example to not use static muts any more.

[chrysn]

When is this safe to call?

[kbarning]

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?

[chrysn]

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.

[kbarning]

Final round of comments while going through this again after the duplicate PR situation is being resolved.

Most of them should be easy to apply. The soundness one will need another round of review, but given that there was a lifetime in the type in earlier iterations, it should be managable.

I will have a look in the following days :)

[kbarning]

Okay, I gave it a thought and I believe this PR is better, in the sense that it is much more mature (documentation, api completeness, etc.).

What is missing compared to my draft is the implementation of embedded_io::Write / embedded_io::ErrorKind that chrysn wished for - But that can be retro fitted easily. I recommend doing that in a follow up PR (maybe I can do that), so this work here can finally get merged.

This PR needs a rebase. I did a shitty one locally to test this PR, it was annoying but not too bad. The code still works as intended.

@kbarning I can imagine you want this done sooner than later, please rebase and squash. I'll poke @chrysn to review it in a timely manner :)

(I can also offer to rebase & squash for you, if you like)

I have rebased my branch. The squashing is done directly via this PR right? (at least I think so, if not I think @chrysn can enable it). It would be great if you can add the embedded_io stuff @Teufelchen1 because I hardly have any time at the moment 😞.

[Teufelchen1]

Yes, squashing can be done via GitHub UI by chrysn (I do not have permissions).
Yes, I will add the embedded_io stuff in a followup PR, once this is merged :)

[chrysn]

I wouldn't insist on squashing in general; if you prefer things squashed to a single commit, that can be done at merge time. (My general approach is that I like to have history visible in the tree, and merges from master into the feature branches, because people can always get a view as-if-squashed from git using --first-parent. For many of the items in here such as the fixes, it makes sense to apply them, and if doing that is excessive, let's rather squash it all -- also because due to the rebasing, it'll be nontrivial to understand where the added pub mod dac comes from before this is squashed.)

The latest version you pushed was not tested locally, was it? I've found a few compile errors, but am fixing them locally and pushing to the branch as part of a final checking round.

[chrysn]

I've added a few fixes to the branch chrysn-impl_uart_wrapper (couldn't push to the PR's branch); am not through with them, but you may want to have a look at them.