From 39da5337acf03ce18b7ba7d19236b66bd3f9cf02 Mon Sep 17 00:00:00 2001 From: Jesse Braham Date: Mon, 6 Jan 2025 04:11:06 -0800 Subject: [PATCH] Fix naming violations and clean up some doc comments in UART driver (#2893) * Resolve naming violation for `Parity` enum * Remove unnecessary/redundant information from doc comments * Resolve naming violations for `DataBits` and `StopBits` enums * Update migration guide * Update `CHANGELOG.md` --- esp-hal/CHANGELOG.md | 1 + esp-hal/MIGRATING-0.22.md | 36 ++++++++++++------- esp-hal/src/uart.rs | 76 ++++++++++++++++++--------------------- 3 files changed, 59 insertions(+), 54 deletions(-) diff --git a/esp-hal/CHANGELOG.md b/esp-hal/CHANGELOG.md index 22abde76b74..36a6e86aed5 100644 --- a/esp-hal/CHANGELOG.md +++ b/esp-hal/CHANGELOG.md @@ -91,6 +91,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - I2C: Replaced potential panics with errors. (#2831) - UART: Make `AtCmdConfig` and `ConfigError` non-exhaustive (#2851) - UART: Make `AtCmdConfig` use builder-lite pattern (#2851) +- UART: Fix naming violations for `DataBits`, `Parity`, and `StopBits` enum variants (#2893) ### Fixed diff --git a/esp-hal/MIGRATING-0.22.md b/esp-hal/MIGRATING-0.22.md index b88c6e30277..6e2d8a256ff 100644 --- a/esp-hal/MIGRATING-0.22.md +++ b/esp-hal/MIGRATING-0.22.md @@ -199,7 +199,7 @@ is enabled. To retrieve the address and size of the initialized external memory, The usage of `esp_alloc::psram_allocator!` remains unchanged. -## embedded-hal 0.2.* is not supported anymore. +## embedded-hal 0.2.\* is not supported anymore. As per https://github.com/rust-embedded/embedded-hal/pull/640, our driver no longer implements traits from `embedded-hal 0.2.x`. Analogs of all traits from the above mentioned version are available in `embedded-hal 1.x.x` @@ -325,15 +325,15 @@ The reexports that were previously part of the prelude are available through oth - `ExtU64` and `RateExtU32` have been moved to `esp_hal::time` - `Clock` and `CpuClock`: `esp_hal::clock::{Clock, CpuClock}` - The following traits need to be individually imported when needed: - - `esp_hal::analog::dac::Instance` - - `esp_hal::gpio::Pin` - - `esp_hal::ledc::channel::ChannelHW` - - `esp_hal::ledc::channel::ChannelIFace` - - `esp_hal::ledc::timer::TimerHW` - - `esp_hal::ledc::timer::TimerIFace` - - `esp_hal::timer::timg::TimerGroupInstance` - - `esp_hal::timer::Timer` - - `esp_hal::interrupt::InterruptConfigurable` + - `esp_hal::analog::dac::Instance` + - `esp_hal::gpio::Pin` + - `esp_hal::ledc::channel::ChannelHW` + - `esp_hal::ledc::channel::ChannelIFace` + - `esp_hal::ledc::timer::TimerHW` + - `esp_hal::ledc::timer::TimerIFace` + - `esp_hal::timer::timg::TimerGroupInstance` + - `esp_hal::timer::Timer` + - `esp_hal::interrupt::InterruptConfigurable` - The `entry` macro can be imported as `esp_hal::entry`, while other macros are found under `esp_hal::macros` ## `AtCmdConfig` now uses builder-lite pattern @@ -343,11 +343,10 @@ The reexports that were previously part of the prelude are available through oth + uart0.set_at_cmd(AtCmdConfig::default().with_cmd_char(b'#')); ``` - ## Crate configuration changes To prevent ambiguity between configurations, we had to change the naming format of configuration -keys. Before, we used `{prefix}_{key}`, which meant that esp-hal and esp-hal-* configuration keys +keys. Before, we used `{prefix}_{key}`, which meant that esp-hal and esp-hal-\* configuration keys were impossible to tell apart. To fix this issue, we are changing the separator from one underscore character to `_CONFIG_`. This also means that users will have to change their `config.toml` configurations to match the new format. @@ -370,3 +369,16 @@ The `Config` struct's setters are now prefixed with `with_`. `parity_none`, `par - .parity_even(); + .with_parity(Parity::Even); ``` + +The `DataBits`, `Parity`, and `StopBits` enum variants are no longer prefixed with the name of the enum. + +e.g.) + +```diff +- DataBits::DataBits8 ++ DataBits::_8 +- Parity::ParityNone ++ Parity::None +- StopBits::Stop1 ++ StopBits::_1 +``` diff --git a/esp-hal/src/uart.rs b/esp-hal/src/uart.rs index 36808e6680e..108fbc60815 100644 --- a/esp-hal/src/uart.rs +++ b/esp-hal/src/uart.rs @@ -322,15 +322,13 @@ impl embedded_io::Error for Error { #[derive(Debug, Default, Clone, Copy, PartialEq, Eq)] #[cfg_attr(feature = "defmt", derive(defmt::Format))] pub enum ClockSource { - /// APB_CLK clock source (default for UART on all the chips except of - /// esp32c6 and esp32h2) + /// APB_CLK clock source #[cfg_attr(not(any(esp32c6, esp32h2, lp_uart)), default)] Apb, /// RC_FAST_CLK clock source (17.5 MHz) #[cfg(not(any(esp32, esp32s2)))] RcFast, - /// XTAL_CLK clock source (default for UART on esp32c6 and esp32h2 and - /// LP_UART) + /// XTAL_CLK clock source #[cfg(not(any(esp32, esp32s2)))] #[cfg_attr(any(esp32c6, esp32h2, lp_uart), default)] Xtal, @@ -353,14 +351,14 @@ const UART_TOUT_THRESH_DEFAULT: u8 = 10; #[cfg_attr(feature = "defmt", derive(defmt::Format))] pub enum DataBits { /// 5 data bits per frame. - DataBits5 = 0, + _5 = 0, /// 6 data bits per frame. - DataBits6 = 1, + _6 = 1, /// 7 data bits per frame. - DataBits7 = 2, - /// 8 data bits per frame (most common). + _7 = 2, + /// 8 data bits per frame. #[default] - DataBits8 = 3, + _8 = 3, } /// Parity check @@ -371,17 +369,16 @@ pub enum DataBits { /// either even or odd. #[derive(Debug, Default, Clone, Copy, PartialEq, Eq)] #[cfg_attr(feature = "defmt", derive(defmt::Format))] -#[allow(clippy::enum_variant_names)] // FIXME: resolve this pub enum Parity { - /// No parity bit is used (most common). + /// No parity bit is used. #[default] - ParityNone, + None, /// Even parity: the parity bit is set to make the total number of /// 1-bits even. - ParityEven, + Even, /// Odd parity: the parity bit is set to make the total number of 1-bits /// odd. - ParityOdd, + Odd, } /// Number of stop bits @@ -391,15 +388,14 @@ pub enum Parity { /// bits. #[derive(Debug, Default, Clone, Copy, PartialEq, Eq)] #[cfg_attr(feature = "defmt", derive(defmt::Format))] -#[allow(clippy::enum_variant_names)] // FIXME: resolve this pub enum StopBits { /// 1 stop bit. #[default] - Stop1 = 1, + _1 = 1, /// 1.5 stop bits. - Stop1P5 = 2, + _1p5 = 2, /// 2 stop bits. - Stop2 = 3, + _2 = 3, } /// UART Configuration @@ -430,17 +426,17 @@ impl Config { fn symbol_length(&self) -> u8 { let mut length: u8 = 1; // start bit length += match self.data_bits { - DataBits::DataBits5 => 5, - DataBits::DataBits6 => 6, - DataBits::DataBits7 => 7, - DataBits::DataBits8 => 8, + DataBits::_5 => 5, + DataBits::_6 => 6, + DataBits::_7 => 7, + DataBits::_8 => 8, }; length += match self.parity { - Parity::ParityNone => 0, + Parity::None => 0, _ => 1, }; length += match self.stop_bits { - StopBits::Stop1 => 1, + StopBits::_1 => 1, _ => 2, // esp-idf also counts 2 bits for settings 1.5 and 2 stop bits }; length @@ -461,10 +457,10 @@ impl Default for Config { } } +/// Configuration for the AT-CMD detection functionality #[derive(Clone, Copy, PartialEq, Eq, Hash, Debug, procmacros::BuilderLite)] #[cfg_attr(feature = "defmt", derive(defmt::Format))] #[non_exhaustive] -/// Configuration for the AT-CMD detection functionality pub struct AtCmdConfig { /// Optional idle time before the AT command detection begins, in clock /// cycles. @@ -2125,16 +2121,16 @@ pub mod lp_uart { } fn change_parity(&mut self, parity: Parity) -> &mut Self { - if parity != Parity::ParityNone { + if parity != Parity::None { self.uart .conf0() .modify(|_, w| w.parity().bit((parity as u8 & 0x1) != 0)); } self.uart.conf0().modify(|_, w| match parity { - Parity::ParityNone => w.parity_en().clear_bit(), - Parity::ParityEven => w.parity_en().set_bit().parity().clear_bit(), - Parity::ParityOdd => w.parity_en().set_bit().parity().set_bit(), + Parity::None => w.parity_en().clear_bit(), + Parity::Even => w.parity_en().set_bit().parity().clear_bit(), + Parity::Odd => w.parity_en().set_bit().parity().set_bit(), }); self @@ -2550,9 +2546,9 @@ impl Info { fn change_parity(&self, parity: Parity) { self.register_block().conf0().modify(|_, w| match parity { - Parity::ParityNone => w.parity_en().clear_bit(), - Parity::ParityEven => w.parity_en().set_bit().parity().clear_bit(), - Parity::ParityOdd => w.parity_en().set_bit().parity().set_bit(), + Parity::None => w.parity_en().clear_bit(), + Parity::Even => w.parity_en().set_bit().parity().clear_bit(), + Parity::Odd => w.parity_en().set_bit().parity().set_bit(), }); } @@ -2560,18 +2556,14 @@ impl Info { #[cfg(esp32)] { // workaround for hardware issue, when UART stop bit set as 2-bit mode. - if stop_bits == StopBits::Stop2 { + if stop_bits == StopBits::_2 { self.register_block() .rs485_conf() - .modify(|_, w| w.dl1_en().bit(stop_bits == StopBits::Stop2)); - - self.register_block().conf0().modify(|_, w| { - if stop_bits == StopBits::Stop2 { - unsafe { w.stop_bit_num().bits(1) } - } else { - unsafe { w.stop_bit_num().bits(stop_bits as u8) } - } - }); + .modify(|_, w| w.dl1_en().bit(stop_bits == StopBits::_2)); + + self.register_block() + .conf0() + .modify(|_, w| unsafe { w.stop_bit_num().bits(1) }); } }