Repository navigation
Conversation
Refs aethersdr#6471. Add a bounded independent analyzer session, source-referenced HID transport, finite FRX acquisition, quick-check window, atomic capture export and explicit analyzer RF automation. Keep count and RF-off claims unqualified until real Stick Pro bring-up; defer station tabs and the TX arbiter interlock.
Replay the live macOS identity failure: firmware 108 marks full 64-byte FULLINFO reports as 63 payload bytes despite the 62-byte text capacity. A 256-byte read and the HID descriptor confirm the size. Bound this convention only for identity metadata, retain the raw report and diagnostic count, and keep malformed measurement and setup reports fatal. The captured regression fails before the fix and passes afterward.
jensenpat
changed the title
![aethersdr-agent[bot]](https://avatars.githubusercontent.com/in/3286087?s=60&v=4){kind=small}
There was a problem hiding this comment.
Thorough, unusually honest work — the PoC test file and docs/rigexpert-usb-poc.md are the best "here is exactly what I did and did not prove" writing I have reviewed on this project. I spent most of my time trying to break the RF gating and the workspace/session switching; the gating held, the workspace switching did not. All findings below are reasoned from code — I am headless, with no build and no way to drive the GUI, so nothing here was reproduced at runtime.
1. Issue fit
Partially. RFC #6471 is labelled RFC Approved, and the PR correctly scopes itself to the "image 1 quick-check" USB bring-up subset. Discovery/identity, serialized finite FRX acquisition, cancellation, raw inspection, the quick-check view, bridge/MCP coverage and user docs are all present and map to RFC requirements.
What the RFC asked for and this PR does not deliver — all of it disclosed in the PR's own docs, none of it waived in writing:
| RFC "RF intent and return-to-radio contract" requirement | This PR |
|---|---|
| "Lab entry requires the AetherSDR-controlled radio to be idle. Busy TX produces a clear refusal" | Not implemented — showAetherLab() enters unconditionally |
| "AetherSDR-originated transceiver TX is inhibited while the lab is active… Cover actual arbiter producers… greyed-out GUI buttons are insufficient" | Not implemented — replaced by a yellow "Radio TX is not inhibited" label (src/gui/AetherLabWidget.cpp:266) |
| "Analyzer Sweep must join the bridge's RF-producing-action policy" | Done — AETHER_AUTOMATION_ALLOW_LAB_RF + isolated token + the aetherLabRf widget gate |
| "Selecting the radio during acquisition cancels it through the qualified stop path" | Done — returnFromAetherLab() calls stop() |
The only approval record on #6471 is the author's own comment reporting verbal maintainer approval; the RFC's five decision checkboxes are unchecked. I am not calling the missing interlock a Constitution VI violation — VI is about keying the transceiver, and nothing here does — but shipping the workspace without the interlock is a departure from the contract the RFC put up for ratification, and it needs @ten9876 to rule rather than a reviewer to wave through. See Blocker 2.
Test layering is right: all three new tests are socket-free injection (rigexpert_usb_poc_test, aetherlab_window_test, automation_lab_test), no fake firmware peer, positive hardware convergence correctly routed to the documented bench session instead of a synthetic device. No socket test is added or removed by this PR — I checked the added test sources for QTcpServer/QUdpSocket/QLocalServer/bind/listen/connectToHost and for Fake* peer classes; automation_lab_test drives AutomationServer::handleLine directly with a nullptr socket.
2. Scope
| File / group | What it changes | Claimed? | Verdict |
|---|---|---|---|
src/core/RigExpertHidTransport.{h,cpp}, src/models/AntennaAnalyzerModel.{h,cpp} |
New instrument transport + model | Yes | In scope |
src/gui/AetherLabWidget.{h,cpp} |
New workspace view | Yes | In scope |
src/gui/MainWindow*.{h,cpp} |
Lazy stack, Tools action, tab routing, lab handler | Yes | In scope (see Blocker 1) |
src/gui/RadioTabBar.{h,cpp} — Kind::Workspace, linkRadioId/linkCarrierTab/setActiveRadio guards, state() kind key |
Workspace tabs must not steal the heartbeat | Yes | In scope, and the right seam |
src/gui/RadioTabBar.cpp:603-605 — viewport-Resize → singleShot(0, applyActiveState) |
Re-runs tab visibility + ensureWidgetVisible on every strip resize |
No | Unexplained — see Nits |
src/gui/RadioTabBar.cpp:952 — if (isActive && !m_compact) { shown = tab; } |
Changes which tab updateTabViewport scrolls to, for radios too |
No | Unexplained — see Nits |
src/gui/RadioTabBar.cpp:1193 — popover scroller 190 → 230 |
Room for the new AetherLab row | Implied | In scope |
CMakeLists.txt, tests/tests.cmake |
Source + target registration | Yes | In scope. AETHER_AUTOMATION_SERVER_TESTS correctly joined |
docs/architecture/aetherd-touchpoint* |
+1 models touchpoint, ThemeManager 178→179 |
Yes | In scope and verified: I ran tools/gen_touchpoint_manifest.py in the head checkout — it reproduces 256 headers with a zero diff, so the manifest is not stale |
docs/rigexpert-usb-poc.md, docs/user/docs/aetherlab.md, sidebars.js, docs/automation-bridge.md, tools/aether_mcp.py |
New docs + lab verb reference |
Yes | In scope |
docs/user/docs/automation-bridge-and-mcp.md:43 |
Observe-only row rewritten from "Blocks every action that changes anything" to "Blocks driving actions; reads and AetherLab stop/disconnect remain available" | Not in the body | Documented-contract change — see Blocker 2 |
No CHANGELOG.md entry (correct). The self-certification holds apart from the three RadioTabBar rows above. New public surface: the lab verb, the AETHER_AUTOMATION_ALLOW_LAB_RF env var, and the radios.tabs[].kind / status: "workspace" bridge keys — third parties bind to these, so they belong in the maintainer decision alongside the interlock.
3. Blockers
1. Minimal mode is permanently degraded after one AetherLab visit — src/gui/MainWindow_Controllers.cpp:3419-3427 vs src/gui/MainWindow.cpp:10379 / 10462.
showAetherLab() builds the stack once and never tears it down:
layout->removeWidget(m_splitter);
layout->addWidget(m_labWorkspaceStack, 1);
m_labWorkspaceStack->addWidget(m_splitter);Minimal mode still reaches straight past the stack for the splitter (MainWindow.cpp:10379-10382):
m_splitter->hide();
m_appletPanel->setParent(centralWidget());
centralWidget()->layout()->addWidget(m_appletPanel);
m_appletPanel->show();Before this PR, m_splitter was the stretch-1 item in the central QVBoxLayout, so hiding it freed the box and m_appletPanel took the window. After one AetherLab visit, m_labWorkspaceStack is the stretch-1 item and nothing hides it: m_splitter is hidden inside the stack, so the central layout keeps a visible, stretch-1 stack showing an empty page, and m_appletPanel is appended below it at its bare sizeHint. QStackedLayout::minimumSize() is also the max over all pages, so m_aetherLabWidget's minimum (the plot alone is setMinimumSize(360, 260)) now contributes to the central layout's minimum and fights setFixedWidth(260) two lines later. Depending on how Qt resolves the explicit-vs-layout minimum, the symptom is either a 260 px window of mostly blank stack with a squeezed applet panel, or a window that will not reach 260 px at all.
This is the exact shape of the #8600 field report the comment eight lines above 10379 was written about, and it persists for the rest of the session — returnFromAetherLab() only changes the stack's current widget. toggleMinimalMode(true)'s new first line (if (on && m_aetherLabActive) { returnFromAetherLab(); }) does not help, because the stack outlives the lab being active. The exit mirror at 10462 has the same shape.
Fix — hide and show the stack when it exists, at both sites:
m_splitter->hide();
if (m_labWorkspaceStack) { m_labWorkspaceStack->hide(); } if (m_labWorkspaceStack) { m_labWorkspaceStack->show(); }
m_splitter->show();Worth re-checking toggleWorkspaceCanvas by hand as well; I grepped and minimal mode is the only other writer of centralWidget()->layout(), so I believe canvas mode is unaffected, but I could not exercise it.
2. The RFC's RF contract ships unimplemented, and the observe-only contract changes — both need a maintainer ruling, not a reviewer's approval.
docs/rigexpert-usb-poc.md:198 states it plainly: "There is no station TX interlock in this PoC." The RFC put the interlock, the busy-TX entry refusal and "review the detailed interlock against the current TX-coordinator contract before a feature PR" up for approval as one contract, and the only approval on record is a self-reported verbal one for the quick-check design. The substitute is a warning label. That may well be the right call for a macOS-only, experimental, documented PoC behind an explicit RF opt-in — but it is @ten9876's call.
Bundle two more items into the same decision:
-
src/core/AutomationServer.cpp:2765-2770reclassifieslab stopandlab disconnectas read-only, so they run under Observe only, anddocs/user/docs/automation-bridge-and-mcp.md:43rewrites the documented promise accordingly.stopis defensible (it is the RF-off safety action; blocking it in observe-only would be worse).disconnecthas no safety rationale —AntennaAnalyzerModel::disconnectDevice()→finish(State::Disconnected)writesoff\rand tears down the operator'sReadysession, so an observe-only client holding the token can destroy in-progress instrument state. Credit to security-audit (CodeGuard) for flagging the authorization split. Minimal change if the maintainer agrees:if (name == QLatin1String("lab")) { // `stop` is a deliberate exception: it is the RF-off safety action, and // refusing it in observe-only mode would be the less safe answer. return normalizedAction.isEmpty() || normalizedAction == QLatin1String("status") || normalizedAction == QLatin1String("devices") || normalizedAction == QLatin1String("stop"); }
-
The new public surface listed in §2 (
labverb,AETHER_AUTOMATION_ALLOW_LAB_RF,kind/status: "workspace").
4. Nits (all non-blocking)
tests/automation_lab_test.cpp:66— thegestureassertion passes vacuously.doGesturereturnserr("gesture requires a live client connection")atsrc/core/AutomationServer.cpp:10258, before thebeginbranch callspointerSafetyErrorat10272. The test passesnullptrforsock, soQVERIFY(!...request(bridge, "gesture begin injectedAnalyzerSweep")...)would pass with the new guard deleted. The guard at9877is correct — it just has no non-vacuous coverage, anddrag/dragAt/wheel(the other threepointerSafetyErrorcallers) are untested.drag injectedAnalyzerSweep 1 1needs no socket and would prove it:QVERIFY(!AutomationServerTestAccess::request(bridge, "drag injectedAnalyzerSweep 1 1").value("ok").toBool());
src/core/RigExpertHidTransport.cpp:61-63— the session lock lives in shared/tmpunder a guessable name (confirmed by security-audit (CodeGuard), low severity, Linux-only).QDir::temp()is per-user on macOS/Windows but shared/tmpon Linux, and the name issha256(<hid path>)over the small enumerable/dev/hidrawNset. Any local unprivileged uid can pre-create and hold the file;tryLock(0)then fails forever with the misleading "claimed by another AetherSDR instance", and/tmp's sticky bit blocksQLockFile's own stale-removal of a foreign-uid file. No escalation (O_CREAT|O_EXCL, so a planted symlink getsEEXIST).QStandardPaths::writableLocation(QStandardPaths::RuntimeLocation), falling back toAppLocalDataLocationwhen empty, is per-uid 0700.src/gui/RadioTabBar.cpp:603-605— unexplained resize hook. Every scroll-viewportResizenow schedulesapplyActiveState(), which re-runs tab visibility andupdateTabViewport()→m_tabHost->setFixedWidth(...)+ a deferredensureWidgetVisible. It converges (the width write is idempotent), but it means the strip now re-scrolls to the active tab on every window resize, overriding an operator's manual drag-scroll. What does this fix? It is not derivable from the RFC.src/gui/RadioTabBar.cpp:952—if (isActive && !m_compact) { shown = tab; }changes radio behaviour too.shownfeedsupdateTabViewport(shown)after the loop, so non-compact strips now scroll to the active tab instead of the link-carrier tab. Normally the same tab; they diverge exactly when a radio is active but the carrier is a different radio. Probably intentional (so the AetherLab tab scrolls into view), worth a line in the body.src/gui/MainWindow_Controllers.cpp:3432—m_connPanel->hide()is never undone.returnFromAetherLab()has no matchingshow(). The panel is a modeless top-level dialog reachable again from Tools/the station label, so this is recoverable, but an operator who had the connection picker open when they opened AetherLab silently loses it. Either re-show it or say so indocs/user/docs/aetherlab.md.src/core/RigExpertHidTransport.cpp:143-151—fail()can emitfailedtwice and overwrite the better error.fail()→finish(stop)→write(stop); if that write fails it callsfail()again, so the model's secondfinish()replaces e.g. "HID receive backlog exceeded 64 reports" with "HID write failed or short". Terminates correctly (m_finishedguards the recursion), just loses the useful diagnosis.docs/rigexpert-usb-poc.md:94— "app close … attemptsoff\r, closes USB" is best-effort, not a sequence.~AntennaAnalyzerModelcallsm_port->finish(...), which only queuesfinishRequestedto a parentlessQThreadthat is never joined (HidTransport::startcreates it withnew QThreadand relies onfinished → deleteLater). Whether theoffreport reaches the wire beforemain()returns is a race. Given how carefully the rest of the doc separates host actions from device evidence, this sentence deserves the same hedge.docs/rigexpert-usb-poc.md:205anddocs/automation-bridge.md:4648call the Sweep buttonaetherLabRf. That is the property; the automation target name isaetherLabSweep(src/gui/AetherLabWidget.cpp:337-339). An automation author following the docs targets a name that does not resolve.src/gui/AetherLabWidget.cpp:525— the status line buildstr("%1 · %2 · %3 received rows%4")with a multi-arg.arg(label, identity)followed by.arg(int); a device identity string containing a literal%3would capture the row count. Cosmetic only, device-controlled.
5. What I tried to break (and could not)
- The analyzer-RF gate, every automation path I could find.
doLabnormalizes withtrimmed().toLower()andAntennaAnalyzerModel::automationre-normalizes identically, so there is no case/whitespace/alias divergence onsweep;connectemits onlyVER/FULLINFO. On the widget sidehasAnalyzerRfInChainis reached fromdoInvoke:4509(before action dispatch, soclick/setValue/focusare all refused),pointerSafetyError:9877(drag/dragAt/wheel/gesture) anddoClickAt:11030(doubleClickdelegates there). Thereturn rabove thedoInvokeguard is the!wQAction-only branch, unreachable for aQPushButton. The keyboard bypass I expected — Tab ontoaetherLabSweep, thenkeyevent press Return— is closed becauseMainWindow::injectKeyEventForAutomationsends tothis, neverfocusWidget()(src/gui/MainWindow_Shortcuts.cpp:1837-1842, with that exact rationale in the comment).hasAnalyzerRfInChainis null-tolerant. I found no bypass. - Device-controlled input into the parser.
declaredis clamped byqMin(declared, size-2); the 63-vs-62 firmware quirk is confined toIdentifying/Readyandrigexpert_usb_poc_test::overclaimedMeasurementRemainsMalformedproves a measurement report with the same overclaim still fails;m_pendingcaps at 512,m_traceat 128,m_infoat 32;m_samplescannot exceedintervals+1because strict monotonicity plus thehz > m_stopHzcheck makes a late extra row an error rather than growth.impedanceSampleanchors every field to a numeric regex and routes all divisions throughfiniteValue(), so R=0/X=0, open, non-passive and1e400all land as JSONnullwith a quality string rather than UB — the test covers each. - Stale-callback and reentrancy paths in the model. Every transport lambda captures
portand compares againstm_port;cancellationAndOldGenerationproves a retired transport's reports are traced but never accepted and cannot drive a new session.finish()is idempotent via theClosingcheck.fail → finish → write → failterminates (see Nits for the cosmetic loss). - The touchpoint manifest. My first guess was that
core/RigExpertHidTransport.hhad atags.jsonentry but no manifest row, i.e. a stale regeneration. I rantools/gen_touchpoint_manifest.pyin the head checkout: it rewrites to the same 256 headers with a zero diff, and the header is correctly absent because nosrc/guifile includes it directly.tools/check_engine_boundary.pyreports 0 blocking findings. Clean. - Could not check: anything at runtime. I never built, ran a test, or drove the GUI. Specifically unverified by me: Blocker 1's exact visual manifestation (I reasoned it from the layout code and
QStackedLayout's all-pages minimum, not from a screenshot); whether the three new tests pass. CI is green across all 7 checks ona2cd777, but.github/workflows/ci.ymlonlyctest -Rs four DV tests,cross_needle_meter_test,mac_nr_filter_testandasr_gpu_probe_test— so green here provesrigexpert_usb_poc_test,aetherlab_window_testandautomation_lab_testcompile on macOS and Windows, not that they pass. Every CodeGuard finding handed to me was in.github/workflows/*files this PR does not touch; I dropped them. The two findings on this PR's new code are credited above.
6. Recommendation
Needs maintainer decision, with one real code blocker to fix first.
Blocker 1 is a concrete regression of a shipped feature with a two-line fix, and it is independent of any policy question. Blocker 2 is not something I can resolve: the RFC's RF contract — TX inhibit while the lab is active, plus a busy-TX entry refusal — is the part the RFC explicitly asked to ratify, and this PR substitutes a warning badge and says so in its own docs. @ten9876 should state whether the warning badge is an acceptable stand-in for an experimental, macOS-only, opt-in-gated PoC, and at the same time bless (or not) the new lab verb, AETHER_AUTOMATION_ALLOW_LAB_RF, the kind/status: "workspace" tab keys, and lab disconnect running under Observe only.
Next step for the author: apply the two m_labWorkspaceStack hide/show lines, swap the vacuous gesture assertion for a drag, and move the lock file off shared /tmp. Everything else above is a nit. The instrument model, the protocol state machine and the injected test suite are genuinely good, and the refusal to over-claim what the bench session proved is exactly the right instinct — thank you.
🤖 aethersdr-agent · cost: $17.7887 · model: claude-opus-5
|
Cisco CodeGuard — static analysis of this PR (1499 finding(s))
Automated static scan by Cisco DefenseClaw CodeGuard on the changed files. Advisory — some may be false positives; the review above verifies them. 🤖 aethersdr-agent · cost: $17.9808 · model: claude-opus-5 |
jensenpat
changed the title
Ozy311
left a comment
Ozy311
left a comment
There was a problem hiding this comment.
Issue fit: this implements the explicitly scoped USB quick-check subset of RFC #6471, rather than the complete analysis/export/interlock roadmap. I reconstructed intent from Summary and the linked RFC/claim. The documented manual-isolation limitation remains a maintainer scope decision already raised in the prior review. Recommendation: request changes for the two additional functional findings below.
Reviewed head a2cd77779e46cb92060babef2b37f61d2efcde56; current trusted main b5b540920985cc214bac259104be2f8045030578; merge base 86f592a9f5303e3581d514d9ceb8a7dead9a3235.
Scope
| File/group | Change | Claimed? / assessment |
|---|---|---|
| RigExpertHidTransport / AntennaAnalyzerModel | Independent USB instrument, finite acquisition, validation and derived measurements | Yes; fits bounded PoC. Current-main HID integration needs correction below. |
| AetherLabWidget / MainWindow siblings / RadioTabBar | Embedded workspace, marker/readouts, radio return and separate heartbeat ownership | Yes; fits scope. Tab scrolling changes support workspace selection. |
| AutomationServer / aether_mcp.py | lab verb, separate analyzer RF opt-in, generic UI refusal, observe-only stop/disconnect exception |
Disclosed across body/docs; public surface and exception remain part of maintainer review. |
| CMake / tests | Source registration, three injected tests, titlebar coverage | Yes; no new socket-owning test or synthetic firmware peer. |
| User/bridge/qualification docs and touchpoint metadata | Feature documentation and model exposure | Yes; no CHANGELOG change, no unrelated source additions found. |
Additional blockers
-
[P1] Preserve current-main HID discovery serialization —
src/core/RigExpertHidTransport.cpp:32–33, also65–67. The new transport callshid_init(),hid_enumerate()andhid_open_path()directly. Main's #6606 (78f20e957) routes these operations throughHidDiscoverybecause macOS HIDAPI 0.15 shares a manager and global error string across threads; concurrent discovery/open can race and abort the application. Clicking Refresh USB or Connect while HID encoder hotplug/CTR2 discovery runs reopens that race even after resolving the textual merge conflicts. The per-device QLockFile does not protect process-wide HIDAPI state. A hardware-free probe compiling the exact PR transport with current-mainHidDiscovery.cppand stub HID symbols recorded zero overlapping operations for two guarded callers, versus hundreds when one caller was the PR transport. Routing the scratch transport through the facade reduced overlap to zero. This proves the synchronization bypass, not an independently reproduced hardware crash. Route init/enumeration/open through the existing facade, includingopenPath's protected error capture, and extend the socket-free concurrency coverage to this caller. The PR's injected model tests replace the production transport, so they cannot detect this. This is the current-base integration check required by Principle X. -
[P2] Make the current marker readings available to accessibility clients —
src/gui/AetherLabWidget.cpp:383–388, also368–370. Each numeric QLabel is given a fixed accessible name such asSWRorMarker frequency, overriding Qt's normal exposure of its visible text. An offscreen probe of the actual production widget after injected acquisition, with only Qt 6.12 frameworks loaded, returned:aetherLabValue_swr: visibleText="1.080", Name="SWR", Value="", Description="", textInterface=false, valueInterface=false aetherLabMarkerFrequency: visibleText="14.350000 MHz", Name="Marker frequency", Value="", Description="", textInterface=false, valueInterface=falseThe current reading is unavailable when a screen-reader user navigates to the completed measurement; emitting
QAccessibleValueChangeEventdoes not populate that persistent interface. The plot exposes sample count, not the missing measurements. The completion-status label has the same problem. Include the current reading and units in the accessible text, or provide a value interface, and test it after acquisition and marker movement. The existing test checks visual text and the plot interface, not these labels' accessible values. This is Qt-interface evidence; VoiceOver itself was not exercised.
Existing review and readiness
I read the existing review at this same head and have not duplicated its findings as new issues. Its Minimal mode regression is now independently reproduced in the pristine application: before any lab visit, the applet panel starts at y=52 with height 748; after Lab → Return → Minimal, the workspace stack stays visible at height 154, pushing the applet panel to y=231 with height 568. Screenshots confirm the blank band. The station-interlock scope decision and other previously noted items remain in that discussion.
GitHub reports CONFLICTING / DIRTY. A local merge-tree calculation against the trusted main reproduced conflicts in CMakeLists.txt, docs/architecture/aetherd-touchpoints.md, src/core/AutomationServer.cpp, src/gui/MainWindow_Session.cpp, and tests/tests.cmake. Merge current main while retaining both sides' additions, regenerate the manifest, and rerun affected checks; the HID facade integration above needs attention even though the new transport file itself has no textual conflict.
Non-blocking process note: the PR has no separate Intent section; its Summary and bounded-scope documentation nevertheless made the intended milestone clear. No additional style nits are being added to the existing review.
Verification
- Trusted-base governance manifest PASS; constitution mirrors match (SHA-256
0214ddb687ce5b5647c0e76080db1c3e85d320bedab335fd706690073869a37f). Full instruction inventory and applicable domain docs were read across the review team. - All four engine gates pass on merge base and head. Per-file tracked findings are unchanged: 102 engine warnings, 70 capability booleans, 296 above-seam commands in 30 files. The touchpoint manifest is current; added model/peripheral exposure matches the stated architecture. Diff whitespace check passes.
- All eight commits have GitHub-verified valid signatures. Linux build, Windows, macOS, Static checks, sanitizer configuration and docs jobs succeeded for this head; release PDF attachment was intentionally skipped. These pull-request runs were created October 14 at 23:22:01 UTC and predate current main, so they do not validate today's integration. The legacy branch-protection endpoint was unavailable to this account; I am not certifying the live required-check configuration or substituting for human CODEOWNERS approval.
- Built and passed all four focused CTests on macOS arm64 / Qt 6.12:
rigexpert_usb_poc_test,aetherlab_window_test,automation_lab_test,unified_title_bar_test(4/4, 3.50 seconds with pinned framework runtime). Source snapshot was checked against every tracked blob with zero mismatches. The lean review build uses shared aethercore; ASR, RADE, DSTAR, DFNR and SPECBLEACH were disabled. The tests exercised malformed reports/numbers, metadata-length exception isolation, ACK order, cancellation/stale generation, math singularities, automation RF refusal and tab heartbeat ownership. These results do not prove physical USB/RF behavior. - Mutation check: a separately compiled copy with the analyzer widget RF guard disabled made the unchanged
automation_lab_test::widgetRfGuardfail at its first denied-invoke assertion. The permission/observe-only test continued to pass. Original source and production objects were untouched; this confirms the invoke guard's test is non-vacuous, without claiming the earlier review's gesture-test concern is resolved. - Full application built successfully. An isolated offscreen run used fresh settings, a private authenticated bridge, the built-in DEMO-0001 session and transceiver TX disabled; analyzer RF permission was absent. Verified the demo identity, workspace switching and the Minimal mode regression above.
lab statusreported disconnected with RF automation disabled, andlab sweepwas refused. This was demo/Qt runtime evidence, not physical-radio proof. Closed only the review instance and verified its PID and socket were gone; all 7,242 tracked source blobs still match the reviewed head. The archive build reports runtime SHAunknown; the complete source-blob comparison establishes its provenance. - No analyzer hardware, live radio, physical RF-off, Windows/Linux USB hardware, calibration or native screen-reader qualification was performed. Historical bench claims remain author-supplied evidence. No fresh automated review service was available; this was a source review with independent subsystem passes and local probes.
Preserve current-main HID serialization and protected open errors; keep analyzer locks private and original failures intact. Restore Minimal Mode layout after lab visits, expose current measurement accessibility text, and tighten Observe Only disconnect and pointer-guard coverage. Validated with the full macOS Qt 6.12 build, eight focused socket-free CTests, 36 private-demo automation bridge requests, mutation checks, and current-main engine/docs drift checks. No new hardware qualification. Refs aethersdr#6471.
Ozy311
left a comment
Ozy311
left a comment
There was a problem hiding this comment.
Follow-up review of 99e09b8db8635824c79824564c2af188c1f822e9, compared with the previously reviewed a2cd77779e46cb92060babef2b37f61d2efcde56 and trusted main 809899110f5d425c0686d90190cd14d9c217f624 (merge base b5b540920985cc214bac259104be2f8045030578).
The bounded Stick Pro USB quick-check milestone remains consistent with approved RFC #6471. The source review found no new blocking defect. The two findings in my earlier change request are verified as fixed at this head.
Scope
| Area | Assessment |
|---|---|
| USB transport/model | Existing instrument scope retained; discovery serialization, private locks and first-error preservation correct the reported defects. |
| Workspace/readouts | Minimal Mode and accessibility corrections restore the intended behavior. Tab selection/scrolling and picker dismissal are now disclosed. |
| Automation | Separate analyzer RF permission and explicit isolation token remain enforced. Observe Only permits Stop and refuses Disconnect; pointer coverage now tests the analyzer refusal. |
| Build/tests/docs | Production transport is covered by injected host HID symbols, with no new socket-owning test. Main's additions were preserved during conflict resolution; generated documentation reflects the final source. |
Prior findings
- HID P1 closed,
src/core/RigExpertHidTransport.cpp:34–35,77–81: initialization, enumeration and opening now useHidDiscovery, including protected open-error capture. Independent exact-headhid_discovery_testpassed. Reverting only the analyzer's guarded initialization/enumeration in a scratch copy made the same test fail with 405 overlaps and 16 wrong errors. This is host-HID synchronization evidence, not hardware-crash reproduction. - Accessibility P2 closed,
src/gui/AetherLabWidget.cpp: accessible names now contain each reading and its units, marker movement updates those names, and the status label exposes its current text. The regression assertions query the actual Qt accessibility interfaces after acquisition and marker movement. The exact-headaetherlab_window_testpasses those interface checks, including marker movement, infinite return loss and undefined phase. A separate copied widget object with the live reading removed from its accessible name makes the unchanged test fail: actualSWR, expectedSWR: 1.080. Original production sources and objects remained untouched. - Previously reported Minimal Mode regression: the stack is now hidden/shown together with the radio workspace. The pristine full application passes fresh-settings DEMO-0001 checks after Lab → Return → Minimal, entering Minimal while Lab is active, and entering Minimal from Workspace Canvas after a Lab visit. In all three cases the stack is hidden and the applet panel occupies the content area (260 × 722 at y=77), rather than retaining the old blank workspace band. Full and Canvas restoration were also checked. This is offscreen Qt runtime evidence, not physical display or radio qualification.
- Other earlier comments: the first transport failure is preserved through a failed cleanup write; locks use an owner-only user directory; the pointer test checks the actual analyzer refusal; Observe Only no longer permits Disconnect. Documentation correctly names the Sweep target and describes asynchronous shutdown as best effort. Tab scrolling remains intentional and is now disclosed, as is picker dismissal. The device-identity formatting now uses one multi-argument substitution.
Verification and limits
- Trusted-base governance manifest PASS; constitution copies agree at SHA-256
0214ddb687ce5b5647c0e76080db1c3e85d320bedab335fd706690073869a37f. Applicable instructions were read across the review team; unchanged documents were checked against their previously read blobs. - Four engine gates pass on merge base and head with unchanged normalized findings: 101 tracked engine warnings, 70 capability booleans, 125 commands in 16 files. Manifest growth is the documented analyzer model with three GUI includers and the new widget's ThemeManager use; no existing vendor header was reclassified or baseline weakened. Whitespace check passes.
- All 8/8 focused CTests passed on macOS arm64 / Qt 6.12 in 8.15 seconds:
hid_discovery_test,rigexpert_usb_poc_test,aetherlab_window_test,automation_lab_test,unified_title_bar_test,automation_boundary_core_test,automation_boundary_widgets_test, andautomation_menu_lookup_test. This lean build uses shared aethercore with ASR, RADE, DSTAR, DFNR and SPECBLEACH disabled. All 7,543 tracked source blobs were checked against the reviewed head. The HID test needed scoped filesystem access for its private lock directory; it links stub HID symbols, so it cannot access a physical analyzer. - All four required checks are green: build, check-windows, check-macos, Static checks. Docs and sanitizer-configuration jobs also pass; release/deployment-only jobs were skipped as intended. CI started October 22 at 19:50:22 UTC. Its merge commit
be70577e7dd7520d81949fc52713686e2e0f752eexplicitly has parents main80989911and head99e09b8d; this run includes that main revision. A local merge-tree check against that revision also succeeds. Main subsequently advanced tof8d46dcdf9c427d0c315df8205e4daf77f29b9f6at 20:12:28 UTC via #6673 (ANAN filter-bank/speaker-mute work). The final merge-tree check now reports a conflict only indocs/user/src/components/LogAnalyzer/rules.json, on generated source-line references. GitHub reports DIRTY. Those new main changes are not covered by this PR CI run. The protection administration endpoint is unavailable to this account; the required-context list was read directly withgh pr checks --required. - All nine commits have valid GitHub signatures. No inline review threads are open. The existing CodeGuard comment concerns unchanged workflow files and adds no verified finding on this diff. A model-invocable automated review service was unavailable; independent subsystem reviews and local probes were performed instead.
- No new physical analyzer, radio/RF, native VoiceOver or Windows/Linux USB-device qualification was performed. The experimental scope still requires physical isolation; station TX inhibition and physical RF-off confirmation are not claimed. Historical hardware evidence remains tied to its original heads.
Review verdict: approve the code at 99e09b8d; no remaining actionable code finding in this pass. This supersedes my previous change request. Merge readiness is separate: the newly introduced generated-file conflict must be resolved and the updated head revalidated before enabling auto-merge. The contributor branch has not been modified during this review. No claim is made that this PR has merged or that post-merge main CI has run.
|
Not a re-review — Ozy's covers the code thoroughly and I have not duplicated it. This adds three things it could not: a merge against today's Reviewed head The conflict is two generated files, on every platform
Every source file auto-merges on every platform, including all four Ozy's P1 fix still holds against current
|
| test | Linux | macOS arm64 |
|---|---|---|
rigexpert_usb_poc_test |
Passed | Passed |
aetherlab_window_test |
Passed | Passed |
automation_lab_test |
Passed | Passed |
control: unified_title_bar_test |
Passed | Passed |
- Linux — gcc 13.3, Qt 6.12.0, hidapi-hidraw 0.14.0, RelWithDebInfo, CI's flags.
- macOS — arm64 26.6.2, Apple clang 21, pinned Qt 6.12.0, hidapi 0.15.0,
-DCMAKE_OSX_SYSROOT=…MacOSX26.5.sdk.
The macOS run is the relevant one for the P1 finding: hidapi 0.15 on macOS is exactly the configuration it was about, and the transport passes its tests there.
I also saw aetherlab_window_test fail to start on my Windows box. That is not attributable to this PR: unified_title_bar_test, which predates it, fails the same way in the same build directory. The control is the only reason I can say that with confidence.
Two defects that are not this PR's
1. main does not compile without hidapi. MainWindow.h declares m_touchHoldTimer, m_touchHoldGlobalPos, m_touchHoldWidget and m_touchHoldSwallowRelease inside #ifdef HAVE_HIDAPI, while MainWindow_Shortcuts.cpp uses all four in MainWindow::eventFilter with no guard — 20 × error C2065 when HAVE_HIDAPI is undefined. Reproduced from origin/main alone (members at 1537–1540, guard opened 1503). Touch-hold is a pointer behaviour rather than a HID one, so moving the declarations out of the guard looks more likely correct than guarding the uses. CI presumably always builds with hidapi, which is why it has not surfaced.
2. tools/gen_touchpoint_manifest.py crashes on Windows and truncates its output. UnicodeEncodeError: 'charmap' codec can't encode character '→' under cp1252, leaving aetherd-touchpoints.md empty (286 lines deleted). PYTHONIOENCODING and PYTHONUTF8 do not help, so the script opens its output without an explicit encoding. It runs cleanly on Linux ("wrote … 275 touchpoint headers"). The practical effect is that a Windows contributor cannot perform the documented conflict resolution for that file.
Happy to open issues for either if useful.
On the change itself
One part deserves singling out, since it is the actual hazard — an analyser injecting RF into a feedline that may still reach a transmitter. The PR treats it seriously: automation sweep requires both AETHER_AUTOMATION_ALLOW_LAB_RF=1 and an explicit isolated token, kept separate from the transceiver's TX gate; the isolation checkbox is cleared on generation change so a stale confirmation cannot authorise a later sweep; the UI states plainly that "Radio TX is not inhibited" rather than implying an interlock the software cannot enforce; and fail() writes off\r before hid_close(), with finish() idempotent, so an error path does not abandon a radiating analyser. That last detail is easy to omit and expensive to miss.
No hardware here — no Stick Pro, no RF, no physical isolation check. Every RF claim above is read from source, not observed.
|
Filed both of those, so they are not lost in this thread:
Neither blocks this PR. |
Intent
Deliver the bounded RigExpert Stick Pro USB quick-check milestone from RFC #6471. AetherLab is an embedded workspace for testing an isolated antenna or load and returning to the retained radio session. The broader RFC remains separate.
Behavior
+ → AetherLab, below Rescan radios, and Tools → AetherLab… open the same main-window workspace. The image 1 layout combines an SWR plot, a marker panel and separate series/parallel models. Wide windows place groups side by side; smaller windows reflow and scroll vertically. Readouts include SWR, return loss, |Z|, reflection magnitude/phase, impedance phase and signed series/parallel R/X/L/C. JSON preserves SI units and raw CSV; the UI uses engineering units and identifies undefined results. No smoothing or host calibration is applied.
Selecting a radio tab or Return to radio stops an open analyzer session and restores the radio workspace. Radio connection and heartbeat ownership remain on the radio. The active workspace tab stays in view as the tab viewport resizes, including when it differs from the heartbeat carrier. Opening AetherLab dismisses the connection picker; it remains available through the switcher. Minimal Mode returns to the radio and gives the applet panel the full available area, including after a previous lab visit.
AntennaAnalyzerModelis an independent peripheral model outsideIRadioBackend. Discovery accepts normal-mode Stick Pro serial prefixes 4600/4601 only. VER/FULLINFO precede an explicit FQ → OK → SW → OK → FRX acquisition. Host limits are 100 kHz–600 MHz, positive even-Hz span, 1–1000 intervals and at most 1001 rows; parsing checks finite CSV, ordering and endpoints. Setup ACKs, inactivity, total acquisition time, line length and the report trace are bounded. Completion is the host's count/endpoint cutoff. The captured Stick Pro 108 FULLINFO count quirk is accepted only for identification/idle metadata; measurement framing stays strict. ERROR-20 provides main-menu recovery guidance without retrying automatically.Each acquisition, cancellation or error attempts
offand closes USB. App shutdown queues best-effort cleanup without joining the I/O thread. Another sweep requires an explicit Connect and isolation confirmation. Physical RF silence remains unverified. Protocol references and qualification details are in the implementation guide; the wire behavior is based on RigExpert's MIT-licensed AntScope2, with no vendor source copied.New public automation surface:
lab status|devices|connect|sweep|stop|disconnect,AETHER_AUTOMATION_ALLOW_LAB_RF=1, and thekind/status: "workspace"fields in titlebar radio-tab state. Sweeps additionally require an explicitisolatedtoken. Generic widget/pointer acquisition is refused; use the typed sweep request. Observe Only permits status, inventory and Stop, and refuses Connect, Sweep and Disconnect.Review remediation
b5b540920, preserving both parents' additions and regenerating the touchpoint manifest and bridge reference.HidDiscoveryserialization, including the protected open-error copy. Device locks now use a private user directory; cleanup failure preserves the original transport diagnosis.Validation
cmake --build build -j22at99e09b8db.QT_QPA_PLATFORM=offscreen,-j22 --no-tests=error:hid_discovery_test,rigexpert_usb_poc_test,aetherlab_window_test,automation_lab_test,unified_title_bar_test,automation_boundary_core_test,automation_boundary_widgets_test, andautomation_menu_lookup_test. The HID test compiles the production transport against injected host HID symbols; no analyzer hardware, firmware peer or new socket-owning test is involved. Accessibility checks query actual Qt interfaces after acquisition and marker movement.a2cd77779completed a real 201-row Stick Pro 108 sweep on macOS, with independent derived-value recomputation. Earliere48a46e99transport soak completed 600.004 seconds, 40/40 sweeps, 8040 samples and 160 inventory checks without disconnect or protocol errors. These are prior-head bench results; hardware was not retested for this remediation.The only added GUI-to-engine touchpoint is the independent analyzer model (three includers); the view also uses the existing ThemeManager. No radio capabilities or persisted settings are added. User and automation docs are updated; CHANGELOG is untouched. Raw device identifiers and private captures are excluded.
Refs #6471.
Generated with OpenAI Codex (GPT-6 Astra)