Merge pull request #594 from rust-embedded/set-cs-high

bus: make constructors set CS high.
rust-embedded · Apr 23, 2024 · 7e3594e · 7e3594e
2 parents 1d33458 + bae4c27
commit 7e3594e
Show file tree

Hide file tree

Showing 6 changed files with 96 additions and 26 deletions.
diff --git a/embedded-hal-bus/CHANGELOG.md b/embedded-hal-bus/CHANGELOG.md
@@ -7,8 +7,8 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
 
 ## [Unreleased]
 
-### Added
 - Added a new `AtomicDevice` for I2C and SPI to enable bus sharing across multiple contexts.
+- SPI shared bus constructors now set `CS` high, to prevent sharing issues if it was low.
 
 ## [v0.1.0] - 2023-12-28
 

diff --git a/embedded-hal-bus/src/spi/atomic.rs b/embedded-hal-bus/src/spi/atomic.rs
@@ -46,9 +46,16 @@ pub enum AtomicError<T: Error> {
 
 impl<'a, BUS, CS, D> AtomicDevice<'a, BUS, CS, D> {
     /// Create a new [`AtomicDevice`].
+    ///
+    /// This sets the `cs` pin high, and returns an error if that fails. It is recommended
+    /// to set the pin high the moment it's configured as an output, to avoid glitches.
     #[inline]
-    pub fn new(bus: &'a AtomicCell<BUS>, cs: CS, delay: D) -> Self {
-        Self { bus, cs, delay }
+    pub fn new(bus: &'a AtomicCell<BUS>, mut cs: CS, delay: D) -> Result<Self, CS::Error>
+    where
+        CS: OutputPin,
+    {
+        cs.set_high()?;
+        Ok(Self { bus, cs, delay })
     }
 }
 
@@ -59,6 +66,9 @@ where
 {
     /// Create a new [`AtomicDevice`] without support for in-transaction delays.
     ///
+    /// This sets the `cs` pin high, and returns an error if that fails. It is recommended
+    /// to set the pin high the moment it's configured as an output, to avoid glitches.
+    ///
     /// **Warning**: The returned instance *technically* doesn't comply with the `SpiDevice`
     /// contract, which mandates delay support. It is relatively rare for drivers to use
     /// in-transaction delays, so you might still want to use this method because it's more practical.
@@ -74,12 +84,16 @@ 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 AtomicCell<BUS>, cs: CS) -> Self {
-        Self {
+    pub fn new_no_delay(bus: &'a AtomicCell<BUS>, mut cs: CS) -> Result<Self, CS::Error>
+    where
+        CS: OutputPin,
+    {
+        cs.set_high()?;
+        Ok(Self {
             bus,
             cs,
             delay: super::NoDelay,
-        }
+        })
     }
 }
 

diff --git a/embedded-hal-bus/src/spi/critical_section.rs b/embedded-hal-bus/src/spi/critical_section.rs
@@ -25,15 +25,25 @@ pub struct CriticalSectionDevice<'a, BUS, CS, D> {
 
 impl<'a, BUS, CS, D> CriticalSectionDevice<'a, BUS, CS, D> {
     /// Create a new [`CriticalSectionDevice`].
+    ///
+    /// This sets the `cs` pin high, and returns an error if that fails. It is recommended
+    /// to set the pin high the moment it's configured as an output, to avoid glitches.
     #[inline]
-    pub fn new(bus: &'a Mutex<RefCell<BUS>>, cs: CS, delay: D) -> Self {
-        Self { bus, cs, delay }
+    pub fn new(bus: &'a Mutex<RefCell<BUS>>, mut cs: CS, delay: D) -> Result<Self, CS::Error>
+    where
+        CS: OutputPin,
+    {
+        cs.set_high()?;
+        Ok(Self { bus, cs, delay })
     }
 }
 
 impl<'a, BUS, CS> CriticalSectionDevice<'a, BUS, CS, super::NoDelay> {
     /// Create a new [`CriticalSectionDevice`] without support for in-transaction delays.
     ///
+    /// This sets the `cs` pin high, and returns an error if that fails. It is recommended
+    /// to set the pin high the moment it's configured as an output, to avoid glitches.
+    ///
     /// **Warning**: The returned instance *technically* doesn't comply with the `SpiDevice`
     /// contract, which mandates delay support. It is relatively rare for drivers to use
     /// in-transaction delays, so you might still want to use this method because it's more practical.
@@ -49,12 +59,16 @@ impl<'a, BUS, CS> CriticalSectionDevice<'a, BUS, CS, super::NoDelay> {
     /// 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 Mutex<RefCell<BUS>>, cs: CS) -> Self {
-        Self {
+    pub fn new_no_delay(bus: &'a Mutex<RefCell<BUS>>, mut cs: CS) -> Result<Self, CS::Error>
+    where
+        CS: OutputPin,
+    {
+        cs.set_high()?;
+        Ok(Self {
             bus,
             cs,
             delay: super::NoDelay,
-        }
+        })
     }
 }
 

diff --git a/embedded-hal-bus/src/spi/exclusive.rs b/embedded-hal-bus/src/spi/exclusive.rs
@@ -24,9 +24,16 @@ pub struct ExclusiveDevice<BUS, CS, D> {
 
 impl<BUS, CS, D> ExclusiveDevice<BUS, CS, D> {
     /// Create a new [`ExclusiveDevice`].
+    ///
+    /// This sets the `cs` pin high, and returns an error if that fails. It is recommended
+    /// to set the pin high the moment it's configured as an output, to avoid glitches.
     #[inline]
-    pub fn new(bus: BUS, cs: CS, delay: D) -> Self {
-        Self { bus, cs, delay }
+    pub fn new(bus: BUS, mut cs: CS, delay: D) -> Result<Self, CS::Error>
+    where
+        CS: OutputPin,
+    {
+        cs.set_high()?;
+        Ok(Self { bus, cs, delay })
     }
 
     /// Returns a reference to the underlying bus object.
@@ -45,6 +52,9 @@ impl<BUS, CS, D> ExclusiveDevice<BUS, CS, D> {
 impl<BUS, CS> ExclusiveDevice<BUS, CS, super::NoDelay> {
     /// Create a new [`ExclusiveDevice`] without support for in-transaction delays.
     ///
+    /// This sets the `cs` pin high, and returns an error if that fails. It is recommended
+    /// to set the pin high the moment it's configured as an output, to avoid glitches.
+    ///
     /// **Warning**: The returned instance *technically* doesn't comply with the `SpiDevice`
     /// contract, which mandates delay support. It is relatively rare for drivers to use
     /// in-transaction delays, so you might still want to use this method because it's more practical.
@@ -60,12 +70,16 @@ impl<BUS, CS> ExclusiveDevice<BUS, CS, super::NoDelay> {
     /// 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: BUS, cs: CS) -> Self {
-        Self {
+    pub fn new_no_delay(bus: BUS, mut cs: CS) -> Result<Self, CS::Error>
+    where
+        CS: OutputPin,
+    {
+        cs.set_high()?;
+        Ok(Self {
             bus,
             cs,
             delay: super::NoDelay,
-        }
+        })
     }
 }
 

diff --git a/embedded-hal-bus/src/spi/mutex.rs b/embedded-hal-bus/src/spi/mutex.rs
@@ -23,15 +23,25 @@ pub struct MutexDevice<'a, BUS, CS, D> {
 
 impl<'a, BUS, CS, D> MutexDevice<'a, BUS, CS, D> {
     /// Create a new [`MutexDevice`].
+    ///
+    /// This sets the `cs` pin high, and returns an error if that fails. It is recommended
+    /// to set the pin high the moment it's configured as an output, to avoid glitches.
     #[inline]
-    pub fn new(bus: &'a Mutex<BUS>, cs: CS, delay: D) -> Self {
-        Self { bus, cs, delay }
+    pub fn new(bus: &'a Mutex<BUS>, mut cs: CS, delay: D) -> Result<Self, CS::Error>
+    where
+        CS: OutputPin,
+    {
+        cs.set_high()?;
+        Ok(Self { bus, cs, delay })
     }
 }
 
 impl<'a, BUS, CS> MutexDevice<'a, BUS, CS, super::NoDelay> {
     /// Create a new [`MutexDevice`] without support for in-transaction delays.
     ///
+    /// This sets the `cs` pin high, and returns an error if that fails. It is recommended
+    /// to set the pin high the moment it's configured as an output, to avoid glitches.
+    ///
     /// **Warning**: The returned instance *technically* doesn't comply with the `SpiDevice`
     /// contract, which mandates delay support. It is relatively rare for drivers to use
     /// in-transaction delays, so you might still want to use this method because it's more practical.
@@ -47,12 +57,16 @@ impl<'a, BUS, CS> MutexDevice<'a, BUS, CS, super::NoDelay> {
     /// 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 Mutex<BUS>, cs: CS) -> Self {
-        Self {
+    pub fn new_no_delay(bus: &'a Mutex<BUS>, mut cs: CS) -> Result<Self, CS::Error>
+    where
+        CS: OutputPin,
+    {
+        cs.set_high()?;
+        Ok(Self {
             bus,
             cs,
             delay: super::NoDelay,
-        }
+        })
     }
 }
 

diff --git a/embedded-hal-bus/src/spi/refcell.rs b/embedded-hal-bus/src/spi/refcell.rs
@@ -22,15 +22,25 @@ pub struct RefCellDevice<'a, BUS, CS, D> {
 
 impl<'a, BUS, CS, D> RefCellDevice<'a, BUS, CS, D> {
     /// Create a new [`RefCellDevice`].
+    ///
+    /// This sets the `cs` pin high, and returns an error if that fails. It is recommended
+    /// to set the pin high the moment it's configured as an output, to avoid glitches.
     #[inline]
-    pub fn new(bus: &'a RefCell<BUS>, cs: CS, delay: D) -> Self {
-        Self { bus, cs, delay }
+    pub fn new(bus: &'a RefCell<BUS>, mut cs: CS, delay: D) -> Result<Self, CS::Error>
+    where
+        CS: OutputPin,
+    {
+        cs.set_high()?;
+        Ok(Self { bus, cs, delay })
     }
 }
 
 impl<'a, BUS, CS> RefCellDevice<'a, BUS, CS, super::NoDelay> {
     /// Create a new [`RefCellDevice`] without support for in-transaction delays.
     ///
+    /// This sets the `cs` pin high, and returns an error if that fails. It is recommended
+    /// to set the pin high the moment it's configured as an output, to avoid glitches.
+    ///
     /// **Warning**: The returned instance *technically* doesn't comply with the `SpiDevice`
     /// contract, which mandates delay support. It is relatively rare for drivers to use
     /// in-transaction delays, so you might still want to use this method because it's more practical.
@@ -46,12 +56,16 @@ impl<'a, BUS, CS> RefCellDevice<'a, BUS, CS, super::NoDelay> {
     /// 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 RefCell<BUS>, cs: CS) -> Self {
-        Self {
+    pub fn new_no_delay(bus: &'a RefCell<BUS>, mut cs: CS) -> Result<Self, CS::Error>
+    where
+        CS: OutputPin,
+    {
+        cs.set_high()?;
+        Ok(Self {
             bus,
             cs,
             delay: super::NoDelay,
-        }
+        })
     }
 }