diff --git a/firmware/src/main.rs b/firmware/src/main.rs index 0ce6f9b..225d6c2 100644 --- a/firmware/src/main.rs +++ b/firmware/src/main.rs @@ -273,15 +273,36 @@ mod app { /// Wait out the inter-frame gap, then answer the frame. /// /// Runs above the sensor so a conversion in progress can never delay a reply, - /// and below the UART ISO so incoming bytes are never dropped while we work. + /// and below the UART ISR so incoming bytes are never dropped while we work. #[task(priority = 2, shared = [bus, rx, reading, state])] async fn frame_gap(mut cx: frame_gap::Context) { let gap = board::Duration::micros(board::INTER_FRAME_GAP_US as u64); // Bytes may still be arriving. Rather than cancelling and respawning a - // timer on every byte, re-read the last arrival time and sleep again. + // timer on every byte, drain whatever has landed, re-read the last + // arrival time, and sleep again. + // + // Draining here as well as in the ISR is load-bearing, not belt and + // braces. The watermark interrupt fires mid-frame once 16 bytes are in + // the FIFO; the rest of the frame then sits *below* the watermark and + // raises no interrupt at all until the PL011's receive timeout + // eventually fires. Measuring the gap from the ISR's mid-frame + // timestamp would therefore declare the frame complete while its tail + // was still arriving, truncating every frame longer than the watermark. + // Draining before each decision means a byte that arrived can always + // push the deadline back. + // + // The cost is that the gap is measured from when we observed the byte + // rather than when it landed, so the reply is up to one gap period + // later than strictly necessary. Correctness is worth 1.8 ms. loop { - let deadline = cx.shared.rx.lock(|rx| rx.last_byte_at) + gap; + let last_byte_at = cx.shared.bus.lock(|bus| { + cx.shared.rx.lock(|rx| { + bus.drain_into(rx, Mono::now()); + rx.last_byte_at + }) + }); + let deadline = last_byte_at + gap; if Mono::now() >= deadline { break; } diff --git a/firmware/src/rs485.rs b/firmware/src/rs485.rs index 41d324d..9ba52c7 100644 --- a/firmware/src/rs485.rs +++ b/firmware/src/rs485.rs @@ -12,11 +12,17 @@ //! FIFO-level ones, so `BUSY` is the only way to know the final stop bit has //! left the shift register and the transceiver may stop driving the bus. //! -//! `RTIM` alone is not a sufficient frame delimiter: it only fires while the RX -//! FIFO is non-empty, so a frame whose length happens to land exactly on the -//! FIFO watermark can be drained by the watermark interrupt and never produce a -//! timeout. The authoritative delimiter is therefore a timer measured from the -//! last received byte; `RTIM` just makes the common case prompt. +//! `RTIM` is not a sufficient frame delimiter on its own, for two reasons. It +//! only fires while the RX FIFO is non-empty, so a frame drained exactly empty +//! by the watermark interrupt never produces a timeout at all. And once the +//! watermark interrupt has fired mid-frame, the remaining bytes sit below the +//! watermark and raise no interrupt until the timeout finally arrives — so an +//! interrupt timestamp says nothing reliable about when the frame ended. +//! +//! The authoritative delimiter is therefore a monotonic timer, and the task +//! running it must drain the FIFO itself before each decision rather than trust +//! a timestamp the ISR left behind. See `frame_gap` in `main.rs`; getting this +//! wrong silently truncates every frame longer than the watermark. use embedded_hal::digital::OutputPin; use rp2040_hal::pac;