From 286219707cf3f8fd5a2d1e475b81f68dfec2076a Mon Sep 17 00:00:00 2001 From: Kirill Mikhailov <62840029+playfulFence@users.noreply.github.com> Date: Fri, 20 Dec 2024 14:18:28 +0100 Subject: [PATCH] I2C: Prefer compile-time checks over runtime checks where possible, prefer a fallible API over panics. (#2831) * I2C module improvements (raw) * fmt * dumb dumb2 dumb 3 dumb 4 dumb 5 * re-use `ExceedingFifo` instead of new `ExceedingTransactionSize` error * changelog entry * address reviews * rebase * dumb 6 * rebase ... --- esp-hal/CHANGELOG.md | 1 + esp-hal/src/i2c/master/mod.rs | 76 +++++++++++++++++++++++++---------- 2 files changed, 56 insertions(+), 21 deletions(-) diff --git a/esp-hal/CHANGELOG.md b/esp-hal/CHANGELOG.md index 0fce97d4ae..be5c0d5048 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: 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 6e3ee3ad93..b5de15d0cc 100644 --- a/esp-hal/src/i2c/master/mod.rs +++ b/esp-hal/src/i2c/master/mod.rs @@ -130,13 +130,21 @@ 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 { + /// Provided bus frequency is invalid for the current configuration. + InvalidFrequency, +} impl core::error::Error for ConfigError {} impl core::fmt::Display for ConfigError { - fn fmt(&self, _f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result { - Ok(()) + fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result { + match self { + ConfigError::InvalidFrequency => write!( + f, + "Provided bus frequency is invalid for the current configuration" + ), + } } } @@ -907,7 +915,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))] @@ -921,10 +929,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().unwrap()); + w.scl_wait_high_period().bits(scl_wait_high_period); w.scl_high_period().bits(scl_high_period as u16) }); @@ -969,6 +981,7 @@ fn configure_clock( } } } + Ok(()) } /// Peripheral data describing a particular I2C instance. @@ -1064,7 +1077,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(); @@ -1108,7 +1121,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) { + 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 +1193,21 @@ impl Driver<'_> { scl_start_hold_time, scl_stop_hold_time, timeout, - ); + )?; + + Ok(()) } #[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 +1250,21 @@ impl Driver<'_> { scl_start_hold_time, scl_stop_hold_time, timeout.map(|to_bus| (to_bus * 2 * half_cycle).min(0xFF_FFFF)), - ); + )?; + + Ok(()) } #[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 +1327,15 @@ impl Driver<'_> { let raw = if to_peri != 1 << log2 { log2 + 1 } else { log2 }; raw.min(0x1F) }), - ); + )?; + + Ok(()) } #[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::FifoExceeded); } self.wait_for_completion(false).await?; @@ -1474,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 { - panic!("On ESP32 and ESP32-S2 the max I2C read is limited to 32 bytes"); + return Err(Error::FifoExceeded); } // wait for completion - then we can just read the data from FIFO @@ -1707,7 +1741,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 @@ -1732,7 +1766,7 @@ impl Driver<'_> { .int_clr() .write(|w| w.txfifo_ovf().clear_bit_by_one()); } - index + Ok(index) } #[cfg(not(any(esp32, esp32s2)))] @@ -1782,20 +1816,20 @@ 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 if bytes.len() > 31 { - panic!("On ESP32 and ESP32-S2 the max I2C transfer is limited to 31 bytes"); + return Err(Error::FifoExceeded); } for b in bytes { write_fifo(self.register_block(), *b); } - bytes.len() + Ok(bytes.len()) } #[cfg(any(esp32, esp32s2))] @@ -1893,7 +1927,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)