diff --git a/esp-hal/CHANGELOG.md b/esp-hal/CHANGELOG.md index b97ea90735c..efff829e28a 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) - `Rtc::time_since_boot()` has been renamed to `Rtc::time_since_power_up()` (#4630) ### Fixed 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..2f4770fd40d 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 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,28 @@ 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 + } + // 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 +83,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 +93,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 +123,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) + } 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. + 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); diff --git a/hil-test/src/bin/rmt.rs b/hil-test/src/bin/rmt.rs index 6ffd3fb6128..c3ab6499271 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_tx_errors_blocking(mut ctx: Context) { + let conf = LoopbackConfig { + end_marker: EndMarkerConfig::None, + ..Default::default() + }; + + 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); + + // 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] - fn rmt_fails_without_end_marker(mut ctx: Context) { + async fn rmt_tx_errors_async(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_async(&conf); let tx_data = generate_tx_data(&conf); - assert!(matches!( - tx_channel.transmit(&tx_data), - Err((Error::EndMarkerMissing, _)) - )); + 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,