From 64ba1c328992097d617f2068196566d821efb7fb Mon Sep 17 00:00:00 2001 From: Grant Miller Date: Tue, 23 Jan 2024 14:52:43 -0600 Subject: [PATCH 1/3] Require infallible chipselect pins --- embedded-hal-bus/src/spi/critical_section.rs | 8 +++---- embedded-hal-bus/src/spi/exclusive.rs | 23 +++++++++--------- embedded-hal-bus/src/spi/mod.rs | 25 -------------------- embedded-hal-bus/src/spi/mutex.rs | 9 +++---- embedded-hal-bus/src/spi/refcell.rs | 8 +++---- embedded-hal-bus/src/spi/shared.rs | 24 +++++++++++-------- 6 files changed, 38 insertions(+), 59 deletions(-) diff --git a/embedded-hal-bus/src/spi/critical_section.rs b/embedded-hal-bus/src/spi/critical_section.rs index 838be4f5a..2837d1da9 100644 --- a/embedded-hal-bus/src/spi/critical_section.rs +++ b/embedded-hal-bus/src/spi/critical_section.rs @@ -1,10 +1,10 @@ use core::cell::RefCell; +use core::convert::Infallible; use critical_section::Mutex; use embedded_hal::delay::DelayNs; use embedded_hal::digital::OutputPin; use embedded_hal::spi::{ErrorType, Operation, SpiBus, SpiDevice}; -use super::DeviceError; use crate::spi::shared::transaction; /// `critical-section`-based shared bus [`SpiDevice`] implementation. @@ -61,15 +61,15 @@ impl<'a, BUS, CS> CriticalSectionDevice<'a, BUS, CS, super::NoDelay> { impl<'a, BUS, CS, D> ErrorType for CriticalSectionDevice<'a, BUS, CS, D> where BUS: ErrorType, - CS: OutputPin, + CS: OutputPin, { - type Error = DeviceError; + type Error = BUS::Error; } impl<'a, Word: Copy + 'static, BUS, CS, D> SpiDevice for CriticalSectionDevice<'a, BUS, CS, D> where BUS: SpiBus, - CS: OutputPin, + CS: OutputPin, D: DelayNs, { #[inline] diff --git a/embedded-hal-bus/src/spi/exclusive.rs b/embedded-hal-bus/src/spi/exclusive.rs index f28a10af4..db23db600 100644 --- a/embedded-hal-bus/src/spi/exclusive.rs +++ b/embedded-hal-bus/src/spi/exclusive.rs @@ -1,5 +1,7 @@ //! SPI bus sharing mechanisms. +use core::convert::Infallible; + use embedded_hal::delay::DelayNs; use embedded_hal::digital::OutputPin; use embedded_hal::spi::{ErrorType, Operation, SpiBus, SpiDevice}; @@ -10,7 +12,6 @@ use embedded_hal_async::{ }; use super::shared::transaction; -use super::DeviceError; /// [`SpiDevice`] implementation with exclusive access to the bus (not shared). /// @@ -72,15 +73,15 @@ impl ExclusiveDevice { impl ErrorType for ExclusiveDevice where BUS: ErrorType, - CS: OutputPin, + CS: OutputPin, { - type Error = DeviceError; + type Error = BUS::Error; } impl SpiDevice for ExclusiveDevice where BUS: SpiBus, - CS: OutputPin, + CS: OutputPin, D: DelayNs, { #[inline] @@ -94,7 +95,7 @@ where impl AsyncSpiDevice for ExclusiveDevice where BUS: AsyncSpiBus, - CS: OutputPin, + CS: OutputPin, D: AsyncDelayNs, { #[inline] @@ -102,7 +103,9 @@ where &mut self, operations: &mut [Operation<'_, Word>], ) -> Result<(), Self::Error> { - self.cs.set_low().map_err(DeviceError::Cs)?; + use crate::spi::shared::into_ok; + + into_ok(self.cs.set_low()); let op_res = 'ops: { for op in operations { @@ -128,12 +131,8 @@ where // On failure, it's important to still flush and deassert CS. let flush_res = self.bus.flush().await; - let cs_res = self.cs.set_high(); - - op_res.map_err(DeviceError::Spi)?; - flush_res.map_err(DeviceError::Spi)?; - cs_res.map_err(DeviceError::Cs)?; + into_ok(self.cs.set_high()); - Ok(()) + op_res.and(flush_res) } } diff --git a/embedded-hal-bus/src/spi/mod.rs b/embedded-hal-bus/src/spi/mod.rs index 8b51242d8..d1928f686 100644 --- a/embedded-hal-bus/src/spi/mod.rs +++ b/embedded-hal-bus/src/spi/mod.rs @@ -1,7 +1,6 @@ //! `SpiDevice` implementations. use core::fmt::Debug; -use embedded_hal::spi::{Error, ErrorKind}; mod exclusive; pub use exclusive::*; @@ -19,30 +18,6 @@ pub use self::critical_section::*; #[cfg(feature = "defmt-03")] use crate::defmt; -/// Error type for [`ExclusiveDevice`] operations. -#[derive(Copy, Clone, Eq, PartialEq, Debug)] -#[cfg_attr(feature = "defmt-03", derive(defmt::Format))] -pub enum DeviceError { - /// An inner SPI bus operation failed. - Spi(BUS), - /// Asserting or deasserting CS failed. - Cs(CS), -} - -impl Error for DeviceError -where - BUS: Error + Debug, - CS: Debug, -{ - #[inline] - fn kind(&self) -> ErrorKind { - match self { - Self::Spi(e) => e.kind(), - Self::Cs(_) => ErrorKind::ChipSelectFault, - } - } -} - /// Dummy [`DelayNs`](embedded_hal::delay::DelayNs) implementation that panics on use. #[derive(Copy, Clone, Eq, PartialEq, Debug)] #[cfg_attr(feature = "defmt-03", derive(defmt::Format))] diff --git a/embedded-hal-bus/src/spi/mutex.rs b/embedded-hal-bus/src/spi/mutex.rs index b06f3fd95..0ee2f664a 100644 --- a/embedded-hal-bus/src/spi/mutex.rs +++ b/embedded-hal-bus/src/spi/mutex.rs @@ -1,9 +1,10 @@ +use core::convert::Infallible; + use embedded_hal::delay::DelayNs; use embedded_hal::digital::OutputPin; use embedded_hal::spi::{ErrorType, Operation, SpiBus, SpiDevice}; use std::sync::Mutex; -use super::DeviceError; use crate::spi::shared::transaction; /// `std` `Mutex`-based shared bus [`SpiDevice`] implementation. @@ -59,15 +60,15 @@ impl<'a, BUS, CS> MutexDevice<'a, BUS, CS, super::NoDelay> { impl<'a, BUS, CS, D> ErrorType for MutexDevice<'a, BUS, CS, D> where BUS: ErrorType, - CS: OutputPin, + CS: OutputPin, { - type Error = DeviceError; + type Error = BUS::Error; } impl<'a, Word: Copy + 'static, BUS, CS, D> SpiDevice for MutexDevice<'a, BUS, CS, D> where BUS: SpiBus, - CS: OutputPin, + CS: OutputPin, D: DelayNs, { #[inline] diff --git a/embedded-hal-bus/src/spi/refcell.rs b/embedded-hal-bus/src/spi/refcell.rs index 382591c34..e360924e1 100644 --- a/embedded-hal-bus/src/spi/refcell.rs +++ b/embedded-hal-bus/src/spi/refcell.rs @@ -1,9 +1,9 @@ use core::cell::RefCell; +use core::convert::Infallible; use embedded_hal::delay::DelayNs; use embedded_hal::digital::OutputPin; use embedded_hal::spi::{ErrorType, Operation, SpiBus, SpiDevice}; -use super::DeviceError; use crate::spi::shared::transaction; /// `RefCell`-based shared bus [`SpiDevice`] implementation. @@ -58,15 +58,15 @@ impl<'a, BUS, CS> RefCellDevice<'a, BUS, CS, super::NoDelay> { impl<'a, BUS, CS, D> ErrorType for RefCellDevice<'a, BUS, CS, D> where BUS: ErrorType, - CS: OutputPin, + CS: OutputPin, { - type Error = DeviceError; + type Error = BUS::Error; } impl<'a, Word: Copy + 'static, BUS, CS, D> SpiDevice for RefCellDevice<'a, BUS, CS, D> where BUS: SpiBus, - CS: OutputPin, + CS: OutputPin, D: DelayNs, { #[inline] diff --git a/embedded-hal-bus/src/spi/shared.rs b/embedded-hal-bus/src/spi/shared.rs index 95730ba19..d3e7aa292 100644 --- a/embedded-hal-bus/src/spi/shared.rs +++ b/embedded-hal-bus/src/spi/shared.rs @@ -1,9 +1,9 @@ +use core::convert::Infallible; + use embedded_hal::delay::DelayNs; use embedded_hal::digital::OutputPin; use embedded_hal::spi::{ErrorType, Operation, SpiBus}; -use crate::spi::DeviceError; - /// Common implementation to perform a transaction against the device. #[inline] pub fn transaction( @@ -11,14 +11,14 @@ pub fn transaction( bus: &mut BUS, delay: &mut D, cs: &mut CS, -) -> Result<(), DeviceError> +) -> Result<(), BUS::Error> where BUS: SpiBus + ErrorType, - CS: OutputPin, + CS: OutputPin, D: DelayNs, Word: Copy, { - cs.set_low().map_err(DeviceError::Cs)?; + into_ok(cs.set_low()); let op_res = operations.iter_mut().try_for_each(|op| match op { Operation::Read(buf) => bus.read(buf), @@ -34,11 +34,15 @@ where // On failure, it's important to still flush and deassert CS. let flush_res = bus.flush(); - let cs_res = cs.set_high(); + into_ok(cs.set_high()); - op_res.map_err(DeviceError::Spi)?; - flush_res.map_err(DeviceError::Spi)?; - cs_res.map_err(DeviceError::Cs)?; + op_res.and(flush_res) +} - Ok(()) +/// see https://github.com/rust-lang/rust/issues/61695 +pub(crate) fn into_ok(res: Result) -> T { + match res { + Ok(t) => t, + Err(infallible) => match infallible {}, + } } From 03e978401f4be5910a37fa10269da6ef93c72971 Mon Sep 17 00:00:00 2001 From: Grant Miller Date: Tue, 23 Jan 2024 15:04:42 -0600 Subject: [PATCH 2/3] Add an unwrapping adapter --- embedded-hal-bus/src/lib.rs | 35 +++++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/embedded-hal-bus/src/lib.rs b/embedded-hal-bus/src/lib.rs index e7e75ec76..b6cf9386a 100644 --- a/embedded-hal-bus/src/lib.rs +++ b/embedded-hal-bus/src/lib.rs @@ -18,3 +18,38 @@ use defmt_03 as defmt; pub mod i2c; pub mod spi; + +/// This adapter will [panic] if the inner device encounters an error. +/// +/// It currently supports [embedded_hal::digital::OutputPin], but other traits may be added in the future. +/// +/// TODO: add usage example +#[repr(transparent)] +pub struct UnwrappingAdapter(pub T); + +impl embedded_hal::digital::ErrorType for UnwrappingAdapter { + type Error = core::convert::Infallible; +} + +impl embedded_hal::digital::OutputPin for UnwrappingAdapter +where + T: embedded_hal::digital::OutputPin, +{ + #[inline] + fn set_low(&mut self) -> Result<(), Self::Error> { + self.0.set_low().unwrap(); + Ok(()) + } + + #[inline] + fn set_high(&mut self) -> Result<(), Self::Error> { + self.0.set_high().unwrap(); + Ok(()) + } + + #[inline] + fn set_state(&mut self, state: embedded_hal::digital::PinState) -> Result<(), Self::Error> { + self.0.set_state(state).unwrap(); + Ok(()) + } +} From 4ff05acc11e31cdceca7ff72ab7848bb73edc895 Mon Sep 17 00:00:00 2001 From: Grant Miller Date: Tue, 30 Jan 2024 15:07:31 -0600 Subject: [PATCH 3/3] Update docs and changelog --- embedded-hal-bus/CHANGELOG.md | 3 ++- embedded-hal-bus/src/lib.rs | 19 ++++++++++++++++++- embedded-hal-bus/src/spi/critical_section.rs | 3 +++ embedded-hal-bus/src/spi/exclusive.rs | 3 +++ embedded-hal-bus/src/spi/mutex.rs | 3 +++ embedded-hal-bus/src/spi/refcell.rs | 3 +++ 6 files changed, 32 insertions(+), 2 deletions(-) diff --git a/embedded-hal-bus/CHANGELOG.md b/embedded-hal-bus/CHANGELOG.md index 04de7aa69..0e815ec0f 100644 --- a/embedded-hal-bus/CHANGELOG.md +++ b/embedded-hal-bus/CHANGELOG.md @@ -7,7 +7,8 @@ and this project adheres to [Semantic Versioning](http://semver.org/). ## [Unreleased] -No unreleased changes +- Require infallible chipselects for all spi utilities +- Add `UnwrappingAdapter` ## [v0.1.0] - 2023-12-28 diff --git a/embedded-hal-bus/src/lib.rs b/embedded-hal-bus/src/lib.rs index b6cf9386a..5cde1ed34 100644 --- a/embedded-hal-bus/src/lib.rs +++ b/embedded-hal-bus/src/lib.rs @@ -23,7 +23,24 @@ pub mod spi; /// /// It currently supports [embedded_hal::digital::OutputPin], but other traits may be added in the future. /// -/// TODO: add usage example +/// # Example +/// +/// ``` +/// use core::convert::Infallible; +/// use embedded_hal::digital::OutputPin; +/// use embedded_hal_bus::UnwrappingAdapter; +/// +/// /// This could be any function or struct that requires an infallible output pin +/// fn requires_infallible(output: impl OutputPin) { /* ... */ } +/// +/// fn accepts_fallible(output: impl OutputPin) { +/// // this wouldn't work: +/// // requires_infallible(output); +/// +/// let unwrapping_output = UnwrappingAdapter(output); +/// requires_infallible(unwrapping_output); +/// } +/// ``` #[repr(transparent)] pub struct UnwrappingAdapter(pub T); diff --git a/embedded-hal-bus/src/spi/critical_section.rs b/embedded-hal-bus/src/spi/critical_section.rs index 2837d1da9..32571cc1d 100644 --- a/embedded-hal-bus/src/spi/critical_section.rs +++ b/embedded-hal-bus/src/spi/critical_section.rs @@ -12,6 +12,9 @@ use crate::spi::shared::transaction; /// This allows for sharing an [`SpiBus`], obtaining multiple [`SpiDevice`] instances, /// each with its own `CS` pin. /// +/// The `CS` pin must be infallible (`CS: OutputPin`) because proper error handling would be complicated +/// and it's usually not needed. If you are using a fallible `CS` pin, you can use [UnwrappingAdapter](crate::UnwrappingAdapter). +/// /// Sharing is implemented with a `critical-section` [`Mutex`]. A critical section is taken for /// the entire duration of a transaction. This allows sharing a single bus across multiple threads (interrupt priority levels). /// The downside is critical sections typically require globally disabling interrupts, so `CriticalSectionDevice` will likely diff --git a/embedded-hal-bus/src/spi/exclusive.rs b/embedded-hal-bus/src/spi/exclusive.rs index db23db600..4cdcb77c2 100644 --- a/embedded-hal-bus/src/spi/exclusive.rs +++ b/embedded-hal-bus/src/spi/exclusive.rs @@ -17,6 +17,9 @@ use super::shared::transaction; /// /// This is the most straightforward way of obtaining an [`SpiDevice`] from an [`SpiBus`], /// ideal for when no sharing is required (only one SPI device is present on the bus). +/// +/// The `CS` pin must be infallible (`CS: OutputPin`) because proper error handling would be complicated +/// and it's usually not needed. If you are using a fallible `CS` pin, you can use [UnwrappingAdapter](crate::UnwrappingAdapter). pub struct ExclusiveDevice { bus: BUS, cs: CS, diff --git a/embedded-hal-bus/src/spi/mutex.rs b/embedded-hal-bus/src/spi/mutex.rs index 0ee2f664a..08a60b274 100644 --- a/embedded-hal-bus/src/spi/mutex.rs +++ b/embedded-hal-bus/src/spi/mutex.rs @@ -12,6 +12,9 @@ use crate::spi::shared::transaction; /// This allows for sharing an [`SpiBus`], obtaining multiple [`SpiDevice`] instances, /// each with its own `CS` pin. /// +/// The `CS` pin must be infallible (`CS: OutputPin`) because proper error handling would be complicated +/// and it's usually not needed. If you are using a fallible `CS` pin, you can use [UnwrappingAdapter](crate::UnwrappingAdapter). +/// /// Sharing is implemented with a `std` [`Mutex`]. It allows a single bus across multiple threads, /// with finer-grained locking than [`CriticalSectionDevice`](super::CriticalSectionDevice). The downside is /// it is only available in `std` targets. diff --git a/embedded-hal-bus/src/spi/refcell.rs b/embedded-hal-bus/src/spi/refcell.rs index e360924e1..62d99befe 100644 --- a/embedded-hal-bus/src/spi/refcell.rs +++ b/embedded-hal-bus/src/spi/refcell.rs @@ -11,6 +11,9 @@ use crate::spi::shared::transaction; /// This allows for sharing an [`SpiBus`], obtaining multiple [`SpiDevice`] instances, /// each with its own `CS` pin. /// +/// The `CS` pin must be infallible (`CS: OutputPin`) because proper error handling would be complicated +/// and it's usually not needed. If you are using a fallible `CS` pin, you can use [UnwrappingAdapter](crate::UnwrappingAdapter). +/// /// Sharing is implemented with a `RefCell`. This means it has low overhead, but `RefCellDevice` instances are not `Send`, /// so it only allows sharing within a single thread (interrupt priority level). If you need to share a bus across several /// threads, use [`CriticalSectionDevice`](super::CriticalSectionDevice) instead.