Martino FerrariandClaude Opus 4.6
d5d1295b26
The two were asked for together and are one design: the packet counter is what makes a hole addressable, so relative timing is the coordinate system in which resequencing is arithmetic rather than inference. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
320 lines
19 KiB
Markdown
320 lines
19 KiB
Markdown
<!-- SPDX-License-Identifier: MIT -->
|
||
<!-- SPDX-FileCopyrightText: 2026 Martino Ferrari -->
|
||
|
||
# A late packet leaves a hole it could have filled, and the clock it would be filed under is the wrong one
|
||
|
||
Two requests, and they are one design.
|
||
|
||
The first is that a reordered packet currently reads as permanent damage. `udps_client.c:504-514`
|
||
orders the stream by the wrapping `uint32` counter and discards anything that arrives behind the
|
||
newest counter — `stats.stale_packets++; return 0` — *before* spending anything on the payload.
|
||
The samples are gone, the span they should have filled stays empty, and udpscope never learns
|
||
there was a span at all. The asked-for remedy is to leave a hole of
|
||
`<N samples> × <LAST_PACKET_ID − NEW_PACKET_ID − 1>`, mark it invalid, and fill it when the
|
||
missing packets arrive.
|
||
|
||
The second is `todo.md` item 2: a relative-time mode in which the plot's x comes only from the
|
||
declared sample rate and the packet number, with absolute stream time kept for export.
|
||
|
||
They are one design because **the counter is what makes a hole addressable**. A reserved hole has
|
||
to be a known number of samples at a known position, and a late packet has to compute which
|
||
position it belongs to. Absolute stamps cannot do either — a lost packet's stamps were never
|
||
received, so its extent in time is a guess, and the guess is what the glitch bridge at
|
||
`src/acquisition.cpp:274-292` is already making. The counter gives the extent exactly. Relative
|
||
timing is not a cosmetic preference that happens to be requested at the same time; it is the
|
||
coordinate system in which resequencing is arithmetic rather than inference.
|
||
|
||
Two facts from the existing code shape almost every decision below, and both say the work is
|
||
smaller than it looks:
|
||
|
||
- **`frame->counter` and `frame->lost` are read nowhere in udpscope.** `frame.lost` is set by the
|
||
client at `udps_client.c:560` and appears in `src/acquisition.cpp` only inside comments
|
||
(`:309`, `:331`, `:336`). The client has been reporting the exact gap width for the life of the
|
||
project and nothing has ever consumed it. Neither input needs inventing.
|
||
- **`clamp_deadline()` already exists** (`udps_client.c:1102`), documented as *"Clamps `budget` so
|
||
a pending deadline is not slept through"* and already used for the keepalive and the silence
|
||
timeout. The reorder deadline is a third call to it.
|
||
|
||
## What was decided, and the constraint that decided it
|
||
|
||
The latency budget is **20 ms**, maximum. That one number settles the shape of the whole receive
|
||
path, because it is a *time* budget and not a packet count: at 1000 pkt/s it is twenty packet
|
||
spans of reorder depth, at 10 pkt/s it is a fifth of one. A hold policy counted in packets would
|
||
either be useless at high rates or add 400 ms at low ones. A policy counted in wall time works at
|
||
every rate, because network reorder is measured in milliseconds regardless of how fast the
|
||
producer sends.
|
||
|
||
It also means a *contiguous* arrival must be released immediately. The 20 ms is spent only when
|
||
there is a gap to wait on, never on the common case.
|
||
|
||
Past the window, a committed hole is still **backfilled for values only**. Math channels and the
|
||
trigger are not replayed. A math channel takes NaN in and gives NaN out, so after a repair the
|
||
raw trace is whole and the math trace derived from it keeps its gap. That is a deliberate,
|
||
documented state, not an oversight: the display then says *"this region arrived too late to be
|
||
part of the computation"*, which is true and is cheaper than any mechanism that could make it
|
||
false.
|
||
|
||
Resequencing and hole reservation run in **both** time modes. In relative mode a hole's stamps
|
||
are exact; in stream mode they are the declared cadence continued from the last good stamp, which
|
||
is the existing bridge mechanism reused. Relative timing is therefore an independent toggle a user
|
||
flips for jitter, not a prerequisite for recovery.
|
||
|
||
A hole is surfaced two ways: **status-bar counters** and a **shaded band** over the invalid span.
|
||
A NaN run already breaks the polyline, but a one-packet hole at a 10 s window is sub-pixel, and a
|
||
gap nobody can see is not a report.
|
||
|
||
## Relative timing
|
||
|
||
`build_stamps()` today picks its clock *by inspection*: `time_source_` latches 1 for the declared
|
||
time signal or 2 for the arrival clock and never flips back (`src/acquisition.cpp:357-367`).
|
||
Relative timing is not a third thing to infer. It is a user-selected mode that bypasses the
|
||
inference: `--relative-time` on the command line, a toggle in the UI, persisted in the session
|
||
file.
|
||
|
||
### The stamp
|
||
|
||
```
|
||
stamps_[k] = double(slot_base_ + k) / rate
|
||
```
|
||
|
||
`slot_base_` is a running `u64` of samples seen. Each packet the counter reports missing advances
|
||
it by `batch`, **before** the frame's own samples are stamped; the delivered frame then advances it
|
||
by its own `n`. The order is what puts the hole ahead of the data that revealed it.
|
||
|
||
This is the requested `counter × batch + sample_index` with the origin left at counter zero, but
|
||
**accumulated rather than multiplied**, and the difference earns three things:
|
||
|
||
- `batch` is then only ever needed to size what is *absent*. That is exactly the
|
||
`<n_samples_per_frame>` of the original formula, and it is the only place the value is load
|
||
bearing.
|
||
- The `uint32` wrap costs nothing. The counter is consumed only as a signed delta — the same
|
||
arithmetic `udps_client.c:505` already performs — so no unwrapping bookkeeping is needed.
|
||
- A frame whose `n` disagrees with the established `batch` cannot silently shift the whole
|
||
timeline, because the timeline is built from the `n` values actually seen.
|
||
|
||
`batch` is established from the first DATA frame and held for the life of the stream. A later
|
||
frame disagreeing is a genuine inconsistency in the producer, not a loss, so it warns once rather
|
||
than re-deriving.
|
||
|
||
### The rate is the declared one, and only the declared one
|
||
|
||
`build_stamps()` currently overrides a declared cadence with the arrival-measured one when the two
|
||
disagree by more than ±25% (`src/acquisition.cpp:152-158`), because a CONFIG declaring 1 MS/s
|
||
while delivering a hundred samples every fifth of a second reads as a staircase in the x column.
|
||
Relative mode must **not** do this. Discarding arrival timing is the entire point of the mode. The
|
||
override stays live in stream mode and is skipped in relative mode.
|
||
|
||
### The constant offset is already absorbed
|
||
|
||
`src/acquisition.cpp:555` stamps `rel = stamps_[k] - epoch`, and the `t0 =` caption names the
|
||
epoch. A counter that happens to read 500 000 on connect therefore shows up as an epoch value, not
|
||
as a 50 000 s axis. No origin subtraction is needed in the formula itself.
|
||
|
||
### The x axis needs no change at all
|
||
|
||
`RowSet::update_time_labels()` already goes to a relative label form on the tick step against the
|
||
label resolution at the axis' magnitude, with the `t0 =` caption naming the absolute anchor
|
||
(`docs/claude/invariants-time-axis-and-window.md:34`). "Counting backwards, like it is now" is
|
||
what that already does. Relative mode changes *where a stamp comes from* and nothing in the label
|
||
layer.
|
||
|
||
### What relative mode makes unreachable
|
||
|
||
The degeneracy guard, the implausible-jump test, `kFailureLimit`, the bridge and the monotonic
|
||
clamp are all dead in relative mode: the stamps are monotone by construction, so there is no bad
|
||
stamp to detect and nothing to clamp. (`kGlitchPackets` itself survives — not as the threshold of
|
||
that test, which is unreachable, but as the reservation cap below, where its value and its
|
||
reasoning carry over unchanged.)
|
||
|
||
That is not tidiness, it is a fix. Measured last session: a loss burst past `kGlitchPackets` (256)
|
||
is misread as a producer clock glitch; each bridge then advances the anchor by *one* packet span
|
||
while the true timeline is 300 ahead, so the next four packets fail too, `kFailureLimit` trips,
|
||
and the whole stream is thrown onto the arrival clock — the exact failure the bridge was added to
|
||
prevent (`--drop-burst 300` reads
|
||
`jumped implausibly far ahead ... (1.299..1.29999 against a previous 0.99899)` then
|
||
`timing samples by arrival instead`). In relative mode a 300-packet gap is a known quantity and
|
||
cannot be misdiagnosed.
|
||
|
||
### Absolute time for export
|
||
|
||
`todo.md` requires absolute stream time to remain available for export. There is exactly one f64
|
||
time ring per `BufferStore`, shared by every channel of that buffer, and relative mode takes it.
|
||
Export therefore needs a **second** f64 ring carrying the absolute stamp under the same group-mean
|
||
rule, written in relative mode only and read by export and by nothing else — eight bytes per
|
||
stored slot.
|
||
|
||
This cost is paid rather than avoided. Without it, selecting relative mode silently degrades the
|
||
one output `todo.md` says must keep absolute time.
|
||
|
||
## The receive path
|
||
|
||
### In the client: a reorder ring of reassembled payloads
|
||
|
||
Every `udps_frame_t` sets `frame.signals = c->sigs; frame.values = c->vals`
|
||
(`udps_client.c:566-567`) — one client-owned decode buffer, reused for every packet. A frame is
|
||
therefore valid only inside the callback, and holding one requires a copy *wherever* the holding
|
||
happens. The client has no advantage there.
|
||
|
||
It has a different one: it still holds the **raw datagram**. Resequencing undecoded wire bytes is
|
||
strictly cheaper than resequencing decoded doubles — an `f32` or `i16` payload is two to four
|
||
times smaller than what `c->valbuf` would hold — and the bytes exist nowhere else. That is why
|
||
this sits in the client and not in `Acquisition`.
|
||
|
||
The hook is the `decode_data()` call site at `udps_client.c:599`: after fragment reassembly, which
|
||
already tolerates arrival order through the slot table, and before decode. Each held entry carries
|
||
the payload bytes, their length, the counter and `recv_time`.
|
||
|
||
The policy, against `next_counter = last_delivered + 1`:
|
||
|
||
| arrival | action | added latency |
|
||
| --- | --- | --- |
|
||
| `== next_counter` | decode and deliver now, then drain held entries that became contiguous | **none** |
|
||
| `> next_counter` | copy into the ring; set the gap deadline to `now + 20 ms` if not already set | the newer packet waits |
|
||
| `< next_counter` | the gap is already committed — decode and deliver, flagged **late** | n/a |
|
||
| deadline fires | give up on the missing counters, advance `next_counter` to the lowest held entry, drain in order | capped at 20 ms |
|
||
|
||
The last row is where `stats.stale_packets++; return 0` at `:506-509` is replaced: a late packet
|
||
is delivered flagged instead of discarded, which is the single change that makes backfill possible
|
||
at all.
|
||
|
||
The deadline is served by a third `clamp_deadline()` call on the `select()` budget
|
||
(`udps_client.c:1145-1152`). Without it, a held packet on a quiet stream would wait for the next
|
||
arrival — 100 ms at 10 pkt/s — and blow the budget.
|
||
|
||
Two bounds on the ring, because 20 ms is a different number of packets at every rate: a fixed
|
||
entry count and a fixed byte budget. Overflow commits the gap early rather than growing, and
|
||
increments its own counter, so an undersized window is visible rather than silent.
|
||
|
||
The window itself is `udps_config_t::reorder_window_s`, defaulting to `0.020`, along
|
||
`recv_buffer_bytes` and `silence_timeout_s` (`udps_client.c:982`). Zero disables resequencing and
|
||
restores today's discard-on-late behaviour exactly, which is what makes the change bisectable
|
||
against a reported regression. The policy value is udpscope's; the mechanism is the client's.
|
||
|
||
### In `Acquisition`: reservation
|
||
|
||
On a frame with `lost > 0`, before pushing the frame's own samples, run `lost × batch` NaN samples
|
||
through the **existing** per-sample loop at `src/acquisition.cpp:553`.
|
||
|
||
That reuse is the design. `tick_` advances, so `tick % g == g - 1` keeps firing on the same phase
|
||
and the decimation grid does not shift; the group mean still lands on the mean stamp; and
|
||
`MinMaxPyramid::append` sees NaN, which its existing non-finite contract already handles as
|
||
*advances the positional count, excluded from every aggregate*. Nothing bespoke is needed for
|
||
decimation, stamping or the pyramid.
|
||
|
||
Stamps across the hole come from the active time source — exact in relative mode, the declared
|
||
cadence continued from the last good stamp in stream mode, which is the bridge at `:274-292`.
|
||
|
||
Math channels take NaN and give NaN, so a math trace keeps its gap. The trigger engine's
|
||
comparisons are false on NaN, so it cannot latch inside a hole; this is asserted by the probe
|
||
rather than assumed.
|
||
|
||
**Reservation is capped at 256 missing packets**, reusing `kGlitchPackets` and its existing
|
||
reasoning verbatim — *256 packet spans in one go is an outage, not loss*. Past the cap there is no
|
||
reservation: log once and continue as today. That cap is also what stops a ten-minute outage from
|
||
pushing billions of NaN samples through the ring and wiping all history to make room for a gap.
|
||
|
||
## Backfill
|
||
|
||
`plot::data::RingBuffer` and `MinMaxPyramid` are both strictly append-only today. Backfill needs
|
||
an amend in each, and the two belong on different threads.
|
||
|
||
`plot_host.hpp:183` calls `acquire(ScopeModel&)` from the frame path, so the **pyramid is mutated
|
||
only on the GTK main thread**, once per frame: the producer pushes into the ring and the main
|
||
thread folds what was pushed into the pyramid. That split carries straight over to the amend.
|
||
|
||
- **Ring amend — producer thread.** `RingBuffer<T>::amend(absolute, v)`, bounds-checked against
|
||
`[produced_ - capacity, produced_)`, a single relaxed store. The same race class as a push, and
|
||
an `f32` cannot tear. Measurements and export read the ring directly, so they see a repair
|
||
immediately.
|
||
- **Pyramid amend — main thread, inside `acquire()`.** The producer enqueues `(slot, value)` on an
|
||
SPSC queue; `ChannelBuffer::acquire()` drains it **after** its append pass, so every amended
|
||
index is already published.
|
||
|
||
The thread split is not a stylistic choice. A bucket is 32 bytes and is not atomic, so amending it
|
||
from the producer would let a reader see a half-updated bucket whose `sum` does not match its
|
||
`count` — a wrong mean from a structure whose readers trust `count == 0` to mean *absent*. On the
|
||
main thread that cannot happen and no seqlock is needed.
|
||
|
||
`MinMaxPyramid::amend(index, v)` folds `v` into every covering bucket: at `count == 0`,
|
||
`min = max = v`, `sum = v`, `sumsq = v²`, `count = 1`; otherwise widen `min`/`max` and accumulate
|
||
`sum`/`sumsq`, `++count`. Its documented precondition is **non-finite → finite only**, which is
|
||
what makes it purely additive and O(log n) with no rebuild. An index already dropped by
|
||
`drop_before` is a no-op.
|
||
|
||
### The time ring is never amended
|
||
|
||
The hole was pre-stamped at reservation, so a backfill changes values only. `neighbours()` keeps
|
||
the monotonic array its binary search requires and `window_x0_` stays valid. This is the reason
|
||
reservation pre-stamps rather than leaving the ring short.
|
||
|
||
### Addressing a late packet
|
||
|
||
Reservation records `(counter_first, counter_last, tick_start, slot_start, slot_count)` in a
|
||
bounded side table. A late frame's counter indexes straight into it:
|
||
|
||
```
|
||
tick = tick_start + (counter - counter_first) * batch
|
||
```
|
||
|
||
Only slots lying **wholly** inside the arrived packet's tick range are amended. A slot straddling
|
||
a hole edge went NaN when its accumulator took a NaN, and its real members were never retained, so
|
||
it stays NaN. A displayed hole is therefore at most one slot wider per edge than the true loss —
|
||
invisible at any hole size worth repairing, and it makes a boundary slot need no special case.
|
||
|
||
The side table is bounded by the 256-packet reservation cap and a maximum entry count; it is also
|
||
the source the shaded band reads.
|
||
|
||
## Surfacing
|
||
|
||
`StreamStats` gains `late_resequenced`, `late_committed` (gave up past 20 ms, or overflowed the
|
||
reorder ring) and `late_backfilled`, and finally exposes `stale_packets` — which the client has
|
||
counted since the beginning and which `src/status_text.cpp:70` has never shown, so an inversion
|
||
today has to be inferred from `packets` and `frames` diverging. All four sit in the status bar's
|
||
second tier.
|
||
|
||
The band is a new `Plot::set_invalid_spans()` in `deps/plotter`, drawn as a tinted quad behind the
|
||
trace. Unlike the `t0` caption this *is* data-anchored — it must pan and zoom with the data it
|
||
marks — so it is annotation-class, not chrome. The invariant it must respect is that it reserves
|
||
no layout space and cannot be picked, so a hole cannot move the layout the labels were measured
|
||
into.
|
||
|
||
## Verification
|
||
|
||
udpscope has no unit test suite; the only automated check is the application's own headless
|
||
self-check (`CLAUDE.md`). This adds exit-dump lines and a `UDPSCOPE_PROBE_RESEQ_MS` probe,
|
||
asserting: a reorder inside the window is resequenced with no hole; a reorder past the window
|
||
leaves a hole and then backfills it; the trigger does not latch inside a hole; a 300-packet burst
|
||
hits the cap and does not throw the stream onto the arrival clock; and relative mode's stamps are
|
||
monotone with the declared spacing.
|
||
|
||
Runs are serial and short, against the two standing baselines: four build warnings on a clean
|
||
build (two on an incremental build) and zero `Gtk-CRITICAL`. Batching several `xvfb` runs back to
|
||
back starves the mock past its 2 s `silence_timeout_s` and reads as a reconnect, which is a
|
||
measurement artefact and not a defect (`docs/claude/verification.md`).
|
||
|
||
**One gap in the mock has to be closed first.** `--reorder-every` releases the held packet
|
||
immediately after the one that overtook it, so it is ~0 ms late and would always land inside the
|
||
20 ms window. That exercises resequencing but leaves the **backfill path untestable**. The mock
|
||
needs `--reorder-delay-ms` so a packet can be released deliberately past the window.
|
||
|
||
## Documentation this lands against
|
||
|
||
- `docs/claude/invariants-data-path.md` — append-only stops being the whole truth; the amend's
|
||
precondition and its thread belong here.
|
||
- `docs/claude/invariants-plot-and-layout.md` — the band, and the layout space it must not take.
|
||
- `docs/claude/invariants-trigger-and-pinning.md` — the trigger does not latch inside a hole.
|
||
- `docs/claude/verification.md` — the new probe and dump lines, and `--reorder-delay-ms`.
|
||
- `docs/claude/build-and-run.md` — the vendored `udpsclient` diff: the reorder ring, the late
|
||
delivery replacing the discard, and the third `clamp_deadline()`.
|
||
- `README.md` and a `SPECS.md` deviation note for relative-time mode.
|
||
|
||
## Deliberately not in scope
|
||
|
||
- **Replaying math channels or the trigger over a backfilled region.** Decided against above.
|
||
- **Interpolating across an unrepaired hole.** A hole is reported, not invented. There is no
|
||
interpolation anywhere in the data path today and this does not add the first.
|
||
- **Making relative mode the default.** It is a toggle. The default stays the declared time
|
||
signal, so no existing session or run changes what it shows.
|
||
- **The `(min, max, mean, rms)` gather mode** that would fix the separate solid-band degeneration
|
||
at wide time windows. Related to the pyramid, unrelated to this; still open.
|