From 11e6688a14adccb955f014749503a6a832b4b643 Mon Sep 17 00:00:00 2001 From: Ryan Summers Date: Mon, 23 Nov 2020 14:30:29 +0100 Subject: [PATCH] Refactoring timer channels to macros, adding safety notes --- Cargo.lock | 1 + Cargo.toml | 1 + src/adc.rs | 45 +++++++--- src/dac.rs | 38 +++++--- src/sampling_timer.rs | 198 ++++++++++++++++++------------------------ 5 files changed, 150 insertions(+), 133 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 3c96c4f..47e2086 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -474,6 +474,7 @@ dependencies = [ "nb 1.0.0", "panic-halt", "panic-semihosting", + "paste", "serde", "serde-json-core", "smoltcp", diff --git a/Cargo.toml b/Cargo.toml index 429fe85..44f2d02 100644 --- a/Cargo.toml +++ b/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" diff --git a/src/adc.rs b/src/adc.rs index 55ed2a8..9b2c53b 100644 --- a/src/adc.rs +++ b/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 { - Self {} + pub fn new(_channel: sampling_timer::tim2::Channel1) -> 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 for SPI2 { /// SPI2 is configured to operate using 16-bit transfer words. type MemSize = u16; @@ -59,6 +64,8 @@ unsafe impl TargetAddress 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() }; ®s.txdr as *const _ as u32 } @@ -66,13 +73,18 @@ unsafe impl TargetAddress 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 { - Self {} + pub fn new(_channel: sampling_timer::tim2::Channel2) -> 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 for SPI3 { /// SPI3 is configured to operate using 16-bit transfer words. type MemSize = u16; @@ -83,6 +95,8 @@ unsafe impl TargetAddress 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() }; ®s.txdr as *const _ as u32 } @@ -144,7 +158,7 @@ impl Adc0Input { spi: hal::spi::Spi, trigger_stream: hal::dma::dma::Stream0, data_stream: hal::dma::dma::Stream1, - 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, trigger_stream: hal::dma::dma::Stream2, data_stream: hal::dma::dma::Stream3, - 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, diff --git a/src/dac.rs b/src/dac.rs index 8057a5a..d7032db 100644 --- a/src/dac.rs +++ b/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 { - Self {} + pub fn new(_channel: sampling_timer::tim2::Channel3) -> 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 for SPI4 { /// SPI2 is configured to operate using 16-bit transfer words. type MemSize = u16; @@ -41,19 +46,25 @@ unsafe impl TargetAddress 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() }; ®s.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 { - Self {} + pub fn new(_channel: sampling_timer::tim2::Channel4) -> 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 for SPI5 { /// SPI5 is configured to operate using 16-bit transfer words. type MemSize = u16; @@ -63,6 +74,7 @@ unsafe impl TargetAddress 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() }; ®s.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, transfer: Transfer< hal::dma::dma::Stream4, @@ -118,7 +131,7 @@ impl Dac0Output { pub fn new( spi: hal::spi::Spi, stream: hal::dma::dma::Stream4, - 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, transfer: Transfer< hal::dma::dma::Stream5, @@ -209,7 +225,7 @@ impl Dac1Output { pub fn new( spi: hal::spi::Spi, stream: hal::dma::dma::Stream5, - 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, diff --git a/src/sampling_timer.rs b/src/sampling_timer.rs index 7b3b557..4755886 100644 --- a/src/sampling_timer.rs +++ b/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, - channels: Option, + channels: Option, } 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. -/// -/// # Note -/// This should not be instantiated directly. -pub struct TimerChannels { - pub ch1: Timer2Channel1, - pub ch2: Timer2Channel2, - pub ch3: Timer2Channel3, - pub ch4: Timer2Channel4, +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(unsafe): This function must only be called once. Once constructed, the + /// constructee guarantees to never modify the timer channel. + unsafe fn new() -> Self { + Self {} + } + + /// Allow CH4 to generate DMA requests. + 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) }); + } + } + } + }; } -impl TimerChannels { - fn new() -> Self { - Self { - ch1: Timer2Channel1 {}, - ch2: Timer2Channel2 {}, - ch3: Timer2Channel3 {}, - ch4: Timer2Channel4 {}, +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: Channel1::new(), + ch2: Channel2::new(), + ch3: Channel3::new(), + ch4: Channel4::new(), + } } } -} - -/// Representation of CH1 of TIM2. -pub struct Timer2Channel1 {} - -impl Timer2Channel1 { - /// Allow CH1 to generate DMA requests. - pub fn listen_dma(&self) { - let regs = unsafe { &*hal::stm32::TIM2::ptr() }; - regs.dier.modify(|_, w| w.cc1de().set_bit()); - } - - /// Operate CH1 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.ccr1.write(|w| w.ccr().bits(value)); - regs.ccmr1_output() - .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) }); - } + + timer_channel!( + Channel1, + hal::stm32::TIM2, + (cc1de, ccr1, ccmr1_output, cc1s) + ); + timer_channel!( + Channel2, + hal::stm32::TIM2, + (cc2de, ccr2, ccmr1_output, cc1s) + ); + timer_channel!( + Channel3, + hal::stm32::TIM2, + (cc3de, ccr3, ccmr2_output, cc3s) + ); + timer_channel!( + Channel4, + hal::stm32::TIM2, + (cc4de, ccr4, ccmr2_output, cc4s) + ); }