I2C: add apply_config, implement SetConfig, add with_sda and with_scl (#2477)

* Implement apply_config, SetConfig

* Remove pins from constructors

* Implement timeout changes, document them

* Fix up changelog

esp-hal/CHANGELOG.md

@@ -33,9 +33,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - `{Uart, UartRx, UartTx}` now implement `embassy_embedded_hal::SetConfig` (#2449)
 - GPIO ETM tasks and events now accept `InputSignal` and `OutputSignal` (#2427)
 - `spi::master::Config` and `{Spi, SpiDma, SpiDmaBus}::apply_config` (#2448)
-- `embassy_embedded_hal::SetConfig` is now implemented for `{Spi, SpiDma, SpiDmaBus}` (#2448)
+- `embassy_embedded_hal::SetConfig` is now implemented for `spi::master::{Spi, SpiDma, SpiDmaBus}`, `i2c::master::I2c` (#2448, #2477)
 - `slave::Spi::{with_mosi(), with_miso(), with_sclk(), with_cs()}` functions (#2485)
 - I8080: Added `set_8bits_order()` to set the byte order in 8-bit mode (#2487)
+- `I2c::{apply_config(), with_sda(), with_scl()}` (#2477)
 
 ### Changed
 
@@ -59,6 +60,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - The I2S driver has been moved to `i2s::master` (#2472)
 - `slave::Spi` constructors no longer take pins (#2485)
 - The `I2c` master driver has been moved from `esp_hal::i2c` to `esp_hal::i2c::master`. (#2476)
+- `I2c` SCL timeout is now defined in bus clock cycles. (#2477)
 
 ### Fixed
 
@@ -75,6 +77,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - The `i2s::{I2sWrite, I2sWriteDma, I2sRead, I2sReadDma, I2sWriteDmaAsync, I2sReadDmaAsync}` traits have been removed. (#2316)
 - The `ledc::ChannelHW` trait is no longer generic. (#2387)
 - The `I2c::new_with_timeout` constructors have been removed (#2361)
+- `I2c::new()` no longer takes `frequency` and pins as parameters. (#2477)
 - The `spi::master::HalfDuplexReadWrite` trait has been removed. (#2373)
 - The `Spi::with_pins` methods have been removed. (#2373)
 - The `Spi::new_half_duplex` constructor have been removed. (#2373)

esp-hal/MIGRATING-0.21.md

@@ -69,13 +69,42 @@ let spi: Spi<'static, FullDuplexMode, SPI2> = Spi::new_typed(peripherals.SPI2, 1
 
 The I2C master driver and related types have been moved to `esp_hal::i2c::master`.
 
-The `with_timeout` constructors have been removed in favour of `set_timeout` or `with_timeout`.
+The `with_timeout` constructors have been removed. `new` and `new_typed` now take a `Config` struct
+with the available configuration options.
+
+- The default configuration is now:
+  - bus frequency: 100 kHz
+  - timeout: about 10 bus clock cycles
+
+The constructors no longer take pins. Use `with_sda` and `with_scl` instead.
 
 ```diff
+-use esp_hal::i2c::I2c;
++use esp_hal::i2c::{Config, I2c};
 -let i2c = I2c::new_with_timeout(peripherals.I2C0, io.pins.gpio4, io.pins.gpio5, 100.kHz(), timeout);
-+let i2c = I2c::new(peripherals.I2C0, io.pins.gpio4, io.pins.gpio5, 100.kHz()).with_timeout(timeout);
++I2c::new_with_config(
++    peripherals.I2C0,
++    {
++        let mut config = Config::default();
++        config.frequency = 100.kHz();
++        config.timeout = timeout;
++        config
++    },
++)
++.with_sda(io.pins.gpio4)
++.with_scl(io.pins.gpio5);
 ```
 
+### The calculation of I2C timeout has changed
+
+Previously, I2C timeouts were counted in increments of I2C peripheral clock cycles. This meant that
+the configure value meant different lengths of time depending on the device. With this update, the
+I2C configuration now expects the timeout value in number of bus clock cycles, which is consistent
+between devices.
+
+ESP32 and ESP32-S2 use an exact number of clock cycles for its timeout. Other MCUs, however, use
+the `2^timeout` value internally, and the HAL rounds up the timeout to the next appropriate value.
+
 ## Changes to half-duplex SPI
 
 The `HalfDuplexMode` and `FullDuplexMode` type parameters have been removed from SPI master and slave

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

@@ -18,7 +18,7 @@
 //!
 //! ```rust, no_run
 #![doc = crate::before_snippet!()]
-//! # use esp_hal::i2c::master::I2c;
+//! # use esp_hal::i2c::master::{Config, I2c};
 //! # use esp_hal::gpio::Io;
 //! let io = Io::new(peripherals.GPIO, peripherals.IO_MUX);
 //!
@@ -26,10 +26,10 @@
 //! // and standard I2C clock speed.
 //! let mut i2c = I2c::new(
 //!     peripherals.I2C0,
-//!     io.pins.gpio1,
+//!     Config::default(),
-//!     io.pins.gpio2,
+//! )
-//!     100.kHz(),
+//! .with_sda(io.pins.gpio1)
-//! );
+//! .with_scl(io.pins.gpio2);
 //!
 //! loop {
 //!     let mut data = [0u8; 22];
@@ -46,6 +46,7 @@ use core::{
     task::{Context, Poll},
 };
 
+use embassy_embedded_hal::SetConfig;
 use embassy_sync::waitqueue::AtomicWaker;
 use embedded_hal::i2c::Operation as EhalOperation;
 use fugit::HertzU32;
@@ -106,6 +107,11 @@ pub enum Error {
     InvalidZeroLength,
 }
 
+/// I2C-specific configuration errors
+#[derive(Debug, Clone, Copy, PartialEq)]
+#[cfg_attr(feature = "defmt", derive(defmt::Format))]
+pub enum ConfigError {}
+
 #[derive(PartialEq)]
 // This enum is used to keep track of the last/next operation that was/will be
 // performed in an embedded-hal(-async) I2c::transaction. It is used to
@@ -230,12 +236,54 @@ impl From<Ack> for u32 {
     }
 }
 
+/// I2C driver configuration
+#[derive(Debug, Clone, Copy)]
+#[cfg_attr(feature = "defmt", derive(defmt::Format))]
+pub struct Config {
+    /// The I2C clock frequency.
+    pub frequency: HertzU32,
+
+    /// I2C SCL timeout period.
+    ///
+    /// When the level of SCL remains unchanged for more than `timeout` bus
+    /// clock cycles, the bus goes to idle state.
+    ///
+    /// The default value is about 10 bus clock cycles.
+    #[doc = ""]
+    #[cfg_attr(
+        not(esp32),
+        doc = "Note that the effective timeout may be longer than the value configured here."
+    )]
+    #[cfg_attr(not(esp32), doc = "Configuring `None` disables timeout control.")]
+    #[cfg_attr(esp32, doc = "Configuring `None` equals to the maximum timeout value.")]
+    // TODO: when supporting interrupts, document that SCL = high also triggers an interrupt.
+    pub timeout: Option<u32>,
+}
+
+impl Default for Config {
+    fn default() -> Self {
+        use fugit::RateExtU32;
+        Config {
+            frequency: 100.kHz(),
+            timeout: Some(10),
+        }
+    }
+}
+
 /// I2C driver
 pub struct I2c<'d, DM: Mode, T = AnyI2c> {
     i2c: PeripheralRef<'d, T>,
     phantom: PhantomData<DM>,
-    frequency: HertzU32,
+    config: Config,
-    timeout: Option<u32>,
+}
+
+impl<T: Instance, DM: Mode> SetConfig for I2c<'_, DM, T> {
+    type Config = Config;
+    type ConfigError = ConfigError;
+
+    fn set_config(&mut self, config: &Self::Config) -> Result<(), Self::ConfigError> {
+        self.apply_config(config)
+    }
 }
 
 impl<T> embedded_hal_02::blocking::i2c::Read for I2c<'_, Blocking, T>
@@ -297,31 +345,6 @@ impl<'d, T, DM: Mode> I2c<'d, DM, T>
 where
     T: Instance,
 {
-    fn new_internal(
-        i2c: impl Peripheral<P = T> + 'd,
-        sda: impl Peripheral<P = impl PeripheralOutput> + 'd,
-        scl: impl Peripheral<P = impl PeripheralOutput> + 'd,
-        frequency: HertzU32,
-    ) -> Self {
-        crate::into_ref!(i2c);
-        crate::into_mapped_ref!(sda, scl);
-
-        let i2c = I2c {
-            i2c,
-            phantom: PhantomData,
-            frequency,
-            timeout: None,
-        };
-
-        PeripheralClockControl::reset(i2c.info().peripheral);
-        PeripheralClockControl::enable(i2c.info().peripheral);
-
-        let i2c = i2c.with_sda(sda).with_scl(scl);
-
-        i2c.info().setup(frequency, None);
-        i2c
-    }
-
     fn info(&self) -> &Info {
         self.i2c.info()
     }
@@ -330,16 +353,15 @@ where
         PeripheralClockControl::reset(self.info().peripheral);
         PeripheralClockControl::enable(self.info().peripheral);
 
-        self.info().setup(self.frequency, self.timeout);
+        // We know the configuration is valid, we can ignore the result.
+        _ = self.info().setup(&self.config);
     }
 
-    /// Set the I2C timeout.
+    /// Applies a new configuration.
-    // TODO: explain this function better - what's the unit, what happens on
+    pub fn apply_config(&mut self, config: &Config) -> Result<(), ConfigError> {
-    // timeout, and just what exactly is a timeout in this context?
+        self.info().setup(config)?;
-    pub fn with_timeout(mut self, timeout: Option<u32>) -> Self {
+        self.config = *config;
-        self.timeout = timeout;
+        Ok(())
-        self.info().setup(self.frequency, self.timeout);
-        self
     }
 
     fn transaction_impl<'a>(
@@ -399,14 +421,16 @@ where
         Ok(())
     }
 
-    fn with_sda(self, sda: impl Peripheral<P = impl PeripheralOutput> + 'd) -> Self {
+    /// Connect a pin to the I2C SDA signal.
+    pub fn with_sda(self, sda: impl Peripheral<P = impl PeripheralOutput> + 'd) -> Self {
         let info = self.info();
         let input = info.sda_input;
         let output = info.sda_output;
         self.with_pin(sda, input, output)
     }
 
-    fn with_scl(self, scl: impl Peripheral<P = impl PeripheralOutput> + 'd) -> Self {
+    /// Connect a pin to the I2C SCL signal.
+    pub fn with_scl(self, scl: impl Peripheral<P = impl PeripheralOutput> + 'd) -> Self {
         let info = self.info();
         let input = info.scl_input;
         let output = info.scl_output;
@@ -438,13 +462,8 @@ impl<'d> I2c<'d, Blocking> {
     /// Create a new I2C instance
     /// This will enable the peripheral but the peripheral won't get
     /// automatically disabled when this gets dropped.
-    pub fn new(
+    pub fn new(i2c: impl Peripheral<P = impl Instance> + 'd, config: Config) -> Self {
-        i2c: impl Peripheral<P = impl Instance> + 'd,
+        Self::new_typed(i2c.map_into(), config)
-        sda: impl Peripheral<P = impl PeripheralOutput> + 'd,
-        scl: impl Peripheral<P = impl PeripheralOutput> + 'd,
-        frequency: HertzU32,
-    ) -> Self {
-        Self::new_typed(i2c.map_into(), sda, scl, frequency)
     }
 }
 
@@ -455,13 +474,20 @@ where
     /// Create a new I2C instance
     /// This will enable the peripheral but the peripheral won't get
     /// automatically disabled when this gets dropped.
-    pub fn new_typed(
+    pub fn new_typed(i2c: impl Peripheral<P = T> + 'd, config: Config) -> Self {
-        i2c: impl Peripheral<P = T> + 'd,
+        crate::into_ref!(i2c);
-        sda: impl Peripheral<P = impl PeripheralOutput> + 'd,
+
-        scl: impl Peripheral<P = impl PeripheralOutput> + 'd,
+        let i2c = I2c {
-        frequency: HertzU32,
+            i2c,
-    ) -> Self {
+            phantom: PhantomData,
-        Self::new_internal(i2c, sda, scl, frequency)
+            config,
+        };
+
+        PeripheralClockControl::reset(i2c.info().peripheral);
+        PeripheralClockControl::enable(i2c.info().peripheral);
+
+        unwrap!(i2c.info().setup(&i2c.config));
+        i2c
     }
 
     // TODO: missing interrupt APIs
@@ -473,8 +499,7 @@ where
         I2c {
             i2c: self.i2c,
             phantom: PhantomData,
-            frequency: self.frequency,
+            config: self.config,
-            timeout: self.timeout,
         }
     }
 
@@ -743,8 +768,7 @@ where
         I2c {
             i2c: self.i2c,
             phantom: PhantomData,
-            frequency: self.frequency,
+            config: self.config,
-            timeout: self.timeout,
         }
     }
 
@@ -1216,8 +1240,7 @@ fn configure_clock(
     scl_stop_setup_time: u32,
     scl_start_hold_time: u32,
     scl_stop_hold_time: u32,
-    time_out_value: u32,
+    timeout: Option<u32>,
-    time_out_en: bool,
 ) {
     unsafe {
         // divider
@@ -1267,19 +1290,15 @@ fn configure_clock(
         // timeout mechanism
         cfg_if::cfg_if! {
             if #[cfg(esp32)] {
-                // timeout
                 register_block
                     .to()
-                    .write(|w| w.time_out().bits(time_out_value));
+                    .write(|w| w.time_out().bits(unwrap!(timeout)));
             } else {
-                // timeout
-                // FIXME: Enable timout for other chips!
-                #[allow(clippy::useless_conversion)]
                 register_block
                     .to()
-                    .write(|w| w.time_out_en().bit(time_out_en)
+                    .write(|w| w.time_out_en().bit(timeout.is_some())
                     .time_out_value()
-                    .bits(time_out_value.try_into().unwrap())
+                    .bits(timeout.unwrap_or(1) as _)
                 );
             }
         }
@@ -1324,7 +1343,7 @@ impl Info {
 
     /// Configures the I2C peripheral with the specified frequency, clocks, and
     /// optional timeout.
-    fn setup(&self, frequency: HertzU32, timeout: Option<u32>) {
+    fn setup(&self, config: &Config) -> Result<(), ConfigError> {
         self.register_block().ctr().write(|w| {
             // Set I2C controller to master mode
             w.ms_mode().set_bit();
@@ -1358,12 +1377,14 @@ impl Info {
                 let clock = clocks.xtal_clock.convert();
             }
         }
-        self.set_frequency(clock, frequency, timeout);
+        self.set_frequency(clock, config.frequency, config.timeout);
 
         self.update_config();
 
         // Reset entire peripheral (also resets fifo)
         self.reset();
+
+        Ok(())
     }
 
     /// Resets the I2C controller (FIFO + FSM + command list)
@@ -1411,8 +1432,9 @@ impl Info {
         let sda_sample = scl_high / 2;
         let setup = half_cycle;
         let hold = half_cycle;
-        // default we set the timeout value to 10 bus cycles
+        let timeout = timeout.map_or(Some(0xF_FFFF), |to_bus| {
-        let time_out_value = timeout.unwrap_or(half_cycle * 20);
+            Some((to_bus * 2 * half_cycle).min(0xF_FFFF))
+        });
 
         // SCL period. According to the TRM, we should always subtract 1 to SCL low
         // period
@@ -1465,8 +1487,7 @@ impl Info {
             scl_stop_setup_time,
             scl_start_hold_time,
             scl_stop_hold_time,
-            time_out_value,
+            timeout,
-            true,
         );
     }
 
@@ -1489,8 +1510,6 @@ impl Info {
         let sda_sample = half_cycle / 2 - 1;
         let setup = half_cycle;
         let hold = half_cycle;
-        // default we set the timeout value to 10 bus cycles
-        let time_out_value = timeout.unwrap_or(half_cycle * 20);
 
         // scl period
         let scl_low_period = scl_low - 1;
@@ -1505,7 +1524,6 @@ impl Info {
         // hold
         let scl_start_hold_time = hold - 1;
         let scl_stop_hold_time = hold;
-        let time_out_en = true;
 
         configure_clock(
             self.register_block(),
@@ -1519,8 +1537,7 @@ impl Info {
             scl_stop_setup_time,
             scl_start_hold_time,
             scl_stop_hold_time,
-            time_out_value,
+            timeout.map(|to_bus| (to_bus * 2 * half_cycle).min(0xFF_FFFF)),
-            time_out_en,
         );
     }
 
@@ -1552,14 +1569,6 @@ impl Info {
         let setup = half_cycle;
         let hold = half_cycle;
 
-        let time_out_value = if let Some(timeout) = timeout {
-            timeout
-        } else {
-            // default we set the timeout value to about 10 bus cycles
-            // log(20*half_cycle)/log(2) = log(half_cycle)/log(2) +  log(20)/log(2)
-            (4 * 8 - (5 * half_cycle).leading_zeros()) + 2
-        };
-
         // According to the Technical Reference Manual, the following timings must be
         // subtracted by 1. However, according to the practical measurement and
         // some hardware behaviour, if wait_high_period and scl_high minus one.
@@ -1579,7 +1588,6 @@ impl Info {
         // hold
         let scl_start_hold_time = hold - 1;
         let scl_stop_hold_time = hold - 1;
-        let time_out_en = true;
 
         configure_clock(
             self.register_block(),
@@ -1593,8 +1601,13 @@ impl Info {
             scl_stop_setup_time,
             scl_start_hold_time,
             scl_stop_hold_time,
-            time_out_value,
+            timeout.map(|to_bus| {
-            time_out_en,
+                let to_peri = (to_bus * 2 * half_cycle).max(1);
+                let log2 = to_peri.ilog2();
+                // Round up so that we don't shorten timeouts.
+                let raw = if to_peri != 1 << log2 { log2 + 1 } else { log2 };
+                raw.min(0x1F)
+            }),
         );
     }
 

examples/src/bin/embassy_i2c.rs

@@ -19,7 +19,12 @@
 use embassy_executor::Spawner;
 use embassy_time::{Duration, Timer};
 use esp_backtrace as _;
-use esp_hal::{gpio::Io, i2c::master::I2c, prelude::*, timer::timg::TimerGroup};
+use esp_hal::{
+    gpio::Io,
+    i2c::master::{Config, I2c},
+    prelude::*,
+    timer::timg::TimerGroup,
+};
 use lis3dh_async::{Lis3dh, Range, SlaveAddr};
 
 #[esp_hal_embassy::main]
@@ -31,7 +36,14 @@ async fn main(_spawner: Spawner) {
 
     let io = Io::new(peripherals.GPIO, peripherals.IO_MUX);
 
-    let i2c0 = I2c::new(peripherals.I2C0, io.pins.gpio4, io.pins.gpio5, 400.kHz()).into_async();
+    let i2c0 = I2c::new(peripherals.I2C0, {
+        let mut config = Config::default();
+        config.frequency = 400.kHz();
+        config
+    })
+    .with_sda(io.pins.gpio4)
+    .with_scl(io.pins.gpio5)
+    .into_async();
 
     let mut lis3dh = Lis3dh::new_i2c(i2c0, SlaveAddr::Alternate).await.unwrap();
     lis3dh.set_range(Range::G8).await.unwrap();

examples/src/bin/embassy_i2c_bmp180_calibration_data.rs

@@ -20,7 +20,12 @@
 use embassy_executor::Spawner;
 use embassy_time::{Duration, Timer};
 use esp_backtrace as _;
-use esp_hal::{gpio::Io, i2c::master::I2c, prelude::*, timer::timg::TimerGroup};
+use esp_hal::{
+    gpio::Io,
+    i2c::master::{Config, I2c},
+    prelude::*,
+    timer::timg::TimerGroup,
+};
 
 #[esp_hal_embassy::main]
 async fn main(_spawner: Spawner) {
@@ -31,7 +36,14 @@ async fn main(_spawner: Spawner) {
 
     let io = Io::new(peripherals.GPIO, peripherals.IO_MUX);
 
-    let mut i2c = I2c::new(peripherals.I2C0, io.pins.gpio4, io.pins.gpio5, 400.kHz()).into_async();
+    let mut i2c = I2c::new(peripherals.I2C0, {
+        let mut config = Config::default();
+        config.frequency = 400.kHz();
+        config
+    })
+    .with_sda(io.pins.gpio4)
+    .with_scl(io.pins.gpio5)
+    .into_async();
 
     loop {
         let mut data = [0u8; 22];

examples/src/bin/i2c_bmp180_calibration_data.rs

@@ -12,7 +12,11 @@
 #![no_main]
 
 use esp_backtrace as _;
-use esp_hal::{gpio::Io, i2c::master::I2c, prelude::*};
+use esp_hal::{
+    gpio::Io,
+    i2c::master::{Config, I2c},
+    prelude::*,
+};
 use esp_println::println;
 
 #[entry]
@@ -23,7 +27,9 @@ fn main() -> ! {
 
     // Create a new peripheral object with the described wiring and standard
     // I2C clock speed:
-    let mut i2c = I2c::new(peripherals.I2C0, io.pins.gpio4, io.pins.gpio5, 100.kHz());
+    let mut i2c = I2c::new(peripherals.I2C0, Config::default())
+        .with_sda(io.pins.gpio4)
+        .with_scl(io.pins.gpio5);
 
     loop {
         let mut data = [0u8; 22];

examples/src/bin/i2c_display.rs

@@ -22,7 +22,12 @@ use embedded_graphics::{
     text::{Alignment, Text},
 };
 use esp_backtrace as _;
-use esp_hal::{delay::Delay, gpio::Io, i2c::master::I2c, prelude::*};
+use esp_hal::{
+    delay::Delay,
+    gpio::Io,
+    i2c::master::{Config, I2c},
+    prelude::*,
+};
 use ssd1306::{prelude::*, I2CDisplayInterface, Ssd1306};
 
 #[entry]
@@ -34,7 +39,9 @@ fn main() -> ! {
 
     // Create a new peripheral object with the described wiring
     // and standard I2C clock speed
-    let i2c = I2c::new(peripherals.I2C0, io.pins.gpio4, io.pins.gpio5, 100.kHz());
+    let i2c = I2c::new(peripherals.I2C0, Config::default())
+        .with_sda(io.pins.gpio4)
+        .with_scl(io.pins.gpio5);
 
     // Initialize display
     let interface = I2CDisplayInterface::new(i2c);

examples/src/bin/lcd_cam_ov2640.rs

@@ -29,7 +29,10 @@ use esp_hal::{
     dma::{Dma, DmaPriority},
     dma_rx_stream_buffer,
     gpio::Io,
-    i2c::{self, master::I2c},
+    i2c::{
+        self,
+        master::{Config, I2c},
+    },
     lcd_cam::{
         cam::{Camera, RxEightBits},
         LcdCam,
@@ -79,7 +82,9 @@ fn main() -> ! {
 
     delay.delay_millis(500u32);
 
-    let i2c = I2c::new(peripherals.I2C0, cam_siod, cam_sioc, 100u32.kHz());
+    let i2c = I2c::new(peripherals.I2C0, Config::default())
+        .with_sda(cam_siod)
+        .with_scl(cam_sioc);
 
     let mut sccb = Sccb::new(i2c);
 

hil-test/tests/i2c.rs

@@ -7,8 +7,7 @@
 
 use esp_hal::{
     gpio::Io,
-    i2c::master::{I2c, Operation},
+    i2c::master::{Config, I2c, Operation},
-    prelude::*,
     Async,
     Blocking,
 };
@@ -40,7 +39,9 @@ mod tests {
 
         // Create a new peripheral object with the described wiring and standard
         // I2C clock speed:
-        let i2c = I2c::new(peripherals.I2C0, sda, scl, 100.kHz());
+        let i2c = I2c::new(peripherals.I2C0, Config::default())
+            .with_sda(sda)
+            .with_scl(scl);
 
         Context { i2c }
     }