From 42e3cfde8ab56c964f1283f0ac3499862451908a Mon Sep 17 00:00:00 2001 From: Robin Mueller Date: Tue, 24 Sep 2024 11:57:35 +0200 Subject: [PATCH] improve UART impl --- .../embassy/src/bin/uart-echo-with-irq.rs | 17 +- flashloader/src/main.rs | 4 +- va416xx-hal/CHANGELOG.md | 2 + va416xx-hal/src/gpio/reg.rs | 22 +- va416xx-hal/src/uart.rs | 339 +++++++++++------- 5 files changed, 232 insertions(+), 152 deletions(-) diff --git a/examples/embassy/src/bin/uart-echo-with-irq.rs b/examples/embassy/src/bin/uart-echo-with-irq.rs index cc8a599..94ba310 100644 --- a/examples/embassy/src/bin/uart-echo-with-irq.rs +++ b/examples/embassy/src/bin/uart-echo-with-irq.rs @@ -128,17 +128,14 @@ async fn blinky(mut led: Pin) { fn UART0_RX() { let mut buf: [u8; 16] = [0; 16]; let mut read_len: usize = 0; - let mut irq_error = None; + let mut errors = None; RX.lock(|static_rx| { let mut rx_borrow = static_rx.borrow_mut(); let rx_mut_ref = rx_borrow.as_mut().unwrap(); - match rx_mut_ref.irq_handler(&mut buf) { - Ok(result) => { - read_len = result.bytes_read; - } - Err(e) => { - irq_error = Some(e); - } + let result = rx_mut_ref.irq_handler(&mut buf); + read_len = result.bytes_read; + if result.errors.is_some() { + errors = result.errors; } }); let mut ringbuf_full = false; @@ -155,8 +152,8 @@ fn UART0_RX() { }); } - if irq_error.is_some() { - rprintln!("error in IRQ handler: {:?}", irq_error); + if errors.is_some() { + rprintln!("UART error: {:?}", errors); } if ringbuf_full { rprintln!("ringbuffer is full, deleted oldest data"); diff --git a/flashloader/src/main.rs b/flashloader/src/main.rs index 96d546d..f9746f6 100644 --- a/flashloader/src/main.rs +++ b/flashloader/src/main.rs @@ -293,8 +293,8 @@ mod app { .read_fixed_len_or_timeout_based_using_irq(cx.local.rx_context) .expect("read operation failed"); } - if result.error() { - log::warn!("UART error: {:?}", result.error()); + if result.has_errors() { + log::warn!("UART error: {:?}", result.errors.unwrap()); } } Err(e) => { diff --git a/va416xx-hal/CHANGELOG.md b/va416xx-hal/CHANGELOG.md index 42dce3f..5b05547 100644 --- a/va416xx-hal/CHANGELOG.md +++ b/va416xx-hal/CHANGELOG.md @@ -16,6 +16,8 @@ and this project adheres to [Semantic Versioning](http://semver.org/). - Added an additional way to read the UART RX with IRQs. The module documentation provides more information. - Made the UART with IRQ API more flexible for future additions. +- Improved UART API result and error handling, added low level API to read from and write + to the FIFO directly ## Fixed diff --git a/va416xx-hal/src/gpio/reg.rs b/va416xx-hal/src/gpio/reg.rs index d8a9022..9fbf17a 100644 --- a/va416xx-hal/src/gpio/reg.rs +++ b/va416xx-hal/src/gpio/reg.rs @@ -113,14 +113,6 @@ pub(super) unsafe trait RegisterInterface { /// this type. fn id(&self) -> DynPinId; - const PORTA: *const PortRegisterBlock = Porta::ptr(); - const PORTB: *const PortRegisterBlock = Portb::ptr(); - const PORTC: *const PortRegisterBlock = Portc::ptr(); - const PORTD: *const PortRegisterBlock = Portd::ptr(); - const PORTE: *const PortRegisterBlock = Porte::ptr(); - const PORTF: *const PortRegisterBlock = Portf::ptr(); - const PORTG: *const PortRegisterBlock = Portg::ptr(); - /// Change the pin mode #[inline] fn change_mode(&mut self, mode: DynPinMode) { @@ -155,13 +147,13 @@ pub(super) unsafe trait RegisterInterface { #[inline] fn port_reg(&self) -> &PortRegisterBlock { match self.id().group { - DynGroup::A => unsafe { &(*Self::PORTA) }, - DynGroup::B => unsafe { &(*Self::PORTB) }, - DynGroup::C => unsafe { &(*Self::PORTC) }, - DynGroup::D => unsafe { &(*Self::PORTD) }, - DynGroup::E => unsafe { &(*Self::PORTE) }, - DynGroup::F => unsafe { &(*Self::PORTF) }, - DynGroup::G => unsafe { &(*Self::PORTG) }, + DynGroup::A => unsafe { &(*Porta::ptr()) }, + DynGroup::B => unsafe { &(*Portb::ptr()) }, + DynGroup::C => unsafe { &(*Portc::ptr()) }, + DynGroup::D => unsafe { &(*Portd::ptr()) }, + DynGroup::E => unsafe { &(*Porte::ptr()) }, + DynGroup::F => unsafe { &(*Portf::ptr()) }, + DynGroup::G => unsafe { &(*Portg::ptr()) }, } } diff --git a/va416xx-hal/src/uart.rs b/va416xx-hal/src/uart.rs index bd2f9c8..a08418c 100644 --- a/va416xx-hal/src/uart.rs +++ b/va416xx-hal/src/uart.rs @@ -9,6 +9,7 @@ //! - [UART simple example](https://egit.irs.uni-stuttgart.de/rust/va416xx-rs/src/branch/main/examples/simple/examples/uart.rs) //! - [UART echo with IRQ and Embassy](https://egit.irs.uni-stuttgart.de/rust/va416xx-rs/src/branch/main/examples/embassy/src/bin/uart-echo-with-irq.rs) //! - [Flashloader app using UART with IRQs](https://egit.irs.uni-stuttgart.de/rust/va416xx-rs/src/branch/main/flashloader) +use core::convert::Infallible; use core::ops::Deref; use embedded_hal_nb::serial::Read; @@ -69,15 +70,28 @@ impl RxPin for Pin {} // Regular Definitions //================================================================================================== -#[derive(Debug)] +#[derive(Debug, PartialEq, Eq)] +#[cfg_attr(feature = "defmt", derive(defmt::Format))] +pub struct TransferPendingError; + +#[derive(Debug, PartialEq, Eq)] +#[cfg_attr(feature = "defmt", derive(defmt::Format))] +pub enum RxError { + Overrun, + Framing, + Parity, +} +#[derive(Debug, PartialEq, Eq)] #[cfg_attr(feature = "defmt", derive(defmt::Format))] pub enum Error { - Overrun, - FramingError, - ParityError, + Rx(RxError), BreakCondition, - TransferPending, - BufferTooShort, +} + +impl From for Error { + fn from(value: RxError) -> Self { + Self::Rx(value) + } } #[derive(Debug, PartialEq, Eq, Copy, Clone)] @@ -231,50 +245,47 @@ impl IrqContextTimeoutOrMaxSize { #[derive(Debug, Default)] pub struct IrqResult { pub bytes_read: usize, - pub errors: IrqUartError, + pub errors: Option, } /// This struct is used to return the default IRQ handler result to the user #[derive(Debug, Default)] -pub struct IrqResultMaxSizeTimeout { +pub struct IrqResultMaxSizeOrTimeout { complete: bool, timeout: bool, - pub errors: IrqUartError, + pub errors: Option, pub bytes_read: usize, } -impl IrqResultMaxSizeTimeout { +impl IrqResultMaxSizeOrTimeout { pub fn new() -> Self { - IrqResultMaxSizeTimeout { + IrqResultMaxSizeOrTimeout { complete: false, timeout: false, - errors: IrqUartError::default(), + errors: None, bytes_read: 0, } } } -impl IrqResultMaxSizeTimeout { +impl IrqResultMaxSizeOrTimeout { #[inline] - pub fn error(&self) -> bool { - if self.errors.overflow || self.errors.parity || self.errors.framing { - return true; - } - false + pub fn has_errors(&self) -> bool { + self.errors.is_some() } #[inline] pub fn overflow_error(&self) -> bool { - self.errors.overflow + self.errors.map_or(false, |e| e.overflow) } #[inline] pub fn framing_error(&self) -> bool { - self.errors.framing + self.errors.map_or(false, |e| e.framing) } #[inline] pub fn parity_error(&self) -> bool { - self.errors.parity + self.errors.map_or(false, |e| e.parity) } #[inline] @@ -295,43 +306,9 @@ enum IrqReceptionMode { } //================================================================================================== -// UART implementation +// UART peripheral wrapper //================================================================================================== -/// Type erased variant of a UART. Can be created with the [Uart::downgrade] function. -pub struct UartBase { - uart: Uart, - tx: Tx, - rx: Rx, -} -/// Serial abstraction. Entry point to create a new UART -pub struct Uart { - inner: UartBase, - pins: Pins, -} - -/// Serial receiver. -/// -/// Can be created by using the [Uart::split] or [UartBase::split] API. -pub struct Rx(Uart); - -/// Serial transmitter -/// -/// Can be created by using the [Uart::split] or [UartBase::split] API. -pub struct Tx(Uart); - -impl Rx { - fn new(uart: Uart) -> Self { - Self(uart) - } -} - -impl Tx { - fn new(uart: Uart) -> Self { - Self(uart) - } -} - pub trait Instance: Deref { const IDX: u8; const PERIPH_SEL: PeripheralSelect; @@ -389,6 +366,17 @@ impl Instance for Uart2 { } } +//================================================================================================== +// UART implementation +//================================================================================================== + +/// Type erased variant of a UART. Can be created with the [Uart::downgrade] function. +pub struct UartBase { + uart: Uart, + tx: Tx, + rx: Rx, +} + impl UartBase { fn init(self, config: Config, clocks: &Clocks) -> Self { if Uart::IDX == 2 { @@ -522,6 +510,12 @@ impl UartBase { } } +/// Serial abstraction. Entry point to create a new UART +pub struct Uart { + inner: UartBase, + pins: Pins, +} + impl, RxPinInst: RxPin, UartInstance: Instance> Uart { @@ -617,6 +611,17 @@ impl, RxPinInst: RxPin, UartInstanc } } +/// Serial receiver. +/// +/// Can be created by using the [Uart::split] or [UartBase::split] API. +pub struct Rx(Uart); + +impl Rx { + fn new(uart: Uart) -> Self { + Self(uart) + } +} + impl Rx { /// Direct access to the peripheral structure. /// @@ -642,6 +647,33 @@ impl Rx { self.0.enable().modify(|_, w| w.rxenable().clear_bit()); } + /// Low level function to read a word from the UART FIFO. + /// + /// Uses the [nb] API to allow usage in blocking and non-blocking contexts. + /// + /// Please note that you might have to mask the returned value with 0xff to retrieve the actual + /// value if you use the manual parity mode. See chapter 11.4.1 for more information. + #[inline(always)] + pub fn read_fifo(&self) -> nb::Result { + if self.0.rxstatus().read().rdavl().bit_is_clear() { + return Err(nb::Error::WouldBlock); + } + Ok(self.read_fifo_unchecked()) + } + + /// Low level function to read a word from from the UART FIFO. + /// + /// This does not necesarily mean there is a word in the FIFO available. + /// Use the [Self::read_fifo] function to read a word from the FIFO reliably using the [nb] + /// API. + /// + /// Please note that you might have to mask the returned value with 0xff to retrieve the actual + /// value if you use the manual parity mode. See chapter 11.4.1 for more information. + #[inline(always)] + pub fn read_fifo_unchecked(&self) -> u32 { + self.0.data().read().bits() + } + pub fn to_rx_with_irq(self) -> RxWithIrq { RxWithIrq(self) } @@ -651,6 +683,17 @@ impl Rx { } } +/// Serial transmitter +/// +/// Can be created by using the [Uart::split] or [UartBase::split] API. +pub struct Tx(Uart); + +impl Tx { + fn new(uart: Uart) -> Self { + Self(uart) + } +} + impl Tx { /// Direct access to the peripheral structure. /// @@ -675,9 +718,35 @@ impl Tx { pub fn disable(&mut self) { self.0.enable().modify(|_, w| w.txenable().clear_bit()); } + + /// Low level function to write a word to the UART FIFO. + /// + /// Uses the [nb] API to allow usage in blocking and non-blocking contexts. + /// + /// Please note that you might have to mask the returned value with 0xff to retrieve the actual + /// value if you use the manual parity mode. See chapter 11.4.1 for more information. + #[inline(always)] + pub fn write_fifo(&self, data: u32) -> nb::Result<(), Infallible> { + if self.0.txstatus().read().wrrdy().bit_is_clear() { + return Err(nb::Error::WouldBlock); + } + self.write_fifo_unchecked(data); + Ok(()) + } + + /// Low level function to write a word to the UART FIFO. + /// + /// This does not necesarily mean that the FIFO can process another word because it might be + /// full. + /// Use the [Self::read_fifo] function to write a word to the FIFO reliably using the [nb] + /// API. + #[inline(always)] + pub fn write_fifo_unchecked(&self, data: u32) { + self.0.data().write(|w| unsafe { w.bits(data) }); + } } -#[derive(Default, Debug)] +#[derive(Default, Debug, Copy, Clone)] pub struct IrqUartError { overflow: bool, framing: bool, @@ -714,10 +783,11 @@ impl IrqUartError { } } -#[derive(Debug)] -pub enum IrqError { - BufferTooShort { found: usize, expected: usize }, - Uart(IrqUartError), +#[derive(Debug, PartialEq, Eq)] +#[cfg_attr(feature = "defmt", derive(defmt::Format))] +pub struct BufferTooShortError { + found: usize, + expected: usize, } /// Serial receiver, using interrupts to offload reading to the hardware. @@ -763,9 +833,9 @@ impl RxWithIrq { pub fn read_fixed_len_or_timeout_based_using_irq( &mut self, context: &mut IrqContextTimeoutOrMaxSize, - ) -> Result<(), Error> { + ) -> Result<(), TransferPendingError> { if context.mode != IrqReceptionMode::Idle { - return Err(Error::TransferPending); + return Err(TransferPendingError); } context.mode = IrqReceptionMode::Pending; context.rx_idx = 0; @@ -804,8 +874,9 @@ impl RxWithIrq { /// result of the operation. /// /// This function will not disable the RX interrupts, so you don't need to call any other - /// API after calling this function to continue emptying the FIFO. - pub fn irq_handler(&mut self, buf: &mut [u8; 16]) -> Result { + /// API after calling this function to continue emptying the FIFO. RX errors are handled + /// as partial errors and are returned as part of the [IrqResult]. + pub fn irq_handler(&mut self, buf: &mut [u8; 16]) -> IrqResult { let mut result = IrqResult::default(); let irq_end = self.uart().irq_end().read(); @@ -847,7 +918,7 @@ impl RxWithIrq { self.uart() .irq_clr() .write(|w| unsafe { w.bits(irq_end.bits()) }); - Ok(result) + result } /// This function should be called in the user provided UART interrupt handler. @@ -860,19 +931,20 @@ impl RxWithIrq { /// [IrqContextTimeoutOrMaxSize] structure. /// /// If passed buffer is equal to or larger than the specified maximum length, an - /// [`Error::BufferTooShort`] will be returned + /// [BufferTooShortError] will be returned. Other RX errors are treated as partial errors + /// and returned inside the [IrqResultMaxSizeOrTimeout] structure. pub fn irq_handler_max_size_or_timeout_based( &mut self, context: &mut IrqContextTimeoutOrMaxSize, buf: &mut [u8], - ) -> Result { + ) -> Result { if buf.len() < context.max_len { - return Err(IrqError::BufferTooShort { + return Err(BufferTooShortError { found: buf.len(), expected: context.max_len, }); } - let mut result = IrqResultMaxSizeTimeout::default(); + let mut result = IrqResultMaxSizeOrTimeout::default(); let irq_end = self.uart().irq_end().read(); let enb_status = self.uart().enable().read(); @@ -936,49 +1008,51 @@ impl RxWithIrq { fn read_handler( &self, - errors: &mut IrqUartError, - read_res: &nb::Result, + errors: &mut Option, + read_res: &nb::Result, ) -> Option { match read_res { Ok(byte) => Some(*byte), Err(nb::Error::WouldBlock) => None, Err(nb::Error::Other(e)) => { + // Ensure `errors` is Some(IrqUartError), initializing if it's None + let err = errors.get_or_insert(IrqUartError::default()); + + // Now we can safely modify fields inside `err` match e { - Error::Overrun => { - errors.overflow = true; - } - Error::FramingError => { - errors.framing = true; - } - Error::ParityError => { - errors.parity = true; - } - _ => { - errors.other = true; - } + RxError::Overrun => err.overflow = true, + RxError::Framing => err.framing = true, + RxError::Parity => err.parity = true, } None } } } - fn check_for_errors(&self, errors: &mut IrqUartError) { - // Read status register again, might have changed since reading received data + fn check_for_errors(&self, errors: &mut Option) { let rx_status = self.uart().rxstatus().read(); - if rx_status.rxovr().bit_is_set() { - errors.overflow = true; - } - if rx_status.rxfrm().bit_is_set() { - errors.framing = true; - } - if rx_status.rxpar().bit_is_set() { - errors.parity = true; + + if rx_status.rxovr().bit_is_set() + || rx_status.rxfrm().bit_is_set() + || rx_status.rxpar().bit_is_set() + { + let err = errors.get_or_insert(IrqUartError::default()); + + if rx_status.rxovr().bit_is_set() { + err.overflow = true; + } + if rx_status.rxfrm().bit_is_set() { + err.framing = true; + } + if rx_status.rxpar().bit_is_set() { + err.parity = true; + } } } fn irq_completion_handler_max_size_timeout( &mut self, - res: &mut IrqResultMaxSizeTimeout, + res: &mut IrqResultMaxSizeOrTimeout, context: &mut IrqContextTimeoutOrMaxSize, ) { self.disable_rx_irq_sources(); @@ -1000,18 +1074,34 @@ impl embedded_io::Error for Error { } } +impl embedded_io::Error for RxError { + fn kind(&self) -> embedded_io::ErrorKind { + embedded_io::ErrorKind::Other + } +} + impl embedded_hal_nb::serial::Error for Error { fn kind(&self) -> embedded_hal_nb::serial::ErrorKind { embedded_hal_nb::serial::ErrorKind::Other } } +impl embedded_hal_nb::serial::Error for RxError { + fn kind(&self) -> embedded_hal_nb::serial::ErrorKind { + match self { + RxError::Overrun => embedded_hal_nb::serial::ErrorKind::Overrun, + RxError::Framing => embedded_hal_nb::serial::ErrorKind::FrameFormat, + RxError::Parity => embedded_hal_nb::serial::ErrorKind::Parity, + } + } +} + impl embedded_io::ErrorType for Rx { - type Error = Error; + type Error = RxError; } impl embedded_hal_nb::serial::ErrorType for Rx { - type Error = Error; + type Error = RxError; } impl embedded_hal_nb::serial::Read for Rx { @@ -1019,11 +1109,11 @@ impl embedded_hal_nb::serial::Read for Rx { let uart = unsafe { &(*Uart::ptr()) }; let status_reader = uart.rxstatus().read(); let err = if status_reader.rxovr().bit_is_set() { - Some(Error::Overrun) + Some(RxError::Overrun) } else if status_reader.rxfrm().bit_is_set() { - Some(Error::FramingError) + Some(RxError::Framing) } else if status_reader.rxpar().bit_is_set() { - Some(Error::ParityError) + Some(RxError::Parity) } else { None }; @@ -1032,14 +1122,15 @@ impl embedded_hal_nb::serial::Read for Rx { // and parity status bits. We have to read the DATA register // so that the next status reflects the next DATA word // For overrun error, we read as well to clear the peripheral - uart.data().read().bits(); - Err(err.into()) - } else if status_reader.rdavl().bit_is_set() { - let data = uart.data().read().bits(); - Ok((data & 0xff) as u8) - } else { - Err(nb::Error::WouldBlock) + self.read_fifo_unchecked(); + return Err(err.into()); } + self.read_fifo().map(|val| (val & 0xff) as u8).map_err(|e| { + if let nb::Error::Other(_) = e { + unreachable!() + } + nb::Error::WouldBlock + }) } } @@ -1059,28 +1150,16 @@ impl embedded_io::Read for Rx { } impl embedded_io::ErrorType for Tx { - type Error = Error; + type Error = Infallible; } impl embedded_hal_nb::serial::ErrorType for Tx { - type Error = Error; + type Error = Infallible; } impl embedded_hal_nb::serial::Write for Tx { fn write(&mut self, word: u8) -> nb::Result<(), Self::Error> { - let reader = unsafe { &(*Uart::ptr()) }.txstatus().read(); - if reader.wrrdy().bit_is_clear() { - return Err(nb::Error::WouldBlock); - } else { - // DPARITY bit not supported yet - unsafe { - // NOTE(unsafe) atomic write to data register - // NOTE(write_volatile) 8-bit write that's not - // possible through the svd2rust API - (*Uart::ptr()).data().write(|w| w.bits(word as u32)); - } - } - Ok(()) + self.write_fifo(word as u32) } fn flush(&mut self) -> nb::Result<(), Self::Error> { @@ -1123,16 +1202,26 @@ impl embedded_hal_nb::serial::ErrorType for UartBase impl embedded_hal_nb::serial::Read for UartBase { fn read(&mut self) -> nb::Result { - self.rx.read() + self.rx.read().map_err(|e| e.map(Error::Rx)) } } impl embedded_hal_nb::serial::Write for UartBase { fn write(&mut self, word: u8) -> nb::Result<(), Self::Error> { - self.tx.write(word) + self.tx.write(word).map_err(|e| { + if let nb::Error::Other(_) = e { + unreachable!() + } + nb::Error::WouldBlock + }) } fn flush(&mut self) -> nb::Result<(), Self::Error> { - self.tx.flush() + self.tx.flush().map_err(|e| { + if let nb::Error::Other(_) = e { + unreachable!() + } + nb::Error::WouldBlock + }) } } -- 2.43.0