Skip to content

Commit

Permalink
Merge #380
Browse files Browse the repository at this point in the history
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]>
  • Loading branch information
bors[bot] and Dirbaio authored Apr 27, 2022
2 parents dd7bd7f + 8cf16a7 commit 58587e8
Showing 1 changed file with 30 additions and 42 deletions.
72 changes: 30 additions & 42 deletions embedded-hal-async/src/spi.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,13 +22,8 @@ pub trait SpiDevice: ErrorType {
where
Self: 'a,
R: 'a,
F: FnOnce(&'a mut Self::Bus) -> Fut + 'a,
Fut: Future<
Output = (
&'a mut Self::Bus,
Result<R, <Self::Bus as ErrorType>::Error>,
),
> + 'a;
F: FnOnce(*mut Self::Bus) -> Fut + 'a,
Fut: Future<Output = Result<R, <Self::Bus as ErrorType>::Error>> + 'a;

/// Perform a transaction against the device.
///
Expand All @@ -42,15 +37,14 @@ pub trait SpiDevice: ErrorType {
/// The locking mechanism is implementation-defined. The only requirement is it must prevent two
/// transactions from executing concurrently against the same bus. Examples of implementations are:
/// critical sections, blocking mutexes, async mutexes, returning an error or panicking if the bus is already busy.
///
/// The current state of the Rust typechecker doesn't allow expressing the necessary lifetime constraints, so
/// the `f` closure receives a lifetime-less `*mut Bus` raw pointer instead. The pointer is guaranteed
/// to be valid for the entire duration the closure is running, so dereferencing it is safe.
fn transaction<'a, R, F, Fut>(&'a mut self, f: F) -> Self::TransactionFuture<'a, R, F, Fut>
where
F: FnOnce(&'a mut Self::Bus) -> Fut + 'a,
Fut: Future<
Output = (
&'a mut Self::Bus,
Result<R, <Self::Bus as ErrorType>::Error>,
),
> + 'a;
F: FnOnce(*mut Self::Bus) -> Fut + 'a,
Fut: Future<Output = Result<R, <Self::Bus as ErrorType>::Error>> + 'a;
}

/// Helper methods for SpiDevice.
Expand Down Expand Up @@ -147,8 +141,9 @@ impl<T: SpiDevice> SpiDeviceExt for T {
Word: Copy + 'static,
{
self.transaction(move |bus| async move {
let res = bus.read(buf).await;
(bus, res)
// safety: `bus` is a valid pointer we're allowed to use for the duration of the closure.
let bus = unsafe { &mut *bus };
bus.read(buf).await
})
}

Expand All @@ -164,8 +159,9 @@ impl<T: SpiDevice> SpiDeviceExt for T {
Word: Copy + 'static,
{
self.transaction(move |bus| async move {
let res = bus.write(buf).await;
(bus, res)
// safety: `bus` is a valid pointer we're allowed to use for the duration of the closure.
let bus = unsafe { &mut *bus };
bus.write(buf).await
})
}

Expand All @@ -185,8 +181,9 @@ impl<T: SpiDevice> SpiDeviceExt for T {
Word: Copy + 'static,
{
self.transaction(move |bus| async move {
let res = bus.transfer(read, write).await;
(bus, res)
// safety: `bus` is a valid pointer we're allowed to use for the duration of the closure.
let bus = unsafe { &mut *bus };
bus.transfer(read, write).await
})
}

Expand All @@ -205,8 +202,9 @@ impl<T: SpiDevice> SpiDeviceExt for T {
Word: Copy + 'static,
{
self.transaction(move |bus| async move {
let res = bus.transfer_in_place(buf).await;
(bus, res)
// safety: `bus` is a valid pointer we're allowed to use for the duration of the closure.
let bus = unsafe { &mut *bus };
bus.transfer_in_place(buf).await
})
}
}
Expand All @@ -216,18 +214,13 @@ impl<T: SpiDevice> SpiDevice for &mut T {

type TransactionFuture<'a, R, F, Fut> = T::TransactionFuture<'a, R, F, Fut>
where
Self: 'a, R: 'a, F: FnOnce(&'a mut Self::Bus) -> Fut + 'a,
Fut: Future<Output = (&'a mut Self::Bus, Result<R, <Self::Bus as ErrorType>::Error>)> + 'a;
Self: 'a, R: 'a, F: FnOnce(*mut Self::Bus) -> Fut + 'a,
Fut: Future<Output = Result<R, <Self::Bus as ErrorType>::Error>> + 'a;

fn transaction<'a, R, F, Fut>(&'a mut self, f: F) -> Self::TransactionFuture<'a, R, F, Fut>
where
F: FnOnce(&'a mut Self::Bus) -> Fut + 'a,
Fut: Future<
Output = (
&'a mut Self::Bus,
Result<R, <Self::Bus as ErrorType>::Error>,
),
> + 'a,
F: FnOnce(*mut Self::Bus) -> Fut + 'a,
Fut: Future<Output = Result<R, <Self::Bus as ErrorType>::Error>> + 'a,
{
T::transaction(self, f)
}
Expand Down Expand Up @@ -449,27 +442,22 @@ where

type TransactionFuture<'a, R, F, Fut> = impl Future<Output = Result<R, Self::Error>> + 'a
where
Self: 'a, R: 'a, F: FnOnce(&'a mut Self::Bus) -> Fut + 'a,
Fut: Future<Output = (&'a mut Self::Bus, Result<R, <Self::Bus as ErrorType>::Error>)> + 'a;
Self: 'a, R: 'a, F: FnOnce(*mut Self::Bus) -> Fut + 'a,
Fut: Future<Output = Result<R, <Self::Bus as ErrorType>::Error>> + 'a;

fn transaction<'a, R, F, Fut>(&'a mut self, f: F) -> Self::TransactionFuture<'a, R, F, Fut>
where
R: 'a,
F: FnOnce(&'a mut Self::Bus) -> Fut + 'a,
Fut: Future<
Output = (
&'a mut Self::Bus,
Result<R, <Self::Bus as ErrorType>::Error>,
),
> + 'a,
F: FnOnce(*mut Self::Bus) -> Fut + 'a,
Fut: Future<Output = Result<R, <Self::Bus as ErrorType>::Error>> + 'a,
{
async move {
self.cs.set_low().map_err(ExclusiveDeviceError::Cs)?;

let (bus, f_res) = f(&mut self.bus).await;
let f_res = f(&mut self.bus).await;

// On failure, it's important to still flush and deassert CS.
let flush_res = bus.flush().await;
let flush_res = self.bus.flush().await;
let cs_res = self.cs.set_high();

let f_res = f_res.map_err(ExclusiveDeviceError::Spi)?;
Expand Down

0 comments on commit 58587e8

Please sign in to comment.