Skip to content

Conversation

Dirbaio
Copy link
Member

@Dirbaio Dirbaio commented Apr 26, 2022

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

@Dirbaio Dirbaio requested a review from a team as a code owner April 26, 2022 18:12
@rust-highfive
Copy link

r? @ryankurte

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

@ryankurte
Copy link
Contributor

ryankurte commented Apr 26, 2022

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
Copy link
Member Author

Dirbaio commented Apr 26, 2022

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<..>>?

Copy link
Contributor

@ryankurte ryankurte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe you're confusing it with Arc<Mutex<..>>?

riight, yep that's it. this feels like something we should be able to solve without pointers but, i can't see how rn so lgtm!

bors r+

@bors bors bot merged commit 58587e8 into rust-embedded:master Apr 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Review is incomplete T-hal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants