From 151e66c3b3a1d0838526797df38c7addec2fe911 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?D=C3=A1niel=20Buga?= Date: Thu, 19 Dec 2024 12:31:54 +0100 Subject: [PATCH 1/3] Implement missing traits on GPIO types (#2842) --- esp-hal/CHANGELOG.md | 3 +++ esp-hal/src/gpio/mod.rs | 33 +++++++++++++++++++++++++-------- 2 files changed, 28 insertions(+), 8 deletions(-) diff --git a/esp-hal/CHANGELOG.md b/esp-hal/CHANGELOG.md index 14d1379d712..02cdd72f7be 100644 --- a/esp-hal/CHANGELOG.md +++ b/esp-hal/CHANGELOG.md @@ -45,6 +45,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - `DmaRxBuf`, `DmaTxBuf` and `DmaRxTxBuf` now implement `Debug` and `defmt::Format` (#2823) - DMA channels (`AnyGdmaChannel`, `SpiDmaChannel`, `I2sDmaChannel`, `CryptoDmaChannel`) and their RX/TX halves now implement `Debug` and `defmt::Format` (#2823) - `DmaDescriptor` and `DmaDescriptorFlags` now implement `PartialEq` and `Eq` (#2823) +- `gpio::{Event, WakeEvent, GpioRegisterAccess}` now implement `Debug`, `Eq`, `PartialEq` and `Hash` (#2842) +- `gpio::{Level, Pull, AlternateFunction, RtcFunction}` now implement `Hash` (#2842) +- `gpio::{GpioPin, AnyPin, Io, Output, OutputOpenDrain, Input, Flex}` now implement `Debug`, `defmt::Format` (#2842) ### Changed diff --git a/esp-hal/src/gpio/mod.rs b/esp-hal/src/gpio/mod.rs index 9f23c91088e..9e0de9e3b11 100644 --- a/esp-hal/src/gpio/mod.rs +++ b/esp-hal/src/gpio/mod.rs @@ -114,7 +114,7 @@ impl CFnPtr { } /// Event used to trigger interrupts. -#[derive(Copy, Clone)] +#[derive(Debug, Eq, PartialEq, Copy, Clone, Hash)] #[cfg_attr(feature = "defmt", derive(defmt::Format))] pub enum Event { /// Interrupts trigger on rising pin edge. @@ -139,7 +139,7 @@ impl From for Event { } /// Event used to wake up from light sleep. -#[derive(Copy, Clone)] +#[derive(Debug, Eq, PartialEq, Copy, Clone, Hash)] #[cfg_attr(feature = "defmt", derive(defmt::Format))] pub enum WakeEvent { /// Wake on low level @@ -157,7 +157,7 @@ pub enum WakeEvent { /// input, the peripheral will read the corresponding level from that signal. /// /// When connected to a peripheral output, the level will be ignored. -#[derive(Debug, Eq, PartialEq, Copy, Clone)] +#[derive(Debug, Eq, PartialEq, Copy, Clone, Hash)] #[cfg_attr(feature = "defmt", derive(defmt::Format))] pub enum Level { /// Low @@ -198,7 +198,7 @@ impl From for bool { } /// Pull setting for an input. -#[derive(Debug, Eq, PartialEq, Copy, Clone)] +#[derive(Debug, Eq, PartialEq, Copy, Clone, Hash)] #[cfg_attr(feature = "defmt", derive(defmt::Format))] pub enum Pull { /// No pull @@ -210,7 +210,7 @@ pub enum Pull { } /// Drive strength (values are approximates) -#[derive(Debug, Eq, PartialEq, Copy, Clone)] +#[derive(Debug, Eq, PartialEq, Copy, Clone, Hash, PartialOrd, Ord)] #[cfg_attr(feature = "defmt", derive(defmt::Format))] pub enum DriveStrength { /// Drive strength of approximately 5mA. @@ -234,7 +234,7 @@ pub enum DriveStrength { /// The different variants correspond to different functionality depending on /// the chip and the specific pin. For more information, refer to your chip's #[doc = crate::trm_markdown_link!("iomuxgpio")] -#[derive(Debug, Eq, PartialEq, Copy, Clone)] +#[derive(Debug, Eq, PartialEq, Copy, Clone, Hash)] #[cfg_attr(feature = "defmt", derive(defmt::Format))] pub enum AlternateFunction { /// Alternate function 0. @@ -268,7 +268,7 @@ impl TryFrom for AlternateFunction { } /// RTC function -#[derive(Debug, Eq, PartialEq, Copy, Clone)] +#[derive(Debug, Eq, PartialEq, Copy, Clone, Hash)] #[cfg_attr(feature = "defmt", derive(defmt::Format))] pub enum RtcFunction { /// RTC mode. @@ -547,7 +547,8 @@ pub trait TouchPin: Pin { } #[doc(hidden)] -#[derive(Clone, Copy, EnumCount)] +#[derive(Debug, Eq, PartialEq, Copy, Clone, Hash, EnumCount)] +#[cfg_attr(feature = "defmt", derive(defmt::Format))] pub enum GpioRegisterAccess { Bank0, #[cfg(gpio_bank_1)] @@ -758,9 +759,13 @@ impl Bank1GpioRegisterAccess { /// GPIO pin #[non_exhaustive] +#[derive(Debug)] +#[cfg_attr(feature = "defmt", derive(defmt::Format))] pub struct GpioPin; /// Type-erased GPIO pin +#[derive(Debug)] +#[cfg_attr(feature = "defmt", derive(defmt::Format))] pub struct AnyPin(pub(crate) AnyPinInner); impl GpioPin @@ -858,6 +863,8 @@ pub(crate) fn bind_default_interrupt_handler() { } /// General Purpose Input/Output driver +#[derive(Debug)] +#[cfg_attr(feature = "defmt", derive(defmt::Format))] pub struct Io { _io_mux: IO_MUX, } @@ -1073,6 +1080,8 @@ macro_rules! gpio { } )+ + #[derive(Debug)] + #[cfg_attr(feature = "defmt", derive(defmt::Format))] pub(crate) enum AnyPinInner { $( []($crate::gpio::GpioPin<$gpionum>), @@ -1174,6 +1183,8 @@ macro_rules! gpio { /// This driver configures the GPIO pin to be a push-pull output driver. /// Push-pull means that the driver actively sets the output voltage level /// for both high and low logical [`Level`]s. +#[derive(Debug)] +#[cfg_attr(feature = "defmt", derive(defmt::Format))] pub struct Output<'d, P = AnyPin> { pin: Flex<'d, P>, } @@ -1345,6 +1356,8 @@ where /// /// This driver configures the GPIO pin to be an input. Input drivers read the /// voltage of their pins and convert it to a logical [`Level`]. +#[derive(Debug)] +#[cfg_attr(feature = "defmt", derive(defmt::Format))] pub struct Input<'d, P = AnyPin> { pin: Flex<'d, P>, } @@ -1615,6 +1628,8 @@ where /// for the low logical [`Level`], but leaves the high level floating, which is /// then determined by external hardware, or internal pull-up/pull-down /// resistors. +#[derive(Debug)] +#[cfg_attr(feature = "defmt", derive(defmt::Format))] pub struct OutputOpenDrain<'d, P = AnyPin> { pin: Flex<'d, P>, } @@ -1838,6 +1853,8 @@ where /// Flexible pin driver. /// /// This driver allows changing the pin mode between input and output. +#[derive(Debug)] +#[cfg_attr(feature = "defmt", derive(defmt::Format))] pub struct Flex<'d, P = AnyPin> { pin: PeripheralRef<'d, P>, } From f1c372f2503aa644e09df414e66b168f38f89809 Mon Sep 17 00:00:00 2001 From: C2D <50617709+i404788@users.noreply.github.com> Date: Thu, 19 Dec 2024 14:57:21 +0100 Subject: [PATCH 2/3] Fix SpiDmaBus write impl (#2843) * Fix SpiDmaBus write impl * Add hil test for SpiDmaBus::{read,write} --------- Co-authored-by: ferris --- esp-hal/src/spi/master.rs | 2 +- hil-test/tests/spi_full_duplex.rs | 33 +++++++++++++++++++++++++++++++ 2 files changed, 34 insertions(+), 1 deletion(-) diff --git a/esp-hal/src/spi/master.rs b/esp-hal/src/spi/master.rs index b49cb2e2450..a3de6b79288 100644 --- a/esp-hal/src/spi/master.rs +++ b/esp-hal/src/spi/master.rs @@ -1732,8 +1732,8 @@ mod dma { unsafe { self.spi_dma.start_dma_transfer( - chunk.len(), 0, + chunk.len(), &mut EmptyBuf, &mut self.tx_buf, )?; diff --git a/hil-test/tests/spi_full_duplex.rs b/hil-test/tests/spi_full_duplex.rs index cc8a4b5ffa4..7938ba4dd3a 100644 --- a/hil-test/tests/spi_full_duplex.rs +++ b/hil-test/tests/spi_full_duplex.rs @@ -447,6 +447,39 @@ mod tests { ); } + #[test] + #[cfg(pcnt)] + fn test_dma_bus_read_write_pcnt(ctx: Context) { + const TRANSFER_SIZE: usize = 4; + let (rx_buffer, rx_descriptors, tx_buffer, tx_descriptors) = dma_buffers!(4); + let dma_rx_buf = DmaRxBuf::new(rx_descriptors, rx_buffer).unwrap(); + let dma_tx_buf = DmaTxBuf::new(tx_descriptors, tx_buffer).unwrap(); + + ctx.pcnt_unit.channel0.set_edge_signal(ctx.pcnt_source); + ctx.pcnt_unit + .channel0 + .set_input_mode(EdgeMode::Hold, EdgeMode::Increment); + + let mut spi = ctx + .spi + .with_dma(ctx.dma_channel) + .with_buffers(dma_rx_buf, dma_tx_buf); + + // Fill the buffer where each byte has 3 pos edges. + let tx_buf = [0b0110_1010; TRANSFER_SIZE]; + let mut rx_buf = [0; TRANSFER_SIZE]; + + for i in 1..4 { + // Preset as 5, expect 0 repeated receive + rx_buf.copy_from_slice(&[5; TRANSFER_SIZE]); + spi.read(&mut rx_buf).unwrap(); + assert_eq!(rx_buf, [0; TRANSFER_SIZE]); + + spi.write(&tx_buf).unwrap(); + assert_eq!(ctx.pcnt_unit.value(), (i * 3 * TRANSFER_SIZE) as _); + } + } + #[test] fn test_dma_bus_symmetric_transfer(ctx: Context) { let (rx_buffer, rx_descriptors, tx_buffer, tx_descriptors) = dma_buffers!(4); From f83ab23f0452dd8ecee189d24d50e5698a73146c Mon Sep 17 00:00:00 2001 From: Scott Mabin Date: Thu, 19 Dec 2024 14:39:40 +0000 Subject: [PATCH 3/3] Avoid abbreviations in API (#2844) * Avoid abbreviations in API * changelog/migration --- documentation/API-GUIDELINES.md | 3 +++ esp-hal/CHANGELOG.md | 2 ++ esp-hal/MIGRATING-0.22.md | 17 +++++++++++++ esp-hal/src/i2c/master/mod.rs | 44 +++++++++++++++++---------------- 4 files changed, 45 insertions(+), 21 deletions(-) diff --git a/documentation/API-GUIDELINES.md b/documentation/API-GUIDELINES.md index 5f5f21c39a5..28c41026d8c 100644 --- a/documentation/API-GUIDELINES.md +++ b/documentation/API-GUIDELINES.md @@ -80,6 +80,9 @@ In general, the [Rust API Guidelines](https://rust-lang.github.io/api-guidelines - If a driver only implements a subset of a peripheral's capabilities, it should be placed in the `peripheral::subcategory` module. - For example, if a driver implements the slave-mode I2C driver, it should be placed into `i2c::slave`. - This helps us reducing the need of introducing breaking changes if we implement additional functionalities. +- Avoid abbreviations and contractions in the API, where possible. + - Saving a few characters may introduce ambiguity, e.g `SpiTransDone`, is it `Transmit` or `Transfer`? + - Common abbreviations, that are well understood such as `Dma` are perfectly fine. ## Maintainability diff --git a/esp-hal/CHANGELOG.md b/esp-hal/CHANGELOG.md index 02cdd72f7be..2441a450948 100644 --- a/esp-hal/CHANGELOG.md +++ b/esp-hal/CHANGELOG.md @@ -81,6 +81,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Updated `esp-pacs` with support for Wi-Fi on the ESP32 and made the peripheral non virtual - `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) + ### Fixed - Xtensa devices now correctly enable the `esp-hal-procmacros/rtc-slow` feature (#2594) diff --git a/esp-hal/MIGRATING-0.22.md b/esp-hal/MIGRATING-0.22.md index b129771528c..159475131f0 100644 --- a/esp-hal/MIGRATING-0.22.md +++ b/esp-hal/MIGRATING-0.22.md @@ -300,3 +300,20 @@ is not compatible with the hardware. - spi_dma.transfer(dma_rx_buf, dma_tx_buf); + spi_dma.transfer(5, dma_rx_buf, 5, dma_tx_buf); ``` + +## I2C Error changes + +To avoid abbreviations and contractions (as per the esp-hal guidelines), some error variants have changed + +```diff +- Error::ExecIncomplete ++ Error::ExecutionIncomplete +- Error::CommandNrExceeded ++ Error::CommandNumberExceeded +- Error::ExceedingFifo ++ Error::FifoExceeded +- Error::TimeOut ++ Error::Timeout +- Error::InvalidZeroLength ++ Error::ZeroLengthInvalid +``` diff --git a/esp-hal/src/i2c/master/mod.rs b/esp-hal/src/i2c/master/mod.rs index 8fa40dca198..6e3ee3ad93f 100644 --- a/esp-hal/src/i2c/master/mod.rs +++ b/esp-hal/src/i2c/master/mod.rs @@ -91,19 +91,19 @@ const MAX_ITERATIONS: u32 = 1_000_000; #[non_exhaustive] pub enum Error { /// The transmission exceeded the FIFO size. - ExceedingFifo, + FifoExceeded, /// The acknowledgment check failed. AckCheckFailed, /// A timeout occurred during transmission. - TimeOut, + Timeout, /// The arbitration for the bus was lost. ArbitrationLost, /// The execution of the I2C command was incomplete. - ExecIncomplete, + ExecutionIncomplete, /// The number of commands issued exceeded the limit. - CommandNrExceeded, + CommandNumberExceeded, /// Zero length read or write operation. - InvalidZeroLength, + ZeroLengthInvalid, } impl core::error::Error for Error {} @@ -111,15 +111,17 @@ impl core::error::Error for Error {} impl core::fmt::Display for Error { fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result { match self { - Error::ExceedingFifo => write!(f, "The transmission exceeded the FIFO size"), + Error::FifoExceeded => write!(f, "The transmission exceeded the FIFO size"), Error::AckCheckFailed => write!(f, "The acknowledgment check failed"), - Error::TimeOut => write!(f, "A timeout occurred during transmission"), + Error::Timeout => write!(f, "A timeout occurred during transmission"), Error::ArbitrationLost => write!(f, "The arbitration for the bus was lost"), - Error::ExecIncomplete => write!(f, "The execution of the I2C command was incomplete"), - Error::CommandNrExceeded => { + Error::ExecutionIncomplete => { + write!(f, "The execution of the I2C command was incomplete") + } + Error::CommandNumberExceeded => { write!(f, "The number of commands issued exceeded the limit") } - Error::InvalidZeroLength => write!(f, "Zero length read or write operation"), + Error::ZeroLengthInvalid => write!(f, "Zero length read or write operation"), } } } @@ -204,7 +206,7 @@ impl embedded_hal::i2c::Error for Error { use embedded_hal::i2c::{ErrorKind, NoAcknowledgeSource}; match self { - Self::ExceedingFifo => ErrorKind::Overrun, + Self::FifoExceeded => ErrorKind::Overrun, Self::ArbitrationLost => ErrorKind::ArbitrationLoss, Self::AckCheckFailed => ErrorKind::NoAcknowledge(NoAcknowledgeSource::Unknown), _ => ErrorKind::Other, @@ -643,7 +645,7 @@ impl<'a> I2cFuture<'a> { } if r.time_out().bit_is_set() { - return Err(Error::TimeOut); + return Err(Error::Timeout); } if r.nack().bit_is_set() { @@ -1337,7 +1339,7 @@ impl Driver<'_> { let max_len = if start { 254usize } else { 255usize }; if bytes.len() > max_len { // we could support more by adding multiple write operations - return Err(Error::ExceedingFifo); + return Err(Error::FifoExceeded); } let write_len = if start { bytes.len() + 1 } else { bytes.len() }; @@ -1386,7 +1388,7 @@ impl Driver<'_> { I: Iterator, { if buffer.is_empty() { - return Err(Error::InvalidZeroLength); + return Err(Error::ZeroLengthInvalid); } let (max_len, initial_len) = if will_continue { (255usize, buffer.len()) @@ -1395,7 +1397,7 @@ impl Driver<'_> { }; if buffer.len() > max_len { // we could support more by adding multiple read operations - return Err(Error::ExceedingFifo); + return Err(Error::FifoExceeded); } if start { @@ -1579,7 +1581,7 @@ impl Driver<'_> { tout -= 1; if tout == 0 { - return Err(Error::TimeOut); + return Err(Error::Timeout); } embassy_futures::yield_now().await; @@ -1607,7 +1609,7 @@ impl Driver<'_> { tout -= 1; if tout == 0 { - return Err(Error::TimeOut); + return Err(Error::Timeout); } } self.check_all_commands_done()?; @@ -1623,7 +1625,7 @@ impl Driver<'_> { let cmd = cmd_reg.read(); if cmd.bits() != 0x0 && !cmd.opcode().is_end() && !cmd.command_done().bit_is_set() { - return Err(Error::ExecIncomplete); + return Err(Error::ExecutionIncomplete); } } @@ -1646,7 +1648,7 @@ impl Driver<'_> { if #[cfg(esp32)] { // Handle error cases let retval = if interrupts.time_out().bit_is_set() { - Err(Error::TimeOut) + Err(Error::Timeout) } else if interrupts.nack().bit_is_set() { Err(Error::AckCheckFailed) } else if interrupts.arbitration_lost().bit_is_set() { @@ -1657,7 +1659,7 @@ impl Driver<'_> { } else { // Handle error cases let retval = if interrupts.time_out().bit_is_set() { - Err(Error::TimeOut) + Err(Error::Timeout) } else if interrupts.nack().bit_is_set() { Err(Error::AckCheckFailed) } else if interrupts.arbitration_lost().bit_is_set() { @@ -2181,7 +2183,7 @@ fn add_cmd<'a, I>(cmd_iterator: &mut I, command: Command) -> Result<(), Error> where I: Iterator, { - let cmd = cmd_iterator.next().ok_or(Error::CommandNrExceeded)?; + let cmd = cmd_iterator.next().ok_or(Error::CommandNumberExceeded)?; cmd.write(|w| match command { Command::Start => w.opcode().rstart(),