Merge #237

237: DMA: Fix problems with DMASet and timer wrappers r=therealprof a=thalesfragoso

The initial idea of the DMASet trait was that people could __unsafely__ implement it themselves from outside of the HAL. Unfortunately, we missed the problem with the fact that tuples are considered a foreign type. This PR drops the tuple approach but still maintains the check that DMASet provides.

I also added a fix for #227.
Fixes #227 

This is also a possible solution to #226, but it takes a different approach, it still forces people (or the HAL) to make a new type if they want a "non-default" memory size (or address). This is more verbose than just dropping the associated type of `PeriAddress` in favor of a generic parameter, but, IMO, the verbosity can be an advantage here.

Marking as a draft because I still want to make sure this works as intended and that there isn't another corner case, I also would like the opinion of @albru123

Co-authored-by: Thales Fragoso <thales.fragosoz@gmail.com>

Author: bors[bot]

CHANGELOG.md

@@ -39,8 +39,15 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
 - Added support for hardware-based CRC32 functionality
 
 ### Fixed
+
 - Stability fixes related to SD card write
 - Fixed issue where timer generated a spurious interrupt after start
+- Allow implementations for DMASet from outside the crate [#237]
+- DMA: Make it possible to create the wrapper types for the timers [#237]
+- DMA: Fix some `compiler_fences` [#237]
+- DMA: Fix docs [#237]
+
+[#237]: https://github.com/stm32-rs/stm32f4xx-hal/pull/237
 
 ## [v0.8.3] - 2020-06-12
 

Cargo.toml

@@ -33,7 +33,7 @@ rand_core = "0.5.1"
 stm32f4 = "0.11"
 synopsys-usb-otg = { version = "0.2.0", features = ["cortex-m"], optional = true }
 sdio-host = { version = "0.5.0", optional = true }
-embedded-dma = "0.1.0"
+embedded-dma = "0.1.2"
 
 [dependencies.bare-metal]
 version = "0.2.5"

src/dma/mod.rs

@@ -864,9 +864,8 @@ where
     STREAM: Stream,
     CHANNEL: Channel,
     DIR: Direction,
-    PERIPHERAL: PeriAddress,
+    PERIPHERAL: PeriAddress + DMASet<STREAM, CHANNEL, DIR>,
     BUF: StaticWriteBuffer<Word = <PERIPHERAL as PeriAddress>::MemSize>,
-    (STREAM, CHANNEL, PERIPHERAL, DIR): DMASet,
 {
     /// Applies all fields in DmaConfig.
     fn apply_config(&mut self, config: config::DmaConfig) {
@@ -1008,8 +1007,9 @@ where
     }
 
     /// Changes the buffer and restarts or continues a double buffer transfer. This must be called
-    /// immediately after a transfer complete event. Returns the old buffer together with its
-    /// `CurrentBuffer`. If an error occurs, this method will return the new buffer with the error.
+    /// immediately after a transfer complete event if using double buffering, otherwise you might
+    /// lose data. Returns the old buffer together with its `CurrentBuffer`. If an error occurs,
+    /// this method will return the new buffer with the error.
     ///
     /// This method will clear the transfer complete flag on entry, it will also clear it again if
     /// an overrun occurs during its execution. Moreover, if an overrun occurs, the stream will be
@@ -1036,7 +1036,10 @@ where
             }
 
             if STREAM::current_buffer() == CurrentBuffer::DoubleBuffer {
+                // "Preceding reads and writes cannot be moved past subsequent writes"
+                compiler_fence(Ordering::Release);
                 self.stream.set_memory_address(new_buf_ptr as u32);
+
                 // Check if an overrun occurred, the buffer address won't be updated in that case
                 if self.stream.get_memory_address() != new_buf_ptr as u32 {
                     self.stream.clear_transfer_complete_interrupt();
@@ -1051,8 +1054,11 @@ where
                 // We always have a buffer, so unwrap can't fail
                 return Ok((old_buf.unwrap(), CurrentBuffer::FirstBuffer));
             } else {
+                // "Preceding reads and writes cannot be moved past subsequent writes"
+                compiler_fence(Ordering::Release);
                 self.stream
                     .set_memory_double_buffer_address(new_buf_ptr as u32);
+
                 // Check if an overrun occurred, the buffer address won't be updated in that case
                 if self.stream.get_memory_double_buffer_address() != new_buf_ptr as u32 {
                     self.stream.clear_transfer_complete_interrupt();
@@ -1081,9 +1087,6 @@ where
         self.stream.set_number_of_transfers(buf_len as u16);
         let old_buf = self.buf.replace(new_buf);
 
-        // "Preceding reads and writes cannot be moved past subsequent writes"
-        compiler_fence(Ordering::Release);
-
         unsafe {
             self.stream.enable();
         }
@@ -1155,15 +1158,15 @@ where
     }
 
     /// Changes the buffer and restarts or continues a double buffer transfer. This must be called
-    /// immediately after a transfer complete event. The closure must return `(BUF, T)` where `BUF`
-    /// is the new buffer to be used. This method can be called before the end of an ongoing
-    /// transfer only if not using double buffering, in that case, the current transfer will be
-    /// canceled and a new one will be started. A `NotReady` error will be returned if this method
-    /// is called before the end of a transfer while double buffering and the closure won't be
-    /// executed.
+    /// immediately after a transfer complete event if using double buffering, otherwise you might
+    /// lose data. The closure must return `(BUF, T)` where `BUF` is the new buffer to be used. This
+    /// method can be called before the end of an ongoing transfer only if not using double
+    /// buffering, in that case, the current transfer will be canceled and a new one will be
+    /// started. A `NotReady` error will be returned if this method is called before the end of a
+    /// transfer while double buffering and the closure won't be executed.
     ///
     /// # Panics
-    /// This method will panic when double buffering and one or both of the following conditions
+    /// This method will panic when double buffering if one or both of the following conditions
     /// happen:
     ///
     /// * The new buffer's length is smaller than the one used in the `init` method.
@@ -1173,10 +1176,6 @@ where
     ///
     /// Memory corruption might occur in the previous buffer, the one passed to the closure, if an
     /// overrun occurs in double buffering mode.
-    ///
-    /// # Panics
-    ///
-    /// This method will panic if an overrun is detected while double buffering.
     pub unsafe fn next_transfer_with<F, T>(&mut self, f: F) -> Result<T, DMAError<()>>
     where
         F: FnOnce(BUF, CurrentBuffer) -> (BUF, T),
@@ -1216,6 +1215,8 @@ where
             }
 
             if current_buffer == CurrentBuffer::DoubleBuffer {
+                // "Preceding reads and writes cannot be moved past subsequent writes"
+                compiler_fence(Ordering::Release);
                 self.stream.set_memory_address(new_buf_ptr as u32);
 
                 // Check again if an overrun occurred, the buffer address won't be updated in that
@@ -1230,8 +1231,11 @@ where
                 self.buf.replace(new_buf);
                 return Ok(r.1);
             } else {
+                // "Preceding reads and writes cannot be moved past subsequent writes"
+                compiler_fence(Ordering::Release);
                 self.stream
                     .set_memory_double_buffer_address(new_buf_ptr as u32);
+
                 if self.stream.get_memory_double_buffer_address() != new_buf_ptr as u32 {
                     panic!("Overrun");
                 }
@@ -1259,11 +1263,7 @@ where
         self.stream.set_number_of_transfers(buf_len as u16);
         self.buf.replace(new_buf);
 
-        // "Preceding reads and writes cannot be moved past subsequent writes"
-        compiler_fence(Ordering::Release);
-
         self.stream.enable();
-
         Ok(r.1)
     }
 }

src/dma/traits.rs

@@ -285,7 +285,7 @@ macro_rules! tim_channels {
     ($($name:ident),+ $(,)*) => {
         $(
             /// Wrapper type that indicates which register of the contained timer to use for DMA.
-            pub struct $name<T> (T);
+            pub struct $name<T> (pub T);
 
             impl<T> Deref for $name<T> {
                 type Target = T;
@@ -305,19 +305,19 @@ pub trait Channel: Bits<u8> {
     fn new() -> Self;
 }
 
-/// Trait to mark a set of Stream, Channel, Peripheral and Direction as correct together.
+/// Trait to mark a set of Stream, Channel and Direction for a Peripheral as correct together.
 ///
 /// # Safety
 ///
 /// Memory corruption might occur if this trait is implemented for an invalid combination.
-pub unsafe trait DMASet {}
+pub unsafe trait DMASet<STREAM, CHANNEL, DIRECTION> {}
 
 tim_channels!(CCR1, CCR2, CCR3, CCR4, DMAR, ARR);
 
 macro_rules! dma_map {
-    ($(($Stream:ty, $channel:ty, $Peripheral:ty, $dir:ty)),+ $(,)*) => {
+    ($(($Stream:ty, $Channel:ty, $Peripheral:ty, $Dir:ty)),+ $(,)*) => {
         $(
-            unsafe impl DMASet for ($Stream, $channel, $Peripheral, $dir) {}
+            unsafe impl DMASet<$Stream, $Channel, $Dir> for $Peripheral {}
         )+
     };
 }