Validate 7-bit I2C addresses (#3343)

* 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 <bugadani@gmail.com>

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

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
+    /// Note that 7-bit addresses are specified in **right-aligned** form, e.g.
-    /// **right-aligned** form, e.g. in the range `0x00..=0x7F`.
+    /// 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<u8> 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<Item = Operation<'a>>,
     ) -> Result<(), Error> {
+        address.validate()?;
+
         let mut last_op: Option<OpKind> = None;
         // filter out 0 length read operations
         let mut op_iter = operations
@@ -866,6 +889,8 @@ where
         address: I2cAddress,
         operations: impl Iterator<Item = Operation<'a>>,
     ) -> Result<(), Error> {
+        address.validate()?;
+
         let mut last_op: Option<OpKind> = 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

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