diff --git a/esp-hal/src/rmt.rs b/esp-hal/src/rmt.rs index eea025cd407..764ab3aded4 100644 --- a/esp-hal/src/rmt.rs +++ b/esp-hal/src/rmt.rs @@ -1863,7 +1863,9 @@ impl<'ch> RxTransaction<'ch, '_> { // `RmtReader::read()` is safe to call even if `poll_internal` is called repeatedly // after the receiver finished since it returns immediately if already done. - self.reader.read(&mut self.data, raw, true); + if !self.reader.read(&mut self.data, raw, true) { + return Some(Event::Error); + } } #[cfg(rmt_has_rx_wrap)] Some(Event::Threshold) => { @@ -2077,11 +2079,13 @@ impl core::future::Future for RxFuture<'_> { // might not be desired. raw.stop_rx(false); - this.reader.read(&mut this.data, raw, true); - - match ev { - Event::Error => Err(Error::ReceiverError), - _ => Ok(this.reader.total), + if !this.reader.read(&mut this.data, raw, true) { + Err(Error::ReceiverError) + } else { + match ev { + Event::Error => Err(Error::ReceiverError), + _ => Ok(this.reader.total), + } } } #[cfg(rmt_has_rx_wrap)] diff --git a/esp-hal/src/rmt/reader.rs b/esp-hal/src/rmt/reader.rs index 674be47cdca..36c822091b3 100644 --- a/esp-hal/src/rmt/reader.rs +++ b/esp-hal/src/rmt/reader.rs @@ -39,15 +39,21 @@ impl RmtReader { // // If `final_` is set, read a full buffer length, potentially wrapping around. Otherwise, fetch // half the buffer's length. + // + // The return value is only valuable if `final_` is set. + // In that case `true` indicates that the read completed successfully, while `false` indicates + // that the read was incomplete and an error occurred. + // This was only observed on ESP32 (and probably ESP32-S2) - other chips indicated an error in + // that case. #[cfg_attr(place_rmt_driver_in_ram, ram)] pub(super) fn read( &mut self, data: &mut &mut [PulseCode], raw: DynChannelAccess, final_: bool, - ) { + ) -> bool { if self.state != ReaderState::Active { - return; + return true; } let ram_start = raw.channel_ram_start(); @@ -62,8 +68,7 @@ impl RmtReader { // => If both are the same, we're done, max_count = 0 let max_count = (if offset <= hw_offset { 0 } else { memsize }) + hw_offset - offset; - debug_assert!( - max_count == 0 && self.total == 0 + if !(max_count == 0 && self.total == 0 // We always enable wrapping if it is available. If it's unavailable, rx might // stop when the buffer is full, without an end marker present! // (Checking for two value of hw_offset here, because it's not documented what @@ -74,8 +79,10 @@ impl RmtReader { .add(hw_offset.checked_sub(1).unwrap_or(memsize - 1)) .read_volatile() } - .is_end_marker() - ); + .is_end_marker()) + { + return false; + } max_count } else { @@ -139,5 +146,7 @@ impl RmtReader { } debug_assert!(self.offset == 0 || self.offset as usize == memsize / 2); + + true } } diff --git a/examples/async/embassy_rmt_rx/src/main.rs b/examples/async/embassy_rmt_rx/src/main.rs index 3f4acdf76a2..2614eddc609 100644 --- a/examples/async/embassy_rmt_rx/src/main.rs +++ b/examples/async/embassy_rmt_rx/src/main.rs @@ -80,7 +80,11 @@ async fn main(spawner: Spawner) { loop { println!("receive"); - channel.receive(&mut data).await.unwrap(); + if channel.receive(&mut data).await.is_err() { + println!("receive error"); + continue; + } + let mut total = 0usize; for entry in &data[..data.len()] { if entry.length1() == 0 { diff --git a/hil-test/src/bin/rmt.rs b/hil-test/src/bin/rmt.rs index c02d4d1ff59..57a8ef57193 100644 --- a/hil-test/src/bin/rmt.rs +++ b/hil-test/src/bin/rmt.rs @@ -461,6 +461,7 @@ cfg_select! { struct Context { rmt: RMT<'static>, pin: AnyPin<'static>, + pin2: AnyPin<'static>, } impl Context { @@ -523,11 +524,13 @@ mod tests { esp_rtos::start(timg0.timer0, software_interrupt.software_interrupt0); - let pin = AnyPin::from(hil_test::common_test_pins!(peripherals).1); + let pins = hil_test::common_test_pins!(peripherals); + let (pin, pin2) = (AnyPin::from(pins.1), AnyPin::from(pins.0)); Context { rmt: peripherals.RMT, pin, + pin2, } } @@ -1229,4 +1232,58 @@ mod tests { "tx with loopcount 0 did not complete immediately" ); } + + // Regression test for esp-rs/esp-hal#4697 + // + // This tests ESP32-specific (mis-)behavior + #[cfg(esp32)] + #[test] + async fn rmt_check_regression_4697(mut ctx: Context) { + let rmt = Rmt::new(ctx.rmt.reborrow(), FREQ).unwrap().into_async(); + + let (rx, tx) = (ctx.pin, ctx.pin2); + + let mut rx_channel = rx_channel_creator!(rmt) + .configure_rx( + &RxChannelConfig::default() + .with_clk_divider(255) + .with_idle_threshold(10000), + ) + .unwrap() + .with_pin(rx); + + let mut spi = esp_hal::spi::master::Spi::new( + unsafe { esp_hal::peripherals::SPI2::steal() }, + Default::default(), + ) + .unwrap() + .with_sck(tx) + .into_async(); + + let mut buf = [0u8; 1000]; + + // This code is to drive the hardware into the state that caused the + // originally failed assertion. + // + // We are "spamming" RMT with pulses by feeding it the SPI SCK. + // + // While not really explainable from any documentation, this turned out to drive the + // hardware into an unexpected state. + // + // With the previous assert in place, interestingly the first two receive calls failed as + // expected but then we ran into the issue where the assert failed instead of seeing + // the error bit set. + embassy_futures::join::join( + async { + let mut data = [PulseCode::default(); 10]; + for _ in 0..3 { + assert!(rx_channel.receive(&mut data).await.is_err()); + } + }, + async { + spi.transfer_in_place_async(&mut buf).await.unwrap(); + }, + ) + .await; + } }