Improve nRF Saadc sampling

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.

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

The example has been updated to illustrate these new features.

2 files changed, 116 insertions, 24 deletions

diff --git a/embassy-nrf/src/saadc.rs b/embassy-nrf/src/saadc.rs
index 617c9e041..dcc7e86a2 100644
--- a/embassy-nrf/src/saadc.rs
+++ b/embassy-nrf/src/saadc.rs
@@ -10,7 +10,7 @@ use embassy_hal_common::unborrow;
 use futures::future::poll_fn;
 
 use crate::interrupt;
-use crate::ppi::Task;
+use crate::ppi::{Event, Task};
 use crate::{pac, peripherals};
 
 use pac::{saadc, SAADC};
@@ -207,6 +207,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 +227,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 +297,46 @@ impl<'d, const N: usize> Saadc<'d, N> {
 
     /// Continuous sampling with double buffers.
     ///
+    /// NOTE: It is important that the time spent within the callback supplied
+    /// does 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 the peripheral re-writing
+    /// the other buffer.
+    ///
     /// A task-driven approach to driving TASK_SAMPLE is expected. With a task
     /// driven approach, multiple channels can be used.
     ///
+    /// In addition, the caller is responsible for triggering TASK_START in
+    /// relation to the previous one having ended (EVENTS_END). The the initial
+    /// TASKS_START is triggered by this method.
+    ///
+    /// A closure is provided so that any required initialization such as starting
+    /// the sampling task can occur once the peripheral has been started.
+    ///
     /// 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>(
+    pub async fn run_task_sampler<I, S, const N0: usize>(
         &mut self,
         bufs: &mut [[[i16; N]; N0]; 2],
+        init: I,
         sampler: S,
     ) where
+        I: FnMut(),
         S: FnMut(&[[i16; N]]) -> SamplerState,
     {
-        self.run_sampler(bufs, None, sampler).await;
+        self.run_sampler(bufs, None, init, 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 +381,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 +400,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 +409,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
@@ -368,11 +425,23 @@ impl<'d, const N: usize> Saadc<'d, N> {
         .await;
     }
 
+    /// Return the end event for use with PPI
+    pub fn event_end(&self) -> Event {
+        let r = Self::regs();
+        Event::from_reg(&r.events_end)
+    }
+
     /// Return the sample task for use with PPI
     pub fn task_sample(&self) -> Task {
         let r = Self::regs();
         Task::from_reg(&r.tasks_sample)
     }
+
+    /// Return the start task for use with PPI
+    pub fn task_start(&self) -> Task {
+        let r = Self::regs();
+        Task::from_reg(&r.tasks_start)
+    }
 }
 
 impl<'d> Saadc<'d, 1> {
@@ -386,7 +455,7 @@ impl<'d> Saadc<'d, 1> {
     /// 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 +463,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;
     }
 }
diff --git a/examples/nrf/src/bin/saadc_continuous.rs b/examples/nrf/src/bin/saadc_continuous.rs
index 81559237b..991adabad 100644
--- a/examples/nrf/src/bin/saadc_continuous.rs
+++ b/examples/nrf/src/bin/saadc_continuous.rs
@@ -5,6 +5,7 @@
 #[path = "../example_common.rs"]
 mod example_common;
 use embassy::executor::Spawner;
+use embassy::time::Duration;
 use embassy_nrf::ppi::Ppi;
 use embassy_nrf::saadc::{ChannelConfig, Config, Saadc, SamplerState};
 use embassy_nrf::timer::{Frequency, Timer};
@@ -26,34 +27,56 @@ async fn main(_spawner: Spawner, mut p: Peripherals) {
         [channel_1_config, channel_2_config, channel_3_config],
     );
 
+    // We want the task start to effectively short with the last one ending so
+    // we don't miss any samples. The Saadc will trigger the initial TASKS_START.
+    let mut start_ppi = Ppi::new_one_to_one(p.PPI_CH0, saadc.event_end(), saadc.task_start());
+    start_ppi.enable();
+
     let mut timer = Timer::new(p.TIMER0);
     timer.set_frequency(Frequency::F1MHz);
-    timer.cc(0).write(100); // We want to sample at 10KHz
+    timer.cc(0).write(1000); // We want to sample at 1KHz
     timer.cc(0).short_compare_clear();
 
-    let mut ppi = Ppi::new_one_to_one(p.PPI_CH0, timer.cc(0).event_compare(), saadc.task_sample());
-    ppi.enable();
+    let mut sample_ppi =
+        Ppi::new_one_to_one(p.PPI_CH1, timer.cc(0).event_compare(), saadc.task_sample());
 
     timer.start();
 
-    let mut bufs = [[[0; 3]; 50]; 2];
+    // This delay demonstrates that starting the timer prior to running
+    // the task sampler is benign given the calibration that follows.
+    embassy::time::Timer::after(Duration::from_millis(500)).await;
+    saadc.calibrate().await;
+
+    let mut bufs = [[[0; 3]; 500]; 2];
 
     let mut c = 0;
     let mut a: i32 = 0;
 
     saadc
-        .run_task_sampler(&mut bufs, move |buf| {
-            for b in buf {
-                a += b[0] as i32;
-            }
-            c += buf.len();
-            if c > 10000 {
-                a = a / c as i32;
-                info!("channel 1: {=i32}", a);
-                c = 0;
-                a = 0;
-            }
-            SamplerState::Sampled
-        })
+        .run_task_sampler(
+            &mut bufs,
+            || {
+                sample_ppi.enable();
+            },
+            move |buf| {
+                // NOTE: It is important that the time spent within this callback
+                // does not exceed the time taken to acquire the 1500 samples we
+                // have in this example, which would be 10us + 2us per
+                // 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;
 }