From 62c72947ddadeb6128f54f6649365434d8b73ad7 Mon Sep 17 00:00:00 2001 From: Kirill Mikhailov <62840029+playfulFence@users.noreply.github.com> Date: Fri, 20 Dec 2024 15:01:11 +0100 Subject: [PATCH] Multiple API fixes in UART driver (#2851) * Close a number of issues doctest update fmt * changelog entry * dumb * addressing reviews * Add an entry to migration guide --- esp-hal/CHANGELOG.md | 3 ++ esp-hal/MIGRATING-0.22.md | 7 +++++ esp-hal/src/uart.rs | 50 +++++++++++++----------------- examples/src/bin/embassy_serial.rs | 2 +- hil-test/tests/uart_regression.rs | 2 +- hil-test/tests/uart_tx_rx.rs | 4 +-- 6 files changed, 36 insertions(+), 32 deletions(-) diff --git a/esp-hal/CHANGELOG.md b/esp-hal/CHANGELOG.md index be5c0d5048c..145bb8788d4 100644 --- a/esp-hal/CHANGELOG.md +++ b/esp-hal/CHANGELOG.md @@ -87,6 +87,8 @@ 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: Replaced potential panics with errors. (#2831) +- UART: Make `AtCmdConfig` and `ConfigError` non-exhaustive (#2851) +- UART: Make `AtCmdConfig` use builder-lite pattern (#2851) ### Fixed @@ -94,6 +96,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - User-bound GPIO interrupt handlers should no longer interfere with async pins. (#2625) - `spi::master::Spi::{into_async, into_blocking}` are now correctly available on the typed driver, to. (#2674) - It is no longer possible to safely conjure `GpioPin` instances (#2688) +- UART: Public API follows `C-WORD_ORDER` Rust API standard (`VerbObject` order) (#2851) ### Removed diff --git a/esp-hal/MIGRATING-0.22.md b/esp-hal/MIGRATING-0.22.md index 4a312f08c16..59f6a2d5c38 100644 --- a/esp-hal/MIGRATING-0.22.md +++ b/esp-hal/MIGRATING-0.22.md @@ -336,3 +336,10 @@ The reexports that were previously part of the prelude are available through oth - `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 + +```diff +- uart0.set_at_cmd(AtCmdConfig::new(None, None, None, b'#', None)); ++ uart0.set_at_cmd(AtCmdConfig::default().with_cmd_char(b'#')); +``` diff --git a/esp-hal/src/uart.rs b/esp-hal/src/uart.rs index 2b69e3cee84..0091cdf22b5 100644 --- a/esp-hal/src/uart.rs +++ b/esp-hal/src/uart.rs @@ -164,7 +164,7 @@ //! uart0.set_interrupt_handler(interrupt_handler); //! //! critical_section::with(|cs| { -//! uart0.set_at_cmd(AtCmdConfig::new(None, None, None, b'#', None)); +//! uart0.set_at_cmd(AtCmdConfig::default().with_cmd_char(b'#')); //! uart0.listen(UartInterrupt::AtCmd | UartInterrupt::RxFifoFull); //! //! SERIAL.borrow_ref_mut(cs).replace(uart0); @@ -250,6 +250,7 @@ use crate::{ }; const UART_FIFO_SIZE: u16 = 128; +const CMD_CHAR_DEFAULT: u8 = 0x2b; /// UART Error #[derive(Debug, Clone, Copy, PartialEq)] @@ -510,6 +511,9 @@ impl Default for Config { } } +#[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 @@ -524,28 +528,17 @@ pub struct AtCmdConfig { /// The character that triggers the AT command detection. pub cmd_char: u8, /// Optional number of characters to detect as part of the AT command. - pub char_num: Option, + pub char_num: u8, } -impl AtCmdConfig { - /// Creates a new `AtCmdConfig` with the specified configuration. - /// - /// This function sets up the AT command detection parameters, including - /// pre- and post-idle times, a gap timeout, the triggering command - /// character, and the number of characters to detect. - pub fn new( - pre_idle_count: Option, - post_idle_count: Option, - gap_timeout: Option, - cmd_char: u8, - char_num: Option, - ) -> AtCmdConfig { +impl Default for AtCmdConfig { + fn default() -> Self { Self { - pre_idle_count, - post_idle_count, - gap_timeout, - cmd_char, - char_num, + pre_idle_count: None, + post_idle_count: None, + gap_timeout: None, + cmd_char: CMD_CHAR_DEFAULT, + char_num: 1, } } } @@ -631,6 +624,7 @@ pub struct UartRx<'d, Dm, T = AnyUart> { /// A configuration error. #[derive(Debug, Clone, Copy, PartialEq, Eq)] #[cfg_attr(feature = "defmt", derive(defmt::Format))] +#[non_exhaustive] pub enum ConfigError { /// The requested timeout is not supported. UnsupportedTimeout, @@ -752,7 +746,7 @@ where } /// Flush the transmit buffer of the UART - pub fn flush_tx(&mut self) -> nb::Result<(), Error> { + pub fn flush(&mut self) -> nb::Result<(), Error> { if self.is_tx_idle() { Ok(()) } else { @@ -1219,7 +1213,7 @@ where register_block.at_cmd_char().write(|w| unsafe { w.at_cmd_char().bits(config.cmd_char); - w.char_num().bits(config.char_num.unwrap_or(1)) + w.char_num().bits(config.char_num) }); if let Some(pre_idle_count) = config.pre_idle_count { @@ -1254,8 +1248,8 @@ where } /// Flush the transmit buffer of the UART - pub fn flush_tx(&mut self) -> nb::Result<(), Error> { - self.tx.flush_tx() + pub fn flush(&mut self) -> nb::Result<(), Error> { + self.tx.flush() } /// Read a byte from the UART @@ -1477,7 +1471,7 @@ where } fn flush(&mut self) -> nb::Result<(), Self::Error> { - self.flush_tx() + self.flush() } } @@ -1491,7 +1485,7 @@ where } fn flush(&mut self) -> nb::Result<(), Self::Error> { - self.flush_tx() + self.flush() } } @@ -1581,7 +1575,7 @@ where } fn flush(&mut self) -> Result<(), Self::Error> { - self.tx.flush() + embedded_io::Write::flush(&mut self.tx) } } @@ -1598,7 +1592,7 @@ where fn flush(&mut self) -> Result<(), Self::Error> { loop { - match self.flush_tx() { + match self.flush() { Ok(_) => break, Err(nb::Error::WouldBlock) => { /* Wait */ } Err(nb::Error::Other(e)) => return Err(e), diff --git a/examples/src/bin/embassy_serial.rs b/examples/src/bin/embassy_serial.rs index 2df7dcc9968..e32fb50f2ad 100644 --- a/examples/src/bin/embassy_serial.rs +++ b/examples/src/bin/embassy_serial.rs @@ -92,7 +92,7 @@ async fn main(spawner: Spawner) { let mut uart0 = Uart::new(peripherals.UART0, config, rx_pin, tx_pin) .unwrap() .into_async(); - uart0.set_at_cmd(AtCmdConfig::new(None, None, None, AT_CMD, None)); + uart0.set_at_cmd(AtCmdConfig::default().with_cmd_char(AT_CMD)); let (rx, tx) = uart0.split(); diff --git a/hil-test/tests/uart_regression.rs b/hil-test/tests/uart_regression.rs index e0a4775f9a1..de1d9ad0275 100644 --- a/hil-test/tests/uart_regression.rs +++ b/hil-test/tests/uart_regression.rs @@ -31,7 +31,7 @@ mod tests { // set up TX and send a byte let mut tx = UartTx::new(peripherals.UART0, uart::Config::default(), tx).unwrap(); - tx.flush_tx().unwrap(); + tx.flush().unwrap(); tx.write_bytes(&[0x42]).unwrap(); let read = block!(rx.read_byte()); diff --git a/hil-test/tests/uart_tx_rx.rs b/hil-test/tests/uart_tx_rx.rs index 5dd1de26c3e..10840df1c3d 100644 --- a/hil-test/tests/uart_tx_rx.rs +++ b/hil-test/tests/uart_tx_rx.rs @@ -38,7 +38,7 @@ mod tests { fn test_send_receive(mut ctx: Context) { let byte = [0x42]; - ctx.tx.flush_tx().unwrap(); + ctx.tx.flush().unwrap(); ctx.tx.write_bytes(&byte).unwrap(); let read = block!(ctx.rx.read_byte()); @@ -50,7 +50,7 @@ mod tests { let bytes = [0x42, 0x43, 0x44]; let mut buf = [0u8; 3]; - ctx.tx.flush_tx().unwrap(); + ctx.tx.flush().unwrap(); ctx.tx.write_bytes(&bytes).unwrap(); ctx.rx.read_bytes(&mut buf).unwrap();