Repository navigation
feat(anan): publish link telemetry and withdraw the PA readout. Principle VIII. - #6137
Conversation
There was a problem hiding this comment.
Issue fit
No linked issue — the body is the spec, and I reviewed against it. Both stated goals land. I traced the Network: claim end to end: m_panStream is assigned only on the Flex and Sim paths (RadioModel.cpp:832, :856), so it is null for ANAN, usesBackendLinkStats() reduces to m_linkStats.reported, and applyBackendLinkStats() (RadioModel.cpp:1399) early-returns on !reported — so the new linkStats() genuinely engages the readout and the idle case genuinely doesn't. GOVERNANCE.md wants an RFC for architectural changes and explicitly exempts bug fixes with a clear root cause; this is two contained fixes plus one new capability record, which I've flagged below as the maintainer's call rather than a violation.
Scope
| File(s) | Change | Claimed in body | Verdict |
|---|---|---|---|
anan/P2Client.{h,cpp} |
rx/tx/packet counters, 1 s snapshot timer, localEndpoint(), required txAccum on sendTo() |
yes | in scope — the required-parameter choice is what makes the nine touched call sites self-explaining |
anan/AnanBackend.{h,cpp} |
linkStats(), onLinkCounters(), two capability declarations |
yes | in scope |
anan/P2Protocol.{h,cpp} |
decode supply rail, bytes 49–50 | yes, and explicitly flagged as decoded-but-unconsumed | maintainer decision — a new public struct field with zero consumers anywhere in src/. Author discloses it and asks; it is tested and documented, so I'd accept it, but it's a ruling not a review call |
RadioCapabilities.h + docs/architecture/radio-capabilities-map.md |
new paTelemetryAudit record + map row |
yes | maintainer decision — new capability surface. The #5262 M2 argument checks out: I ran tools/check_capability_records.py --strict on the head and it reports 70 boolean(s), at the frozen 70 — ok (shrink only), so the nested-bool-in-optional shape does pass, and AGENTS.md's capability trio (record + caps-map row + gating test) is satisfied |
gui/MainWindow.{h,cpp} |
the PA-temperature gate, m_paStack handle, rewritten rationale comment |
yes | in scope |
tests/anan_{backend,p2_protocol,link_telemetry}_test.cpp, tests/tests.cmake |
coverage + registration | yes | in scope |
Nothing in the diff is unexplained; no CHANGELOG.md entry was added (correct). No deleted guard — the one removal is the empty dropsUpdated lambda, and drops still reach the readout through LinkCounters::drops.
Socket test — recorded here, as canon asks
tests/anan_link_telemetry_test.cpp calls client.start(), which binds a QUdpSocket (QHostAddress::AnyIPv4, ephemeral port, P2Client.cpp:112) and sends to 127.0.0.1. Target anan_link_telemetry_test, registered at tests/tests.cmake:1082, SKIP_RETURN_CODE 77 set. It is our own client with no fake peer, so the fake-firmware ban isn't engaged. CI: it is not on the per-PR gate — ci.yml's only ctest -R steps name four DV tests and cross_needle_meter_test — so it runs at merge on full-suite.yml and weekly under the sanitizers. This entry is the notification; it is not a finding.
Blockers
None. Nothing here breaks users, violates canon I could cite, or fails on current main (all five checks green on 840cf4ff).
Nits (non-blocking)
- The counter-cache reset sits on one path, but the thing it compensates for is
P2Client::start(). See inline onAnanBackend.cpp:1434. Narrow, with a one-line self-heal. - Hiding
m_paStackleaves two adjacent separators, and the min-width refresh runs before the hide. See inline onMainWindow.cpp:7791. There is a precedent for the separator half in the same function. - A bind failure can turn earlier red checks into a green skip. See inline on
anan_link_telemetry_test.cpp:151. - Socket-test disclosure.
docs/agents/tests-ci.md:76-80: "any new socket-owning test is disclosed in the PR body, itstests.cmakeblock names the socket it binds … and the test fails fast (or skips, exit 77) when it cannot bind." The exit-77 half is done; the Evidence section never says a new test binds a UDP socket, and thetests.cmakeblock says "Loopback send only" without naming the bind. One sentence in each closes it. - Foreign UDP can hold the new
aliveflag true — security-audit (CodeGuard) lead, confirmed as code fact and routed as a maintainer question. See inline onP2Client.cppat the counter-ingest hunk. - Stale comment left behind.
tests/spectrum_sequence_gap_test.cpp:25-26still reads "On the ANAN side P2Client'sdropsUpdatedreaches a deliberately empty lambda inAnanBackend." This PR deleted that lambda. Comment-only, no assertion depends on it. - The subtlest logic in the PR is the least covered.
onLinkCounters()'s first-snapshot-vs-zero rule and thealivecomparison carry a six-line justification and no test;anan_backend_testonly pins the idlereported == falsecase. I'm not asking for a test:onLinkCountersis private andm_clientis backend-owned, so there's no deterministic seam without widening access. Worth naming because the mutation list claims coverage of "LinkStatsclaiming a measured transport while idle" — which is the one branch that is tested — and not of the comparison itself.
What I tried to break
- "Zoom restarts the session and strands the cache." This looked like a blocker:
startP2ClientSession()claims to be "the one place BOTH a first connect and a rate change pass through" and branches onisRateChangeto callm_client->start(), which zeroes the counters. It does not hold —finishRateChange()sendssetDdcRateLive()to the running session and never restarts the client, andfinishDspSetup()states it is "only ever reached from connectRadio()'s first-connect path now". Those two comments contradict each other and thelinkUphandler's "a live rate change is a real stop+restart of this session"; the code is right and the comments are vestigial. Not this PR's to fix, but it is what made the surviving cache finding look bigger than it is. - Out-of-bounds on the new decode.
readU16be(data.data() + 49)reads bytes 49–50 underif (data.size() != kHighPriorityStatusBytes)— an exact-size guard admitting only 60 bytes, placed before the indexing. In bounds. The byte-order test is real too: 540 is0x021C, so it pins against both the swapped read (7170) and the single-byte read (2). - The test's friend access.
friend struct P2ClientTestAccesspre-exists atP2Client.h:192and two other tests already define their own copy of that struct in their own translation units, so the third copy is fine and won't ODR-clash. - Does the withdrawal get undone? I read every writer of
m_paTempLabel.updatePaTempLabel()(MainWindow.cpp:4838) only sets text and tooltip — it never callssetVisible, so no telemetry tick re-shows a withdrawn row.isVisibleTo(m_paStack)is the right API here: both labels are in aQVBoxLayoutparented topaStack(:5976). - Counter arithmetic and threading.
quint64, reset per session, unsigned-wrap-defined anyway. The counters are written only fromhandleDatagram()/the send paths,publishLinkCounters()is a timer slot on the same object (so same thread as the writers), and the only cross-thread egress is the auto-queuedlinkCountersUpdated(AnanBackend.cpp:261).m_linkCountersTimeris parented tothis, stopped instop(), andstart()returns before starting it when the bind fails. - Would the ordering test pass against the unfixed code? No — the 3-byte runt case asserts
rxBytes == 60 + 3after a datagram every parser rejects, which only holds if the increment precedes the shape checks. That assertion is about where counting happens, as the body claims. - Distrust green. Five checks pass on
840cf4ff, but as noted above the per-PR gate doesn't run any of the three touched test targets, so "green" here means build + static checks, not that the new assertions ran in CI. I could not verify the body's627/627or the seven mutations.
Unverifiable, stated plainly: the load-bearing premise for the PA withdrawal — "Protocol 2's entire analog payload is six fields and none is a temperature" — cites OutHighPriority.c:145-156 and "spec p.47", neither of which is in this repo (the only OutHighPriority mention is a comment in P2Protocol.h:251). Same for byte 49–50 being AIN6. I have no way to check either, and the whole withdrawal rests on the first. That the record defaults to "not audited" is what keeps the cost of being wrong contained to this one family, which is the right design given the premise can't be checked in-tree.
Everything above is reasoned from reading the head checkout. I have no build, no test run, and no way to drive the GUI, so nothing here was reproduced at runtime.
Recommendation
Approve with nits — with two maintainer decisions attached. The engineering is careful and the two things most likely to be wrong in a PR like this (the thread ownership of the counters, and the blast radius of a new capability) are both right, verifiably: I ran the ratchet script, and the default-nullopt record genuinely cannot touch another family. The nits are all small and none needs to gate a merge. The decisions that aren't mine: whether to accept the unconsumed supply decode and the new capability surface, and whether the transport heartbeat should be source-bound (the security lead). Next step: fix the return 77 ordering and add the socket-test disclosure line, take or decline the two inline one-liners, and let a maintainer rule on the two surface questions.
Thanks for the body — flagging the board-id scaling you implemented and then removed on finding the isSaturn() warning, rather than shipping either version quietly, is the thing that made this reviewable.
🤖 aethersdr-agent · cost: $15.7172 · model: claude-opus-5
ten9876
left a comment
There was a problem hiding this comment.
Second-pass review at head 840cf4ff. This one builds on the earlier aethersdr-agent review, which was reasoned from source only. I built the head, ran the socket-free tests, mutation-tested two claims, drove the status bar against the demo simulator, and checked the protocol premise against p2app's source. The four inline threads from that review are still open and unanswered, and no commit has landed since, so I don't repeat them below. Each one blocks merge until it's resolved (required_conversation_resolution).
Issue fit
There is no linked issue, so I reviewed against the body. The PA half lands, and its load-bearing premise is now verified, not just plausible. The Network: half wires the readout, but the quality verdict it now drives is scaled wrong on this transport (Blocker 1).
The premise the earlier review couldn't check: in laurencebarker/Saturn sw_projects/P2_app/OutHighPriority.c, the main loop writes the analog fields as GetAnalogueIn(4)→byte 6 (exciter), (0)→14 (forward), (1)→22 (reverse), (5)→49 "supply voltage", (2)→57 and (3)→55 (user analogs). The rest of the packet is PTT bits, ADC overflow and peak-hold, FIFO depths and user I/O. There's no temperature field anywhere in the packet. So paTelemetryAudit{temperatureAbsent=true} is a true statement about the wire, and the byte-49 decode reads the right word.
Scope
| File(s) | Change | Claimed | Verdict |
|---|---|---|---|
anan/P2Client.{h,cpp} |
rx/tx/packet counters, 1 s snapshot, localEndpoint(), required txAccum |
yes | in scope; see Blocker 1 on which packets count |
anan/AnanBackend.{h,cpp} |
linkStats(), onLinkCounters(), audit declaration |
yes | in scope |
anan/P2Protocol.{h,cpp} |
supply-rail decode, bytes 49–50, no consumer | yes, disclosed | maintainer decision (as the earlier review said); the decode is correct per p2app |
RadioCapabilities.h + caps map |
paTelemetryAudit record |
yes | maintainer decision; check_capability_records.py --strict → 70 boolean(s), at the frozen 70 — ok |
gui/MainWindow.{h,cpp} |
PA-row gate, m_paStack |
yes | in scope; see the separator nit, now reproduced |
tests/*, tests.cmake |
one new target, two extended | yes | in scope; the new target binds a socket (see below) |
Everything in the diff is explained by the body, and there's no CHANGELOG.md entry. The engine-boundary findings are identical on the merge base d2e85fa3 and on the head (I diffed the per-file ::warning sets and they match). The touchpoint manifest is up to date. check_command_plane, check_test_registration, check_ci_test_gate, check_a11y, check_network_timeouts and check_qsettings are all clean.
Socket-owning test, recorded for the operator
tests/anan_link_telemetry_test.cpp:147 calls client.start(), which runs QUdpSocket::bind(QHostAddress::AnyIPv4, 0) (P2Client.cpp:112) and sends the four startup datagrams to 127.0.0.1 on 1024 and up. There's no fake peer, so the fake-firmware ban isn't engaged. It is still a new socket-owning test, though, and under step 0 I did not build or run it, and I've flagged it to the operator. SKIP_RETURN_CODE 77 is set. The body's disclosure line and the bind ordering are still open in the existing threads on tests/anan_link_telemetry_test.cpp:151.
Blockers
1. src/core/backends/anan/P2Client.cpp:472-473: rxPackets counts every datagram, so the new Network: quality verdict under-reports loss about 4.7× at the default rate.
RadioModel::packetLossPercent() (RadioModel.cpp:8632-8636) is rxPacketsLost / rxPackets over a window (:8352-8353, :8421-8428), and evaluateNetworkQuality() grades it at 3 / 1 / 0.35 / 0.05 % (:8450-8458). The two numbers come from different streams:
rxPacketsLost=m_drops, which counts DDC sequence gaps only (:523-525).rxPackets=++m_rxPacketson every datagram, before parse (:472-473). That includes Mic Data and High Priority Status, whichhandleDatagram()'s own comment says arrive on this socket (:477).
p2app sends Mic Data continuously while the session is active, not just on transmit. OutMicAudio.c sits in while(SDRActive && !InitError), one 132-byte packet per 64 samples at 48 kHz, so about 750 packets/s. DDC0 at the 48 ksps default is 48000 / 238 (VIQSAMPLESPERFRAME 238, OutDDCIQ.c:47), about 202 packets/s. The denominator is therefore roughly 4.7× the stream the numerator measures, before High Priority Status is counted. In practice, 3 % real DDC loss (the "poor" tier) reads as about 0.64 % ("good-ish"), and 1 % reads as about 0.2 %. The readout this PR exists to light up comes out optimistic by one to two tiers on the default configuration. At 1536 ksps the error shrinks to about 12 %, which is why a wide-span bench session wouldn't show it.
The sibling backend already makes the split this needs. MetisClient::handleDatagram() counts rxBytes before the parse (MetisClient.cpp:1698) and ++m_link.rxPackets only after the EP6 test (:1709), so the numerator and denominator cover the same stream. The body's reason for counting before parse (choice #1: a stalled IQ stream with live status packets must not read as dead) holds for bytes, and alive already reads bytes (AnanBackend.cpp:1413). That reason doesn't extend to the packet count.
Suggested fix: leave m_rxBytes where it is, and move the packet increment to just after the DDC index resolves:
// Before any parse decides what this datagram IS: the transport carried it
// either way, and LinkStats::alive reads bytes, so a session whose IQ has
// stalled while Status packets still arrive does not read as a dead link.
m_rxBytes += static_cast<quint64>(bytes.size()); const std::size_t slot = static_cast<std::size_t>(*ddcIndex);
// The denominator rxPacketsLost is a fraction of: sequence-tracked DDC
// frames only, as MetisClient counts EP6. Mic Data and Status would dilute
// RadioModel's loss percentage several-fold at low DDC rates.
++m_rxPackets;The test currently pins the wrong contract. In tests/anan_link_telemetry_test.cpp:88-111, "a Status packet counts" and "a datagram from an unexpected sender port counts as traffic too" assert rxPackets == 1/2/3. Body mutation #2 ("receive traffic counted after the parse") therefore proves this test enforces the miscount. Those assertions should move to rxBytes, and one positive case is needed: a DDC frame from port 1035 increments rxPackets.
Reasoned from source plus p2app; not reproduced. The demo has no P2 transport and I won't touch the live radio. The arithmetic only needs the two cadences quoted above.
Nits (non-blocking)
src/gui/MainWindow.cpp:7787-7792: double separator, reproduced. To exercise the gate, I patchedSimBackend::capabilities()locally to declarepaTelemetryAudit{true}. That was review-only and reverted; the ANAN declaration itself is covered byanan_backend_test. I connected the demo and read the status bar. Without the patch, the PA stack is present:'PA --°F' vis=True x=1015, between separators at x=991 and x=1105. With the patch, the PA stack is gone and the separators sit back to back:' · ' vis=True x=1081,' · ' vis=True x=1105. The bar rendersMem: 332 MB · · Network:. The fix is them_tgxlSeparatorpattern from the same file: keep theaddSep()at:5992and hide it with the stack. The earlier thread onMainWindow.cpp:7791covers this along with the min-width call that runs before the hide.src/gui/MainWindow.cpp:7775-7783now contradicts the code. It says re-running the minimum width "cannot currently matter" because "hiding one of its children leavesminimumSizeHint()unchanged". This PR now hides the whole stack, which does change it.tests/spectrum_sequence_gap_test.cpp:25-26still describes the deleteddropsUpdatedlambda (earlier review, still open).- The
alivecomparison and the first-snapshot-vs-zero rule have no test. This is the existing thread onAnanBackend.cpp:1436, along with the cache reset that sits only ondisconnectRadio(). - Foreign UDP holding
alivetrue (P2Client.cpp:459-460,onReadyRead()passessenderPortonly and never comparessenderAddress()tom_host). I confirmed this is a code fact. It stays a maintainer question, as in the existing thread.
What I tried to break
- The PA-temperature premise, against p2app's
OutHighPriority.c(quoted above). It held, and so did byte 49 =GetAnalogueIn(5)= AIN6. - The tests, by mutation. Reading the supply word at
+51instead of+49→FAIL: bytes 49-50 are the supply rail in raw counts, big-endian. Forcingc.paTelemetryAudit = std::nullopt→FAIL: ANAN declares a PA telemetry auditplus its sibling assertion. Both are real pins. I restored the tree and confirmed it clean (the probe build reportsdirty:false, sha840cf4ff6). - Other families unchanged. I ran the head build against the demo (
AetherSDR Demo,DEMO-0001, offscreen, isolated settings store,AETHER_AUTOMATION_NO_TX=1). The PA row is present and showsPA --°Fexactly as before, so a record left at itsnulloptdefault doesn't touch the UI. - Build + run:
anan_backend_testandanan_p2_protocol_testpass (2/2). The socket target was deliberately not built. - CI freshness: runs for
840cf4ffwere created2026-10-03T23:05:31Z, andmainis at2026-10-04T18:26:40Z. The green checks describe a merge about 19 h old, and none of the three touched test targets is on the per-PRctest -Rgate.
Could not verify: the 627/627 claim, the seven mutations against the socket test (I didn't run it), and anything on a live ANAN.
Recommendation
Request changes for Blocker 1, which needs one moved line plus test updates, and has in-tree precedent for the exact split. The rest is what the earlier review said, plus the protocol premise, now verified. The open threads and the two maintainer decisions (the unconsumed supply decode, and the new capability record) still stand.
|
Maintainer rulings on #6137 (following the review above)
|
…iple VIII.
The status bar's "Network:" field was blank on an ANAN, and its PA
temperature row read "PA --°C" for the whole session. These are the same
mistake pointing in two directions: one readout had no source wired, the
other has no source that can exist.
NETWORK
AnanBackend never emitted linkStatsUpdated -- the dropsUpdated handler was
an empty stub with a comment deferring it -- so
RadioModel::usesBackendLinkStats() was always false and everything behind
the label fell back to a stream source this family does not have.
P2Client now counts received bytes and packets, sent bytes, and its bound
local endpoint, and publishes them on a fixed one-second cadence.
AnanBackend caches each snapshot and emits LinkStats built from it.
Three deliberate choices:
* Receive traffic is counted BEFORE any parse decides what a datagram is.
The readout describes the TRANSPORT, so a session whose IQ has stalled
while status packets still arrive must not read as a dead link --
and counting after the shape checks would make exactly those datagrams
invisible. A mutation that moves the counting after the parse is in the
test evidence below.
* The counters are I/O-thread-only and reach the backend as a queued
snapshot, never a cross-thread read. That is what the LinkStats
contract asks for: a backend with worker-side values caches them on
its own thread.
* rttMs, jitterMs, gapMs and gapMaxMs stay at -1, which the struct
defines as "not measured". This transport is stream-only -- the radio
is told where to send, then sends -- so there is no request/response
exchange to time. Reporting 0 would render as a flawless "< 1 ms" link
that was never measured. RadioModel already ignores negative values and
leaves m_backendLinkShape.hasRtt disengaged, so the UI omits those
fields rather than inventing them.
PA TEMPERATURE
Protocol 2's entire analog payload is six fields (OutHighPriority.c:145-156)
and none of them is a temperature. No client can ever populate that readout
for this radio, so the honest change is to withdraw it rather than leave a
placeholder that cannot resolve.
A new RadioCapabilities::paTelemetryAudit record carries that claim. It is
a statement about the WIRE FORMAT -- "the protocol defines no such field" --
as distinct from the existing hasPaTemperatureTelemetry, which says only
that nothing has been received. The status bar withdraws the row when the
record is engaged and no PA-current reading is available to occupy the same
row instead; the whole stack comes down when neither of its rows survives,
because reserveTelemetryStack() pins the container's width and hiding only
the children would leave a reserved empty gap between two separators.
The record defaults to std::nullopt, meaning NOT AUDITED, so every other
family keeps its present behaviour exactly and only this backend declares
it. That default is the point: it does not silently reclassify an existing
`false` as "proven absent".
A record and not another bool because the RadioCapabilities boolean count
is frozen and shrink-only (aethersdr#5262 M2) -- a new bool fails
check_capability_records.py outright -- and because the audit can grow a
current or supply field later without a second ratchet argument.
SUPPLY VOLTAGE: DECODED, NOT PUBLISHED
The radio does send a supply rail (AIN6, high-priority status bytes 49-50).
This decodes it and pins the decode with a test, so the wire format is
recorded rather than rediscovered. Nothing displays a voltage.
Raw counts become volts only against the RF board's ADC reference, and
nothing the radio reports identifies it: board type is a p2app launch
option rather than a fact about the hardware, which is why
DiscoveryReply::isSaturn() already carries an explicit warning that it must
not gate backend behaviour. The candidate references differ by a factor of
1.5, so choosing one would show roughly 21 V for a 13.8 V rail on the wrong
board. A confidently wrong voltage is worse than no readout, so the
reference has to come from the operator or from a load-bearing field before
a reading can be offered.
EVIDENCE
627/627 ctest, the fifteen checks in the Static checks job run locally, and
seven mutations -- every new claim reverted in turn, each killing its test:
the supply decode read one byte late; receive traffic counted after the
parse instead of before; sent bytes not accumulated; start() not resetting
the session; the local endpoint never reported; the audit never declared;
and LinkStats claiming a measured transport while idle.
Confirmed on the radio: against a live ANAN-G2, the status bar's "Network:"
field reports a quality verdict instead of staying blank, and the PA row is
withdrawn rather than sitting on its placeholder. ctest cannot reach either
of those -- one needs a real transport to measure and the other needs the
status bar built against a connected backend.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…'s datagrams. Principle VIII. Answers the 10-04 review of aethersdr#6137 and the maintainer's rulings on it. rxPackets counted every datagram, while rxPacketsLost counts DDC sequence gaps. p2app streams Mic Data at about 750 packets a second against about 202 DDC frames at 48 ksps, so RadioModel's loss percentage read about 4.7x low. P2Client now counts a packet only once a DDC frame's index resolves. Bytes are still counted before any parse, so LinkStats::alive still sees Status traffic. onReadyRead() hands each datagram to handleDatagramFrom(), which drops anything not from the session host, as ruled, and logs the first foreign sender once per session. handleDatagram()'s signature is unchanged. AnanBackend::onLinkCounters() treats a backward jump in rxBytes as a new session, so a restart that skipped disconnectRadio(), or a dead session's snapshot delivered after its reset, is no longer compared against the old total. The status bar hides the separator after the PA stack along with it, and re-reads its minimum width after the hide. The comment saying that re-read could not matter is replaced. Tests: the link test's transport asserts move to rxBytes. Its stray-port frame had bitsPerSample 0, so the parser rejected it before the port check; it is now a real DDC frame, and a DDC0 frame pins the packet count. A new block pins the address filter, and exit 77 applies only when nothing has failed. anan_backend_test gains a friend that drives onLinkCounters() for alive, the first snapshot and the backward jump. The tests.cmake block names the socket it binds. Comments: the supply-rail rationale no longer blames only the 5 V / 3.3 V reference. A G2 bench read a 13.8 V rail as about 40 V through the 5 V scale, which no board choice explains. The stale spectrum_sequence_gap_test paragraph and the link test's speaker-stream claim are corrected. On 3342761: build clean; ctest 655 tests, 652 passed, 1 skipped, 2 failed (pgxl_panel_test, a known local failure, and rtl_capture_worker_test, which passes 5 of 5 alone). Every Static checks step passes with --strict. Fourteen mutations, all killed. On the G2: Network: reports and updates, one separator, and no datagram from the radio was ignored. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
840cf4f to
b93217a
Compare
|
Pushed Blocker:
Ruling: foreign UDP.
The four inline threads. Each one is answered here, and I'll resolve them.
Nit. Also corrected, though neither review raised them.
Side effect, disclosed. Evidence on
|
jensenpat
left a comment
There was a problem hiding this comment.
Verdict
Approve with nits. Reviewed b93217a1ee8cd1e6927b49ca0cc249b225a3cbdd. No linked issue; judged against the PR body. The change fits that intent: ANAN publishes LinkStats so the status-bar Network: field has a source, and withdraws the PA-temperature row because Protocol 2 has no temperature field.
The 2026-10-04 change request is fixed on this commit. rxPackets increments only after a datagram resolves to a session DDC (P2Client.cpp:542), so Mic Data and High Priority Status stay in rxBytes (what alive reads) and out of the loss denominator.
No blockers. Required checks on this SHA are green (build, check-macos, check-windows, Static checks, Sanitizer option configures). A merge of this head onto current origin/main (25de97c3) is textually clean; the PA-stack gate still sits after the capability writes and hides m_paSeparator with the stack.
Scope
| Group | What changed | Other families |
|---|---|---|
P2Client |
Byte/packet/sent counters, 1 s snapshot, host filter before parse | ANAN I/O thread only |
AnanBackend |
Cached linkStats(), restart self-heal, paTelemetryAudit |
Only this backend emits the new stats and engages the record |
P2Protocol |
Decode high-priority bytes 49–50 into an unused supplyVoltageRaw |
Not published; hasSupplyVoltageTelemetry stays false |
RadioCapabilities + map |
Optional PaTelemetryAudit, default nullopt |
Flex, HL2, Icom, Sim, RTL keep today's PA row |
MainWindow |
Withdraw the temp row and the following separator only when the audit says the field cannot exist and no current reading occupies the row | A missing record does not hide anything |
| Tests | anan_backend_test extended; anan_p2_protocol_test extended; new anan_link_telemetry_test; comment in spectrum_sequence_gap_test |
No CHANGELOG entry |
RadioModel's loss window and quality tiers are shared and unchanged. ANAN reaches them only once LinkStats.reported is true, which this backend now sets. Flex stays on the VITA-49 branch while m_panStream exists.
Nits (non-blocking)
P2Client.cpp:547still does++m_dropsonce per sequence mismatch, including a rewind.MetisClientadds the forward gap and ignores a backward one.RadioModel::packetLossPercent()treatsrxPacketsLostas missing packets, so a multi-packet hole counts as one error. Single-packet loss matches. Worth aligning with the Metis gap, not required to land the wiring.P2Protocol.h:265andAnanBackend.cppstill describe bytes 49–50 as the supply rail that no scale can fit. The PR body says a later G2 bench put the rail on bytes 57–58 and that#6211replaces this decode. Nothing in this PR displays the value.- A snapshot already queued when
disconnectRadio()resets the cache can still install the old totals for one delivery. The next smaller total self-heals.anan_backend_testpins that backward jump.
Verification
- Read the full diff at
b93217a1. Merge-tree onto25de97c3producedf4c107c4with no conflicts; the PA gate andtests.cmakeregistration survived. anan_backend_testandanan_p2_protocol_testpassed headless (2/2) on this SHA. Removing the backward-jump self-heal madeanan_backend_testfail on that assertion; restoring it passed again.- Did not build or run
anan_link_telemetry_test. It callsP2Client::start(), which binds oneQUdpSocketonAnyIPv4at an ephemeral port and sends the startup sequence to127.0.0.1. No listener and no fake peer.tests.cmakenames that bind and setsSKIP_RETURN_CODE 77. The DDC-only packet count and the address filter are reviewed from source; those assertions live in that target. - CI on this SHA compiled the tree and ran Static checks. The per-PR
ctest -Rsteps do not include these ANAN targets, so the green jobs are not a run of them. The body's 655-test figure is reported against base33427611, not this head.
Follow-ups (not merge prerequisites)
- Count a forward DDC sequence delta into
m_drops(and skip rewinds), then pin a 5-packet hole as five errors in the loss window. Seam:P2Client::handleDatagram→LinkStats.rxPacketsLost→RadioModel::recordNetworkHealthSample. ANAN only. - When
#6211lands, drop the AIN6-as-rail comments with the decode they describe.
Dismissing the 2026-10-04 change request. It asked for rxPackets to count DDC frames only. That landed on b93217a (increment after the DDC index resolves) and is covered by the approving review on that SHA.
…rries it. Principle VIII. The status bar and the Meter applet now show an ANAN-G2's DC supply voltage, published as RAD:+13.8A from every High Priority status packet. The rail is on bytes 57-58 (AIN3, p2app's "user_analog1"), not on bytes 49-50, which the spec calls "Supply Volts". Measured on a G2 with a meter at the DC input jack reading 13.65 V: AIN3's median was 536 counts, which the 23.0/1.1 divider at a 5 V reference reads as 13.68 V -- the figure predicted before the run. AIN6 read 1589 counts on the same bench, which fits no published scale. That same scale applied to AIN6 is what read 40.3 V when this readout was first tried, and why it was withdrawn. One rail voltage only: the supply could not be varied, so the point fixes the scale where it is used and does not separate gain from offset. - P2Protocol decodes the rail from bytes 57-58 (supplyRailCounts) and drops the AIN6 decode, which had no consumer and no scale. - P2Client emits each status packet's count. - AnanBackend claims hasSupplyVoltageTelemetry, defines RAD:+13.8A in volts, and publishes the mean of every count since the last publication once a second -- inside MeterModel's 1.5 s freshness window. One packet alone wanders by about +/-11 counts (+/-0.3 V). - The comments aethersdr#6137 added that called the user analogs unassigned and the rail unscalable are corrected. Verification: build clean; ctest 655 tests, 653 passed, 1 skipped (app_settings_safety's environment skip), 1 failed (pgxl_panel_test, a known local-environment failure: its sources are main's and CI passes it); 12/12 mutations each killed by the assert written for it; on a G2, the status bar agrees with the meter at the DC input and holds within +/-10 mV, the Meter applet shows the same value, and the row clears on disconnect and returns on reconnect. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…rries it. Principle VIII (#6211) ## Summary An ANAN-G2 now shows its DC supply voltage in the status bar and the Meter applet, like a Flex or a profiled Icom. **Rebased onto main** after #6137 merged (`42b9e18d`). It replaces #6137's AIN6 decode and relies on #6137's status-bar gate: with the PA temperature row withdrawn, this is the row that remains. No other family changes. Only `AnanBackend` changes what it declares, and the meter it publishes, `RAD:+13.8A`, is the one Flex and Icom already feed. ## Why the first attempt read 40 V #6137 decoded bytes 49-50 (AIN6), which the spec calls "Supply Volts". It left the readout off because the published scale read 40.3 V on a 13.8 V rail. That scale was right. **The input was wrong.** For the G2's volts, deskHPSDR and Thetis both read **bytes 57-58** (AIN3, p2app's `user_analog1`) with the same `23.0/1.1` divider at a 5 V reference. I took that as a prediction to test, not as code: at 13.8 V, AIN3 should read about 541 counts. **The bench result, on a G2:** - **Setup:** a receive-only probe outside the tree. It sent run=1, recorded every analog field of 26 status packets, then sent run=0. A meter on the DC input jack read **13.65 V**. - **AIN3** (bytes 57-58): median **536** counts, range 521-543. That is **13.68 V**, 0.2% from the meter. - **AIN6** (bytes 49-50): **1589** counts, 8.59 mV/count against the meter, which fits no published scale. 1589 counts through the scale above is the 40 V reading. - **AIN4** (bytes 55-56): 0-16 counts while receiving. It is not decoded, and nothing here measured what it carries. **One rail voltage only.** The supply could not be varied. The point fixes the scale where it is used, and the prediction matching to 0.2% makes a large offset unlikely, but one point does not separate gain from offset. ## What changes - **`P2Protocol`** decodes the rail from bytes 57-58 into `HighPriorityStatus::supplyRailCounts`. The AIN6 decode had no consumer and no scale, so it is removed. `kSupplyRailVoltsPerCount` carries the scale; its comment cites the two reference clients, and the bench reading is in the capabilities-map row and above. - **`P2Client`** emits each status packet's count as `supplyRailSampled`. p2app sends one packet every 200 ms while receiving. - **`AnanBackend`**: - It claims `hasSupplyVoltageTelemetry` and defines `RAD:+13.8A` in volts, with the unit and range Icom uses. - It publishes the **mean** of every count since the last publication, once a second. A single packet wanders by about ±11 counts (±0.3 V), and one publication a second stays inside `MeterModel`'s 1.5 s freshness window. - The window is fixed: each one opens where the last closed, so the cadence does not drift by a packet each time. - The window is emptied on disconnect and again at each new session's `linkUp`, so a session never averages in the previous one's counts. Both are needed: a status packet can arrive before `linkUp`. - A count outside 0-4095, the 12-bit ADC's range, is dropped. A misidentified input then shows no reading rather than a plausible wrong one. - **#6137's comments** that called the user analogs "unassigned" and the rail unscalable are corrected. #6137's body section "Supply voltage: decoded, deliberately not published" is superseded by this PR. **For the TX phase, not this PR:** under MOX p2app sends status every 1 ms (`OutHighPriority.c`), so once ANAN transmits, `supplyRailSampled` will cross threads at about 1 kHz. The mean window handles that; the per-packet queued signal is the part to revisit then. ## Blast radius | Family | `hasSupplyVoltageTelemetry` | Volts row | |---|---|---| | Flex | true (unchanged) | unchanged | | Icom | per profile (unchanged) | unchanged | | HL2, Sim, RTL-SDR | false (unchanged) | unchanged | | ANAN | false → **true** | withdrawn → **shown** | ## Evidence - **CI** builds the head and runs the static checks. It runs none of the ANAN tests: `ci.yml`'s per-PR test gate is frozen, and these tests run in the full suite on `main` after merge (`full-suite.yml`). - **This head** (`f7396082`, on `42b9e18d`), locally on Linux with Qt 6.12.0: the 17 tests that use `AnanBackend`, `P2Client` or `P2Protocol` rebuilt and passed 17/17, `anan_link_telemetry_test` included. No warnings in the changed files. Static checks: all 17 steps pass with `--strict`. - **Before the rebase,** on `b93217a1` (#6137's head, on `33427611`): 655 tests, 653 passed, 1 skipped, 1 failed. - The skip is `app_settings_safety`'s environment skip. - The failure is `pgxl_panel_test`, a known local-environment failure: its sources are main's, and CI passes it. - No `src/gui` file changes, so the colour ratchet and a11y steps do not apply. - **Sixteen mutations,** each confirmed to fail the assert written for it: - the rail decoded from AIN6 - the rail read one byte late - the scale at a 3.3 V reference - `P2Client` not emitting the count - the backend not connected to `supplyRailSampled` - the capability not claimed - the meter defined under another name - the meter defined in another unit - the meter published under another id - the last packet published instead of the mean - the next window opening at its first packet - the sum carried into the next window - a 500 ms window - no reset at `linkUp` - no reset on disconnect - no 0-4095 bound - **No socket needed:** the test emits `P2Client`'s own `supplyRailSampled` and `linkUp` on the test thread, so the backend's slots run directly. That covers the connection and the per-session reset, which earlier versions of this body listed as unreachable. - **On the radio,** against a live ANAN-G2: - The status bar agrees with the meter at the DC input and holds within ±10 mV. - The Meter applet shows the same value. - The row clears to "—" on disconnect and returns on reconnect. ## Relation to #6106 The scale is a G2 hardware fact. When #6106's stage 1 extracts the G2 profile, `kSupplyRailVoltsPerCount` and the choice of input belong in its "Drive and telemetry" record. Until then it is one constant, used in one place. It is measured on a G2 only, which is the one radio this backend names. ## Review guide - **`src/core/backends/anan/P2Protocol.{h,cpp}`:** the decode position and the scale. - **`src/core/backends/anan/P2Client.{h,cpp}`:** one signal, emitted from the status branch. - **`src/core/backends/anan/AnanBackend.{h,cpp}`:** the capability, the meter definition, the averaging window, its bound and its resets. - **`tests/`:** - `anan_p2_protocol_test`: decode, byte order, and the bench point. The test packet sets AIN6 to 1589, so decoding the wrong input fails. - `anan_link_telemetry_test`: the client hands the count on. - `anan_backend_test`: the meter, the mean and the cadence, against an injected clock; the signal wiring, both resets and the bound, through `P2Client`'s own signals. - **`docs/architecture/radio-capabilities-map.md`:** the ANAN note on the row. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
… and re-take the Stable snapshot. Principle VIII. (#6396) ## Summary This brings the user guide (docs.aethersdr.com) up to date with everything in v26.10.2 and re-takes the Stable snapshot so the default site shows it. Docs only, apart from the TCI page prose in `tools/docs/gen_reference.py`; no application source, test or CI changes. Every page was audited against the 106 PRs in the release. Every fact that was added or kept was checked against the source at `f753dbfc4`, the v26.10.2 release commit, not taken from the PR bodies alone. ## What changed - **Menu paths.** - AetherModem, Callsign Lookup, AetherMap and FreeDV Reporter are on **Tools**. They are created on View and then moved to Tools (`MainWindow_Menus.cpp:1519-1520`, `:1569-1575`); the style guide's example says **Tools → AetherMap...**. - **Show live CW contacts** is on **View**. - Waveforms is on **Tools**, not File. - The VUDU applet button was called TXDSP. - Labels now match the source's `...` vs `…`. - **Gaps from this cycle.** - **Windows and Network Diagnostics:** Windows Firewall rules and the Network Diagnostics Windows Firewall page (#6289); the `aether.network` and `aether.nvafx` log categories (#6289, #6308). - **Remote access:** Tailscale shim revocation semantics (#6325). - **Sleep, App Nap and power throttling (#6195):** per-OS notes. - **Hermes-Lite 2:** state restore, RX audio at unity, CW at TUNE level, multi-interface discovery and a discovery troubleshooting entry (#6259, #6254, #5975, #6009, #6293). - **ANAN-G2:** AGC-T range, a "What is remembered" section, and supply voltage and link status (#6282, #6129, #6137, #6211). - **NB2** on the HL2 and ANAN-G2 (#5966). - **DVK:** WAV import requirements, the slot menu without Delete, status text and licensing (#6247, #6262, #6251). - **Copy Assist** from unprocessed audio (#6216). - **Live CW contacts** replace the decoder's contact card (#6268). - **Status-bar indicators** work from the keyboard (#6267). - **Volume:** controllers, hotkeys and TCI `volume` follow the active output (#6129, #6260). - **Peripherals:** the KPA1500 (#5905), CTR2 link extensions and the AetherKnob (#6338), TMate 2 udev (#6250), MIDI RADE Modem and FDV modes (#6031, #6038). - **RTL-SDR:** live PPM (#6082). - **Icom:** COMP and the MK2 squelch scale (#6174, #6184). - **Known issues.** Entries whose GitHub issues are closed are removed (each checked with `gh issue view`). As decided, this includes three closed today without a demonstrated fix: #5107, #4824 and #5665. - **US spelling.** `docs-style-guide.md` asked for British spelling; it now asks for US, by the maintainer's decision, and every page is converted. The generated TCI pages are converted at their source, the prose strings in `tools/docs/gen_reference.py`. Code spans, links and URLs were left alone. UI labels were checked against the source strings, and none of them are British. Two in-page anchors and two Log Analyzer rule anchors (`#bnr-is-grayed-out-…`) follow the renamed headings. - **Stable snapshot.** `snapshot_stable.py 26.10.2` was re-run on the release tree with these corrections, before `main` was merged in, so Stable carries no post-release docs. `docs/` also has `main`'s post-release pages, in US spelling. Stable alone keeps the IC-9700 waterfall-rate entry (#5996): #6402 fixed it after the 26.10.2 tag, so 26.10.2 still has it. The PDF manual already attached to the v26.10.2 release keeps the tag's docs. ## Validation - `npm ci && npm run build` in `docs/user`: success. This is the broken-link and broken-anchor check. - `node tools/docs/test_log_rules.mjs`: 707 checks passed for 45 rules. - `python3 tools/docs/gen_reference.py --check`: up to date (8 pages). - `git diff --check`: clean. - Menu paths on a build of this branch (offscreen, isolated settings, no radio), `menu list`: `Tools → AetherModem...`, `Tools → Callsign Lookup...`, `Tools → AetherMap...`, `Tools → FreeDV Reporter...`, `Tools → Waveforms...`, `View → Show live CW contacts`. ## Screenshots now out of date (not recaptured) These need a recapture pass on the radio: - **Old panadapter title strip (#6386):** `panadapter`, `main-window`, `first-session-*`, `multi-slice-two-slices`, `split-pair`, `tnf-markers`, `offscreen-vfo-indicator`, `pan-3d-stacked-trace`, `default-light-theme`. - **Pre-canon windows (#6334, #6339, #6341, #6372):** `aetherrx*`, `aethertx*`, `aethermodem-aprs`, `bnr-panel`, `connect-dialog`, `connect-by-ip-card`, `connect-icom-rows`, `connect-smartlink-card`, `rx-agc-calibration`. - **Other:** - `title-bar` (lowercase tab states, #6281); - `menu-help` (#6294); - `display-panel` (#6371); - `support-diagnostics-dialog`; - `waveforms-dialog` and `tailnet-remote-access` (show shim 0.4.0); - `about-window` (v26.10.1); - `radio-setup-qrz`. ## Found during the audit, not fixed here - **App string bug:** `TailnetShimDialog.cpp:628` tells the user to "Connect remotely with Connect → Manual"; the real path is **File → Connect to Radio... → Connect by IP**. - **ANAN AGC-T tooltips:** the RX applet and VFO tooltips show the slider position as dB, which is wrong on ANAN. #6282 lists this as a follow-up. - **MIDI Master Volume on non-Flex radios:** it is refused while PC Audio is off, whereas hotkeys, FlexControl and HID drive the line out. The docs describe the code as it is. - **RTL mode names:** `rtl-sdr.md` writes "FM-N" and "CW-R", but the source strings are `FMN` and `CWR`. Left as is until someone confirms what the mode menu displays. - **IC-705 COMP readback:** inferred from the MK2 (#6174) and not verified on an IC-705. - **Not yet documented:** the #6230 Radio Health row "Forward responses skipped", and the Store/MSIX package. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Summary
Two status-bar readouts were wrong on an ANAN, in opposite directions:
Network:field was blank because nothing fed it.PA --°Cfor the whole session because nothing can feed it.This PR fixes the first and withdraws the second.
No HL2, Flex, Icom, Sim or RTL behaviour changes. The one new capability is a record that defaults to "not declared", so every other family is untouched by construction. The blast-radius table below shows each family.
Network:— nothing was wiredAnanBackendnever emittedlinkStatsUpdated. ThedropsUpdatedhandler was an empty lambda whose comment deferred the work to "a future commit". As a resultRadioModel::usesBackendLinkStats()was permanently false, and every field behind the label fell back to a stream source this family does not have.P2Clientnow counts received bytes and packets, sent bytes and its bound local endpoint, and publishes them on a fixed one-second cadence.AnanBackend::onLinkCounters()caches each snapshot and emitsLinkStatsbuilt from it.Four choices a reviewer should weigh deliberately:
Bytes and packets measure different things.
LinkStats::alivereads bytes.RadioModel's loss percentage, whose numerator is DDC sequence gaps.Only the radio's own datagrams count.
onReadyRead()hands each datagram tohandleDatagramFrom(), which drops anything not from the session's host before the counters or any parser see it. Without that, stray LAN traffic reaching the ephemeral port could holdalivetrue after the radio has gone.The counters never cross a thread unguarded.
LinkStatscontract asks for: "a backend with worker-side values caches them on this thread".linkStats()getter reads only the backend-thread cache.disconnectRadio().rttMs,jitterMs,gapMsandgapMaxMsstay at-1.0would render as a flawless "< 1 ms" link the client never measured.RadioModelalready ignores negatives, so the UI omits those fields rather than inventing them.Side effect:
RadioModel::firstUdpPacketSeen()readsrxPackets > 0. Network Diagnostics' "First UDP Packet" therefore turns Yes on the first DDC frame, not on the first datagram of any kind.PA temperature — nothing can be wired
Protocol 2's entire analog payload is six fields (
OutHighPriority.c:145-156): forward, reverse and exciter power, the input p2app calls "supply voltage", and two user analogs. There is no temperature field anywhere in it, so no client can ever populate that readout for this radio. A placeholder that can never resolve is what needs fixing.The new
RadioCapabilities::paTelemetryAuditrecord carries the claim.hasPaTemperatureTelemetry, which says only that nothing has been received.reserveTelemetryStack()pins the container's width, so hiding only the children would leave a reserved, empty gap.Why a record and not a bool:
RadioCapabilitiesis at its frozen boolean count and shrink-only (#5262 M2). A new bool failscheck_capability_records.pyoutright, and raising the freeze needs a maintainer ruling rather than a quiet edit. The record is the sanctioned shape, and it can grow a current or supply field later without a second ratchet argument.Blast radius. The record defaults to
std::nullopt, meaning not audited, and only this backend declares it:paTelemetryAuditPA --°CforeverThat default is the point: it does not silently reclassify an existing
falseas "proven absent". The prior comment inapplyCapabilitiesToUi()argued against hiding the stack, because that would delete the HL2's genuine reading to suppress a broken sibling. That reasoning holds, and it is why this is a per-radio gate on declared evidence rather than a change to the stack. The comment is rewritten in place rather than left describing a mechanism that no longer matches the code.Supply voltage: decoded, not published here
This PR decodes high-priority status bytes 49-50 (AIN6), which the spec calls "Supply Volts", and pins the decode with a test. Nothing in this PR displays a voltage.
On a G2, that input is not the rail. A later bench run found the rail on bytes 57-58 (AIN3). The
23.0/1.1scale at a 5 V reference, which read about 40 V on AIN6, reads 13.68 V on AIN3 against a meter's 13.65 V at the DC input.#6211, stacked on this PR, replaces the AIN6 decode with AIN3 and publishes the voltage. It also corrects this PR's two comments that call the rail unscalable.
Evidence
33427611. The skip is an environment skip inapp_settings_safety. Neither failure shares code with this PR:pgxl_panel_testis a known local-environment failure: its sources are main's, and main's Full Suite passes it.rtl_capture_worker_testpasses 5 of 5 when run alone. It fails only under the full serial run, on its 3 s waits.--strictwhere the step takes it. That includesgen_touchpoint_manifest --check(no new touchpoint) andcheck_capability_records --strict. The colour ratchet is +0 and the a11y count is unchanged.start()keepingrxBytesstart()keepingrxPacketsLinkStatsclaiming a measured transport while idlealivereported on an unchanged totalonReadyRead()calling the filter at all. Proving it needs a second socket sending to the client, a live fake peer the canon rules out. The G2 run below covers the radio's side of it.anan_link_telemetry_test's last block callsP2Client::start(). That binds one UDP socket on AnyIPv4 at an ephemeral port and sends the startup sequence to127.0.0.1. There is no listener, no fake peer and no radio. It skips with exit 77 when it cannot bind, unless an earlier check has already failed. Itstests.cmakeblock names the bind.Network:reports a quality verdict and keeps updating.Review guide
src/core/backends/anan/P2Client.{h,cpp}: the counters, the snapshot timer,handleDatagramFrom()'s address filter, and the I/O-thread ownership note.sendTo()gained a required accumulator parameter, so a new send site cannot compile without deciding where its bytes are counted. That is why nine call sites are touched.src/core/backends/anan/AnanBackend.{h,cpp}:linkStats(),onLinkCounters()with its restart self-heal, and the two capability declarations.src/gui/MainWindow.{h,cpp}: the gate, the PA separator, and the rewritten rationale comment.src/core/backends/RadioCapabilities.h+docs/architecture/radio-capabilities-map.md: the record and its row. The map is not CI-enforced; the row is there because the map is where this is documented.🤖 Generated with Claude Code