Repository navigation
Conversation
Share saved connection identities between workspace tabs and Locations; fence superseded startup attempts; confirm and sequence single-session replacements; retain precise dialog recovery context. Persist browser-style tab ordering and use named canon colors for every menu selection state. Add bridge, rendered-menu, lifecycle and startup regression coverage plus user documentation and screenshots.
Preserve the selected-radio hierarchy from aethersdr#6885 while retaining active-path disabling and confirmed single-session switching.
![aethersdr-agent[bot]](https://avatars.githubusercontent.com/in/3286087?s=60&v=4){kind=small}
There was a problem hiding this comment.
Issue fit
Partially. The Icom half of this PR is genuinely good work, and the root-cause claim holds up under reading. On main, IcomCivBackend::m_scope is default-constructed (ScopeDecoder m_scope;), so it always ran at ScopeGeometry::points = kScopePointsIc705 = 475 regardless of model — while IcomModels.cpp has carried 689 for the IC-7610 since before this PR. The single-frame WLAN path then did m_partial.raw.resize(475, 0), truncating the sweep, and the downstream geom at IcomCivBackend.cpp:2523 mapped it across the full reported edges. A signal at true centre (bin 344 of 689) lands at 344/475 ≈ 72% of the display width — "about halfway between the center and the right side", which is #6871's wording verbatim, and the displacement scales with span, which is #5714's. src/core/backends/icom/IcomCivBackend.cpp:1462-1466 installs the model geometry at identity adoption and is the actual fix. That is a well-identified bug with a one-line cause, and the diagnosis is independently checkable in-tree.
What does not fit: six issues are on Closes: lines, five of them (#5714, #5341, #5154, #5096, #5084) IC-7610 hardware displacement reports, closed on offline fixtures alone — see Blocker 4. And roughly 70% of the diff is not this issue (Scope, below).
Scope
| Files | What it changes | Claimed? | Verdict |
|---|---|---|---|
src/core/backends/icom/{IcomCivBackend*,IcomModels*,IcomScope*,IcomAudio*,IcomSession*,CivCodec}, tests/icom_{dual_receiver,scope,audio,family,panadapter_capacity}_test.cpp |
Dual-receiver bring-up, 689-bin geometry, stereo RX, split/TX | Yes | In scope |
src/models/ConnectionWorkspace.{h,cpp}, src/gui/{ConnectionPanel,MainWindow,MainWindow_Session,RadioTabBar}.*, src/core/{SettingsCredentialPolicy.h,AppSettings.cpp,AutomationServer.cpp,IConnectionAutomation.h}, src/core/backends/flex/SmartLinkClient.*, src/core/backends/icom/IcomCredentials.*, tests/{connection_workspace,connection_panel_size,connection_panel_source_pick,startup_autoconnect_lockout,unified_title_bar,backend_family_switch,icom_credentials_singleflight}_test.cpp |
#6885 + #6889 verbatim (~4,500 lines) | Disclosed as "integrates" | Needs maintainer decision |
docs/user/docs/station-profiles.md (new, 134 lines), 15 new screenshots, screens.json, sidebars.js, 8 rewritten user pages, docs/architecture/window-chrome.md, aetherd-touchpoint* |
#6885/#6889 documentation | Disclosed | Same |
docs/{icom-dual-receiver-bringup.md,user/docs/networked-icom.md,user/docs/supported-radios.md} |
This PR's docs | Yes | In scope |
CMakeLists.txt, tests/tests.cmake |
Source + test registration | Yes | In scope |
docs/BUILDING.md:33 |
Adds -DREQUIRE_KEYCHAIN=ON to the macOS configure line |
No requirement explains it | Nit (the option does exist, CMakeLists.txt:34) |
CHANGELOG.md |
Untouched | — | Correct — don't add one |
Commits e5a66a1, 7a3fdbd, 136dba0, d878d7e are #6885 and #6889, and git merge-base confirms this branch sits on main (155850f5), not on either of those branches. Both are open. I'm not calling that concealment — the body says so plainly — but "integrates" and "stacked on" are different things, and only the maintainer can decide the ordering.
Blockers
1. src/models/ConnectionWorkspace.cpp:1 + src/gui/ConnectionPanel.cpp — this PR carries two other open PRs in full.
Merging #6912 merges #6885 and #6889, and their own reviews never happen under their own titles. #6885 in particular adds a new versioned feature document, a whole new connection UX, two new bridge verbs and a new credential-key class — the kind of change GOVERNANCE.md routes through an RFC, and it should get that scrutiny on its own PR, not as a prerequisite inside an Icom PR. If either changes in review, this branch diverges silently.
The Icom backend work is independent of the dialog: IcomCivBackend*, IcomModels*, IcomScope*, IcomAudio* and tests/icom_* touch nothing in #6885/#6889. Only the model-chooser EXPERIMENTAL badge does. Recommendation: merge #6885 → #6889 → rebase this, or rebase the Icom commits onto main and ship them first with the badge deferred. Maintainer's call on which.
2. src/core/backends/icom/IcomCivBackend_DualReceiver.cpp:306 — an unanswered LAN AF/IF read silently withholds all receive audio, which is the symptom #6872 reports.
|| m_lanOutput != 0 || !m_dualInitialisedand the same gate in the mono path at src/core/backends/icom/IcomCivBackend.cpp:4066:
if (hasDualReceivers() && m_lanOutput != 0) { return; }m_lanOutput initialises to -1 (IcomCivBackend.h:378) and resets to -1 on disconnect (IcomCivBackend.cpp:1065). The only thing that moves it off -1 is the 1A 05 <item> reply matched at IcomCivBackend.cpp:3436-3446, keyed on profileFor(*m_model).dualReceiver->lanOutputItem — 86 on the IC-7610, 123 on the IC-7760, both read from the guides and not yet confirmed on hardware. The operator warning only fires on changed && m_lanOutput != 0, i.e. only when a well-formed reply says IF:
if (changed && m_lanOutput != 0) {
emit configurationWarning(QStringLiteral(
"LAN output is IF. Select AF in the radio's LAN AF/IF Output menu for PC receive audio."));So if the item number is wrong for a firmware revision, or the radio NAKs the read, or the SET-menu encoding differs, m_lanOutput stays -1 forever, every audio frame is dropped at the top of onDualAudio/onAudio, and nothing is logged or surfaced. The operator sees working spectrum, working frequency readout, and dead audio — which is exactly #6872, one of the two reports this path is meant to address. The -1 case needs to be distinguishable from 1:
|| m_lanOutput != 0 || !m_dualInitialised→ keep the drop, but make the unknown state visible. Either warn once after the item read has gone unanswered for a bounded number of pollDualReceivers() cycles, or fail open to audio while m_lanOutput < 0 and warn that the AF/IF menu could not be read. Whichever you pick, lanOutput: -1 should not be a state the operator can only find by reading IcomCivBackend.cpp:6933's bridge snapshot.
3. tests/icom_scope_test.cpp:233-236 — an existing assertion was rewritten to accommodate a behavior change on the already-supported radios.
Before:
auto last = d.feed(makeContinuation(11, 11, chunk));
check(last->raw.size() == static_cast<std::size_t>(kScopePointsIc705),
"and is padded/trimmed to the geometry");After:
auto last = d.feed(makeContinuation(11, 11, std::vector<std::uint8_t>(25, 0x20)));
check(last->raw.size() == static_cast<std::size_t>(kScopePointsIc705),
"and contains exactly the native geometry");The input was changed to be exactly the right size, so the assertion no longer tests anything about a mismatch. That matters because IcomScope.cpp:130-132 and IcomScope.cpp:163-166 changed the shared decoder from pad/trim to reject:
if (m_partial.raw.size() != static_cast<std::size_t>(m_geom.points)) {
return std::nullopt;
}ScopeDecoder is shared by every Icom model. A sweep whose byte count differs from the table figure by even one now drops the entire sweep, silently, with no qCWarning — where main rendered a padded or trimmed trace. The models this lands on are not hypothetical: the IC-9700's 475 is measured, verified = false; the IC-7300's 475 and the IC-785x's 689 are cross-referenced and unverified. On any of those, a geometry discrepancy turns the panadapter permanently black with nothing in the log to say why. IcomScope.cpp:75-78 makes the same pad-vs-reject flip for an out-of-range mode byte (previously clamped to Fixed).
The PR body says "Existing Icom tests protect the previous models." This hunk is the counterexample. Either keep the old lenient path for models that aren't verified and reject only where the geometry is guide-confirmed, or keep the strictness and add a rate-limited qCWarning(lcIcomPan) naming the expected and received byte counts — plus a test that feeds a mismatched length and asserts the new behavior deliberately, rather than one that no longer exercises it.
4. The Closes: lines contradict this PR's own documentation.
docs/icom-dual-receiver-bringup.md:108 states the rule:
Do not describe the devices as qualified or close #6871/#6872 from offline tests alone.
The body then closes #5714, #5341, #5154, #5096 and #5084 — five IC-7610 waterfall-displacement reports, the same symptom class, the same evidence level, from the same offline fixtures. The geometry arithmetic above makes the fix likely correct, but likely is not what closing a device report asserts. Move those five to Related and let a hardware confirmation close them. (#6910 is fine to close — it is the author's own issue, created minutes before the PR with this body pasted into it.)
Nits
src/models/ConnectionWorkspace.cpp:206and:222—if (!save(document)) { return {}; }. A valid document was just loaded and validated; if a cosmetic one-time edit (renaming "Existing preferences" → "Radio operator", appending starter Locations) cannot be written back because the store is momentarily unavailable, the whole workspace is discarded and the operator sees an empty connection window. Them_workspace.isEmpty()guards atConnectionPanel.cpp:4666/4681/4743do stop it overwriting the good data, so this is degradation rather than loss — but returning the loadeddocumentand dropping only the failed cosmetic edit would be strictly better.src/core/SettingsCredentialPolicy.h:63-72—isScopedCredentialKeyrequires exactlyprefix + 64 + suffix. Correct today (both ids are SHA-256 hex), but it means a future id format silently un-reserves the key rather than failing loudly. Worth a comment tying the64toConnectionWorkspace::accountId/connectionCredentialId.src/core/backends/icom/IcomCivBackend_DualReceiver.cpp:116-120—mod->dataOffInputItemdereferences the optional returned bycurrentModulationProfile()unchecked. Safe today (both new profiles setmodulation), but a future profile withdualReceiverand nomodulationcrashes here rather than degrading.src/core/backends/icom/IcomCivBackend_DualReceiver.cpp:109— the scope reference read is queued under the literal key"scope.reference." + N, whilesemanticKey()(IcomCivBackend.cpp:4301-4304) would compute"scope.<sub>.<data0>"for the same frame. Two keys for one request means no coalescing between them if another site ever queues it.docs/BUILDING.md:41-43— theREQUIRE_KEYCHAIN=ONguidance is unrelated to either issue. Not wrong (the option exists atCMakeLists.txt:34), just undisclosed.docs/icom-dual-receiver-bringup.md:30says "Center mode is centre ± half-span", which reads ascentre ± span/2; the code atIcomScope.cpp:95-99doescentre ∓ spanbecause Icom's reported span is the half-width. Both are defensible readings of the sentence — worth one clarifying word.src/core/backends/icom/IcomCivBackend.cpp:1462-1466also fixes the IC-785x (689 points, unverified) by the same mechanism. That is almost certainly an improvement and it goes unmentioned in the body.onDualAudioreturns without output ifstreams[0].size() != streams[1].size(). Two independentResamplers can transiently disagree after construction; a dropped frame is the right call, but it is silent.
Socket-test disclosure: none. I grepped the added and modified tests (connection_workspace_test, icom_dual_receiver_test, unified_title_bar_test, startup_autoconnect_lockout_test, backend_family_switch_test, connection_panel_size_test, icom_credentials_singleflight_test) for QTcpServer/QUdpSocket/QLocalServer/QWebSocketServer/connectToHost/listen(/bind( — zero hits. icom_dual_receiver_test reaches the production backend through friend test-access structs with an unstarted IcomSession and a m_testWriter capture hook; no socket is opened. Going the other way, connection_panel_source_pick_test removed its SKIP_RETURN_CODE 77 host dependence by injecting a fixed m_manualSourceCandidates snapshot instead of enumerating the host's real interfaces — that is the right direction and not a finding.
CodeGuard: every listed hit is noise on this change and I'm dropping all of them — backticks inside the JSON note strings in aetherd-touchpoint-tags.json, &&/; as C++ operators in AppSettings.cpp/AutomationServer.cpp, and the 192.0.2.x RFC-5737 documentation addresses plus the example email in screens.json. None is a shell invocation, an outbound request, or real PII. A security-audit subagent pass was launched over the credential, deserialization and audio/scope-indexing paths; it had not reported back when I posted, so treat the credential and bounds review below as mine rather than its.
What I tried to break
- 0x29 wrapper round-trip.
receiverCommand()(IcomCivBackend_DualReceiver.cpp:31-60) rewrites whitelisted MAIN level/meter/function reads into29 00 <cmd> <sub> …, andhandleDualReceiverFramereturnsfalsefor29 00replies. If nothing unwrapped them, every MAIN readback on these models would be dropped and the scheduler would time out on all of them. It does unwrap, atIcomCivBackend.cpp:2759-2764, rebuilding the inner frame before the registerswitch. Survived. - MAIN/SUB request coalescing. If
semanticKey()keyed only on command,25 00and25 01would collapse into one queue entry and SUB frequency would never be read.IcomCivBackend.cpp:4288-4299tags the receiver sub into the key for 0x25/0x26/0x29, including the wrapped inner cmd+sub for 0x29. The body's "MAIN and SUB use separate semantic generations" claim is real. Survived. - Monitor-mix gain inversion.
monitorScalecomputes2.0 * balanceand2.0 * (1.0 - balance)frommonitorPan / 100.0. If pan were the ±100 convention, one channel's scale would go negative and invert phase.monitorPandefaults to 50 (IcomCivBackend.h:364) andSliceAudioRequest::valid()(src/core/backends/ReceiveCommand.h:94-100) bounds it to 0..100, sobalance ∈ [0,1]and both scales stay in[0,1]after thestd::min. Survived. - Transceive-push amplification.
IcomCivBackend.cpp:2515-2519fires a fullpollDualReceivers()(≈12 reads, four of them SET-menu) on every inbound00/01/03/04frame. With CI-V Transceive on, a spun VFO knob pushes those continuously. I expected an unbounded queue;queueReadpassescoalescethrough to the scheduler and the keys are stable, so outstanding work is bounded at ~12 entries. Worth confirming you measured it, but I could not construct a flood from the code. - CI-V stall recovery. That same early
returnprecedesrecoveryFrequencyCandidate's consumption atIcomCivBackend.cpp:2660, so I expected the dual path to strand an in-flight recovery. It cannot: recovery requiresIcomFeature::CivDataRestart(IcomCivBackend.cpp:8350), andkDualReceiverEvidencedeclares only Core/Scope/VfoMode/ModulationInput/AntennaTuner — so these models never enter recovery and keepmain's warn-only stall behavior. Survived. The samereturnalso skips them_lastInboundCivAtMs = frameAtMsat line 2634, but the reads it queues refresh the clock viahandleDualReceiverFrame, so a genuine silence still stalls correctly. - SET-menu item numbering against the docs.
ModulationProfile{-1, -1, 90, 91, 92, 0x05, …}unpacks tonetworkLevelItem=90, dataOffInputItem=91, dataInputItem=92andDualReceiverProfile{86, {92,93,94}}, matching the bring-up doc's "LAN=05, DATA OFF item 91, D1/D2/D3 items 92/93/94" exactly; the IC-7760's 128/129/130 + {130,131,132} matches its row too. No off-by-one despite the overlapping ranges. Survived. - Credentials reaching the settings store.
ConnectionWorkspace::validaterejects any document containing a key matchingpassword/token/secretat any depth (ConnectionWorkspace.cpp:26-39), which also meansworkspaceSnapshot()— returned verbatim to theconnect profilesbridge verb — cannot carry one by construction.AppSettings::extractCredentialsstrips scoped keys into the session vault before anything persists. The one thing that does reach both the store and the bridge is the imported Icom networkusername(ConnectionWorkspace.cpp:245); low sensitivity over aQLocalSocket, but you should know it's there. - Observe-only gating on the new verbs.
connect profilesis whitelisted read-only (AutomationServer.cpp:2777-2779); the mutatingconnect profile stations|operators <id>is not, so it is refused in observe-only — matchingdocs/automation-bridge.md. It is also handled before theisConnected()guard, so it works while a radio is live, also as documented. Survived. - Scope decoder bounds.
m_partial.raw.size() + frame.data.size() - kOffContinuationatIcomScope.cpp:152underflows if a continuation frame is shorter than 3 bytes — but the result is a huge value that trips the> m_geom.pointsreject, so it fails safe rather than over-reading. Everym_receivers[...]index inhandleDualReceiverFrameis guarded by a precedingdata[0] > 1/sub > 1check. - Could not check: anything requiring a build or execution. I never compiled, never ran a test, and never drove the GUI — every finding above is read off the source at the cited line, not reproduced at runtime. I also cannot verify the IC-7610 A7380-7EX-4 or IC-7760 A7788-8EX-2 guide figures (689 bins, 0–200, 15-byte header, SET items 86/90-94 and 123/128-132), the
rr-addressed29whitelist, or the MAIN-left/SUB-right channel assumption — those rest on documents only the author holds, and the PR is appropriately explicit that they are guide-derived rather than hardware-qualified. CI is green on all 11 checks for543e8dc, butbuild/check-macos/check-windowsrun-R-filteredctestsubsets, so green here does not mean the ~240-test suite passed.
Recommendation
Needs maintainer decision. The Icom diagnosis is the best part of this PR and I'd like to see it land soon — the 475-vs-689 geometry bug is real, cheaply confirmed, and explains five open reports. But it arrives wrapped around two other open PRs (Blocker 1), and that is an ordering question only a maintainer can settle.
Independent of that ordering, three things want fixing on this branch: the m_lanOutput == -1 silent audio drop (Blocker 2), the shared-decoder strictness change and the test assertion that was rewritten around it (Blocker 3), and the five Closes: lines that the PR's own documentation says not to close offline (Blocker 4). Blocker 4 is a two-minute edit to the body. Concrete next step: get the maintainer's ruling on sequencing, fix 2 and 3 in the Icom commits either way, and reduce the closure list to #6910.
Thanks for the unusually careful evidence discipline here — separating guide evidence from hardware evidence in FeatureEvidence, labelling the tab-strip screenshots as offline fixtures rather than hardware, and writing the "do not close these from offline tests" rule into the doc. That last one is the right instinct; the Closes: list just needs to catch up with it.
🤖 aethersdr-agent · cost: $20.3686 · model: claude-opus-5
|
Cisco CodeGuard — static analysis of this PR (483 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: $20.7900 · model: claude-opus-5 |
Closes #6910.
Closes #5714.
Closes #5341.
Closes #5154.
Closes #5096.
Closes #5084.
Closes #6920.
Adds an initial Ethernet/RS-BA1 bring-up path for the IC-7610 and IC-7760: two fixed MAIN/SUB receive slices and panadapters, native scope geometry, separate receive audio feeds plus a monitor mix, and one shared PTT/mono transmit-audio path. Both models are marked experimental.
Integrates the connection-dialog changes from #6885 and #6889.
Related issues
IC-7610 device reports:
IC-7760 device reports and support evidence:
Related Icom bring-up, control and receiver-presentation context:
The links include historical reports and shared Icom context.
What changes
19 00supplies the canonical model independently of the configurable bus address and network nickname. That identity reaches the status-bar model label through the existing RadioModel information update.25 rr/26 rraddressing, retain DATA OFF/D1/D2/D3 and FIL1/FIL2/FIL3, and qualify command-29 controls by the exact model guides. Global TX controls remain unwrapped. Editing SUB does not change the radio's TX route.1C 03. Key-on waits for receiver mode/frequency and split readbacks. The active TX receiver's DATA variant chooses the appropriate model-specific modulation register.Protocol authority: the operator's Icom oracle §§13.2–13.6/13.13 and the official IC-7610 A7380-7EX-4 and IC-7760 A7788-8EX-2 CI-V guides. Profiles record official-guide evidence, not live-hardware qualification. The IC-9700 retains its existing single selectable scope.
Validation
Screenshots
The dialog/list are live automation-bridge captures from the integrated offline app. The tab strips are explicitly offline TitleBar fixtures; they are not hardware evidence.