Own the sequence buffer

This approach owns the sequence buffers which, while introducing an extra move, it eliminates the need to guard the lifetime of the sequence buffer. Given ownership, the buffer will be retained until the PWM sequence task is stopped.

embassy-nrf/src/pwm.rs

@@ -25,14 +25,14 @@ pub struct SimplePwm<'d, T: Instance> {
 
 /// SequencePwm allows you to offload the updating of a sequence of duty cycles
 /// to up to four channels, as well as repeat that sequence n times.
-pub struct SequencePwm<'d, T: Instance> {
+pub struct SequencePwm<'d, T: Instance, const S0: usize, const S1: usize> {
     phantom: PhantomData<&'d mut T>,
     ch0: Option<AnyPin>,
     ch1: Option<AnyPin>,
     ch2: Option<AnyPin>,
     ch3: Option<AnyPin>,
-    sequence0: Option<Sequence<'d>>,
+    sequence0: Option<Sequence<S0>>,
-    sequence1: Option<Sequence<'d>>,
+    sequence1: Option<Sequence<S1>>,
 }
 
 #[derive(Debug, Clone, Copy, PartialEq, Eq)]
@@ -43,13 +43,17 @@ pub enum Error {
     SequenceTooLong,
     /// Min Sequence count is 1
     SequenceTimesAtLeastOne,
+    /// Sequence 0 is required, Sequence 1 is NOT required
+    SequenceTimesRequireSeq0Only,
+    /// Sequence 0 is required, Sequence 1 is required
+    SequenceTimesRequireBothSeq0AndSeq1,
     /// EasyDMA can only read from data memory, read only buffers in flash will fail.
     DMABufferNotInDataMemory,
 }
 
 const MAX_SEQUENCE_LEN: usize = 32767;
 
-impl<'d, T: Instance> SequencePwm<'d, T> {
+impl<'d, T: Instance, const S0: usize, const S1: usize> SequencePwm<'d, T, S0, S1> {
     /// Creates the interface to a `SequencePwm`.
     ///
     /// Must be started by calling `start`
@@ -68,6 +72,10 @@ impl<'d, T: Instance> SequencePwm<'d, T> {
         ch3: impl Unborrow<Target = impl GpioOptionalPin> + 'd,
         config: Config,
     ) -> Result<Self, Error> {
+        if S0 > MAX_SEQUENCE_LEN || S1 > MAX_SEQUENCE_LEN {
+            return Err(Error::SequenceTooLong);
+        }
+
         unborrow!(ch0, ch1, ch2, ch3);
 
         let r = T::regs();
@@ -133,31 +141,49 @@ impl<'d, T: Instance> SequencePwm<'d, T> {
     }
 
     /// Start or restart playback. Takes at least one sequence along with its
-    /// configuration. Optionally takes a second sequence and its configuration.
+    /// configuration. A second sequence must be provided when looping i.e.
-    /// In the case where no second sequence is provided then the first sequence
+    /// when the sequence mode is anything other than Times(1).
-    /// is used. The sequence mode applies to both sequences combined as one.
     #[inline(always)]
     pub fn start(
         &mut self,
-        sequence0: Sequence<'d>,
+        sequence0: Sequence<S0>,
-        sequence1: Option<Sequence<'d>>,
+        sequence1: Sequence<S1>,
         times: SequenceMode,
     ) -> Result<(), Error> {
-        let alt_sequence = sequence1.as_ref().unwrap_or(&sequence0);
+        slice_in_ram_or(&sequence0.words, Error::DMABufferNotInDataMemory)?;
+        slice_in_ram_or(&sequence1.words, Error::DMABufferNotInDataMemory)?;
 
-        slice_in_ram_or(sequence0.words, Error::DMABufferNotInDataMemory)?;
+        let seq_0_word_count = sequence0.word_count.unwrap_or(S0);
-        slice_in_ram_or(alt_sequence.words, Error::DMABufferNotInDataMemory)?;
+        let seq_1_word_count = sequence0.word_count.unwrap_or(S1);
-
+        if seq_0_word_count > S0 || seq_1_word_count > S1 {
-        if sequence0.words.len() > MAX_SEQUENCE_LEN || alt_sequence.words.len() > MAX_SEQUENCE_LEN {
             return Err(Error::SequenceTooLong);
         }
 
-        if let SequenceMode::Times(0) = times {
+        match times {
-            return Err(Error::SequenceTimesAtLeastOne);
+            SequenceMode::Times(0) => return Err(Error::SequenceTimesAtLeastOne),
+            SequenceMode::Times(1) if seq_0_word_count == 0 || seq_1_word_count != 0 => {
+                return Err(Error::SequenceTimesRequireSeq0Only)
+            }
+            SequenceMode::Times(1) => (),
+            SequenceMode::Times(_) | SequenceMode::Infinite
+                if seq_0_word_count == 0 || seq_1_word_count == 0 =>
+            {
+                return Err(Error::SequenceTimesRequireBothSeq0AndSeq1)
+            }
+            SequenceMode::Times(_) | SequenceMode::Infinite => (),
         }
 
         let _ = self.stop();
 
+        // We now own these sequences and they will be moved. We want
+        // the peripheral to point at the right bits of memory hence
+        // moving the sequences early.
+        self.sequence0 = Some(sequence0);
+        self.sequence1 = Some(sequence1);
+
+        let sequence0 = self.sequence0.as_ref().unwrap();
+        let sequence1 = self.sequence1.as_ref().unwrap();
+
         let r = T::regs();
 
         r.seq0
@@ -171,20 +197,20 @@ impl<'d, T: Instance> SequencePwm<'d, T> {
             .write(|w| unsafe { w.bits(sequence0.words.as_ptr() as u32) });
         r.seq0
             .cnt
-            .write(|w| unsafe { w.bits(sequence0.words.len() as u32) });
+            .write(|w| unsafe { w.bits(seq_0_word_count as u32) });
 
         r.seq1
             .refresh
-            .write(|w| unsafe { w.bits(alt_sequence.config.refresh) });
+            .write(|w| unsafe { w.bits(sequence1.config.refresh) });
         r.seq1
             .enddelay
-            .write(|w| unsafe { w.bits(alt_sequence.config.end_delay) });
+            .write(|w| unsafe { w.bits(sequence1.config.end_delay) });
         r.seq1
             .ptr
-            .write(|w| unsafe { w.bits(alt_sequence.words.as_ptr() as u32) });
+            .write(|w| unsafe { w.bits(sequence1.words.as_ptr() as u32) });
         r.seq1
             .cnt
-            .write(|w| unsafe { w.bits(alt_sequence.words.len() as u32) });
+            .write(|w| unsafe { w.bits(seq_1_word_count as u32) });
 
         r.enable.write(|w| w.enable().enabled());
 
@@ -226,9 +252,6 @@ impl<'d, T: Instance> SequencePwm<'d, T> {
             }
         }
 
-        self.sequence0 = Some(sequence0);
-        self.sequence1 = sequence1;
-
         Ok(())
     }
 
@@ -336,7 +359,7 @@ impl<'d, T: Instance> SequencePwm<'d, T> {
     /// cycle from the pin. Returns any sequences previously provided to
     /// `start` so that they may be further mutated.
     #[inline(always)]
-    pub fn stop(&mut self) -> (Option<Sequence<'d>>, Option<Sequence<'d>>) {
+    pub fn stop(&mut self) -> (Option<Sequence<S0>>, Option<Sequence<S1>>) {
         let r = T::regs();
 
         r.shorts.reset();
@@ -352,7 +375,7 @@ impl<'d, T: Instance> SequencePwm<'d, T> {
     }
 }
 
-impl<'a, T: Instance> Drop for SequencePwm<'a, T> {
+impl<'a, T: Instance, const S0: usize, const S1: usize> Drop for SequencePwm<'a, T, S0, S1> {
     fn drop(&mut self) {
         let r = T::regs();
 
@@ -381,6 +404,7 @@ impl<'a, T: Instance> Drop for SequencePwm<'a, T> {
     }
 }
 
+/// Configuration for the PWM as a whole.
 #[non_exhaustive]
 pub struct Config {
     /// Selects up mode or up-and-down mode for the counter
@@ -404,6 +428,7 @@ impl Default for Config {
     }
 }
 
+/// Configuration per sequence
 #[non_exhaustive]
 #[derive(Clone)]
 pub struct SequenceConfig {
@@ -422,19 +447,38 @@ impl Default for SequenceConfig {
     }
 }
 
+/// A composition of a sequence buffer and its configuration.
 #[non_exhaustive]
-pub struct Sequence<'d> {
+#[derive(Clone)]
+pub struct Sequence<const S: usize> {
     /// The words comprising the sequence. Must not exceed 32767 words.
-    pub words: &'d mut [u16],
+    pub words: [u16; S],
+    /// The count of words to use. If None the S will be used.
+    pub word_count: Option<usize>,
     /// Configuration associated with the sequence.
     pub config: SequenceConfig,
 }
 
-impl<'d> Sequence<'d> {
+impl<const S: usize> Sequence<S> {
-    pub fn new(words: &'d mut [u16], config: SequenceConfig) -> Self {
+    pub const fn new(words: [u16; S], config: SequenceConfig) -> Self {
-        Self { words, config }
+        Self {
+            words,
+            word_count: None,
+            config,
         }
     }
+}
+
+/// Declares an empty sequence which will cause it to be disabled.
+/// Note that any looping i.e. !Times(1), will require a second
+/// sequence given the way the PWM peripheral works.
+pub const EMPTY_SEQ: Sequence<0> = Sequence::new(
+    [],
+    SequenceConfig {
+        refresh: 0,
+        end_delay: 0,
+    },
+);
 
 /// How many times to run the sequence
 #[derive(Debug, Eq, PartialEq, Clone, Copy)]
@@ -446,7 +490,7 @@ pub enum SequenceMode {
     /// 5 to 6 = Run sequence 0, sequence 1, sequence 0, sequence 1, sequence 0 and then sequence 1
     /// i.e the when >= 2 the loop count is determined by dividing by 2 and rounding up
     Times(u16),
-    /// Repeat until `stop` is called.
+    /// Repeat until `stop` is called. Both sequences must be provided.
     Infinite,
 }
 

examples/nrf/src/bin/pwm_sequence.rs

@@ -8,14 +8,13 @@ use defmt::*;
 use embassy::executor::Spawner;
 use embassy::time::{Duration, Timer};
 use embassy_nrf::gpio::NoPin;
-use embassy_nrf::pwm::{Config, Prescaler, Sequence, SequenceConfig, SequenceMode, SequencePwm};
+use embassy_nrf::pwm::{
+    Config, Prescaler, Sequence, SequenceConfig, SequenceMode, SequencePwm, EMPTY_SEQ,
+};
 use embassy_nrf::Peripherals;
 
 #[embassy::main]
 async fn main(_spawner: Spawner, p: Peripherals) {
-    let mut seq_words_1: [u16; 5] = [1000, 250, 100, 50, 0];
-    let mut seq_words_2: [u16; 5] = [0, 50, 100, 250, 1000];
-
     let mut config = Config::default();
     config.prescaler = Prescaler::Div128;
     // 1 period is 1000 * (128/16mhz = 0.000008s = 0.008ms) = 8us
@@ -26,25 +25,20 @@ async fn main(_spawner: Spawner, p: Peripherals) {
     seq_config.refresh = 624;
     // thus our sequence takes 5 * 5000ms or 25 seconds
 
+    let seq_1 = Sequence::new([1000, 250, 100, 50, 0], seq_config.clone());
+    let seq_2 = Sequence::new([0, 50, 100, 250, 1000], seq_config);
+
     let mut pwm = unwrap!(SequencePwm::new(
         p.PWM0, p.P0_13, NoPin, NoPin, NoPin, config,
     ));
-    let _ = pwm.start(
+    unwrap!(pwm.start(seq_1, EMPTY_SEQ, SequenceMode::Times(1)));
-        Sequence::new(&mut seq_words_1, seq_config.clone()),
-        None,
-        SequenceMode::Infinite,
-    );
 
     info!("pwm started!");
 
     Timer::after(Duration::from_millis(20000)).await;
     info!("pwm starting with another sequence!");
 
-    let _ = pwm.start(
+    unwrap!(pwm.start(seq_2, EMPTY_SEQ, SequenceMode::Times(1)));
-        Sequence::new(&mut seq_words_2, seq_config),
-        None,
-        SequenceMode::Infinite,
-    );
 
     // we can abort a sequence if we need to before its complete with pwm.stop()
     // or stop is also implicitly called when the pwm peripheral is dropped

examples/nrf/src/bin/pwm_sequence_ppi.rs

@@ -16,7 +16,7 @@ use embassy_nrf::Peripherals;
 
 #[embassy::main]
 async fn main(_spawner: Spawner, p: Peripherals) {
-    let mut seq_words: [u16; 5] = [1000, 250, 100, 50, 0];
+    let seq_words: [u16; 5] = [1000, 250, 100, 50, 0];
 
     let mut config = Config::default();
     config.prescaler = Prescaler::Div128;
@@ -31,11 +31,12 @@ async fn main(_spawner: Spawner, p: Peripherals) {
         p.PWM0, p.P0_13, NoPin, NoPin, NoPin, config,
     ));
 
-    let _ = pwm.start(
+    // If we loop in any way i.e. not Times(1), then we must provide
-        Sequence::new(&mut seq_words, seq_config),
+    // the PWM peripheral with two sequences.
-        None,
+    let seq_0 = Sequence::new(seq_words, seq_config);
-        SequenceMode::Infinite,
+    let seq_1 = seq_0.clone();
-    );
+
+    unwrap!(pwm.start(seq_0, seq_1, SequenceMode::Infinite));
     // pwm.stop() deconfigures pins, and then the task_start_seq0 task cant work
     // so its going to have to start running in order load the configuration
 

examples/nrf/src/bin/pwm_sequence_ws2812b.rs

@@ -30,19 +30,6 @@ const RES: u16 = 0x8000;
 // line is assumed to be P1_05.
 #[embassy::main]
 async fn main(_spawner: Spawner, p: Peripherals) {
-    // Declare the bits of 24 bits
-    let mut color_seq_words = [
-        T0H, T0H, T0H, T0H, T0H, T0H, T0H, T0H, // G
-        T0H, T0H, T0H, T0H, T0H, T0H, T0H, T0H, // R
-        T1H, T1H, T1H, T1H, T1H, T1H, T1H, T1H, // B
-    ];
-    let color_seq = Sequence::new(&mut color_seq_words, SequenceConfig::default());
-
-    let mut reset_seq_words = [RES; 1];
-    let mut reset_seq_config = SequenceConfig::default();
-    reset_seq_config.end_delay = 799; // 50us (20 ticks * 40) - 1 tick because we've already got one RES;
-    let reset_seq = Sequence::new(&mut reset_seq_words, reset_seq_config);
-
     let mut config = Config::default();
     config.sequence_load = SequenceLoad::Common;
     config.prescaler = Prescaler::Div1;
@@ -51,7 +38,21 @@ async fn main(_spawner: Spawner, p: Peripherals) {
         p.PWM0, p.P1_05, NoPin, NoPin, NoPin, config,
     ));
 
-    unwrap!(pwm.start(color_seq, Some(reset_seq), SequenceMode::Times(2)));
+    // Declare the bits of 24 bits
+    let color_seq = Sequence::new(
+        [
+            T0H, T0H, T0H, T0H, T0H, T0H, T0H, T0H, // G
+            T0H, T0H, T0H, T0H, T0H, T0H, T0H, T0H, // R
+            T1H, T1H, T1H, T1H, T1H, T1H, T1H, T1H, // B
+        ],
+        SequenceConfig::default(),
+    );
+
+    let mut reset_seq_config = SequenceConfig::default();
+    reset_seq_config.end_delay = 799; // 50us (20 ticks * 40) - 1 tick because we've already got one RES;
+    let reset_seq = Sequence::new([RES], reset_seq_config);
+
+    unwrap!(pwm.start(color_seq, reset_seq, SequenceMode::Times(2)));
 
     Timer::after(Duration::from_millis(1000)).await;
 
@@ -59,9 +60,9 @@ async fn main(_spawner: Spawner, p: Peripherals) {
     let mut bit_value = T0H;
 
     loop {
-        if let (Some(color_seq), Some(reset_seq)) = pwm.stop() {
+        if let (Some(mut color_seq), Some(reset_seq)) = pwm.stop() {
             color_seq.words[color_bit] = bit_value;
-            unwrap!(pwm.start(color_seq, Some(reset_seq), SequenceMode::Times(2)));
+            unwrap!(pwm.start(color_seq, reset_seq, SequenceMode::Times(2)));
         }
 
         Timer::after(Duration::from_millis(50)).await;