SPE: reply-paced LCD polling and a calmer freshness gate — no more mirror blink

[opalito]

Summary

Closes #5541.

On v26.9.2, the floating LCD mirror from #5393 visibly appeared and disappeared in seconds-long cycles on a real 1.5K-FA over a ser2net telnet link. Bench iteration on that station (three screen recordings, one brightness-mapped frame-by-frame) established the ground truth: display-frame gaps of 0.5–4 s are routine on a best-effort link — eleven episodes in an 81 s recording taken in plain standby on a quiet band, with Status telemetry flowing throughout — and transmit-time RF makes them longer and denser (the 371-byte display reply is ~5x as exposed to bit errors as the 76-byte Status reply). Four changes:

1. Reply-paced LCD polling (SpeConnection): the cadence timer re-arms from each decoded display reply instead of free-running from the request. The original 600 ms period was an exact multiple of the 100 ms Status poll; two free-running coarse timers phase-lock (Qt deliberately coalesces their wakeups), and when the locked phase puts each display reply across a status poll on the wire, display frames drop in consecutive bursts until clock drift walks the alignment out. Folding the amp's variable response latency into the period makes a stable phase relationship impossible.
2. The interval becomes an idle gap, and shrinks to 250 ms. Because pacing is reply-driven, a second request is never in flight before the previous reply has fully arrived — the amplifier is never asked to interleave display blocks, and a slow proxy serial side stretches the effective cadence instead of accumulating a backlog. 250 ms gives ~3.5 refreshes/s at 115200 (<25% of the wire including the Status poll) and self-throttles to ~2/s on a 19200 link. The design note now records that a 9600 proxy serial side is saturated by the 100 ms Status poll alone, with a ≥57600 recommendation.
3. A display frame that arrives complete but fails validation triggers a prompt re-request (80 ms pause; each retry is itself provoked by a full received-and-rejected frame, so the retry stream is self-limited by the link's own serialization time). FrameParser gains a display-reject callback, covered by new protocol tests: no fire while a frame is merely incomplete, exactly one fire per corrupted frame, and a clean frame afterward still hands out. One clean frame every second or two is all the mirror needs through a transmission. (The parser itself was exonerated first: a transliteration ran a 20-case torture suite over telnet-escaped inverse-video frames — attribute bytes full of 0xFF, splits inside doubled-IAC pairs, 0xFF checksum bytes — all passing.)
4. Staleness is never marked on the glass (SpeApplet::setLcdFresh): the mirror holds its newest image at full brightness for as long as the connection lives — exactly like the amplifier's own LCD holds its picture — and freshness gates exactly one thing, the FRONT PANEL key group, with the window widened to an absolute 2400 ms so relay-transition quiet spells don't flap it. Both visible treatments were field-tested and rejected on the real amp: blanking to the idle glass made the mirror blink in and out, and even a light dim read as the LCD switching off through every routine gap. The disabled key group is the one and only not-live signal — which was always the actual safety property (no blind menu keystrokes). Hard clears remain where the image is truly obsolete: disconnect and presentation switches. The staleness transition is logged with its window so field reports can measure real gap lengths.

Why #5393's hardware validation didn't catch it: the builds validated on the real amp predated the freshness gate — they never blanked the mirror, so the same routine gaps were invisible (a gap was just a late refresh). The gate correctly made display liveness observable; this PR keeps the observability where it belongs (the key gate, plus a log line) and removes the burst-loss mechanisms that made it constant.

Colour ratchet, strict vs main, run locally: +0 unique / +0 references / +0 setStyleSheet.

Constitution principle honored

Principle II — the mirror renders the device's own screen as received and holds the newest picture it has, exactly like the device itself; late data gates the remote keys rather than being repainted as something it isn't. Principle VII unaffected: corrupted display frames are still discarded at the parser boundary (with the new reject hook and tests); this PR changes what happens after the discard.

Test plan

• Local build passes (CI compiles; no local C++ toolchain on the authoring machine — same arrangement as Add SPE Expert amplifier support (1.3K-FA/1.5K-FA/2K-FA) #4531/SPE applet: floating presentation with live LCD mirror and full front-panel keys #5393)
• Behavior verified on a real radio: fork installer build of this exact head bench-tested on the reporting station (1.5K-FA, ser2net 4.3.11 telnet, Windows client) — the mirror now stays continuously visible through full-power transmit, OPERATE⇄STANDBY relay transitions, and quiet-band standby; menu keys gate and return as designed; refresh is visibly smoother at the 250 ms gap.
• Existing tests pass (CI) — plus three new spe_protocol_test cases for the display-reject callback
• Reproduction steps documented (issue SPE floating LCD mirror appears and disappears in seconds-long cycles on a ser2net telnet link (v26.9.2) #5541, from screen recordings against the real amp)

Checklist

• Commits are signed
• No new flat-key AppSettings calls
• Code is clean-room
• All meter UI uses MeterSmoother (no meter changes)
• Documentation updated — design note §11 (reply-paced cadence, corrupted-frame retry, glass-holds-image staleness presentation, proxy baud recommendation). CHANGELOG.md untouched
• No security-sensitive changes

73 de Miguel, EC5W

🤖 Generated with Claude Code

[aethersdr-agent]

Reviewed against the head checkout at bad920de. CI is green on all five checks (build, check-macos, check-windows, Static checks, Sanitizer option configures) — noted, not relied on: none of them exercise SPE cadence.

Findings below are reasoned from code, not reproduced at runtime — I have no build, no amplifier, and no ser2net proxy here.

1. Issue fit

Partially. #5541 names two causes and this PR addresses both in the right places: the freshness gate becomes an absolute 1.8 s window decoupled from the poll gap, and SpeApplet::setLcdFresh dims via SpeLcdWidget::setStale instead of clear()ing, with the hard clears correctly retained at setConnected(false)/clearTelemetry() (SpeApplet.cpp:665-666) and at the presentation switch. I walked the disconnect and the docked⇄floating paths and both do reach the idle glass, so the body's "returns to the idle glass only when truly obsolete" claim holds.

Cause 1 — the phase-lock — is where I could not make the code match the claim. The triage on #5541 specified "re-arm m_lcdTimer from the display reply and make it single-shot … needs a fallback re-arm on the timeout path too." This PR adds the reply re-arm but leaves m_lcdTimer a repeating timer still armed inside requestLcdFrame() (SpeConnection.cpp:96) — setSingleShot is set for m_reconnectTimer, m_powerOnTimer and m_lcdStaleTimer, never for m_lcdTimer. That solves the lost-reply retry the triage worried about, but it means the design's central invariant is not actually implemented. See Blocker 1.

2. Scope

File	What it changes	Claimed by title/body?	Verdict
docs/architecture/spe-expert-amplifier-design.md	§11 request bullet rewritten for reply-pacing; freshness paragraph rewritten; new ser2net ≥57600 operator guidance	Yes (the baud recommendation is new operator-facing guidance, not in the issue)	In scope; the baud line is a small addition worth a maintainer glance
src/core/SpeConnection.cpp	Reply re-arms m_lcdTimer; comment reword at the ACK path	Yes	In scope
src/core/SpeConnection.h	kLcdPollIntervalMs 600→250; kLcdStaleTimeoutMs derived→absolute 1800; comment updates	Yes; #5541's fix direction names 250 ms and ~1.8 s explicitly, so this is a fix, not a smuggled preference	In scope
src/gui/SpeApplet.cpp	setLcdFresh dims instead of clears; setFloating clears first	Yes	In scope
src/gui/SpeLcdWidget.{h,cpp}	setStale, veil paint, clear() also resets stale	Yes	In scope

Everything in the diff is explained by #5541. No CHANGELOG.md entry (correct). No settings, protocol verb, capability or public-API surface added. No guard or confirmation removed — the key gate on m_lcdFresh in updateCommandsEnabled() (SpeApplet.cpp:653) is untouched, which is the important negative: dimming instead of blanking does not loosen the TX-adjacent front-panel key gating.

3. Blockers

1. The "a second request is never in flight before the previous reply has fully arrived" invariant is not implemented, and it fails on exactly the slow link the doc uses to justify the 250 ms gap. (inline: docs/…/spe-expert-amplifier-design.md:323, src/core/SpeConnection.cpp:36)

m_lcdTimer is repeating, interval 250 ms, and requestLcdFrame() arms it at send time (SpeConnection.cpp:96). The reply re-arm only wins the race when the reply decodes within 250 ms of the request. When round-trip ≥ 250 ms the send-time timer fires first, issues a second request while the first reply is still arriving, and — because requestLcdFrame() restarts the timer — the loop free-runs at a fixed 250 ms send-to-send cadence with none of the amp's latency folded in.

That is the original bug's precondition, not its cure: a fixed cadence commensurate with the 100 ms status poll (LCM 500 ms, so every second display request lands on a status-poll boundary). And it lands precisely where the doc says the design degrades gracefully — the doc's own arithmetic puts a 371-byte display frame at ~390 ms on a 9600 baud proxy serial side, i.e. well past 250 ms. At 19200 the doc's own ~450 ms cadence implies ~200 ms of reply latency, leaving ~50 ms of margin against an amp response latency the same doc calls variable.

The old 600 ms free-run needed a 600 ms round trip to overlap; this needs 250 ms. The margin shrank 2.4× while the doc's claim got stronger.

Two ways out, both small — a maintainer should pick:

• What the triage specified: m_lcdTimer.setSingleShot(true), re-arm only from the reply, and add the explicit timeout-path re-arm so a lost reply still retries (a separate, longer retry interval rather than reusing the 250 ms gap).
• Or keep the repeating timer purely as a watchdog with an interval above worst-case RTT, and gate requestLcdFrame() on an m_lcdRequestOutstanding flag cleared by the display callback.

Either way the doc sentence needs to state the condition (round trip < the gap) rather than assert the invariant unconditionally.

4. Nits (non-blocking)

• The raised collision probability is unaddressed and the triage asked for a decision on it. #5541's triage flagged that 600→250 ms raises display traffic ~2.4× and therefore raises per-status-poll collision probability even once bursts can't form, and proposed pollTick() skipping a status request while a display reply is outstanding (with care that a skipped tick must not count toward m_silentPolls). The PR doesn't do it and the body doesn't say why not. Reasonable to defer — worth saying so explicitly. Needs maintainer decision.
• m_lcdTimer.start() in the display callback is unguarded (SpeConnection.cpp:36) where requestLcdFrame() checks m_lcdWanted && m_connected. If anything on the lcdFrameReceived chain ever tore the connection down, the timer would be re-armed after teardown stopped it and would then tick forever into an early return. I traced the current chain (MainWindow_Wiring.cpp:6496 → SpeApplet::setLcdFrame → m_lcd->setFrame) and it is not reachable today; the adjacent pre-existing m_lcdStaleTimer.start() has the same shape. Cheap to make both conditional.
• The veil alpha 170 is an unmotivated magic number and not a token (SpeLcdWidget.cpp:148). It leaves ~33 % of the glass's original fg/bg contrast, which is the whole point — but "keeps the last screen for context" and "legible" are different bars, and docs/a11y.md asks 3:1 for non-text state distinctions. Concrete values are explicitly allowed as an intermediate there, with a follow-up issue for the token; worth filing one.
• No test, and half the change does have a deterministic seam. SpeLcdWidget is a plain QWidget: setFrame → setStale(true) → clear() retaining/dropping m_hasFrame is assertable socket-free, and the triage on #5541 asked for exactly that plus a lost-reply re-arm check. The scheduler half genuinely has no injection seam (SpeConnection owns its QTcpSocket and requestLcdFrame() early-returns on !m_connected), and I would not ask for a fake amplifier peer to get one. Calling this a nit rather than a blocker because the primary reported symptom's root cause sits on the untestable side — a maintainer could reasonably require the widget-side test.
• #5541 carries maintainer-review ("Requires maintainer review before any action is taken") and this PR was opened a minute after the issue, before any ruling. Procedural, not a code problem.

5. What I tried to break (and failed)

• Stale-state leakage across reconnect. Chased whether m_stale can survive into a live frame: setFrame() clears it (SpeLcdWidget.cpp:57) and clear() clears it, so a fresh frame is never veiled. Held.
• clear()'s new early-return condition. if (!m_hasFrame && !m_stale) return; — I looked for the state m_stale == true, m_hasFrame == false (reachable: setResponding(false) clears the widget, then the 1.8 s stale timer fires and calls setStale(true)). It paints correctly (no veil branch, "waiting for display…" branch taken) and the next clear() still resets both. Held.
• Painter state under the veil. p.setPen(Qt::NoPen) is set before the bezel and never changed before drawRect, so the veil has no stray 1px stroke, and the veil/!m_hasFrame branches are mutually exclusive. Held.
• Whether dimming loosens the front-panel key gate. updateCommandsEnabled() still gates on m_lcdFresh (SpeApplet.cpp:653) and setLcdFresh still assigns it before dimming. The keys go dead on staleness exactly as before; only the glass changed. Held — this was the one I most expected to find a hole in.
• The docked⇄floating clear() ordering. m_lcd->clear() runs before setLcdFresh(false), so the subsequent setStale(true) lands on an already-blank widget rather than veiling a previous session's image. Correct as written.
• Could not check: anything runtime — the phase-lock claim, whether 1.8 s is the right window on the reporter's link, and whether the veil is legible on a real 1.5K-FA glass. The reporter's ser2net serial-side baud is not stated in #5541, which is what would settle whether Blocker 1 bites their station specifically; worth asking them.

6. Recommendation

Request changes, on Blocker 1 only. The freshness-gate and dim-don't-blank halves are well-reasoned, correctly scoped, and leave the safety-relevant key gating untouched — I tried several ways to break them and could not. But the reply-pacing half ships the doc's strongest claim without the code that makes it true, and the 250 ms gap is only defensible because of that claim; on a slow proxy serial side it degrades into a fixed-cadence free-run at 2.4× the old request rate, which is the reported bug with more traffic behind it.

Concrete next step: make m_lcdTimer single-shot with an explicit longer-interval retry on the lost-reply path (or add the in-flight guard), and condition the doc sentence on round-trip < gap. Then the maintainer decision on pollTick() skipping during an outstanding display reply can be taken on its own merits rather than as a load-bearing assumption.

Nice work on the root-cause writeup — the phase-lock explanation is the kind of thing that usually gets papered over with a bigger timeout.

---

_{🤖 aethersdr-agent · cost: $7.2874 · model: claude-opus-5}

[opalito]

Thanks @aethersdr-agent — Blocker 1 is exactly right, and it's fixed in 6c7033e the way the triage specified. Note the review ran against bad920d; three commits have landed since, two of them driven by continued bench iteration on the real amp, so several findings are addressed or mooted. Point by point:

Blocker 1 (send-armed repeating timer defeats the no-overlap invariant) — accepted and fixed in 6c7033e. m_lcdTimer is now single-shot: a request arms only a 1 s lost-reply fallback (sized above the worst plausible round trip — a 9600 proxy serial side spends ~390 ms serializing the frame alone), and only a decoded reply re-arms the 250 ms gap. Within the fallback a request is either answered or genuinely lost, never merely still in flight; beyond it the reply is treated as lost and retried, accepting the overlap risk on a link that degenerate. The design note §11 now states the condition (round trip < the fallback) instead of asserting the invariant unconditionally. Your nit about the unguarded reply-side re-arms is fixed in the same commit (m_lcdWanted && m_connected gate).

What changed since bad920d, from bench iteration:

• a1ad6eb: display frames that arrive complete but fail validation now trigger a prompt re-request (80 ms pause, self-limited — each retry is provoked by a full received-and-rejected frame), with a FrameParser display-reject callback covered by three new protocol tests. Key-gate window 1800 → 2400 ms.
• 8603992: the veil is gone entirely — bench measurement (brightness-mapping an 81 s recording in plain standby on a quiet band) showed display gaps of 0.5–4 s are routine on this link, eleven episodes in 81 s, so any visible staleness treatment fires constantly; the glass now holds its newest image at full brightness and the disabled key group is the only not-live signal. That moots the alpha-170 magic-number/a11y nit and the setStale widget-test seam (the API no longer exists); the key gate in updateCommandsEnabled() remains untouched, as you verified.

Nit: pollTick() skipping status while a display reply is outstanding — explicitly deferred, maintainer's call. Reasons: it couples the two poll loops and complicates the silent-poll accounting exactly as the triage warned; and with reply-paced single-shot scheduling plus the corrupted-frame retry, the residual collision cost is one lost display frame recovered ~80 ms later, which the bench no longer registers. Happy to implement it as a follow-up if ruled worthwhile.

Hardware validation (answers §5's "could not check", including the serial-side baud question): a fork installer build of 8603992 was bench-tested on the reporting station — 1.5K-FA over ser2net 4.3.11 plain telnet, serial side at 115200, Windows client. The mirror stays continuously visible through full-power transmit, OPERATE⇄STANDBY relay transitions, and quiet-band standby; the FRONT PANEL keys gate and return as designed; refresh is visibly smoother at the 250 ms gap. On a 115200 link the round trip is far under the gap, so the reply re-arm always won the send-timer race — meaning 6c7033e changes scheduling only in the slow-link regime that station never enters, and its validated timing is identical by construction. I'll re-bench on the exact merge head if a maintainer wants that datum on record.

On the procedural note: acknowledged that #5541 carries maintainer-review — the PR is ready for that ruling, not trying to pre-empt it.

73 de Miguel, EC5W