From 9fd49fb9d675f858691491a57ab50dd049168def Mon Sep 17 00:00:00 2001 From: James Munns Date: Tue, 19 Dec 2023 00:11:35 +0100 Subject: [PATCH 1/6] Add a basic "read to break" function --- embassy-rp/src/uart/mod.rs | 88 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 88 insertions(+) diff --git a/embassy-rp/src/uart/mod.rs b/embassy-rp/src/uart/mod.rs index 99fce0fc9..3488d6c73 100644 --- a/embassy-rp/src/uart/mod.rs +++ b/embassy-rp/src/uart/mod.rs @@ -443,6 +443,90 @@ impl<'d, T: Instance> UartRx<'d, T, Async> { } unreachable!("unrecognized rx error"); } + + pub async fn read_to_break<'a>(&mut self, buffer: &'a mut [u8]) -> Result<&'a mut [u8], Error> { + // 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); + T::regs().uarticr().write(|w| { + w.set_oeic(true); + w.set_beic(true); + w.set_peic(true); + w.set_feic(true); + }); + + // then drain the fifo. we need to read at most 32 bytes. errors that apply + // to fifo bytes will be reported directly. + let sbuffer = match { + let limit = buffer.len().min(32); + self.drain_fifo(&mut buffer[0..limit]) + } { + Ok(len) if len < buffer.len() => &mut buffer[len..], + Ok(_) => return Ok(buffer), + Err(e) => return Err(e), + }; + + // start a dma transfer. if errors have happened in the interim some error + // interrupt flags will have been raised, and those will be picked up immediately + // by the interrupt handler. + let mut ch = self.rx_dma.as_mut().unwrap(); + T::regs().uartimsc().write_set(|w| { + w.set_oeim(true); + w.set_beim(true); + w.set_peim(true); + w.set_feim(true); + }); + T::regs().uartdmacr().write_set(|reg| { + reg.set_rxdmae(true); + reg.set_dmaonerr(true); + }); + let transfer = unsafe { + // If we don't assign future to a variable, the data register pointer + // is held across an await and makes the future non-Send. + crate::dma::read(&mut ch, T::regs().uartdr().as_ptr() as *const _, sbuffer, T::RX_DREQ) + }; + + // wait for either the transfer to complete or an error to happen. + let transfer_result = select( + transfer, + poll_fn(|cx| { + T::dma_state().rx_err_waker.register(cx.waker()); + match T::dma_state().rx_errs.swap(0, Ordering::Relaxed) { + 0 => Poll::Pending, + e => Poll::Ready(Uartris(e as u32)), + } + }), + ) + .await; + + let errors = match transfer_result { + Either::First(()) => return Ok(buffer), + Either::Second(e) => e, + }; + + if errors.0 == 0 { + return Ok(buffer); + } else if errors.oeris() { + return Err(Error::Overrun); + } else if errors.beris() { + // Begin "James is a chicken" region - I'm not certain if there is ever + // a case where the write addr WOULDN'T exist between the start and end. + // This assert checks that and hasn't fired (yet). + let sval = buffer.as_ptr() as usize; + let eval = sval + buffer.len(); + // Note: the `write_addr()` is where the NEXT write would be, BUT we also + // received one extra byte that represents the line break. + let val = ch.regs().write_addr().read() as usize - 1; + assert!((val >= sval) && (val <= eval)); + let taken = val - sval; + return Ok(&mut buffer[..taken]); + } else if errors.peris() { + return Err(Error::Parity); + } else if errors.feris() { + return Err(Error::Framing); + } + unreachable!("unrecognized rx error"); + } } impl<'d, T: Instance> Uart<'d, T, Blocking> { @@ -743,6 +827,10 @@ impl<'d, T: Instance> Uart<'d, T, Async> { pub async fn read(&mut self, buffer: &mut [u8]) -> Result<(), Error> { self.rx.read(buffer).await } + + pub async fn read_to_break<'a>(&mut self, buf: &'a mut [u8]) -> Result<&'a mut [u8], Error> { + self.rx.read_to_break(buf).await + } } impl<'d, T: Instance, M: Mode> embedded_hal_02::serial::Read for UartRx<'d, T, M> { From 24fc12667dc3b929d4fef633cdf0c1ada9765484 Mon Sep 17 00:00:00 2001 From: James Munns Date: Wed, 20 Dec 2023 14:14:14 +0100 Subject: [PATCH 2/6] Update with more docs and less panics --- embassy-rp/src/uart/mod.rs | 118 +++++++++++++++++++++++++++++-------- 1 file changed, 95 insertions(+), 23 deletions(-) diff --git a/embassy-rp/src/uart/mod.rs b/embassy-rp/src/uart/mod.rs index 3488d6c73..a89cb5932 100644 --- a/embassy-rp/src/uart/mod.rs +++ b/embassy-rp/src/uart/mod.rs @@ -116,6 +116,10 @@ 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, } /// Internal DMA state of UART RX. @@ -275,13 +279,13 @@ impl<'d, T: Instance, M: Mode> UartRx<'d, T, M> { /// Read from UART RX blocking execution until done. pub fn blocking_read(&mut self, mut buffer: &mut [u8]) -> Result<(), Error> { while buffer.len() > 0 { - let received = self.drain_fifo(buffer)?; + let received = self.drain_fifo(buffer).map_err(|(_i, e)| e)?; buffer = &mut buffer[received..]; } Ok(()) } - fn drain_fifo(&mut self, buffer: &mut [u8]) -> Result { + fn drain_fifo(&mut self, buffer: &mut [u8]) -> Result { let r = T::regs(); for (i, b) in buffer.iter_mut().enumerate() { if r.uartfr().read().rxfe() { @@ -291,13 +295,13 @@ impl<'d, T: Instance, M: Mode> UartRx<'d, T, M> { let dr = r.uartdr().read(); if dr.oe() { - return Err(Error::Overrun); + return Err((i, Error::Overrun)); } else if dr.be() { - return Err(Error::Break); + return Err((i, Error::Break)); } else if dr.pe() { - return Err(Error::Parity); + return Err((i, Error::Parity)); } else if dr.fe() { - return Err(Error::Framing); + return Err((i, Error::Framing)); } else { *b = dr.data(); } @@ -389,7 +393,7 @@ impl<'d, T: Instance> UartRx<'d, T, Async> { } { Ok(len) if len < buffer.len() => &mut buffer[len..], Ok(_) => return Ok(()), - Err(e) => return Err(e), + Err((_i, e)) => return Err(e), }; // start a dma transfer. if errors have happened in the interim some error @@ -425,14 +429,33 @@ impl<'d, T: Instance> UartRx<'d, T, Async> { ) .await; + let mut did_finish = false; let errors = match transfer_result { - Either::First(()) => return Ok(()), - Either::Second(e) => e, + 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) => { + // We're here because we errored, which means this is the error that + // was problematic. + e + } }; + // If we got no error, just return at this point if errors.0 == 0 { return Ok(()); - } else if errors.oeris() { + } + + // 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 errors.oeris() { return Err(Error::Overrun); } else if errors.beris() { return Err(Error::Break); @@ -444,6 +467,14 @@ impl<'d, T: Instance> UartRx<'d, T, Async> { unreachable!("unrecognized rx error"); } + /// Read from the UART, until one of the following occurs: + /// + /// * We read `buffer.len()` bytes without a line break + /// * returns `Ok(buffer)` + /// * We read `n` bytes then a line break occurs + /// * returns `Ok(&mut buffer[..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> { // clear error flags before we drain the fifo. errors that have accumulated // in the flags will also be present in the fifo. @@ -461,9 +492,14 @@ impl<'d, T: Instance> UartRx<'d, T, Async> { let limit = buffer.len().min(32); self.drain_fifo(&mut buffer[0..limit]) } { + // 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), - Err(e) => return Err(e), + // 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]), + // Some other error, just return the error + Err((_i, e)) => return Err(e), }; // start a dma transfer. if errors have happened in the interim some error @@ -499,27 +535,62 @@ 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(()) => return Ok(buffer), - Either::Second(e) => e, + 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) => { + // We're here because we errored, which means this is the error that + // was problematic. + e + } }; if errors.0 == 0 { + // No errors? That means we filled the buffer without a line break. return Ok(buffer); - } else if errors.oeris() { - return Err(Error::Overrun); } else if errors.beris() { - // Begin "James is a chicken" region - I'm not certain if there is ever - // a case where the write addr WOULDN'T exist between the start and end. - // This assert checks that and hasn't fired (yet). + // 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 + // by looking at the write_addr. + // + // First, we do a sanity check to make sure the write value is within the + // range of DMA we just did. let sval = buffer.as_ptr() as usize; let eval = sval + buffer.len(); - // Note: the `write_addr()` is where the NEXT write would be, BUT we also - // received one extra byte that represents the line break. - let val = ch.regs().write_addr().read() as usize - 1; - assert!((val >= sval) && (val <= eval)); - let taken = val - sval; + + // Note: the `write_addr()` is where the NEXT write would be. + let mut last_written = 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 (last_written < sval) || (last_written > eval) { + return Err(Error::Calculation); + } + let taken = last_written - sval; return Ok(&mut buffer[..taken]); + } else if errors.oeris() { + return Err(Error::Overrun); } else if errors.peris() { return Err(Error::Parity); } else if errors.feris() { @@ -930,6 +1001,7 @@ 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, } } } From fe172109be8644b1e0d86735f4bd267ef7180c36 Mon Sep 17 00:00:00 2001 From: James Munns Date: Wed, 20 Dec 2023 14:17:24 +0100 Subject: [PATCH 3/6] A little more cleanup --- embassy-rp/src/uart/mod.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/embassy-rp/src/uart/mod.rs b/embassy-rp/src/uart/mod.rs index a89cb5932..998f7ccac 100644 --- a/embassy-rp/src/uart/mod.rs +++ b/embassy-rp/src/uart/mod.rs @@ -278,13 +278,16 @@ impl<'d, T: Instance, M: Mode> UartRx<'d, T, M> { /// Read from UART RX blocking execution until done. pub fn blocking_read(&mut self, mut buffer: &mut [u8]) -> Result<(), Error> { - while buffer.len() > 0 { + while !buffer.is_empty() { let received = self.drain_fifo(buffer).map_err(|(_i, e)| e)?; buffer = &mut buffer[received..]; } Ok(()) } + /// Returns Ok(len) if no errors occurred. Returns Err((len, err)) if an error was + /// encountered. in both cases, `len` is the number of *good* bytes copied into + /// `buffer`. fn drain_fifo(&mut self, buffer: &mut [u8]) -> Result { let r = T::regs(); for (i, b) in buffer.iter_mut().enumerate() { From 94290981c359cfc4bb2355055a8a5d1497cf09aa Mon Sep 17 00:00:00 2001 From: James Munns Date: Wed, 20 Dec 2023 17:06:57 +0100 Subject: [PATCH 4/6] 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, } } } From 1ce96f79fb356b29b882a3571415f347e48e89c9 Mon Sep 17 00:00:00 2001 From: James Munns Date: Wed, 20 Dec 2023 17:27:58 +0100 Subject: [PATCH 5/6] Fun Learning about the RP2040 UART impl! --- embassy-rp/src/uart/mod.rs | 77 +++++++++++++++++++------------------- 1 file changed, 39 insertions(+), 38 deletions(-) diff --git a/embassy-rp/src/uart/mod.rs b/embassy-rp/src/uart/mod.rs index 61d3af5de..a9ae6c3f4 100644 --- a/embassy-rp/src/uart/mod.rs +++ b/embassy-rp/src/uart/mod.rs @@ -577,54 +577,55 @@ 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, but we ALSO - // got a line break, so take an offset of 1 - let mut next_addr = ch.regs().write_addr().read() as usize; + // This is the address where the DMA would write to next + let next_addr = ch.regs().write_addr().read() as usize; // If we DON'T end up inside the range, something has gone really wrong. + // Note that it's okay that `eval` is one past the end of the slice, as + // this is where the write pointer will end up at the end of a full + // transfer. 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; + let all_full = next_addr == eval; - 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); + // NOTE: This is off label usage of RSR! See the issue below for + // why I am not checking if there is an "extra" FIFO byte, and why + // I am checking RSR directly (it seems to report the status of the LAST + // POPPED value, rather than the NEXT TO POP value like the datasheet + // suggests!) + // + // issue: https://github.com/raspberrypi/pico-feedback/issues/367 + let last_was_break = regs.uartrsr().read().be(); - 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"); + return match (all_full, last_was_break) { + (true, true) | (false, _) => { + // We got less than the full amount + a break, or the full amount + // and the last byte was a break. Subtract the break off. + Ok((next_addr - 1) - sval) } - } - - 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!"); - } - - defmt::println!("->{=usize}", next_addr - sval); - - return Ok(next_addr - sval); + (true, false) => { + // We finished the whole DMA, and the last DMA'd byte was NOT a break + // character. This is an error. + // + // NOTE: we COULD potentially return Ok(buffer.len()) here, since we + // know a line break occured at SOME POINT after the DMA completed. + // + // However, we have no way of knowing if there was extra data BEFORE + // that line break, so instead return an Err to signal to the caller + // that there are "leftovers", and they'll catch the actual line break + // on the next call. + // + // Doing it like this also avoids racyness: now whether you finished + // the full read BEFORE the line break occurred or AFTER the line break + // occurs, you still get `MissingBreak(buffer.len())` instead of sometimes + // getting `Ok(buffer.len())` if you were "late enough" to observe the + // line break. + Err(ReadToBreakError::MissingBreak(buffer.len())) + } + }; } else if errors.oeris() { return Err(ReadToBreakError::Other(Error::Overrun)); } else if errors.peris() { From 5e08bb8bc3a7e0e308cbada03c9c363cdf5223b9 Mon Sep 17 00:00:00 2001 From: James Munns Date: Fri, 19 Jan 2024 15:46:36 +0100 Subject: [PATCH 6/6] A rebase ate my doc comment! --- embassy-rp/src/uart/mod.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/embassy-rp/src/uart/mod.rs b/embassy-rp/src/uart/mod.rs index a9ae6c3f4..9f5ba4e8a 100644 --- a/embassy-rp/src/uart/mod.rs +++ b/embassy-rp/src/uart/mod.rs @@ -125,6 +125,7 @@ pub enum Error { pub enum ReadToBreakError { /// Read this many bytes, but never received a line break. MissingBreak(usize), + /// Other, standard issue with the serial request Other(Error), } @@ -936,6 +937,9 @@ impl<'d, T: Instance> Uart<'d, T, Async> { self.rx.read(buffer).await } + /// Read until the buffer is full or a line break occurs. + /// + /// See [`UartRx::read_to_break()`] for more details pub async fn read_to_break<'a>(&mut self, buf: &'a mut [u8]) -> Result { self.rx.read_to_break(buf).await }