From 1ce96f79fb356b29b882a3571415f347e48e89c9 Mon Sep 17 00:00:00 2001
From: James Munns <james@onevariable.com>
Date: Wed, 20 Dec 2023 17:27:58 +0100
Subject: [PATCH] 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() {