From 0525d2de979d3084f2ec98dd271bc61991f95a00 Mon Sep 17 00:00:00 2001 From: Fabian Date: Tue, 28 Nov 2023 09:17:34 +0100 Subject: [PATCH] Remove Serial::split (#351) --- CHANGELOG.md | 3 +- src/dma.rs | 6 - src/serial.rs | 333 ---------------------------------------- testsuite/tests/uart.rs | 29 +--- 4 files changed, 10 insertions(+), 361 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c28d8e85..939ecfb9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -26,7 +26,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/). - Add `num_traits::PrimInt` bounds to `Word` - Remove `Serial::split`, which possibly creates two mutable references two one Serial instance, which could've caused UB. The use case of this function - was hard to find out anyway. ([#352]) + was hard to find out anyway. ([#351]) ### Added @@ -610,6 +610,7 @@ let clocks = rcc [filter]: https://defmt.ferrous-systems.com/filtering.html [#352]: https://github.com/stm32-rs/stm32f3xx-hal/pull/352 +[#351]: https://github.com/stm32-rs/stm32f3xx-hal/pull/351 [#345]: https://github.com/stm32-rs/stm32f3xx-hal/pull/345 [#346]: https://github.com/stm32-rs/stm32f3xx-hal/pull/346 [#347]: https://github.com/stm32-rs/stm32f3xx-hal/pull/347 diff --git a/src/dma.rs b/src/dma.rs index 211462d0..96bcc315 100644 --- a/src/dma.rs +++ b/src/dma.rs @@ -571,8 +571,6 @@ dma!( 2: { 1,2,3,4,5 } ); /// Marker trait mapping DMA targets to their channels pub trait OnChannel: Target + crate::private::Sealed {} -use crate::serial::{RxPin, TxPin}; - macro_rules! on_channel { ( $( @@ -581,10 +579,6 @@ macro_rules! on_channel { ) => { $( $( - impl crate::private::Sealed for serial::Tx<$USART, Pin> {} - impl OnChannel<$dma::$TxChannel> for serial::Tx<$USART, Pin> where Pin: TxPin<$USART> {} - impl crate::private::Sealed for serial::Rx<$USART, Pin> {} - impl OnChannel<$dma::$RxChannel> for serial::Rx<$USART, Pin> where Pin: RxPin<$USART> {} impl crate::private::Sealed for serial::Serial<$USART, (Tx, Rx)> {} impl OnChannel<$dma::$TxChannel> for serial::Serial<$USART, (Tx, Rx)> {} impl OnChannel<$dma::$RxChannel> for serial::Serial<$USART, (Tx, Rx)> {} diff --git a/src/serial.rs b/src/serial.rs index bf9e3723..ae8b6206 100644 --- a/src/serial.rs +++ b/src/serial.rs @@ -17,7 +17,6 @@ use crate::{ gpio::{gpioa, gpiob, gpioc, AF7}, hal::{blocking, serial, serial::Write}, pac::{ - self, rcc::cfgr3::USART1SW_A, usart1::{cr1::M_A, cr1::PCE_A, cr1::PS_A, RegisterBlock}, Interrupt, USART1, USART2, USART3, @@ -36,7 +35,6 @@ use cfg_if::cfg_if; use enumset::{EnumSet, EnumSetType}; use crate::dma; -use cortex_m::interrupt; /// Interrupt and status events. /// @@ -332,128 +330,6 @@ pub struct Serial { pins: Pins, } -mod split { - use super::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 - } - } - - 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 - } - } -} - -pub use split::{Rx, Tx}; - impl Serial where Usart: Instance, @@ -543,35 +419,6 @@ where .modify(|_, w| w.ue().disabled().re().disabled().te().disabled()); (self.usart, self.pins) } - - /// Joins previously [`Serial::split()`] serial. - /// - /// This is often needed to access methods only implemented for [`Serial`] - /// but not for [`Tx`] nor [`Rx`]. - /// - /// # Example - /// - /// ``` - /// let dp = pac::Peripherals::take().unwrap(); - /// - /// (tx, rx) = Serial::new(dp.USART1, ...).split(); - /// - /// // Do something with tx and rx - /// - /// serial = Serial::join(tx, rx); - /// ``` - pub fn join(tx: split::Tx, rx: split::Rx) -> Self - where - Tx: TxPin, - Rx: RxPin, - { - let (usart, tx_pin) = tx.free(); - let rx_pin = rx.free(); - Self { - usart, - pins: (tx_pin, rx_pin), - } - } } impl Serial @@ -965,19 +812,6 @@ where } } -impl serial::Read for Rx -where - Usart: Instance, - Pin: RxPin, -{ - type Error = Error; - - /// This implementation shares the same effects as the [`Serial`]s [`serial::Read`] implemenation. - fn read(&mut self) -> nb::Result { - eh_read(unsafe { self.usart_mut() }) - } -} - impl serial::Write for Serial where Usart: Instance, @@ -1022,145 +856,6 @@ 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) -> dma::Transfer - where - Self: dma::OnChannel, - B: dma::WriteBuffer + 'static, - C: dma::Channel, - { - // NOTE(unsafe) usage of a valid peripheral address - unsafe { - channel.set_peripheral_address( - core::ptr::addr_of!(self.usart().rdr) as u32, - dma::Increment::Disable, - ); - }; - - dma::Transfer::start_write(buffer, channel, self) - } -} - -impl blocking::serial::write::Default for Tx -where - Usart: Instance, - Pin: TxPin, -{ -} - -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) -> dma::Transfer - where - Self: dma::OnChannel, - B: dma::ReadBuffer + 'static, - C: dma::Channel, - { - // NOTE(unsafe) usage of a valid peripheral address - unsafe { - channel.set_peripheral_address( - core::ptr::addr_of!(self.usart().tdr) as u32, - dma::Increment::Disable, - ); - }; - - 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, @@ -1259,34 +954,6 @@ macro_rules! usart { const INTERRUPT: Interrupt = $INTERRUPT; } - impl Serial<$USARTX, (Tx, Rx)> - where Tx: TxPin<$USARTX>, Rx: RxPin<$USARTX> { - /// Splits the [`Serial`] abstraction into a transmitter and a receiver half. - /// - /// This allows using [`Tx`] and [`Rx`] related actions to - /// be handled independently and even use these safely in different - /// contexts (like interrupt routines) without needing to do synchronization work - /// between them. - pub fn split(self) -> (split::Tx<$USARTX, Tx>, split::Rx<$USARTX, Rx>) { - // NOTE(unsafe): This essentially duplicates the USART peripheral - // - // As RX and TX both do have direct access to the peripheral, - // they must guarantee to only do atomic operations on the peripheral - // registers to avoid data races. - // - // Tx and Rx won't access the same registers anyways, - // as they have independent responsibilities, which are NOT represented - // in the type system. - let (tx, rx) = unsafe { - ( - pac::Peripherals::steal().$USARTX, - pac::Peripherals::steal().$USARTX, - ) - }; - (split::Tx::new(tx, self.pins.0), split::Rx::new(rx, self.pins.1)) - } - } - #[cfg(feature = "defmt")] impl defmt::Format for Serial<$USARTX, Pins> { fn format(&self, f: defmt::Formatter) { diff --git a/testsuite/tests/uart.rs b/testsuite/tests/uart.rs index 31fe5c21..8a2fd933 100644 --- a/testsuite/tests/uart.rs +++ b/testsuite/tests/uart.rs @@ -178,19 +178,6 @@ mod tests { } } - #[test] - fn send_receive_split(state: &mut super::State) { - let (mut tx, mut rx) = unwrap!(state.serial1.take()).split(); - - for i in TEST_MSG { - defmt::unwrap!(nb::block!(tx.write(i))); - let c = unwrap!(nb::block!(rx.read())); - assert_eq!(c, i); - } - - state.serial1 = Some(Serial::join(tx, rx)); - } - #[test] fn test_overrun(state: &mut super::State) { let (usart, pins) = unwrap!(state.serial1.take()).free(); @@ -340,20 +327,20 @@ mod tests { #[test] fn send_receive_wrong_baud(state: &mut super::State) { - let (mut tx_slow, mut rx_slow) = unwrap!(state.serial_slow.take()).split(); - let (mut tx_fast, mut rx_fast) = unwrap!(state.serial_fast.take()).split(); + let mut usart_slow = unwrap!(state.serial_slow.take()); + let mut usart_fast = unwrap!(state.serial_fast.take()); // provoke an error (framing) - unwrap!(nb::block!(tx_slow.write(b'a'))); - let c = nb::block!(rx_fast.read()); + unwrap!(nb::block!(usart_slow.write(b'a'))); + let c = nb::block!(usart_fast.read()); defmt::debug!("{}", c); assert!(matches!(c, Err(Error::Framing))); // provoke an error (this does not seem to be absolutely deterministic // and we've seen multiple error variants in the wild, including // receiving the wrong but valid character) - unwrap!(nb::block!(tx_fast.write(b'a'))); - let result = nb::block!(rx_slow.read()); + unwrap!(nb::block!(usart_fast.write(b'a'))); + let result = nb::block!(usart_slow.read()); defmt::debug!("{}", result); assert!(match result { Ok(c) => { @@ -364,8 +351,8 @@ mod tests { Err(_) => false, }); - state.serial_slow = Some(Serial::join(tx_slow, rx_slow)); - state.serial_fast = Some(Serial::join(tx_fast, rx_fast)); + state.serial_slow = Some(usart_slow); + state.serial_fast = Some(usart_fast); } // TODO: Currently, this is a limited test, just to see, that