Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

e-h-bus/spi: Require infallible chipselect pins #574

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion embedded-hal-bus/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
52 changes: 52 additions & 0 deletions embedded-hal-bus/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,3 +18,55 @@ 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.
///
/// # 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<Error = Infallible>) { /* ... */ }
///
/// 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<T>(pub T);

impl<T> embedded_hal::digital::ErrorType for UnwrappingAdapter<T> {
type Error = core::convert::Infallible;
}

impl<T> embedded_hal::digital::OutputPin for UnwrappingAdapter<T>
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(())
}
}
11 changes: 7 additions & 4 deletions embedded-hal-bus/src/spi/critical_section.rs
Original file line number Diff line number Diff line change
@@ -1,17 +1,20 @@
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.
///
/// This allows for sharing an [`SpiBus`], obtaining multiple [`SpiDevice`] instances,
/// each with its own `CS` pin.
///
/// The `CS` pin must be infallible (`CS: OutputPin<Error = Infallible>`) 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
Expand Down Expand Up @@ -61,15 +64,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<Error = Infallible>,
{
type Error = DeviceError<BUS::Error, CS::Error>;
type Error = BUS::Error;
}

impl<'a, Word: Copy + 'static, BUS, CS, D> SpiDevice<Word> for CriticalSectionDevice<'a, BUS, CS, D>
where
BUS: SpiBus<Word>,
CS: OutputPin,
CS: OutputPin<Error = Infallible>,
D: DelayNs,
{
#[inline]
Expand Down
26 changes: 14 additions & 12 deletions embedded-hal-bus/src/spi/exclusive.rs
Original file line number Diff line number Diff line change
@@ -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};
Expand All @@ -10,12 +12,14 @@ use embedded_hal_async::{
};

use super::shared::transaction;
use super::DeviceError;

/// [`SpiDevice`] implementation with exclusive access to the bus (not shared).
///
/// 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<Error = Infallible>`) 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, CS, D> {
bus: BUS,
cs: CS,
Expand Down Expand Up @@ -72,15 +76,15 @@ impl<BUS, CS> ExclusiveDevice<BUS, CS, super::NoDelay> {
impl<BUS, CS, D> ErrorType for ExclusiveDevice<BUS, CS, D>
where
BUS: ErrorType,
CS: OutputPin,
CS: OutputPin<Error = Infallible>,
{
type Error = DeviceError<BUS::Error, CS::Error>;
type Error = BUS::Error;
}

impl<Word: Copy + 'static, BUS, CS, D> SpiDevice<Word> for ExclusiveDevice<BUS, CS, D>
where
BUS: SpiBus<Word>,
CS: OutputPin,
CS: OutputPin<Error = Infallible>,
Copy link
Member

Choose a reason for hiding this comment

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

Is this change really needed for ExclusiveDevice? The scenario described in #573 doesn't apply here, as the bus isn't shared in this case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The issue is less severe with ExclusiveDevice, but there is still a problem. The SpiDevice trait says that all transactions must start by asserting the chipselect (a falling edge), which isn't possible if it is already asserted. This is important for devices that use chipselect for framing. In practice it is possible to work around the issue in this case (always de-assert cs again if you get any kind of DeviceError) but it is a pretty big footgun

D: DelayNs,
{
#[inline]
Expand All @@ -94,15 +98,17 @@ where
impl<Word: Copy + 'static, BUS, CS, D> AsyncSpiDevice<Word> for ExclusiveDevice<BUS, CS, D>
where
BUS: AsyncSpiBus<Word>,
CS: OutputPin,
CS: OutputPin<Error = Infallible>,
D: AsyncDelayNs,
{
#[inline]
async fn transaction(
&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 {
Expand All @@ -128,12 +134,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)
}
}
39 changes: 1 addition & 38 deletions embedded-hal-bus/src/spi/mod.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
//! `SpiDevice` implementations.

use core::fmt::{self, Debug, Display, Formatter};
use embedded_hal::spi::{Error, ErrorKind};
use core::fmt::Debug;

mod exclusive;
pub use exclusive::*;
Expand All @@ -19,42 +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<BUS, CS> {
/// An inner SPI bus operation failed.
Spi(BUS),
/// Asserting or deasserting CS failed.
Cs(CS),
}

impl<BUS: Display, CS: Display> Display for DeviceError<BUS, CS> {
fn fmt(&self, f: &mut Formatter) -> fmt::Result {
match self {
Self::Spi(bus) => write!(f, "SPI bus error: {}", bus),
Self::Cs(cs) => write!(f, "SPI CS error: {}", cs),
}
}
}

#[cfg(feature = "std")]
impl<BUS: Debug + Display, CS: Debug + Display> std::error::Error for DeviceError<BUS, CS> {}

impl<BUS, CS> Error for DeviceError<BUS, CS>
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))]
Expand Down
12 changes: 8 additions & 4 deletions embedded-hal-bus/src/spi/mutex.rs
Original file line number Diff line number Diff line change
@@ -1,16 +1,20 @@
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.
///
/// This allows for sharing an [`SpiBus`], obtaining multiple [`SpiDevice`] instances,
/// each with its own `CS` pin.
///
/// The `CS` pin must be infallible (`CS: OutputPin<Error = Infallible>`) 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.
Expand Down Expand Up @@ -59,15 +63,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<Error = Infallible>,
{
type Error = DeviceError<BUS::Error, CS::Error>;
type Error = BUS::Error;
}

impl<'a, Word: Copy + 'static, BUS, CS, D> SpiDevice<Word> for MutexDevice<'a, BUS, CS, D>
where
BUS: SpiBus<Word>,
CS: OutputPin,
CS: OutputPin<Error = Infallible>,
D: DelayNs,
{
#[inline]
Expand Down
11 changes: 7 additions & 4 deletions embedded-hal-bus/src/spi/refcell.rs
Original file line number Diff line number Diff line change
@@ -1,16 +1,19 @@
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.
///
/// This allows for sharing an [`SpiBus`], obtaining multiple [`SpiDevice`] instances,
/// each with its own `CS` pin.
///
/// The `CS` pin must be infallible (`CS: OutputPin<Error = Infallible>`) 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.
Expand Down Expand Up @@ -58,15 +61,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<Error = Infallible>,
{
type Error = DeviceError<BUS::Error, CS::Error>;
type Error = BUS::Error;
}

impl<'a, Word: Copy + 'static, BUS, CS, D> SpiDevice<Word> for RefCellDevice<'a, BUS, CS, D>
where
BUS: SpiBus<Word>,
CS: OutputPin,
CS: OutputPin<Error = Infallible>,
D: DelayNs,
{
#[inline]
Expand Down
24 changes: 14 additions & 10 deletions embedded-hal-bus/src/spi/shared.rs
Original file line number Diff line number Diff line change
@@ -1,24 +1,24 @@
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<Word, BUS, CS, D>(
operations: &mut [Operation<Word>],
bus: &mut BUS,
delay: &mut D,
cs: &mut CS,
) -> Result<(), DeviceError<BUS::Error, CS::Error>>
) -> Result<(), BUS::Error>
where
BUS: SpiBus<Word> + ErrorType,
CS: OutputPin,
CS: OutputPin<Error = Infallible>,
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),
Expand All @@ -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<T>(res: Result<T, Infallible>) -> T {
match res {
Ok(t) => t,
Err(infallible) => match infallible {},
}
}
Loading