Repository navigation
fix(audio): keep the saved device while it is absent. Principle VIII. - #6378
Conversation
The saved PC audio device (AudioOutputDeviceId / AudioInputDeviceId) was also the live device, so every writer of the live device rewrote the preference. A saved device that was missing at launch left the engine on the system default with a null selection; the next device-list change then reset to the default through AudioEngine::setOutputDevice() / setInputDevice(), which wrote the empty id and saved it. A device that dropped out mid-session was forgotten the same way (aethersdr#6040). The setters now take an AudioDeviceSelection. Only OperatorChoice (the default: Radio Setup, the accepted hot-plug prompt) writes the saved id; MainWindow's reset path passes Fallback, which re-points the engine for this run and leaves the saved id alone. handleAudioDeviceListChanged() takes its decision from a pure policy, core/AudioDevicePreferencePolicy.h. When the saved device is among the newly added ids and the engine sits on the system default, the engine returns to it, without a prompt and without a write. A live device that is present is never swapped. The prompt condition is unchanged; when it does show, a returning saved device is offered as the current one. audio_device_preference_policy_test pins the rule over made-up ids: missing at launch then connected, a dropout and return, removal while a different device is selected, an explicit choice, and the prompt condition. With the policy mutated back to the old behaviour (every selection persisted; no restore on arrival) it fails. Not covered: no real device was unplugged, and AudioEngine's setters and the MainWindow handler are reached by no test. Refs aethersdr#6040 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
![aethersdr-agent[bot]](https://avatars.githubusercontent.com/in/3286087?s=60&v=4){kind=small}
There was a problem hiding this comment.
Reviewed at head 941e2100. I read the issue and the triage comment, then went after the policy, the rewritten handler, and every path that can still write the two saved ids. The fix is sound and the body is unusually honest about its own limits — including the one thing it correctly says it cannot decide for itself. Findings below, all reasoned from code: I have no build and no way to drive the GUI here, so nothing was reproduced at runtime.
1. Issue fit — yes, and it says where it stops
#6040 asks for two things, and the triage comment split them: (a) stop the fallback path persisting, and (b) return to the saved device when it comes back. The PR does both, and the body flags (b) as the maintainer's call — which matches the reporter ("If automatically returning to a device counts as a UX behaviour change under GOVERNANCE.md, the first bullet alone still fixes the lost setting") and the triage's own recommendation (silent restore).
Mapping the issue's three mechanism steps to the diff:
| issue step | addressed by |
|---|---|
| 1 — launch keeps the preference on disk, drops it in memory | not changed, by design; the settings row stays the only holder (body discloses this) |
| 2 — a null live selection makes the next device event a reset | src/core/AudioDevicePreferencePolicy.h:52-66, via liveMissing/followsDefault |
| 3 — the reset is persisted as if it were a choice | src/core/AudioEngine.cpp:9941-9945, :9960-9964, gated on persistsAudioDeviceSelection() |
Triage item (c), "pull the decision out into a pure function so it can be tested", is done at exactly the seam the triage named, including the filename.
Coverage. The policy is covered by a socket-free CTest — the right layer for a decision policy per AGENTS.md. The test is registered (tests/tests.cmake:3098-3105) and links Qt6::Core only, so it runs on every runner including the Windows and macOS lanes. It discriminates: a decideAudioDeviceChange stubbed to always return Keep fails both "a removal while on the default restarts on the default" and "saved device arrives while on the default -> restore it". The fail-before story holds up for the right reason — persistsAudioDeviceSelection is constexpr, so the two static_asserts genuinely cannot compile against the old always-persist setters.
Socket tests: none added, none removed, none modified. The new target opens no socket and binds nothing.
2. Scope
Everything in the diff is explained by the issue thread.
| file(s) | what it changes | claimed? | verdict |
|---|---|---|---|
src/core/AudioDevicePreferencePolicy.h |
new header-only policy over device ids | yes — triage (c) | in scope |
src/core/AudioEngine.{h,cpp} |
AudioDeviceSelection param; persist only on OperatorChoice |
yes — triage (a) | in scope |
src/gui/MainWindow.{h,cpp} |
handler rewritten onto the policy; restorePreferredAudioDevices(); audioDevicePresent → audioDeviceById |
yes — triage (a) + (b) | in scope |
tests/audio_device_preference_policy_test.cpp, tests/tests.cmake |
the new target | yes — triage (c) | in scope |
docs/architecture/aetherd-touchpoint{s.md,-tags.json} |
one new ui-support row, totals 252 → 253 |
yes, and disclosed as a new gui/→core/ include |
in scope |
No build-config, CI, vendored or formatting churn. No new protocol verb, wire message, settings key, CLI flag or capability field — the two keys it touches (AudioInputDeviceId, AudioOutputDeviceId) are pre-existing app-global flat keys, which is the correct shape for a host audio device (not per-radio state), so Principle V and the AGENTS.md persistence rules are not in play. The new test needs no TestSettingsProfile.h because it never touches AppSettings. I ran tools/gen_touchpoint_manifest.py --check against the head checkout: "manifest up to date", and the 253 table rows match the header.
Deleted behaviour, read as carefully as the additions: the only removals are audioDevicePresent() (fully gone, no dangling references anywhere in src/ or tests/) and the if (resetInput || resetOutput) guards at the three applyFallback() sites. Those guards moved rather than vanished — resetMissingAudioDevicesToDefault already carries if (!m_audio || (!resetInput && !resetOutput)) return; at src/gui/MainWindow.cpp:5451, and restorePreferredAudioDevices has the matching both-null guard at :5481. No comment naming a fixed symptom was deleted; the #2864 comment was rewritten into the policy header at :70-71 and its rule is preserved.
3. Blockers
None.
4. Needs maintainer decision
(b) silent auto-restore is a user-visible behaviour change, and only a maintainer can ratify it. src/core/AudioDevicePreferencePolicy.h:52-56:
if (followsDefault && !change.preferredId.isEmpty()
&& change.addedIds.contains(change.preferredId)) {
decision.action = AudioDeviceAction::RestorePreferred;I am naming this rather than treating it as smuggled preference, because it is not: the reporter raised it, the triage recommended exactly this (silent, not prompted, on the grounds that the operator already expressed the preference and #2864 deliberately avoids prompting on arrivals), and the PR body puts it in its own bullet with a clean fallback — drop the RestorePreferred branch and (a) stands alone, the saved id survives, and the device is selected again at the next launch. Per GOVERNANCE.md that is the ruling the maintainer owes; the implementation is ready either way.
Two smaller user-visible consequences of (b) that are worth ruling on in the same breath, since neither is in the issue:
src/gui/MainWindow.cpp:5396-5397— when the hot-plug prompt does appear, it now preselects the returning saved device as "current" instead of the engine's actual device:Defensible (it is what the operator wanted), but it changes what the dialog asserts is in use at the moment it opens.restoreInput.isNull() ? currentInput : restoreInput, restoreOutput.isNull() ? currentOutput : restoreOutput,src/gui/MainWindow.cpp:5383-5388— with "Prompt on Audio Device Changes" switched off, the restore now happens silently where previously only a reset did. Also defensible, also new.
5. Nits (non-blocking)
-
src/gui/MainWindow.cpp:5346-5351— the policy input is built by positional brace-init of a six-field struct.const AudioDeviceDecision inputDecision = decideAudioDeviceChange({ currentInputIds, addedInputIds, !removedInputIds.isEmpty(), defaultInputChanged, currentInput.id(), preferredInputId});
The ordering is correct today, but
anyRemoved/defaultChangedare adjacentbools andliveId/preferredIdare adjacentQByteArrays. Swapping either pair compiles silently, and swapping the last pair would invert the whole fix — the engine's device would be treated as the preference. Designated initializers make it unswappable at no cost:const AudioDeviceDecision inputDecision = decideAudioDeviceChange({ .presentIds = currentInputIds, .addedIds = addedInputIds, .anyRemoved = !removedInputIds.isEmpty(), .defaultChanged = defaultInputChanged, .liveId = currentInput.id(), .preferredId = preferredInputId});
-
The disclosed hot-plug-accept hole is real, and it is the same defect in miniature.
src/gui/MainWindow.cpp:5405-5417: accepting the prompt routes toapplyAudioDeviceSelection(), which persists both directions with the defaultOperatorChoice. The reachable case is concrete — the live output is unplugged while a new input arrives, sooutput.liveMissingis true and the prompt fires; the output combo is filled fromoutputDevices(present devices only), so it cannot offer the now-absent saved device and shows the default; the operator accepts for the input and the saved output id is overwritten with"". That is #6040's symptom through the one door left open. Arguably the operator did look at a dialog and accept, which is why I am not calling it a blocker — but it means "Fixes #6040" is not quite complete, and it is the natural follow-up issue. -
The body's Radio Setup sentence is imprecise. It says "Radio Setup still shows its own selection while the engine is on the fallback".
src/gui/RadioSetupDialog.cpp:5034-5040substitutesQMediaDevices::defaultAudioOutput()for a null engine device, andrepopulateAudioDeviceCombo(:4698-4723) only falls back to the combo's previous id when the passed device's id is empty — which it no longer is after that substitution. So the pane names the fallback device, not its own prior selection and not the saved preference. Triage corollary (c) — no in-memory record of the preference for the dialogs to display — is genuinely still open, which the body does say elsewhere; just the one sentence reads the wrong way. -
The handler's wiring has no test, and the test's harness re-implements it.
Session::devicesChangeintests/audio_device_preference_policy_test.cpp:62-92mirrors whathandleAudioDeviceListChangeddoes rather than calling it, so a mis-wiring inMainWindow.cppwould leave all 28 checks green. The body states this plainly ("their wiring was read, built and not exercised"), and I am not asking for more: the handler is entangled withQMediaDevicesand there is no deterministic policy-compliant seam for it, so this is the honest coverage boundary, not a gap to fill.
6. Security audit
Required by the harness (59 CodeGuard findings across seven rule families). I ran the security-audit subagent over the head checkout: no findings. Every hit is a lexical false positive or sits on untouched lines, and I confirmed the dispositions that matter:
CS-VLN-CMDI-AMP/CS-VLN-CMDI-BACKTICK— the C++&&operator and Markdown backticks in comment prose (e.g.src/gui/MainWindow.cpp:589,Null when \id` is empty or not in `devices`). NoQProcess`, no shell, no command string anywhere in the diff.CS-VLN-SSRF-HEX— the flaggedAudioEngine.cpp:9807-9808constants are pre-existing VITA-49 OUI/ICC values well outside the changed region (:9937-9977). No URL or host is built from hex.CS-VLN-PATH-PROC/CG-PATH-001— the flaggedMainWindow.cpplines (6262, 6341, 9739, 9851, 9973-9975) are all pre-existing and unrelated to this change; the diff constructs no filesystem path.CS-PII-IP—MainWindow.cpp:6954, the pre-existing WSJT-X multicast group224.0.0.1.CS-INJ-homoglyph— U+2500 box-drawing comment banners, matching house style across the tree.
On the substance: the saved id is read as a QByteArray at src/gui/MainWindow.cpp:5342-5345 and used only for QList::contains() and an id-equality lookup against OS-enumerated devices. It is compared, never interpreted, executed, or used as a path; a tampered settings row can at most name a device that is actually enumerable. The change strictly reduces settings writes, and adds no logging — the dev.description() qCDebug lines at AudioEngine.cpp:9940/:9960 are pre-existing.
7. What I tried to break
- Prompt-condition equivalence. The old gate was
deviceAdded && !currentSelectionStillValid, whereaudioDevicePresent()returnedtruefor a null target — i.e. prompt iff a device arrived and a non-null live device is absent. The newliveMissing = !followsDefault && !presentIds.contains(liveId)is the same predicate, andaudioDeviceChoiceRequired(AudioDevicePreferencePolicy.h:72-78) composes it the same way. I could not construct an input where they diverge. LikewiseresetInput/resetOutput:RestorePreferredrequiresfollowsDefault, so it can only ever preempt the olddefaultNeedsRestartdisjunct and never theliveMissingone — the only behaviour delta is the intended (b). - Can the engine ever sit on a device the saved id does not name, so a restore swaps something live out from under the operator? No. Live diverges from saved only via a
Fallback, andFallbackonly ever writesQAudioDevice{}or the saved device itself; everyOperatorChoicepath updates both. AndRestorePreferredis gated onfollowsDefault(emptyliveId), so a present live device is never swapped — the test pins this at:166-171. - Can
audioDeviceByIdreturn null on aRestorePreferreddecision and silently drop the restore? No:addedIdsis derived from the same enumerated list bynewlyAddedAudioDeviceIds, soaddedIds ⊆ presentIdsby construction, and the policy already requiredaddedIds.contains(preferredId). If it somehow did, the result is a benign no-op (that direction stays on the default) rather than a crash, becauseresetInputis false on that branch. - Sibling call sites of the changed setters. All four remaining are intentional persist paths:
RadioSetupDialog.cpp:5105/:5107(the combo'sapplyChoice) andMainWindow.cpp:5440/:5441(applyAudioDeviceSelection).AudioOutputRouter.h:43'sguarded->setOutputDevice(dev)is a template over the registered followers, and the only two arem_finalMonitor(ClientPuduMonitor*) andm_qsoRecorder(QsoRecorder*) — notAudioEngine, so no fallback leaks through it as anOperatorChoice. No string-basedSLOT()orinvokeMethod(obj, "setOutputDevice", …)exists, so the added defaulted parameter cannot break a signature match at runtime. - Does a reseed re-enter the persisting setter? This was my best candidate for a regression: the
Fallbackarm stillemitsoutputDeviceChanged/inputDeviceChanged, andRadioSetupDialogis connected to both (:5078-5082) and reseeds the combos. IfsetCurrentIndexfiredcurrentIndexChanged,applyChoicewould persist the fallback as an operator choice and reintroduce #6040 whenever Radio Setup is open. It does not:repopulateAudioDeviceComboholds aQSignalBlocker(RadioSetupDialog.cpp:4713). Clean. - Do the external playback sinks follow a restore? Yes —
MainWindow.cpp:1364-1366drivesAudioOutputRouter::setCurrentDeviceoffoutputDeviceChanged, which both setter arms still emit, so the post-DSP monitor and QSO playback track aRestorePreferredthe same as a reset. - Lifecycle and reentrancy.
applyFallbackcaptures by reference ([&]) but is only ever invoked synchronously within the same frame, including on the post-dialog.exec()reject branch — no dangling capture. Both helpers re-guardm_audiowith aQPointerinside the queued lambda.FollowDefaultandRestorePreferredare mutually exclusive per direction, so the two queued invocations can never contend for one direction, andstartTxStreamstays behind!audio->isTxStreaming()in both. Them_audioDeviceDialogOpenearly return happens beforem_knownAudio*Idsis updated, so a device arriving while the dialog is open is still seen as "added" on the retry — the restore is not lost.m_knownAudio*Idsis seeded from the real enumeration at:5277-5280, so the first change after launch does not report every device as new. - Distrusting green. All five checks on
941e2100aresuccess(build,check-macos,check-windows,Static checks,Sanitizer option configures) against basea4a39e75, which is currentmain— not a stale merge base.check-windowsis the-DENABLE_ASR=OFFlane and it passed, so the new header is not config-scoped. Note that this proves compilation and the-R-filtered subset, not the new test:audio_device_preference_policy_testruns only in the full Ubuntu lane. - Could not check: whether an OS re-enumerates a returning Bluetooth or USB device under the id it had when saved (the whole restore hinges on it; the body flags this too, and the failure mode is benign — nothing restores and the saved id simply stays), and any on-screen behaviour of the Radio Setup combo or the hot-plug prompt.
8. Recommendation
Needs maintainer decision — on part (b) only.
Part (a) is a clean, contained fix for a confirmed root cause, lands at exactly the seam the triage specified, and is covered by a discriminating socket-free test. I found no correctness defect in it. Part (b) is well-implemented and is what the triage recommended, but it is a user-visible behaviour change that both the reporter and the author routed to a maintainer, so it needs a ruling rather than a reviewer's nod. Concrete next step: rule on silent auto-restore. If yes, this merges as-is (the nits are all optional, and the designated-initializer one is worth taking). If no, the author's stated fallback — drop the RestorePreferred branch from the policy and the two restore* locals from the handler — leaves (a) intact and still fixes the lost setting.
Nice work on the limits section — "no real device was unplugged", the named fail-before mutations with their failure counts, and the explicit "one path still overwrites the saved id, by design" made this much faster to review honestly. Thank you.
73
🤖 aethersdr-agent · cost: $11.6178 · model: claude-opus-5
|
Cisco CodeGuard — static analysis of this PR (64 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: $11.8090 · model: claude-opus-5 |
Answers two nits on the review of aethersdr#6378. Nit 1 (MainWindow.cpp, the two decideAudioDeviceChange calls): the six-field AudioDeviceListChange was built positionally, with adjacent bools and adjacent QByteArrays that swap silently. Both calls use designated initializers (the project builds as C++20). Nit 2 (the hot-plug-accept hole): the prompt lists present devices only. With the saved device of one direction absent, that list shows "System default", and accepting the prompt for the other direction wrote "" over the saved id. acceptedAudioDeviceSelection() in the policy header decides per direction: "System default" accepted while a saved id exists and is not present is a Fallback (live device set, saved id kept). Every other accepted selection is an OperatorChoice as before: a real device, and "System default" with the saved device present or with nothing saved. applyAudioDeviceSelection takes the selection kind per direction; it has one caller, the prompt. audio_device_preference_policy_test gains five checks: the review's scenario (output unplugged as a new input arrives, accept for the input) keeps the output's saved id and saves the input; default with the saved device present clears the saved id; default with nothing saved is an operator's choice; a real device replaces the saved id. The RestorePreferred branch and the prompt's preselection are not touched. Refs aethersdr#6040 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Thanks for the review. Fixed in
Not touched, waiting for the ruling on (b): the Not tested, as before: the wiring in Full ctest at 73, ON8ST |
NF0T
left a comment
NF0T
left a comment
There was a problem hiding this comment.
Thanks @on8st: this is a well-built fix, and the limits section made it easy to review honestly. I agree with the root cause, I think part (a) is the right shape, and I like that the decision lives in a socket-free policy header with a test that discriminates. I'm requesting changes for one Linux-only regression in the glue and one docs gap. Neither touches the policy's design. Part (b) is waiting on a ruling (below), which I am not asking you to resolve.
I reviewed independently of the earlier bot review and compared afterwards. Where I disagree with it, I say so.
Blockers
1. On Linux, a selected echo-cancel / combined device is now treated as "missing"
handleAudioDeviceListChanged passes currentInputIds / currentOutputIds as the policy's presentIds. Those come from audioDeviceIds(), which drops "transient" PipeWire ids (MainWindow.cpp:527, the Q_OS_LINUX branch: .monitor, pulse_input_loopback, auto_null, echo-cancel, combined). That filter exists so churny devices aren't announced as new (#2864). It's right for addedIds, but it is not a statement about what is present.
On main, liveness was checked against the unfiltered list: audioDevicePresent(inputDevices, currentInput). Now it is !presentIds.contains(liveId) over the filtered list. So an operator whose selected PC mic or speaker has a transient-pattern id has a live device that is enumerable and in use, but liveMissing is true. The handler runs on every device-list change, so:
- the engine is re-pointed at the system default (
FollowDefault,Fallback: not persisted, so the saved id survives, but the live mic/speaker is swapped under the operator, and the saved preference and the device actually in use now silently disagree), and - if a non-transient device arrives,
audioDeviceChoiceRequirednow shows the hot-plug prompt wheremainshows nothing.
The same filtered list feeds acceptedAudioDeviceSelection, so a present transient-id preference is treated as absent there too.
This is the "input where the old and new predicates diverge" that the earlier review said it could not construct. It is invisible on macOS and Windows (the filter is the identity there), which is where the PR was built and run, and it is invisible to the test, because the test's Session models ids with no filter. The filter lives in the glue the test doesn't contain.
Observed in the running app, not just reasoned. I did not run this myself: a second session on a Linux desktop did (PipeWire 1.4.2 via its PulseAudio shim, Qt 6.12.0, demo radio only, no TX). It built the app from the PR's base (a4a39e75) and head (60f20deb), seeded AudioInputDeviceId=echo-cancel-source, and triggered device-list changes. I read its transcript and re-checked its code citations against the head. Results (live/saved input; "prompt on" = the hot-plug prompt enabled):
| build | live/saved input | trigger | prompt | hot-plug dialog | engine re-pointed to default | saved id after |
|---|---|---|---|---|---|---|
| head | echo-cancel-source |
ordinary device arrives | on | shown | (blocked behind the modal) | unchanged |
| base | echo-cancel-source |
same | on | none | no | unchanged |
| head | plain alsa_input…Mic1__source |
same | on | none | no | unchanged (control) |
| head | echo-cancel-source |
arrival | off | none | yes, to the system default | unchanged |
| base | echo-cancel-source |
arrival | off | none | no | unchanged |
| head | echo-cancel-source |
a device removed, nothing added | off | none | yes | unchanged |
| base | echo-cancel-source |
removal | off | none | no | unchanged |
The plain-device control on the same head binary is clean, which pins it on the id filter and not on anything else in the PR. The silent re-point from the app's own log (head, prompt off), about 700 ms after the change:
ENGINE TX device -> "Echo-Cancel Source"
ENGINE TX device -> "Scarlett 2i2 4th Gen Input 1 Mic/Inst/Line"
On the base the log stops at the first line. The same session also confirmed that Radio Setup offers these ids to the operator, because its combos are filled from the unfiltered QMediaDevices::audioInputs() / audioOutputs() (RadioSetupDialog.cpp:5036, :5038), so this is reachable from the UI.
Who it hits, and the limits of what was measured:
- It bites a default
module-echo-cancelsetup (idsecho-cancel-source/echo-cancel-sink) and a defaultmodule-combine-sink(combined). A hand-named module (source_name=ec_source) does not match the substring filter and is unaffected, so the filter is fragile in both directions. - The
.monitorarm of the filter never fires on this stack, since Qt's PipeWire backend doesn't list monitor sources at all. It is not part of this regression. - One machine and one audio stack. A true PulseAudio server, JACK, or a native PipeWire filter-chain were not measured.
Suggested fix: build the presentIds for both calls (and the two acceptedAudioDeviceSelection calls) from the unfiltered device lists, and keep addedIds filtered as it is. A line on AudioDeviceListChange::presentIds saying "every enumerable id, including the ones the hot-plug monitor ignores as churn" would stop the next reader repeating this. On the policy header, unfiltered ids give liveMissing=0 → Keep for the same input; I have not re-run the in-app A/B on a fixed build, so it would be worth a second look from someone with a PipeWire box once it is in.
2. User docs: the PR fixes a documented Known issue without touching docs/user/
AGENTS.md ("User documentation"): a PR that changes what a user sees or does updates the page in the same PR. docs/user/docs/audio-settings.md currently lists exactly this bug under Known issues (#6040, "…is forgotten at the next device change, so you have to select it again"). After merge that bullet is false. Please:
- remove the #6040 bullet;
- in Choosing PC audio devices / When a device is plugged in or removed, say that a saved device that is missing keeps its saved choice and the app runs on the system default meanwhile (state current behaviour, not history, per the docs style guide).
If part (b) is ruled in, add that the saved device is picked up again when it returns; if it is ruled out, leave that sentence out and say the device is selected again at next launch.
Needs a ruling (not a request to you): part (b)
Silent auto-restore is a user-visible change, and you've routed it to a maintainer, which I agree is the right call. For whoever rules, the blast radius as I read it: it affects only operators with a saved device that is currently absent; it is local audio routing only (nothing on-air changes directly); there are three visible consequences (the silent restore, the hot-plug prompt preselecting the returning device, and the restore also happening with "Prompt on Audio Device Changes" off). One more worth knowing: restoring the input goes through setInputDevice, which stops and restarts a running TX capture, so a preferred mic returning mid-over would restart capture. The same restart already happens today when the default moves under a null device (e.g. macOS promoting AirPods), so this isn't new, but it's part of what is being approved. Your fallback (drop RestorePreferred) is clean and keeps (a) intact.
Scope, preference-vs-fix, CI
| file(s) | claimed by #6040? | verdict |
|---|---|---|
AudioDevicePreferencePolicy.h, AudioEngine.{h,cpp}, MainWindow.{h,cpp} |
yes (triage a/b/c) | in scope |
audio_device_preference_policy_test.cpp, tests.cmake |
yes (triage c) | in scope |
aetherd-touchpoint{s.md,-tags.json} |
yes, disclosed new gui/→core/ include |
in scope |
docs/user/ |
should be: see blocker 2 | missing |
Preference-vs-fix: (a) is a fix. (b) is a behaviour addition the reporter explicitly raised, so I don't read it as a smuggled preference; it just needs the ruling above. No new settings key, protocol verb or CLI flag. No CHANGELOG entry (good). Commits are signed and the squash-subject convention is already followed.
CI: all five checks green against current main (a4a39e75, 0 behind). That proves compilation on three platforms and the static gates. The new test is not in the frozen per-PR allow-list, so it runs in full-suite.yml on main, not on this PR; I ran it myself (below). Constitution VIII is a fair citation: the PR's own limits section does the work of not over-claiming.
What I tried to break
Held:
- Every writer of the saved ids. Only the two
setValuesites inAudioEngine.cpp;DeviceDiagnosticsonly reads. Of the setter callers,RadioSetupDialog::applyChoiceis the only operator path besides the prompt, and it applies one direction per combo change, so changing the input combo can't overwrite an absent saved output (the combos' reseed holds aQSignalBlocker). - Stale restore / swap of a present device. A non-null live device always equals the saved one (only
Fallbackmakes them differ, andFallbackwrites null or the saved id), so there is nothing to swap under the operator. - Prompt predicate on macOS/Windows. Equivalent to
mainwhere the transient filter is a no-op. - Test. I built the PR's test standalone against Qt 6.12.0 (mingw): 34/34 pass. I then broke the policy four ways in scratch copies: dropping the
liveMissingfallback (2 checks fail), the accepted-prompt rule collapsed to always-OperatorChoice(1 fails), the prompt gate ignoring the output (2 fail), and restore without thefollowsDefaultgate (0 fail, see nit). - No sockets or peers in the new test; no auto-squash armed; PR head signed.
Broke: blocker 1, as above.
Not verified: any real device unplug/replug on any platform; the Radio Setup combo and hot-plug prompt on screen (the Linux run used an offscreen app and found the dialog through the automation bridge's tree dump); behaviour on macOS or Windows; and Linux beyond PipeWire's pulse shim. The restore path in part (b) was not exercised in-app by anyone.
Non-blocking
- The
followsDefaultgate onRestorePreferredis not pinned by any check (mutation above survives). It's also unreachable in the test's model, since a non-null live device always equals the saved one, so this is defensive code; fine to keep, but the comment that a present live device is "never swapped" isn't actually exercised. - Agree the handler wiring can't be unit-tested as it stands. Blocker 1 is an example of what that costs: the glue holds a filter the model doesn't, so a one-line comment on
presentIdsis the cheapest guard. - Separate from this PR and not yours to fix here:
isTransientAudioDeviceIdmatches substrings of a node name the operator controls, so whether it fires depends on what an echo-cancel module was called, and its.monitorarm is inert on PipeWire. Probably worth its own issue. - Taking the hot-plug-accept hole in this PR rather than a follow-up was the right call, and the disclosed trade-off (the prompt can't make "System default" the saved choice while the saved device is absent) is reasonable.
Both blockers are small. Happy to re-review quickly once they're in. Nice work on the root-cause analysis and the honesty of the limits section.
73,
Ryan NF0T
👨🏼💻 Co-authored by Claude Sonnet 5.5
Review of aethersdr#6378 (NF0T), blocker 1: on Linux the handler passed the arrival list as presentIds, and that list drops ids the hot-plug monitor ignores as churn (echo-cancel, combined, ...). A selected echo-cancel device was enumerable but reported missing: the engine was re-pointed at the system default on every device-list change, and an unrelated arrival raised the prompt. presentIds is every enumerable id again, for both decisions and for the accepted prompt; addedIds stays filtered. Blocker 2: docs/user/docs/audio-settings.md states the behaviour and no longer lists aethersdr#6040 as a known issue. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@NF0T, thank you. You are right on both, and the first one is a real regression that I introduced and could not see on macOS. Fixed in Blocker 1. Confirmed in the source:
Blocker 2. Not changed: the Full ctest at 73, ON8ST |
NF0T
left a comment
NF0T
left a comment
There was a problem hiding this comment.
Thank you @on8st: both blockers are fixed the way they needed to be, the PR body now says plainly what the first version cost, and the Linux evidence you asked for now exists and agrees. Approving.
Part (b) is accepted. The silent restore of the saved device when it returns is a user-visible behaviour, and you routed it to a reviewer rather than assuming it. My ruling: it stays as written. A prompt on device arrivals is the shape #2864 had to take back out, so silence is the house precedent; the saved id is the operator's own instruction; and it only ever acts for someone whose saved device is currently absent. Detecting a flapping device and backing off is a sensible future improvement, and deliberately not part of this PR.
What I checked on 061a62e8
Blocker 1 (Linux presence). presentIds is now enumeratedAudioDeviceIds() for both decideAudioDeviceChange calls and both acceptedAudioDeviceSelection calls. I read every use of the two id lists in the handler: the filtered audioDeviceIds() result now only feeds m_knownAudio*Ids, addedIds and removedIds, exactly as on main, and nothing that asks "is the selected device still there?" uses it. No other caller of audioDeviceIds() or isTransientAudioDeviceId() asks a presence question.
Tests. I built the policy test standalone against Qt 6.12.0: 37/37 pass. To check the new row means something, I fed it the filtered list the old glue passed ({built-in, usb} without echo-cancel-source): it fails both new live-device checks. The earlier mutations are still caught (dropped liveMissing fallback: 2 fail; accepted-prompt rule collapsed to always-OperatorChoice: 1; prompt gate ignoring the output: 2). As you say, the test pins the policy's contract and not the wiring in MainWindow.cpp, so the filtered-list mistake itself is not something a CTest can catch; the in-app run below is what covers it.
Docs. docs/user/docs/audio-settings.md: the #6040 Known-issues bullet is gone, and the passages say what the code does. Build docs site is green.
CI. All checks green on 061a62e8. It ran after #6375 merged, so it tested the current main; the one file both PRs touch (aetherd-touchpoints.md) is a different row.
In-app evidence (Linux, demo backend)
I did not run this myself. A second session ran it on a Linux desktop (PipeWire 1.4.2, Qt 6.12.0), on the demo radio only, with no TX and no FLEX contacted. It built three binaries with identical flags: this head, the PR's base a4a39e75, and the earlier 60f20deb as a positive control. I read its transcripts and checked them against the code.
Blocker 1 is fixed. With AudioInputDeviceId=echo-cancel-source selected and a default-named echo-cancel source loaded:
| case | head 061a62e8 |
base | 60f20deb |
|---|---|---|---|
| ordinary device arrives, prompt on | no dialog | no dialog | dialog shown |
| arrival, prompt off | no input re-point | no re-point | (re-pointed, per the first run) |
| a removal only, prompt off | no input re-point | no re-point | (re-pointed, per the first run) |
control: plain alsa_input selected |
nothing | n/a | n/a |
The saved id stayed echo-cancel-source in every row. The old build still shows the dialog, so the method is sensitive to the bug it is checking.
Part (a) fixes #6040, and it reproduces on base. A saved device that drops out mid-session:
| direction | head | base |
|---|---|---|
output (restore_probe) |
engine falls back, saved id kept | saved id overwritten with empty |
input (restore_src) |
engine falls back, saved id kept | saved id overwritten with empty |
Both builds log the identical engine line; only the persistence differs, which is persistsAudioDeviceSelection(Fallback) == false. A saved output that is absent at launch also keeps its id on the head across two unrelated device changes.
Part (b) restores silently. Reloading the saved device:
- Output direction, prompt off: the engine returns to it within the 5 s observation window, no dialog, saved id unchanged.
- Output direction, prompt on: also restores with no dialog.
- Input direction: restores, no dialog, saved id unchanged.
- Base: no restore (the id was already lost).
- As the PR states, a saved churn-id device (
echo-cancel-source) that returns is not restored live and is used at the next launch, confirmed by relaunching.
The TX-capture restart is real, and it is on the demo. The demo radio reports mic source PC, so pcMicInUse is true. On an input restore the log shows input device set to "restore_src source" and then TX stream started 3 ms later. That shows the branch is taken and startTxStream is called; it does not show audio reaching a real radio, which only a live FLEX would. Restoring the input therefore restarts a running capture, as I noted in my first review.
One case was left out of the evidence: an input-direction "missing at launch" variant tested nothing (its trigger produced no input event), and the session flagged it itself. The input direction is covered by the dropout and restore cases above.
Scope / process
- Files since my first review: docs, policy header, handler, test. Nothing outside the fix. No CHANGELOG entry. Commits signed.
- Constitution VIII is a fair citation: the limits section does the work of not claiming what wasn't run.
Not verified
- macOS and Windows in the app. The code path is the same, but nobody ran those builds against a real device.
- A real Bluetooth or USB unplug and replug on any platform, including whether the OS reports a returning device under the same id.
- Real mic audio reaching a real radio after a restore.
- PipeWire's native filter-chain, JACK, or a true PulseAudio server on Linux.
Forward guidance (not blockers)
- A saved device whose id matches the churn filter returns at the next launch, not silently. The user docs sentence "switches back … when that device connects" is slightly broader than that on Linux, so it may be worth a clause later.
- Moving
audioDeviceIds()/enumeratedAudioDeviceIds()into a small header would let a test pin "presence uses the unfiltered list" instead of relying on review. Not needed for this PR.
Squash
fix(audio): a saved audio device that is absent is kept, and used again when it returns. Principle VIII. (#6378)
Nice recovery on the Linux regression, and thank you for writing it into the body instead of leaving it for the next reader.
73,
Ryan NF0T
👨🏼💻 Co-authored by Claude Sonnet 5.5
…I. (#6387) Extract the host audio-device ID filtering and change construction into the existing header-only policy. Preserve unfiltered presence checks and filtered arrival/removal checks, and carry the filtered list forward for the next device check. Add socket-free coverage for Linux filtering and successive snapshots, and document the Linux virtual-device restore exception. Refs #6378. Validation: ARM64 macOS build with Qt 6.12 and RADE enabled; policy CTest 63/63; six mutation checks; 134,162 old/new list comparisons; isolated demo connect/audio/disconnect smoke test with TX disabled. Linux physical hot-plug behavior was not exercised locally.
2 participants
Fixes #6040.
Limits, first
AudioEngine's setters andMainWindow::handleAudioDeviceListChanged()are reached by no test; their wiring was read, built and not exercised.echo-cancel,combined, …), so such a selected device was treated as missing. Fixed in061a62e8: presence uses every enumerable id, arrivals stay filtered. The fixed build has not been run on Linux by me.RestorePreferredbranch and the saved id is still kept, so the device is selected again at the next launch.m_preferred*DeviceIdmembers inAudioEngine. The settings row is the only holder of the preference andMainWindowreads it there; while the engine is on the fallback, Radio Setup's device combos show the system default device it fell back to, not the pane's earlier selection and not the saved preference.gui/→core/include:MainWindow.cppincludescore/AudioDevicePreferencePolicy.h(one new touchpoint row, taggedui-support). Justification: it is the header-only policy the triage asked for, host audio only, no radio state;AudioEngine.h, whichMainWindow.cppalready includes, needs the same enum for the setter.What was wrong (at
main16cfa80)One setting per direction (
AudioOutputDeviceId/AudioInputDeviceId) was both the operator's preference and the live device, so every writer of the live device rewrote the preference.AudioEngine's constructor setsm_outputDevice/m_inputDeviceonly when the saved id is enumerable (src/core/AudioEngine.cpp:1991,:1996).startRxStream()andstartTxStream()null the member when the device is gone (:4180,:8871). The saved id is still intact here.handleAudioDeviceListChanged()(src/gui/MainWindow.cpp:5275) a null live device plus any removal or default change asks for a reset (:5317,:5319).audioDevicePresent()returns true for a null device (:590), so no prompt is needed (:5334) and the reset runs.resetMissingAudioDevicesToDefault()(:5414) callssetInputDevice(QAudioDevice{})/setOutputDevice(QAudioDevice{})(:5432,:5434), and those setters always write the id and save (src/core/AudioEngine.cpp:9944,:9963). A null device has an empty id, so the saved choice is replaced by "".A device that was present and then dropped out went through the same step 3.
The rule now
AudioDeviceSelection. OnlyOperatorChoice(the default argument: Radio Setup, the accepted hot-plug prompt) writes the saved id.Fallbackre-points the engine for this run and writes nothing.MainWindow's reset path passesFallback.decideAudioDeviceChange()insrc/core/AudioDevicePreferencePolicy.h:RestorePreferred: engine returns to the saved device, no prompt, no writeFollowDefault: engine falls back, saved id keptFollowDefault: restart on the default, as beforeKeepTests
New:
audio_device_preference_policy_test(linksQt6::Coreonly; no QtMultimedia, no audio device, no settings store, no socket). 37 checks plus twostatic_asserts (three pin that an enumerable churn-id device is present; the last five cover the accepted prompt):Fail-before. The test cannot be compiled against
main, because the seam it tests is new. Instead the policy was mutated back to the old behaviour, in a scratch copy, and the test built against it:static_assert"a fallback never rewrites the saved preference (A saved audio device that is missing at launch is forgotten at the next device change, so reconnected Bluetooth headphones are no longer selected #6040)". With the twostatic_asserts removed from a scratch copy of the test, 5 of 28 checks fail, among them "the dropout leaves the saved id" and "an unrelated removal leaves the saved id".Results on macOS with Qt 6.12.0:
audio_device_preference_policy_test: 37 of 37 checks pass061a62e8: 761 tests, 758 passed, 3 failed at-j8and again on a serial rerun. Besides the two below,vkamp_connection_testfailed with only its two known checks (Replace retained socket fixtures with injected and opt-in coverage #5254):unified_title_bar_test: the two checks "Alt+F still opens the File menu with the menu bar hidden" and "opening a menu rounds the submenus added to it since". Both fail on clean main on this machine (Unified title bar (#6198): native-platform checks still open — Windows Snap Layouts, #4557 drift, macOS M1, xcb move #6224). This PR editsMainWindow.cpp, but only the audio-device handler; no menu or title-bar code.peripheral_auth_dialog_interaction_test: "Clear code or Status is clipped at the default dialog size", which also fails on clean main on this machine (feat(peripherals): authenticate direct TGXL, PGXL, and Antenna Genius connections #6008). This PR touches no dialog file.check_engine_boundary,gen_touchpoint_manifest --check,check_test_registration,check_capability_records,check_command_plane,check_ci_test_gate,check_qsettings,check_network_timeouts,check_build_options,check_wdsp_test_links(all--strictwhere offered).User docs:
docs/user/docs/audio-settings.mddescribes the behaviour and no longer lists #6040 under Known issues. Its sentence about switching back when the device connects follows part (b) and changes if (b) is ruled out.Not verified
73, ON8ST
🤖 Generated with Claude Code