From 469852c667cd7105d5eba7b197e1e2b5b9528d15 Mon Sep 17 00:00:00 2001 From: huntc Date: Wed, 1 Dec 2021 09:14:24 +1100 Subject: [PATCH 1/2] Removed unsafe from uarte The constructors themselves are not strictly unsafe. Interactions with DMA can be generally unsafe if a future is dropped, but that's a separate issue. It is important that we use the `unsafe` keyword diligently as it can lead to confusion otherwise. --- embassy-nrf/src/buffered_uarte.rs | 29 ++++++++++++------------ embassy-nrf/src/uarte.rs | 9 +------- examples/nrf/src/bin/buffered_uart.rs | 32 +++++++++++++-------------- examples/nrf/src/bin/uart.rs | 3 +-- 4 files changed, 32 insertions(+), 41 deletions(-) diff --git a/embassy-nrf/src/buffered_uarte.rs b/embassy-nrf/src/buffered_uarte.rs index 717ada78..9763774d 100644 --- a/embassy-nrf/src/buffered_uarte.rs +++ b/embassy-nrf/src/buffered_uarte.rs @@ -65,8 +65,7 @@ pub struct BufferedUarte<'d, U: UarteInstance, T: TimerInstance> { impl<'d, U: UarteInstance, T: TimerInstance> Unpin for BufferedUarte<'d, U, T> {} impl<'d, U: UarteInstance, T: TimerInstance> BufferedUarte<'d, U, T> { - /// unsafe: may not leak self or futures - pub unsafe fn new( + pub fn new( state: &'d mut State<'d, U, T>, _uarte: impl Unborrow + 'd, timer: impl Unborrow + 'd, @@ -160,20 +159,22 @@ impl<'d, U: UarteInstance, T: TimerInstance> BufferedUarte<'d, U, T> { ppi_ch2.enable(); Self { - inner: PeripheralMutex::new_unchecked(irq, &mut state.0, move || StateInner { - phantom: PhantomData, - timer, - _ppi_ch1: ppi_ch1, - _ppi_ch2: ppi_ch2, + inner: unsafe { + PeripheralMutex::new_unchecked(irq, &mut state.0, move || StateInner { + phantom: PhantomData, + timer, + _ppi_ch1: ppi_ch1, + _ppi_ch2: ppi_ch2, - rx: RingBuffer::new(rx_buffer), - rx_state: RxState::Idle, - rx_waker: WakerRegistration::new(), + rx: RingBuffer::new(rx_buffer), + rx_state: RxState::Idle, + rx_waker: WakerRegistration::new(), - tx: RingBuffer::new(tx_buffer), - tx_state: TxState::Idle, - tx_waker: WakerRegistration::new(), - }), + tx: RingBuffer::new(tx_buffer), + tx_state: TxState::Idle, + tx_waker: WakerRegistration::new(), + }) + }, } } diff --git a/embassy-nrf/src/uarte.rs b/embassy-nrf/src/uarte.rs index 459bc843..543f7fb7 100644 --- a/embassy-nrf/src/uarte.rs +++ b/embassy-nrf/src/uarte.rs @@ -48,14 +48,7 @@ pub struct Uarte<'d, T: Instance> { impl<'d, T: Instance> Uarte<'d, T> { /// Creates the interface to a UARTE instance. /// Sets the baud rate, parity and assigns the pins to the UARTE peripheral. - /// - /// # Safety - /// - /// The returned API is safe unless you use `mem::forget` (or similar safe mechanisms) - /// on stack allocated buffers which which have been passed to [`send()`](Uarte::send) - /// or [`receive`](Uarte::receive). - #[allow(unused_unsafe)] - pub unsafe fn new( + pub fn new( _uarte: impl Unborrow + 'd, irq: impl Unborrow + 'd, rxd: impl Unborrow + 'd, diff --git a/examples/nrf/src/bin/buffered_uart.rs b/examples/nrf/src/bin/buffered_uart.rs index 90d28878..5d9075ed 100644 --- a/examples/nrf/src/bin/buffered_uart.rs +++ b/examples/nrf/src/bin/buffered_uart.rs @@ -24,23 +24,21 @@ async fn main(_spawner: Spawner, p: Peripherals) { let irq = interrupt::take!(UARTE0_UART0); let mut state = State::new(); - let u = unsafe { - BufferedUarte::new( - &mut state, - p.UARTE0, - p.TIMER0, - p.PPI_CH0, - p.PPI_CH1, - irq, - p.P0_08, - p.P0_06, - NoPin, - NoPin, - config, - &mut rx_buffer, - &mut tx_buffer, - ) - }; + let u = BufferedUarte::new( + &mut state, + p.UARTE0, + p.TIMER0, + p.PPI_CH0, + p.PPI_CH1, + irq, + p.P0_08, + p.P0_06, + NoPin, + NoPin, + config, + &mut rx_buffer, + &mut tx_buffer, + ); pin_mut!(u); info!("uarte initialized!"); diff --git a/examples/nrf/src/bin/uart.rs b/examples/nrf/src/bin/uart.rs index 6b89e083..208961c8 100644 --- a/examples/nrf/src/bin/uart.rs +++ b/examples/nrf/src/bin/uart.rs @@ -18,8 +18,7 @@ async fn main(_spawner: Spawner, p: Peripherals) { config.baudrate = uarte::Baudrate::BAUD115200; let irq = interrupt::take!(UARTE0_UART0); - let mut uart = - unsafe { uarte::Uarte::new(p.UARTE0, irq, p.P0_08, p.P0_06, NoPin, NoPin, config) }; + let mut uart = uarte::Uarte::new(p.UARTE0, irq, p.P0_08, p.P0_06, NoPin, NoPin, config); info!("uarte initialized!"); From 496ad4ed43ab3b0948553d371709771c010de565 Mon Sep 17 00:00:00 2001 From: huntc Date: Wed, 1 Dec 2021 09:29:45 +1100 Subject: [PATCH 2/2] Rationale for uarte usage --- embassy-nrf/src/buffered_uarte.rs | 4 ++++ embassy-nrf/src/uarte.rs | 11 +++++++++++ 2 files changed, 15 insertions(+) diff --git a/embassy-nrf/src/buffered_uarte.rs b/embassy-nrf/src/buffered_uarte.rs index 9763774d..5468f6c0 100644 --- a/embassy-nrf/src/buffered_uarte.rs +++ b/embassy-nrf/src/buffered_uarte.rs @@ -1,3 +1,7 @@ +//! Async buffered UART +//! +//! Please ee [uarte] to understand when [BufferedUarte] should be used. + use core::cmp::min; use core::marker::PhantomData; use core::mem; diff --git a/embassy-nrf/src/uarte.rs b/embassy-nrf/src/uarte.rs index 543f7fb7..60e69e03 100644 --- a/embassy-nrf/src/uarte.rs +++ b/embassy-nrf/src/uarte.rs @@ -1,6 +1,17 @@ #![macro_use] //! Async UART +//! +//! Async UART is provided in two flavors - this one and also [buffered_uarte::BufferedUarte]. +//! The [Uarte] here is useful for those use-cases where reading the UARTE peripheral is +//! exclusively awaited on. If the [Uarte] is required to be awaited on with some other future, +//! for example when using `futures_util::future::select`, then you should consider +//! [buffered_uarte::BufferedUarte] so that reads may continue while processing these +//! other futures. If you do not then you may lose data between reads. +//! +//! An advantage of the [Uarte] has over [buffered_uarte::BufferedUarte] is that less +//! memory may be used given that buffers are passed in directly to its read and write +//! methods. use core::future::Future; use core::marker::PhantomData;