bus: make AtomicDevice use a per-bus "busy" flag, not per-device.

Needed for soundness.

Author: Dirbaio

embedded-hal-bus/src/i2c/atomic.rs

@@ -1,6 +1,7 @@
-use core::cell::UnsafeCell;
 use embedded_hal::i2c::{Error, ErrorKind, ErrorType, I2c};
 
+use crate::util::AtomicCell;
+
 /// `UnsafeCell`-based shared bus [`I2c`] implementation.
 ///
 /// Sharing is implemented with a `UnsafeCell`. This means it has low overhead, similar to [`crate::i2c::RefCellDevice`] instances, but they are `Send`.
@@ -18,7 +19,7 @@ use embedded_hal::i2c::{Error, ErrorKind, ErrorType, I2c};
 ///
 /// ```
 /// use embedded_hal_bus::i2c;
-/// use core::cell::UnsafeCell;
+/// use embedded_hal_bus::util::AtomicCell;
 /// # use embedded_hal::i2c::{self as hali2c, SevenBitAddress, TenBitAddress, I2c, Operation, ErrorKind};
 /// # pub struct Sensor<I2C> {
 /// #     i2c: I2C,
@@ -56,19 +57,18 @@ use embedded_hal::i2c::{Error, ErrorKind, ErrorType, I2c};
 /// # let hal = Hal;
 ///
 /// let i2c = hal.i2c();
-/// let i2c_unsafe_cell = UnsafeCell::new(i2c);
+/// let i2c_cell = AtomicCell::new(i2c);
 /// let mut temperature_sensor = TemperatureSensor::new(
-///   i2c::AtomicDevice::new(&i2c_unsafe_cell),
+///   i2c::AtomicDevice::new(&i2c_cell),
 ///   0x20,
 /// );
 /// let mut pressure_sensor = PressureSensor::new(
-///   i2c::AtomicDevice::new(&i2c_unsafe_cell),
+///   i2c::AtomicDevice::new(&i2c_cell),
 ///   0x42,
 /// );
 /// ```
 pub struct AtomicDevice<'a, T> {
-    bus: &'a UnsafeCell<T>,
-    busy: portable_atomic::AtomicBool,
+    bus: &'a AtomicCell<T>,
 }
 
 #[derive(Debug, Copy, Clone)]
@@ -100,18 +100,16 @@ where
 {
     /// Create a new `AtomicDevice`.
     #[inline]
-    pub fn new(bus: &'a UnsafeCell<T>) -> Self {
-        Self {
-            bus,
-            busy: portable_atomic::AtomicBool::from(false),
-        }
+    pub fn new(bus: &'a AtomicCell<T>) -> Self {
+        Self { bus }
     }
 
     fn lock<R, F>(&self, f: F) -> Result<R, AtomicError<T::Error>>
     where
         F: FnOnce(&mut T) -> Result<R, <T as ErrorType>::Error>,
     {
-        self.busy
+        self.bus
+            .busy
             .compare_exchange(
                 false,
                 true,
@@ -120,9 +118,11 @@ where
             )
             .map_err(|_| AtomicError::<T::Error>::Busy)?;
 
-        let result = f(unsafe { &mut *self.bus.get() });
+        let result = f(unsafe { &mut *self.bus.bus.get() });
 
-        self.busy.store(false, core::sync::atomic::Ordering::SeqCst);
+        self.bus
+            .busy
+            .store(false, core::sync::atomic::Ordering::SeqCst);
 
         result.map_err(AtomicError::Other)
     }

embedded-hal-bus/src/lib.rs

@@ -18,3 +18,4 @@ use defmt_03 as defmt;
 
 pub mod i2c;
 pub mod spi;
+pub mod util;

embedded-hal-bus/src/spi/atomic.rs

@@ -1,10 +1,10 @@
-use core::cell::UnsafeCell;
 use embedded_hal::delay::DelayNs;
 use embedded_hal::digital::OutputPin;
 use embedded_hal::spi::{Error, ErrorKind, ErrorType, Operation, SpiBus, SpiDevice};
 
 use super::DeviceError;
 use crate::spi::shared::transaction;
+use crate::util::AtomicCell;
 
 /// `UnsafeCell`-based shared bus [`SpiDevice`] implementation.
 ///
@@ -19,10 +19,9 @@ use crate::spi::shared::transaction;
 /// rules, such as the RTIC framework.
 ///
 pub struct AtomicDevice<'a, BUS, CS, D> {
-    bus: &'a UnsafeCell<BUS>,
+    bus: &'a AtomicCell<BUS>,
     cs: CS,
     delay: D,
-    busy: portable_atomic::AtomicBool,
 }
 
 #[derive(Debug, Copy, Clone)]
@@ -40,13 +39,8 @@ pub enum AtomicError<T: Error> {
 impl<'a, BUS, CS, D> AtomicDevice<'a, BUS, CS, D> {
     /// Create a new [`AtomicDevice`].
     #[inline]
-    pub fn new(bus: &'a UnsafeCell<BUS>, cs: CS, delay: D) -> Self {
-        Self {
-            bus,
-            cs,
-            delay,
-            busy: portable_atomic::AtomicBool::from(false),
-        }
+    pub fn new(bus: &'a AtomicCell<BUS>, cs: CS, delay: D) -> Self {
+        Self { bus, cs, delay }
     }
 }
 
@@ -72,18 +66,15 @@ where
     /// The returned device will panic if you try to execute a transaction
     /// that contains any operations of type [`Operation::DelayNs`].
     #[inline]
-    pub fn new_no_delay(bus: &'a UnsafeCell<BUS>, cs: CS) -> Self {
+    pub fn new_no_delay(bus: &'a AtomicCell<BUS>, cs: CS) -> Self {
         Self {
             bus,
             cs,
             delay: super::NoDelay,
-            busy: portable_atomic::AtomicBool::from(false),
         }
     }
 }
 
-unsafe impl<'a, BUS, CS, D> Send for AtomicDevice<'a, BUS, CS, D> {}
-
 impl<T: Error> Error for AtomicError<T> {
     fn kind(&self) -> ErrorKind {
         match self {
@@ -109,7 +100,8 @@ where
 {
     #[inline]
     fn transaction(&mut self, operations: &mut [Operation<'_, Word>]) -> Result<(), Self::Error> {
-        self.busy
+        self.bus
+            .busy
             .compare_exchange(
                 false,
                 true,
@@ -118,11 +110,13 @@ where
             )
             .map_err(|_| AtomicError::Busy)?;
 
-        let bus = unsafe { &mut *self.bus.get() };
+        let bus = unsafe { &mut *self.bus.bus.get() };
 
         let result = transaction(operations, bus, &mut self.delay, &mut self.cs);
 
-        self.busy.store(false, core::sync::atomic::Ordering::SeqCst);
+        self.bus
+            .busy
+            .store(false, core::sync::atomic::Ordering::SeqCst);
 
         result.map_err(AtomicError::Other)
     }

embedded-hal-bus/src/util.rs

@@ -0,0 +1,25 @@
+//! Utilities shared by all bus types.
+
+use core::cell::UnsafeCell;
+
+/// Cell type used by [`spi::AtomicDevice`](crate::spi::AtomicDevice) and [`i2c::AtomicDevice`](crate::i2c::AtomicDevice).
+///
+/// To use `AtomicDevice`, you must wrap the bus with this struct, and then
+/// construct multiple `AtomicDevice` instances with references to it.
+pub struct AtomicCell<BUS> {
+    pub(crate) bus: UnsafeCell<BUS>,
+    pub(crate) busy: portable_atomic::AtomicBool,
+}
+
+unsafe impl<BUS: Send> Send for AtomicCell<BUS> {}
+unsafe impl<BUS: Send> Sync for AtomicCell<BUS> {}
+
+impl<BUS> AtomicCell<BUS> {
+    /// Create a new `AtomicCell`
+    pub fn new(bus: BUS) -> Self {
+        Self {
+            bus: UnsafeCell::new(bus),
+            busy: portable_atomic::AtomicBool::from(false),
+        }
+    }
+}