fix(i2c): Send repeated starts in byte-by-byte I2C transactions

hal/src/sercom/i2c.rs

@@ -199,10 +199,10 @@
 //! use atsamd_hal::dmac::channel::{AnyChannel, Ready};
 //! use atsand_hal::sercom::i2c::{I2c, AnyConfig, Error};
 //! use atsamd_hal::embedded_hal::i2c::I2c;
-//! fn i2c_send_with_dma<A: AnyConfig, C: AnyChannel<Status = Ready>>(i2c: I2c<A>, channel: C, bytes: &[u8]) -> Result<(), Error>{
+//! fn i2c_write_with_dma<A: AnyConfig, C: AnyChannel<Status = Ready>>(i2c: I2c<A>, channel: C, bytes: &[u8]) -> Result<(), Error>{
 //!     // Attach a DMA channel
 //!     let i2c = i2c.with_dma_channel(channel);
-//!     i2c.send(0x54, bytes)?;
+//!     i2c.write(0x54, bytes)?;
 //! }
 //! ```
 //!
@@ -215,6 +215,13 @@
 //!   across all adjacent operations must not exceed 256. If you need continuous
 //!   transfers of 256 bytes or more, use the non-DMA [`I2c`] implementations.
 //!
+//! * When using [`I2c::transaction`] or [`I2c::write_read`], the
+//!   [`embedded_hal::i2c::I2c`] specification mandates that a REPEATED START
+//!   (instead of a STOP+START) is sent between transactions of a different type
+//!   (read/write). Unfortunately, in DMA mode, the hardware is only capable of
+//!   sending STOP+START. If you absolutely need repeated starts, the only
+//!   workaround is to use the I2C without DMA.
+//!
 //! * Using [`I2c::transaction`] consumes significantly more memory than the
 //!   other methods provided by [`embedded_hal::i2c::I2c`] (at least 256 bytes
 //!   extra).
@@ -239,6 +246,7 @@
 //! [`PinMode`]: crate::gpio::pin::PinMode
 //! [`embedded_hal::i2c::I2c`]: crate::ehal::i2c::I2c
 //! [`I2c::transaction`]: crate::ehal::i2c::I2c::transaction
+//! [`I2c::write_read`]: crate::ehal::i2c::I2c::write_read
 
 use atsamd_hal_macros::hal_module;
 

hal/src/sercom/i2c/impl_ehal.rs

@@ -1,7 +1,7 @@
 //! [`embedded-hal`] trait implementations for [`I2c`]s
 
 use super::{config::AnyConfig, flags::Error, I2c};
-use crate::ehal::i2c::{self, ErrorKind, ErrorType, NoAcknowledgeSource};
+use crate::ehal::i2c::{self, ErrorKind, ErrorType, NoAcknowledgeSource, Operation};
 
 impl i2c::Error for Error {
     #[allow(unreachable_patterns)]
@@ -26,42 +26,42 @@ impl<C: AnyConfig, D> I2c<C, D> {
     fn transaction_byte_by_byte(
         &mut self,
         address: u8,
-        operations: &mut [i2c::Operation<'_>],
+        operations: &mut [Operation<'_>],
     ) -> Result<(), Error> {
-        /// Helper type for keeping track of the type of operation that was
-        /// executed last
-        #[derive(Clone, Copy)]
-        enum Operation {
-            Read,
-            Write,
-        }
+        let mut op_groups = chunk_operations(operations).peekable();
+
+        while let Some(group) = op_groups.next() {
+            let mut group = group.iter_mut();
+            // Unwrapping is OK here because chunk_operations will never give us a 0-length
+            // chunk.
+            let op = group.next().unwrap();
 
-        // Keep track of the last executed operation type. The method
-        // specification demands, that no repeated start condition is sent
-        // between adjacent operations of the same type.
-        let mut last_op = None;
-        for op in operations {
+            // First operation in the group - send a START with the address, and the first
+            // operation.
             match op {
-                i2c::Operation::Read(buf) => {
-                    if let Some(Operation::Read) = last_op {
-                        self.continue_read(buf)?;
-                    } else {
-                        self.do_read(address, buf)?;
-                        last_op = Some(Operation::Read);
-                    }
-                }
-                i2c::Operation::Write(bytes) => {
-                    if let Some(Operation::Write) = last_op {
-                        self.continue_write(bytes)?;
-                    } else {
-                        self.do_write(address, bytes)?;
-                        last_op = Some(Operation::Write);
-                    }
+                Operation::Read(buf) => self.do_read(address, buf)?,
+                Operation::Write(buf) => self.do_write(address, buf)?,
+            }
+
+            // For all subsequent operations, just send/read more bytes without any more
+            // ceremony.
+            for op in group {
+                match op {
+                    Operation::Read(buf) => self.continue_read(buf)?,
+                    Operation::Write(buf) => self.continue_write(buf)?,
                 }
             }
+
+            let regs = &mut self.config.as_mut().registers;
+            if op_groups.peek().is_some() {
+                // If we still have more groups to go, send a repeated start
+                regs.cmd_repeated_start();
+            } else {
+                // Otherwise, send a stop
+                regs.cmd_stop();
+            }
         }
 
-        self.cmd_stop();
         Ok(())
     }
 }
@@ -70,7 +70,7 @@ impl<C: AnyConfig> i2c::I2c for I2c<C> {
     fn transaction(
         &mut self,
         address: u8,
-        operations: &mut [i2c::Operation<'_>],
+        operations: &mut [Operation<'_>],
     ) -> Result<(), Self::Error> {
         self.transaction_byte_by_byte(address, operations)?;
         Ok(())
@@ -290,10 +290,7 @@ mod dma {
             //  the same type, we must revert to the byte-by-byte I2C implementations.
             let mut descriptors = heapless::Vec::<DmacDescriptor, NUM_LINKED_TRANSFERS>::new();
 
-            // Arrange operations in groups of contiguous types (R/W)
-            let op_groups = operations.chunk_by_mut(|this, next| {
-                matches!((this, next), (Write(_), Write(_)) | (Read(_), Read(_)))
-            });
+            let op_groups = chunk_operations(operations);
 
             for group in op_groups {
                 descriptors.clear();
@@ -435,3 +432,14 @@ impl<C: AnyConfig> crate::ehal_02::blocking::i2c::WriteRead for I2c<C> {
         Ok(())
     }
 }
+
+/// Arrange all operations in contiguous chunks of the same R/W type
+pub(super) fn chunk_operations<'a, 'op>(
+    operations: &'a mut [Operation<'op>],
+) -> impl Iterator<Item = &'a mut [Operation<'op>]> {
+    use i2c::Operation::{Read, Write};
+
+    operations.chunk_by_mut(|this, next| {
+        matches!((this, next), (Write(_), Write(_)) | (Read(_), Read(_)))
+    })
+}

hal/src/sercom/i2c/reg.rs

@@ -10,6 +10,7 @@ use atsamd_hal_macros::hal_cfg;
 
 const MASTER_ACT_READ: u8 = 2;
 const MASTER_ACT_STOP: u8 = 3;
+const MASTER_ACT_REPEATED_START: u8 = 1;
 
 #[hal_cfg(any("sercom0-d11", "sercom0-d21"))]
 type DataReg = u8;
@@ -330,18 +331,32 @@ impl<S: Sercom> Registers<S> {
         self.sync_sysop();
     }
 
+    /// Send a STOP condition. If the I2C is performing a read, will also send a
+    /// NACK to the slave.
     #[inline]
-    pub(super) fn issue_command(&mut self, cmd: u8) {
-        self.i2c_master()
-            .ctrlb()
-            .modify(|_, w| unsafe { w.cmd().bits(cmd) });
-
+    pub(super) fn cmd_stop(&mut self) {
+        unsafe {
+            self.i2c_master().ctrlb().modify(|_, w| {
+                // set bit means send NACK
+                w.ackact().set_bit();
+                w.cmd().bits(MASTER_ACT_STOP)
+            });
+        }
         self.sync_sysop();
     }
 
+    /// Send a REPEATED START condition. If the I2C is performing a read, will
+    /// also send a NACK to the slave.
     #[inline]
-    pub(super) fn cmd_stop(&mut self) {
-        self.issue_command(MASTER_ACT_STOP)
+    pub(super) fn cmd_repeated_start(&mut self) {
+        unsafe {
+            self.i2c_master().ctrlb().modify(|_, w| {
+                // set bit means send NACK
+                w.ackact().set_bit();
+                w.cmd().bits(MASTER_ACT_REPEATED_START)
+            });
+        }
+        self.sync_sysop();
     }
 
     #[inline]