Merge #648 #656

648: Fix nRF Saadc continuous sampling r=Dirbaio a=huntc

Starting the sampling task prior to starting the SAADC peripheral can lead to unexpected buffer behaviour with multiple channels. We now provide an init callback at the point where the SAADC has started for the first time. This callback can be used to kick off sampling via PPI.

We also need to trigger the SAADC to start sampling the next buffer when the previous one is ended so that we do not drop samples - the major benefit of double buffering.

Given these additional tasks, we now simplify the API by passing in the TIMER and two PPI channels.

As a bonus, we provide an async `calibrate` method as it is recommended to use before starting up the sampling.

The example has been updated to illustrate these new features along with the simplified API.

The changes here have been tested on my nRF52840-DK.

656: stm32: Refactor DMA interrupts r=Dirbaio a=GrantM11235

Previously, every dma interrupt handler called the same `on_irq`
function which had to check the state of every dma channel.

Now, each dma interrupt handler only calls an `on_irq` method for its
corresponding channel or channels.

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

embassy-nrf/src/saadc.rs

@@ -10,7 +10,8 @@ use embassy_hal_common::unborrow;
 use futures::future::poll_fn;
 
 use crate::interrupt;
-use crate::ppi::Task;
+use crate::ppi::{ConfigurableChannel, Event, Ppi, Task};
+use crate::timer::{Frequency, Instance as TimerInstance, Timer};
 use crate::{pac, peripherals};
 
 use pac::{saadc, SAADC};
@@ -207,6 +208,11 @@ impl<'d, const N: usize> Saadc<'d, N> {
     fn on_interrupt(_ctx: *mut ()) {
         let r = Self::regs();
 
+        if r.events_calibratedone.read().bits() != 0 {
+            r.intenclr.write(|w| w.calibratedone().clear());
+            WAKER.wake();
+        }
+
         if r.events_end.read().bits() != 0 {
             r.intenclr.write(|w| w.end().clear());
             WAKER.wake();
@@ -222,6 +228,35 @@ impl<'d, const N: usize> Saadc<'d, N> {
         unsafe { &*SAADC::ptr() }
     }
 
+    /// Perform SAADC calibration. Completes when done.
+    pub async fn calibrate(&self) {
+        let r = Self::regs();
+
+        // Reset and enable the end event
+        r.events_calibratedone.reset();
+        r.intenset.write(|w| w.calibratedone().set());
+
+        // Order is important
+        compiler_fence(Ordering::SeqCst);
+
+        r.tasks_calibrateoffset.write(|w| unsafe { w.bits(1) });
+
+        // Wait for 'calibratedone' event.
+        poll_fn(|cx| {
+            let r = Self::regs();
+
+            WAKER.register(cx.waker());
+
+            if r.events_calibratedone.read().bits() != 0 {
+                r.events_calibratedone.reset();
+                return Poll::Ready(());
+            }
+
+            Poll::Pending
+        })
+        .await;
+    }
+
     /// One shot sampling. The buffer must be the same size as the number of channels configured.
     pub async fn sample(&mut self, buf: &mut [i16; N]) {
         let r = Self::regs();
@@ -263,29 +298,77 @@ impl<'d, const N: usize> Saadc<'d, N> {
 
     /// Continuous sampling with double buffers.
     ///
-    /// A task-driven approach to driving TASK_SAMPLE is expected. With a task
+    /// A TIMER and two PPI peripherals are passed in so that precise sampling
-    /// driven approach, multiple channels can be used.
+    /// can be attained. The sampling interval is expressed by selecting a
+    /// timer clock frequency to use along with a counter threshold to be reached.
+    /// For example, 1KHz can be achieved using a frequency of 1MHz and a counter
+    /// threshold of 1000.
     ///
     /// A sampler closure is provided that receives the buffer of samples, noting
     /// that the size of this buffer can be less than the original buffer's size.
     /// A command is return from the closure that indicates whether the sampling
     /// should continue or stop.
-    pub async fn run_task_sampler<S, const N0: usize>(
+    ///
+    /// NOTE: The time spent within the callback supplied should not exceed the time
+    /// taken to acquire the samples into a single buffer. You should measure the
+    /// time taken by the callback and set the sample buffer size accordingly.
+    /// Exceeding this time can lead to samples becoming dropped.
+    pub async fn run_task_sampler<S, T: TimerInstance, const N0: usize>(
         &mut self,
+        timer: &mut T,
+        ppi_ch1: &mut impl ConfigurableChannel,
+        ppi_ch2: &mut impl ConfigurableChannel,
+        frequency: Frequency,
+        sample_counter: u32,
         bufs: &mut [[[i16; N]; N0]; 2],
         sampler: S,
     ) where
         S: FnMut(&[[i16; N]]) -> SamplerState,
     {
-        self.run_sampler(bufs, None, sampler).await;
+        let r = Self::regs();
+
+        // We want the task start to effectively short with the last one ending so
+        // we don't miss any samples. It'd be great for the SAADC to offer a SHORTS
+        // register instead, but it doesn't, so we must use PPI.
+        let mut start_ppi = Ppi::new_one_to_one(
+            ppi_ch1,
+            Event::from_reg(&r.events_end),
+            Task::from_reg(&r.tasks_start),
+        );
+        start_ppi.enable();
+
+        let mut timer = Timer::new(timer);
+        timer.set_frequency(frequency);
+        timer.cc(0).write(sample_counter);
+        timer.cc(0).short_compare_clear();
+
+        let mut sample_ppi = Ppi::new_one_to_one(
+            ppi_ch2,
+            timer.cc(0).event_compare(),
+            Task::from_reg(&r.tasks_sample),
+        );
+
+        timer.start();
+
+        self.run_sampler(
+            bufs,
+            None,
+            || {
+                sample_ppi.enable();
+            },
+            sampler,
+        )
+        .await;
     }
 
-    async fn run_sampler<S, const N0: usize>(
+    async fn run_sampler<I, S, const N0: usize>(
         &mut self,
         bufs: &mut [[[i16; N]; N0]; 2],
         sample_rate_divisor: Option<u16>,
+        mut init: I,
         mut sampler: S,
     ) where
+        I: FnMut(),
         S: FnMut(&[[i16; N]]) -> SamplerState,
     {
         let r = Self::regs();
@@ -330,6 +413,8 @@ impl<'d, const N: usize> Saadc<'d, N> {
 
         r.tasks_start.write(|w| unsafe { w.bits(1) });
 
+        let mut inited = false;
+
         let mut current_buffer = 0;
 
         // Wait for events and complete when the sampler indicates it has had enough.
@@ -347,7 +432,6 @@ impl<'d, const N: usize> Saadc<'d, N> {
                 if sampler(&bufs[current_buffer]) == SamplerState::Sampled {
                     let next_buffer = 1 - current_buffer;
                     current_buffer = next_buffer;
-                    r.tasks_start.write(|w| unsafe { w.bits(1) });
                 } else {
                     return Poll::Ready(());
                 };
@@ -357,6 +441,11 @@ impl<'d, const N: usize> Saadc<'d, N> {
                 r.events_started.reset();
                 r.intenset.write(|w| w.started().set());
 
+                if !inited {
+                    init();
+                    inited = true;
+                }
+
                 let next_buffer = 1 - current_buffer;
                 r.result
                     .ptr
@@ -367,26 +456,20 @@ impl<'d, const N: usize> Saadc<'d, N> {
         })
         .await;
     }
-
-    /// Return the sample task for use with PPI
-    pub fn task_sample(&self) -> Task {
-        let r = Self::regs();
-        Task::from_reg(&r.tasks_sample)
-    }
 }
 
 impl<'d> Saadc<'d, 1> {
     /// Continuous sampling on a single channel with double buffers.
     ///
     /// The internal clock is to be used with a sample rate expressed as a divisor of
-    /// 16MHz, ranging from 80..2047. For example, 1600 represnts a sample rate of 10KHz
+    /// 16MHz, ranging from 80..2047. For example, 1600 represents a sample rate of 10KHz
     /// given 16_000_000 / 10_000_000 = 1600.
     ///
     /// A sampler closure is provided that receives the buffer of samples, noting
     /// that the size of this buffer can be less than the original buffer's size.
     /// A command is return from the closure that indicates whether the sampling
     /// should continue or stop.
-    pub async fn run_timer_sampler<S, const N0: usize>(
+    pub async fn run_timer_sampler<I, S, const N0: usize>(
         &mut self,
         bufs: &mut [[[i16; 1]; N0]; 2],
         sample_rate_divisor: u16,
@@ -394,7 +477,7 @@ impl<'d> Saadc<'d, 1> {
     ) where
         S: FnMut(&[[i16; 1]]) -> SamplerState,
     {
-        self.run_sampler(bufs, Some(sample_rate_divisor), sampler)
+        self.run_sampler(bufs, Some(sample_rate_divisor), || {}, sampler)
             .await;
     }
 }

embassy-stm32/build.rs

@@ -105,46 +105,41 @@ fn main() {
     // ========
     // Generate DMA IRQs.
 
-    let mut dma_irqs: HashSet<&str> = HashSet::new();
+    let mut dma_irqs: HashMap<&str, Vec<(&str, &str)>> = HashMap::new();
-    let mut bdma_irqs: HashSet<&str> = HashSet::new();
 
     for p in METADATA.peripherals {
         if let Some(r) = &p.registers {
-            match r.kind {
+            if r.kind == "dma" || r.kind == "bdma" {
-                "dma" => {
+                if p.name == "BDMA1" {
-                    for irq in p.interrupts {
+                    // BDMA1 in H7 doesn't use DMAMUX, which breaks
-                        dma_irqs.insert(irq.interrupt);
+                    continue;
-                    }
                 }
-                "bdma" => {
+                for irq in p.interrupts {
-                    for irq in p.interrupts {
+                    dma_irqs
-                        bdma_irqs.insert(irq.interrupt);
+                        .entry(irq.interrupt)
-                    }
+                        .or_default()
+                        .push((p.name, irq.signal));
                 }
-                _ => {}
             }
         }
     }
 
-    let tokens: Vec<_> = dma_irqs.iter().map(|s| format_ident!("{}", s)).collect();
+    for (irq, channels) in dma_irqs {
-    g.extend(quote! {
+        let irq = format_ident!("{}", irq);
-        #(
-            #[crate::interrupt]
-            unsafe fn #tokens () {
-                crate::dma::dma::on_irq();
-            }
-        )*
-    });
 
-    let tokens: Vec<_> = bdma_irqs.iter().map(|s| format_ident!("{}", s)).collect();
+        let channels = channels
-    g.extend(quote! {
+            .iter()
-        #(
+            .map(|(dma, ch)| format_ident!("{}_{}", dma, ch));
+
+        g.extend(quote! {
             #[crate::interrupt]
-            unsafe fn #tokens () {
+            unsafe fn #irq () {
-                crate::dma::bdma::on_irq();
+                #(
+                    <crate::peripherals::#channels as crate::dma::sealed::Channel>::on_irq();
+                )*
             }
-        )*
+        });
-    });
+    }
 
     // ========
     // Generate RccPeripheral impls

embassy-stm32/src/dma/bdma.rs

@@ -38,26 +38,6 @@ impl State {
 
 static STATE: State = State::new();
 
-pub(crate) unsafe fn on_irq() {
-    foreach_peripheral! {
-        (bdma, BDMA1) => {
-            // BDMA1 in H7 doesn't use DMAMUX, which breaks
-        };
-        (bdma, $dma:ident) => {
-            let isr = pac::$dma.isr().read();
-            foreach_dma_channel! {
-                ($channel_peri:ident, $dma, bdma, $channel_num:expr, $index:expr, $dmamux:tt) => {
-                    let cr = pac::$dma.ch($channel_num).cr();
-                    if isr.tcif($channel_num) && cr.read().tcie() {
-                        cr.write(|_| ()); // Disable channel interrupts with the default value.
-                        STATE.ch_wakers[$index].wake();
-                    }
-                };
-            }
-        };
-    }
-}
-
 /// safety: must be called only once
 pub(crate) unsafe fn init() {
     foreach_interrupt! {
@@ -150,6 +130,12 @@ foreach_dma_channel! {
             fn set_waker(&mut self, waker: &Waker) {
                 unsafe { low_level_api::set_waker($index, waker) }
             }
+
+            fn on_irq() {
+                unsafe {
+                    low_level_api::on_irq_inner(pac::$dma_peri, $channel_num, $index);
+                }
+            }
         }
 
         impl crate::dma::Channel for crate::peripherals::$channel_peri {}
@@ -243,4 +229,18 @@ mod low_level_api {
             w.set_teif(channel_number as _, true);
         });
     }
+
+    /// Safety: Must be called with a matching set of parameters for a valid dma channel
+    pub unsafe fn on_irq_inner(dma: pac::bdma::Dma, channel_num: u8, index: u8) {
+        let channel_num = channel_num as usize;
+        let index = index as usize;
+
+        let isr = dma.isr().read();
+        let cr = dma.ch(channel_num).cr();
+
+        if isr.tcif(channel_num) && cr.read().tcie() {
+            cr.write(|_| ()); // Disable channel interrupts with the default value.
+            STATE.ch_wakers[index].wake();
+        }
+    }
 }

embassy-stm32/src/dma/dma.rs

@@ -36,22 +36,6 @@ impl State {
 
 static STATE: State = State::new();
 
-pub(crate) unsafe fn on_irq() {
-    foreach_peripheral! {
-        (dma, $dma:ident) => {
-            foreach_dma_channel! {
-                ($channel_peri:ident, $dma, dma, $channel_num:expr, $index:expr, $dmamux:tt) => {
-                    let cr = pac::$dma.st($channel_num).cr();
-                    if pac::$dma.isr($channel_num/4).read().tcif($channel_num%4) && cr.read().tcie() {
-                        cr.write(|_| ()); // Disable channel interrupts with the default value.
-                        STATE.ch_wakers[$index].wake();
-                    }
-                };
-            }
-        };
-    }
-}
-
 /// safety: must be called only once
 pub(crate) unsafe fn init() {
     foreach_interrupt! {
@@ -137,6 +121,12 @@ foreach_dma_channel! {
             fn set_waker(&mut self, waker: &Waker) {
                 unsafe {low_level_api::set_waker($index, waker )}
             }
+
+            fn on_irq() {
+                unsafe {
+                    low_level_api::on_irq_inner(pac::$dma_peri, $channel_num, $index);
+                }
+            }
         }
 
         impl crate::dma::Channel for crate::peripherals::$channel_peri { }
@@ -240,4 +230,18 @@ mod low_level_api {
             w.set_teif(isrbit, true);
         });
     }
+
+    /// Safety: Must be called with a matching set of parameters for a valid dma channel
+    pub unsafe fn on_irq_inner(dma: pac::dma::Dma, channel_num: u8, index: u8) {
+        let channel_num = channel_num as usize;
+        let index = index as usize;
+
+        let cr = dma.st(channel_num).cr();
+        let isr = dma.isr(channel_num / 4).read();
+
+        if isr.tcif(channel_num % 4) && cr.read().tcie() {
+            cr.write(|_| ()); // Disable channel interrupts with the default value.
+            STATE.ch_wakers[index].wake();
+        }
+    }
 }

embassy-stm32/src/dma/mod.rs

@@ -88,6 +88,11 @@ pub(crate) mod sealed {
 
         /// Sets the waker that is called when this channel stops (either completed or manually stopped)
         fn set_waker(&mut self, waker: &Waker);
+
+        /// This is called when this channel triggers an interrupt.
+        /// Note: Because some channels share an interrupt, this function might be
+        /// called for a channel that didn't trigger an interrupt.
+        fn on_irq();
     }
 }
 

examples/nrf/src/bin/saadc_continuous.rs

@@ -5,9 +5,9 @@
 #[path = "../example_common.rs"]
 mod example_common;
 use embassy::executor::Spawner;
-use embassy_nrf::ppi::Ppi;
+use embassy::time::Duration;
 use embassy_nrf::saadc::{ChannelConfig, Config, Saadc, SamplerState};
-use embassy_nrf::timer::{Frequency, Timer};
+use embassy_nrf::timer::Frequency;
 use embassy_nrf::{interrupt, Peripherals};
 use example_common::*;
 
@@ -26,34 +26,43 @@ async fn main(_spawner: Spawner, mut p: Peripherals) {
         [channel_1_config, channel_2_config, channel_3_config],
     );
 
-    let mut timer = Timer::new(p.TIMER0);
+    // This delay demonstrates that starting the timer prior to running
-    timer.set_frequency(Frequency::F1MHz);
+    // the task sampler is benign given the calibration that follows.
-    timer.cc(0).write(100); // We want to sample at 10KHz
+    embassy::time::Timer::after(Duration::from_millis(500)).await;
-    timer.cc(0).short_compare_clear();
+    saadc.calibrate().await;
 
-    let mut ppi = Ppi::new_one_to_one(p.PPI_CH0, timer.cc(0).event_compare(), saadc.task_sample());
+    let mut bufs = [[[0; 3]; 500]; 2];
-    ppi.enable();
-
-    timer.start();
-
-    let mut bufs = [[[0; 3]; 50]; 2];
 
     let mut c = 0;
     let mut a: i32 = 0;
 
     saadc
-        .run_task_sampler(&mut bufs, move |buf| {
+        .run_task_sampler(
-            for b in buf {
+            &mut p.TIMER0,
-                a += b[0] as i32;
+            &mut p.PPI_CH0,
-            }
+            &mut p.PPI_CH1,
-            c += buf.len();
+            Frequency::F1MHz,
-            if c > 10000 {
+            1000, // We want to sample at 1KHz
-                a = a / c as i32;
+            &mut bufs,
-                info!("channel 1: {=i32}", a);
+            move |buf| {
-                c = 0;
+                // NOTE: It is important that the time spent within this callback
-                a = 0;
+                // does not exceed the time taken to acquire the 1500 samples we
-            }
+                // have in this example, which would be 10us + 2us per
-            SamplerState::Sampled
+                // sample * 1500 = 18ms. You need to measure the time taken here
-        })
+                // and set the sample buffer size accordingly. Exceeding this
+                // time can lead to the peripheral re-writing the other buffer.
+                for b in buf {
+                    a += b[0] as i32;
+                }
+                c += buf.len();
+                if c > 1000 {
+                    a = a / c as i32;
+                    info!("channel 1: {=i32}", a);
+                    c = 0;
+                    a = 0;
+                }
+                SamplerState::Sampled
+            },
+        )
         .await;
 }

examples/stm32l4/src/bin/spi_dma.rs

@@ -21,8 +21,8 @@ async fn main(_spawner: Spawner, p: Peripherals) {
         p.PC10,
         p.PC12,
         p.PC11,
-        p.DMA1_CH0,
         p.DMA1_CH1,
+        p.DMA1_CH2,
         Hertz(1_000_000),
         Config::default(),
     );

stm32-data

@@ -1 +1 @@
-Subproject commit ad77937fb81628b982d2a674a88d983ec020fec7
+Subproject commit d3e8a2fe63eeb403102559f3f9917d9fcf27e1a1

tests/stm32/src/bin/spi_dma.rs

@@ -22,11 +22,11 @@ async fn main(_spawner: Spawner, p: Peripherals) {
     #[cfg(feature = "stm32h755zi")]
     let (sck, mosi, miso, tx_dma, rx_dma) = (p.PA5, p.PB5, p.PA6, p.DMA1_CH0, p.DMA1_CH1);
     #[cfg(feature = "stm32g491re")]
-    let (sck, mosi, miso, tx_dma, rx_dma) = (p.PA5, p.PA7, p.PA6, p.DMA1_CH0, p.DMA1_CH1);
+    let (sck, mosi, miso, tx_dma, rx_dma) = (p.PA5, p.PA7, p.PA6, p.DMA1_CH1, p.DMA1_CH2);
     #[cfg(feature = "stm32g071rb")]
-    let (sck, mosi, miso, tx_dma, rx_dma) = (p.PA5, p.PA7, p.PA6, p.DMA1_CH0, p.DMA1_CH1);
+    let (sck, mosi, miso, tx_dma, rx_dma) = (p.PA5, p.PA7, p.PA6, p.DMA1_CH1, p.DMA1_CH2);
     #[cfg(feature = "stm32wb55rg")]
-    let (sck, mosi, miso, tx_dma, rx_dma) = (p.PA5, p.PA7, p.PA6, p.DMA1_CH0, p.DMA1_CH1);
+    let (sck, mosi, miso, tx_dma, rx_dma) = (p.PA5, p.PA7, p.PA6, p.DMA1_CH1, p.DMA1_CH2);
 
     let mut spi = Spi::new(
         p.SPI1,

tests/stm32/src/bin/usart_dma.rs

@@ -25,13 +25,13 @@ async fn main(_spawner: Spawner, p: Peripherals) {
     #[cfg(feature = "stm32f103c8")]
     let (tx, rx, usart, tx_dma, rx_dma) = (p.PA9, p.PA10, p.USART1, p.DMA1_CH4, p.DMA1_CH5);
     #[cfg(feature = "stm32g491re")]
-    let (tx, rx, usart, tx_dma, rx_dma) = (p.PC4, p.PC5, p.USART1, p.DMA1_CH0, p.DMA1_CH1);
+    let (tx, rx, usart, tx_dma, rx_dma) = (p.PC4, p.PC5, p.USART1, p.DMA1_CH1, p.DMA1_CH2);
     #[cfg(feature = "stm32g071rb")]
-    let (tx, rx, usart, tx_dma, rx_dma) = (p.PC4, p.PC5, p.USART1, p.DMA1_CH0, p.DMA1_CH1);
+    let (tx, rx, usart, tx_dma, rx_dma) = (p.PC4, p.PC5, p.USART1, p.DMA1_CH1, p.DMA1_CH2);
     #[cfg(feature = "stm32f429zi")]
     let (tx, rx, usart, tx_dma, rx_dma) = (p.PG14, p.PG9, p.USART6, p.DMA2_CH6, p.DMA2_CH1);
     #[cfg(feature = "stm32wb55rg")]
-    let (tx, rx, usart, tx_dma, rx_dma) = (p.PA9, p.PA10, p.USART1, p.DMA1_CH0, p.DMA1_CH1); // TODO this is wrong
+    let (tx, rx, usart, tx_dma, rx_dma) = (p.PA9, p.PA10, p.USART1, p.DMA1_CH1, p.DMA1_CH2); // TODO this is wrong
     #[cfg(feature = "stm32h755zi")]
     let (tx, rx, usart, tx_dma, rx_dma) = (p.PB6, p.PB7, p.USART1, p.DMA1_CH0, p.DMA1_CH1);