From 69ea4bdf84479f7355332b792cde4b26e9154356 Mon Sep 17 00:00:00 2001 From: Kirill Mikhailov Date: Tue, 17 Dec 2024 16:09:00 +0100 Subject: [PATCH 1/9] I2C module improvements (raw) --- esp-hal/src/i2c/master/mod.rs | 36 ++++++++++++++++++++--------------- 1 file changed, 21 insertions(+), 15 deletions(-) diff --git a/esp-hal/src/i2c/master/mod.rs b/esp-hal/src/i2c/master/mod.rs index 6e3ee3ad93..cee850b297 100644 --- a/esp-hal/src/i2c/master/mod.rs +++ b/esp-hal/src/i2c/master/mod.rs @@ -130,7 +130,10 @@ impl core::fmt::Display for Error { #[derive(Debug, Clone, Copy, PartialEq)] #[cfg_attr(feature = "defmt", derive(defmt::Format))] #[non_exhaustive] -pub enum ConfigError {} +pub enum ConfigError { + InvalidFrequency, + InvalidTimeout, +} impl core::error::Error for ConfigError {} @@ -907,7 +910,7 @@ fn configure_clock( scl_start_hold_time: u32, scl_stop_hold_time: u32, timeout: Option, -) { +) -> Result<(), ConfigError> { unsafe { // divider #[cfg(any(esp32c2, esp32c3, esp32c6, esp32h2, esp32s3))] @@ -924,7 +927,7 @@ fn configure_clock( register_block.scl_high_period().write(|w| { #[cfg(not(esp32))] // ESP32 does not have a wait_high field w.scl_wait_high_period() - .bits(scl_wait_high_period.try_into().unwrap()); + .bits(scl_wait_high_period.try_into().map_err(|_| Error::InvalidZeroLength)?); w.scl_high_period().bits(scl_high_period as u16) }); @@ -956,9 +959,11 @@ fn configure_clock( // timeout mechanism cfg_if::cfg_if! { if #[cfg(esp32)] { - register_block - .to() - .write(|w| w.time_out().bits(unwrap!(timeout))); + if let Some(timeout_value) = timeout { + register_block.to().write(|w| w.time_out().bits(timeout_value)); + } else { + return Err(Error::InvalidZeroLength); + } } else { register_block .to() @@ -969,6 +974,7 @@ fn configure_clock( } } } + Ok(()) } /// Peripheral data describing a particular I2C instance. @@ -1108,7 +1114,7 @@ impl Driver<'_> { /// Sets the frequency of the I2C interface by calculating and applying the /// associated timings - corresponds to i2c_ll_cal_bus_clk and /// i2c_ll_set_bus_timing in ESP-IDF - fn set_frequency(&self, source_clk: HertzU32, bus_freq: HertzU32, timeout: Option) { + fn set_frequency(&self, source_clk: HertzU32, bus_freq: HertzU32, timeout: Option) -> Result<(), ConfigError> { let source_clk = source_clk.raw(); let bus_freq = bus_freq.raw(); @@ -1175,14 +1181,14 @@ impl Driver<'_> { scl_start_hold_time, scl_stop_hold_time, timeout, - ); + )?; } #[cfg(esp32s2)] /// Sets the frequency of the I2C interface by calculating and applying the /// associated timings - corresponds to i2c_ll_cal_bus_clk and /// i2c_ll_set_bus_timing in ESP-IDF - fn set_frequency(&self, source_clk: HertzU32, bus_freq: HertzU32, timeout: Option) { + fn set_frequency(&self, source_clk: HertzU32, bus_freq: HertzU32, timeout: Option) -> Result<(), ConfigError> { let source_clk = source_clk.raw(); let bus_freq = bus_freq.raw(); @@ -1225,14 +1231,14 @@ impl Driver<'_> { scl_start_hold_time, scl_stop_hold_time, timeout.map(|to_bus| (to_bus * 2 * half_cycle).min(0xFF_FFFF)), - ); + )?; } #[cfg(any(esp32c2, esp32c3, esp32c6, esp32h2, esp32s3))] /// Sets the frequency of the I2C interface by calculating and applying the /// associated timings - corresponds to i2c_ll_cal_bus_clk and /// i2c_ll_set_bus_timing in ESP-IDF - fn set_frequency(&self, source_clk: HertzU32, bus_freq: HertzU32, timeout: Option) { + fn set_frequency(&self, source_clk: HertzU32, bus_freq: HertzU32, timeout: Option) -> Result<(), ConfigError> { let source_clk = source_clk.raw(); let bus_freq = bus_freq.raw(); @@ -1295,13 +1301,13 @@ impl Driver<'_> { let raw = if to_peri != 1 << log2 { log2 + 1 } else { log2 }; raw.min(0x1F) }), - ); + )?; } #[cfg(any(esp32, esp32s2))] async fn read_all_from_fifo(&self, buffer: &mut [u8]) -> Result<(), Error> { if buffer.len() > 32 { - panic!("On ESP32 and ESP32-S2 the max I2C read is limited to 32 bytes"); + return Err(Error::ExceedingTransactionSize); } self.wait_for_completion(false).await?; @@ -1474,7 +1480,7 @@ impl Driver<'_> { // see https://github.com/espressif/arduino-esp32/blob/7e9afe8c5ed7b5bf29624a5cd6e07d431c027b97/cores/esp32/esp32-hal-i2c.c#L615 if buffer.len() > 32 { - panic!("On ESP32 and ESP32-S2 the max I2C read is limited to 32 bytes"); + return Err(Error::ExceedingTransactionSize); } // wait for completion - then we can just read the data from FIFO @@ -1788,7 +1794,7 @@ impl Driver<'_> { // see https://github.com/espressif/arduino-esp32/blob/7e9afe8c5ed7b5bf29624a5cd6e07d431c027b97/cores/esp32/esp32-hal-i2c.c#L615 if bytes.len() > 31 { - panic!("On ESP32 and ESP32-S2 the max I2C transfer is limited to 31 bytes"); + return Err(Error::ExceedingTransactionSize); } for b in bytes { From bf874a91b2c314bddcba446ad13f5f88a870ef15 Mon Sep 17 00:00:00 2001 From: Kirill Mikhailov Date: Wed, 18 Dec 2024 10:19:41 +0100 Subject: [PATCH 2/9] fmt --- esp-hal/src/i2c/master/mod.rs | 33 ++++++++++++++++++++++++++------- 1 file changed, 26 insertions(+), 7 deletions(-) diff --git a/esp-hal/src/i2c/master/mod.rs b/esp-hal/src/i2c/master/mod.rs index cee850b297..74a07749fb 100644 --- a/esp-hal/src/i2c/master/mod.rs +++ b/esp-hal/src/i2c/master/mod.rs @@ -131,6 +131,7 @@ impl core::fmt::Display for Error { #[cfg_attr(feature = "defmt", derive(defmt::Format))] #[non_exhaustive] pub enum ConfigError { + /// Provided bus frequency is invalid for the current configuration. InvalidFrequency, InvalidTimeout, } @@ -926,8 +927,11 @@ fn configure_clock( register_block.scl_high_period().write(|w| { #[cfg(not(esp32))] // ESP32 does not have a wait_high field - w.scl_wait_high_period() - .bits(scl_wait_high_period.try_into().map_err(|_| Error::InvalidZeroLength)?); + w.scl_wait_high_period().bits( + scl_wait_high_period + .try_into() + .map_err(|_| Error::InvalidFrequency)?, + ); w.scl_high_period().bits(scl_high_period as u16) }); @@ -962,7 +966,7 @@ fn configure_clock( if let Some(timeout_value) = timeout { register_block.to().write(|w| w.time_out().bits(timeout_value)); } else { - return Err(Error::InvalidZeroLength); + return Err(Error::InvalidTimeout); } } else { register_block @@ -1070,7 +1074,7 @@ impl Driver<'_> { let clock = clocks.xtal_clock.convert(); } } - self.set_frequency(clock, config.frequency, config.timeout); + self.set_frequency(clock, config.frequency, config.timeout)?; self.update_config(); @@ -1114,7 +1118,12 @@ impl Driver<'_> { /// Sets the frequency of the I2C interface by calculating and applying the /// associated timings - corresponds to i2c_ll_cal_bus_clk and /// i2c_ll_set_bus_timing in ESP-IDF - fn set_frequency(&self, source_clk: HertzU32, bus_freq: HertzU32, timeout: Option) -> Result<(), ConfigError> { + fn set_frequency( + &self, + source_clk: HertzU32, + bus_freq: HertzU32, + timeout: Option, + ) -> Result<(), ConfigError> { let source_clk = source_clk.raw(); let bus_freq = bus_freq.raw(); @@ -1188,7 +1197,12 @@ impl Driver<'_> { /// Sets the frequency of the I2C interface by calculating and applying the /// associated timings - corresponds to i2c_ll_cal_bus_clk and /// i2c_ll_set_bus_timing in ESP-IDF - fn set_frequency(&self, source_clk: HertzU32, bus_freq: HertzU32, timeout: Option) -> Result<(), ConfigError> { + fn set_frequency( + &self, + source_clk: HertzU32, + bus_freq: HertzU32, + timeout: Option, + ) -> Result<(), ConfigError> { let source_clk = source_clk.raw(); let bus_freq = bus_freq.raw(); @@ -1238,7 +1252,12 @@ impl Driver<'_> { /// Sets the frequency of the I2C interface by calculating and applying the /// associated timings - corresponds to i2c_ll_cal_bus_clk and /// i2c_ll_set_bus_timing in ESP-IDF - fn set_frequency(&self, source_clk: HertzU32, bus_freq: HertzU32, timeout: Option) -> Result<(), ConfigError> { + fn set_frequency( + &self, + source_clk: HertzU32, + bus_freq: HertzU32, + timeout: Option, + ) -> Result<(), ConfigError> { let source_clk = source_clk.raw(); let bus_freq = bus_freq.raw(); From 808e45d59f09622b1d0e66dc0b9ca9127d15b271 Mon Sep 17 00:00:00 2001 From: Kirill Mikhailov Date: Wed, 18 Dec 2024 10:42:01 +0100 Subject: [PATCH 3/9] dumb dumb2 dumb 3 dumb 4 dumb 5 --- esp-hal/src/i2c/master/mod.rs | 30 +++++++++++++++++++----------- 1 file changed, 19 insertions(+), 11 deletions(-) diff --git a/esp-hal/src/i2c/master/mod.rs b/esp-hal/src/i2c/master/mod.rs index 74a07749fb..13ebfb8dc2 100644 --- a/esp-hal/src/i2c/master/mod.rs +++ b/esp-hal/src/i2c/master/mod.rs @@ -133,6 +133,7 @@ impl core::fmt::Display for Error { pub enum ConfigError { /// Provided bus frequency is invalid for the current configuration. InvalidFrequency, + /// The specified timeout passed to `Config` is invalid. InvalidTimeout, } @@ -925,13 +926,14 @@ fn configure_clock( .scl_low_period() .write(|w| w.scl_low_period().bits(scl_low_period as u16)); + #[cfg(not(esp32))] + let scl_wait_high_period = scl_wait_high_period + .try_into() + .map_err(|_| ConfigError::InvalidFrequency)?; + register_block.scl_high_period().write(|w| { #[cfg(not(esp32))] // ESP32 does not have a wait_high field - w.scl_wait_high_period().bits( - scl_wait_high_period - .try_into() - .map_err(|_| Error::InvalidFrequency)?, - ); + w.scl_wait_high_period().bits(scl_wait_high_period); w.scl_high_period().bits(scl_high_period as u16) }); @@ -966,7 +968,7 @@ fn configure_clock( if let Some(timeout_value) = timeout { register_block.to().write(|w| w.time_out().bits(timeout_value)); } else { - return Err(Error::InvalidTimeout); + return Err(ConfigError::InvalidTimeout); } } else { register_block @@ -1191,6 +1193,8 @@ impl Driver<'_> { scl_stop_hold_time, timeout, )?; + + Ok(()) } #[cfg(esp32s2)] @@ -1246,6 +1250,8 @@ impl Driver<'_> { scl_stop_hold_time, timeout.map(|to_bus| (to_bus * 2 * half_cycle).min(0xFF_FFFF)), )?; + + Ok(()) } #[cfg(any(esp32c2, esp32c3, esp32c6, esp32h2, esp32s3))] @@ -1321,6 +1327,8 @@ impl Driver<'_> { raw.min(0x1F) }), )?; + + Ok(()) } #[cfg(any(esp32, esp32s2))] @@ -1732,7 +1740,7 @@ impl Driver<'_> { #[cfg(not(any(esp32, esp32s2)))] /// Fills the TX FIFO with data from the provided slice. - fn fill_tx_fifo(&self, bytes: &[u8]) -> usize { + fn fill_tx_fifo(&self, bytes: &[u8]) -> Result { let mut index = 0; while index < bytes.len() && !self @@ -1757,7 +1765,7 @@ impl Driver<'_> { .int_clr() .write(|w| w.txfifo_ovf().clear_bit_by_one()); } - index + Ok(index) } #[cfg(not(any(esp32, esp32s2)))] @@ -1807,7 +1815,7 @@ impl Driver<'_> { #[cfg(any(esp32, esp32s2))] /// Fills the TX FIFO with data from the provided slice. - fn fill_tx_fifo(&self, bytes: &[u8]) -> usize { + fn fill_tx_fifo(&self, bytes: &[u8]) -> Result { // on ESP32/ESP32-S2 we currently don't support I2C transactions larger than the // FIFO apparently it would be possible by using non-fifo mode // see https://github.com/espressif/arduino-esp32/blob/7e9afe8c5ed7b5bf29624a5cd6e07d431c027b97/cores/esp32/esp32-hal-i2c.c#L615 @@ -1820,7 +1828,7 @@ impl Driver<'_> { write_fifo(self.register_block(), *b); } - bytes.len() + Ok(bytes.len()) } #[cfg(any(esp32, esp32s2))] @@ -1918,7 +1926,7 @@ impl Driver<'_> { cmd_iterator, if stop { Command::Stop } else { Command::End }, )?; - let index = self.fill_tx_fifo(bytes); + let index = self.fill_tx_fifo(bytes)?; self.start_transmission(); Ok(index) From 2de74fdd8afe889c88b4058de60e57b862249a95 Mon Sep 17 00:00:00 2001 From: Kirill Mikhailov Date: Wed, 18 Dec 2024 12:08:14 +0100 Subject: [PATCH 4/9] re-use `ExceedingFifo` instead of new `ExceedingTransactionSize` error --- esp-hal/src/i2c/master/mod.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/esp-hal/src/i2c/master/mod.rs b/esp-hal/src/i2c/master/mod.rs index 13ebfb8dc2..0821f6ec17 100644 --- a/esp-hal/src/i2c/master/mod.rs +++ b/esp-hal/src/i2c/master/mod.rs @@ -1334,7 +1334,7 @@ impl Driver<'_> { #[cfg(any(esp32, esp32s2))] async fn read_all_from_fifo(&self, buffer: &mut [u8]) -> Result<(), Error> { if buffer.len() > 32 { - return Err(Error::ExceedingTransactionSize); + return Err(Error::ExceedingFifo); } self.wait_for_completion(false).await?; @@ -1507,7 +1507,7 @@ impl Driver<'_> { // see https://github.com/espressif/arduino-esp32/blob/7e9afe8c5ed7b5bf29624a5cd6e07d431c027b97/cores/esp32/esp32-hal-i2c.c#L615 if buffer.len() > 32 { - return Err(Error::ExceedingTransactionSize); + return Err(Error::ExceedingFifo); } // wait for completion - then we can just read the data from FIFO @@ -1821,7 +1821,7 @@ impl Driver<'_> { // see https://github.com/espressif/arduino-esp32/blob/7e9afe8c5ed7b5bf29624a5cd6e07d431c027b97/cores/esp32/esp32-hal-i2c.c#L615 if bytes.len() > 31 { - return Err(Error::ExceedingTransactionSize); + return Err(Error::ExceedingFifo); } for b in bytes { From a1f37fa1403f5f40cf3c64d29d8d64401625f7f0 Mon Sep 17 00:00:00 2001 From: Kirill Mikhailov Date: Wed, 18 Dec 2024 12:11:07 +0100 Subject: [PATCH 5/9] changelog entry --- esp-hal/CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/esp-hal/CHANGELOG.md b/esp-hal/CHANGELOG.md index 0fce97d4ae..be42abf060 100644 --- a/esp-hal/CHANGELOG.md +++ b/esp-hal/CHANGELOG.md @@ -86,6 +86,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - `SpiBitOrder`, `SpiDataMode`, `SpiMode` were renamed to `BitOder`, `DataMode` and `Mode` (#2828) - `crate::Mode` was renamed to `crate::DriverMode` (#2828) - Renamed some I2C error variants (#2844) +- I2C: Prefer compile-time checks over runtime checks where possible, prefer a fallible API over panics. (#2831) ### Fixed From 7c0690e533e90bc57d8c0e84bd04e0e36ac4cdaa Mon Sep 17 00:00:00 2001 From: Kirill Mikhailov Date: Thu, 19 Dec 2024 13:22:13 +0100 Subject: [PATCH 6/9] address reviews --- esp-hal/CHANGELOG.md | 1 + esp-hal/src/i2c/master/mod.rs | 10 +++------- 2 files changed, 4 insertions(+), 7 deletions(-) diff --git a/esp-hal/CHANGELOG.md b/esp-hal/CHANGELOG.md index be42abf060..51b0c899cd 100644 --- a/esp-hal/CHANGELOG.md +++ b/esp-hal/CHANGELOG.md @@ -87,6 +87,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - `crate::Mode` was renamed to `crate::DriverMode` (#2828) - Renamed some I2C error variants (#2844) - I2C: Prefer compile-time checks over runtime checks where possible, prefer a fallible API over panics. (#2831) +- I2C: Replaced potential panics with errors. (#2831) ### Fixed diff --git a/esp-hal/src/i2c/master/mod.rs b/esp-hal/src/i2c/master/mod.rs index 0821f6ec17..641553f045 100644 --- a/esp-hal/src/i2c/master/mod.rs +++ b/esp-hal/src/i2c/master/mod.rs @@ -133,8 +133,6 @@ impl core::fmt::Display for Error { pub enum ConfigError { /// Provided bus frequency is invalid for the current configuration. InvalidFrequency, - /// The specified timeout passed to `Config` is invalid. - InvalidTimeout, } impl core::error::Error for ConfigError {} @@ -965,11 +963,9 @@ fn configure_clock( // timeout mechanism cfg_if::cfg_if! { if #[cfg(esp32)] { - if let Some(timeout_value) = timeout { - register_block.to().write(|w| w.time_out().bits(timeout_value)); - } else { - return Err(ConfigError::InvalidTimeout); - } + register_block + .to() + .write(|w| w.time_out().bits(unwrap!(timeout))); } else { register_block .to() From 8948393bff606a00351cf7695eef35e7ff9d0a81 Mon Sep 17 00:00:00 2001 From: Kirill Mikhailov Date: Thu, 19 Dec 2024 14:10:31 +0100 Subject: [PATCH 7/9] rebase --- esp-hal/src/i2c/master/mod.rs | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/esp-hal/src/i2c/master/mod.rs b/esp-hal/src/i2c/master/mod.rs index 641553f045..eef7a88a5b 100644 --- a/esp-hal/src/i2c/master/mod.rs +++ b/esp-hal/src/i2c/master/mod.rs @@ -139,7 +139,12 @@ impl core::error::Error for ConfigError {} impl core::fmt::Display for ConfigError { fn fmt(&self, _f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result { - Ok(()) + match self { + ConfigError::InvalidFrequency => write!( + f, + "Provided bus frequency is invalid for the current configuration" + ), + } } } From 41c035ed79abe1bb549e886f6a523ad6316d8277 Mon Sep 17 00:00:00 2001 From: Kirill Mikhailov Date: Thu, 19 Dec 2024 14:16:40 +0100 Subject: [PATCH 8/9] dumb 6 --- esp-hal/src/i2c/master/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/esp-hal/src/i2c/master/mod.rs b/esp-hal/src/i2c/master/mod.rs index eef7a88a5b..f044ac599a 100644 --- a/esp-hal/src/i2c/master/mod.rs +++ b/esp-hal/src/i2c/master/mod.rs @@ -138,7 +138,7 @@ pub enum ConfigError { impl core::error::Error for ConfigError {} impl core::fmt::Display for ConfigError { - fn fmt(&self, _f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result { + fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result { match self { ConfigError::InvalidFrequency => write!( f, From f5278f792a5bd4a070ca8fd911fb2f98bef1fccf Mon Sep 17 00:00:00 2001 From: Kirill Mikhailov Date: Fri, 20 Dec 2024 13:10:39 +0100 Subject: [PATCH 9/9] rebase ... --- esp-hal/CHANGELOG.md | 1 - esp-hal/src/i2c/master/mod.rs | 6 +++--- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/esp-hal/CHANGELOG.md b/esp-hal/CHANGELOG.md index 51b0c899cd..be5c0d5048 100644 --- a/esp-hal/CHANGELOG.md +++ b/esp-hal/CHANGELOG.md @@ -86,7 +86,6 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - `SpiBitOrder`, `SpiDataMode`, `SpiMode` were renamed to `BitOder`, `DataMode` and `Mode` (#2828) - `crate::Mode` was renamed to `crate::DriverMode` (#2828) - Renamed some I2C error variants (#2844) -- I2C: Prefer compile-time checks over runtime checks where possible, prefer a fallible API over panics. (#2831) - I2C: Replaced potential panics with errors. (#2831) ### Fixed diff --git a/esp-hal/src/i2c/master/mod.rs b/esp-hal/src/i2c/master/mod.rs index f044ac599a..b5de15d0cc 100644 --- a/esp-hal/src/i2c/master/mod.rs +++ b/esp-hal/src/i2c/master/mod.rs @@ -1335,7 +1335,7 @@ impl Driver<'_> { #[cfg(any(esp32, esp32s2))] async fn read_all_from_fifo(&self, buffer: &mut [u8]) -> Result<(), Error> { if buffer.len() > 32 { - return Err(Error::ExceedingFifo); + return Err(Error::FifoExceeded); } self.wait_for_completion(false).await?; @@ -1508,7 +1508,7 @@ impl Driver<'_> { // see https://github.com/espressif/arduino-esp32/blob/7e9afe8c5ed7b5bf29624a5cd6e07d431c027b97/cores/esp32/esp32-hal-i2c.c#L615 if buffer.len() > 32 { - return Err(Error::ExceedingFifo); + return Err(Error::FifoExceeded); } // wait for completion - then we can just read the data from FIFO @@ -1822,7 +1822,7 @@ impl Driver<'_> { // see https://github.com/espressif/arduino-esp32/blob/7e9afe8c5ed7b5bf29624a5cd6e07d431c027b97/cores/esp32/esp32-hal-i2c.c#L615 if bytes.len() > 31 { - return Err(Error::ExceedingFifo); + return Err(Error::FifoExceeded); } for b in bytes {