Repository navigation
feat(anan): subnet discovery and a front-end setup page - #6146
Conversation
There was a problem hiding this comment.
Reviewed at head c258baa0, against the head checkout. Findings below are reasoned from source, not reproduced at runtime — no build, no test run, no GUI here.
Issue fit
No linked issue. The body cites "a G2 field report" with no issue or RFC number, so there is no thread to build a requirements list from and nothing redefining the ask. Reviewed against the PR's own stated intent instead.
Against that intent the discovery half lands cleanly and is the strongest part of the PR: the root cause (a datagram to 255.255.255.255 leaves by the default route only) is correct, the fix is additive by construction, and the bench result — a G2 that previously needed a typed IP now appearing in the picker — is exactly the evidence GOVERNANCE's "bug fixes with a clear root cause" exemption contemplates. No RFC needed for that item.
The other three items are a different matter; see Scope and the maintainer-decision section.
Scope
The title claims two things. The diff does four. The body discloses all four honestly and at length — the objection is bundling and title coverage, not concealment.
| File / group | What it changes | Claimed by title/body? | Verdict |
|---|---|---|---|
anan/AnanDiscovery.{h,cpp} |
per-interface directed broadcast + broadcastTargets() |
title | In scope — bug fix, bench-confirmed |
gui/ConnectionPanel.{h,cpp}, gui/RadioSetupDialog.{h,cpp} |
seven ANAN rows removed from the connect form, new "ANAN Front End" page | title | Changes existing UX; no issue or RFC cites the old placement as a defect — §6 ruling (D3) |
anan/P2Protocol.{h,cpp} (AlexRxFilter, alexRxFilterBits, alexRxFilterFor, HighPriorityOptions), P2Client live setter, AnanSettings::bandFilters*, the page combo |
new wire-level hardware control — energises relay banks from a band-edge ladder 4 of whose 7 selections are unverified | body only, not the title | New hardware surface, bundled — ruling (D1) |
P2Protocol byte 1400, P2Client::setSpeakerAmpMuted, AnanSettings::speakerAmpMuted, page checkbox |
new wire control — speaker-amp mute | body only, not the title | New wire surface — ruling (D1) |
AnanBackend.cpp |
anan/frontend.set extension verb, adcFrontEnd declaration |
body | New extension verb — ruling (D2) |
RadioCapabilities.h + docs/architecture/radio-capabilities-map.md |
AdcFrontEnd record + map row |
body | In scope; correct shape per #5262 M2 (record, not five bools) |
models/RadioModel.cpp |
two new anan.* connect params |
body | In scope — inserted inside the pre-existing family == "anan" branch, no new family-string branch |
tests/*, tests/tests.cmake |
2 new targets, 3 extended | body | In scope. No sockets — see below |
AnanBackend.{h,cpp}, P2Client.{h,cpp}, MainWindow.{h,cpp}, anan_link_telemetry_test.cpp |
link telemetry + PA-row withdrawal | declared as #6137's commit | Not reviewed here — belongs to #6137 |
CHANGELOG.md correctly untouched.
Socket tests: none. Both new targets (anan_front_end_test, anan_discovery_test) are registered in tests/tests.cmake with add_test, and I read both sources — no QUdpSocket/QTcpSocket/bind/listen/connectToHost/Fake* peer, and no fake radio standing in for firmware. anan_front_end_test reaches P2Client::highPriorityOptions() through a friend struct P2ClientTestAccess (P2Client.h:218) on a client that is never start()ed. Neither new target is on the frozen per-PR ctest -R gate — .github/workflows/ci.yml contains no anan match at all — so neither gated the green checks above.
Blockers
1. src/core/backends/anan/P2Client.cpp:456-468 — the body's independence claim is false, and the merge order is load-bearing.
The body states:
The dependency is only on the branch, not on the code: nothing in this commit references anything #6137 adds.
sendRunningHighPriority(), added by this commit, is:
void P2Client::sendRunningHighPriority()
{
sendTo(*m_socket,
buildHighPriority(true, sharedFreqWords(),
m_bypassAdc0Filters, m_bypassAdc1Filters,
m_adc0AttenuationDb, m_adc1AttenuationDb,
highPriorityOptions()),
m_host, kHighPriorityPort, m_txBytes);
}Both the fifth sendTo() parameter (txAccum, which #6137 made required precisely so no send site can omit it) and the member m_txBytes are added by #6137 — git log -S'm_txBytes' on this branch returns exactly two commits, #6137's and this one, and nothing earlier. So c258baa0 does not compile against main, and five of this commit's own call sites in start(), setDdc0FrequencyHz(), setStepAttenuationDb() and onKeepaliveTick() are rewrites of #6137's lines.
This matters because the sentence is the one a maintainer would rely on to deviate from the stated order — cherry-picking c258baa0, or rebasing this branch onto main expecting it to build. The remedy is a one-sentence body edit, not a code change: delete the claim and keep the "#6137 merges first" instruction, which is correct and already bolded.
Needs maintainer decision
D1. The Alex band-filter selector and the speaker-amp mute are new wire-level hardware controls in a PR titled "subnet discovery and a front-end setup page."
P2Protocol.cpp:142-178 adds a seven-way relay selection and the ladder that drives it; P2Client.cpp:405-454 makes it live on a 100 ms packet; byte 1400 bit 1 newly asserts a hardware mute. None of this is discovery and none of it is a setup page. The author's own disclosure is the reason to take it seriously rather than wave it through: 4 of the 7 selections are untested, the 22 MHz edge sends 17 m and 15 m to a filter proven only at 14 MHz, and the failure mode is silence, not degradation — the relays are in series ahead of the ADC.
The mitigations are real: it ships defaulting to Bypassed, which the body says is byte-for-byte the previous behaviour (consistent with P2Protocol.cpp:203-206, where opts.adc0Filter is nullopt and the else if (bypassAdc0Filters) branch reproduces the old bit), and the tooltip tells the operator to check band by band. So this is not a risk to anyone who leaves it alone.
Worth noting that you have the author's own agreement on the shape: #6129's body (same author, open) lists "the connect-time options moving into Radio Setup" and "the filter-board selector" as separate changes against this backend — the split I am asking for here is the one that PR already announced. #6129 also edits the same populateFamilyParams() and makes a restored span win over anan.ddc0RateKsps, which would falsify this page's new Sample-rate tooltip ("the width of the panadapter span a session starts at"); whichever lands second needs to reconcile that wording.
My recommendation is to split the band-filter selector into its own PR so it gets a ruling on its own evidence rather than inheriting the discovery fix's. At minimum the title should name both new controls. Your call — the disclosure is complete enough that an informed ruling is possible as-is.
D2. New extension verb anan/frontend.set (AnanBackend.cpp:1361).
Consistent with the established per-family pattern (hl2 hw.set, icom, sim), partial-by-design, and reached from the GUI through backendDeclaresExtension() rather than a family string — which is the right shape and worth crediting. Flagging it because it is new surface on the backend seam.
One question against the AGENTS.md #5554 standing notice ("no new capability declared without a verb behind it"): adcFrontEnd has five fields, and only two — hasTunedBandFilters and hasSpeakerAmpMute — have frontend.set behind them. The other three (hasDitherAndRandom, hasAdcSelect, hasFilterBankBypass) are connect-time Params with no verb. I read that as satisfying the notice's intent (the capability does drive real action, just not all of it through a verb) rather than violating it, but it is your ruling, not mine.
D3. The connect form loses seven working controls (§6 preference check).
ConnectionPanel.cpp drops 187 lines of working UI; the controls reappear as a Radio Setup page. The move is well-argued — those rows only ever wrote AnanSettings, the connect path has always read AnanSettings, and the page is reachable with nothing connected so it can configure a first connect.
But it cannot point at an authority. There is no issue and no RFC calling the connect form's placement a defect, and GOVERNANCE.md's no-RFC list covers "New applets or dialogs that don't change existing UX" — this one does change existing UX. That puts it in §6 preference territory: correct code, plausible reasoning, unratified behaviour change. It needs your ruling rather than my approval. I am not calling it a blocker because it is disclosed in the title and the body, and because the two new live controls genuinely need a home that exists while disconnected.
D4. Security — the sweep now reaches every attached subnet, with no opt-out. (security-audit (CodeGuard), confirmed against source)
AnanDiscovery.cpp:52-62 accepts every interface that is merely IsUp|IsRunning; :184-188 sends the probe to each one's subnet broadcast every 5 s (AnanDiscovery.h:35, default intervalMs = 5000) for as long as discovery runs. The point-to-point tun case is handled (broadcast().isNull() at :81) and loopback is dropped by address, but tap-mode VPNs, Docker/libvirt bridges, hotspot and guest networks are not — those have real broadcast addresses.
The payload itself is harmless: buildDiscovery() is 60 zero bytes with pkt[4] = 0x02, no hostname, no MAC. What is newly disclosed to peers on those subnets is that this machine runs AetherSDR and the ephemeral source port of m_socket. On main the single QHostAddress::Broadcast reached only the default-route subnet.
This is deliberate — AnanDiscovery.h:45-50 names "a VPN up" as the motivating case — so it needs you to accept the trade rather than the author to fix a mistake. If you would rather not: restrict directed broadcast to interfaces whose subnet matches a configured radio address, or make the directed sweep opt-in with the unconditional 255.255.255.255 probe as the default.
D5. Security — onReadyRead() applies no sender filter, and this change widens who can reach it. (security-audit (CodeGuard), confirmed against source)
AnanDiscovery.cpp:207-253 accepts any datagram that parses and passes isSaturn(), then sets info.address = dg.senderAddress() (:220). Selecting that picker entry makes P2Client::start() dial the address. Protocol-2 discovery is unauthenticated by design, so the bound available is topological.
This code is pre-existing and not in the diff — I am raising it because D4 changes the attacker set from "a peer on the default-route subnet" to "a peer on any attached subnet, including a corporate VPN or a container bridge." The widening is what this PR does; the missing check is what makes it matter.
The available fix is a subnet containment test, and broadcastTargets() already enumerates exactly the entries needed (QNetworkAddressEntry::ip()/netmask() → QHostAddress::isInSubnet). Hl2TelemetryPoller.cpp:208-212 is the in-tree shape of a sender filter, though a weaker precedent than it first looks — it filters to a known target, which discovery by definition does not have. Routing this to its own issue rather than growing this PR seems right.
Nits
All non-blocking.
-
src/core/backends/anan/AnanBackend.cpp:1395— stray indentation. The droop branch sits at 8 spaces after the newfrontend.setblock closes, reading as if it were nested inside it:if (ns == QLatin1String("anan") && verb.startsWith(QLatin1String("droop."))) {
-
src/core/backends/hl2/Hl2Discovery.cpp:179-187— the sibling is left unfixed.Hl2Discovery::sweepNow()still sends only toQHostAddress::Broadcast, so a Hermes-Lite 2 on a second NIC or behind a bridge still never appears in the picker — the same defect, the same root cause, the same protocol family.broadcastTargets()is a static onAnanDiscovery, so HL2 cannot reuse it without reaching into the ANAN class. Since the flag test was already derived fromNetworkPathResolver::enumerateIpv4Candidates()(per the comment atAnanDiscovery.cpp:46-51), that looks like the natural home for a shared helper. Completeness note only — the PR does not claim to fix HL2. -
src/core/backends/anan/P2Protocol.cpp:165-177—alexRxFilterFor()has no finite guard. Every comparison against NaN is false, so a NaN tuning falls through the whole ladder toPreamp6m— the one selection whose consequence on HF is silence. A negative frequency is safe (< 1.5→Bypass). I could not find a path that delivers NaN (AnanBackend.cpp:755passesm_sliceFreqHz), so this is cheap insurance rather than a live defect. The file has no<cmath>today, so the fix is two lines:#include <cmath> // std::isfinite, alexRxFilterFor()'s NaN guard
const double mhz = tunedHz / 1e6; if (!std::isfinite(mhz)) return AlexRxFilter::Bypass; if (mhz < 1.5) return AlexRxFilter::Bypass; // below the bank's lowest filter
Separately, the final
returnsends everything above 35 MHz toPreamp6m, including the span between the 6 m band and the 61.44 MHz Nyquist this radio can reach. Inert at the shipped default, and consistent with the body's "untested" labelling. -
src/gui/RadioSetupDialog.cpp:4922-4950— the two bypass checkboxes now have exactly one usable value. When the bank follows the tuned frequency they are superseded and disabled; when it does not, unchecking one leaves that ADC with no signal path at all — which the accessible description says plainly, to its credit. A control whose only non-default setting breaks reception may be better removed than relocated. Raising it because this PR moved them into a new page rather than leaving them alone. -
src/core/backends/anan/AnanBackend.cpp:1361-1393—frontend.setdeliberately does not writeAnanSettings("the setting is what survives a restart and the verb is what reaches the radio now"). The GUI pairs the two correctly, but the verb is on the public extension surface; a future bridge or automation caller would get a change that silently reverts on the next connect. Worth one line in the verb's own comment telling callers they must persist separately.
What I tried to break
- The Alex word halves. The body's protocol finding — that 1432–1435 is really two 16-bit words, and ADC0's receive filter is the low half at 1434–1435 — checks out against the code.
writeU32be(&pkt[1432], alex0)puts ANT1 (bit 24) in byte 1432 and every filter bit (12, 6, 5, 4, 3, 2, 1) in bytes 1434–1435. The claim and the encoder agree, and a test reads 1434–1435 specifically. - Byte-for-byte compatibility at the default. With
bandFiltersEnabledfalse,highPriorityOptions()leaves both filtersnullopt, sobuildHighPriority()takes theelse if (bypassAdc0Filters)branch and emits the old bit. Byte 1400 stays zero. The "default is what this client always sent" claim holds. - A stale relay after a retune.
setDdc0FrequencyHz()setsm_ddc0FreqHzandm_ddc0FreqWordtogether and then sends, and all four run=1 senders funnel throughsendRunningHighPriority(), so no sender can disagree.start()resets both to 0 (P2Client.cpp:84,:92), so the "reset withm_ddc0FreqWordabove" comment is accurate and a reconnect cannot inherit the previous session's band.anan_front_end_testpins the stale case, the both-orders case, and the turn-it-back-off case, and it would fail ifhighPriorityOptions()returned a default — it is not a test asserting the implementation back to itself. - The reseed writing back what it just read.
m_ananFrontEndReseedblocks all nine controls withQSignalBlockerbefore seeding, which it has to: every control persists on change and two of them push live. Unblocked,showEventwould push a mute to the radio as if the operator had asked. Covered. - Nothing lost in the UI move.
populateFamilyParams()(RadioModel.cpp:669-701) reads all nine values fromAnanSettings, never from a widget, and a tree-wide grep for the removedconnectionManualAnan*object names across*.cpp/*.h/*.py/*.md/*.json/*.ymlreturns nothing outsideConnectionPanel.cppitself — so the body's "nothing was lost" holds, including for automation scripts and docs. The connect form keeps a pointer to the new location (ConnectionPanel.cpp:852). - Canon checks that came back clean. The capability is a record, not a bool, so the #5262 M2 freeze is untouched; the map doc has its row and
anan_backend_testpins all five fields. The new settings keys land inside the existing nestedAnandocument (Principle V) with no new flat key. The touchpoint manifest needs no edit —AnanSettings.hkeeps exactly one GUI includer as the include moves fromConnectionPanel.cpptoRadioSetupDialog.cpp(manifest row: 1). The page gates on!connected || capability, not a family string; theme tokens are the ones this file already uses 54 times; noCHANGELOG.mdentry. On the counters and the 1444-byte builders the security audit walked everyAnanBackend→m_clientcall site and every packet index and found no race and no out-of-bounds write; I spot-checked the bounds and agree, and those counters belong to #6137 regardless. - What I could not check. Anything at runtime: no build, no
ctest, no GUI, so the 629/629 and the 9/9 mutation claims are unverified here, as are the bench results and the band edges. CI is 5/5 green onc258baa0, but that merge base contains #6137's branch, so it does not show this commit building onmain— see Blocker 1 — and the two new test targets are not on the frozen per-PR gate.check_a11yI could not run; by reading, the new combos carry accessible names, the checkboxes rely on their label text as Qt intends, the disabled-state explanation reaches the accessible description rather than only the tooltip, and no interactiveQLabelis added.
Recommendation
Needs maintainer decision. The one blocker is in the PR body, not the code — a single false sentence, fixable in an edit — so what actually gates this change is your ruling, not a rework. The discovery fix is the best-evidenced part of this PR and I would take it as-is. What needs your ruling is everything riding with it: a new relay-bank control with four unverified selections and a silence failure mode (D1), a relocation of seven working controls with no issue or RFC behind it (D3), and a discovery sweep that now reaches corporate VPN and container subnets by design (D4/D5).
Concrete next step: fix the one false sentence in the body (Blocker 1) and keep the stated merge order behind #6137; then rule on whether the band-filter selector splits out. If it does, what remains — discovery, the page, the mute, the capability record — is a clean, unusually well-documented change.
Thank you for the disclosure discipline here. The untested-selections table, the "what is not verified" section, and the removed supply-rail readout with its 40.3 V-on-13.8 V measurement are exactly the kind of thing that makes a PR reviewable, and naming the four untested filters cost you a harder review than hiding them would have.
🤖 aethersdr-agent · cost: $23.6457 · model: claude-opus-5
|
Antares Detector — candidate vulnerable file(s)
onReadyRead() receives QNetworkDatagram datagrams and immediately calls asBytes(dg.data()) (line 132), creating a span over raw network memory without validating the datagram size. handleDatagram() then copies bytes into local spans and calls parseDdcFrame/parseHighPriorityStatus (lines 515-535), which will panic on oversized datagrams. This is a raw-memory buffer-copy vulnerability (CWE-800/memory-safety violation) reachable via UDP datagram transport. 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: $23.8805 · model: claude-opus-5 |
…Principle VIII. Review follow-ups on aethersdr#6146, all non-blocking, all from the review of c258baa. - alexRxFilterFor() sent a non-finite tuning to the 6 m preamp. Every comparison against NaN is false, so an unguarded ladder falls through to its last arm -- and on HF that selection is silence, not a weaker signal. No caller delivers NaN today (AnanBackend passes m_sliceFreqHz), so this is insurance rather than a fix. Two assertions pin NaN and +inf; -inf is deliberately not asserted, because it takes the `< 1.5` branch with or without the guard and so would not discriminate. Deleting the guard turns anan_p2_protocol_test red. - frontend.set now states that callers on the extension surface must persist to AnanSettings themselves. The GUI pairs the two, but a bridge or automation caller that only invokes the verb would get a change that silently reverts on the next connect, because connect reads AnanSettings and nothing else. - The droop branch carried a stray 8-space indent that read as if it were nested inside the frontend.set block. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
ten9876
left a comment
There was a problem hiding this comment.
Second-pass review at head 11dda168. It covers only this PR's own two commits (840cf4ff..11dda168); 840cf4ff belongs to #6137 and is reviewed there. Unlike the earlier aethersdr-agent review of c258baa0, which was reasoned from source, this pass builds the head, runs the tests, mutation-tests the claims, drives the new page, and checks the wire claims against p2app's source.
Issue fit
There is no linked issue; the body cites a G2 field report. Against that intent, all four items land, and the protocol claims are verified against p2app (laurencebarker/Saturn), not just consistent with the spec:
- Byte 1400 bit 1 = speaker-amp mute.
InHighPriority.c:154-156:SetXvtrEnable(Byte&1); SetSpkrMute((Byte>>1)&1). Bit 0 is XVTR, as the body says. - "1432–1435 is two 16-bit words."
InHighPriority.c:182readsrd_be_u16(+1432)as the TX/antenna word,:204-205reads+1430→AlexManualRXFilters(Word, 2)(RX2), and:207-208reads+1434→AlexManualRXFilters(Word, 0)(RX1). ADC0's receive filter really is the low half. - "p2app writes its register only when the value changed."
saturnregisters.c:1106,if(Register != GAlexRXRegister). Manual filter select is forced on at startup (p2app.c:519), so the client's bits are the ones that apply.
Follow-up from the earlier review: its Blocker 1, the false "no code dependency" sentence, is fixed in the current body, which now states the dependency correctly. 11dda168 does what its message says. The NaN/+inf guard is real (mutation below), the frontend.set persist note is present (AnanBackend.cpp:1373-1377), and the stray indent is gone (:1400).
Scope
| File / group | Change | Claimed | Verdict |
|---|---|---|---|
anan/AnanDiscovery.{h,cpp} |
per-interface directed broadcast, broadcastTargets() |
title | in scope, bench-confirmed |
gui/ConnectionPanel.{h,cpp}, gui/RadioSetupDialog.{h,cpp} |
7 connect-form rows → "ANAN Front End" page | title | maintainer decision (D3), as before |
anan/P2Protocol.*, P2Client.*, AnanSettings.*: Alex band-filter selector |
new wire-level relay control | body, not title | maintainer decision (D1), as before |
byte 1400 mute (P2Protocol, P2Client, AnanSettings, page) |
new wire control | body, not title | maintainer decision (D1) |
AnanBackend.cpp anan/frontend.set |
new extension verb | body | maintainer decision (D2); see the new nit on the capability fields |
RadioCapabilities.h + caps map |
AdcFrontEnd record |
body | shape OK (check_capability_records --strict: 70 … ok), but see nit 1 |
models/RadioModel.cpp |
two anan.* connect params inside the existing anan branch |
body | in scope |
tests/*, tests.cmake |
anan_front_end_test, anan_discovery_test new; three extended |
body | in scope; socket-free (no bind, listen, start() or fake peer in either new source) |
No CHANGELOG.md entry. Engine-boundary findings on this head versus #6137's head differ only by line shifts (102 findings either way, 0 blocking under --strict). The touchpoint manifest is up to date, and check_a11y, check_command_plane, check_test_registration, check_ci_test_gate, check_network_timeouts and check_qsettings are clean.
Blockers
None from this pass. Decisions D1–D5 from the earlier review (the band-filter selector and mute bundled under a discovery/setup-page title, the frontend.set verb, the connect-form relocation without an issue or RFC, the directed sweep reaching VPN/container subnets, and the unchanged absence of a sender filter in AnanDiscovery::onReadyRead()) are still open. They need a maintainer ruling, not author rework, and I've forwarded them to the maintainer.
Nits (non-blocking)
src/core/backends/RadioCapabilities.h:550-581: the fiveAdcFrontEndfields have no consumer, and the comment promises one. The header says "A false field means the control is not offered, not that it is offered and disabled". Nothing readshasDitherAndRandom,hasAdcSelect,hasFilterBankBypass,hasTunedBandFiltersorhasSpeakerAmpMute; a tree-wide grep finds them only inAnanBackend.cpp:411-415,anan_backend_test.cpp:534-539and the caps map. The page gates onadcFrontEnd.has_value()alone (RadioSetupDialog.cpp:4739) and builds all nine controls unconditionally. The same comment also says "these six" and "all six" (:556-563), but there are five fields. This is harmless today, since ANAN is the only declarer and declares all five true. Still, it is capability surface with nothing behind it, whichdocs/agents/backends.md§ Standing notice — #5554 asks PRs not to add ("no new capability declared without a verb behind it"). Either gate each control group on its field when connected, or collapse the record to the one fact the page actually reads.- Three comments describe a supply-rail calibration that is not on the page.
RadioSetupDialog.h:140("…the radio's speaker amplifier and the supply-rail calibration"),RadioSetupDialog.cpp:5-6("the rate list and the supply-rail reference are reached through this header") andConnectionPanel.cpp:843("the filter bank and the supply-rail calibration that joined them there"). The body says that control was built, bench-tested and removed. Per AGENTS.md § "Comments", these should state the current invariant. - The amp-mute hint overclaims.
RadioSetupDialog.cpp:5016says "A disconnect always hands the radio back unmuted, so quitting here cannot leave a radio that seems to have lost its audio." That holds forP2Client::stop(). It does not hold for a crash, akill, sleep, or a pulled cable. p2app'sCheckForActivity()(p2app.c:351-370) reverts to inactive after 1 s of silence and clears MOX/TX/CW, but never touchesSetSpkrMute. The radio stays muted until some client's next High Priority packet. Suggested wording: "A clean disconnect hands the radio back unmuted." - The disabled bypass checkboxes don't look disabled. Driven on the PR head: with "Follow the tuned frequency" selected,
ananBypassAdc0/1report"enabled": false, but in the grabbed page they render with the same text colour and check glyph as an enabled box.kChecksetsQCheckBox { color: {{color.text.primary}} }for every state, with no:disabledrule. The accessible description explains the state to a screen reader, but a sighted user gets no cue. AGENTS.md asks for a control the radio can't use to be "dimmed with a stated reason". AQCheckBox:disabled { color: {{color.text.secondary}}; }clause would do it. - Every family sees "ANAN Front End" while disconnected. This is driven and by design (
ananFrontEndAvailable()returns true with nothing connected), and it matches how the Flex-only pages already behave when disconnected. I'm noting it so the maintainer rules on it knowingly: a FLEX or HL2 owner opening Radio Setup offline now sees an ANAN-branded page.
What I tried to break
-
Built and ran
anan_backend_test,anan_p2_protocol_test,anan_front_end_test,anan_discovery_testandanan_settings_test: 5/5 pass. I didn't build #6137's socket-owninganan_link_telemetry_test. -
Mutations, each reverted (tree confirmed clean, then rebuilt; the probe build reports
dirty:false, sha11dda1683):- Delete the
std::isfiniteguard →anan_p2_protocol_test2 FAILs (NaN, +inf), as11dda168's message says. - Shift ADC0's filter into the high half of 1432–1435 → 3 FAILs.
- Drop
255.255.255.255frombroadcastTargets()→anan_discovery_test7 FAILs. - Set byte 1400 bit 0 as well → 2 FAILs.
- Stop
setDdc0FrequencyHz()storing hertz (the stale-filter case) →anan_front_end_test5 FAILs.
None of these tests asserts the implementation back to itself.
- Delete the
-
Drove the page (offscreen, isolated settings store,
AETHER_AUTOMATION_NO_TX=1, demo simulator only):- Disconnected: "ANAN Front End" is listed under Radio, and all nine controls are present with their accessible names and the defaults the body claims (48 ksps, ADC0, dither/random on, bank
Bypassed, both bypasses checked, speaker stream and amp mute off). - Interaction:
setCurrentIndex 1onananFilterBoard→ananBypassAdc0/1go"enabled": false. ClickingananSpeakerAmpMute→ the store holdsAnan {"bandFilters":true,"speakerAmpMuted":true}. - Gate: connecting
DEMO-0001hides the page from the nav (the demo declares noadcFrontEnd), and disconnecting brings it back. The nav grab is byte-identical to the first disconnected grab. - Persistence: after killing and relaunching, the page reads back
Follow the tuned frequencyand mute checked, so the settings survive a process boundary.
- Disconnected: "ANAN Front End" is listed under Radio, and all nine controls are present with their accessible names and the defaults the body claims (48 ksps, ADC0, dither/random on, bank
-
Byte-for-byte default. With
bandFiltersEnabledfalse, both filters staynulloptandbuildHighPriority()falls through to the pre-existingbypassAdc0Filtersbranch, and byte 1400 stays 0. The front-end test pins this. -
CI freshness. Runs for
11dda168were created2026-10-04T14:49:42Z, andmainis at18:26:40Z. Green here means a merge about 3.5 h stale that includes #6137's commit, and none of the five ANAN targets is on the per-PRctest -Rgate.
Could not verify: anything on a live G2. That includes the bench claims, the four untested filter selections, and whether the directed sweep reaches a second-NIC radio. The demo has no P2 transport.
Recommendation
Needs maintainer decision. I found no code blocker. The verified protocol work and the mutation-proven tests make this a solid change at the code level. What gates it is D1–D5. Separately, it can't merge before #6137, which currently has a requested-changes review (the rxPackets loss denominator). Nits 1–4 are small and worth folding in with whatever the rulings require.
|
Maintainer rulings on #6146 (following the review above)
|
11dda16 to
8d432ec
Compare
|
Thanks for the review and the rulings. #6146 is now one commit on #6137's head Rulings
Nits
Also changed
EvidenceOn
|
a52718e to
36ce5ce
Compare
|
Adversarial review at head Issue fitThere is no linked issue. The PR is measured against its own body and the 10-04 maintainer rulings: subnet discovery was accepted as-is, and the page move was sent to an RFC. Both halves do what the body says, with one exception covered under D1 below: discovery reaches a second NIC only when that NIC's subnet is unique on the host. The tests pass, the static checks reproduce, and the page behaves as described when driven. The split from the rulings is done: the band-filter selector, amp mute and Scope
Everything in the diff is explained by the body. No BlockersNone in the code. Needs maintainer decisionD1.
The global broadcast also leaves by one interface, so in that topology nothing reaches the radio. Reasoned, not reproduced: I have no multi-NIC host. This matters now because of what landed after the 10-04 ruling:
Related context, pre-existing and outside this diff: Options, your call:
D2. RFC #6209 is still open, and its open question changes this PR's code. On 10-04 you ruled that the page move needs an RFC first and that "This PR waits on that". GOVERNANCE.md:131 says: "Do not open a PR until the RFC issue is approved." #6209 is Its open question is whether the page is listed for every family while disconnected (a) or only for an ANAN profile (b). That is exactly The body discloses this. I am recording it as a merge gate, not a defect. Given that discovery was accepted on its own, you may also prefer to land D1's resolution separately rather than hold it behind the RFC. Your 10-04 ruling bundled them, so that is your call too. Nits (non-blocking)1. The test builds its tunnel entry with no broadcast address ("which is the real shape of a tun device"). But So a /32 tunnel (WireGuard, Tailscale, most Every 5 s sweep then sends a discovery datagram to this host's own The fix below passes both the original fixture and the Qt-shaped one (both run): if (broadcast.isNull() || broadcast.protocol() != QAbstractSocket::IPv4Protocol)
continue;
// A /31 or /32 entry (a WireGuard or Tailscale tunnel, usually) has no
// broadcast domain, but QNetworkInterface still reports ip | ~netmask
// for it -- which for a /32 is this host's own address.
if (entry.prefixLength() >= 31)
continue;In the test, replace the // What QNetworkInterface actually reports: postProcess() fills ip | ~netmask.
tunnel.setBroadcast(QHostAddress(QLatin1String("10.8.0.6")));2. The removed connect-form combos had feRateCombo->setAccessibleDescription(QStringLiteral(
"DDC0 sample rate in ksps, and the panadapter span a session starts at. Higher "
"rates use more of the radio's Ethernet link. Takes effect on the next connect.")); feAdcCombo->setAccessibleDescription(QStringLiteral(
"Which receive chain feeds the receiver: ADC0, behind the switched ANT1/2/3 relay "
"bank, or ADC1, wired directly to its own RX2 jack. Takes effect on the next connect."));3. QStringLiteral("Sample rate, ADC choice, dither, randomization, filter bypass and the "
"radio's own speaker are in Settings > Radio Setup > ANAN Front End. "
"They apply to the next connect."),4. What I tried to break
RecommendationNeeds maintainer decision. I found no code blocker. The page move is clean and verified at runtime, and the capability shape follows canon. Two things gate it:
Nits 1 to 3 are small, and nit 1 comes with tested drop-in code. |
|
@K5PTB, thanks for the review, and for driving the page through the bridge. Nits 1-4: all valid. I'll fold them into the same round as D1, so the branch is rebuilt once. On nit 1 I confirmed the cause in qtbase 6.12: D1: you're right that the two comments overreach. Two interfaces on one subnet get one datagram, and it leaves by whichever interface the route prefers. I recommend (b), in the shape deskHPSDR uses: @ten9876, two decisions are yours:
Once you rule, D1, nits 1-4 and anything #6209 changes go in together. |
36ce5ce to
cccc3ab
Compare
|
@tropo1234 Heads up - this PR got delayed because it combined three sets of UX changes on connect, in radio setup, and subnet discovery. I'm going to merge this with single interface discovery, if you want to propose multi-interface discovery, I'd file that implementation in a separate PR. |
cccc3ab to
a8ae12c
Compare
|
@K5PTB @tropo1234: the landing pass is now at signed head
The final clean app reports The earlier Antares advisory is not reproducible from the current parser: unchanged No unresolved inline review threads or changes-requested reviews were present. Fresh PR CI is running on this exact head; approval and merge will follow its completion. Generated with OpenAI Codex (GPT-6 Astra) |
Send directed discovery broadcasts to distinct attached IPv4 subnets while retaining the global broadcast. The single wildcard socket still follows the preferred route when multiple NICs share a subnet. Exclude /31 and /32 point-to-point entries, including Qt-synthesized self-address broadcasts. Move the seven existing ANAN connect-time options to Radio Setup's ANAN Front End page, retaining the same AnanSettings values and next-connect semantics. Keep the page available offline and capability-gated online, with an ANAN-only pointer on the connection form and accessible descriptions for both combos. Update the matching user guide pages and log references. RFC aethersdr#6209 approves the setup page. The operator explicitly retained the current discovery design. Band-filter selection, amplifier mute and the frontend.set extension remain outside this change. Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com> Co-authored-by: jensenpat <patjensen@gmail.com>
a8ae12c to
04400ba
Compare
|
Main advanced during the first CI cycle:
Fresh CI on this head must pass before approval/merge; the earlier head's results are not being used for the new integration. Generated with OpenAI Codex (GPT-6 Astra) |
jensenpat
left a comment
There was a problem hiding this comment.
Approved at exact signed head 04400ba. RFC #6209 is approved with offline visibility choice (a); the operator explicitly retained single-socket discovery with its distinct-subnet scope. The four remaining review nits are fixed: Qt-shaped /31 and /32 tunnel filtering, accessible descriptions, the complete connection hint, and removal of the redundant rate assertion. Matching user docs and source references are updated.
Current-head evidence: macOS Qt 6.12 build passed; 100% tests passed, 0 tests failed out of 3 for anan_discovery_test, anan_settings_test and anan_backend_test. The prefix-guard mutation at a8ae12c caused both /31 and /32 assertions to fail; git range-diff proves that patch unchanged by this rebase, and restored code passes on this head. The isolated offscreen bridge reports sha=04400ba31, dirty=false; it verified seven controls and their descriptions, all seven preferences after process restart, the ANAN-only hint across five family selections, and withdrawal/restoration of the page across DEMO-0001 connect/disconnect. TX was disabled and proof instances were closed.
Base/head boundary findings are identical; capability and command-plane gates pass with no baseline growth. Docs build and 707 Log Analyzer checks pass. The manual final source pass found no remaining blocker; automated code-review was unavailable. No unresolved inline threads or blocking reviews remain. No live-radio or multi-NIC hardware claims are added.
The unchanged parser's exact-length guards refute the older advisory; P2Client.cpp is outside this diff. Windows, Docs and Static checks have passed on this head. Linux and macOS remain in progress; this approval does not claim they passed. GitHub's required checks and mergeability gates must pass before auto-merge can fire, under the operator's concurrent-merge workflow.
ANAN-G2 receive front-end options live under Settings → Radio Setup → Radio → ANAN Front End, so users connecting from Available radios can configure them too. The seven existing options retain their
AnanSettingsvalues and next-connect semantics; Connect by IP shows an ANAN-only pointer to the page.RFC #6209 is approved, with visibility choice (a): the page is available while disconnected, and capability-gated while connected. Other families' controls, settings and protocol paths are unchanged. The band-filter selector, speaker-amplifier mute and
anan/frontend.setremain outside this PR.ANAN discovery retains the global broadcast and adds one directed broadcast per distinct attached IPv4 subnet. This covers a second NIC on a distinct subnet. It retains the existing single wildcard socket: two NICs sharing a subnet still follow the preferred route, and VPN/router broadcast restrictions still apply. The operator explicitly chose to retain this design.
/31and/32point-to-point entries are excluded even when Qt synthesizes a broadcast address equal to the local address.The user guide covers the page location, all seven current labels, next-connect behavior and discovery limitations. The connection hint includes dither and randomization, and both new combos retain accessible descriptions.
Validation on signed head
04400ba31, rebased onto5813556e0:anan_discovery_test,anan_settings_test,anan_backend_test./31and/32assertions ona8ae12c0a; the discovery source and PR patch are unchanged by the second rebase (git range-diffreports equality). The restored guard passes on the current head.npm ciand user-doc build pass; generated references are current; Log Analyzer passes 707 checks with refreshed source locations.No live-radio, multi-NIC hardware, RF or TX validation was performed in this landing pass. Cross-platform CI is checked separately before merging. The automated
code-reviewskill was unavailable; the final source pass was performed manually.Generated with OpenAI Codex (GPT-6 Astra)