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

Dirbaio commented 2 years ago

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

rust-highfive commented 2 years ago

r? @ryankurte

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

ryankurte commented 2 years ago

hmmm interesting, so mutexes in embassy are a singleton with shared references rather than a cheaply cloned object? ime in the async world you'd usually clone the mutex before the async move to avoid the issue, just having a play to see if there are any other strange workarounds.

(one thought is to return an actual transaction object implementing Future with poll over a set of states / containing the refs, but i can't tell if this helps yet)

Dirbaio commented 2 years ago

mutexes in embassy are a singleton with shared references rather than a cheaply cloned object?

It works the same as std or tokio's Mutex. Maybe you're confusing it with Arc<Mutex<..>>?

bors[bot] commented 2 years ago

Build succeeded:

rust-embedded / embedded-hal

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