From 65f643bd309d6a3b345df70e64088a4c6f249901 Mon Sep 17 00:00:00 2001 From: wisp3rwind <17089248+wisp3rwind@users.noreply.github.com> Date: Sun, 19 Oct 2025 09:18:30 +0200 Subject: [PATCH 1/6] RMT: add HIL tests for more tx error cases to ensure that upcoming Iterator/Encoder-related refactoring dont't break anything. The EndMarkerMissing cases with different tx lengths seem redundant right now (since this is verified before even starting the transmission), but that will change when iterator input is supported and the presence of an end marker can only be verified when the iterator is exhausted. --- hil-test/src/bin/rmt.rs | 136 ++++++++++++++++++++++++++++++++++++---- 1 file changed, 125 insertions(+), 11 deletions(-) diff --git a/hil-test/src/bin/rmt.rs b/hil-test/src/bin/rmt.rs index 6ffd3fb6128..bea2cd0ea2c 100644 --- a/hil-test/src/bin/rmt.rs +++ b/hil-test/src/bin/rmt.rs @@ -13,7 +13,10 @@ extern crate alloc; +use core::task::Poll; + use allocator_api2::{vec, vec::Vec}; +use embassy_futures::poll_once; use esp_hal::{ Async, Blocking, @@ -38,6 +41,7 @@ use esp_hal::{ ConfigError, Error, HAS_RX_WRAP, + LoopMode, PulseCode, Rmt, Rx, @@ -407,8 +411,8 @@ async fn do_rmt_loopback_async_inner( // ...poll them, but then drop them before completion... // (`poll_once` takes the future by value and drops it before returning) - let rx_poll = embassy_futures::poll_once(rx_fut); - let tx_poll = embassy_futures::poll_once(tx_fut); + let rx_poll = poll_once(rx_fut); + let tx_poll = poll_once(tx_fut); // The test should fail here when the the delay above is increased, e.g. to 100ms. assert!(rx_poll.is_pending()); @@ -609,21 +613,133 @@ mod tests { do_rmt_loopback_async(&mut ctx, &conf).await; } + // Test that errors on invalid data are returned as expected (in particular, checks that the + // methods don't hang executing a never-ending/never-started tranmission). #[test] - fn rmt_fails_without_end_marker(mut ctx: Context) { + fn rmt_tx_errors_blocking(mut ctx: Context) { let conf = LoopbackConfig { end_marker: EndMarkerConfig::None, ..Default::default() }; - let (tx_channel, _) = ctx.setup_loopback(&conf); + let (mut tx_channel, _) = ctx.setup_loopback(&conf); + + let tx_data = generate_tx_data(&conf); + + assert!( + matches!( + tx_channel.reborrow().transmit(&tx_data), + Err((Error::EndMarkerMissing, _)) + ), + "Expected transmit to return an error without end marker" + ); + + assert!( + matches!( + tx_channel.reborrow().transmit(&tx_data[..0]), + Err((Error::InvalidArgument, _)) + ), + "Expected transmit to return an error on empty data" + ); + + // assert!( + // matches!( + // tx_channel + // .reborrow() + // .transmit_continuously(&tx_data, LoopMode::Infinite), + // Err((Error::EndMarkerMissing, _)) + // ), + // "Expected transmit_continuously to return an error without end marker" + // ); + + assert!( + matches!( + tx_channel + .reborrow() + .transmit_continuously(&tx_data[..0], LoopMode::Infinite), + Err((Error::InvalidArgument, _)) + ), + "Expected transmit_continuously to return an error on empty data" + ); + + // Configuration that requires wrapping tx and is missing an end marker + let conf = LoopbackConfig { + tx_len: CHANNEL_RAM_SIZE * 3 / 2, + end_marker: EndMarkerConfig::None, + ..Default::default() + }; let tx_data = generate_tx_data(&conf); - assert!(matches!( - tx_channel.transmit(&tx_data), - Err((Error::EndMarkerMissing, _)) - )); + // Most importantly, this should not hang indefinitely due to the missing end marker and + // re-transmitting the channel RAM content over and over again. + assert!( + matches!( + tx_channel + .reborrow() + .transmit(&tx_data) + .map(|t| t.wait()) + .flatten(), + Err((Error::EndMarkerMissing, _)) + ), + "Expected transmit to return an error without end marker when wrapping" + ); + + assert!( + matches!( + tx_channel + .reborrow() + .transmit_continuously(&tx_data, LoopMode::Infinite), + Err((Error::Overflow, _)) + ), + "Expected transmit_continuously to return an error on overflow" + ); + } + + #[test] + async fn rmt_tx_errors_async(mut ctx: Context) { + let conf = LoopbackConfig { + end_marker: EndMarkerConfig::None, + ..Default::default() + }; + + let (mut tx_channel, _) = ctx.setup_loopback_async(&conf); + + let tx_data = generate_tx_data(&conf); + + assert!( + matches!( + poll_once(tx_channel.transmit(&tx_data)), + Poll::Ready(Err(Error::EndMarkerMissing)) + ), + "Expected transmit to return an error without end marker" + ); + + assert!( + matches!( + poll_once(tx_channel.transmit(&tx_data[..0])), + Poll::Ready(Err(Error::InvalidArgument)) + ), + "Expected transmit to return an error on empty data" + ); + + // Configuration that requires wrapping tx and is missing an end marker + let conf = LoopbackConfig { + tx_len: CHANNEL_RAM_SIZE * 3 / 2, + end_marker: EndMarkerConfig::None, + ..Default::default() + }; + let tx_data = generate_tx_data(&conf); + + // Most importantly, this should not hang indefinitely due to the missing end marker and + // re-transmitting the channel RAM content over and over again. + assert!( + matches!( + tx_channel.transmit(&tx_data).await, + Err(Error::EndMarkerMissing) + ), + "Expected transmit to return an error without end marker when wrapping" + ); } #[test] @@ -952,8 +1068,6 @@ mod tests { #[cfg(not(esp32))] fn rmt_loopback_continuous_tx_impl(mut ctx: Context, use_autostop: bool) { - use esp_hal::rmt::LoopMode; - const TX_COUNT: usize = 10; const MAX_LOOP_COUNT: usize = 3; const MAX_RX_LEN: usize = (MAX_LOOP_COUNT + 1) * TX_COUNT; @@ -1046,7 +1160,7 @@ mod tests { #[cfg(any(esp32c6, esp32h2, esp32s3))] #[test] fn rmt_continuous_tx_zero_loopcount(mut ctx: Context) { - use esp_hal::{rmt::LoopMode, time::Instant}; + use esp_hal::time::Instant; let conf = LoopbackConfig { idle_output: true, From 381a465ba808f4f763ae1d9788f4fd3f9dd40c77 Mon Sep 17 00:00:00 2001 From: wisp3rwind <17089248+wisp3rwind@users.noreply.github.com> Date: Tue, 9 Dec 2025 15:23:27 +0100 Subject: [PATCH 2/6] RMT: revise error handling, track total in writer, make writer.state read-only - shares a tiny bit more code between Blocking and Async tx - removes Slice-specific error handling in preparation for supporting iterator (-like) data types - makes RmtWriter.state read-only and only write it from the rmt::write module to make it easier to reason about As a side effect, continuous tx will now also report EndMarkerMissing errors. This makes sense: The loopcount interrupt only appears to trigger when hitting and end marker, and does not trigger when in loop tx mode with wrapping enabled and wrapping at the buffer end. In other words, in that case, loop tx seems to work at first, but does not trigger interrupts, so it should probably be prevented (it might make sense to allow LoopMode::Infinite without end marker, however). --- esp-hal/src/rmt.rs | 64 ++++++++++----------------------- esp-hal/src/rmt/writer.rs | 74 ++++++++++++++++++++++++++++++++++++--- 2 files changed, 88 insertions(+), 50 deletions(-) diff --git a/esp-hal/src/rmt.rs b/esp-hal/src/rmt.rs index 8863d45d60f..1c9c09f175a 100644 --- a/esp-hal/src/rmt.rs +++ b/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,7 +1915,7 @@ 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)); } @@ -1933,20 +1923,13 @@ impl core::future::Future for TxFuture<'_> { 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, diff --git a/esp-hal/src/rmt/writer.rs b/esp-hal/src/rmt/writer.rs index 9630b0d8fee..4f5da2d383e 100644 --- a/esp-hal/src/rmt/writer.rs +++ b/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 occured, 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 writen an end marker. (Short of + // overwriting some other code, which might be ok since hitting this error case + // always indicates a bu 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); From 3a2d0a3652969982310067a82eb40df5fa84df0e Mon Sep 17 00:00:00 2001 From: wisp3rwind <17089248+wisp3rwind@users.noreply.github.com> Date: Tue, 9 Dec 2025 15:28:31 +0100 Subject: [PATCH 3/6] RMT: enable transmit_continuously EndMarkerMissing HIl test cf. the commit message of the preceding commit --- hil-test/src/bin/rmt.rs | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/hil-test/src/bin/rmt.rs b/hil-test/src/bin/rmt.rs index bea2cd0ea2c..c3ab6499271 100644 --- a/hil-test/src/bin/rmt.rs +++ b/hil-test/src/bin/rmt.rs @@ -642,15 +642,15 @@ mod tests { "Expected transmit to return an error on empty data" ); - // assert!( - // matches!( - // tx_channel - // .reborrow() - // .transmit_continuously(&tx_data, LoopMode::Infinite), - // Err((Error::EndMarkerMissing, _)) - // ), - // "Expected transmit_continuously to return an error without end marker" - // ); + assert!( + matches!( + tx_channel + .reborrow() + .transmit_continuously(&tx_data, LoopMode::Infinite), + Err((Error::EndMarkerMissing, _)) + ), + "Expected transmit_continuously to return an error without end marker" + ); assert!( matches!( From 17d4de5e774001e31ea38c5439eb1f2bb227d7db Mon Sep 17 00:00:00 2001 From: wisp3rwind <17089248+wisp3rwind@users.noreply.github.com> Date: Tue, 9 Dec 2025 15:43:20 +0100 Subject: [PATCH 4/6] update changelog --- esp-hal/CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/esp-hal/CHANGELOG.md b/esp-hal/CHANGELOG.md index 726b4ba6156..0cbedc573a8 100644 --- a/esp-hal/CHANGELOG.md +++ b/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` and `From` 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) ### Fixed From a83bfebe6d4eef0fda1a9cae5b916848e9f305fe Mon Sep 17 00:00:00 2001 From: wisp3rwind <17089248+wisp3rwind@users.noreply.github.com> Date: Tue, 9 Dec 2025 17:13:14 +0100 Subject: [PATCH 5/6] address typos noted by copilot --- esp-hal/src/rmt/writer.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/esp-hal/src/rmt/writer.rs b/esp-hal/src/rmt/writer.rs index 4f5da2d383e..a832977d33e 100644 --- a/esp-hal/src/rmt/writer.rs +++ b/esp-hal/src/rmt/writer.rs @@ -9,7 +9,7 @@ pub(super) enum WriterState { // There's still data left to write Active, - // An error occured, and either + // 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 @@ -143,9 +143,9 @@ impl RmtWriter { unsafe { ram_ptr.write_volatile(PulseCode::end_marker()) }; WriterState::Error(Error::EndMarkerMissing) } else { - // The buffer is full, and we can't easily writen an end marker. (Short of + // 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 bu in user code.) + // 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. From 61f8128573e0da9c23445a26588ecf5b3ead96fb Mon Sep 17 00:00:00 2001 From: wisp3rwind <17089248+wisp3rwind@users.noreply.github.com> Date: Sat, 13 Dec 2025 08:49:00 +0100 Subject: [PATCH 6/6] address review - remove (currently) unused getter - code style --- esp-hal/src/rmt/writer.rs | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/esp-hal/src/rmt/writer.rs b/esp-hal/src/rmt/writer.rs index a832977d33e..2f4770fd40d 100644 --- a/esp-hal/src/rmt/writer.rs +++ b/esp-hal/src/rmt/writer.rs @@ -64,12 +64,6 @@ impl RmtWriter { 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 @@ -132,10 +126,10 @@ impl RmtWriter { 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() { + // 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. WriterState::Done } else { // Write an extra end marker to prevent looping forever with wrapping tx.