feat(peripherals): add TelePost LP-100A wattmeter support (serial/ser2net)

[NF0T]

Summary

Closes #5315. Adds support for the TelePost LP-100A digital vector RF
wattmeter as a station peripheral — live power, SWR, impedance, phase and dBm
over the meter's RS-232 port, from either a local serial port or a raw-TCP
serial proxy (ser2net / Lantronix / Digi). Requested by users in the Facebook
community.

It follows the four-peripheral precedent already in main — AcomConnection,
SpeConnection, VkampConnection, GreenHeronModel — rather than inventing
anything: a pure protocol layer, a transport, a dedicated applet, and a
Peripherals row. Not behind IRadioBackend; the LP-100A is a standalone
instrument the radio has never heard of.

Read-only in v1. The meter's three write commands (A/M/F) are
deferred deliberately — every one is a blind increment with no acknowledgement,
so a UI built on them must never show a commanded state, only what the next
poll reports. That rule is written into the design note now so v2 inherits it.

Developed against real hardware throughout. Every wire fact below was measured,
not taken from a document — and the manual turned out to be wrong about
three of them.

What the hardware settled that the manual did not

• The record layout. The manual's example record is 41 characters; the wire
  sends 42. The extra character is in the Z field, which is zero-padded to
  5 where the manual prints 4 — not, as an early draft of this work assumed,
  a missing pad on the Power field. Both readings fit the length equally well,
  which is exactly why it needed a capture rather than an inference.
• Both enum tables. The manual lists five SWR-alarm values and two
  peak-hold modes. Cycling A and F through full rotations gives six
  (0–5) and three (0–2).
• Phase carries no sign at all. Not a decode gap — the protocol never
  transmits it. The manual (p.12) has the operator recover it by QSY-ing
  ~100 kHz and watching the reactance slope, and LP-Plot automates that only
  "since it can control your transmitter's frequency". 954 captured records
  across three frequencies contained no sign character. So the applet renders
  |Z| and |phase| and never signed reactance or an R+jX form.

Three design points worth a reviewer's attention

A record is not a coherent snapshot. At key-up the meter holds power and
dBm for ~1.7 s while Z, phase and SWR revert to idle: 54 of 894 captured
records carry real power beside SWR 1.00. So nothing cross-derives between
fields — that is why there is no reflected-power readout, which
P_fwd·Γ² would render as 0 W against 8.77 W forward. Detection is from
physics (|Z|∠φ must reproduce the reported SWR) rather than from the peak-hold
field, because the mechanism is "power and impedance have different time
constants" and Avg mode has its own hold.

The wire is commonly shared, so the poll loop is gated. Other clients often
poll the same ser2net port, and a second connection receives their replies
(measured: 60 complete records in 6 s while sending nothing). Polling blindly
on top of that doubles the meter's work and makes any blind-read client
misattribute our replies to its own polls. PollGate rides along when someone
else is polling and polls when the wire is quiet. The subtlety: gating on "no
record in the last N ms" does not work, because our own replies reset the
same timestamp — measured against a 100 ms foreign cadence, N=130 ms caps solo
polling at 7.7 Hz while N=100 ms polls over the other client 48.5% of the time.
Gating on foreign records breaks that coupling.

There is no checksum in this protocol, so looksLikeRecord() — field
count, separator positions, per-field parse, physical range checks — is the
entire integrity mechanism. The reference implementation consulted validates
length alone, which accepts any single flipped digit.

Constitution principle honored

• Principle VII — Untrusted Input Is Validated At The Boundary. The meter's
  byte stream is untrusted and unchecksummed. looksLikeRecord() bounds the
  buffer, validates field shape and ranges, and fails closed; the parser
  self-heals on garbage (it silently absorbs the ser2net connect banner) and
  caps its buffer when no frame marker ever arrives.
• Principle V — Each Feature Owns Its Configuration As A Single Object.
  All settings go through PeripheralSettings under one nested Lp100a
  object. No new flat-key AppSettings calls.
• Principle IV — Every Contribution Is Clean-Room. Implemented from the
  manufacturer's published manual plus our own Node-RED flow and observation of
  the wire. No vendor code. Provenance recorded in THIRD_PARTY_LICENSES.
• Principle VI — Never Transmits Without Operator Intent. v1 sends a single
  poll byte and nothing else; it touches no transmit path.

Test plan

• Local build passes — aethercore and AetherSDR clean, no new warnings
• Behavior verified on real hardware — a real LP-100A over ser2net, on both
  the idle and transmit paths, including a range transition observed during
  an FT8 CQ at 43.10 W (meter reported Range Mid; the applet scaled to the
  Mid ceiling)
• Existing tests pass — full ctest suite run; the new test passes and no
  failure is attributable to this branch (see Pre-existing failures below)
• New test: lp100a_protocol_test, 49 assertions, declared in
  tests/tests.cmake, links only Qt6::Core, needs no hardware and no
  display

The protocol tests run against literal captured bytes and the manual's own
example rather than against our own encoder — the property that lets them catch
a real bug. They also pin each place this implementation deliberately diverges
from the reference flow, so a later "simplification" back to it fails loudly.

An independent cross-check worth stating: |Z| and phase reproduce the SWR the
meter separately reports to a mean absolute error of 0.0038 over 218
records under drive. That validated the field interpretation against physics
rather than against another document — and it is what surfaced the coherence
problem above, after the aggregate first came back wrong.

Checklist

• Commits are signed
• No new flat-key AppSettings calls — nested JSON under one key
  (Principle V)
• Code is clean-room (Principle IV)
• All meter UI uses MeterSmoother — both gauges are HGauge, which
  drives its animation through it
• Documentation updated — new docs/architecture/lp-100a-wattmeter-design.md,
  docs/architecture/README.md entry, peripheral(lp100a) touchpoint tag
  with the manifest regenerated, and THIRD_PARTY_LICENSES.
  No CHANGELOG.md entry.
• No new hardcoded colours — the applet and the Peripherals row use
  ThemeManager {{color.*}} tokens only, so the colour ratchet counts
  zero new literals. This deliberately does not reuse the amplifier
  family's makeValueLabel(), which carries one.
• Security-sensitive changes — n/a

Pre-existing failures in the suite

Three tests fail on this branch. None is caused by it, and I checked rather
than assumed:

Test	Verdict
bridge_docs_check	Pre-existing. Fails identically on upstream/main with none of this code present — "docs verb table is STALE (71 verbs in the registry)". This branch adds no automation verbs.
phone_tx_filter_numeric_entry_test	Not attributable. Compiles none of the files this branch touches; it aborts inside Qt's own qasciikey.cpp:470, a key-mapping assert under the offscreen platform.
hl2_state_restore_test	Pre-existing — verified by building and running it from a clean upstream/main worktree. It links aethercore, to which this branch adds two sources, so it was the one that genuinely needed the control rather than a file-list argument.

bridge_docs_check looks worth a separate fix; it is a stale generated table,
not a real regression, but it will fail for everyone until someone regenerates
it.

Notes for review

Three questions remain genuinely open, and are documented as open rather
than rounded off (design note §8):

1. Whether field 0 is forward or net power is not settled. The meter read
   8.62–8.93 W against a 10 W TUNE at SWR ≈ 1.68; forward implies 0.53 dB of
   coax and calibration loss, net implies 0.24 dB, and both are ordinary. It
   needs a deliberately high SWR to discriminate. Until then the applet labels
   the field "PWR" as the meter does and claims nothing, and nothing derives
   from it in a way the answer would change.
2. dBm width on other units. It is the only field that is signed and not
   zero-padded. This meter idles at −2.3 and peaks at 39.5, but a value in
   −99.9…−10.0 would be 5 characters. The parser keys on field shape rather
   than a hard length, so such a record degrades to a rejection, not a
   misparse.
3. ser2net rebroadcast is confirmed on one proxy, not guaranteed. Where a
   proxy refuses a second client or serialises per connection, the gate simply
   never trips and we poll — the plain design. It is an optimisation that
   engages where the transport allows and costs nothing where it does not.

On #4944 (where an undiscoverable peripheral gets configured): I raised a
third question on the issue — whether to gate the tile on stored configuration
rather than on the connection — and then withdrew it. The objection to
gate-on-connect is that a configured-but-unreachable device shows no tile at
all, but that does not bite here: LpMeterConnection deliberately does not
drop the link when the meter stops answering, so a wedged meter keeps its tile
and shows NO DATA inside it. This therefore follows ACOM/SPE/VKAMP exactly
and adds no further divergence.

That state is not hypothetical — during bring-up the meter wedged with the
serial link perfectly healthy (TCP up, ser2net serving its banner, zero records
until it was power-cycled). VkampConnection's dead-link watchdog aborts its
socket in that situation, which hides its applet at the moment the operator
most needs the explanation. Worth a follow-up on its own.

Section 9 of the design note audits every borrowed behaviour with an
adopt / improve / reject verdict and a reason — including the rejections, and
including one against our own AcomConnection: its
kAutoRangeConsecutiveFrames = 2 was chosen against an 8-bit checksum, and
this protocol has none, so the same constant is defensible here only because
looksLikeRecord() carries the weight instead. That record exists so a later
contributor does not "restore" the originals.

73,
Ryan Butler NF0T

---

👨🏼‍💻 Co-authored by Claude Opus 5

[aethersdr-agent]

Thanks for this — the LP-100A work is nicely scoped, and the design note calling out the manual-vs-wire discrepancies is exactly the kind of thing that saves the next person a day.

One CI job is red, and it's a small, mechanical fix.

What failed

Static checks → Hardcoded-colour ratchet (the other three jobs — build, check-macos, check-windows — were still running when I looked).

I reproduced it locally against your head 42a05e9, merge base b32d035:

=== ratchet (vs PR base) ===
  OK   unique_colours       613  (base 613, +0)
  OK   total_references    2724  (base 2724, +0)
  OVER setstylesheet       1094  (base 1090, +4)

FAIL: this PR raises the hardcoded-colour count above its base.
  setstylesheet: 1094 > 1090  (+4)

Good news first: you added zero new colours. Unique colours and total colour references are both +0 — you reused the existing kLabelStyle / kEditStyle / kBtnStyle constants rather than inventing new hex, which is the right instinct. The only metric over is setstylesheet, which counts raw setStyleSheet( call sites, not colours (tools/audit_colours.py:105). So migrating a colour won't clear it — the count is about the call, not its argument.

The four sites

All in the new Peripherals row 8 in src/gui/RadioSetupDialog.cpp (line numbers on your head):

line	call
7990	devLbl->setStyleSheet(kLabelStyle);
8014	serialCustomEdit->setStyleSheet(kEditStyle);
8033	netIpEdit->setStyleSheet(kEditStyle);
8103	lpBtn->setStyleSheet(kBtnStyle);

They're a faithful copy of the ACOM row above (7399/7426/7444/7508), so this is the ratchet catching replicated legacy style rather than anything new you invented — the gate is a delta against your base, so pre-existing sites are grandfathered but new ones aren't, regardless of how they got there.

How to fix it

Route them through ThemeManager::applyStyleSheet() instead — same runtime result (its first step is literally widget->setStyleSheet(resolve(template))), plus it registers the widget in the token reverse-map so it repaints on theme change. ThemeManager.cpp is on the audit allow-list, so the call site stops being counted.

You're already doing this correctly a few lines up with kComboStyle — that block defines a {{color.*}} template and calls tm.applyStyleSheet(serialCombo, kComboStyle). Extending the same pattern to the other four is the cleanest fix, and the tokens you need already exist and map to the exact hex those constants use:

• kLabelStyle #c8d8e8 → {{color.text.secondary}}
• kEditStyle #1a2a3a / #304050 / #c8d8e8 → {{color.background.1}} / {{color.background.2}} / {{color.text.primary}}
• kBtnStyle #1a2a3a / #304050 / #c8d8e8, hover #203040 → same family (see docs/theming/canonical-tokens.md)

i.e. define local token-based kLpLabelStyle / kLpEditStyle / kLpBtnStyle next to your kComboStyle and use tm.applyStyleSheet(w, ...) for all four. tools/migrate_setStyleSheet_to_applyStyleSheet.py does this mechanical rewrite if you'd rather not do it by hand.

To check before pushing:

BASE=$(git merge-base HEAD origin/main)
git worktree add /tmp/colour-base "$BASE"
python3 tools/audit_colours.py --src src --compare-src /tmp/colour-base/src --summary-only --strict

References: .github/workflows/static-checks.yml:174, tools/audit_colours.py, docs/theming/canonical-tokens.md.

No Copilot or reviewer comments on the PR yet, so nothing else to fold in. Nothing about the protocol, connection, or applet logic is implicated — this is purely the styling call shape in the settings row.

---

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

[NF0T]

Thanks — the diagnosis is right, and the distinction you drew is the one that matters: the metric counts setStyleSheet( call sites, not colours, so "we added zero new hex" was true and irrelevant. Your reproduction against 42a05e9 matches what CI saw.

Two notes, one where you caught something I'd already half-fixed badly, and one where I don't think the mapping table agrees with you.

The fix I'd already pushed was insufficient, and your comment is what made that visible

12f7a785 (pushed before your triage landed) routed the four calls through ThemeManager::applyStyleSheet() — but passed the existing file-level kLabelStyle / kEditStyle / kBtnStyle constants through unchanged. Those are literal hex.

That clears the gate, because ThemeManager.cpp is allow-listed. It achieves nothing else. applyStyleSheet() stores the template and re-resolves it on a theme change (ThemeManager.cpp:1689-1711), but a template with no {{color.*}} in it resolves to itself, so the re-resolve is a no-op. My commit message and the code comment both claimed the widgets now follow the theme. They didn't — I'd satisfied the ratchet and written down a benefit I hadn't actually delivered.

Your "define local token-based constants" is the substantive fix, and 65d4974 does it.

One mapping I read differently — kLabelStyle

You suggested #c8d8e8 → {{color.text.secondary}} for kLabelStyle, and #c8d8e8 → {{color.text.primary}} for kEditStyle. Same hex, two destinations, so at most one can be right.

docs/theming/canonical-tokens.md:57 folds it explicitly:

Token	Canonical	Collapses
color.text.primary	#e6f0fa	#c8d8e8 (369 — most-referenced colour in codebase), …
color.text.secondary	#8ea8c0	#8aa8c0 (132), #8090a0 (57), …

text.secondary is a visibly dimmer tier. Taking it would have dimmed this row's device label relative to every other Peripherals row's, which is a real visual regression rather than a neutral choice. I've used text.primary for both — which also keeps the row self-consistent, since the kComboStyle immediately above it already resolves #c8d8e8 that way.

Worth flagging because tools/audit_colours.py's own suggestion column prints #c8d8e8 → color.text.secondary, which is where I assume the suggestion came from. If so that's a discrepancy between the tool's hint and the canonical doc, on the single most-referenced colour in the tree — probably worth its own issue, since it will mislead every future migration of that colour and not just this one. Happy to file it.

One mapping that needed a decision rather than a lookup

kBtnStyle fills with #1a2a3a and hovers to #203040. The table (:47) collapses both into color.background.1 — so a literal translation makes the hover identical to the base fill and silently deletes the affordance. The gate would have passed and the button would have stopped visibly reacting to the mouse.

Lifted the hover one tier to {{color.background.2}}, which is what Theme.h:376 and MainWindow_Menus.cpp:1438 already do for a background.1-filled button. Noted in a comment so the next person doesn't "correct" it back.

Verification

Ratchet re-run with CI's own invocation. One trap worth recording: git merge-base HEAD origin/main gave the wrong base here, because origin is my fork and its main was stale — it reported +6 colours in SettingsBrowserDialog.cpp, a file this branch never touches. Against the real base (upstream/main, b32d035, the same one you used):

=== ratchet (vs PR base) ===
  OK   unique_colours       613  (base 613, +0)
  OK   total_references    2724  (base 2724, +0)
  OK   setstylesheet       1090  (base 1090, +0)

All four jobs were already green on 12f7a785; 65d4974 is a no-op for the gate and changes only which colours actually get resolved.

73,

Ryan NF0T

---

👨🏼‍💻 Co-authored by Claude Opus 5

[aethersdr-agent]

Issue fit

Yes, with one gap. #5315's v1 scope — pure unit-tested LpMeterProtocol + LpMeterConnection under src/core/, a dedicated LpMeterApplet, serial and raw-TCP transports, a Peripherals row, peripheral(lp100a) in the touchpoint manifest, read-only P only — is all present and each piece maps to a hunk. The two decisions the issue asked to settle before coding are both settled the way the issue proposed (markHardwareConditional("LP100") + absent from kDefaultOrder, ceilings in the applet's own context menu), and the issue's three explicit open questions are each resolved against hardware and the resolution recorded: the 41-vs-42 discrepancy is the Z field, not Power (§8); alarm 5 and peak-hold 2 are confirmed real by cycling A/F; listen-before-talk is implemented as PollGate. The gap is that the design note's own §9 audit lists AcomConnection's "5 s reconnect — Adopt" and the code carries the machinery but never enables it (blocker 1).

Scope

File / group	What it changes	Claimed?	Verdict
src/core/LpMeterProtocol.{h,cpp}	Pure protocol: parse, validate, PollGate, RangeTracker	Yes	In scope
src/core/LpMeterConnection.{h,cpp}	Serial/TCP transport, poll loop, watchdogs	Yes	In scope
src/gui/LpMeterApplet.{h,cpp}	The applet	Yes	In scope
src/gui/AppletPanel.{h,cpp}	LP100 registration, Metering category, hardware-conditional	Yes	In scope
src/gui/MainWindow{.h,.cpp,_Wiring.cpp}	Member connection + signal wiring + auto-connect	Yes	In scope
src/gui/RadioSetupDialog.{h,cpp}	Peripherals row 8	Yes	In scope
tests/lp100a_protocol_test.cpp, tests/tests.cmake, CMakeLists.txt	New test target + sources	Yes	In scope
docs/architecture/lp-100a-wattmeter-design.md, README.md, THIRD_PARTY_LICENSES	Design note, index entry, provenance	Yes	In scope
docs/architecture/aetherd-touchpoint-tags.json	The LpMeterConnection.h entry plus ~20 unrelated — → — rewrites	Partly	Regenerator churn — see nits
docs/architecture/aetherd-touchpoints.md	LP entries plus 6 unrelated headers (CtcssTones.h, SystemInfo.h, SystemInfoCollector.h, ThreadCpuRing.h, ConnectionSharingPolicy.h, RadioCapabilities.h) and drifted includer counts	Partly	Same — manifest regeneration, not smuggled behaviour

No new flat AppSettings keys (everything goes through PeripheralSettings::setDevice*, matching ACOM/SPE/VKAMP). No CHANGELOG.md entry — correct. No behaviour change to any existing UI element, so nothing here reads as a smuggled preference: every changed surface is new. No - lines removing a guard anywhere in the functional diff.

Blockers

1. setAutoReconnect() is never called, so the whole reconnect path is dead — and a dropped ser2net link takes the applet tile with it. (inline on LpMeterConnection.h:74)

m_autoReconnect defaults to false and grep -rn "setAutoReconnect" src/ across the head checkout returns no m_lpMeterConn call site. The startup propagation block at MainWindow_Wiring.cpp:6041-6049 sets it on m_tgxlConn/m_pgxlConn/m_antennaGenius/m_acomConn/m_speConn/m_vkampConn and omits the new connection; RadioSetupDialog.cpp:7946-7962's live toggle omits it too. All three armReconnect() call sites are guarded on m_autoReconnect, so kReconnectMs, m_reconnectTimer and its lambda are unreachable.

Failure scenario (reasoned from code, not observed running): ser2net restarts or the network blips. QTcpSocket emits disconnected → onTransportDown() → emit disconnected() → the wiring lambda at MainWindow_Wiring.cpp:6530 calls setLpMeterVisible(false). No reconnect is armed, so the tile is gone until the operator reopens Radio Setup → Peripherals and clicks Connect. That is materially worse than the ACOM/SPE peers this note says it follows, and it undercuts §10's argument that this integration doesn't worsen the #4944 divergence — a configured and previously reachable device does end up with no tile here. Fix: add m_lpMeterConn.setAutoReconnect(ar); to the MainWindow_Wiring.cpp:6041 block and to the RadioSetupDialog.cpp:7946 toggle.

Nits (non-blocking)

• ResponseParser::feed() can grow without bound once a marker has arrived (inline on LpMeterProtocol.cpp:199). The kRecordLength * 4 cap only runs on the firstMarker < 0 path. Once m_buf starts with ; and neither a second marker nor a valid fixed-length body ever appears, the break leaves everything buffered and every subsequent feed() appends. Needs a stuck stream on a connection that already delivered one ;, so it is not a likely path — but it is the one case the existing cap was written to prevent and it sits just outside it.
• reflectedWattsFromSwr() has no production caller (inline on LpMeterProtocol.h:206). §7 explains why the applet shows no reflected figure, which is right — but that leaves a public function exercised only by two test rows, and its comment points readers at "the per-field liveness tracking in LpMeterConnection", which does not exist: the coherence gating lives in LpMeterApplet::applyDimming() via Reading::coherent. Either drop the function until v2 needs it, or fix the pointer.
• An operator lowering a ceiling after auto-expansion silently no-ops (inline on LpMeterConnection.cpp:144). RangeTracker::setCeilings()'s up-only guard is exactly right for a re-read of the same config, but the same path serves a deliberate edit from the context menu. Someone who sees the tooltip's "widened automatically" and sets High to 700 W to match their meter gets no change and no feedback until reconnect.
• Ceilings persist through setDeviceInt, so a fractional full scale truncates. QInputDialog::getDouble is called with 0 decimals, so this is unreachable today — worth a static_cast<int> comment or an int-typed setting.
• m_gateSettleTimer is restarted on every 100 ms tick once it has fired (pollTick's || !m_gateSettleTimer.isActive()), so it wakes roughly every 1.6 s forever and returns immediately. Harmless, just perpetual.
• The touchpoint manifest churn above is almost certainly a regeneration side effect rather than hand edits, but it makes the diff harder to read and AppSettings.h 102→101 / ThemeManager.h 139→142 will conflict with any other PR that regenerates. A sentence in the PR body saying the manifest was regenerated would cover it.

What I tried to break

• CodeGuard's five CG-PATH-001 hits: refuted, dropped. MainWindow.cpp:8806/8918/9037-9039 are the FFTW-wisdom progress dialog and the macOS bundle-relaunch QDir::cdUp() walk. This PR touches 3 lines in that file (git diff --stat), none near them. Pre-existing, not this diff's.
• M_PI on MSVC (my usual suspect for a Windows-only break on new DSP code): LpMeterProtocol.cpp:275 and the test both use it with only <cmath> and no _USE_MATH_DEFINES. Five other src/core/*.cpp do the same and check-windows is green on 12f7a78, so the include chain supplies it. Not a finding.
• Attacked the tests for self-confirmation. They largely survive: kManualExample is quoted verbatim from the manufacturer's PDF and kCapturedIdle/kCapturedTx are literal captured bytes, not encoder output, so they can fail. The "42 chars but misplaced separators" row would pass against a naive length == 42 check being broken — it is a real mutation guard, not a tautology. The PollGate solo row (polls == 100) fails if own replies are ever allowed to move the suppression timestamp, which is the exact defect §6 describes. The 43-char wider dBm row genuinely pins the "shape not length" rule.
• Walked the disconnect/teardown reentrancy. disconnect() sets m_connected = false before teardownDevice(), so QTcpSocket::abort()'s synchronous disconnected() re-enters onTransportDown() with wasConnected == false and no duplicate disconnected() is emitted; m_deliberateDisconnect suppresses the reconnect arm in the re-entrant frame. Mode-switch (network→serial while connected) produces a disconnect/connect pair rather than a wedge. m_serialPort's signals are connected exactly once behind if (!m_serialPort), so repeated connectSerial() does not multiply them. No defect found here.
• Checked the added code is actually reachable. lp100a_protocol_test is registered in tests/tests.cmake:1982-1988; both new .cpp files are in CORE_SOURCES/GUI_SOURCES; LpMeterApplet is constructed and registered at AppletPanel.cpp:830; setLpMeterVisible exists and is called. Nothing dead except reflectedWattsFromSwr and the reconnect path above.
• Could not check: anything requiring a build, a GUI, or a real LP-100A. Every finding above is reasoned from the head checkout at /tmp/aetherclaude/pr-5320, not reproduced at runtime. In particular the §3 field-width claims, the §6 measured constants (15 ms reply latency, 79 ms minimum foreign gap, 60 records in 6 s from a silent second connection) and the §7 ~1.7 s key-up hold are all single-station measurements I have no way to falsify — they are stated as measured, and the code degrades safely if a different unit disagrees, which is the right shape for an unverifiable claim.
• CI is green on all four checks for 12f7a78 (build, check-windows, check-macos, Static checks) — noting for the record that ctest in ci.yml is -R-filtered, so green does not mean lp100a_protocol_test ran.

Recommendation

Request changes — for blocker 1 only, which is a two-line fix. Everything else here is unusually strong work: the protocol layer is pure and tested against literal captured bytes, the design note distinguishes measured facts from unverified ones and names what the manufacturer's manual gets wrong, and the §9 provenance audit records what was rejected from the reference sources rather than just what was borrowed. Wire setAutoReconnect into the two existing propagation sites and this is ready for a maintainer to take on its merits; the nits can ride in the same push or a follow-up.

Thanks for filing the issue first and settling both open questions before writing code — it made this reviewable.

---

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

[aethersdr-agent]

Blocker — this setter has no caller, so the reconnect machinery below it is dead.

grep -rn "setAutoReconnect" src/ on the head checkout shows the startup propagation block at MainWindow_Wiring.cpp:6041-6049 setting it on m_tgxlConn/m_pgxlConn/m_antennaGenius/m_acomConn/m_speConn/m_vkampConn, and the live Peripherals toggle at RadioSetupDialog.cpp:7946-7962 setting it on five of those — neither mentions m_lpMeterConn. m_autoReconnect therefore stays false for the lifetime of the app, and all three armReconnect() call sites are guarded on it, so kReconnectMs, m_reconnectTimer and its timeout lambda are unreachable.

The consequence isn't just dead code. When the ser2net proxy restarts or the network blips, onTransportDown() emits disconnected(), and the wiring lambda at MainWindow_Wiring.cpp:6530 calls setLpMeterVisible(false) — so the tile disappears and nothing ever brings it back until the operator reopens Radio Setup and clicks Connect. That's the exact "configured but unreachable device shows no tile" state §10 of the design note argues this integration avoids.

Fix is to add m_lpMeterConn.setAutoReconnect(ar); alongside its peers in the MainWindow_Wiring.cpp:6041 block, and to the RadioSetupDialog.cpp:7946 toggle so the setting takes effect without a restart.

(Reasoned from the checkout — I can't build or run this.)

[NF0T]

Confirmed and fixed in `6c5deb5c`. I verified it independently rather than taking the trace: `m_autoReconnect{false}` at `LpMeterConnection.h:142`, all three `armReconnect()` sites guarded on it, no `m_lpMeterConn` call site anywhere in `src/`, and the `disconnected` lambda calls `setLpMeterVisible(false)`. The path was unreachable. Added to both sites you named.

One correction to the severity. PeripheralSettings::autoReconnect() defaults to False (PeripheralSettings.h:35), so for a default-configured operator ACOM/SPE/VKAMP do not reconnect either and the LP-100A behaved exactly like its peers. The defect bit the operator who had explicitly turned the setting on — for whom every other peripheral honoured it and this one silently did not. Still worth blocking on, but it is a setting quietly excluding one device rather than a blip permanently killing the tile for everyone.

You were right to link it to §10. My #4944 argument covered the meter-wedges-while-the-link-is-healthy case, which the transport decision does handle; it did not cover a transport drop, which does hide the tile. With the wiring in place, parity with the peers is restored and the argument holds again.

While in that toggle: m_vkamp is missing from it too, and that is pre-existing from #4919 — VkampConnection has the setter and the startup block does call it, so toggling the checkbox mid-session reaches every peripheral except that one. Left alone per scope discipline and noted in a comment; filing separately.

[aethersdr-agent]

This bound only applies while no marker has been seen. Once m_buf starts with ;, the loop below can break on the incomplete path and every subsequent feed() appends without any cap: no second marker ever arrives, and looksLikeRecord(m_buf.mid(1, kRecordLength)) keeps failing, so nothing is consumed.

It needs a link that delivers one ; and then an unbounded stream containing no further marker — unlikely, but it's precisely the scenario this cap was added to prevent, just one branch over. A cheap fix is to apply the same clamp at the break, e.g. drop everything before the second marker (or truncate to the last kRecordLength bytes) once the buffer exceeds kRecordLength * 4 with no valid decode.

[NF0T]

Confirmed and fixed in `6c5deb5c`. Your reading of the control flow is exact — once m_buf starts with ;, indexOf always succeeds, so the firstMarker < 0 cap can never run again and the break leaves everything buffered.

A body past kRecordLength * 4 now drops its marker and resyncs at the next one, clearing if there is none. Same bound as the existing cap, for the same reason.

Two regression rows added: feed one ; then 12.8 kB of never-valid non-marker bytes and assert the buffer stays bounded, then feed a good record and assert it still decodes — a cap that resynced but left the parser wedged would be a worse bug than the one being fixed.

[aethersdr-agent]

Nit: this has no production caller — grep -rn reflectedWattsFromSwr src/ tests/ finds only the definition, the declaration, and two test rows. §7 of the design note explains correctly why the applet shows no reflected figure, so the absence is deliberate; what's left is a public function whose only exercise is a test that pins nothing shipping.

The comment above it also points readers at "the per-field liveness tracking in LpMeterConnection", which isn't there — I grepped, and the coherence gating actually lives in LpMeterApplet::applyDimming() (zLive = live && m_reading.coherent). Either drop the function until v2 needs it, or retarget that reference to the applet.

[NF0T]

Confirmed — the pointer is wrong, and worse than stale. "Per-field liveness tracking in LpMeterConnection" was a design from the plan that never got built; Reading::coherent, computed in decodeReading() and consumed by LpMeterApplet::applyDimming(), replaced it. The comment survived the design change. Fixed in `6c5deb5c`.

On dropping the function: keeping it, and now saying why in the comment. It sits alongside kAlarmCommand / kModeCommand / kPeakAvgCommand, which are also deliberately unused in v1 so the control PR adds no protocol surface — §4 states that intent. It reads as dead code precisely because nothing said so; that is the defect, and it is a comment defect.

[aethersdr-agent]

Nit: RangeTracker::setCeilings()'s up-only guard is right for the case its comment describes (a re-read of the same config must not shrink a ceiling that observed power expanded), but this same path also serves a deliberate operator edit from the applet's context menu.

Sequence: a reading exceeds the configured 1500 W High ceiling twice, so m_ceilingW becomes e.g. 1600 and the tooltip says "widened automatically". The operator reads that, right-clicks, and sets High to 700 W to match their actual meter. setCeilings() sees m_autoExpanded && configured < m_ceilingW, keeps 1600, and emits nothing — no change, no feedback, until reconnect.

A flag distinguishing "re-read" from "explicit edit" (the latter always wins and clears m_autoExpanded) would keep the guard where it earns its keep without silently ignoring the operator.

[NF0T]

Confirmed, and I rate this higher than a nit — fixed in `6c5deb5c`.

Verified both callers: MainWindow_Wiring.cpp:6579 is the stored-config load, :6561 is the context-menu edit, and they share one entry point. The guard is right for the first and wrong for the second, and the failure is worse than "no change" — because the ceiling does not move, setRangeCeilings never emits gaugeCeilingChanged, so the operator gets no feedback either. The value does persist, so it appears to work after a restart, which makes it harder to report than a plain refusal.

RangeTracker::setCeilings() now takes a CeilingSource with no default, so each call site states intent. OperatorEdit always wins: it is a person telling us what their meter is set to, which is the one thing this protocol never puts on the wire. Honouring a mistaken edit is self-correcting — power above the new ceiling re-expands within kCeilingExpandRecords records.

Three test rows pin it in both directions (edit lowers and clears the flag; a re-read still cannot lower; power above a lowered ceiling re-expands) so the two paths cannot be merged back into one. Documented in §5 of the design note.

[aethersdr-agent]

Cisco CodeGuard — static analysis of this PR (5 finding(s))

• [MEDIUM] CG-PATH-001 — Potential path traversal in src/gui/MainWindow.cpp /tmp/aetherclaude/pr-5320/src/gui/MainWindow.cpp:8806
• [MEDIUM] CG-PATH-001 — Potential path traversal in src/gui/MainWindow.cpp /tmp/aetherclaude/pr-5320/src/gui/MainWindow.cpp:8918
• [MEDIUM] CG-PATH-001 — Potential path traversal in src/gui/MainWindow.cpp /tmp/aetherclaude/pr-5320/src/gui/MainWindow.cpp:9037
• [MEDIUM] CG-PATH-001 — Potential path traversal in src/gui/MainWindow.cpp /tmp/aetherclaude/pr-5320/src/gui/MainWindow.cpp:9038
• [MEDIUM] CG-PATH-001 — Potential path traversal in src/gui/MainWindow.cpp /tmp/aetherclaude/pr-5320/src/gui/MainWindow.cpp:9039

_{Automated static scan by Cisco DefenseClaw CodeGuard on the changed files. Advisory — some may be false positives; the review above verifies them.}

---

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

[NF0T]

All four inline findings are fixed in 6c5deb5c, each answered on its own thread. Plus the two non-inline items, and one correction to the review's attribution.

Blocker and nits

Finding	Status
setAutoReconnect never wired	Fixed — both propagation sites
ResponseParser::feed() unbounded after a marker	Fixed — bounded resync, 2 regression rows
reflectedWattsFromSwr stale pointer	Fixed — points at Reading::coherent / applyDimming()
Lowering a ceiling silently no-ops	Fixed — CeilingSource split, 3 regression rows
m_gateSettleTimer perpetual restart	Fixed, with a caveat below
setDeviceInt truncation	Not taken — see below

Tests go 49 → 56 assertions, all passing. Colour ratchet +0/+0/+0, gen_touchpoint_manifest.py --check clean, full app builds.

m_gateSettleTimer — your mechanism was right and my first fix was wrong

I initially traced it as never re-firing, since a 100 ms tick would keep resetting a 1500 ms single-shot. Wrong: the restart is conditional, so after each expiry exactly one tick re-arms it and the cycle is ~1.6 s forever, as you said.

But dropping the clause outright loses something. At connect, m_ridingAlongSeen is false and the gate is not riding along, so a pure change-only test never arms the timer and the opening POLLING/SHARED line disappears from the support bundle — the one log line that distinguishes a genuine hand-off from a broken poll loop after the fact. Kept a latched !m_gateStateReported clause so it arms exactly once per connection and never again.

setDeviceInt truncation — not taken, and I think correctly

The cast is already explicit at the call site (static_cast<int>(c.highW)), and QInputDialog::getDouble is called with 0 decimals, so a fractional full scale is unreachable twice over. Added a comment recording that rather than changing code that has no reachable defect.

The manifest churn was two different things, and one of them was mine

You filed both files under regeneration side-effect. Half of that is right; half isn't, and the difference matters because only one half is fixable.

aetherd-touchpoints.md — genuinely not this branch's. I ran the control: checked out b32d035 clean, with none of this branch's code present, and ran the generator there.

$ python3 tools/gen_touchpoint_manifest.py --check
::error::aetherd-touchpoints.md is stale — run `python tools/gen_touchpoint_manifest.py`
exit 1

main's committed manifest is already stale. Regenerating on pristine main reproduces every header you listed — CtcssTones.h, SystemInfo.h, SystemInfoCollector.h, ThreadCpuRing.h, ConnectionSharingPolicy.h, RadioCapabilities.h — and every drifted count (AppSettings 102→101, LogManager 30→31, BandDefs 4→5, BandPlanManager 8→9). Totals go 199 → 205 from staleness alone, then 205 → 207 for this branch's two headers. The single line genuinely mine is ThemeManager.h 141 → 142, from LpMeterApplet.cpp including it. That half has to stay — hand-trimming it would make --check fail.

aetherd-touchpoint-tags.json — mine, and now fixed. The generator never writes this file; it is the human-authored sidecar, so "regenerator churn" was the wrong cause. What happened is an encoding normalisation: the file was committed mixed (29 escaped —, 12 literal —), and my edit re-serialized it with ensure_ascii=False, converting 27 unrelated entries. Rebuilt in 20395d98 as the base file plus the one core/LpMeterConnection.h block. The diff is now 6 added lines and nothing else.

Worth flagging beyond this PR: --check exists but is not wired into any workflow, so main drifts and the next contributor to regenerate inherits the whole backlog in their diff. That looks like a standalone cleanup — regenerate main, add --check to static-checks — rather than anything this PR should carry.

On the -R-filtered ctest

Independently confirmed — every ctest invocation in ci.yml is filtered, so a green run does not mean lp100a_protocol_test executed. Worth adding that no peripheral protocol test is in the gate: not acom_protocol_test, not spe_protocol_test. This one matches every peer exactly, so it is not a gap this PR should close unilaterally. You filed it as a note rather than a finding, which is the right call.

Also confirmed your CodeGuard refutation independently before reading it — git blame puts all five on #2434, #3100 and #2449, and two of them are QLabel string literals rather than path operations at all.

73,

Ryan NF0T

---

👨🏼‍💻 Co-authored by Claude Opus 5

[NF0T]

@AetherClaude you may re-review at your convenience

[aethersdr-agent]

Human-directed independent review (Claude / @NF0T)

1. Issue fit

Yes, fully — and it settles the open questions #5315 left open rather than papering over them. Every acceptance item in the issue maps to a hunk: LpMeterProtocol pure + unit-tested (tests/lp100a_protocol_test.cpp, registered at tests/tests.cmake:1982-1988, links Qt6::Core only as promised), LpMeterConnection under src/core/ outside the radio seam, a dedicated LpMeterApplet, markHardwareConditional("LP100") + Metering category + absent from kDefaultOrder (AppletPanel.cpp:830-834, :1350), Peripherals row, peripheral(lp100a) touchpoint tag. Both "decisions I'd like settled" went the way the issue proposed (#4944-consistent placement; ceilings in the applet context menu, LpMeterApplet.cpp:368-408).

The issue's own 41-vs-42 open question is resolved by capture and — correctly — by not validating on length: looksLikeRecord() checks field count, separator placement (only at the known-good length), per-field parseability and physical range, and the test pins the manual's literal 41-char example decoding alongside the 42-char capture and a hypothetical 43-char wide-dBm record. That is the right shape.

2. Scope

Three files in this diff have nothing to do with the LP-100A, and two of them are governance-owned.

File / group	What it changes	Claimed by title/body?	Verdict
src/core/LpMeterProtocol.{h,cpp}	Pure protocol layer	Yes	In scope
src/core/LpMeterConnection.{h,cpp}	Serial/TCP transport	Yes	In scope
src/gui/LpMeterApplet.{h,cpp}	The applet	Yes	In scope
src/gui/AppletPanel.{h,cpp}, MainWindow{,_Wiring}.{h,cpp}, RadioSetupDialog.{h,cpp}	Registration, wiring, Peripherals row 8	Yes	In scope
CMakeLists.txt, tests/tests.cmake, tests/lp100a_protocol_test.cpp	Build + unit test	Yes	In scope (Tier 2)
THIRD_PARTY_LICENSES, docs/architecture/*	Provenance + design note + tag	Yes	In scope (Tier 1/2)
tree.json (+42,301 lines)	Generated QWidget tree dump from tools/automation_probe.py:485, committed at repo root	No	Blocker — remove
.claude/settings.local.json.bak-20260725-202211	A personal Claude Code settings backup	No	Blocker — remove
.claude/commands/review-pr.md (new, 75 lines)	A new PR-review slash command	No	Blocker — unbundle
docs/architecture/aetherd-touchpoints.md (6 unrelated header rows, AppSettings.h 102→101)	Manifest regeneration drift	Partially	Non-blocking, see nits

3. Blockers

1. tree.json — a 42,301-line generated artifact committed at the repo root. (inline)

git log puts it in 6c5deb5c "fix(lp100a): five defects from the #5320 review", whose message does not mention it. It is the output of tools/automation_probe.py (tree_path = os.path.join(args.out, "tree.json"), line 485) — a dump of a running app's widget hierarchy, with geometry and objectName for every widget on that machine. It is not in .gitignore, so once merged every future probe run leaves a dirty tree for everyone. Delete it from the branch and add tree.json to .gitignore in a separate one-liner.

2. .claude/settings.local.json.bak-20260725-202211 — a stray local backup. (inline)

Four lines granting WebSearch, dated a month before this work. .gitignore:44 ignores .claude/launch.json specifically and its comment explains the reasoning ("per-developer and per-worktree… a committed copy is wrong for everybody else") — which applies verbatim to a settings.local.json backup. Remove it.

3. .claude/commands/review-pr.md is unrelated new tooling in a CODEOWNERS-gated path. (inline)

A complete 75-line PR-review workflow command, bundled into a wattmeter feature PR. .github/CODEOWNERS:156 assigns .claude/commands/ to @aethersdr/infrastructure, and there is a comment at :152 explaining that line exists specifically so this directory is not swept up by a broader pattern. There is a certain irony in the file's own Tier table listing .claude/commands/ as Tier 1 while arriving inside a Tier 3 feature PR. It may well be a good command — it needs its own PR and its own approver.

None of the three is a code defect; all three are unbundling asks, and the LP-100A code itself is not blocked by them.

4. Nits (non-blocking)

• docs/architecture/aetherd-touchpoints.md carries manifest drift beyond this PR. The totals go 199→207, but only two of the eight new headers are yours (LpMeterConnection.h, LpMeterProtocol.h). CtcssTones.h, SystemInfo.h, SystemInfoCollector.h, ConnectionSharingPolicy.h, RadioCapabilities.h and ThreadCpuRing.h are pre-existing headers the regeneration swept in, and AppSettings.h drops an includer (102→101) which nothing in this diff can explain. That is regeneration correcting accumulated drift, not a problem — but the body should say so, because a reviewer diffing the totals against your two files will not reconcile them.
• PollGate::m_lastForeignMs and m_prevForeignMs are always assigned the same value (LpMeterProtocol.cpp:377-378) and only the former is read. One of them is redundant state.
• setCeilings(OperatorEdit) clears m_autoExpanded even when the edited range is not the displayed one. Editing Low while displaying an auto-expanded High discards the expansion for High. Self-correcting within kCeilingExpandRecords, and the header already argues that self-correction is acceptable — noting it only because the comment reasons about the same-range case.
• docs/architecture/README.md: the new bullet is missing the blank line its neighbours have before the next entry.

5. Bot findings I could not confirm

• CodeGuard CG-PATH-001 ×5, src/gui/MainWindow.cpp:8806/8918/9037-9039 — refuted, all five. Those lines are untouched by this PR. 8806 is the NR2 wisdom dialog's QLabel text; 9037-9039 are the macOS relaunch path walking up from QCoreApplication::applicationFilePath() to the .app bundle. No attacker-controlled path component, and no line in the LP-100A diff appears in the finding set.
• The two earlier aethersdr-agent blockers are genuinely fixed, verified independently rather than taken on the author's word: m_lpMeterConn.setAutoReconnect(ar) is now in the startup block (MainWindow_Wiring.cpp:6049) and in the live toggle (RadioSetupDialog.cpp:7964); and the unbounded-buffer branch now drops its marker and resyncs at LpMeterProtocol.cpp:247-256, with a regression row at lp100a_protocol_test.cpp:512-530 that feeds 12.8 kB of x after a bare ; and asserts bufferedBytes() <= kRecordLength * 4 + 1. The bot's third note (the stale "per-field liveness tracking in LpMeterConnection" comment) is fixed at LpMeterProtocol.h:216-219 and now points at LpMeterApplet::applyDimming(), which is where zLive = live && m_reading.coherent actually lives (LpMeterApplet.cpp:322).

6. What I tried to break — and failed

• M_PI on MSVC. LpMeterProtocol.cpp:302 and the test at line 118 use bare M_PI with only <cmath>, which is the recurring check-windows break on this project. Not a finding here: src/core/MaidenheadLocator.h:112, RttyDecoder.cpp:107 and IcomCivBackend.cpp:3077 use bare M_PI with the same <QString>/<cmath> include shape and build on Windows today, because Qt 6's platform target supplies _USE_MATH_DEFINES to everything linking Qt. Both the library and lp100a_protocol_test link Qt6::Core, so both inherit it.
• Attacked the tests rather than trusting them. They are not self-referential: the manual's example is quoted verbatim rather than produced by our own encoder, the captures are literal bytes, and the physics cross-check at lines 114-128 re-derives SWR from |Z|∠φ independently of swrFromImpedance() — so it would catch a field-order error the parser and the test agreed on. The 42-char-with-misplaced-separator row (line 173) fails against a length == 42 implementation, and the negative-dBm row fails against an unsigned parse. looksLikeRecord reduced to a length check would fail at least four rows.
• Parser resync and buffer growth, per above — and I walked the fixed-length emit path (:224-232) for double-emission: consume = 1 + kRecordLength followed by the trailing indexOf realignment leaves no window for a record to be decoded twice.
• Reconnect ordering. connectNetwork() sets m_deliberateDisconnect = false before teardownDevice(), so aborting a live socket to reconnect can arm the 5 s reconnect timer just after m_reconnectTimer.stop(). Real, but byte-identical to AcomConnection.cpp:127-129 — a pre-existing pattern, not this PR's to change.
• Constitution / AGENTS.md. Nothing radio-authoritative is cached or re-asserted (Principle II is not engaged — this device is outside the radio seam entirely). No TX keying, no markTxKeying() surface. Persistence goes through PeripheralSettings::setDeviceInt/deviceString under a "Lp100a" device key, matching the ACOM/SPE/VKAMP peripherals; no credentials, no radio_settings scope needed. No CHANGELOG.md entry — correct. GOVERNANCE.md's "new applets or dialogs that don't change existing UX" exemption is quoted accurately in #5315; no RFC required.
• Personal-preference check: clean. Every changed line in an existing file is additive registration or wiring. No default altered, no existing UI restyled, no confirmation removed. The one deliberate divergence from a peer (leaving DTR/RTS alone, unlike AcomConnection) is documented at its point of use with the measurement behind it.
• Could not check: anything at runtime. No build, no test execution, no GUI — every finding above is reasoned from the head checkout at /tmp/aetherclaude/pr-5320, not reproduced. I also cannot independently verify the hardware captures, the 954-record counts, or the 60-records-in-6-s shared-transport measurement; those rest on the author's bench and are stated as such in the design note, which is the right way to carry them.

7. Recommendation

Request changes — for the bundling only. The LP-100A implementation is the most carefully evidenced peripheral integration I have reviewed on this project: the design note distinguishes what was measured from what remains unverified, names where the manufacturer's manual is wrong and why the wire wins, audits each borrowed behaviour with an explicit adopt/improve/reject verdict, and the two protocol facts that read as gaps (unsigned phase, non-snapshot records) are documented at their point of use with tests pinning them. I found no correctness defect in the LP-100A code.

What blocks it is three files that are not the LP-100A: a 42k-line generated tree.json, a stray .claude settings backup, and a new CODEOWNERS-gated slash command. Drop the first two, split the third into its own PR for @aethersdr/infrastructure, and this is ready.

Review event is COMMENT — a human maintainer makes the merge call. Nice work, and thank you for bringing hardware to it.

---

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

---

GitHub rejected the inline anchors (GitHub API 422: Unprocessable Entity); findings below:

tree.json:1
Blocker — generated artifact, 42,301 lines, unrelated to the LP-100A.

This is the output of tools/automation_probe.py (tree_path = os.path.join(args.out, "tree.json"), line 485): a dump of a running app's QWidget hierarchy including per-widget geometry and objectName from whichever machine ran the probe. git log --oneline -1 -- tree.json puts it in 6c5deb5c "fix(lp100a): five defects from the #5320 review", which does not mention it — so it looks like an accidental git add.

It is also not in .gitignore, so after merge every future probe run leaves a dirty working tree for every developer.

Please drop it from the branch. Adding tree.json to .gitignore is worth a separate one-liner so this cannot recur.

.claude/settings.local.json.bak-20260725-202211:1
Blocker — a personal machine artifact, unrelated to this PR.

A four-line backup of a local Claude Code settings file, timestamped 20260725, roughly a month before this work started.

.gitignore:44 ignores .claude/launch.json and the comment above it states the principle exactly: "Per-developer and per-worktree… a committed copy is wrong for everybody else." A settings.local.json backup is the same category — settings.local.json is per-developer by construction, which is what the .local infix means.

Please remove it from the branch.

.claude/commands/review-pr.md:1
Blocker — unrelated new tooling, and in a CODEOWNERS-gated path.

A complete 75-line PR-review workflow command, bundled into a TelePost wattmeter feature PR. Nothing in #5315 or the PR title asks for it.

Two reasons it needs its own PR rather than a fixup here:

1. .github/CODEOWNERS:156 assigns .claude/commands/ to @aethersdr/infrastructure, and the comment at :152 says that line exists specifically so this directory is not swallowed by a broader pattern — i.e. the gating is deliberate, not incidental. A reviewer approving the LP-100A code is not thereby approving this.
2. A shared review methodology deserves discussion on its own merits. Buried at the bottom of a 24-file feature diff, it will be approved by inattention.

Worth noting, gently: this file's own Tier table lists .claude/commands/ as Tier 1 requiring @ten9876, while arriving inside a Tier 3 feature PR. That is the rule it states, applied to itself.

No objection to the content — please split it out.

src/core/LpMeterProtocol.cpp:378
Nit, non-blocking: m_lastForeignMs and m_prevForeignMs are unconditionally assigned the same value on every foreign record, and only m_lastForeignMs is ever read (shouldPoll(), line 383). m_prevForeignMs is read at line 369 — but only to compute a gap against a value it is about to be set to, which m_lastForeignMs would serve identically.

The two-name split reads as if it encodes a distinction (last-vs-previous) that the code does not actually make, which will cost the next reader a minute. Either collapse them, or add a line saying they are deliberately kept separate for a future change.

[aethersdr-agent]

Cisco CodeGuard — static analysis of this PR (5 finding(s))

• [MEDIUM] CG-PATH-001 — Potential path traversal in src/gui/MainWindow.cpp /tmp/aetherclaude/pr-5320/src/gui/MainWindow.cpp:8806
• [MEDIUM] CG-PATH-001 — Potential path traversal in src/gui/MainWindow.cpp /tmp/aetherclaude/pr-5320/src/gui/MainWindow.cpp:8918
• [MEDIUM] CG-PATH-001 — Potential path traversal in src/gui/MainWindow.cpp /tmp/aetherclaude/pr-5320/src/gui/MainWindow.cpp:9037
• [MEDIUM] CG-PATH-001 — Potential path traversal in src/gui/MainWindow.cpp /tmp/aetherclaude/pr-5320/src/gui/MainWindow.cpp:9038
• [MEDIUM] CG-PATH-001 — Potential path traversal in src/gui/MainWindow.cpp /tmp/aetherclaude/pr-5320/src/gui/MainWindow.cpp:9039

_{Automated static scan by Cisco DefenseClaw CodeGuard on the changed files. Advisory — some may be false positives; the review above verifies them.}

---

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

[aethersdr-agent]

Antares Detector — candidate vulnerable file(s)

• src/core/WsjtxClient.cpp
• src/core/WsjtxClient.h

The WsjtxClient uses QDataStream to parse binary protocol data, and its readQString helper reads raw bytes from the stream into a QString buffer without validating that the serialized length is bounded by available memory or the expected packet size. This can lead to out-of-bounds allocation/copying when parsing malformed packets.

_{Localized by Cisco Foundation AI Antares-1B running locally in the AetherClaude sandbox, seeded by the Cartographer security map. Advisory only — please verify before acting.}

---

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

[NF0T]

Thank you — this is the review that caught the thing I couldn't see. All three blockers are gone from history, not just from the tip, and the three nits are fixed. Head is 07cc2585; 21 files, +3410/−8, down from 24 files and +45,748.

The three files, and how they got in

They are not artifacts of this work. All three were already untracked in my working tree before the branch existed — tree.json from an automation-probe run, the .claude backup dated a month earlier, and a local slash command.

The mechanism is worth writing down because it is reusable and it is not "I didn't look":

• I reviewed that commit with git diff --stat, which reports tracked modifications only. It showed the eight files I expected.
• I then committed with git add -A, which stages untracked files too.

The review step and the commit step had different scopes, so the check I ran was structurally incapable of showing what the commit was about to do. And because all three files pre-dated the branch, nothing in my own work drew attention to them. You spotted the tell I should have: 6c5deb5c's message doesn't mention tree.json, because I didn't know it was there.

The other ten commits used explicit paths and were clean — I verified each one. 07cc2585 was staged with explicit paths, which is the actual fix; the removal was just cleanup.

Removed by history rewrite, not a follow-up commit, so git log -- <those paths> across the branch now returns nothing and a rebase-merge is as safe as a squash. Verified before force-pushing: the rewritten tree is byte-identical to the pre-rewrite tip (git diff empty — history changed, content didn't), and all 12 commits are signature-verified. Worth flagging for anyone reaching for the obvious tool: git filter-branch would have been easier and produces unsigned commits, which breaks branch protection in a way you'd only discover after pushing. Cherry-pick with -S avoids that.

Not bundled, per your reasoning on both:

• tree.json → .gitignore. Agreed it will recur for everyone after merge. It's in my local .git/info/exclude for now so it can't recur here; the .gitignore line wants its own one-liner.
• .claude/commands/review-pr.md → its own PR. Your point about approval-by-inattention at the bottom of a 24-file diff is the right one, and CODEOWNERS:156 gating that directory to @aethersdr/infrastructure settles it independently. Noted the irony about its own Tier table.

Nits — all three fixed

m_prevForeignMs / m_lastForeignMs. Confirmed and collapsed. Initialised together, reset together, assigned the same value on adjacent lines, each read in exactly one place. Your diagnosis of the cost is the right one: the two-name split implies a last-vs-previous distinction the code never made.

setCeilings(OperatorEdit) clearing m_autoExpanded across ranges. Confirmed, and I think it deserved more than the "self-correcting" framing you generously gave it. The menu edits one range while the gauge follows whichever the meter reports, so editing Low while High was displayed overwrote High's expanded ceiling with its unchanged configured value and cleared the flag. It recovers within kCeilingExpandRecords — but a scale that visibly jumps because the operator edited a different range is its own defect, not a slow path to the right answer. Now gated on whether the displayed range's configured value actually moved. Two rows pin it in both directions so it can't widen into "operator edits never apply".

docs/architecture/README.md blank line — fixed.

Tests 56 → 58 assertions. Ratchet +0/+0/+0, gen_touchpoint_manifest.py --check clean, full app builds.

On the manifest drift

Agreed, and I have the control for it. Regenerating on a pristine b32d035 worktree with none of this branch's code present reproduces every one of those six headers and every drifted count — gen_touchpoint_manifest.py --check exits 1 there. main's committed manifest is already stale; totals move 199 → 205 before this branch adds its two. The single line genuinely mine is ThemeManager.h 141 → 142, from LpMeterApplet.cpp including it. Your read is right and the PR body should say so.

Root cause beyond this PR: --check is wired into no workflow, so main drifts and the next contributor to regenerate inherits the whole backlog in their diff. Worth its own PR — regenerate, then add --check to static-checks.

Independent agreement on two of your other points, checked before reading yours: CodeGuard's five are all pre-existing (git blame puts them on #2434, #3100, #2449, and two are QLabel string literals rather than path operations at all), and the M_PI/MSVC question is settled by check-windows being green.

73,

Ryan NF0T

---

👨🏼‍💻 Co-authored by Claude Opus 5

[rfoust]

Thanks for the thorough implementation and for addressing the earlier review threads. I reviewed the current-main merge result at base b32d035a / head 07cc2585, built the full application locally on ARM64, ran the focused protocol test, and ran the strict boundary, registration, accessibility, and diff checks. CI is also green on Linux, macOS, Windows, and static checks.

I found three correctness blockers:

1. Reject unbounded and noncanonical numeric fields before they reach the gauges. looksLikeRecord() enforces separator/field widths only when the body is exactly 42 bytes, while parseDouble() accepts exponent notation and any finite double. I reproduced an oversized parser-bounded record reaching Reading as 1e+100 watts; a canonical-width exponent-form value can reach the same path. After two readings, RangeTracker auto-expands the ceiling, LpMeterApplet converts it to float infinity, and evenTicks() converts infinity/NaN to int. Since this protocol has no checksum, the boundary validation is the integrity mechanism. Please require field-specific lexical formats and documented widths/variants, impose generous physical maxima, defensively reject nonfinite GUI values, and add regressions for exponent-form, oversized-power, and extra-field-byte records. (src/core/LpMeterProtocol.cpp:25, :91; src/gui/LpMeterApplet.cpp:56, :193.)

2. Correct return loss at SWR 1.0. returnLossDb() returns 0 dB for a perfect match, and the focused test explicitly pins that result. At SWR 1, the reflection coefficient is zero, so return loss approaches positive infinity; 0 dB represents total reflection. Please represent the limit safely—either infinity with appropriate formatting or the meter's documented display ceiling—and update the test. (src/core/LpMeterProtocol.cpp:291; tests/lp100a_protocol_test.cpp:249.)

3. Cancel a pending retry when auto-reconnect is disabled. setAutoReconnect() only flips the flag, while an already armed timer callback never rechecks it. If the operator disables the option during the five-second delay, the meter still reconnects once. Please stop the timer when disabling and/or guard the callback, and add a timer-lifecycle regression. (src/core/LpMeterConnection.h:74; src/core/LpMeterConnection.cpp:53.)

The overall issue fit and scope are strong: protocol, serial/raw-TCP transport, read-only polling, applet/settings integration, tests, and provenance/design documentation all belong here. I found no remaining unrelated artifacts in the current delta. The four existing review concerns appear addressed in the current code, although their threads remain unresolved.

After these correctness fixes, the disclosed range-default and applet/menu-placement choices still need maintainer/CODEOWNERS judgment.

No application launch, hardware contact, or TX activity was performed during this review.

[NF0T]

Thank you — all three are real, all three are fixed in 4430d900, and CI is green on all four jobs. I reproduced each one against the built library before changing anything rather than reasoning from the diff, and that turned up things worth reporting back.

1. Boundary validation — confirmed, and broader than your description

You framed this as offsets being enforced only at exactly 42 bytes. That's true, but the case that matters defeats the obvious fix:

A exponent, len!=42                len=41 looksLikeRecord=1 powerW=1e+100 asFloat=inf
B exponent, len==42 canonical      len=42 looksLikeRecord=1 powerW=1e+100 asFloat=inf   <-- offsets PASSED
D via ResponseParser: records=1 powerW=1e+100
E RangeTracker ceilingW=1e+100  asFloat=inf finite=0

01e+100 is exactly seven characters — the canonical Power width — so it satisfies the separator-offset check as well. Widening or always-applying that check would have fixed nothing. The real gap is that no field had a lexical constraint or an upper bound.

Your UB call is right, and it's worse than one cast: evenTicks() iterates {0.0, 0.25, 0.5, 0.75, 1.0}, and inf * 0.0f is NaN, so the first tick was already UB. On this target it yields -2147483648 and 2147483647 as axis labels rather than crashing, which is a target-specific accident, not a mitigation.

Three gaps beyond the ones you cited, found while reproducing:

SWR, Z, dBm	equally unbounded — 1e300 accepted for each; you named only power
callsign	no length cap — a 200-character callsign accepted straight into a QLabel, against a field documented as 6

Credit where the existing guard held: inf, nan, hex and overflow-to-infinity were all correctly rejected by the isfinite check. It's finite-but-absurd that got through.

On "generous physical maxima": I went a slightly different way, because I couldn't justify any specific invented number. The bounds are now read off the measured wire format — n characters cannot express a magnitude of 10ⁿ, and a 7-character Power field in 0000.00 form tops out at 9999.99. Strict per-field lexical form (digits, at most one decimal point, sign only on dBm, no exponent) plus a per-field width cap, with the one documented variant (dBm 4 or 5) preserved. evenTicks() also rejects a non-finite scale on its own account, since a GUI helper shouldn't depend on its caller having validated. After:

A / B / C exponent + oversized             rejected
huge SWR / huge Z / huge dBm               rejected
200-char callsign / hex / stray sign       rejected
canonical, captured idle, captured TX,
the manual's 41-char example, dBm 5-char   all still decode
E RangeTracker ceilingW=1500  finite=1

One thing the fix itself exposed. looksLikeRecord() and decodeReading() parse the same fields and must agree — they diverged mid-patch, and because decodeReading() ignored parseDouble()'s return value it published a silently defaulted dBm instead of refusing the record. Both now share a single signAllowedFor() policy and decodeReading() checks the returns. That latent fragility was pre-existing and I'd have missed it without your finding.

2. Return loss — you are right, and it was wrong twice over

This is the catch of the review. Two AI reviews passed over it.

returnLossDb: swr=1.0 -> 0   swr=1.001 -> 66.02   swr=3.0 -> 6.02   swr=1e9 -> 1.7e-08

SWR 1.001 gives 66 dB; SWR 1e9 — near-total reflection — gives ≈0 dB. The special case snapped a perfect match to the value for a dead short, and rendered it throughout receive, since the meter idles at SWR 1.00.

The comment made it worse by asserting the opposite while claiming the edge was handled: "0.00 dB at a perfect match, which is the division-by-zero edge every naive implementation gets wrong." It isn't a division by zero — γ = (1−1)/(1+1) = 0 — the divergence is log10(0). And a unit test pinned the wrong value, so the suite protected the defect rather than catching it. The replacement asserts monotonicity (a near-match must be large, and larger than SWR 3.0) so an inverted implementation cannot pass by matching one point.

On the display, I took your second option but derived the ceiling rather than picking one. The function now returns +infinity, which is correct. The applet renders RL >52 dB — because the SWR field carries two decimals, a reported 1.00 means [0.995, 1.005), which justifies no more than ~52 dB. Rendering ∞ would claim precision the wire doesn't carry. The bound comes from the field's own quantisation, so there's no chosen constant to defend.

What the LP-100A itself shows at a perfect match is undocumented and unobserved by us — it's now in the design note's unverified list. If it has a house convention, matching the instrument beats our derivation and I'll change it.

3. Reconnect cancel — confirmed, with context you didn't have

Fixed both halves: the setter stops the timer, and the callback re-reads the flag.

All three sibling peripherals are byte-identical and have the same defect:

AcomConnection     void setAutoReconnect(bool on) { m_autoReconnect = on; }
SpeConnection      void setAutoReconnect(bool on) { m_autoReconnect = on; }
VkampConnection    void setAutoReconnect(bool on) { m_autoReconnect = on; }

That doesn't excuse it here — copying a working sibling's defect is exactly what the design note's standing rule exists to prevent, and I'd audited that class and missed it. But it does mean this is now a deliberate divergence from three peers, which is documented as such rather than left to look like an inconsistency. Their identical bug is a separate follow-up, not this PR's to fix.

Regression added as you asked: tests/lp100a_reconnect_test.cpp, a new target declared in tests/tests.cmake per #5002, exercising the lifecycle through the public API — a failed connect arms the retry, disabling cancels it, re-enabling doesn't resurrect it, a deliberate disconnect arms nothing. It arms the timer from a loopback port nothing listens on (no external network, no hardware) and skips with exit 77 where loopback is unavailable, because a flaky red is worse than a gap.

Verification

Tests 58 → 75 assertions in the protocol suite plus 4 in the new target. Colour ratchet +0/+0/+0, gen_touchpoint_manifest.py --check clean, full application builds, and full local ctest shows no new failures — the three that fail (bridge_docs_check, hl2_state_restore_test, phone_tx_filter_numeric_entry_test) also fail on a clean upstream/main worktree, which I checked rather than assumed. All four CI jobs green on 4430d900.

Design note updated in three places: the boundary-validation reasoning in §3 (including why kCeilingExpandRecords = 2 depends on it, so nobody weakens one without the other), the return-loss correction and its unverified display convention in §8, and the sibling divergence before §9.

The four earlier threads are unresolved because I can't resolve my own — happy for them to be closed out whenever you or the bot are satisfied.

73,

Ryan NF0T

---

👨🏼‍💻 Co-authored by Claude Opus 5