RMT: Revise Tx error handling and add more HIL tests

esp-hal/CHANGELOG.md

@@ -27,6 +27,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - `AtomicWaker::wake` is now placed in IRAM (#4627)
 - Internal clock configuration rework (#4501)
 - RMT: Support for `Into<PulseCode>` and `From<PulseCode>` has been removed from Tx and Rx methods, respectively, in favor of requiring `PulseCode` directly. (#4616)
+- RMT: Tx handling has been revised: Some errors will now be returned by `TxTransaction::wait()` instead of `Channel::transmit`. `Channel::transmit_continuously()` can now also report `Error::EndMarkerMissing`. (#4617)
 - `Rtc::time_since_boot()` has been renamed to `Rtc::time_since_power_up()` (#4630)
 
 ### Fixed

esp-hal/src/rmt.rs

@@ -1481,15 +1481,7 @@ impl<'ch> TxTransaction<'ch, '_> {
         let result = loop {
             match self.poll_internal() {
                 Some(Event::Error) => break Err(Error::TransmissionError),
-                Some(Event::End) => {
-                    if !self.remaining_data.is_empty() {
-                        // Unexpectedly done, even though we have data left: For example, this could
-                        // happen if there is a stop code inside the data and not just at the end.
-                        break Err(Error::TransmissionError);
-                    } else {
-                        break Ok(());
-                    }
-                }
+                Some(Event::End) => break self.writer.state().to_result(),
                 _ => continue,
             }
         };
@@ -1710,15 +1702,13 @@ impl<'ch> Channel<'ch, Blocking, Tx> {
         let raw = self.raw;
         let memsize = raw.memsize();
 
-        match data.last() {
-            None => return Err((Error::InvalidArgument, self)),
-            Some(&code) if code.is_end_marker() => (),
-            Some(_) => return Err((Error::EndMarkerMissing, self)),
-        }
-
         let mut writer = RmtWriter::new();
         writer.write(&mut data, raw, true);
 
+        if let WriterState::Error(e) = writer.state() {
+            return Err((e, self));
+        }
+
         raw.clear_tx_interrupts(EnumSet::all());
         raw.start_send(None, memsize);
 
@@ -1756,10 +1746,13 @@ impl<'ch> Channel<'ch, Blocking, Tx> {
             return Err((Error::InvalidArgument, self));
         }
 
-        if data.is_empty() {
-            return Err((Error::InvalidArgument, self));
-        } else if data.len() > memsize.codes() {
-            return Err((Error::Overflow, self));
+        let mut writer = RmtWriter::new();
+        writer.write(&mut data, raw, true);
+
+        match writer.state() {
+            WriterState::Error(e) => return Err((e, self)),
+            WriterState::Active => return Err((Error::Overflow, self)),
+            WriterState::Done => (),
         }
 
         let mut _guard = TxGuard::new(raw);
@@ -1769,9 +1762,6 @@ impl<'ch> Channel<'ch, Blocking, Tx> {
         }
 
         if _guard.is_active() {
-            let mut writer = RmtWriter::new();
-            writer.write(&mut data, raw, true);
-
             raw.clear_tx_interrupts(EnumSet::all());
             raw.start_send(Some(mode), memsize);
         }
@@ -1925,28 +1915,21 @@ impl core::future::Future for TxFuture<'_> {
         let this = self.get_mut();
         let raw = this.raw;
 
-        if let WriterState::Error(err) = this.writer.state {
+        if let WriterState::Error(err) = this.writer.state() {
             return Poll::Ready(Err(err));
         }
 
         WAKER[raw.channel() as usize].register(ctx.waker());
 
         let result = match raw.get_tx_status() {
             Some(Event::Error) => Err(Error::TransmissionError),
-            Some(Event::End) => {
-                if this.writer.state == WriterState::Active {
-                    // Unexpectedly done, even though we have data left.
-                    Err(Error::TransmissionError)
-                } else {
-                    Ok(())
-                }
-            }
+            Some(Event::End) => this.writer.state().to_result(),
             Some(Event::Threshold) => {
                 raw.clear_tx_interrupts(Event::Threshold);
 
                 this.writer.write(&mut this.data, raw, false);
 
-                if this.writer.state == WriterState::Active {
+                if this.writer.state() == WriterState::Active {
                     raw.listen_tx_interrupt(Event::Threshold);
                 }
 
@@ -1970,21 +1953,10 @@ impl Channel<'_, Async, Tx> {
         let memsize = raw.memsize();
 
         let mut writer = RmtWriter::new();
+        writer.write(&mut data, raw, true);
 
-        match data.last() {
-            None => {
-                writer.state = WriterState::Error(Error::InvalidArgument);
-            }
-            Some(&code) if code.is_end_marker() => (),
-            Some(_) => {
-                writer.state = WriterState::Error(Error::EndMarkerMissing);
-            }
-        }
-
-        let _guard = if !matches!(writer.state, WriterState::Error(_)) {
-            writer.write(&mut data, raw, true);
-
-            let wrap = match writer.state {
+        let _guard = if writer.state().is_ok() {
+            let wrap = match writer.state() {
                 WriterState::Error(_) => false,
                 WriterState::Active => true,
                 WriterState::Done => false,

esp-hal/src/rmt/writer.rs

@@ -3,16 +3,37 @@ use procmacros::ram;
 
 use super::{DynChannelAccess, Error, PulseCode, Tx};
 
-#[derive(Debug, PartialEq)]
+#[derive(Clone, Copy, Debug, PartialEq)]
 #[cfg_attr(feature = "defmt", derive(defmt::Format))]
 pub(super) enum WriterState {
+    // There's still data left to write
     Active,
 
+    // An error occurred, and either
+    // - no data was written (and transmission was never started), or
+    // - some data was written and the last code written is an end marker, such that transmission
+    //   will eventually stop
     Error(Error),
 
+    // Completed without errors
     Done,
 }
 
+impl WriterState {
+    pub(super) fn is_ok(self) -> bool {
+        !matches!(self, Self::Error(_))
+    }
+
+    pub(super) fn to_result(self) -> Result<(), Error> {
+        match self {
+            // tx ended but writer wasn't done yet
+            Self::Active => Err(Error::TransmissionError),
+            Self::Error(e) => Err(e),
+            Self::Done => Ok(()),
+        }
+    }
+}
+
 #[derive(Debug)]
 #[cfg_attr(feature = "defmt", derive(defmt::Format))]
 pub(super) struct RmtWriter {
@@ -21,17 +42,34 @@ pub(super) struct RmtWriter {
     // The position may be invalid if there's no data left.
     offset: u16,
 
-    pub state: WriterState,
+    written: usize,
+
+    last_code: PulseCode,
+
+    state: WriterState,
 }
 
 impl RmtWriter {
     pub(super) fn new() -> Self {
         Self {
             offset: 0,
+            written: 0,
+            last_code: PulseCode::default(),
             state: WriterState::Active,
         }
     }
 
+    #[inline]
+    pub(super) fn state(&self) -> WriterState {
+        self.state
+    }
+
+    #[allow(unused)]
+    #[inline]
+    pub(super) fn written(&self) -> usize {
+        self.written
+    }
+
     // Copy from `data` to the hardware buffer, advancing the `data` slice accordingly.
     //
     // If `initial` is set, fill the entire buffer. Otherwise, append half the buffer's length from
@@ -51,6 +89,7 @@ impl RmtWriter {
 
         let ram_start = raw.channel_ram_start();
         let memsize = raw.memsize().codes();
+        let ram_end = unsafe { ram_start.add(memsize) };
 
         let max_count = if initial { memsize } else { memsize / 2 };
         let count = data.len().min(max_count);
@@ -60,16 +99,21 @@ impl RmtWriter {
         let mut data_ptr = data.as_ptr();
         let data_end = unsafe { data_ptr.add(count) };
 
+        let mut last_code = self.last_code;
         while data_ptr < data_end {
             // SAFETY: The iteration `count` is smaller than both `max_count` and `data.len()` such
             // that incrementing both pointers cannot advance them beyond their allocation's end.
             unsafe {
-                ram_ptr.write_volatile(data_ptr.read());
+                last_code = data_ptr.read();
+                ram_ptr.write_volatile(last_code);
                 ram_ptr = ram_ptr.add(1);
                 data_ptr = data_ptr.add(1);
             }
         }
 
+        self.last_code = last_code;
+        self.written += count;
+
         // If the input data was not exhausted, update offset as
         //
         // | initial | offset      | max_count   | new offset  |
@@ -85,7 +129,29 @@ impl RmtWriter {
         // The panic can never trigger since count <= data.len()!
         data.split_off(..count).unwrap();
         if data.is_empty() {
-            self.state = WriterState::Done;
+            self.state = if self.written == 0 {
+                // data was empty
+                WriterState::Error(Error::InvalidArgument)
+            // Do not check for end markers in the inner loop above since this would substantially
+            // increase the instruction count there. Instead, only check the last code to report on
+            // error.
+            } else if last_code.is_end_marker() {
+                WriterState::Done
+            } else {
+                // Write an extra end marker to prevent looping forever with wrapping tx.
+                if ram_ptr < ram_end {
+                    unsafe { ram_ptr.write_volatile(PulseCode::end_marker()) };
+                    WriterState::Error(Error::EndMarkerMissing)
+                } else {
+                    // The buffer is full, and we can't easily write an end marker. (Short of
+                    // overwriting some other code, which might be ok since hitting this error case
+                    // always indicates a bug in user code.)
+                    // Thus, remain in Active state. On the next Event::Threshold, this function
+                    // will be called again, with data already exhausted, and hit the other arm of
+                    // this conditional.
+                    WriterState::Active
+                }
+            };
         }
 
         debug_assert!(self.offset == 0 || self.offset as usize == memsize / 2);