From 523bd381ebd1dacfce62b621650a6a17fedc62ce Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bj=C3=B6rn=20Quentin?= Date: Tue, 8 Apr 2025 13:35:36 +0200 Subject: [PATCH] Validate 7-bit I2C addresses (#3343) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Validate 7-bit I2C addresses * add test * CHANGELOG.md * Docs * Include address in `Error::AddressInvalid` * Fix * CHANGELOG.md * Revert CHANGELOG change --------- Co-authored-by: Dániel Buga --- esp-hal/CHANGELOG.md | 1 + esp-hal/src/i2c/master/mod.rs | 35 +++++++++++++++++++++++++++++++++-- hil-test/tests/i2c.rs | 18 +++++++++++++++++- 3 files changed, 51 insertions(+), 3 deletions(-) diff --git a/esp-hal/CHANGELOG.md b/esp-hal/CHANGELOG.md index 6cdb3660e..29c37deba 100644 --- a/esp-hal/CHANGELOG.md +++ b/esp-hal/CHANGELOG.md @@ -50,6 +50,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Fix PCNT counter not keeping the peripheral enabled (#3334) - Fixed an issue where inverting a pin via the interconnect matrix was ineffective (#3312) - The half-duplex SPI APIs should accept more valid line width combinations (#3325) +- Passing an invalid seven bit I2C address is now rejected (#3343) - PARL_IO: Use correct max transfer size (#3346) ### Removed diff --git a/esp-hal/src/i2c/master/mod.rs b/esp-hal/src/i2c/master/mod.rs index bae8a9300..05357a270 100644 --- a/esp-hal/src/i2c/master/mod.rs +++ b/esp-hal/src/i2c/master/mod.rs @@ -79,17 +79,33 @@ const MAX_ITERATIONS: u32 = 1_000_000; pub enum I2cAddress { /// 7-bit address mode type. /// - /// Note that 7-bit addresses defined by drivers should be specified in - /// **right-aligned** form, e.g. in the range `0x00..=0x7F`. + /// Note that 7-bit addresses are specified in **right-aligned** form, e.g. + /// in the range `0x00..=0x7F`. /// /// For example, a device that has the seven bit address of `0b011_0010`, /// and therefore is addressed on the wire using: /// /// * `0b0110010_0` or `0x64` for *writes* /// * `0b0110010_1` or `0x65` for *reads* + /// + /// The above address is specified as 0b0011_0010 or 0x32, NOT 0x64 or 0x65. SevenBit(u8), } +impl I2cAddress { + fn validate(&self) -> Result<(), Error> { + match self { + I2cAddress::SevenBit(addr) => { + if *addr > 0x7F { + return Err(Error::AddressInvalid(*self)); + } + } + } + + Ok(()) + } +} + impl From for I2cAddress { fn from(value: u8) -> Self { I2cAddress::SevenBit(value) @@ -168,6 +184,8 @@ pub enum Error { CommandNumberExceeded, /// Zero length read or write operation. ZeroLengthInvalid, + /// The given address is invalid. + AddressInvalid(I2cAddress), } /// I2C no acknowledge error reason. @@ -231,6 +249,9 @@ impl core::fmt::Display for Error { write!(f, "The number of commands issued exceeded the limit") } Error::ZeroLengthInvalid => write!(f, "Zero length read or write operation"), + Error::AddressInvalid(address) => { + write!(f, "The given address ({:?}) is invalid", address) + } } } } @@ -794,6 +815,8 @@ impl<'d> I2c<'d, Async> { address: I2cAddress, operations: impl Iterator>, ) -> Result<(), Error> { + address.validate()?; + let mut last_op: Option = None; // filter out 0 length read operations let mut op_iter = operations @@ -866,6 +889,8 @@ where address: I2cAddress, operations: impl Iterator>, ) -> Result<(), Error> { + address.validate()?; + let mut last_op: Option = None; // filter out 0 length read operations let mut op_iter = operations @@ -2211,6 +2236,7 @@ impl Driver<'_> { bytes: &[u8], start: bool, ) -> Result<(), Error> { + address.validate()?; self.reset_fifo(); self.reset_command_list(); let cmd_iterator = &mut self.regs().comd_iter(); @@ -2244,6 +2270,7 @@ impl Driver<'_> { start: bool, will_continue: bool, ) -> Result<(), Error> { + address.validate()?; self.reset_fifo(); self.reset_command_list(); @@ -2275,6 +2302,7 @@ impl Driver<'_> { start: bool, stop: bool, ) -> Result<(), Error> { + address.validate()?; self.clear_all_interrupts(); // Short circuit for zero length writes without start or end as that would be an @@ -2311,6 +2339,7 @@ impl Driver<'_> { stop: bool, will_continue: bool, ) -> Result<(), Error> { + address.validate()?; self.clear_all_interrupts(); // Short circuit for zero length reads as that would be an invalid operation @@ -2364,6 +2393,7 @@ impl Driver<'_> { start: bool, stop: bool, ) -> Result<(), Error> { + address.validate()?; self.clear_all_interrupts(); // Short circuit for zero length writes without start or end as that would be an @@ -2400,6 +2430,7 @@ impl Driver<'_> { stop: bool, will_continue: bool, ) -> Result<(), Error> { + address.validate()?; self.clear_all_interrupts(); // Short circuit for zero length reads as that would be an invalid operation diff --git a/hil-test/tests/i2c.rs b/hil-test/tests/i2c.rs index 032ef0416..c6fc32fa7 100644 --- a/hil-test/tests/i2c.rs +++ b/hil-test/tests/i2c.rs @@ -7,7 +7,7 @@ #![no_main] use esp_hal::{ - i2c::master::{AcknowledgeCheckFailedReason, Config, Error, I2c, Operation}, + i2c::master::{AcknowledgeCheckFailedReason, Config, Error, I2c, I2cAddress, Operation}, Async, Blocking, }; @@ -49,6 +49,22 @@ mod tests { Context { i2c } } + #[test] + fn invalid_address_returns_error(mut ctx: Context) { + assert_eq!( + ctx.i2c.write(0x80, &[]), + Err(Error::AddressInvalid(I2cAddress::SevenBit(0x80))) + ); + assert_eq!( + ctx.i2c.read(0x80, &mut [0; 1]), + Err(Error::AddressInvalid(I2cAddress::SevenBit(0x80))) + ); + assert_eq!( + ctx.i2c.write_read(0x80, &[0x77], &mut [0; 1]), + Err(Error::AddressInvalid(I2cAddress::SevenBit(0x80))) + ); + } + #[test] fn empty_write_returns_ack_error_for_unknown_address(mut ctx: Context) { // on some chips we can determine the ack-check-failed reason but not on all