async: add SPI #347

Dirbaio · 2022-01-12T11:53:42Z

This is an exact mirror of the blocking SPI trait (including the proposed changes in #351 ), except the following differences:

W: 'static is required everywhere, otherwise complicated lifetime bounds are required in the future GATs.

rust-highfive · 2022-01-12T11:53:46Z

r? @ryankurte

(rust-highfive has picked a reviewer for you, use r? to override)

363: async/i2c: fix lifetimes on transaction() r=ryankurte a=Dirbaio Trying to implement i2c for a shared i2c bus behind an async mutex yielded lots of cursed lifetime errors, because `&'a mut [Operation<'b>]` is invariant on `'b`, not covariant as one would expect... To fix this, the GAT future needs two lifetimes. Also counterintuitively, the future must be `+ 'a`, but NOT `+ 'b`. Then `AddressMode: 'static` is needed because Rust wants annoying `where A: 'a` bounds otherwise. The async SPI PR has the same issue, will fix later. #347 With these fixes, implementing i2c on a mutex works nicely now: ```rust struct SharedI2c<T>(tokio::sync::Mutex<T>); impl<T: ErrorType> ErrorType for SharedI2c<T> { type Error = T::Error; } impl<A: AddressMode, T: I2c<A>> I2c<A> for SharedI2c<T> { type ReadFuture<'a> where Self: 'a, = impl Future<Output = Result<(), Self::Error>> + 'a; fn read<'a>(&'a mut self, address: A, read: &'a mut [u8]) -> Self::ReadFuture<'a> { async move { self.0.lock().await.read(address, read).await } } type WriteFuture<'a> where Self: 'a, = impl Future<Output = Result<(), Self::Error>> + 'a; fn write<'a>(&'a mut self, address: A, write: &'a [u8]) -> Self::WriteFuture<'a> { async move { self.0.lock().await.write(address, write).await } } type WriteReadFuture<'a> where Self: 'a, = impl Future<Output = Result<(), Self::Error>> + 'a; fn write_read<'a>( &'a mut self, address: A, write: &'a [u8], read: &'a mut [u8], ) -> Self::WriteReadFuture<'a> { async move { self.0.lock().await.write_read(address, write, read).await } } type TransactionFuture<'a, 'b> where Self: 'a, 'b: 'a, = impl Future<Output = Result<(), Self::Error>> + 'a; fn transaction<'a, 'b>( &'a mut self, address: A, operations: &'a mut [Operation<'b>], ) -> Self::TransactionFuture<'a, 'b> { async move { self.0.lock().await.transaction(address, operations).await } } } ``` cc `@matoushybl` Co-authored-by: Dario Nieuwenhuis <[email protected]>

Dirbaio · 2022-02-16T02:08:21Z

Pushed new version

Rebased on top of SPI: split into device/bus, allows sharing buses with automatic CS #351
Added async SpiDevice trait, mirroring the blocking one.

Dirbaio · 2022-02-18T23:08:02Z

New version

Rebased on SPI: split into device/bus, allows sharing buses with automatic CS #351, spi: allow impls returning early, add flush. #365
Added ExclusiveDevice impl

I've had LOTS of trouble with the "async closure". The right bound would be something like where F: for<'b> FnOnce(&'b mut Bus) -> (impl Future<Output=Result<R, Bus::Error>> + 'a), but it's impossible to express that in today's Rust.

The workaround is to use the outer 'a lifetime, and have the future "give back" the bus: where F: FnOnce(&'a mut Bus) -> Fut, Fut: Future<Output=(&'a mut Bus, Result<R, Bus::Error>)> + 'a.

This has the nasty implication that it's no longer possible to use ? inside the closure. Also rustc can't infer the result of transaction(), so you have to assign it to a temp variable. It can't infer the closure arg type either, so you have to write |bus: &mut T::Bus| ...

It's not possible to add the helper default methods to SpiDevice to hide this for single-transfer transactions either.

blocking:

    let mut buf = [0;64];
    device.transaction(|bus: &mut T::Bus| {
        bus.write(&[0x42])?;
        bus.read(&mut buf)?;
        Ok(())
    }).unwrap()

async:

    let mut buf = [0;64];
    let res: Result<(), T::Error> = device.transaction(|bus: &mut T::Bus| async {
        if let Err(e) = bus.write(&[0x42]).await {
            return (bus, Err(e));
        }
        if let Err(e) = bus.read(&mut buf).await {
            return (bus, Err(e));
        }
        (bus, Ok(())
    }).await;
    res.unwrap()

Not pretty, but it works. I'm afraid it's the best we can do with today's async traits.

Updated embassy here
Example of a dumb driver here

embedded-hal-async/src/spi.rs

ryankurte

ohh wow that gets gnarley with the bus callbacks huh. i'm a little cautious that folks are going to have bad experiences trying to use it but, seems well documented, and given it is -for now- it doesn't seem like there's a better option (and your examples look great!).

in my cursed async experience sometimes things can be simplified by concretising async return types, and using &'static dyn dispatch, however, only sometimes and i don't really have the time to bash my head on it at the moment...

Dirbaio · 2022-02-25T22:00:21Z

You can do it with Box<dyn Future<..>>, but that requires alloc. &'static dyn Future won't work without horrible hacks, as the impl has to allocate the future somewhere... Also dyn itself should be avoided, it causes some bloat with the vtable and virtual calls, and prevents optimizations.

Also, in the long-term, first-class support for async closures will make this even more ergonomic.

I've been looking deeper at this. It turns out it is possible to express where F: for<'b> FnOnce(&'b mut Bus) -> (impl Future<Output=Result<R, Bus::Error>> + 'a), by desugaring it manually like this: playground

where 
  for<'b> F: FnOnce<(&'b mut Bus,)>,
  for<'b> <F as FnOnce<(&'b mut Bus,)>>::Output: Future<Output = Result<R, Bus::Error>> + 'b,

but there's a few issues:

Rust's current trait solver isn't smart enough to process these bounds. It kinda works with -Zchalk (if you change it to not use futures, as Chalk can't handle generators right now..)
It works with a standalone async fn but not with an || async { ... } closure, because Rust is not inferring it to be higher-ranked. HRTBs: "implementation is not general enough", but it is rust-lang/rust#70263

It's likely this will improve in the future, we can change the trait to take advantage of whatever solution comes up later...

Dirbaio · 2022-03-01T22:54:33Z

Rebased on latest master.

Update on the "Rust's current trait solver isn't smart enough" issue above: By reducing the where clauses from the post above I've arrived to this

Seems the issue is bounds on higher-ranked associated types don't work in general. There's quite a few issues open, closest is maybe rust-lang/rust#23481 . Seems it's blocked on "lazy normalization": rust-lang/rust#60471 . These bounds all work fine with Chalk, so maybe they just plan to just wait for Chalk...? Either way, this is going to take quite a while...

I think it's best to take the unfortunate usability hit for now, and not block this. When either Chalk, lazy normalization, or first-class async closures land, we'll be able to remove the hack (with a breaking change, of course...).

eldruin

Thank you! The code is difficult to grasp, though.
I just added a few nitpicks but it looks good to me.

embedded-hal-async/src/spi.rs

ryankurte · 2022-03-02T21:44:31Z

I think it's best to take the unfortunate usability hit for now, and not block this. When either Chalk, lazy normalization, or first-class async closures land, we'll be able to remove the hack (with a breaking change, of course...).

hugely appreciate your effort in exploring these options @Dirbaio! 👍 to landing this once those small changes are applied.

Dirbaio · 2022-03-10T05:15:28Z

New version

Rebased on async: update to new location of where clause. #371
Doc fixes from review comments. Since most of them applied to both blocking+async, I've fixed them in both.

eldruin

Looks good to me, thanks! Let's merge #371 and then this one

eldruin

Looks good to me, thanks!
bors r+

bors · 2022-03-10T16:35:55Z

Build succeeded:

380: async/spi: replace the "give back the Bus" hack with a raw pointer. r=ryankurte a=Dirbaio The async SPI trait contains a "hack" to workaround limitations in Rust's borrow checker: #347 It turns out the hack doesn't allow many shared bus implementations, for example when using an async Mutex: The `transaction` method gets `&'a mut self`, and the closure wants `&'a mut Self::Bus`. This only works if the Bus is a direct field in `self`. In the mutex case, the Bus has to come from inside the `MutexGuard`, so it'll live for less than `'a`. See https://gist.github.com/kalkyl/ad3075182d610e7b413b8bbe1228ab90 which fails with the following error: ``` error[E0597]: `bus` does not live long enough --> src/shared_[spi.rs:78](http://spi.rs:78/):34 | 63 | fn transaction<'a, R, F, Fut>(&'a mut self, f: F) -> Self::TransactionFuture<'a, R, F, Fut> | -- lifetime `'a` defined here ... 78 | let (bus, f_res) = f(&mut bus).await; | --^^^^^^^^- | | | | | borrowed value does not live long enough | argument requires that `bus` is borrowed for `'a` ... 89 | } | - `bus` dropped here while still borrowed ``` This is an alternative hack. If lifetimes don't work, simply don't use lifetimes at all! - Downside: it needs `unsafe{}` in all callers of transaction (but these should be rare, many users will use the `SpiDeviceExt` helpers. - Upside: it's now possible to write sound shared bus impls. - Upside: it no longer requires the "give back the bus" hack, it's now possible again to use `?` inside the closure for error handling. cc `@kalkyl` Co-authored-by: Dario Nieuwenhuis <[email protected]>

Dirbaio requested a review from a team as a code owner January 12, 2022 11:53

rust-highfive assigned ryankurte Jan 12, 2022

rust-highfive added S-waiting-on-review Review is incomplete T-hal labels Jan 12, 2022

Dirbaio force-pushed the async-spi branch from 68cd1be to 6013187 Compare January 12, 2022 11:54

Dirbaio mentioned this pull request Jan 12, 2022

Async #334

Closed

3 tasks

eldruin mentioned this pull request Feb 10, 2022

Asynchronous embedded-hal traits roadmap #356

Closed

8 tasks

Dirbaio mentioned this pull request Feb 14, 2022

async/i2c: fix lifetimes on transaction() #363

Merged

Dirbaio force-pushed the async-spi branch from 6013187 to e0a30b5 Compare February 16, 2022 02:06

Dirbaio force-pushed the async-spi branch from e0a30b5 to 7288d56 Compare February 18, 2022 19:33

ryankurte reviewed Feb 25, 2022

View reviewed changes

embedded-hal-async/src/spi.rs Show resolved Hide resolved

ryankurte reviewed Feb 25, 2022

View reviewed changes

Dirbaio force-pushed the async-spi branch from 7288d56 to 4cf35b5 Compare March 1, 2022 22:49

eldruin added the no changelog label Mar 2, 2022

eldruin previously approved these changes Mar 2, 2022

View reviewed changes

Dirbaio dismissed eldruin’s stale review via af94a7d March 10, 2022 05:14

Dirbaio force-pushed the async-spi branch from 4cf35b5 to af94a7d Compare March 10, 2022 05:14

eldruin previously approved these changes Mar 10, 2022

View reviewed changes

Dirbaio added 2 commits March 10, 2022 17:27

async: add SPI.

ad66fa9

spi: Small doc fixes

62b177c

Dirbaio dismissed eldruin’s stale review via 62b177c March 10, 2022 16:27

Dirbaio force-pushed the async-spi branch from af94a7d to 62b177c Compare March 10, 2022 16:27

eldruin approved these changes Mar 10, 2022

View reviewed changes

bors bot merged commit b5c29b3 into rust-embedded:master Mar 10, 2022

Dirbaio mentioned this pull request Apr 26, 2022

async/spi: replace the "give back the Bus" hack with a raw pointer. #380

Merged

Dirbaio mentioned this pull request Jul 31, 2023

How to maintain an SPI transaction while manipulating GPIO pins? #478

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

async: add SPI #347

async: add SPI #347

Dirbaio commented Jan 12, 2022 •

edited

Loading

rust-highfive commented Jan 12, 2022

Dirbaio commented Feb 16, 2022

Dirbaio commented Feb 18, 2022

ryankurte left a comment •

edited

Loading

Dirbaio commented Feb 25, 2022

Dirbaio commented Mar 1, 2022

eldruin left a comment

ryankurte commented Mar 2, 2022

Dirbaio commented Mar 10, 2022

eldruin left a comment

eldruin left a comment

bors bot commented Mar 10, 2022

async: add SPI #347

async: add SPI #347

Conversation

Dirbaio commented Jan 12, 2022 • edited Loading

rust-highfive commented Jan 12, 2022

Dirbaio commented Feb 16, 2022

Dirbaio commented Feb 18, 2022

ryankurte left a comment • edited Loading

Choose a reason for hiding this comment

Dirbaio commented Feb 25, 2022

Dirbaio commented Mar 1, 2022

eldruin left a comment

Choose a reason for hiding this comment

ryankurte commented Mar 2, 2022

Dirbaio commented Mar 10, 2022

eldruin left a comment

Choose a reason for hiding this comment

eldruin left a comment

Choose a reason for hiding this comment

bors bot commented Mar 10, 2022

Dirbaio commented Jan 12, 2022 •

edited

Loading

ryankurte left a comment •

edited

Loading