I2c: attempt empty writes (#2506)

* Attempt 0-length writes

* Deduplicate

* Changelog

* Test existing address

* Name addresses

* Fix formatting

esp-hal/CHANGELOG.md

@@ -199,6 +199,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - TWAI should no longer panic when receiving a non-compliant frame (#2255)
 - OneShotTimer: fixed `delay_nanos` behaviour (#2256)
 - Fixed unsoundness around `Efuse` (#2259)
+- Empty I2C writes to unknown addresses now correctly fail with `AckCheckFailed`. (#2506)
 
 ### Removed
 

esp-hal/src/i2c/master/mod.rs

@@ -1855,30 +1855,51 @@ impl Driver<'_> {
             .write(|w| w.rxfifo_full().clear_bit_by_one());
     }
 
-    /// Executes an I2C write operation.
+    fn start_write_operation(
+        &self,
+        address: u8,
+        bytes: &[u8],
+        start: bool,
+        stop: bool,
+    ) -> Result<usize, Error> {
+        self.reset_fifo();
+        self.reset_command_list();
+        let cmd_iterator = &mut self.register_block().comd_iter();
+
+        if start {
+            add_cmd(cmd_iterator, Command::Start)?;
+        }
+
+        self.setup_write(address, bytes, start, cmd_iterator)?;
+
+        add_cmd(
+            cmd_iterator,
+            if stop { Command::Stop } else { Command::End },
+        )?;
+        let index = self.fill_tx_fifo(bytes);
+        self.start_transmission();
+
+        Ok(index)
+    }
+
+    /// Executes an I2C read operation.
     /// - `addr` is the address of the slave device.
-    /// - `bytes` is the data two be sent.
+    /// - `buffer` is the buffer to store the read data.
     /// - `start` indicates whether the operation should start by a START
     ///   condition and sending the address.
     /// - `stop` indicates whether the operation should end with a STOP
     ///   condition.
+    /// - `will_continue` indicates whether there is another read operation
+    ///   following this one and we should not nack the last byte.
     /// - `cmd_iterator` is an iterator over the command registers.
-    fn write_operation_blocking(
+    fn start_read_operation(
         &self,
         address: u8,
-        bytes: &[u8],
+        buffer: &mut [u8],
         start: bool,
         stop: bool,
+        will_continue: bool,
     ) -> Result<(), Error> {
-        self.clear_all_interrupts();
-
-        // Short circuit for zero length writes without start or end as that would be an
-        // invalid operation write lengths in the TRM (at least for ESP32-S3) are 1-255
-        if bytes.is_empty() && !start && !stop {
-            return Ok(());
-        }
-
-        // Reset FIFO and command list
         self.reset_fifo();
         self.reset_command_list();
 
@@ -1887,14 +1908,41 @@ impl Driver<'_> {
         if start {
             add_cmd(cmd_iterator, Command::Start)?;
         }
-        self.setup_write(address, bytes, start, cmd_iterator)?;
+
+        self.setup_read(address, buffer, start, will_continue, cmd_iterator)?;
+
         add_cmd(
             cmd_iterator,
             if stop { Command::Stop } else { Command::End },
         )?;
-        let index = self.fill_tx_fifo(bytes);
         self.start_transmission();
+        Ok(())
+    }
 
+    /// Executes an I2C write operation.
+    /// - `addr` is the address of the slave device.
+    /// - `bytes` is the data two be sent.
+    /// - `start` indicates whether the operation should start by a START
+    ///   condition and sending the address.
+    /// - `stop` indicates whether the operation should end with a STOP
+    ///   condition.
+    /// - `cmd_iterator` is an iterator over the command registers.
+    fn write_operation_blocking(
+        &self,
+        address: u8,
+        bytes: &[u8],
+        start: bool,
+        stop: bool,
+    ) -> Result<(), Error> {
+        self.clear_all_interrupts();
+
+        // Short circuit for zero length writes without start or end as that would be an
+        // invalid operation write lengths in the TRM (at least for ESP32-S3) are 1-255
+        if bytes.is_empty() && !start && !stop {
+            return Ok(());
+        }
+
+        let index = self.start_write_operation(address, bytes, start, stop)?;
         // Fill the FIFO with the remaining bytes:
         self.write_remaining_tx_fifo_blocking(index, bytes)?;
         self.wait_for_completion_blocking(!stop)?;
@@ -1927,23 +1975,7 @@ impl Driver<'_> {
             return Ok(());
         }
 
-        // Reset FIFO and command list
-        self.reset_fifo();
-        self.reset_command_list();
-
-        let cmd_iterator = &mut self.register_block().comd_iter();
-
-        if start {
-            add_cmd(cmd_iterator, Command::Start)?;
-        }
-
-        self.setup_read(address, buffer, start, will_continue, cmd_iterator)?;
-
-        add_cmd(
-            cmd_iterator,
-            if stop { Command::Stop } else { Command::End },
-        )?;
-        self.start_transmission();
+        self.start_read_operation(address, buffer, start, stop, will_continue)?;
         self.read_all_from_fifo_blocking(buffer)?;
         self.wait_for_completion_blocking(!stop)?;
         Ok(())
@@ -1972,22 +2004,7 @@ impl Driver<'_> {
             return Ok(());
         }
 
-        // Reset FIFO and command list
-        self.reset_fifo();
-        self.reset_command_list();
-
-        let cmd_iterator = &mut self.register_block().comd_iter();
-        if start {
-            add_cmd(cmd_iterator, Command::Start)?;
-        }
-        self.setup_write(address, bytes, start, cmd_iterator)?;
-        add_cmd(
-            cmd_iterator,
-            if stop { Command::Stop } else { Command::End },
-        )?;
-        let index = self.fill_tx_fifo(bytes);
-        self.start_transmission();
-
+        let index = self.start_write_operation(address, bytes, start, stop)?;
         // Fill the FIFO with the remaining bytes:
         self.write_remaining_tx_fifo(index, bytes).await?;
         self.wait_for_completion(!stop).await?;
@@ -2020,19 +2037,7 @@ impl Driver<'_> {
             return Ok(());
         }
 
-        // Reset FIFO and command list
-        self.reset_fifo();
-        self.reset_command_list();
-        let cmd_iterator = &mut self.register_block().comd_iter();
-        if start {
-            add_cmd(cmd_iterator, Command::Start)?;
-        }
-        self.setup_read(address, buffer, start, will_continue, cmd_iterator)?;
-        add_cmd(
-            cmd_iterator,
-            if stop { Command::Stop } else { Command::End },
-        )?;
-        self.start_transmission();
+        self.start_read_operation(address, buffer, start, stop, will_continue)?;
         self.read_all_from_fifo(buffer).await?;
         self.wait_for_completion(!stop).await?;
         Ok(())
@@ -2067,6 +2072,9 @@ impl Driver<'_> {
         start: bool,
         stop: bool,
     ) -> Result<(), Error> {
+        if buffer.is_empty() {
+            return self.write_operation_blocking(address, &[], start, stop);
+        }
         let chunk_count = buffer.len().div_ceil(I2C_CHUNK_SIZE);
         for (idx, chunk) in buffer.chunks(I2C_CHUNK_SIZE).enumerate() {
             self.write_operation_blocking(
@@ -2110,6 +2118,9 @@ impl Driver<'_> {
         start: bool,
         stop: bool,
     ) -> Result<(), Error> {
+        if buffer.is_empty() {
+            return self.write_operation(address, &[], start, stop).await;
+        }
         let chunk_count = buffer.len().div_ceil(I2C_CHUNK_SIZE);
         for (idx, chunk) in buffer.chunks(I2C_CHUNK_SIZE).enumerate() {
             self.write_operation(

hil-test/tests/i2c.rs

@@ -6,7 +6,7 @@
 #![no_main]
 
 use esp_hal::{
-    i2c::master::{Config, I2c, Operation},
+    i2c::master::{Config, Error, I2c, Operation},
     Async,
     Blocking,
 };
@@ -24,6 +24,9 @@ fn _async_driver_is_compatible_with_blocking_ehal() {
     fn _with_ehal(_: impl embedded_hal::i2c::I2c) {}
 }
 
+const DUT_ADDRESS: u8 = 0x77;
+const NON_EXISTENT_ADDRESS: u8 = 0x6b;
+
 #[cfg(test)]
 #[embedded_test::tests]
 mod tests {
@@ -44,17 +47,31 @@ mod tests {
         Context { i2c }
     }
 
+    #[test]
+    #[timeout(3)]
+    fn empty_write_returns_ack_error_for_unknown_address(mut ctx: Context) {
+        assert_eq!(
+            ctx.i2c.write(NON_EXISTENT_ADDRESS, &[]),
+            Err(Error::AckCheckFailed)
+        );
+        assert_eq!(ctx.i2c.write(DUT_ADDRESS, &[]), Ok(()));
+    }
+
     #[test]
     #[timeout(3)]
     fn test_read_cali(mut ctx: Context) {
         let mut read_data = [0u8; 22];
 
         // have a failing read which might could leave the peripheral in an undesirable
         // state
-        ctx.i2c.write_read(0x55, &[0xaa], &mut read_data).ok();
+        ctx.i2c
+            .write_read(NON_EXISTENT_ADDRESS, &[0xaa], &mut read_data)
+            .ok();
 
         // do the real read which should succeed
-        ctx.i2c.write_read(0x77, &[0xaa], &mut read_data).ok();
+        ctx.i2c
+            .write_read(DUT_ADDRESS, &[0xaa], &mut read_data)
+            .ok();
 
         assert_ne!(read_data, [0u8; 22])
     }
@@ -67,7 +84,7 @@ mod tests {
         // do the real read which should succeed
         ctx.i2c
             .transaction(
-                0x77,
+                DUT_ADDRESS,
                 &mut [Operation::Write(&[0xaa]), Operation::Read(&mut read_data)],
             )
             .ok();