Merge #440

440: I2c: simplify, expand docs, document shared bus usage. r=eldruin a=Dirbaio

~Depends on #441 -- check that one out first.~

This does some simplifications to the trait that I think we should do:

- Implement all methods in terms of `transaction`. This way HALs have to implement just that.
- Removed byte-wise-iteration methods: `write_iter` and `write_iter_read`. The reason is that they're quite inefficient, especially with DMA implementations. We've already removed these on other traits, so I think we should do as well here.
- Removed `transaction_iter`. I don't think it's much useful in practice, because the way iterators work all the yielded `Operation`s must have the same lifetime. This means that, even if the user can generate the `Operation`s on the fly, they can't allocate buffers for these on the fly, all buffers must be pre-allocated. So it's not useful for, say, streaming a large transfer by reusing some small buffer repeatedly. See #367 
- Removed useless lifetimes
- Standardized buffer names on `read` and `write`, I think they're clearer.

It also specifies how i2c bus sharing is supposed to work. This is an alternative to #392 . After the discussions there, I don't think we should split I2C into Bus and Device anymore. For SPI it makes sense, because drivers want to enforce that there's a CS pin (`SpiDevice`) or not (`SpiBus`). This is not the case with I2C, the API is exactly the same in the shared and non-shared case. Drivers shouldn't care which case it is.

So all we have to do to "support" bus sharing is docs, This PR does:

- Document that it's allowed for implementations to be either shared or not.
- Document some guidelines for drivers and HALs on how to best use the traits, expand the examples.


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

Author: bors[bot]

embedded-hal-async/CHANGELOG.md

@@ -12,6 +12,8 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
 
 ### Changed
 - delay: make infallible.
+- i2c: remove `_iter()` methods.
+- i2c: add default implementations for all methods based on `transaction()`.
 
 ## [v0.2.0-alpha.0] - 2022-11-23
 

embedded-hal-async/src/i2c.rs

@@ -41,7 +41,10 @@ pub trait I2c<A: AddressMode = SevenBitAddress>: ErrorType {
     /// - `MAK` = master acknowledge
     /// - `NMAK` = master no acknowledge
     /// - `SP` = stop condition
-    async fn read<'a>(&'a mut self, address: A, read: &'a mut [u8]) -> Result<(), Self::Error>;
+    async fn read(&mut self, address: A, read: &mut [u8]) -> Result<(), Self::Error> {
+        self.transaction(address, &mut [Operation::Read(read)])
+            .await
+    }
 
     /// Writes bytes to slave with address `address`
     ///
@@ -59,7 +62,10 @@ pub trait I2c<A: AddressMode = SevenBitAddress>: ErrorType {
     /// - `SAK` = slave acknowledge
     /// - `Bi` = ith byte of data
     /// - `SP` = stop condition
-    async fn write<'a>(&'a mut self, address: A, write: &'a [u8]) -> Result<(), Self::Error>;
+    async fn write(&mut self, address: A, write: &[u8]) -> Result<(), Self::Error> {
+        self.transaction(address, &mut [Operation::Write(write)])
+            .await
+    }
 
     /// Writes bytes to slave with address `address` and then reads enough bytes to fill `read` *in a
     /// single transaction*.
@@ -83,12 +89,18 @@ pub trait I2c<A: AddressMode = SevenBitAddress>: ErrorType {
     /// - `MAK` = master acknowledge
     /// - `NMAK` = master no acknowledge
     /// - `SP` = stop condition
-    async fn write_read<'a>(
-        &'a mut self,
+    async fn write_read(
+        &mut self,
         address: A,
-        write: &'a [u8],
-        read: &'a mut [u8],
-    ) -> Result<(), Self::Error>;
+        write: &[u8],
+        read: &mut [u8],
+    ) -> Result<(), Self::Error> {
+        self.transaction(
+            address,
+            &mut [Operation::Write(write), Operation::Read(read)],
+        )
+        .await
+    }
 
     /// Execute the provided operations on the I2C bus as a single transaction.
     ///
@@ -103,35 +115,35 @@ pub trait I2c<A: AddressMode = SevenBitAddress>: ErrorType {
     /// - `SAD+R/W` = slave address followed by bit 1 to indicate reading or 0 to indicate writing
     /// - `SR` = repeated start condition
     /// - `SP` = stop condition
-    async fn transaction<'a, 'b>(
-        &'a mut self,
+    async fn transaction(
+        &mut self,
         address: A,
-        operations: &'a mut [Operation<'b>],
+        operations: &mut [Operation<'_>],
     ) -> Result<(), Self::Error>;
 }
 
 impl<A: AddressMode, T: I2c<A>> I2c<A> for &mut T {
-    async fn read<'a>(&'a mut self, address: A, buffer: &'a mut [u8]) -> Result<(), Self::Error> {
-        T::read(self, address, buffer).await
+    async fn read(&mut self, address: A, read: &mut [u8]) -> Result<(), Self::Error> {
+        T::read(self, address, read).await
     }
 
-    async fn write<'a>(&'a mut self, address: A, bytes: &'a [u8]) -> Result<(), Self::Error> {
-        T::write(self, address, bytes).await
+    async fn write(&mut self, address: A, write: &[u8]) -> Result<(), Self::Error> {
+        T::write(self, address, write).await
     }
 
-    async fn write_read<'a>(
-        &'a mut self,
+    async fn write_read(
+        &mut self,
         address: A,
-        bytes: &'a [u8],
-        buffer: &'a mut [u8],
+        write: &[u8],
+        read: &mut [u8],
     ) -> Result<(), Self::Error> {
-        T::write_read(self, address, bytes, buffer).await
+        T::write_read(self, address, write, read).await
     }
 
-    async fn transaction<'a, 'b>(
-        &'a mut self,
+    async fn transaction(
+        &mut self,
         address: A,
-        operations: &'a mut [Operation<'b>],
+        operations: &mut [Operation<'_>],
     ) -> Result<(), Self::Error> {
         T::transaction(self, address, operations).await
     }

embedded-hal/CHANGELOG.md

@@ -13,6 +13,9 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
 ### Changed
 - gpio: add `ErrorKind` enum for consistency with other traits and for future extensibility. No kinds are defined for now.
 - delay: make infallible.
+- i2c: remove `_iter()` methods.
+- i2c: add default implementations for all methods based on `transaction()`.
+- i2c: document guidelines for shared bus usage.
 
 ## [v1.0.0-alpha.9] - 2022-09-28
 

embedded-hal/src/i2c-shared-bus.svg

@@ -1,8 +1,8 @@
 //! Blocking I2C API
 //!
-//! This API supports 7-bit and 10-bit addresses. Traits feature an `AddressMode`
-//! marker type parameter. Two implementation of the `AddressMode` exist:
-//! `SevenBitAddress` and `TenBitAddress`.
+//! This API supports 7-bit and 10-bit addresses. Traits feature an [`AddressMode`]
+//! marker type parameter. Two implementation of the [`AddressMode`] exist:
+//! [`SevenBitAddress`] and [`TenBitAddress`].
 //!
 //! Through this marker types it is possible to implement each address mode for
 //! the traits independently in `embedded-hal` implementations and device drivers
@@ -14,128 +14,140 @@
 //! is not supported by the hardware.
 //!
 //! Since 7-bit addressing is the mode of the majority of I2C devices,
-//! `SevenBitAddress` has been set as default mode and thus can be omitted if desired.
+//! [`SevenBitAddress`] has been set as default mode and thus can be omitted if desired.
 //!
-//! ## Examples
+//! # Bus sharing
 //!
-//! ### `embedded-hal` implementation for an MCU
-//! Here is an example of an embedded-hal implementation of the `Write` trait
-//! for both modes:
-//! ```
-//! # use embedded_hal::i2c::{ErrorKind, ErrorType, SevenBitAddress, TenBitAddress, I2c, Operation};
-//! /// I2C0 hardware peripheral which supports both 7-bit and 10-bit addressing.
-//! pub struct I2c0;
+//! I2C allows sharing a single bus between many I2C devices. The SDA and SCL lines are
+//! wired in parallel to all devices. When starting a transfer an "address" is sent
+//! so that the addressed device can respond and all the others can ignore the transfer.
 //!
-//! # impl ErrorType for I2c0 { type Error = ErrorKind; }
-//! impl I2c<SevenBitAddress> for I2c0
-//! {
-//!     fn read(&mut self, addr: u8, buffer: &mut [u8]) -> Result<(), Self::Error> {
-//!         // ...
-//! #       Ok(())
-//!     }
-//!     fn write(&mut self, addr: u8, bytes: &[u8]) -> Result<(), Self::Error> {
-//!         // ...
-//! #       Ok(())
-//!     }
-//!     fn write_iter<B: IntoIterator<Item = u8>>(&mut self, addr: u8, bytes: B) -> Result<(), Self::Error> {
-//!         // ...
-//! #       Ok(())
-//!     }
-//!     fn write_read(&mut self, addr: u8, bytes: &[u8], buffer: &mut [u8]) -> Result<(), Self::Error> {
-//!         // ...
-//! #       Ok(())
-//!     }
-//!     fn write_iter_read<B: IntoIterator<Item = u8>>(&mut self, addr: u8, bytes: B, buffer: &mut [u8]) -> Result<(), Self::Error> {
-//!         // ...
-//! #       Ok(())
-//!     }
-//!     fn transaction<'a>(&mut self, address: u8, operations: &mut [Operation<'a>]) -> Result<(), Self::Error> {
-//!         // ...
-//! #       Ok(())
-//!     }
-//!     fn transaction_iter<'a, O: IntoIterator<Item = Operation<'a>>>(&mut self, address: u8, operations: O) -> Result<(), Self::Error> {
-//!         // ...
-//! #       Ok(())
-//!     }
-//! }
+#![doc = include_str!("i2c-shared-bus.svg")]
 //!
-//! impl I2c<TenBitAddress> for I2c0
-//! {
-//!     fn read(&mut self, addr: u16, buffer: &mut [u8]) -> Result<(), Self::Error> {
-//!         // ...
-//! #       Ok(())
-//!     }
-//!     fn write(&mut self, addr: u16, bytes: &[u8]) -> Result<(), Self::Error> {
-//!         // ...
-//! #       Ok(())
-//!     }
-//!     fn write_iter<B: IntoIterator<Item = u8>>(&mut self, addr: u16, bytes: B) -> Result<(), Self::Error> {
-//!         // ...
-//! #       Ok(())
-//!     }
-//!     fn write_read(&mut self, addr: u16, bytes: &[u8], buffer: &mut [u8]) -> Result<(), Self::Error> {
-//!         // ...
-//! #       Ok(())
-//!     }
-//!     fn write_iter_read<B: IntoIterator<Item = u8>>(&mut self, addr: u16, bytes: B, buffer: &mut [u8]) -> Result<(), Self::Error> {
-//!         // ...
-//! #       Ok(())
-//!     }
-//!     fn transaction<'a>(&mut self, address: u16, operations: &mut [Operation<'a>]) -> Result<(), Self::Error> {
-//!         // ...
-//! #       Ok(())
-//!     }
-//!     fn transaction_iter<'a, O: IntoIterator<Item = Operation<'a>>>(&mut self, address: u16, operations: O) -> Result<(), Self::Error> {
-//!         // ...
-//! #       Ok(())
-//!     }
-//! }
-//! ```
+//! This bus sharing is common when having multiple I2C devices in the same board, since it uses fewer MCU
+//! pins (`2` instead of `2*n`), and fewer MCU I2C peripherals (`1` instead of `n`).
 //!
-//! ### Device driver compatible only with 7-bit addresses
+//! This API supports bus sharing natively. Types implementing [`I2c`] are allowed
+//! to represent either exclusive or shared access to an I2C bus. HALs typically
+//! provide exclusive access implementations. Drivers shouldn't care which
+//! kind they receive, they just do transactions on it and let the
+//! underlying implementation share or not.
+//!
+//! The [`embedded-hal-bus`](https://docs.rs/embedded-hal-bus) crate provides several
+//! implementations for sharing I2C buses. You can use them to take an exclusive instance
+//! you've received from a HAL and "split" it into mulitple shared ones, to instantiate
+//! several drivers on the same bus.
+//!
+//! # For driver authors
+//!
+//! Drivers can select the adequate address length with `I2c<SevenBitAddress>` or `I2c<TenBitAddress>` depending
+//! on the target device. If it can use either, the driver can
+//! be generic over the address kind as well, though this is rare.
+//!
+//! Drivers should take the `I2c` instance as an argument to `new()`, and store it in their
+//! struct. They **should not** take `&mut I2c`, the trait has a blanket impl for all `&mut T`
+//! so taking just `I2c` ensures the user can still pass a `&mut`, but is not forced to.
+//!
+//! Drivers **should not** try to enable bus sharing by taking `&mut I2c` at every method.
+//! This is much less ergonomic than owning the `I2c`, which still allows the user to pass an
+//! implementation that does sharing behind the scenes
+//! (from [`embedded-hal-bus`](https://docs.rs/embedded-hal-bus), or others).
+//!
+//! ## Device driver compatible only with 7-bit addresses
 //!
 //! For demonstration purposes the address mode parameter has been omitted in this example.
 //!
 //! ```
-//! # use embedded_hal::i2c::{I2c, Error};
-//! const ADDR: u8  = 0x15;
+//! use embedded_hal::i2c::{I2c, Error};
+//!
+//! const ADDR: u8 = 0x15;
 //! # const TEMP_REGISTER: u8 = 0x1;
 //! pub struct TemperatureSensorDriver<I2C> {
 //!     i2c: I2C,
 //! }
 //!
-//! impl<I2C, E: Error> TemperatureSensorDriver<I2C>
-//! where
-//!     I2C: I2c<Error = E>,
-//! {
-//!     pub fn read_temperature(&mut self) -> Result<u8, E> {
+//! impl<I2C: I2c> TemperatureSensorDriver<I2C> {
+//!     pub fn new(i2c: I2C) -> Self {
+//!         Self { i2c }
+//!     }
+//!
+//!     pub fn read_temperature(&mut self) -> Result<u8, I2C::Error> {
 //!         let mut temp = [0];
-//!         self.i2c
-//!             .write_read(ADDR, &[TEMP_REGISTER], &mut temp)
-//!             .and(Ok(temp[0]))
+//!         self.i2c.write_read(ADDR, &[TEMP_REGISTER], &mut temp)?;
+//!         Ok(temp[0])
 //!     }
 //! }
 //! ```
 //!
-//! ### Device driver compatible only with 10-bit addresses
+//! ## Device driver compatible only with 10-bit addresses
 //!
 //! ```
-//! # use embedded_hal::i2c::{Error, TenBitAddress, I2c};
-//! const ADDR: u16  = 0x158;
+//! use embedded_hal::i2c::{Error, TenBitAddress, I2c};
+//!
+//! const ADDR: u16 = 0x158;
 //! # const TEMP_REGISTER: u8 = 0x1;
 //! pub struct TemperatureSensorDriver<I2C> {
 //!     i2c: I2C,
 //! }
 //!
-//! impl<I2C, E: Error> TemperatureSensorDriver<I2C>
-//! where
-//!     I2C: I2c<TenBitAddress, Error = E>,
-//! {
-//!     pub fn read_temperature(&mut self) -> Result<u8, E> {
+//! impl<I2C: I2c<TenBitAddress>> TemperatureSensorDriver<I2C> {
+//!     pub fn new(i2c: I2C) -> Self {
+//!         Self { i2c }
+//!     }
+//!
+//!     pub fn read_temperature(&mut self) -> Result<u8, I2C::Error> {
 //!         let mut temp = [0];
-//!         self.i2c
-//!             .write_read(ADDR, &[TEMP_REGISTER], &mut temp)
-//!             .and(Ok(temp[0]))
+//!         self.i2c.write_read(ADDR, &[TEMP_REGISTER], &mut temp)?;
+//!         Ok(temp[0])
+//!     }
+//! }
+//! ```
+//!
+//! # For HAL authors
+//!
+//! HALs **should not** include bus sharing mechanisms. They should expose a single type representing
+//! exclusive ownership over the bus, and let the user use [`embedded-hal-bus`](https://docs.rs/embedded-hal-bus)
+//! if they want to share it. (One exception is if the underlying platform already
+//! supports sharing, such as Linux or some RTOSs.)
+//!
+//! Here is an example of an embedded-hal implementation of the `I2C` trait
+//! for both addressing modes. All trait methods have have default implementations in terms of `transaction`.
+//! As such, that is the only method that requires implementation in the HAL.
+//!
+//! ```
+//! use embedded_hal::i2c::{self, SevenBitAddress, TenBitAddress, I2c, Operation};
+//!
+//! /// I2C0 hardware peripheral which supports both 7-bit and 10-bit addressing.
+//! pub struct I2c0;
+//!
+//! #[derive(Debug, Copy, Clone, Eq, PartialEq)]
+//! pub enum Error {
+//!     // ...
+//! }
+//!
+//! impl i2c::Error for Error {
+//!     fn kind(&self) -> i2c::ErrorKind {
+//!         match *self {
+//!             // ...
+//!         }
+//!     }
+//! }
+//!
+//! impl i2c::ErrorType for I2c0 {
+//!     type Error = Error;
+//! }
+//!
+//! impl I2c<SevenBitAddress> for I2c0 {
+//!     fn transaction(&mut self, address: u8, operations: &mut [Operation<'_>]) -> Result<(), Self::Error> {
+//!         // ...
+//! #       Ok(())
+//!     }
+//! }
+//!
+//! impl I2c<TenBitAddress> for I2c0 {
+//!     fn transaction(&mut self, address: u16, operations: &mut [Operation<'_>]) -> Result<(), Self::Error> {
+//!         // ...
+//! #       Ok(())
 //!     }
 //! }
 //! ```
@@ -256,7 +268,7 @@ impl AddressMode for SevenBitAddress {}
 
 impl AddressMode for TenBitAddress {}
 
-/// Transactional I2C operation.
+/// I2C operation.
 ///
 /// Several operations can be combined as part of a transaction.
 #[derive(Debug, PartialEq, Eq)]
@@ -269,7 +281,7 @@ pub enum Operation<'a> {
 
 /// Blocking I2C
 pub trait I2c<A: AddressMode = SevenBitAddress>: ErrorType {
-    /// Reads enough bytes from slave with `address` to fill `buffer`
+    /// Reads enough bytes from slave with `address` to fill `read`
     ///
     /// # I2C Events (contract)
     ///
@@ -287,7 +299,9 @@ pub trait I2c<A: AddressMode = SevenBitAddress>: ErrorType {
     /// - `MAK` = master acknowledge
     /// - `NMAK` = master no acknowledge
     /// - `SP` = stop condition
-    fn read(&mut self, address: A, buffer: &mut [u8]) -> Result<(), Self::Error>;
+    fn read(&mut self, address: A, read: &mut [u8]) -> Result<(), Self::Error> {
+        self.transaction(address, &mut [Operation::Read(read)])
+    }
 
     /// Writes bytes to slave with address `address`
     ///
@@ -305,18 +319,11 @@ pub trait I2c<A: AddressMode = SevenBitAddress>: ErrorType {
     /// - `SAK` = slave acknowledge
     /// - `Bi` = ith byte of data
     /// - `SP` = stop condition
-    fn write(&mut self, address: A, bytes: &[u8]) -> Result<(), Self::Error>;
-
-    /// Writes bytes to slave with address `address`
-    ///
-    /// # I2C Events (contract)
-    ///
-    /// Same as the `write` method
-    fn write_iter<B>(&mut self, address: A, bytes: B) -> Result<(), Self::Error>
-    where
-        B: IntoIterator<Item = u8>;
+    fn write(&mut self, address: A, write: &[u8]) -> Result<(), Self::Error> {
+        self.transaction(address, &mut [Operation::Write(write)])
+    }
 
-    /// Writes bytes to slave with address `address` and then reads enough bytes to fill `buffer` *in a
+    /// Writes bytes to slave with address `address` and then reads enough bytes to fill `read` *in a
     /// single transaction*
     ///
     /// # I2C Events (contract)
@@ -338,27 +345,12 @@ pub trait I2c<A: AddressMode = SevenBitAddress>: ErrorType {
     /// - `MAK` = master acknowledge
     /// - `NMAK` = master no acknowledge
     /// - `SP` = stop condition
-    fn write_read(
-        &mut self,
-        address: A,
-        bytes: &[u8],
-        buffer: &mut [u8],
-    ) -> Result<(), Self::Error>;
-
-    /// Writes bytes to slave with address `address` and then reads enough bytes to fill `buffer` *in a
-    /// single transaction*
-    ///
-    /// # I2C Events (contract)
-    ///
-    /// Same as the `write_read` method
-    fn write_iter_read<B>(
-        &mut self,
-        address: A,
-        bytes: B,
-        buffer: &mut [u8],
-    ) -> Result<(), Self::Error>
-    where
-        B: IntoIterator<Item = u8>;
+    fn write_read(&mut self, address: A, write: &[u8], read: &mut [u8]) -> Result<(), Self::Error> {
+        self.transaction(
+            address,
+            &mut [Operation::Write(write), Operation::Read(read)],
+        )
+    }
 
     /// Execute the provided operations on the I2C bus.
     ///
@@ -373,79 +365,31 @@ pub trait I2c<A: AddressMode = SevenBitAddress>: ErrorType {
     /// - `SAD+R/W` = slave address followed by bit 1 to indicate reading or 0 to indicate writing
     /// - `SR` = repeated start condition
     /// - `SP` = stop condition
-    fn transaction<'a>(
+    fn transaction(
         &mut self,
         address: A,
-        operations: &mut [Operation<'a>],
+        operations: &mut [Operation<'_>],
     ) -> Result<(), Self::Error>;
-
-    /// Execute the provided operations on the I2C bus (iterator version).
-    ///
-    /// Transaction contract:
-    /// - Before executing the first operation an ST is sent automatically. This is followed by SAD+R/W as appropriate.
-    /// - Data from adjacent operations of the same type are sent after each other without an SP or SR.
-    /// - Between adjacent operations of a different type an SR and SAD+R/W is sent.
-    /// - After executing the last operation an SP is sent automatically.
-    /// - If the last operation is a `Read` the master does not send an acknowledge for the last byte.
-    ///
-    /// - `ST` = start condition
-    /// - `SAD+R/W` = slave address followed by bit 1 to indicate reading or 0 to indicate writing
-    /// - `SR` = repeated start condition
-    /// - `SP` = stop condition
-    fn transaction_iter<'a, O>(&mut self, address: A, operations: O) -> Result<(), Self::Error>
-    where
-        O: IntoIterator<Item = Operation<'a>>;
 }
 
 impl<A: AddressMode, T: I2c<A>> I2c<A> for &mut T {
-    fn read(&mut self, address: A, buffer: &mut [u8]) -> Result<(), Self::Error> {
-        T::read(self, address, buffer)
-    }
-
-    fn write(&mut self, address: A, bytes: &[u8]) -> Result<(), Self::Error> {
-        T::write(self, address, bytes)
-    }
-
-    fn write_iter<B>(&mut self, address: A, bytes: B) -> Result<(), Self::Error>
-    where
-        B: IntoIterator<Item = u8>,
-    {
-        T::write_iter(self, address, bytes)
+    fn read(&mut self, address: A, read: &mut [u8]) -> Result<(), Self::Error> {
+        T::read(self, address, read)
     }
 
-    fn write_read(
-        &mut self,
-        address: A,
-        bytes: &[u8],
-        buffer: &mut [u8],
-    ) -> Result<(), Self::Error> {
-        T::write_read(self, address, bytes, buffer)
+    fn write(&mut self, address: A, write: &[u8]) -> Result<(), Self::Error> {
+        T::write(self, address, write)
     }
 
-    fn write_iter_read<B>(
-        &mut self,
-        address: A,
-        bytes: B,
-        buffer: &mut [u8],
-    ) -> Result<(), Self::Error>
-    where
-        B: IntoIterator<Item = u8>,
-    {
-        T::write_iter_read(self, address, bytes, buffer)
+    fn write_read(&mut self, address: A, write: &[u8], read: &mut [u8]) -> Result<(), Self::Error> {
+        T::write_read(self, address, write, read)
     }
 
-    fn transaction<'a>(
+    fn transaction(
         &mut self,
         address: A,
-        operations: &mut [Operation<'a>],
+        operations: &mut [Operation<'_>],
     ) -> Result<(), Self::Error> {
         T::transaction(self, address, operations)
     }
-
-    fn transaction_iter<'a, O>(&mut self, address: A, operations: O) -> Result<(), Self::Error>
-    where
-        O: IntoIterator<Item = Operation<'a>>,
-    {
-        T::transaction_iter(self, address, operations)
-    }
 }