Merge #331

331: spi: enforce all traits have the same Error type. r=ryankurte a=Dirbaio

Previously discussed in #323, split off to its own PR now.

This PR enforces all SPI trait impls for a given type use the same Error associated type, by moving it to its own ErrorType trait.

## How breaking is this?

I believe this is not very breaking in practice (it won't make upgrading existing code to 1.0 harder), because:

- HALs already use the same error type for all methods (I haven't seen one that doesn't)
- Drivers already often add bounds to enforce the errors are the same. [Example](https://github.com/rust-iot/rust-radio-sx127x/blob/8188c20c89603dbda9592c40a8e3fc5b37769a00/src/lib.rs#L122). Without these bounds, propagating errors up would be more annoying, drivers would need even more generic types `SpiWriteError, SpiReadError, SpiTransferError, SpiTransactionalError`...

## Why is this good?

Traits being able to have different Error types is IMO a case of "bad freedom" for the HALs. Today's traits don't stop HALs from implementing them with different error types, but this would cause them to be incompatible with drivers with these bounds. If traits force error types to be the same, the problem is gone.

## What about other traits?

I believe this should be also applied to the other traits. I propose discussing this in the context of SPI here, and if the consensus is it's good I'll send PRs doing the same for the other traits.

Co-authored-by: Dario Nieuwenhuis <[email protected]>

Author: bors[bot]

CHANGELOG.md

@@ -12,6 +12,8 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
 
 ### Fixed
 - Fixed blanket impl of `DelayUs` not covering the `delay_ms` method.
+### Changed
+- `spi`: traits now enforce all impls on the same struct (eg `Transfer` and `Write`) have the same `Error` type. 
 
 ## [v1.0.0-alpha.6] - 2021-11-19
 

src/spi/blocking.rs

@@ -1,10 +1,9 @@
 //! Blocking SPI API
 
-/// Blocking transfer with separate buffers
-pub trait Transfer<W: Copy = u8> {
-    /// Error type
-    type Error: crate::spi::Error;
+use super::ErrorType;
 
+/// Blocking transfer with separate buffers
+pub trait Transfer<W = u8>: ErrorType {
     /// Writes and reads simultaneously. `write` is written to the slave on MOSI and
     /// words received on MISO are stored in `read`.
     ///
@@ -17,37 +16,27 @@ pub trait Transfer<W: Copy = u8> {
 }
 
 impl<T: Transfer<W>, W: Copy> Transfer<W> for &mut T {
-    type Error = T::Error;
-
     fn transfer(&mut self, read: &mut [W], write: &[W]) -> Result<(), Self::Error> {
         T::transfer(self, read, write)
     }
 }
 
 /// Blocking transfer with single buffer (in-place)
-pub trait TransferInplace<W: Copy = u8> {
-    /// Error type
-    type Error: crate::spi::Error;
-
+pub trait TransferInplace<W: Copy = u8>: ErrorType {
     /// Writes and reads simultaneously. The contents of `words` are
     /// written to the slave, and the received words are stored into the same
     /// `words` buffer, overwriting it.
     fn transfer_inplace(&mut self, words: &mut [W]) -> Result<(), Self::Error>;
 }
 
 impl<T: TransferInplace<W>, W: Copy> TransferInplace<W> for &mut T {
-    type Error = T::Error;
-
     fn transfer_inplace(&mut self, words: &mut [W]) -> Result<(), Self::Error> {
         T::transfer_inplace(self, words)
     }
 }
 
 /// Blocking read
-pub trait Read<W: Copy = u8> {
-    /// Error type
-    type Error: crate::spi::Error;
-
+pub trait Read<W: Copy = u8>: ErrorType {
     /// Reads `words` from the slave.
     ///
     /// The word value sent on MOSI during reading is implementation-defined,
@@ -56,44 +45,32 @@ pub trait Read<W: Copy = u8> {
 }
 
 impl<T: Read<W>, W: Copy> Read<W> for &mut T {
-    type Error = T::Error;
-
     fn read(&mut self, words: &mut [W]) -> Result<(), Self::Error> {
         T::read(self, words)
     }
 }
 
 /// Blocking write
-pub trait Write<W: Copy = u8> {
-    /// Error type
-    type Error: crate::spi::Error;
-
+pub trait Write<W: Copy = u8>: ErrorType {
     /// Writes `words` to the slave, ignoring all the incoming words
     fn write(&mut self, words: &[W]) -> Result<(), Self::Error>;
 }
 
 impl<T: Write<W>, W: Copy> Write<W> for &mut T {
-    type Error = T::Error;
-
     fn write(&mut self, words: &[W]) -> Result<(), Self::Error> {
         T::write(self, words)
     }
 }
 
 /// Blocking write (iterator version)
-pub trait WriteIter<W: Copy = u8> {
-    /// Error type
-    type Error: crate::spi::Error;
-
+pub trait WriteIter<W: Copy = u8>: ErrorType {
     /// Writes `words` to the slave, ignoring all the incoming words
     fn write_iter<WI>(&mut self, words: WI) -> Result<(), Self::Error>
     where
         WI: IntoIterator<Item = W>;
 }
 
 impl<T: WriteIter<W>, W: Copy> WriteIter<W> for &mut T {
-    type Error = T::Error;
-
     fn write_iter<WI>(&mut self, words: WI) -> Result<(), Self::Error>
     where
         WI: IntoIterator<Item = W>,
@@ -119,17 +96,12 @@ pub enum Operation<'a, W: 'static + Copy = u8> {
 
 /// Transactional trait allows multiple actions to be executed
 /// as part of a single SPI transaction
-pub trait Transactional<W: 'static + Copy = u8> {
-    /// Associated error type
-    type Error: crate::spi::Error;
-
+pub trait Transactional<W: 'static + Copy = u8>: ErrorType {
     /// Execute the provided transactions
     fn exec<'a>(&mut self, operations: &mut [Operation<'a, W>]) -> Result<(), Self::Error>;
 }
 
 impl<T: Transactional<W>, W: 'static + Copy> Transactional<W> for &mut T {
-    type Error = T::Error;
-
     fn exec<'a>(&mut self, operations: &mut [Operation<'a, W>]) -> Result<(), Self::Error> {
         T::exec(self, operations)
     }

src/spi/mod.rs

@@ -113,3 +113,15 @@ impl core::fmt::Display for ErrorKind {
         }
     }
 }
+
+/// SPI error type trait
+///
+/// This just defines the error type, to be used by the other SPI traits.
+pub trait ErrorType {
+    /// Error type
+    type Error: Error;
+}
+
+impl<T: ErrorType> ErrorType for &mut T {
+    type Error = T::Error;
+}

src/spi/nb.rs

@@ -1,5 +1,7 @@
 //! Serial Peripheral Interface
 
+use super::ErrorType;
+
 /// Full duplex (master mode)
 ///
 /// # Notes
@@ -16,10 +18,7 @@
 ///
 /// - Some SPIs can work with 8-bit *and* 16-bit words. You can overload this trait with different
 /// `Word` types to allow operation in both modes.
-pub trait FullDuplex<Word: Copy = u8> {
-    /// An enumeration of SPI errors
-    type Error: crate::spi::Error;
-
+pub trait FullDuplex<Word: Copy = u8>: ErrorType {
     /// Reads the word stored in the shift register
     ///
     /// **NOTE** A word must be sent to the slave before attempting to call this
@@ -31,8 +30,6 @@ pub trait FullDuplex<Word: Copy = u8> {
 }
 
 impl<T: FullDuplex<Word>, Word: Copy> FullDuplex<Word> for &mut T {
-    type Error = T::Error;
-
     fn read(&mut self) -> nb::Result<Word, Self::Error> {
         T::read(self)
     }