From cc6fc308c3495e873aee559e43156886ce90fdeb Mon Sep 17 00:00:00 2001 From: Robin Mueller Date: Sat, 23 Mar 2024 13:05:30 +0100 Subject: [PATCH] DMA impl improvements 1. Wrap the TX transfer struct in a helper type which also checks whether the TC IRQ flag was set. This is strongly recommended in the datasheet 2. Basic refactoring to prepare allowing checks of events for both the split Tx and Rx handle --- CHANGELOG.md | 18 ++ src/dma.rs | 5 + src/serial.rs | 468 +++++++++++++++++++++++++++++++++++++++++++++++--- 3 files changed, 470 insertions(+), 21 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d357e1f7..7f018519 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -68,6 +68,24 @@ No changes. ### Changed - The MSRV was bumped to 1.59 ([#340]) +### Changed + +- serial: The DMA functions `write_all` and `read_exact` now returns + the wrapper structs `SerialDmaTx` and `SerialDmaRx` instead of the direct + DMA transfer struct. These allow checking the USART ISR events + with `is_event_triggered` as well. + +### Fixed + +- serial: The previous DMA `write_all` implementation did use the DMA transfer completion + event to check for transfer completion, but MCU datasheet specifies that the TC + flag of the USART peripheral should be checked for transfer completion to avoid + corruption of the last transfer. This is now done by the new `SerialDmaTx` wrapper. + +## Added + +- serial: Public `is_event_triggered` method which allows to check for events + given an event and a USART reference. ## [v0.9.1] - 2022-09-07 diff --git a/src/dma.rs b/src/dma.rs index 01f771c9..30f39c5c 100644 --- a/src/dma.rs +++ b/src/dma.rs @@ -172,6 +172,11 @@ impl Transfer { self.stop() } + + pub(crate) fn target(&self) -> &T { + let inner = crate::unwrap!(self.inner.as_ref()); + &inner.target + } } impl Drop for Transfer { diff --git a/src/serial.rs b/src/serial.rs index 7af493f4..c9f481dd 100644 --- a/src/serial.rs +++ b/src/serial.rs @@ -139,6 +139,28 @@ pub enum Event { // WakeupFromStopMode, } +/// Check if an interrupt event happend. +#[inline] +pub fn is_event_triggered(uart: &impl Instance, event: Event) -> bool { + let isr = uart.isr.read(); + match event { + Event::TransmitDataRegisterEmtpy => isr.txe().bit(), + Event::CtsInterrupt => isr.ctsif().bit(), + Event::TransmissionComplete => isr.tc().bit(), + Event::ReceiveDataRegisterNotEmpty => isr.rxne().bit(), + Event::OverrunError => isr.ore().bit(), + Event::Idle => isr.idle().bit(), + Event::ParityError => isr.pe().bit(), + Event::LinBreak => isr.lbdf().bit(), + Event::NoiseError => isr.nf().bit(), + Event::FramingError => isr.fe().bit(), + Event::CharacterMatch => isr.cmf().bit(), + Event::ReceiverTimeout => isr.rtof().bit(), + // Event::EndOfBlock => isr.eobf().bit(), + // Event::WakeupFromStopMode => isr.wuf().bit(), + } +} + /// Serial error /// /// As these are status events, they can be converted to [`Event`]s, via [`Into`]. @@ -330,6 +352,144 @@ pub struct Serial { pins: Pins, } +mod split { + use super::{is_event_triggered, Event, Instance}; + /// Serial receiver + #[derive(Debug)] + #[cfg_attr(feature = "defmt", derive(defmt::Format))] + pub struct Rx { + usart: Usart, + pub(crate) pin: Pin, + } + + /// Serial transmitter + #[derive(Debug)] + #[cfg_attr(feature = "defmt", derive(defmt::Format))] + pub struct Tx { + usart: Usart, + pub(crate) pin: Pin, + } + + impl Tx + where + Usart: Instance, + Pin: super::TxPin, + { + pub(crate) fn new(usart: Usart, pin: Pin) -> Self { + Tx { usart, pin } + } + + /// Destruct [`Tx`] to regain access to underlying USART and pin. + pub(crate) fn free(self) -> (Usart, Pin) { + (self.usart, self.pin) + } + } + + impl Tx + where + Usart: Instance, + { + /// Get a reference to internal usart peripheral + /// + /// # Safety + /// + /// This is unsafe, because the creation of this struct + /// is only possible by splitting the the USART peripheral + /// into Tx and Rx, which are internally both pointing + /// to the same peripheral. + /// + /// Therefor, if getting a mutuable reference to the peripheral + /// or changing any of it's configuration, the exclusivity + /// is no longer guaranteed by the type system. + /// + /// Ensure that the Tx and Rx implemtation only do things with + /// the peripheral, which do not effect the other. + pub(crate) unsafe fn usart(&self) -> &Usart { + &self.usart + } + + /// Get a reference to internal usart peripheral + /// + /// # Saftey + /// + /// Same as in [`Self::usart()`]. + #[allow(dead_code)] + pub(crate) unsafe fn usart_mut(&mut self) -> &mut Usart { + &mut self.usart + } + + /// Check if an interrupt event happend. + #[inline] + pub fn is_event_triggered(&self, event: Event) -> bool { + // Safety: We are only reading the ISR register here, which + // should not affect the RX half + is_event_triggered(unsafe { self.usart() }, event) + } + } + + impl Rx + where + Usart: Instance, + Pin: super::RxPin, + { + pub(crate) fn new(usart: Usart, pin: Pin) -> Self { + Rx { usart, pin } + } + + /// Destruct [`Rx`] to regain access to the underlying pin. + /// + /// The USART is omitted, as it is returnend from Tx already to avoid + /// beeing able to crate a duplicate reference to the same underlying + /// peripheral. + pub(crate) fn free(self) -> Pin { + self.pin + } + } + + impl Rx + where + Usart: Instance, + { + /// Get a reference to internal usart peripheral + /// + /// # Safety + /// + /// This is unsafe, because the creation of this struct + /// is only possible by splitting the the USART peripheral + /// into Tx and Rx, which are internally both pointing + /// to the same peripheral. + /// + /// Therefor, if getting a mutuable reference to the peripheral + /// or changing any of it's configuration, the exclusivity + /// is no longer guaranteed by the type system. + /// + /// Ensure that the Tx and Rx implemtation only do things with + /// the peripheral, which do not effect the other. + pub(crate) unsafe fn usart(&self) -> &Usart { + &self.usart + } + + /// Get a reference to internal usart peripheral + /// + /// # Saftey + /// + /// Same as in [`Self::usart()`]. + pub(crate) unsafe fn usart_mut(&mut self) -> &mut Usart { + &mut self.usart + } + + /// Check if an interrupt event happend. + #[inline] + pub fn is_event_triggered(&self, event: Event) -> bool { + // Safety: We are only reading the ISR register here, which + // should not affect the TX half + is_event_triggered(unsafe { self.usart() }, event) + } + } +} + +pub use split::{Rx, Tx}; + impl Serial where Usart: Instance, @@ -572,23 +732,7 @@ where /// Check if an interrupt event happend. #[inline] pub fn is_event_triggered(&self, event: Event) -> bool { - let isr = self.usart.isr.read(); - match event { - Event::TransmitDataRegisterEmtpy => isr.txe().bit(), - Event::CtsInterrupt => isr.ctsif().bit(), - Event::TransmissionComplete => isr.tc().bit(), - Event::ReceiveDataRegisterNotEmpty => isr.rxne().bit(), - Event::OverrunError => isr.ore().bit(), - Event::Idle => isr.idle().bit(), - Event::ParityError => isr.pe().bit(), - Event::LinBreak => isr.lbdf().bit(), - Event::NoiseError => isr.nf().bit(), - Event::FramingError => isr.fe().bit(), - Event::CharacterMatch => isr.cmf().bit(), - Event::ReceiverTimeout => isr.rtof().bit(), - // Event::EndOfBlock => isr.eobf().bit(), - // Event::WakeupFromStopMode => isr.wuf().bit(), - } + is_event_triggered(&self.usart, event) } /// Get an [`EnumSet`] of all fired interrupt events. @@ -856,12 +1000,290 @@ impl blocking::serial::write::Default for Serial serial::Write for Tx +where + Usart: Instance, + Pin: TxPin, +{ + // NOTE(Infallible) See section "29.7 USART interrupts"; the only possible errors during + // transmission are: clear to send (which is disabled in this case) errors and + // framing errors (which only occur in SmartCard mode); neither of these apply to + // our hardware configuration + type Error = Infallible; + + fn flush(&mut self) -> nb::Result<(), Infallible> { + let isr = unsafe { self.usart().isr.read() }; + + if isr.tc().bit_is_set() { + Ok(()) + } else { + Err(nb::Error::WouldBlock) + } + } + + fn write(&mut self, byte: u8) -> nb::Result<(), Infallible> { + // NOTE(unsafe) atomic read with no side effects + let isr = unsafe { self.usart().isr.read() }; + + if isr.txe().bit_is_set() { + // NOTE(unsafe) atomic write to stateless register + unsafe { self.usart().tdr.write(|w| w.tdr().bits(u16::from(byte))) }; + Ok(()) + } else { + Err(nb::Error::WouldBlock) + } + } +} + +impl fmt::Write for Tx +where + Tx: serial::Write, +{ + fn write_str(&mut self, s: &str) -> fmt::Result { + s.bytes() + .try_for_each(|c| nb::block!(self.write(c))) + .map_err(|_| fmt::Error) + } +} + +impl Rx +where + Usart: Instance + Dma, +{ + /// Fill the buffer with received data using DMA. + pub fn read_exact(self, buffer: B, mut channel: C) -> SerialDmaRx + where + Self: dma::OnChannel, + B: dma::WriteBuffer + 'static, + C: dma::Channel, + { + // NOTE(unsafe) usage of a valid peripheral address + unsafe { + channel.set_peripheral_address( + &self.usart().rdr as *const _ as u32, + dma::Increment::Disable, + ) + }; + + SerialDmaRx { + transfer: dma::Transfer::start_write(buffer, channel, self), + } + } +} + +impl blocking::serial::write::Default for Tx +where + Usart: Instance, + Pin: TxPin, +{ +} + +/// Thin wrapper struct over the DMA transfer struct. +/// +/// This wrapper mostly exposes the [`dma::Transfer`] API but also also exposes +/// an API to check for other USART ISR events during on-going transfers. +pub struct SerialDmaRx + 'static, C: dma::Channel, T: dma::Target> { + transfer: dma::Transfer, +} + +impl SerialDmaRx +where + B: dma::WriteBuffer, + C: dma::Channel, + T: dma::Target, +{ + /// Call [`dma::Transfer::stop`]. + pub fn stop(self) -> (B, C, T) { + self.transfer.stop() + } + + /// Call [`dma::Transfer::is_complete`]. + pub fn is_complete(&self) -> bool { + self.transfer.is_complete() + } + + /// Call [`dma::Transfer::wait`]. + pub fn wait(self) -> (B, C, T) { + self.transfer.wait() + } +} + +impl SerialDmaRx> +where + B: dma::WriteBuffer, + C: dma::Channel, + Usart: Instance + Dma, + Pin: RxPin, +{ + /// Check if an interrupt event happened. + pub fn is_event_triggered(&self, event: Event) -> bool { + self.transfer.target().is_event_triggered(event) + } +} + +impl SerialDmaRx> +where + B: dma::WriteBuffer, + C: dma::Channel, + Usart: Instance + Dma, +{ + /// Check if an interrupt event happened. + pub fn is_event_triggered(&self, event: Event) -> bool { + self.transfer.target().is_event_triggered(event) + } +} + +/// Thin wrapper struct over the DMA transfer struct. +/// +/// This wrapper mostly exposes the [`dma::Transfer`] API but also implements +/// some additional checks because the conditions for DMA transfer completion +/// require that the USART TC ISR flag is set as well. It also exposes an API +/// to check for other USART ISR events during ongoing transfers. +pub struct SerialDmaTx + 'static, C: dma::Channel, T: dma::Target> { + transfer: dma::Transfer, +} + +impl SerialDmaTx +where + B: dma::ReadBuffer, + C: dma::Channel, + T: dma::Target, +{ + /// Calls [`dma::Transfer::stop`]. + pub fn stop(self) -> (B, C, T) { + self.transfer.stop() + } +} + +impl SerialDmaTx> +where + Usart: Instance + Dma, + C: dma::Channel, + B: dma::ReadBuffer, + Pin: TxPin, +{ + /// Wrapper function which can be used to check transfer completion. + /// + /// In addition to checking the transfer completion of the DMA, it also checks that the + /// USART Transmission Complete flag was set by the hardware. According to RM0316 29.5.15, this + /// is required to avoid corrupting the last transmission before disabling the USART or entering + /// stop mode. + pub fn is_complete(&self) -> bool { + let target = self.transfer.target(); + self.transfer.is_complete() && target.is_event_triggered(Event::TransmissionComplete) + } + + /// Block until the transfer is complete. This function also uses + /// [`SerialDmaTx::is_complete`] to check that the USART TC flag was set by + /// the hardware. + pub fn wait(self) -> (B, C, Tx) { + while !self.is_complete() {} + self.stop() + } + + /// Check if an interrupt event happened. + pub fn is_event_triggered(&self, event: Event) -> bool { + self.transfer.target().is_event_triggered(event) + } +} + +impl SerialDmaTx> +where + Usart: Instance + Dma, + C: dma::Channel, + B: dma::ReadBuffer, +{ + /// Wrapper function which can be used to check transfer completion. + /// + /// In addition to checking the transfer completion of the DMA, it also checks that the + /// USART Transmission Complete flag was set by the hardware. According to RM0316 29.5.15, this + /// is required to avoid corrupting the last transmission before disabling the USART or entering + /// stop mode. + pub fn is_complete(&self) -> bool { + let target = self.transfer.target(); + self.transfer.is_complete() && target.is_event_triggered(Event::TransmissionComplete) + } + + /// Block until the transfer is complete. This function also uses + /// [`SerialDmaTx::is_complete`] to check that the USART TC flag was set by + /// the hardware. + pub fn wait(self) -> (B, C, Serial) { + while !self.is_complete() {} + self.stop() + } +} + +impl Tx +where + Usart: Instance + Dma, + Pin: TxPin, +{ + /// Transmit all data in the buffer using DMA. + pub fn write_all(self, buffer: B, mut channel: C) -> SerialDmaTx + where + Self: dma::OnChannel, + B: dma::ReadBuffer + 'static, + C: dma::Channel, + { + // NOTE(unsafe) usage of a valid peripheral address + unsafe { + channel.set_peripheral_address( + &self.usart().tdr as *const _ as u32, + dma::Increment::Disable, + ) + }; + + SerialDmaTx { + transfer: dma::Transfer::start_read(buffer, channel, self), + } + } +} + +impl dma::Target for Rx +where + Usart: Instance + Dma, +{ + fn enable_dma(&mut self) { + // NOTE(unsafe) critical section prevents races + interrupt::free(|_| unsafe { + self.usart().cr3.modify(|_, w| w.dmar().enabled()); + }); + } + + fn disable_dma(&mut self) { + // NOTE(unsafe) critical section prevents races + interrupt::free(|_| unsafe { + self.usart().cr3.modify(|_, w| w.dmar().disabled()); + }); + } +} + +impl dma::Target for Tx +where + Usart: Instance + Dma, + Pin: TxPin, +{ + fn enable_dma(&mut self) { + // NOTE(unsafe) critical section prevents races + interrupt::free(|_| unsafe { + self.usart().cr3.modify(|_, w| w.dmat().enabled()); + }); + } + + fn disable_dma(&mut self) { + // NOTE(unsafe) critical section prevents races + interrupt::free(|_| unsafe { + self.usart().cr3.modify(|_, w| w.dmat().disabled()); + }); + } +} + impl Serial where Usart: Instance + Dma, { /// Fill the buffer with received data using DMA. - pub fn read_exact(self, buffer: B, mut channel: C) -> dma::Transfer + pub fn read_exact(self, buffer: B, mut channel: C) -> SerialDmaRx where Self: dma::OnChannel, B: dma::WriteBuffer + 'static, @@ -875,11 +1297,13 @@ where ); }; - dma::Transfer::start_write(buffer, channel, self) + SerialDmaRx { + transfer: dma::Transfer::start_write(buffer, channel, self), + } } /// Transmit all data in the buffer using DMA. - pub fn write_all(self, buffer: B, mut channel: C) -> dma::Transfer + pub fn write_all(self, buffer: B, mut channel: C) -> SerialDmaTx where Self: dma::OnChannel, B: dma::ReadBuffer + 'static, @@ -893,7 +1317,9 @@ where ); }; - dma::Transfer::start_read(buffer, channel, self) + SerialDmaTx { + transfer: dma::Transfer::start_read(buffer, channel, self), + } } }