fix(RadioModel,NetworkDiag): remove Qt::UniqueConnection from adaptive-throttle lambda slots#3235
Conversation
701d767 to
98b168a
Compare
|
Squashed to a single signed commit (98b168a). Tested on Windows with autothrottle on and off — build clean, no assert dialogs, manifest verified via |
|
@AetherClaude thoughts on this? |
|
Looked at the diff. The three documented fixes themselves are sound — but the patch as posted does more than the description says, and that part needs to come out before merge.
|
|
@AetherClaude appropriate tag added. |
|
Label received — the Bot review holds upDFNR-embedding regression is real.
And the Three documented fixes still apply. Both lambda-with-
For reference, the Plan for the orchestratorRebase on current
Test plan to mirror in the new PR description is exactly the one already on this issue (autothrottle on/off × single/dual slice, plus 73, Jeremy KK7GWY & Claude (AI dev partner) 🤖 aethersdr-agent · cost: $3.9889 · model: claude-opus-4-7 |
There was a problem hiding this comment.
Thanks for the careful write-up @chibondking — the three stated fixes are well-reasoned and individually look correct. There's one significant scope issue I'd like to flag before this lands, though.
Likely-unintended revert of #3225 in CMakeLists.txt
The diff against CMakeLists.txt removes ~50 lines that aren't mentioned anywhere in the PR description. Specifically, this PR undoes the DFNR-model-embedding mechanism that was merged in 44eaef7c ("[windows] Embed DFNR model and set Store identity", #3225) ~13 hours before this PR opened:
- Deletes the
AETHER_EMBED_DFNR_MODEL_DEFAULT/AETHER_EMBED_DFNR_MODELoption block (around line 32) - Deletes the
dfnr_model.qrcgeneration +qt_add_resources(DFNR_RESOURCES …)block - Drops
DFNR_RESOURCESfromadd_executable(AetherSDR …) - Drops the
AND NOT AETHER_EMBED_DFNR_MODELguards on the loose-model install rules
That feature is load-bearing for the Windows Store MSIX path. My read is this is a rebase accident — the branch was likely cut before #3225 landed and didn't pick up cleanly. Could you rebase on current main and force-push, keeping only the three intended changes in CMakeLists.txt? The only intentional edit to that file should be the target_link_options block under if(MSVC).
The three intended fixes themselves — LGTM
Manifest pipeline (CMakeLists.txt MSVC block + AetherSDR.rc): Switching from /MANIFESTINPUT to /MANIFEST:NO + an RT_MANIFEST resource is the right escape hatch for the CMake vs_link_exe ↔ MSVC 14.50 mismatch. The inline comment explaining the reasoning is great.
Qt::UniqueConnection removals (NetworkDiagnosticsDialog.cpp:2416, RadioModel.cpp:3440): Both are correct.
NetworkDiagnosticsHistoryctor runs once, so the flag was always a no-op.- For
ensureOwnedPanadapter, I verified the early-return guard claim —RadioModel.cpp:3412returnsexistingbefore reaching theconnect, so the connection is genuinely made at most once per pan;UniqueConnectionwas both meaningless on a lambda and redundant here.
Once the DFNR-related CMake hunks are dropped, this should be ready to go.
🤖 aethersdr-agent · cost: $7.1866 · model: claude-opus-4-7
There was a problem hiding this comment.
Thanks @chibondking — the three named fixes look correct, but the CMakeLists.txt diff is materially larger than the PR description claims, and the unmentioned portion looks like it will break Windows Store packaging.
On the documented fixes — looks good
-
Manifest pipeline (
CMakeLists.txt+AetherSDR.rc): switching from/MANIFESTINPUTto/MANIFEST:NO+ a1 24 "AetherSDR.exe.manifest"RT_MANIFEST resource is a clean, well-justified workaround for the CMakevs_link_exe/ MSVC 14.50 mismatch. Comment inCMakeLists.txtexplains the rationale clearly. -
NetworkDiagnosticsHistoryctor (NetworkDiagnosticsDialog.cpp:2416): removingQt::UniqueConnectionfrom the lambda is correct — Qt has no way to deduplicate lambda closures (no stable address), and the connect runs once per ctor, so the flag was both invalid and redundant. -
ensureOwnedPanadapterdeferred-cap lambda (RadioModel.cpp:3440): same reasoning, and the surroundingif (auto* existing = …) return existingearly-return +if (activeCap > 0)guard already make this a once-per-pan connect.
Concern — undocumented scope creep on CMakeLists.txt
The PR body lists only Replace /MANIFESTINPUT with /MANIFEST:NO under if(MSVC) guard. But the actual diff also reverts the DFNR model embed feature merged 1 commit ago in PR #3225:
- Removes the
AETHER_EMBED_DFNR_MODELoption + Windows-default block (CMakeLists.txt:32-39 onmain) - Removes the
dfnr_model.qrcgeneration +qt_add_resources(DFNR_RESOURCES ...)block (~20 lines) - Drops
${DFNR_RESOURCES}fromadd_executable(AetherSDR ...) - Removes the
AND NOT AETHER_EMBED_DFNR_MODELguards from the install/copy paths
This change isn't mentioned in the PR description or commits, and it has downstream consequences:
src/core/DeepFilterFilter.cpp:22still references:/models/DeepFilterNet3_onnx.dfmodeland tries to load it as a fallback. After this revert, that resource will never be populated on any build, so the embedded-model branch silently dies..github/workflows/windows-installer.yml:173invokescreate-msix.ps1 ... -ExcludeDfnrModel, which assumes the model is embedded. With this revert, MSIX packages produced by CI will be missing the model entirely — neither in resources nor as a loose file.docs/WINDOWS-STORE-MSIX.mddocumentsAETHER_EMBED_DFNR_MODEL=ONas the supported Store-build path. WACK previously flagged the loose.tar.gz(Archive files usage), which is exactly why #3225 embedded it; reverting reintroduces that failure on the next Store submission.
Suggested fix
Could you either:
- Drop the DFNR revert from this PR and limit the CMakeLists.txt change to the documented manifest line (the manifest fix and the UniqueConnection fixes are independent of the DFNR machinery and stand on their own); or
- If the DFNR embed feature is genuinely broken on your local MSVC 14.50 toolchain (e.g., the QRC file path or
qt_add_resourcesinteracts badly with the new manifest pipeline), call that out explicitly in the PR description, coordinate with @jensenpat (PR #3225 author) on the Store-build flow, and updatewindows-installer.yml+WINDOWS-STORE-MSIX.mdso CI doesn't silently regress.
Option 1 is the lower-risk path given this PR's "demonstrated fix" framing — keep the scope tight, ship the three real fixes, file the DFNR question separately if the embed pipeline is also broken under VS 2022 17.10.
🤖 aethersdr-agent · cost: $7.3091 · model: claude-opus-4-7
jensenpat
left a comment
There was a problem hiding this comment.
Please ask your agent to rebase this to the latest main.
98b168a to
a8bc6ed
Compare
|
Good catch @aethersdr-agent and @jensenpat — the prior squash was built on a local Rebased cleanly onto current |
|
Rebase looks clean — pulled the PR diff post-force-push (
All of #3225's DFNR machinery — Quick sanity sweep on the other
|
…e-throttle lambda slots — Principle XI.
Both connects pass Qt::UniqueConnection to a lambda slot. Qt cannot
deduplicate lambda closures (no stable address to compare) and asserts
immediately at runtime:
ASSERT failure in "QObject::connect: Unique connection requires the
slot to be a pointer to a member function of a QObject subclass."
Qt6Core.dll qobject.h:269
Qt::UniqueConnection is only valid with &ClassName::memberFunction
(member function pointer) syntax, not with lambdas.
─── Site 1 — RadioModel::ensureOwnedPanadapter (RadioModel.cpp:3440) ────────
The adaptive-throttle deferred-cap lambda asserts whenever a second slice
is added while autothrottle is actively capping fps.
The function already guards against re-entry at RadioModel.cpp:3412:
if (auto* existing = m_panadapters.value(normalizedPanId, nullptr))
return existing;
A given pan pointer is freshly constructed inside this call; the connection
is made at most once per PanadapterModel instance. Qt::UniqueConnection
was both invalid (lambda) and redundant (early-return guarantee).
─── Site 2 — NetworkDiagnosticsHistory ctor (NetworkDiagnosticsDialog.cpp:2416) ─
Same issue. The connection is made exactly once in the constructor, so
the flag was always a no-op in addition to being invalid with a lambda.
─── Verification (Principle XI) ─────────────────────────────────────────────
Tested on Windows with autothrottle off (single slice), autothrottle on
(single slice), and autothrottle on with a second slice added mid-session.
No assert dialog in any scenario. LNK1220 is handled separately by aethersdr#3237.
a8bc6ed to
8b78e0f
Compare
|
Rebased onto current |
|
Hey @jensenpat — @chibondking did the rebase you asked for; current head 73, Jeremy KK7GWY & Claude (AI dev partner) |
Summary
Two
Qt::UniqueConnection+ lambda assert sites introduced by the adaptive throttle feature. Both assert at runtime with:Qt::UniqueConnectionis only valid with&ClassName::memberFunctionpointer syntax — Qt has no way to compare lambda closures for deduplication.Fixes
1 —
RadioModel::ensureOwnedPanadapter(RadioModel.cpp:3440)Asserts whenever a second slice is added while autothrottle is actively capping fps. The function already has an early-return guard at line 3412 (
if (auto* existing = …) return existing) that ensures the connection is made at most once per pan —Qt::UniqueConnectionwas both invalid with a lambda and redundant.2 —
NetworkDiagnosticsHistoryconstructor (NetworkDiagnosticsDialog.cpp:2416)Same issue. The connection is made exactly once in the constructor so the flag was always a no-op in addition to being invalid.
Files changed
src/models/RadioModel.cppQt::UniqueConnectionfrom deferred-cap lambda inensureOwnedPanadaptersrc/gui/NetworkDiagnosticsDialog.cppQt::UniqueConnectionfromadaptiveThrottleChangedlambda inNetworkDiagnosticsHistoryctorTest plan
Constitution
Principle XI — Fixes Are Demonstrated: both assert sites verified absent on Windows under autothrottle on and off before this commit was staged.
🤖 Generated with Claude Code