From 94290981c359cfc4bb2355055a8a5d1497cf09aa Mon Sep 17 00:00:00 2001 From: James Munns Date: Wed, 20 Dec 2023 17:06:57 +0100 Subject: [PATCH] Debugging RSR --- embassy-rp/src/uart/mod.rs | 128 +++++++++++++++++++++++-------------- 1 file changed, 80 insertions(+), 48 deletions(-) diff --git a/embassy-rp/src/uart/mod.rs b/embassy-rp/src/uart/mod.rs index 998f7ccac..61d3af5de 100644 --- a/embassy-rp/src/uart/mod.rs +++ b/embassy-rp/src/uart/mod.rs @@ -116,10 +116,16 @@ pub enum Error { Parity, /// Triggered when the received character didn't have a valid stop bit. Framing, - /// There was an issue when calculating the number of transferred items - /// in an aborted DMA transaction. This is likely an error in the - /// driver implementation, please open an embassy issue. - Calculation, +} + +/// Read To Break error +#[derive(Debug, Eq, PartialEq, Copy, Clone)] +#[cfg_attr(feature = "defmt", derive(defmt::Format))] +#[non_exhaustive] +pub enum ReadToBreakError { + /// Read this many bytes, but never received a line break. + MissingBreak(usize), + Other(Error), } /// Internal DMA state of UART RX. @@ -432,12 +438,10 @@ impl<'d, T: Instance> UartRx<'d, T, Async> { ) .await; - let mut did_finish = false; let errors = match transfer_result { Either::First(()) => { // We're here because the DMA finished, BUT if an error occurred on the LAST // byte, then we may still need to grab the error state! - did_finish = true; Uartris(T::dma_state().rx_errs.swap(0, Ordering::Relaxed) as u32) } Either::Second(e) => { @@ -452,12 +456,7 @@ impl<'d, T: Instance> UartRx<'d, T, Async> { return Ok(()); } - // If we DID get an error, and DID finish, we'll have one error byte left in the FIFO. - // Pop it since we are reporting the error on THIS transaction. - if did_finish { - let _ = T::regs().uartdr().read(); - } - + // If we DID get an error, we need to figure out which one it was. if errors.oeris() { return Err(Error::Overrun); } else if errors.beris() { @@ -470,15 +469,27 @@ impl<'d, T: Instance> UartRx<'d, T, Async> { unreachable!("unrecognized rx error"); } - /// Read from the UART, until one of the following occurs: + /// Read from the UART, waiting for a line break. + /// + /// We read until one of the following occurs: /// /// * We read `buffer.len()` bytes without a line break - /// * returns `Ok(buffer)` + /// * returns `Err(ReadToBreakError::MissingBreak(buffer.len()))` /// * We read `n` bytes then a line break occurs - /// * returns `Ok(&mut buffer[..n])` + /// * returns `Ok(n)` /// * We encounter some error OTHER than a line break - /// * returns `Err(Error)` - pub async fn read_to_break<'a>(&mut self, buffer: &'a mut [u8]) -> Result<&'a mut [u8], Error> { + /// * returns `Err(ReadToBreakError::Other(error))` + /// + /// **NOTE**: you MUST provide a buffer one byte larger than your largest expected + /// message to reliably detect the framing on one single call to `read_to_break()`. + /// + /// * If you expect a message of 20 bytes + line break, and provide a 20-byte buffer: + /// * The first call to `read_to_break()` will return `Err(ReadToBreakError::MissingBreak(20))` + /// * The next call to `read_to_break()` will immediately return `Ok(0)`, from the "stale" line break + /// * If you expect a message of 20 bytes + line break, and provide a 21-byte buffer: + /// * The first call to `read_to_break()` will return `Ok(20)`. + /// * The next call to `read_to_break()` will work as expected + pub async fn read_to_break(&mut self, buffer: &mut [u8]) -> Result { // clear error flags before we drain the fifo. errors that have accumulated // in the flags will also be present in the fifo. T::dma_state().rx_errs.store(0, Ordering::Relaxed); @@ -498,11 +509,11 @@ impl<'d, T: Instance> UartRx<'d, T, Async> { // Drained fifo, still some room left! Ok(len) if len < buffer.len() => &mut buffer[len..], // Drained (some/all of the fifo), no room left - Ok(_) => return Ok(buffer), + Ok(len) => return Err(ReadToBreakError::MissingBreak(len)), // We got a break WHILE draining the FIFO, return what we did get before the break - Err((i, Error::Break)) => return Ok(&mut buffer[..i]), + Err((i, Error::Break)) => return Ok(i), // Some other error, just return the error - Err((_i, e)) => return Err(e), + Err((_i, e)) => return Err(ReadToBreakError::Other(e)), }; // start a dma transfer. if errors have happened in the interim some error @@ -538,14 +549,11 @@ impl<'d, T: Instance> UartRx<'d, T, Async> { ) .await; - let mut did_finish = false; - // Figure out our error state let errors = match transfer_result { Either::First(()) => { // We're here because the DMA finished, BUT if an error occurred on the LAST // byte, then we may still need to grab the error state! - did_finish = true; Uartris(T::dma_state().rx_errs.swap(0, Ordering::Relaxed) as u32) } Either::Second(e) => { @@ -557,7 +565,8 @@ impl<'d, T: Instance> UartRx<'d, T, Async> { if errors.0 == 0 { // No errors? That means we filled the buffer without a line break. - return Ok(buffer); + // For THIS function, that's a problem. + return Err(ReadToBreakError::MissingBreak(buffer.len())); } else if errors.beris() { // We got a Line Break! By this point, we've finished/aborted the DMA // transaction, which means that we need to figure out where it left off @@ -568,36 +577,60 @@ impl<'d, T: Instance> UartRx<'d, T, Async> { let sval = buffer.as_ptr() as usize; let eval = sval + buffer.len(); - // Note: the `write_addr()` is where the NEXT write would be. - let mut last_written = ch.regs().write_addr().read() as usize; + // Note: the `write_addr()` is where the NEXT write would be, but we ALSO + // got a line break, so take an offset of 1 + let mut next_addr = ch.regs().write_addr().read() as usize; - // Did we finish the whole DMA transfer? - if !did_finish { - // No, we did not! We stopped because we got a line break. That means the - // DMA transferred one "garbage byte" from the FIFO that held an error. - last_written -= 1; - } else { - // We did finish and got a "late break", where the interrupt error fired AFTER - // we got the last byte. Pop that from the FIFO so we don't trip on it next time. - let dr = T::regs().uartdr().read(); - if !dr.be() { - // Got an error after DMA but no error in the FIFO? - return Err(Error::Calculation); + // If we DON'T end up inside the range, something has gone really wrong. + if (next_addr < sval) || (next_addr > eval) { + unreachable!("UART DMA reported invalid `write_addr`"); + } + + // If we finished the full DMA, AND the FIFO is not-empty, AND that + // byte reports a break error, THAT byte caused the error, and not data + // in the DMA transfer! Otherwise: our DMA grabbed one "bad" byte. + // + // Note: even though we COULD detect this and return `Ok(buffer.len())`, + // we DON'T, as that is racy: if we read the error state AFTER the data + // was transferred but BEFORE the line break interrupt fired, we'd return + // `MissingBreak`. Ignoring the fact that there's a line break in the FIFO + // means callers consistently see the same error regardless of + let regs = T::regs(); + let is_end = next_addr == eval; + let not_empty = !regs.uartfr().read().rxfe(); + let is_break = regs.uartrsr().read().be(); + let last_good = is_end && not_empty && is_break; + + defmt::println!("next: {=usize}, sval: {=usize}, eval: {=usize}", next_addr, sval, eval); + defmt::println!("lg: {=bool}, is_end: {=bool}, not_empty: {=bool}, is_break: {=bool}", last_good, is_end, not_empty, is_break); + + if is_end && not_empty && !is_break { + let val = regs.uartdr().read(); + let tb = regs.uartrsr().read().be(); + let te = regs.uartfr().read().rxfe(); + defmt::println!("THEN: {=bool}, {=bool}", tb, te); + if val.be() { + panic!("Oh what the hell"); } } - // If we DON'T end up inside the range, something has gone really wrong. - if (last_written < sval) || (last_written > eval) { - return Err(Error::Calculation); + if !last_good { + defmt::println!("Last not good!"); + // The last is NOT good (it's the line-break `0x00`), so elide it + next_addr -= 1; + } else { + defmt::println!("last good!"); } - let taken = last_written - sval; - return Ok(&mut buffer[..taken]); + + defmt::println!("->{=usize}", next_addr - sval); + + return Ok(next_addr - sval); } else if errors.oeris() { - return Err(Error::Overrun); + return Err(ReadToBreakError::Other(Error::Overrun)); } else if errors.peris() { - return Err(Error::Parity); + return Err(ReadToBreakError::Other(Error::Parity)); } else if errors.feris() { - return Err(Error::Framing); + return Err(ReadToBreakError::Other(Error::Framing)); } unreachable!("unrecognized rx error"); } @@ -902,7 +935,7 @@ impl<'d, T: Instance> Uart<'d, T, Async> { self.rx.read(buffer).await } - pub async fn read_to_break<'a>(&mut self, buf: &'a mut [u8]) -> Result<&'a mut [u8], Error> { + pub async fn read_to_break<'a>(&mut self, buf: &'a mut [u8]) -> Result { self.rx.read_to_break(buf).await } } @@ -1004,7 +1037,6 @@ impl embedded_hal_nb::serial::Error for Error { Self::Break => embedded_hal_nb::serial::ErrorKind::Other, Self::Overrun => embedded_hal_nb::serial::ErrorKind::Overrun, Self::Parity => embedded_hal_nb::serial::ErrorKind::Parity, - Self::Calculation => embedded_hal_nb::serial::ErrorKind::Other, } } }