Refactoring timer channels to macros, adding safety notes

Cargo.lock

@@ -474,6 +474,7 @@ dependencies = [
  "nb 1.0.0",
  "panic-halt",
  "panic-semihosting",
+ "paste",
  "serde",
  "serde-json-core",
  "smoltcp",

Cargo.toml

@@ -40,6 +40,7 @@ embedded-hal = "0.2.4"
 nb = "1.0.0"
 asm-delay = "0.9.0"
 enum-iterator = "0.6.0"
+paste = "1"
 
 [dependencies.mcp23017]
 git = "https://github.com/mrd0ll4r/mcp23017.git"

src/adc.rs

@@ -42,13 +42,18 @@ static mut ADC1_BUF1: [u16; SAMPLE_BUFFER_SIZE] = [0; SAMPLE_BUFFER_SIZE];
 
 /// SPI2 is used as a ZST (zero-sized type) for indicating a DMA transfer into the SPI2 TX FIFO
 /// whenever the tim2 update dma request occurs.
-struct SPI2 {}
+struct SPI2 {
+    _channel: sampling_timer::tim2::Channel1,
+}
 impl SPI2 {
-    pub fn new() -> Self {
+    pub fn new(_channel: sampling_timer::tim2::Channel1) -> Self {
-        Self {}
+        Self { _channel }
     }
 }
 
+// Note(unsafe): This structure is only safe to instantiate once. The DMA request is hard-coded and
+// may only be used if ownership of the timer2 channel 1 compare channel is assured, which is
+// ensured by maintaining ownership of the channel.
 unsafe impl TargetAddress<MemoryToPeripheral> for SPI2 {
     /// SPI2 is configured to operate using 16-bit transfer words.
     type MemSize = u16;
@@ -59,6 +64,8 @@ unsafe impl TargetAddress<MemoryToPeripheral> for SPI2 {
     /// Whenever the DMA request occurs, it should write into SPI2's TX FIFO to start a DMA
     /// transfer.
     fn address(&self) -> u32 {
+        // Note(unsafe): It is assumed that SPI2 is owned by another DMA transfer and this DMA is
+        // only used for the transmit-half of DMA.
         let regs = unsafe { &*hal::stm32::SPI2::ptr() };
         &regs.txdr as *const _ as u32
     }
@@ -66,13 +73,18 @@ unsafe impl TargetAddress<MemoryToPeripheral> for SPI2 {
 
 /// SPI3 is used as a ZST (zero-sized type) for indicating a DMA transfer into the SPI3 TX FIFO
 /// whenever the tim2 update dma request occurs.
-struct SPI3 {}
+struct SPI3 {
+    _channel: sampling_timer::tim2::Channel2,
+}
 impl SPI3 {
-    pub fn new() -> Self {
+    pub fn new(_channel: sampling_timer::tim2::Channel2) -> Self {
-        Self {}
+        Self { _channel }
     }
 }
 
+// Note(unsafe): This structure is only safe to instantiate once. The DMA request is hard-coded and
+// may only be used if ownership of the timer2 channel 2 compare channel is assured, which is
+// ensured by maintaining ownership of the channel.
 unsafe impl TargetAddress<MemoryToPeripheral> for SPI3 {
     /// SPI3 is configured to operate using 16-bit transfer words.
     type MemSize = u16;
@@ -83,6 +95,8 @@ unsafe impl TargetAddress<MemoryToPeripheral> for SPI3 {
     /// Whenever the DMA request occurs, it should write into SPI3's TX FIFO to start a DMA
     /// transfer.
     fn address(&self) -> u32 {
+        // Note(unsafe): It is assumed that SPI3 is owned by another DMA transfer and this DMA is
+        // only used for the transmit-half of DMA.
         let regs = unsafe { &*hal::stm32::SPI3::ptr() };
         &regs.txdr as *const _ as u32
     }
@@ -144,7 +158,7 @@ impl Adc0Input {
         spi: hal::spi::Spi<hal::stm32::SPI2, hal::spi::Enabled, u16>,
         trigger_stream: hal::dma::dma::Stream0<hal::stm32::DMA1>,
         data_stream: hal::dma::dma::Stream1<hal::stm32::DMA1>,
-        trigger_channel: sampling_timer::Timer2Channel1,
+        trigger_channel: sampling_timer::tim2::Channel1,
     ) -> Self {
         // Generate DMA events when an output compare of the timer hitting zero (timer roll over)
         // occurs.
@@ -164,7 +178,10 @@ impl Adc0Input {
         let mut trigger_transfer: Transfer<_, _, MemoryToPeripheral, _> =
             Transfer::init(
                 trigger_stream,
-                SPI2::new(),
+                SPI2::new(trigger_channel),
+                // Note(unsafe): Because this is a Memory->Peripheral transfer, this data is never
+                // actually modified. It technically only needs to be immutably borrowed, but the
+                // current HAL API only supports mutable borrows.
                 unsafe { &mut SPI_START },
                 None,
                 trigger_config,
@@ -192,6 +209,8 @@ impl Adc0Input {
             Transfer::init(
                 data_stream,
                 spi,
+                // Note(unsafe): The ADC0_BUF0 is "owned" by this peripheral. It shall not be used
+                // anywhere else in the module.
                 unsafe { &mut ADC0_BUF0 },
                 None,
                 data_config,
@@ -210,6 +229,8 @@ impl Adc0Input {
         trigger_transfer.start(|_| {});
 
         Self {
+            // Note(unsafe): The ADC0_BUF1 is "owned" by this peripheral. It shall not be used
+            // anywhere else in the module.
             next_buffer: unsafe { Some(&mut ADC0_BUF1) },
             transfer: data_transfer,
             _trigger_transfer: trigger_transfer,
@@ -265,7 +286,7 @@ impl Adc1Input {
         spi: hal::spi::Spi<hal::stm32::SPI3, hal::spi::Enabled, u16>,
         trigger_stream: hal::dma::dma::Stream2<hal::stm32::DMA1>,
         data_stream: hal::dma::dma::Stream3<hal::stm32::DMA1>,
-        trigger_channel: sampling_timer::Timer2Channel2,
+        trigger_channel: sampling_timer::tim2::Channel2,
     ) -> Self {
         // Generate DMA events when an output compare of the timer hitting zero (timer roll over)
         // occurs.
@@ -285,7 +306,7 @@ impl Adc1Input {
         let mut trigger_transfer: Transfer<_, _, MemoryToPeripheral, _> =
             Transfer::init(
                 trigger_stream,
-                SPI3::new(),
+                SPI3::new(trigger_channel),
                 unsafe { &mut SPI_START },
                 None,
                 trigger_config,
@@ -314,6 +335,8 @@ impl Adc1Input {
             Transfer::init(
                 data_stream,
                 spi,
+                // Note(unsafe): The ADC1_BUF0 is "owned" by this peripheral. It shall not be used
+                // anywhere else in the module.
                 unsafe { &mut ADC1_BUF0 },
                 None,
                 data_config,
@@ -332,6 +355,8 @@ impl Adc1Input {
         trigger_transfer.start(|_| {});
 
         Self {
+            // Note(unsafe): The ADC1_BUF1 is "owned" by this peripheral. It shall not be used
+            // anywhere else in the module.
             next_buffer: unsafe { Some(&mut ADC1_BUF1) },
             transfer: data_transfer,
             _trigger_transfer: trigger_transfer,

src/dac.rs

@@ -25,13 +25,18 @@ static mut DAC1_BUF0: [u16; SAMPLE_BUFFER_SIZE] = [0; SAMPLE_BUFFER_SIZE];
 static mut DAC1_BUF1: [u16; SAMPLE_BUFFER_SIZE] = [0; SAMPLE_BUFFER_SIZE];
 
 /// SPI4 is used as a ZST (zero-sized type) for indicating a DMA transfer into the SPI4 TX FIFO
-struct SPI4 {}
+struct SPI4 {
+    _channel: sampling_timer::tim2::Channel3,
+}
 impl SPI4 {
-    pub fn new() -> Self {
+    pub fn new(_channel: sampling_timer::tim2::Channel3) -> Self {
-        Self {}
+        Self { _channel }
     }
 }
 
+// Note(unsafe): This is safe because the DMA request line is logically owned by this module.
+// Additionally, it is only safe if the SPI TX functionality is never used, which is managed by the
+// Dac0Output.
 unsafe impl TargetAddress<MemoryToPeripheral> for SPI4 {
     /// SPI2 is configured to operate using 16-bit transfer words.
     type MemSize = u16;
@@ -41,19 +46,25 @@ unsafe impl TargetAddress<MemoryToPeripheral> for SPI4 {
 
     /// Whenever the DMA request occurs, it should write into SPI4's TX FIFO.
     fn address(&self) -> u32 {
+        // Note(unsafe): This is only safe as long as no other users write to the SPI TX FIFO.
         let regs = unsafe { &*hal::stm32::SPI4::ptr() };
         &regs.txdr as *const _ as u32
     }
 }
 
 /// SPI5 is used as a ZST (zero-sized type) for indicating a DMA transfer into the SPI5 TX FIFO
-struct SPI5 {}
+struct SPI5 {
+    _channel: sampling_timer::tim2::Channel4,
+}
 impl SPI5 {
-    pub fn new() -> Self {
+    pub fn new(_channel: sampling_timer::tim2::Channel4) -> Self {
-        Self {}
+        Self { _channel }
     }
 }
 
+// Note(unsafe): This is safe because the DMA request line is logically owned by this module.
+// Additionally, it is only safe if the SPI TX functionality is never used, which is managed by the
+// Dac1Output.
 unsafe impl TargetAddress<MemoryToPeripheral> for SPI5 {
     /// SPI5 is configured to operate using 16-bit transfer words.
     type MemSize = u16;
@@ -63,6 +74,7 @@ unsafe impl TargetAddress<MemoryToPeripheral> for SPI5 {
 
     /// Whenever the DMA request occurs, it should write into SPI5's TX FIFO
     fn address(&self) -> u32 {
+        // Note(unsafe): This is only safe as long as no other users write to the SPI TX FIFO.
         let regs = unsafe { &*hal::stm32::SPI5::ptr() };
         &regs.txdr as *const _ as u32
     }
@@ -98,6 +110,7 @@ impl DacOutputs {
 /// Represents data associated with DAC0.
 pub struct Dac0Output {
     next_buffer: Option<&'static mut [u16; SAMPLE_BUFFER_SIZE]>,
+    // Note: SPI TX functionality may not be used from this structure to ensure safety with DMA.
     _spi: hal::spi::Spi<hal::stm32::SPI4, hal::spi::Disabled, u16>,
     transfer: Transfer<
         hal::dma::dma::Stream4<hal::stm32::DMA1>,
@@ -118,7 +131,7 @@ impl Dac0Output {
     pub fn new(
         spi: hal::spi::Spi<hal::stm32::SPI4, hal::spi::Enabled, u16>,
         stream: hal::dma::dma::Stream4<hal::stm32::DMA1>,
-        trigger_channel: sampling_timer::Timer2Channel3,
+        trigger_channel: sampling_timer::tim2::Channel3,
     ) -> Self {
         // Generate DMA events when an output compare of the timer hitting zero (timer roll over)
         // occurs.
@@ -133,7 +146,8 @@ impl Dac0Output {
         // Construct the trigger stream to write from memory to the peripheral.
         let transfer: Transfer<_, _, MemoryToPeripheral, _> = Transfer::init(
             stream,
-            SPI4::new(),
+            SPI4::new(trigger_channel),
+            // Note(unsafe): This buffer is only used once and provided for the DMA transfer.
             unsafe { &mut DAC0_BUF0 },
             None,
             trigger_config,
@@ -153,6 +167,7 @@ impl Dac0Output {
 
         Self {
             transfer,
+            // Note(unsafe): This buffer is only used once and provided for the next DMA transfer.
             next_buffer: unsafe { Some(&mut DAC0_BUF1) },
             _spi: spi,
             first_transfer: true,
@@ -189,6 +204,7 @@ impl Dac0Output {
 /// Represents the data output stream from DAC1.
 pub struct Dac1Output {
     next_buffer: Option<&'static mut [u16; SAMPLE_BUFFER_SIZE]>,
+    // Note: SPI TX functionality may not be used from this structure to ensure safety with DMA.
     _spi: hal::spi::Spi<hal::stm32::SPI5, hal::spi::Disabled, u16>,
     transfer: Transfer<
         hal::dma::dma::Stream5<hal::stm32::DMA1>,
@@ -209,7 +225,7 @@ impl Dac1Output {
     pub fn new(
         spi: hal::spi::Spi<hal::stm32::SPI5, hal::spi::Enabled, u16>,
         stream: hal::dma::dma::Stream5<hal::stm32::DMA1>,
-        trigger_channel: sampling_timer::Timer2Channel4,
+        trigger_channel: sampling_timer::tim2::Channel4,
     ) -> Self {
         // Generate DMA events when an output compare of the timer hitting zero (timer roll over)
         // occurs.
@@ -225,7 +241,8 @@ impl Dac1Output {
         // Construct the stream to write from memory to the peripheral.
         let transfer: Transfer<_, _, MemoryToPeripheral, _> = Transfer::init(
             stream,
-            SPI5::new(),
+            SPI5::new(trigger_channel),
+            // Note(unsafe): This buffer is only used once and provided to the transfer.
             unsafe { &mut DAC1_BUF0 },
             None,
             trigger_config,
@@ -244,6 +261,7 @@ impl Dac1Output {
         spi.inner().cr1.modify(|_, w| w.cstart().started());
 
         Self {
+            // Note(unsafe): This buffer is only used once and provided for the next DMA transfer.
             next_buffer: unsafe { Some(&mut DAC1_BUF1) },
             transfer,
             _spi: spi,

src/sampling_timer.rs

@@ -1,12 +1,10 @@
 ///! The sampling timer is used for managing ADC sampling and external reference timestamping.
 use super::hal;
 
-pub use hal::stm32::tim2::ccmr2_input::CC4S_A;
-
 /// The timer used for managing ADC sampling.
 pub struct SamplingTimer {
     timer: hal::timer::Timer<hal::stm32::TIM2>,
-    channels: Option<TimerChannels>,
+    channels: Option<tim2::Channels>,
 }
 
 impl SamplingTimer {
@@ -16,12 +14,17 @@ impl SamplingTimer {
 
         Self {
             timer,
-            channels: Some(TimerChannels::new()),
+            // Note(unsafe): Once these channels are taken, we guarantee that we do not modify any
+            // of the underlying timer channel registers, as ownership of the channels is now
+            // provided through the associated channel structures. We additionally guarantee this
+            // can only be called once because there is only one Timer2 and this resource takes
+            // ownership of it once instantiated.
+            channels: unsafe { Some(tim2::Channels::new()) },
         }
     }
 
     /// Get the timer capture/compare channels.
-    pub fn channels(&mut self) -> TimerChannels {
+    pub fn channels(&mut self) -> tim2::Channels {
         self.channels.take().unwrap()
     }
 
@@ -32,116 +35,85 @@ impl SamplingTimer {
     }
 }
 
-/// The capture/compare channels for the sampling timer.
+macro_rules! timer_channel {
+    ($name:ident, $TY:ty, ($ccxde:expr, $ccrx:expr, $ccmrx_output:expr, $ccxs:expr)) => {
+        pub struct $name {}
+
+        paste::paste! {
+            impl $name {
+                /// Construct a new timer channel.
                 ///
-/// # Note
+                /// Note(unsafe): This function must only be called once. Once constructed, the
-/// This should not be instantiated directly.
+                /// constructee guarantees to never modify the timer channel.
-pub struct TimerChannels {
+                unsafe fn new() -> Self {
-    pub ch1: Timer2Channel1,
+                    Self {}
-    pub ch2: Timer2Channel2,
-    pub ch3: Timer2Channel3,
-    pub ch4: Timer2Channel4,
                 }
 
-impl TimerChannels {
+                /// Allow CH4 to generate DMA requests.
-    fn new() -> Self {
+                pub fn listen_dma(&self) {
+                    let regs = unsafe { &*<$TY>::ptr() };
+                    regs.dier.modify(|_, w| w.[< $ccxde >]().set_bit());
+                }
+
+                /// Operate CH2 as an output-compare.
+                ///
+                /// # Args
+                /// * `value` - The value to compare the sampling timer's counter against.
+                pub fn to_output_compare(&self, value: u32) {
+                    let regs = unsafe { &*<$TY>::ptr() };
+                    assert!(value <= regs.arr.read().bits());
+                    regs.[< $ccrx >].write(|w| w.ccr().bits(value));
+                    regs.[< $ccmrx_output >]()
+                        .modify(|_, w| unsafe { w.[< $ccxs >]().bits(0) });
+                }
+            }
+        }
+    };
+}
+
+pub mod tim2 {
+    use stm32h7xx_hal as hal;
+
+    /// The channels representing the timer.
+    pub struct Channels {
+        pub ch1: Channel1,
+        pub ch2: Channel2,
+        pub ch3: Channel3,
+        pub ch4: Channel4,
+    }
+
+    impl Channels {
+        /// Construct a new set of channels.
+        ///
+        /// Note(unsafe): This is only safe to call once.
+        pub unsafe fn new() -> Self {
             Self {
-            ch1: Timer2Channel1 {},
+                ch1: Channel1::new(),
-            ch2: Timer2Channel2 {},
+                ch2: Channel2::new(),
-            ch3: Timer2Channel3 {},
+                ch3: Channel3::new(),
-            ch4: Timer2Channel4 {},
+                ch4: Channel4::new(),
             }
         }
     }
 
-/// Representation of CH1 of TIM2.
+    timer_channel!(
-pub struct Timer2Channel1 {}
+        Channel1,
-
+        hal::stm32::TIM2,
-impl Timer2Channel1 {
+        (cc1de, ccr1, ccmr1_output, cc1s)
-    /// Allow CH1 to generate DMA requests.
+    );
-    pub fn listen_dma(&self) {
+    timer_channel!(
-        let regs = unsafe { &*hal::stm32::TIM2::ptr() };
+        Channel2,
-        regs.dier.modify(|_, w| w.cc1de().set_bit());
+        hal::stm32::TIM2,
-    }
+        (cc2de, ccr2, ccmr1_output, cc1s)
-
+    );
-    /// Operate CH1 as an output-compare.
+    timer_channel!(
-    ///
+        Channel3,
-    /// # Args
+        hal::stm32::TIM2,
-    /// * `value` - The value to compare the sampling timer's counter against.
+        (cc3de, ccr3, ccmr2_output, cc3s)
-    pub fn to_output_compare(&self, value: u32) {
+    );
-        let regs = unsafe { &*hal::stm32::TIM2::ptr() };
+    timer_channel!(
-        assert!(value <= regs.arr.read().bits());
+        Channel4,
-        regs.ccr1.write(|w| w.ccr().bits(value));
+        hal::stm32::TIM2,
-        regs.ccmr1_output()
+        (cc4de, ccr4, ccmr2_output, cc4s)
-            .modify(|_, w| unsafe { w.cc1s().bits(0) });
+    );
-    }
-}
-
-/// Representation of CH2 of TIM2.
-pub struct Timer2Channel2 {}
-
-impl Timer2Channel2 {
-    /// Allow CH2 to generate DMA requests.
-    pub fn listen_dma(&self) {
-        let regs = unsafe { &*hal::stm32::TIM2::ptr() };
-        regs.dier.modify(|_, w| w.cc2de().set_bit());
-    }
-
-    /// Operate CH2 as an output-compare.
-    ///
-    /// # Args
-    /// * `value` - The value to compare the sampling timer's counter against.
-    pub fn to_output_compare(&self, value: u32) {
-        let regs = unsafe { &*hal::stm32::TIM2::ptr() };
-        assert!(value <= regs.arr.read().bits());
-        regs.ccr2.write(|w| w.ccr().bits(value));
-        regs.ccmr1_output()
-            .modify(|_, w| unsafe { w.cc2s().bits(0) });
-    }
-}
-
-/// Representation of CH3 of TIM2.
-pub struct Timer2Channel3 {}
-
-impl Timer2Channel3 {
-    /// Allow CH4 to generate DMA requests.
-    pub fn listen_dma(&self) {
-        let regs = unsafe { &*hal::stm32::TIM2::ptr() };
-        regs.dier.modify(|_, w| w.cc3de().set_bit());
-    }
-
-    /// Operate CH2 as an output-compare.
-    ///
-    /// # Args
-    /// * `value` - The value to compare the sampling timer's counter against.
-    pub fn to_output_compare(&self, value: u32) {
-        let regs = unsafe { &*hal::stm32::TIM2::ptr() };
-        assert!(value <= regs.arr.read().bits());
-        regs.ccr3.write(|w| w.ccr().bits(value));
-        regs.ccmr2_output()
-            .modify(|_, w| unsafe { w.cc3s().bits(0) });
-    }
-}
-
-/// Representation of CH4 of TIM2.
-pub struct Timer2Channel4 {}
-
-impl Timer2Channel4 {
-    /// Allow CH4 to generate DMA requests.
-    pub fn listen_dma(&self) {
-        let regs = unsafe { &*hal::stm32::TIM2::ptr() };
-        regs.dier.modify(|_, w| w.cc4de().set_bit());
-    }
-
-    /// Operate CH2 as an output-compare.
-    ///
-    /// # Args
-    /// * `value` - The value to compare the sampling timer's counter against.
-    pub fn to_output_compare(&self, value: u32) {
-        let regs = unsafe { &*hal::stm32::TIM2::ptr() };
-        assert!(value <= regs.arr.read().bits());
-        regs.ccr4.write(|w| w.ccr().bits(value));
-        regs.ccmr2_output()
-            .modify(|_, w| unsafe { w.cc4s().bits(0) });
-    }
 }