Add docs concerning safety considerations when using async+DMA transfers

Author: jbeaurivage

hal/src/sercom/i2c.rs

@@ -181,9 +181,9 @@
 //! * High-speed mode is not supported.
 //! * 4-wire mode is not supported.
 //! * 32-bit extension mode is not supported (SAMx5x). If you need to transfer
-//!   slices, consider using the DMA methods instead . The <span class="stab
+//!   slices, consider using the DMA methods instead. <span class="stab
 //!   portability" title="Available on crate feature `dma`
-//!   only"><code>dma</code></span> Cargo feature must be enabled.
+//!   only"><code>dma</code></span>
 //!
 //! # Using I2C with DMA <span class="stab portability" title="Available on crate feature `dma` only"><code>dma</code></span>
 //!
@@ -243,6 +243,99 @@
 //! `I2cFuture` implements `AsRef<I2c>` and `AsMut<I2c>` so
 //! that it can be reconfigured using the regular [`I2c`] methods.
 //!
+//! ## Considerations when using `async` [`I2c`] with DMA <span class="stab portability" title="Available on crate feature `async` only"><code>async</code></span> <span class="stab portability" title="Available on crate feature `dma` only"><code>dma</code></span>
+//!
+//! * An [`I2c`] struct must be turned into an [`I2cFuture`] by calling
+//!   [`I2c::into_future`] before calling `with_dma_channel`. The DMA channel
+//!   itself must also be configured in async mode by using
+//!   [`DmaController::into_future`](crate::dmac::DmaController::into_future).
+//!   If a DMA channel is added to the [`I2c`] struct before it is turned into
+//!   an [`I2cFuture`], it will not be able to use DMA in async mode.
+//!
+//! ```
+//! // This will work
+//! let i2c = i2c.into_future().with_dma_channel(channel);
+//!
+//! // This won't
+//! let i2c = i2c.with_dma_channel(channel).into_future();
+//! ```
+//!
+//! ### Safety considerations
+//!
+//! In `async` mode, an I2C+DMA transfer does not require `'static` source and
+//! destination buffers. This, in theory, makes its use `unsafe`. However it is
+//! marked as safe for better ergonomics, and to enable the implementation of
+//! the [`embedded_hal_async::i2c::I2c`] trait.
+//!
+//! This means that, as an user, you **must** ensure that the [`Future`]s
+//! returned by the [`embedded_hal_async::i2c::I2c`] methods may never be
+//! forgotten through [`forget`] or by wrapping them with a [`ManuallyDrop`].
+//!
+//! The returned futures implement [`Drop`] and will automatically stop any
+//! ongoing transfers; this guarantees that the memory occupied by the
+//! now-dropped buffers may not be corrupted by running transfers.
+//!
+//! This means that using functions like [`futures::select_biased`] to implement
+//! timeouts is safe; transfers will be safely cancelled if the timeout expires.
+//!
+//! This also means that should you [`forget`] this [`Future`] after its
+//! first [`poll`] call, the transfer will keep running, ruining the
+//! now-reclaimed memory, as well as the rest of your day.
+//!
+//! * `await`ing is fine: the [`Future`] will run to completion.
+//! * Dropping an incomplete transfer is also fine. Dropping can happen, for
+//!   example, if the transfer doesn't complete before a timeout expires.
+//! * Dropping an incomplete transfer *without running its destructor* is
+//!   **unsound** and will trigger undefined behavior.
+//!
+//! ```ignore
+//! async fn always_ready() {}
+//!
+//! let mut buffer = [0x00; 10];
+//!
+//! // This is completely safe
+//! i2c.read(&mut buffer).await?;
+//!
+//! // This is also safe: we launch a transfer, which is then immediately cancelled
+//! futures::select_biased! {
+//!     _ = i2c.read(&mut buffer)?,
+//!     _ = always_ready(),
+//! }
+//!
+//! // This, while contrived, is also safe.
+//! {
+//!     use core::future::Future;
+//!
+//!     let future = i2c.read(&mut buffer);
+//!     futures::pin_mut!(future);
+//!     // Assume ctx is a `core::task::Context` given out by the executor.
+//!     // The future is polled, therefore starting the transfer
+//!     future.as_mut().poll(ctx);
+//!
+//!     // Future is dropped here - transfer is cancelled.
+//! }
+//!
+//! // DANGER: This is an example of undefined behavior
+//! {
+//!     use core::future::Future;
+//!     use core::ops::DerefMut;
+//!
+//!     let future = core::mem::ManuallyDrop::new(i2c.read(&mut buffer));
+//!     futures::pin_mut!(future);
+//!     // To actually make this example compile, we would need to wrap the returned
+//!     // future from `i2c.read()` in a newtype that implements Future, because we
+//!     // can't actually call as_mut() without being able to name the type we want
+//!     // to deref to.
+//!     let future_ref: &mut SomeNewTypeFuture = &mut future.as_mut();
+//!     future.as_mut().poll(ctx);
+//!
+//!     // Future is NOT dropped here - transfer is not cancelled, resulting un UB.
+//! }
+//! ```
+//!
+//! As you can see, unsoundness is relatively hard to come by - however, caution
+//! should still be exercised.
+//!
 //! [`enable`]: Config::enable
 //! [`disable`]: I2c::disable
 //! [`reconfigure`]: I2c::reconfigure
@@ -262,6 +355,10 @@
 //! [`i2c::Read`]: embedded_hal::blocking::i2c::Read
 //! [`i2c::WriteRead`]: embedded_hal::blocking::i2c::WriteRead
 //! [`async_hal`]: crate::async_hal
+//! [`forget`]: core::mem::forget
+//! [`ManuallyDrop`]: core::mem::ManuallyDrop
+//! [`Future`]: core::future::Future
+//! [`poll`]: core::future::Future::poll
 
 use atsamd_hal_macros::hal_module;
 
@@ -471,7 +568,8 @@ where
     D: crate::dmac::AnyChannel<Status = S>,
     S: crate::dmac::ReadyChannel,
 {
-    /// Reclaim the DMA channel. Any subsequent I2C operations will no longer use DMA.
+    /// Reclaim the DMA channel. Any subsequent I2C operations will no longer
+    /// use DMA.
     pub fn take_dma_channel(self) -> (I2c<C, crate::typelevel::NoneT>, D) {
         (
             I2c {

hal/src/sercom/i2c/async_api.rs

@@ -324,7 +324,8 @@ mod dma {
         C: AnyConfig,
         D: AnyChannel<Status = ReadyFuture>,
     {
-        /// Reclaim the DMA channel. Any subsequent I2C operations will no longer use DMA.
+        /// Reclaim the DMA channel. Any subsequent I2C operations will no
+        /// longer use DMA.
         pub fn take_dma_channel(self) -> (I2cFuture<C, crate::typelevel::NoneT>, D) {
             let (i2c, channel) = self.i2c.take_dma_channel();
             (I2cFuture { i2c }, channel)

hal/src/sercom/spi.rs

@@ -322,6 +322,99 @@
 //! `SpiFuture` implements `AsRef<Spi>` and `AsMut<Spi>` so
 //! that it can be reconfigured using the regular [`Spi`] methods.
 //!
+//! ## Considerations when using `async` [`Spi`] with DMA <span class="stab portability" title="Available on crate feature `async` only"><code>async</code></span> <span class="stab portability" title="Available on crate feature `dma` only"><code>dma</code></span>
+//!
+//! * An [`Spi`] struct must be turned into an [`SpiFuture`] by calling
+//!   [`Spi::into_future`] before calling `with_dma_channel`. The DMA channel
+//!   itself must also be configured in async mode by using
+//!   [`DmaController::into_future`](crate::dmac::DmaController::into_future).
+//!   If a DMA channel is added to the [`Spi`] struct before it is turned into
+//!   an [`SpiFuture`], it will not be able to use DMA in async mode.
+//!
+//! ```
+//! // This will work
+//! let spi = spi.into_future().with_dma_channels(rx_channel, tx_channel);
+//!
+//! // This won't
+//! let spi = spi.with_dma_channels(rx_channel, tx_channel).into_future();
+//! ```
+//!
+//! ### Safety considerations
+//!
+//! In `async` mode, an SPI+DMA transfer does not require `'static` source and
+//! destination buffers. This, in theory, makes its use `unsafe`. However it is
+//! marked as safe for better ergonomics, and to enable the implementation of
+//! the [`embedded_hal_async::spi::SpiBus`] trait.
+//!
+//! This means that, as an user, you **must** ensure that the [`Future`]s
+//! returned by the [`embedded_hal_async::spi::SpiBus`] methods may never be
+//! forgotten through [`forget`] or by wrapping them with a [`ManuallyDrop`].
+//!
+//! The returned futures implement [`Drop`] and will automatically stop any
+//! ongoing transfers; this guarantees that the memory occupied by the
+//! now-dropped buffers may not be corrupted by running transfers.
+//!
+//! This means that using functions like [`futures::select_biased`] to implement
+//! timeouts is safe; transfers will be safely cancelled if the timeout expires.
+//!
+//! This also means that should you [`forget`] this [`Future`] after its
+//! first [`poll`] call, the transfer will keep running, ruining the
+//! now-reclaimed memory, as well as the rest of your day.
+//!
+//! * `await`ing is fine: the [`Future`] will run to completion.
+//! * Dropping an incomplete transfer is also fine. Dropping can happen, for
+//!   example, if the transfer doesn't complete before a timeout expires.
+//! * Dropping an incomplete transfer *without running its destructor* is
+//!   **unsound** and will trigger undefined behavior.
+//!
+//! ```ignore
+//! async fn always_ready() {}
+//!
+//! let mut buffer = [0x00; 10];
+//!
+//! // This is completely safe
+//! spi.read(&mut buffer).await?;
+//!
+//! // This is also safe: we launch a transfer, which is then immediately cancelled
+//! futures::select_biased! {
+//!     _ = spi.read(&mut buffer)?,
+//!     _ = always_ready(),
+//! }
+//!
+//! // This, while contrived, is also safe.
+//! {
+//!     use core::future::Future;
+//!
+//!     let future = spi.read(&mut buffer);
+//!     futures::pin_mut!(future);
+//!     // Assume ctx is a `core::task::Context` given out by the executor.
+//!     // The future is polled, therefore starting the transfer
+//!     future.as_mut().poll(ctx);
+//!
+//!     // Future is dropped here - transfer is cancelled.
+//! }
+//!
+//! // DANGER: This is an example of undefined behavior
+//! {
+//!     use core::future::Future;
+//!     use core::ops::DerefMut;
+//!
+//!     let future = core::mem::ManuallyDrop::new(spi.read(&mut buffer));
+//!     futures::pin_mut!(future);
+//!     // To actually make this example compile, we would need to wrap the returned
+//!     // future from `i2c.read()` in a newtype that implements Future, because we
+//!     // can't actually call as_mut() without being able to name the type we want
+//!     // to deref to.
+//!     let future_ref: &mut SomeNewTypeFuture = &mut future.as_mut();
+//!     future.as_mut().poll(ctx);
+//!
+//!     // Future is NOT dropped here - transfer is not cancelled, resulting un UB.
+//! }
+//! ```
+//!
+//! As you can see, unsoundness is relatively hard to come by - however, caution
+//! should still be exercised.
+//!
 //! [`enable`]: Config::enable
 //! [`gpio`]: crate::gpio
 //! [`Pin`]: crate::gpio::pin::Pin
@@ -330,6 +423,10 @@
 //! [`embedded_hal::spi::SpiBus`]: crate::ehal::spi::SpiBus
 //! [`embedded_hal::spi::SpiDevice`]: crate::ehal::spi::SpiDevice
 //! [`async_hal`]: crate::async_hal
+//! [`forget`]: core::mem::forget
+//! [`ManuallyDrop`]: core::mem::ManuallyDrop
+//! [`Future`]: core::future::Future
+//! [`poll`]: core::future::Future::poll
 
 use core::marker::PhantomData;
 
@@ -1430,7 +1527,8 @@ where
     TxDma: crate::dmac::AnyChannel<Status = S>,
     S: crate::dmac::ReadyChannel,
 {
-    /// Reclaim the DMA channels. Any subsequent SPI transaction will not use DMA.
+    /// Reclaim the DMA channels. Any subsequent SPI transaction will not use
+    /// DMA.
     pub fn take_dma_channels(self) -> (Spi<C, D, NoneT, NoneT>, RxDma, TxDma) {
         (
             Spi {
@@ -1500,7 +1598,8 @@ where
     R: crate::dmac::AnyChannel<Status = S>,
     S: crate::dmac::ReadyChannel,
 {
-    /// Reclaim the Rx DMA channel. Any subsequent SPI transaction will not use DMA.
+    /// Reclaim the Rx DMA channel. Any subsequent SPI transaction will not use
+    /// DMA.
     #[cfg(feature = "dma")]
     pub fn take_rx_channel(self) -> (Spi<C, D, NoneT, T>, R) {
         (
@@ -1546,7 +1645,8 @@ where
     T: crate::dmac::AnyChannel<Status = S>,
     S: crate::dmac::ReadyChannel,
 {
-    /// Reclaim the DMA channel. Any subsequent SPI transaction will not use DMA.
+    /// Reclaim the DMA channel. Any subsequent SPI transaction will not use
+    /// DMA.
     pub fn take_tx_channel(self) -> (Spi<C, D, R, NoneT>, T) {
         (
             Spi {

hal/src/sercom/spi/async_api.rs

@@ -404,7 +404,8 @@ mod dma {
         RxDma: AnyChannel<Status = ReadyFuture>,
         TxDma: AnyChannel<Status = ReadyFuture>,
     {
-        /// Reclaim the DMA channels. Any subsequent SPI transaction will not use DMA.
+        /// Reclaim the DMA channels. Any subsequent SPI transaction will not
+        /// use DMA.
         pub fn take_dma_channels(self) -> (SpiFuture<C, D, NoneT, NoneT>, RxDma, TxDma) {
             let (spi, rx, tx) = self.spi.take_dma_channels();
             (SpiFuture { spi }, rx, tx)
@@ -417,7 +418,8 @@ mod dma {
         D: Receive,
         R: AnyChannel<Status = ReadyFuture>,
     {
-        /// Reclaim the Rx DMA channel. Any subsequent SPI transaction will not use DMA.
+        /// Reclaim the Rx DMA channel. Any subsequent SPI transaction will not
+        /// use DMA.
         pub fn take_rx_channel(self) -> (SpiFuture<C, D, NoneT, T>, R) {
             let (spi, channel) = self.spi.take_rx_channel();
             (SpiFuture { spi }, channel)
@@ -430,7 +432,8 @@ mod dma {
         D: Capability,
         T: AnyChannel<Status = ReadyFuture>,
     {
-        /// Reclaim the DMA channel. Any subsequent SPI transaction will not use DMA.
+        /// Reclaim the DMA channel. Any subsequent SPI transaction will not use
+        /// DMA.
         pub fn take_tx_channel(self) -> (SpiFuture<C, D, R, NoneT>, T) {
             let (spi, channel) = self.spi.take_tx_channel();
             (SpiFuture { spi }, channel)