diff --git a/esp-hal/CHANGELOG.md b/esp-hal/CHANGELOG.md index d11e2fc515d..f57a9e67bbd 100644 --- a/esp-hal/CHANGELOG.md +++ b/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 diff --git a/esp-hal/src/i2c/master/mod.rs b/esp-hal/src/i2c/master/mod.rs index b336ea1b3af..167a1b855f7 100644 --- a/esp-hal/src/i2c/master/mod.rs +++ b/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 { + 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( diff --git a/hil-test/tests/i2c.rs b/hil-test/tests/i2c.rs index 9ea9f267c6b..8f786717c9b 100644 --- a/hil-test/tests/i2c.rs +++ b/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,6 +47,16 @@ 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) { @@ -51,10 +64,14 @@ mod tests { // 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();