From d91957a2fec945ca982710cd65bf74933537cf4a Mon Sep 17 00:00:00 2001 From: Martino Ferrari Date: Thu, 15 Oct 2026 16:30:48 +0200 Subject: [PATCH] fix: a recompute that renumbered its ring, and readouts frozen under a zoom Three defects, all of them a window describing data that is not there. A math edit refilled the derived ring with `clear()`, which renumbers it from 0, leaving `produced()` short by the inputs' retained base. Since `live_window_locked()` clamps against every channel's `produced()`, that one lagging channel answered `n = 0` for the live view, every pause view and every trigger capture until a whole ring of fresh data had gone by -- the reported freeze, invisible until the ring first wraps, hence its intermittence. It now restarts with `clear_to(base)`, and `ChannelBuffer::republish_from(base)` puts the published window back: `acquire()` is incremental, so a rewrite in place published nothing and the pyramid went on describing the samples the edit replaced. The trigger's channel is an index, re-resolved by name after a renumbering -- but only in `rebuild_enabled_locked()`, so a math add or delete went through `set_math_channels()` with no restore, the index was clamped to -1, and `process()` returned at once while the panel still named the channel. The restore moves into `rebuild_channels()`, which *is* the renumbering, so no caller can forget it. And `update_measures()` suppressed its "the window moved" test while an event was held -- the one mode where a gesture is the only thing that can move it. Zooming a latched event left the pane describing the whole captured event. A pinned window needs no exemption: it lands on the range it already measured and fails the 2 % test by itself. The checks that missed all three: `UDPSCOPE_PROBE_MATHWRAP_MS` edits a graph whose ring has already wrapped, which `math_probe` structurally cannot reach; `[measure-trace]` now prints the mode, the window on screen and the window the numbers describe; and the dump's `marks_in_window` counts against the window on screen instead of against the one the marks were computed over, where it was a tautology. Co-Authored-By: Claude Opus 4.6 --- README.md | 7 + deps/plotter | 2 +- docs/claude/build-and-run.md | 12 ++ docs/claude/invariants-math-channels.md | 22 ++++ docs/claude/invariants-measurements.md | 11 ++ docs/claude/invariants-trigger-and-pinning.md | 11 +- docs/claude/probe-readings.md | 51 +++++++- docs/claude/verification.md | 47 +++++-- src/channel.cpp | 12 ++ src/channel.hpp | 10 ++ src/model.cpp | 55 +++++--- src/ui.cpp | 19 ++- src/ui_app.hpp | 10 ++ src/ui_measure.cpp | 28 +++- src/ui_probe.cpp | 122 ++++++++++++++++++ 15 files changed, 374 insertions(+), 45 deletions(-) diff --git a/README.md b/README.md index 2b9fb68..e8112c3 100644 --- a/README.md +++ b/README.md @@ -388,6 +388,13 @@ tick). The trigger is 2 % of the span, so the numbers track the window within a while the cost stays bounded by the window's motion rather than by the frame rate; one pass copies the visible samples of each *channel* once, however many measurements read it. +"The window moved" is asked in **every** mode, including a held trigger event — which is the +one mode where a zoom or a pan is the only thing that can move it. Held views were once +exempted, on the grounds that a pinned window must not recompute every frame, and the result +was a pane that kept describing the whole captured event while you zoomed into a slice of it. +A pinned window needs no exemption: it lands on exactly the range it already measured and +fails the 2 % test by itself. + **Show on plot** marks the result on the row that plots the signal: a ruler at the level (`top`/`bottom`), both rulers at the extremes (`min`/`max`), the pulse's or the cycle's two edge times as vertical markers. The marks are **readouts, not instruments**: a fixed pool of diff --git a/deps/plotter b/deps/plotter index 9790bec..3ad02e5 160000 --- a/deps/plotter +++ b/deps/plotter @@ -1 +1 @@ -Subproject commit 9790becb33b1eec2a644c9281fa66f40ae5aec0c +Subproject commit 3ad02e5120b5e2b8fb0a6d1a3ff4661b1d07325c diff --git a/docs/claude/build-and-run.md b/docs/claude/build-and-run.md index 81fd456..f52482d 100644 --- a/docs/claude/build-and-run.md +++ b/docs/claude/build-and-run.md @@ -209,6 +209,18 @@ adapter `PlotArea`'s **figure mode** — `set_figure()`, one `input::Controller` per row, nearest-row event routing, a figure-wide crosshair — plus `PlotArea::row_at()` for naming the row a pointer is over. +`plot::data::RingBuffer` carries two writes a plain SPSC ring has no use for and this app +does: `amend(absolute, v)` rewrites a retained slot when a late packet's data finally +arrives, and `clear_to(absolute)` empties a ring and **resumes its count at a given +origin**. The second exists because a math channel's ring is recomputed from its inputs' +*retained* window: `clear()` would restart it at 0 and leave every absolute index — +`base()`, `produced()`, `copy_from()` — short by that window's base, which is the +desynchronisation `live_window_locked()` then reads as an empty window (see +[invariants-math-channels.md](invariants-math-channels.md)). Its guards are +`ring_clear_to_resumes_the_absolute_count` — a derived ring refilled from a source's retained +window must come back with the source's own `produced()`/`base()` — and +`ring_clear_to_zero_is_clear`. + The render-quality work went there for the same reason: `kTextFS` now **filters** the glyph atlas (a 2×2 bilinear clamped to the instance's own cell) instead of point-sampling it, and `LineProgram::set_snap()` / `u_snap` rounds an axis-aligned segment's ink onto diff --git a/docs/claude/invariants-math-channels.md b/docs/claude/invariants-math-channels.md index f8c8794..4540b0f 100644 --- a/docs/claude/invariants-math-channels.md +++ b/docs/claude/invariants-math-channels.md @@ -323,6 +323,28 @@ the assertion: `finite` must be about `M` minus the hole share, and `finite=0` b `[math-probe] graph edit: stored_slots N -> M` reads it: M must be at least N -- the store keeps filling while the edit runs -- and never 0. +- **A recompute rewrites a ring *in place*, and both of the ring's counters have to survive it.** + `recompute_math_locked()` replays the inputs' **retained** window, which begins at the time ring's + `base()` and not at zero, so an emptied derived ring is restarted with `clear_to(base)` and never + `clear()`. With `clear()` the refill renumbered the channel from 0 and left its `produced()` short by + exactly `base` -- and `live_window_locked()` clamps its window against *every* channel's `produced()`, + so that one lagging channel answered `n = 0` for the live view, every pause view **and** every trigger + capture, until a whole ring of fresh data had gone by. That is the "a math edit freezes acquisition, + and the trigger stops firing on a channel that is not even derived" report, and it is **invisible + before the ring first wraps** (`base == 0`, where the two numberings coincide), which is why it reads + as intermittent and uptime-dependent. The dump's `math '': N samples vs M time slots` is the + reading -- both numbers are `produced()`, so a mismatch *is* this defect -- but only on a run whose + ring has wrapped: give it a small `--buffer-mb`. `UDPSCOPE_PROBE_MATHWRAP_MS` is the probe that + arranges exactly that, and it exists because **`math_probe` structurally cannot** reach this state: + every edit that probe makes follows a structural change, which replans the buffer and restarts every + ring at absolute 0, where both halves of this are identities. + The published window is the second counter. `acquire()` is incremental, so a rewrite that leaves + `produced()` where it was publishes **nothing** and the pyramid -- what the renderer and every + measurement read -- goes on describing the samples the edit replaced. `ChannelBuffer::republish_from(base)` + is what puts it back, and it takes the ring's base rather than 0 because `invalidate()` would make the + next `acquire()` walk every absolute index ever produced (an hour at 1 MS/s is 3.6 G appends) to publish + one window. + - **A math channel whose source is switched off is switched off with it.** `Source` resolves to a *channel*, so a derived channel cannot be computed from a signal nobody acquires: left enabled it would silently compute nothing, which is what "all math signals depending on it do not compute anymore" looked like from outside. diff --git a/docs/claude/invariants-measurements.md b/docs/claude/invariants-measurements.md index d6e4844..ef60113 100644 --- a/docs/claude/invariants-measurements.md +++ b/docs/claude/invariants-measurements.md @@ -20,6 +20,17 @@ Part of [CLAUDE.md](../../CLAUDE.md). These are rules that cost a defect to lear own -- `top`/`bottom`/`amplitude` take a second read of that same **bucket** list and never of the window (the flat-level invariant below). **Adding a type must not add a pass** -- that is what `compute_measure()`'s `stats` argument is for. + **The "it moved" test is asked in every mode, a held event included.** Suppressing it while + held (`moved = ... && !held`) is the shape the pinned-window fix first took, and it is wrong + for the one mode where a *gesture* is the only thing that can move the window: a latched event + zoomed forty times over left the pane describing the whole 2 s event with `passes` frozen, + which is the reported "measurements move when zooming or panning on data" -- the numbers stand + still while the data under them moves. Nothing is needed for the pinned case anyway, because a + window that is merely re-pinned satisfies `swin == measure_window` exactly and fails the move + test on its own; a pause is the standing proof, since `display_mode()` reports `Paused` rather + than `Held` for one and the statistics probe already requires `n` to advance by at most one + across a two-second pause. The held/live state is still a recompute of its own, so a mode + change is not missed. The marks' *ink* is set where they are created (`install()` in `plot_host.cpp`): 0.75 px and dashed 4/4 through `Plot::set_cursor_stroke()`, which is per cursor because the trigger marker (also a cursor) keeps the solid `line_width_px` stroke. A new measure mark must take that diff --git a/docs/claude/invariants-trigger-and-pinning.md b/docs/claude/invariants-trigger-and-pinning.md index bf72173..4e779b0 100644 --- a/docs/claude/invariants-trigger-and-pinning.md +++ b/docs/claude/invariants-trigger-and-pinning.md @@ -52,8 +52,15 @@ Part of [CLAUDE.md](../../CLAUDE.md). These are rules that cost a defect to lear `Acquisition::on_data()` on **every packet** out of `model_.trigger()`. `TriggerConfig::channel` is an index into `channels()`, and that index is not stable: every rebuild of the channel list renumbers it — enabling a signal that sorts before the trigger's own is enough — and - `ScopeModel::set_signal_enabled()` re-resolves the trigger by name and writes the *new* index - back. Keying the reset on the index therefore made any such rebuild abandon the capture **and + **`ScopeModel::rebuild_channels()` re-resolves the trigger by name and writes the *new* index + back, for every caller.** It lives there and not at the call sites because the function *is* the + renumbering, and a caller that forgot is a trigger that silently stops: the name-based restore + used to be in `rebuild_enabled_locked()` alone, so adding or removing a math channel went through + `set_math_channels()`'s structural path with no restore at all, `rebuild_locked()` clamped the + now-wrong index to `-1`, and `process()` returns at once on a channel of `-1` while the panel went + on naming the channel. That is the second half of the "it sometimes does not trigger, even with + the trigger on a non-math channel" report. Keying the reset on the index made any such rebuild + abandon the capture **and set no hold-off**, so the next edge inside the post window became t0: that is the "the trigger re-triggers during a capture and the hold-off does not apply" defect, and it needed no glitch and no user action beyond enabling a channel. The name changes when the source genuinely diff --git a/docs/claude/probe-readings.md b/docs/claude/probe-readings.md index 19a76ba..d08a2ef 100644 --- a/docs/claude/probe-readings.md +++ b/docs/claude/probe-readings.md @@ -259,13 +259,27 @@ Two lines of the self-check output are assertions in disguise: **The reading that must hold is `marks_in_window=K/K`**: every *time* mark inside the window on screen. Both ranges are on the **stream's** clock, which is the space `measure_window` and the time marks are stored in -- a live axis is a fixed `[-window, 0]`, so comparing display x - would read no drift whatever the data did. + would read no drift whatever the data did. `win` is the window as of the last **tick** + (`App::measure_view_window`) and is deliberately not read back here: the dump runs from the + destroy handler, by which point the exit probe has switched the trigger on, and + `stream_at_x0()` then answers for a mode no frame was ever drawn in -- which read as a 45000 % + drift on a run whose every frame was exact. And the count is against `win`, not against + `measure_window`, which contains every mark by construction: read that way it was a tautology + that printed `2/2` while the pane described a window a forty-fold zoom away from the one + displayed. On a window **shorter than the refresh interval** the count rides the same sawtooth + `drift` does -- the 50 ms window the zoom sweep leaves behind can lag far enough that *every* + time mark is off screen, so `0/K` there is read beside `drift` and not alone. The strict + reading belongs to the documented 2 s run, and to a **held** window in any run: a latched + event does not scroll, so `[measure-trace]` must show `for=` equal to `swin=` exactly for as + long as the mode stays `held`, zoom and pan included. `drift` itself is a **single sample of a sawtooth** and is not an assertion on its own: a live view scrolls between recomputes, so it rises to one `kMeasureRefreshUs` of scroll and drops to zero at each pass. The figure is `(|Δmin| + |Δmax|) / span`, so a pure scroll - counts **twice** and the top of the sawtooth is `2 × refresh / window`: ~10 % on a 2 s window - at the 100 ms cadence, and a few *hundred* percent on a window shorter than the interval (a - 50 ms window reads 254 % for 63.6 ms of scroll, and its ceiling is 400 %). Compute the ceiling + counts **twice** and the top of the sawtooth is `2 × (refresh + one frame) / window` -- the + frame term is not noise under `xvfb`, where a frame costs 30--70 ms: ~10 % on a 2 s window at + the 100 ms cadence with instant frames, and **15.8 %** measured on the documented run, which is + one interval plus one 58 ms frame. On a window shorter than the interval it is a few *hundred* + percent (a 50 ms window reads 254 % for 63.6 ms of scroll, and its ceiling is 400 %). Compute the ceiling for the window the run actually used before calling a figure large. A `0.0000%` is a dump that landed just after a pass, not a better state. Read it against the recompute count: *more* recomputes with a *higher* drift sample is the sawtooth, while a drift that exceeds one @@ -1947,6 +1961,35 @@ channels and an edit does not move it, so the store survives: the count *grows* while the edit runs) and never falls to 0, which is what a rebuild would do. Every channel then still reads `N samples vs M time slots` with `N == M`, so no channel was left behind either. +## `[mathwrap-probe]` + +``` +[mathwrap-probe] installed 'math wrap' on 'sine': channel=1 +[mathwrap-probe] wrapped: base=864 slots=66400 (want base > 0) +[mathwrap-probe] after edit: produced=72400 slots=72400 (want equal) y=[-0.5000,0.5000] (want +-0.5, was +-2.0000) +``` + +`UDPSCOPE_PROBE_MATHWRAP_MS`, and it must be run with a **small `--buffer-mb`** and given time to +saturate it: `--buffer-mb 1` against `tools/mock_udpstreamer.py --rate 20000 --batch 200`, with +`UDPSCOPE_PROBE_MATHWRAP_MS=1500 UDPSCOPE_EXIT_AFTER_MS=25000`, reads the lines above. It installs one +`Gain` graph of 2 over the mock's unit sine, polls (100 ms) until the ring has **wrapped**, and only then +edits the gain to 0.5 — the same channel list with one node changed, which is the in-place recompute and +not a replan. + +`base > 0` on the second line is the precondition and not a result: at `base == 0` both readings below are +identities, so **`base=0` means the run tested nothing** — it was too short, or the buffer too large. This +is also why `math_probe` cannot stand in for this one: every edit it makes follows a structural change, +which replans the buffer and restarts every ring at absolute 0. + +The third line is one reading per half of `recompute_math_locked()`. `produced` against `slots` is the +derived ring's absolute count: short by exactly `base` is the renumbering `clear_to()` exists to prevent, +and `live_window_locked()` clamps against *every* channel's `produced()`, so that one lagging channel is +`n = 0` for the live view, every pause view and every trigger capture — the reported freeze. `y` is the +**pyramid's**, so it describes the published window rather than the samples, and the edit *shrinks* the +gain for that reading to exist: a pyramid left unpublished still describes the replaced ±2 samples, a +window holding both quotes the larger, and a *growing* gain would therefore read correct either way. +`y=[-2,2]` here is `republish_from()` missing. + ## `[noise-probe]` ``` diff --git a/docs/claude/verification.md b/docs/claude/verification.md index c17bf0f..2d2ed4b 100644 --- a/docs/claude/verification.md +++ b/docs/claude/verification.md @@ -96,11 +96,20 @@ precedent, not an exception. overshoot, kept by the extremes and excluded by the levels. The per-type values, the reason for each, and the cost of the histogram are in [invariants-measurements.md](invariants-measurements.md). - `[measure-trace] view=[..] =... marks=N level:V/time:V...` is a repeating line - (250 ms) from the probe: the pane's numbers beside the *current* window and the marks actually - placed, which is the only way a stale result or a mark that should not be drawn shows up as a - number. It must never print `marks=0` while measurements with `show_on_plot` are on and the - data is there -- that was the rulers blinking off at a mode change. + `[measure-trace] mode=M view=[..] s0=S swin=[..] for=[..] passes=N =... marks=N + level:V/time:V...` is a repeating line (250 ms) from the probe: the pane's numbers beside the + *current* window and the marks actually placed, which is the only way a stale result or a mark + that should not be drawn shows up as a number. `swin` is `view` moved onto the stream's clock by + `s0` (`stream_at_x0()`), and `for` is the window the printed numbers were computed over, in the + same clock -- so a reading that moved while `view` stood still is a change of `s0` (a new + snapshot under the same mode) and not a stale pass, and only these three fields tell the two + apart. `passes` beside them says whether a recompute answered the move. **While `mode=held`, + `for` must equal `swin` exactly**, through any number of zooms and pans: a latched event does + not scroll, so there is nothing for a recompute to be late for. That reading is where + the measurements standing still under a gesture was found -- `for=[0.47..2.47]` against a + `swin=[1.417..1.467]` with `passes` frozen, the pane describing the whole 2 s event while a 50 ms + slice of it was on screen. The line must also never print `marks=0` while measurements with + `show_on_plot` are on and the data is there -- that was the rulers blinking off at a mode change. `measure cost: N recomputes, X ms total, Y ms each, data D ms (P%), covered=C buckets=B raw=R (F%), N ns/covered sample` follows it: **read it against a run with `UDPSCOPE_PROBE_MEASURE_MS` unset**, because a frame mean cannot attribute a cost. The last @@ -283,9 +292,14 @@ as distinct from the fault injection below, which exercises ingest: conditions for the first four: the mock at `--rate 10000 --batch 100 --freq 5`, udpscope with no extra options, under `xvfb-run` with `UDPSCOPE_EXIT_AFTER_MS=8000 UDPSCOPE_ENABLE_ALL_MS=600 UDPSCOPE_EXIT_DUMP=1 - UDPSCOPE_PROBE_RESEQ_MS=5000`. Delete `~/.config/udpscope/127.0.0.1_8000.toml` first: a - session left by an earlier run restores its math channel, and five channels instead of four - change `channels=`, `span=` and every count derived from them. + UDPSCOPE_PROBE_RESEQ_MS=5000`. Move `~/.config/udpscope/127.0.0.1_8000/` aside first (the + per-endpoint session is a *directory* now -- `active` plus one `.toml` per named configuration; + a bare `127.0.0.1_8000.toml` is a pre-migration leftover): a session left by an earlier run + restores its math channel, and five channels instead of four change `channels=`, `span=` and + every count derived from them. It restores the **window and the measurements** too, which is + the subtler trap: a run after a zoom-sweep probe starts at `window_s = 0.05` with the previous + runs' measurements still in the pane, so `measures window:` then reports a short-window + sawtooth (144 %, marks off screen) on a run that asked for none of it. | case | mock extra flags | key reading | | --- | --- | --- | @@ -328,6 +342,14 @@ as distinct from the fault injection below, which exercises ingest: and **`finite=0` beside a non-zero `hole_slots` is the defect** (a biquad that integrated the hole's NaN, permanently). The filters' `sd` must stay near the no-drop value; run the same command without `--drop-every` as the control, where every `finite` reads 4000. +- **A math edit on a ring that has already wrapped.** `UDPSCOPE_PROBE_MATHWRAP_MS` — installs one + `Gain` graph over the mock's sine, waits for the ring to **saturate**, and only then edits the gain, + which is the one state `UDPSCOPE_PROBE_MATH_MS` cannot reach (each of its edits follows a structural + change, and a replan restarts every ring at absolute 0). It therefore needs a **small buffer and a long + run**: `--buffer-mb 1` against `--rate 20000 --batch 200`, with + `UDPSCOPE_PROBE_MATHWRAP_MS=1500 UDPSCOPE_EXIT_AFTER_MS=25000`. `base > 0` on its second line is the + precondition — **`base=0` means the run tested nothing** — and the third line's `produced == slots` and + `y=±0.5` are the two assertions; both are in [probe-readings.md](probe-readings.md). - `UDPSCOPE_PROBE_CONFIGS_MS` — saves the running session as a configuration named `probe` with a note, through `save_configuration_as()` (the same entry point the dialog's button uses), then reads it back off disk. Reading: `save-as=1`; `names=` gains `probe`; `active=probe`; and the @@ -615,7 +637,7 @@ Four readings, all off the once-a-second `[udpscope] trig engine:` line: Before the fix the same run read a fallback warning, `history` collapsing 4.03 → 0.95, `now_t` jumping 5.040 → 16.040, and two latches **1.2 s** apart. Note the window here comes from the saved session for that port; the figures above are the default 2 s window (50 % pre), so move -`~/.config/udpscope/_.toml` aside to reproduce them. +`~/.config/udpscope/_/` aside to reproduce them. The other two are **packet loss** (`--drop-every N`, `--drop-burst K`) and **wire inversion** (`--reorder-every N`). Loss withholds a datagram the counter and the stream clock have already @@ -759,9 +781,10 @@ have run. `--spike-every`/`--spike-amp`/ The three lines and what each must read are in `probe-readings.md`. `UDPSCOPE_PROBE_NOISE_MS`, `UDPSCOPE_PROBE_MEASURE_MS`, `UDPSCOPE_PROBE_MEASURES_MS`, -`UDPSCOPE_PROBE_BANDAVG_MS` and `UDPSCOPE_PROBE_MATH_MS` must **not** be scheduled -in the same run. The noise, measurement, measurements and band-average probes all push onto +`UDPSCOPE_PROBE_BANDAVG_MS`, `UDPSCOPE_PROBE_MATH_MS` and `UDPSCOPE_PROBE_MATHWRAP_MS` must **not** be +scheduled in the same run. The noise, measurement, measurements and band-average probes all push onto `app.measures`, and the later one would read -the earlier one's rows as its own; the noise and math probes both write `app.math`, so the later one's graph +the earlier one's rows as its own; the noise, math and math-wrap probes all write `app.math`, so the later +one's graph lands on top of the earlier one's and both sets of readings describe a state neither probe set up. The noise probe carries its own measurement legs (see `probe-readings.md`), so nothing is lost by keeping them apart. diff --git a/src/channel.cpp b/src/channel.cpp index aa8fd9e..6d57410 100644 --- a/src/channel.cpp +++ b/src/channel.cpp @@ -78,6 +78,18 @@ void ChannelBuffer::invalidate() noexcept { pending_amends_.clear(); } +void ChannelBuffer::republish_from(u64 absolute) noexcept { + published_ = absolute; + window_x0_ = 0.0; + pending_amends_.clear(); + // `clear()` leaves the pyramid empty at absolute 0, and `drop_before()` past the end + // restarts an empty window at the index it is given -- which is the only way to say + // "nothing is published, and the next append is sample `absolute`". The no-op guard is + // for `absolute == 0`, where `clear()` has already said exactly that. + pyramid_.clear(); + if (absolute > 0) pyramid_.drop_before(absolute); +} + f64 ChannelBuffer::time_of(u64 absolute_index) const noexcept { // Channel sample `j` is produced on the tick that also pushes time slot // `j*stride + stride - 1` (see `Acquisition::on_data`): the producer stamps the diff --git a/src/channel.hpp b/src/channel.hpp index 1d741c0..4e9eeb4 100644 --- a/src/channel.hpp +++ b/src/channel.hpp @@ -48,6 +48,16 @@ class ChannelBuffer final : public plot::data::Series { void acquire() noexcept; /// Consumer thread: forgets the published window (after a rebind or a reset). void invalidate() noexcept; + /// Consumer thread: drops the published window and re-publishes it from absolute sample + /// `absolute` on the next `acquire()`. + /// + /// For a ring whose contents were *rewritten in place* rather than appended to -- a math + /// channel recomputed over its inputs' retained window. `acquire()` is incremental, so + /// a rewrite that leaves `produced()` where it was publishes nothing and the pyramid goes + /// on describing the samples the edit replaced. `invalidate()` would do it too, but it + /// restarts at 0 and `acquire()` would then walk every absolute index ever produced; this + /// restarts at the oldest index the ring still holds, so the rebuild is one window. + void republish_from(u64 absolute) noexcept; /// Producer thread, under `ScopeModel::mutex()`: records that absolute sample `absolute` /// has been backfilled, for `acquire()` to fold into the pyramid. diff --git a/src/model.cpp b/src/model.cpp index a0383de..60a987b 100644 --- a/src/model.cpp +++ b/src/model.cpp @@ -449,6 +449,14 @@ void ScopeModel::recompute_math_locked(const std::vector& affected) { const std::size_t slots = static_cast(store.time().size()); if (slots == 0) return; + // The refill replays the inputs' *retained* window, which starts at this absolute index and not at + // zero, so an emptied derived ring has to resume its count there: `clear()` would renumber it from 0 + // and leave its `produced()` short by `base` for a whole ring's worth of fresh data. `live_window_locked()` + // clamps the window against every channel's `produced()`, so that one lagging channel collapses the + // live view, every pause view and every trigger capture to nothing -- the "acquisition freezes after a + // math edit" defect, invisible until the ring has wrapped once (`base == 0` before that). + const std::size_t base = static_cast(store.time().base()); + std::vector derived(channels_.size(), false); for (std::size_t m = 0; m < math_.size(); ++m) { const int channel = m < math_channel_.size() ? math_channel_[m] : -1; @@ -456,14 +464,18 @@ void ScopeModel::recompute_math_locked(const std::vector& affected) { derived[static_cast(channel)] = true; // Only the affected channels start again. The rest keep their samples *and* their runtimes: their graphs // are untouched, so the state they hold is the answer to the inputs they have always had. - if (affected[m]) store.values(static_cast(channel)).clear(); + if (!affected[m]) continue; + store.values(static_cast(channel)).clear_to(base); + // And the published window with them: this is a rewrite in place, so `produced()` lands back where it + // started and the incremental `acquire()` would publish nothing at all -- leaving the pyramid, which is + // what the renderer and every measurement read, describing the samples the edit just replaced. + store.channel(static_cast(channel)).republish_from(base); } // One bulk copy per channel per chunk, through the ring's own reader. A derived channel is fed a zero: // `math_apply_locked()` overwrites its value as it walks the list in order, and reading its half-rebuilt // ring here would read out of range. constexpr std::size_t kChunk = 1u << 14; - const std::size_t base = static_cast(store.time().base()); std::vector> inputs(channels_.size()); for (std::size_t c = 0; c < channels_.size(); ++c) { if (!derived[c]) inputs[c].resize(kChunk); @@ -482,10 +494,22 @@ void ScopeModel::recompute_math_locked(const std::vector& affected) { math_apply_locked(slot, &affected, /*timed=*/false); } } - // The pyramid is the consumer's and the tick publishes it; the samples are written here. + // The pyramid is the consumer's and the tick publishes it (`republish_from()` above put its window + // back at `base` so that publish is one window's worth of work); the samples are written here. } void ScopeModel::rebuild_channels() { + // The trigger names a channel by its display name, which is stable across the renumbering this + // function *is*; the index is not. Captured and restored here rather than at the call sites + // because every caller renumbers and every caller needs it: adding a math channel appends a + // signal and shifts nothing, but removing one, or a CONFIG that reorders the stream's own + // signals, moves every index after it. `rebuild_locked()` then only clamps an out-of-range + // index to -1, and `TriggerEngine::process()` returns immediately on a channel of -1 -- so the + // panel went on naming the channel while nothing could ever fire again. + const std::string trigger_name = + (trigger_.channel >= 0 && trigger_.channel < static_cast(channels_.size())) + ? channels_[static_cast(trigger_.channel)].name + : std::string(); channels_.clear(); // Which channel each math channel ended up with, by name: a disabled one has none, and the // producer must not push a derived sample into another channel's ring. @@ -524,6 +548,15 @@ void ScopeModel::rebuild_channels() { } channels_.push_back(std::move(ch)); } + + // A name that is no longer a channel -- its signal was disabled, or the stream stopped + // declaring it -- leaves the trigger unassigned, which is a wait rather than a silent + // mis-aim; re-enabling the signal brings it back here. + trigger_.channel = -1; + if (trigger_name.empty()) return; + for (std::size_t i = 0; i < channels_.size(); ++i) { + if (channels_[i].name == trigger_name) trigger_.channel = static_cast(i); + } } void ScopeModel::rebuild_locked() { @@ -639,22 +672,8 @@ bool ScopeModel::enable_locked(std::size_t signal, bool on) { } void ScopeModel::rebuild_enabled_locked() { - // The trigger names a channel by its display name, which is stable across - // renumbering: when its own signal is disabled the channel vanishes (the trigger - // then waits, and comes back when the signal is re-enabled). Read before - // `rebuild_channels()`, which is what renumbers. - const int old_trigger = trigger_.channel; - const std::string trigger_name = - (old_trigger >= 0 && old_trigger < static_cast(channels_.size())) - ? channels_[static_cast(old_trigger)].name - : std::string(); + // `rebuild_channels()` carries the trigger's channel across the renumbering by name. rebuild_channels(); - trigger_.channel = -1; - if (!trigger_name.empty()) { - for (std::size_t i = 0; i < channels_.size(); ++i) { - if (channels_[i].name == trigger_name) trigger_.channel = static_cast(i); - } - } rebuild_locked(); generation_.fetch_add(1, std::memory_order_release); } diff --git a/src/ui.cpp b/src/ui.cpp index 650aca3..cf095e4 100644 --- a/src/ui.cpp +++ b/src/ui.cpp @@ -5809,6 +5809,9 @@ int run_ui(int argc, char** argv, const Config& cfg, ScopeModel& model, Acquisit if (const char* env = g_getenv("UDPSCOPE_PROBE_MATH_MS")) { g_timeout_add(static_cast(std::atoi(env)), math_probe, &app); } + if (const char* env = g_getenv("UDPSCOPE_PROBE_MATHWRAP_MS")) { + g_timeout_add(static_cast(std::atoi(env)), mathwrap_probe, &app); + } if (const char* env = g_getenv("UDPSCOPE_PROBE_MEASURE_MS")) { g_timeout_add(static_cast(std::atoi(env)), measure_probe, &app); } @@ -6476,17 +6479,23 @@ int run_ui(int argc, char** argv, const Config& cfg, ScopeModel& model, Acquisit // Both on the **stream's** clock, which is the space `measure_window` and the time marks // are stored in: a live axis is a fixed `[-window, 0]`, so comparing display x would read // no drift whatever the data did (see `App::measure_window`). - const double s0 = model.stream_at_x0(); - const RowSet::Row& r0 = rows.row(0); - const plot::Range view = r0.plot != nullptr ? r0.plot->x_range(r0.axis_x) : plot::Range{}; - const plot::Range win{view.min + s0, view.max + s0}; + // + // `win` comes from the last **tick** (`App::measure_view_window`) and is not read back here: + // this runs from the destroy handler, where the exit probe has already switched the trigger + // on, and `stream_at_x0()` then answers for a mode no frame was drawn in. + const plot::Range win = app.measure_view_window; const double span = app.measure_window.span(); int in_window = 0; int marks = 0; + // Against `win`, the window **on screen** -- not against `measure_window`, which is the + // window the mark was computed over and so contains it by construction. Read that way the + // count was a tautology: it printed `2/2` while the pane's numbers described a window 11 s + // and a forty-fold zoom away from the one displayed, which is how a held event that never + // recomputed under a gesture stayed invisible to every run of this check. for (const RowSet::MeasureMark& mk : app.measure_marks) { if (mk.level) continue; ++marks; - if (mk.value >= app.measure_window.min && mk.value <= app.measure_window.max) ++in_window; + if (mk.value >= win.min && mk.value <= win.max) ++in_window; } // Whether the marks the app placed are *unselectable* -- the property the pointer's hit // test respects. Read back out of the plot, so this is the app's own setting and not diff --git a/src/ui_app.hpp b/src/ui_app.hpp index 132719d..969e593 100644 --- a/src/ui_app.hpp +++ b/src/ui_app.hpp @@ -487,6 +487,13 @@ struct App { plot::Range measure_window{}; bool measure_window_valid = false; bool measure_was_held = false; + /// The window **on screen**, in the same stream seconds, as of the last *tick* -- which is the + /// only thing `measure_window` is comparable with. Captured here rather than read back in the + /// dump for the reason every other dump field is (see verification.md): the dump runs from the + /// destroy handler, after the exit probe has switched the trigger on, and `stream_at_x0()` by + /// then answers for a mode no frame was ever drawn in -- which read as a 45000 % drift on a run + /// whose every frame was exact. + plot::Range measure_view_window{}; /// What the two walks over one channel's visible window produced, shared by every measurement /// on that signal: the window's statistics, and one accumulator per *distinct level* any of /// them asked for. Several measurements on one signal must cost two walks, not two each -- and @@ -1699,6 +1706,9 @@ gboolean trig_trace_probe(gpointer user); gboolean eventzoom_wait(gpointer user); gboolean eventzoom_probe(gpointer user); gboolean math_probe(gpointer user); +gboolean mathwrap_probe(gpointer user); +gboolean mathwrap_probe_wait(gpointer user); +gboolean mathwrap_probe_report(gpointer user); gboolean measure_probe(gpointer user); gboolean measure_trace(gpointer user); gboolean stats_probe_report(gpointer user); diff --git a/src/ui_measure.cpp b/src/ui_measure.cpp index 626530c..b0bfc58 100644 --- a/src/ui_measure.cpp +++ b/src/ui_measure.cpp @@ -284,8 +284,19 @@ void update_measures(App& app, bool force, bool accumulate) { const double s0 = app.model->stream_at_x0(); const plot::Range view = r0.plot->x_range(r0.axis_x); const plot::Range swin{view.min + s0, view.max + s0}; + app.measure_view_window = swin; const bool held = app.model->display_mode() == DisplayMode::Held; - const bool moved = app.measure_window_valid && !held && + // "It moved" is asked in **every** mode, a held event included. This test used to carry a + // `&& !held`, and a held event is precisely the mode where a *gesture* is the only thing that + // can move the window: with it, zooming a latched event forty times over left the pane + // describing the whole 2 s event (`for=[0.47..2.47]` against a `swin=[1.417..1.467]`, `passes` + // frozen), which is what "the measurements move when zooming or panning" was -- the numbers and + // the time marks belonged to a window the plot had left. The term was aimed at a *pinned* + // window recomputing every frame, and nothing is needed for that: a settled window satisfies + // `swin == measure_window` exactly and so reads `moved == false` on its own. A pause is the + // standing proof, since `display_mode()` reports `Paused` rather than `Held` for one and the + // statistics probe already requires `n` to advance by at most one across a two-second pause. + const bool moved = app.measure_window_valid && std::fabs(swin.min - app.measure_window.min) + std::fabs(swin.max - app.measure_window.max) > 0.02 * std::max(1.0e-12, swin.span()); @@ -638,8 +649,19 @@ gboolean measure_trace(gpointer user) { RowSet::Row& r0 = a->rows->row(0); if (r0.plot == nullptr) return G_SOURCE_REMOVE; const plot::Range view = r0.plot->x_range(r0.axis_x); - std::string line = "[measure-trace] view=[" + std::to_string(view.min) + ".." + - std::to_string(view.max) + "]"; + // The window on screen *and* the window the numbers describe, both on the stream's clock, with + // the offset that converts between them: a reading that moved without the view moving is a + // change of `s0` (a new snapshot under the same mode), and only these three fields tell the two + // apart. `passes` beside them says whether a recompute answered it. + const double s0 = a->model->stream_at_x0(); + std::string line = "[measure-trace] mode=" + + std::string(display_mode_name(a->model->display_mode())) + " view=[" + + std::to_string(view.min) + ".." + std::to_string(view.max) + "] s0=" + + std::to_string(s0) + " swin=[" + std::to_string(view.min + s0) + ".." + + std::to_string(view.max + s0) + "] for=[" + + std::to_string(a->measure_window.min) + ".." + + std::to_string(a->measure_window.max) + "] passes=" + + std::to_string(a->measure_passes); for (std::size_t i = 0; i < a->measures.size() && i < a->measure_results.size(); ++i) { const MeasureResult& r = a->measure_results[i]; line += " " + std::string(measure_type_name(a->measures[i].type)) + "=" + diff --git a/src/ui_probe.cpp b/src/ui_probe.cpp index f243d12..99777e9 100644 --- a/src/ui_probe.cpp +++ b/src/ui_probe.cpp @@ -1170,6 +1170,122 @@ gboolean math_probe(gpointer user) { return G_SOURCE_REMOVE; } +// The one state `math_probe` structurally cannot reach: an edit of a graph whose ring has already +// *wrapped*. Every edit that probe makes follows a structural change, which replans the buffer and +// restarts every ring at absolute 0 -- and at `base == 0` the recompute's two renumbering bugs are +// both identities. Run it with a small `--buffer-mb` and give it time to saturate; `base=0` in its +// second line means the run was too short or the buffer too large to have read anything. +int g_mathwrap_index = -1; +double g_mathwrap_y_before = 0.0; +int g_mathwrap_polls = 0; + +gboolean mathwrap_probe_report(gpointer user) { + App* a = static_cast(user); + if (a->window == nullptr || g_mathwrap_index < 0) return G_SOURCE_REMOVE; + u64 produced = 0; + u64 slots = 0; + plot::Range y{0.0, 0.0}; + { + std::lock_guard lock(a->model->mutex()); + const std::size_t idx = static_cast(g_mathwrap_index); + if (idx >= a->model->channel_count()) return G_SOURCE_REMOVE; + produced = a->model->live().values(idx).produced(); + slots = a->model->live().time().produced(); + y = a->model->live().channel(idx).y_bounds(); + } + // Two readings, one per half of the recompute. `produced` against `slots` is the ring's absolute + // count: short by `base` is a channel the live window clamps everything else down to, which is the + // reported freeze. `y` is the *pyramid's*, so it reads the published window rather than the samples, + // and the edit **shrinks** the gain (2 -> 0.5) for that reading to exist at all: a pyramid that was + // not republished still describes the replaced +-2 samples, and a window holding both would quote + // the larger -- so a growing gain reads correct either way. + std::fprintf(stderr, + "[mathwrap-probe] after edit: produced=%llu slots=%llu (want equal) y=[%.4f,%.4f] " + "(want +-0.5, was +-%.4f)\n", + static_cast(produced), static_cast(slots), + y.min, y.max, g_mathwrap_y_before); + std::fflush(stderr); + return G_SOURCE_REMOVE; +} + +gboolean mathwrap_probe_wait(gpointer user) { + App* a = static_cast(user); + if (a->window == nullptr || a->math.empty()) return G_SOURCE_REMOVE; + u64 base = 0; + u64 slots = 0; + { + std::lock_guard lock(a->model->mutex()); + base = a->model->live().time().base(); + slots = a->model->live().time().produced(); + const std::size_t idx = static_cast(g_mathwrap_index); + if (g_mathwrap_index >= 0 && idx < a->model->channel_count()) { + g_mathwrap_y_before = a->model->live().channel(idx).y_bounds().max; + } + } + // Polled rather than timed: the wrap is a function of the rate and of `--buffer-mb`, and a fixed + // delay that fell short would print a passing line having tested nothing. + if (base == 0 && ++g_mathwrap_polls < 200) return G_SOURCE_CONTINUE; + std::fprintf(stderr, "[mathwrap-probe] wrapped: base=%llu slots=%llu (want base > 0)\n", + static_cast(base), static_cast(slots)); + std::fflush(stderr); + // The edit itself: the *same* channel list with one node's gain changed, which is the in-place + // path -- `set_math_channels()` replans only when the names or the enables moved. + for (MathNode& n : a->math.front().nodes) { + if (n.op == MathOp::Gain) n.gain = 0.5; + } + apply_math_channels(*a); + // After a tick, because the pyramid half of the reading is published by one. + g_timeout_add(300, mathwrap_probe_report, a); + return G_SOURCE_REMOVE; +} + +gboolean mathwrap_probe(gpointer user) { + App* a = static_cast(user); + if (a->window == nullptr) return G_SOURCE_REMOVE; + // Its own signal and its own graph: a probe must not depend on another's side effects, and this one + // needs a source whose extremes are known (a unit sine doubled reads +-2) for the pyramid reading. + { + const std::vector& signals = a->model->signals(); + for (std::size_t i = 0; i < signals.size(); ++i) { + if (signals[i].name == "sine") a->model->set_signal_enabled(i, true); + } + } + std::string name; + { + std::lock_guard lock(a->model->mutex()); + if (a->model->channels().empty()) return G_SOURCE_REMOVE; + name = a->model->channels().front().name; + } + MathChannel gain; + gain.name = "math wrap"; + gain.enabled = true; + // Field by field: a partial brace-initialiser would add a fourth build warning, and the baseline + // is exactly three. + MathNode src; + src.op = MathOp::Source; + src.source = name; + MathNode op; + op.op = MathOp::Gain; + op.in_a = 0; + op.gain = 2.0; + gain.nodes = {src, op}; + gain.output = 1; + a->math.assign(1, gain); + apply_math_channels(*a); + { + std::lock_guard lock(a->model->mutex()); + for (std::size_t c = 0; c < a->model->channels().size(); ++c) { + if (a->model->channels()[c].name == gain.name) g_mathwrap_index = static_cast(c); + } + } + std::fprintf(stderr, "[mathwrap-probe] installed 'math wrap' on '%s': channel=%d\n", name.c_str(), + g_mathwrap_index); + std::fflush(stderr); + if (g_mathwrap_index < 0) return G_SOURCE_REMOVE; + g_timeout_add(100, mathwrap_probe_wait, a); + return G_SOURCE_REMOVE; +} + gboolean measure_probe(gpointer user) { App* a = static_cast(user); if (a->window == nullptr) return G_SOURCE_REMOVE; @@ -2768,6 +2884,12 @@ gboolean measures_probe_step(gpointer user) { rebuild_measures_pane(*a); std::fprintf(stderr, "[measures-probe] added %zu measurements on '%s'\n", a->measures.size(), signal.c_str()); + // This is the only probe set that asks for every type, and so the only one that places a + // *time* mark: `pulse duration`, `period` and `frequency` mark the crossings they were read + // from, and those are the marks `marks_in_window` counts. Without the trace the run reports + // the count with nothing to attribute it to -- which is how a window that lagged the screen + // read `0/6` with no way to tell a stale recompute from a mark placed correctly. + g_timeout_add(250, measure_trace, a); measures_probe_next(*a, 1, 400); break; }