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 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
35 changes: 35 additions & 0 deletions embedded-hal-bus/src/lib.rs
Expand Up @@ -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<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(())
}
}
8 changes: 4 additions & 4 deletions 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.
Expand Down Expand Up @@ -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<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
23 changes: 11 additions & 12 deletions 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};
Expand All @@ -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).
///
Expand Down Expand Up @@ -72,15 +73,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
Contributor

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 +95,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 +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)
}
}
25 changes: 0 additions & 25 deletions 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::*;
Expand All @@ -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<BUS, CS> {
/// An inner SPI bus operation failed.
Spi(BUS),
/// Asserting or deasserting CS failed.
Cs(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
9 changes: 5 additions & 4 deletions 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.
Expand Down Expand Up @@ -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<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
8 changes: 4 additions & 4 deletions 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.
Expand Down Expand Up @@ -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<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
@@ -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 {},
}
}