Merge #509

509: Remove unsafe from nRF uarte and improve doco with rationale r=Dirbaio a=huntc

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.

I've also provided some rationale re. the usage of [Uarte] vs [BufferedUarte].

Co-authored-by: huntc <huntchr@gmail.com>

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;
@@ -65,8 +69,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 fn new(
-    pub unsafe fn new(
         state: &'d mut State<'d, U, T>,
         _uarte: impl Unborrow<Target = U> + 'd,
         timer: impl Unborrow<Target = T> + 'd,
@@ -160,7 +163,8 @@ impl<'d, U: UarteInstance, T: TimerInstance> BufferedUarte<'d, U, T> {
         ppi_ch2.enable();
 
         Self {
-            inner: PeripheralMutex::new_unchecked(irq, &mut state.0, move || StateInner {
+            inner: unsafe {
+                PeripheralMutex::new_unchecked(irq, &mut state.0, move || StateInner {
                     phantom: PhantomData,
                     timer,
                     _ppi_ch1: ppi_ch1,
@@ -173,7 +177,8 @@ impl<'d, U: UarteInstance, T: TimerInstance> BufferedUarte<'d, U, T> {
                     tx: RingBuffer::new(tx_buffer),
                     tx_state: TxState::Idle,
                     tx_waker: WakerRegistration::new(),
-            }),
+                })
+            },
         }
     }
 

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;
@@ -48,14 +59,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.
-    ///
+    pub fn new(
-    /// # 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(
         _uarte: impl Unborrow<Target = T> + 'd,
         irq: impl Unborrow<Target = T::Interrupt> + 'd,
         rxd: impl Unborrow<Target = impl GpioPin> + 'd,

examples/nrf/src/bin/buffered_uart.rs

@@ -24,8 +24,7 @@ async fn main(_spawner: Spawner, p: Peripherals) {
 
     let irq = interrupt::take!(UARTE0_UART0);
     let mut state = State::new();
-    let u = unsafe {
+    let u = BufferedUarte::new(
-        BufferedUarte::new(
         &mut state,
         p.UARTE0,
         p.TIMER0,
@@ -39,8 +38,7 @@ async fn main(_spawner: Spawner, p: Peripherals) {
         config,
         &mut rx_buffer,
         &mut tx_buffer,
-        )
+    );
-    };
     pin_mut!(u);
 
     info!("uarte initialized!");

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 =
+    let mut uart = uarte::Uarte::new(p.UARTE0, irq, p.P0_08, p.P0_06, NoPin, NoPin, config);
-        unsafe { uarte::Uarte::new(p.UARTE0, irq, p.P0_08, p.P0_06, NoPin, NoPin, config) };
 
     info!("uarte initialized!");