Fix silent truncation of frames longer than the RX FIFO watermark

Every frame of 16 bytes or more was rejected as LengthMismatch and
silently dropped. Found by the new end-to-end suite; neither the host
tests nor the sensor verification could reach it, because it needs real
bus timing and a frame past the watermark.

The watermark interrupt fires mid-frame once 16 bytes are in the FIFO.
The ISR drained those, stamped last_byte_at, and started frame_gap. The
remaining bytes then sat *below* the watermark and raised no interrupt at
all until the PL011's receive timeout eventually fired, so frame_gap
measured its 1822 us gap from a mid-frame timestamp, concluded the frame
had ended while its tail was still arriving, and parsed a prefix whose
LEN field disagreed with its length.

Frames under 16 bytes never trip the watermark, so their only interrupt
is the receive timeout, by which point every byte is drained and the
timestamp is honest. That is why all five real commands worked and only
an oversized PING exposed it.

frame_gap now drains the FIFO itself before each decision rather than
trusting a timestamp the ISR left behind, so a byte that has landed can
always push the deadline back. The gap is then measured from when the
byte was observed rather than when it arrived, making the reply up to one
gap period later than strictly necessary; 1.8 ms is a fair price.

Verified on hardware across every payload size from 0 to 59 — frames of
5 to 64 bytes — with the node's error counters accounting exactly for the
faults the suite injects deliberately and nothing else.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
Oliver Walter
2026-07-28 20:32:23 +02:00
parent ddfda14085
commit aa48d8784f
2 changed files with 35 additions and 8 deletions
+24 -3
View File
@@ -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;
}
+11 -5
View File
@@ -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;