Skip to content

fix(RadioModel,NetworkDiag): remove Qt::UniqueConnection from adaptive-throttle lambda slots#3235

Merged
ten9876 merged 1 commit into
aethersdr:mainfrom
chibondking:fix/radiomodel-lambda-unique-connection
May 28, 2026
Merged

fix(RadioModel,NetworkDiag): remove Qt::UniqueConnection from adaptive-throttle lambda slots#3235
ten9876 merged 1 commit into
aethersdr:mainfrom
chibondking:fix/radiomodel-lambda-unique-connection

Conversation

@chibondking
Copy link
Copy Markdown
Collaborator

@chibondking chibondking commented May 28, 2026

Summary

Two Qt::UniqueConnection + lambda assert sites introduced by the adaptive throttle feature. Both assert at runtime with:

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 pointer syntax — Qt has no way to compare lambda closures for deduplication.

Note: the LNK1220 build failure is handled separately by #3237 (already merged). This PR covers the two runtime asserts only.

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::UniqueConnection was both invalid with a lambda and redundant.

2 — NetworkDiagnosticsHistory constructor (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

File Change
src/models/RadioModel.cpp Remove Qt::UniqueConnection from deferred-cap lambda in ensureOwnedPanadapter
src/gui/NetworkDiagnosticsDialog.cpp Remove Qt::UniqueConnection from adaptiveThrottleChanged lambda in NetworkDiagnosticsHistory ctor

Test plan

  • Autothrottle off, single slice — no assert
  • Autothrottle on, single slice — no assert
  • Autothrottle on, second slice added mid-session — no assert, both slices render at capped fps
  • Autothrottle toggled on/off mid-session — correct behavior throughout
  • CI green

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

@chibondking chibondking requested review from a team as code owners May 28, 2026 02:42
@chibondking chibondking force-pushed the fix/radiomodel-lambda-unique-connection branch from 701d767 to 98b168a Compare May 28, 2026 02:47
@chibondking
Copy link
Copy Markdown
Collaborator Author

Squashed to a single signed commit (98b168a). Tested on Windows with autothrottle on and off — build clean, no assert dialogs, manifest verified via mt.exe. Ready for review.

@chibondking
Copy link
Copy Markdown
Collaborator Author

@AetherClaude thoughts on this?

@aethersdr-agent
Copy link
Copy Markdown
Contributor

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.

⚠️ Undeclared scope: this PR reverts #3225

CMakeLists.txt here doesn't just swap /MANIFESTINPUT for /MANIFEST:NO. It also removes everything #3225 just landed two days ago:

  • The AETHER_EMBED_DFNR_MODEL option + AETHER_EMBED_DFNR_MODEL_DEFAULT block (lines 32–39 on main)
  • The DFNR_MODEL/DFNR_EMBEDDED_MODEL_NAME declarations at the top
  • The entire qt_add_resources(DFNR_RESOURCES …) QRC-generation block
  • ${DFNR_RESOURCES} from add_executable(AetherSDR …)
  • The AND NOT AETHER_EMBED_DFNR_MODEL guards on the loose-file install rules

Net effect: on Windows the DFNR weights stop getting embedded into the binary and revert to a loose DeepFilterNet3_onnx.tar.gz next to the .exe. The Store identity / single-binary deploy story from #3225 silently breaks. This isn't mentioned anywhere in the PR description or the "Files changed" table.

Most likely cause: the branch was cut from a parent commit older than 44eaef7c and never rebased, so squashing carried the deletion forward. Worth checking with git log --oneline main..HEAD -- CMakeLists.txt and git diff main…HEAD -- CMakeLists.txt.

Suggested action: rebase on current main and re-stage only the manifest hunk. The CMakeLists.txt diff should be just this region:

    if(MSVC)
        # Suppress the linker's own manifest pipeline so vs_link_exe does not generate
        # a competing manifest.res.  The manifest is embedded via AetherSDR.rc (type 24
        # id 1) which is compiled into the binary as a regular Win32 resource.
        # Background: MSVC 14.50+ (VS 2022 17.10) requires /MANIFEST:EMBED alongside
        # /MANIFESTINPUT, but CMake's vs_link_exe (up through 4.2.3) still emits the
        # old /MANIFEST /MANIFESTFILE: pair, producing LNK1220.  Switching to the .rc
        # approach avoids the two-pass manifest pipeline entirely.
        target_link_options(AetherSDR PRIVATE "/MANIFEST:NO")
    endif()

…with the rest of the file (DFNR option, QRC block, ${DFNR_RESOURCES} in add_executable, install guards) untouched.

On the three documented fixes themselves — all correct

  1. /MANIFEST:NO + RT_MANIFEST in .rc is the textbook workaround for the vs_link_exe / MSVC 14.50 mismatch. Resource id 1 type 24 is CREATEPROCESS_MANIFEST_RESOURCE_ID / RT_MANIFEST — the loader picks it up the same way it would an /MANIFEST:EMBED blob. The path in 1 24 "AetherSDR.exe.manifest" is resolved relative to the .rc file, matching the existing IDI_ICON1 ICON "AetherSDR.ico" pattern that already works, so no rc-include-path change needed. ✔
  2. Qt::UniqueConnection on a lambda in NetworkDiagnosticsHistory — yes, Qt's de-dup compares QObject* + const char* slot signature (or PointerToMemberFunction); functor connections fall through to a path where UniqueConnection triggers Q_ASSERT(type != Qt::UniqueConnection). Removing it is correct, and the ctor only runs once so there's no duplicate-connect risk. ✔
  3. Qt::UniqueConnection on the deferred-cap lambda in ensureOwnedPanadapter — same Qt rule, and the early-return on getOwnedPanadapter(panId) higher in the function genuinely makes the connect-site reachable at most once per panId, so the flag was both invalid and redundant. The autothrottle-on + add-second-slice repro you listed is exactly the path that would have surfaced this. ✔

Other notes

  • The mt.exe -inputresource:AetherSDR.exe;#1 verification step is the right check; nice to see it called out explicitly.
  • The Linux CI unaffected claim is true — /MANIFEST:NO is inside if(MSVC), and .rc files are only consumed via WIN_RC. No risk there.
  • Consider grepping for any other Qt::UniqueConnection paired with a lambda — same assert lurks anywhere this pattern recurs. A quick grep -rn 'UniqueConnection' src/ and a glance for \[.*\] in the same connect(...) call would catch the next one before it ships.

Net

Manifest fix and both UniqueConnection fixes: ready to land. Need to drop the unintended DFNR-embedding revert from CMakeLists.txt first. The aetherclaude-eligible label is not currently on this issue — a maintainer can authorize the orchestrator to land the cleaned-up version by adding it.

73, Jeremy KK7GWY & Claude (AI dev partner)


🤖 aethersdr-agent · cost: $2.3670 · model: claude-opus-4-7

@chibondking chibondking added the aetherclaude-eligible Issue approved for AetherClaude automated agent label May 28, 2026
@chibondking chibondking self-assigned this May 28, 2026
@chibondking
Copy link
Copy Markdown
Collaborator Author

@AetherClaude appropriate tag added.

@aethersdr-agent
Copy link
Copy Markdown
Contributor

Label received — the aetherclaude-eligible tag is on, so the orchestrator will pick this up on the next webhook cycle. Quick verification against current main (HEAD 44eaef7c) before handing off:

Bot review holds up

DFNR-embedding regression is real. CMakeLists.txt on main has all five regions the bot flagged still in place:

  • Lines 32–39: AETHER_EMBED_DFNR_MODEL_DEFAULT + option block
  • Line 411: DFNR_MODEL path declaration
  • Lines 912–928: qt_add_resources(DFNR_RESOURCES …) QRC-generation block
  • Line 933: ${DFNR_RESOURCES} in add_executable(AetherSDR …)
  • Lines 1409 / 1427: AND NOT AETHER_EMBED_DFNR_MODEL guards on macOS bundle + Linux install rules

And the /MANIFESTINPUT line the PR is replacing is at line 1064 — a single target_link_options(AetherSDR PRIVATE …) call inside the existing if(MSVC) block. So the cleaned-up diff really is a one-line swap.

Three documented fixes still apply. Both lambda-with-UniqueConnection sites are present:

  • src/gui/NetworkDiagnosticsDialog.cpp:2416adaptiveThrottleChanged lambda
  • src/models/RadioModel.cpp:3440waterfallIdChanged deferred-cap lambda in ensureOwnedPanadapter

For reference, the Qt::UniqueConnection uses in src/gui/RxApplet.cpp (lines 1671, 1680) and src/gui/MainWindow.cpp are not affected — those pair with member-function pointers, which is the supported path (the comment at RxApplet.cpp:1661–1665 calls this out explicitly).

Plan for the orchestrator

Rebase on current main, then make exactly these changes:

  1. CMakeLists.txt — only this hunk:

        if(MSVC)
            # Suppress the linker's own manifest pipeline so vs_link_exe does not generate
            # a competing manifest.res.  The manifest is embedded via AetherSDR.rc (type 24
            # id 1) which is compiled into the binary as a regular Win32 resource.
            # Background: MSVC 14.50+ (VS 2022 17.10) requires /MANIFEST:EMBED alongside
            # /MANIFESTINPUT, but CMake's vs_link_exe (up through 4.2.3) still emits the
            # old /MANIFEST /MANIFESTFILE: pair, producing LNK1220.  Switching to the .rc
            # approach avoids the two-pass manifest pipeline entirely.
            target_link_options(AetherSDR PRIVATE "/MANIFEST:NO")
        endif()

    Everything else from [windows] Embed DFNR model and set Store identity #3225 (DFNR option, QRC block, ${DFNR_RESOURCES} in add_executable, install guards) stays put.

  2. packaging/windows/AetherSDR.rc — append 1 24 "AetherSDR.exe.manifest" alongside the existing IDI_ICON1 ICON "AetherSDR.ico".

  3. src/gui/NetworkDiagnosticsDialog.cpp:2416 — drop the , Qt::UniqueConnection argument; the closing brace + paren stay.

  4. src/models/RadioModel.cpp:3440 — same: drop , Qt::UniqueConnection.

Test plan to mirror in the new PR description is exactly the one already on this issue (autothrottle on/off × single/dual slice, plus mt.exe -inputresource:AetherSDR.exe;#1 manifest extraction).

73, Jeremy KK7GWY & Claude (AI dev partner)


🤖 aethersdr-agent · cost: $3.9889 · model: claude-opus-4-7

Copy link
Copy Markdown
Contributor

@aethersdr-agent aethersdr-agent Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_MODEL option block (around line 32)
  • Deletes the dfnr_model.qrc generation + qt_add_resources(DFNR_RESOURCES …) block
  • Drops DFNR_RESOURCES from add_executable(AetherSDR …)
  • Drops the AND NOT AETHER_EMBED_DFNR_MODEL guards 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.

  • NetworkDiagnosticsHistory ctor runs once, so the flag was always a no-op.
  • For ensureOwnedPanadapter, I verified the early-return guard claim — RadioModel.cpp:3412 returns existing before reaching the connect, so the connection is genuinely made at most once per pan; UniqueConnection was 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

Copy link
Copy Markdown
Contributor

@aethersdr-agent aethersdr-agent Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  1. Manifest pipeline (CMakeLists.txt + AetherSDR.rc): switching from /MANIFESTINPUT to /MANIFEST:NO + a 1 24 "AetherSDR.exe.manifest" RT_MANIFEST resource is a clean, well-justified workaround for the CMake vs_link_exe / MSVC 14.50 mismatch. Comment in CMakeLists.txt explains the rationale clearly.

  2. NetworkDiagnosticsHistory ctor (NetworkDiagnosticsDialog.cpp:2416): removing Qt::UniqueConnection from 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.

  3. ensureOwnedPanadapter deferred-cap lambda (RadioModel.cpp:3440): same reasoning, and the surrounding if (auto* existing = …) return existing early-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_MODEL option + Windows-default block (CMakeLists.txt:32-39 on main)
  • Removes the dfnr_model.qrc generation + qt_add_resources(DFNR_RESOURCES ...) block (~20 lines)
  • Drops ${DFNR_RESOURCES} from add_executable(AetherSDR ...)
  • Removes the AND NOT AETHER_EMBED_DFNR_MODEL guards 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:22 still references :/models/DeepFilterNet3_onnx.dfmodel and 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:173 invokes create-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.md documents AETHER_EMBED_DFNR_MODEL=ON as 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:

  1. 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
  2. If the DFNR embed feature is genuinely broken on your local MSVC 14.50 toolchain (e.g., the QRC file path or qt_add_resources interacts 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 update windows-installer.yml + WINDOWS-STORE-MSIX.md so 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

Copy link
Copy Markdown
Collaborator

@jensenpat jensenpat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please ask your agent to rebase this to the latest main.

@chibondking chibondking force-pushed the fix/radiomodel-lambda-unique-connection branch from 98b168a to a8bc6ed Compare May 28, 2026 05:12
@chibondking
Copy link
Copy Markdown
Collaborator Author

Good catch @aethersdr-agent and @jensenpat — the prior squash was built on a local main that was 1 commit behind origin/main, so the DFNR embed from #3225 was missing from the base and got accidentally reverted in the diff.

Rebased cleanly onto current origin/main (44eaef7) and re-applied the four intended changes by hand — no cherry-picks, no soft-reset artifacts. The only edit to CMakeLists.txt is now the target_link_options block under if(MSVC); all DFNR machinery is intact. Force-pushed as a8bc6ed.

@aethersdr-agent
Copy link
Copy Markdown
Contributor

Rebase looks clean — pulled the PR diff post-force-push (a8bc6ed5) and it now contains exactly the four intended hunks, nothing else:

File Hunk Lines
CMakeLists.txt /MANIFESTINPUT/MANIFEST:NO inside the existing if(MSVC) block (+ explanatory comment) +8 / −2
packaging/windows/AetherSDR.rc Append 1 24 "AetherSDR.exe.manifest" (+ comment) +6 / −0
src/gui/NetworkDiagnosticsDialog.cpp:2416 Drop , Qt::UniqueConnection from the adaptiveThrottleChanged lambda +1 / −1
src/models/RadioModel.cpp:3440 Drop , Qt::UniqueConnection from the deferred-cap lambda in ensureOwnedPanadapter +1 / −1

All of #3225's DFNR machinery — AETHER_EMBED_DFNR_MODEL option, DFNR_MODEL/DFNR_EMBEDDED_MODEL_NAME declarations, the qt_add_resources(DFNR_RESOURCES …) block, ${DFNR_RESOURCES} in add_executable, and the AND NOT AETHER_EMBED_DFNR_MODEL install guards — is no longer touched in the diff. The Windows single-binary deploy story is preserved.

Quick sanity sweep on the other Qt::UniqueConnection sites

Grepped the rest of src/ for Qt::UniqueConnection to make sure the lambda-paired anti-pattern isn't lurking elsewhere. Every remaining occurrence pairs with a member-function-pointer slot, which is the supported deduplication path:

  • src/gui/RxApplet.cpp:1671,1680 — member-fn slots; the inline comment at 1662 explicitly documents the lambda restriction.
  • src/gui/MainWindow.cpp:2916,2919&SpectrumWidget::syncWnbState / &SpectrumOverlayMenu::syncWnbState
  • src/gui/MainWindow.cpp:11784,11787 — same pair as above
  • src/gui/MainWindow.cpp:15452&MainWindow::onRadeSliceModeChanged
  • src/gui/MainWindow.cpp:15823,15826&MainWindow::onFdvMeterUpdated / &MainWindow::onFdvMetersChanged

No other lambda+UniqueConnection pairings to clean up.

Handoff status

The aetherclaude-eligible label is on, the diff is the cleaned-up version the prior review asked for, and the test plan in the PR body covers the autothrottle on/off × single/dual-slice matrix plus the mt.exe -inputresource:AetherSDR.exe;#1 manifest extraction check. Orchestrator should be able to land this on the next webhook cycle without further hand-edits — pending CI gate.

73, Jeremy KK7GWY & Claude (AI dev partner)


🤖 aethersdr-agent · cost: $2.1602 · model: claude-opus-4-7

…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.
@chibondking chibondking force-pushed the fix/radiomodel-lambda-unique-connection branch from a8bc6ed to 8b78e0f Compare May 28, 2026 05:15
@chibondking chibondking changed the title fix(windows): LNK1220 manifest pipeline + Qt::UniqueConnection lambda asserts (3 sites) fix(RadioModel,NetworkDiag): remove Qt::UniqueConnection from adaptive-throttle lambda slots May 28, 2026
@chibondking
Copy link
Copy Markdown
Collaborator Author

Rebased onto current upstream/main (14b45a0, which includes #3237). Scope narrowed to the two Qt::UniqueConnection lambda asserts only — the LNK1220 fix is no longer needed here since #3237 already handles it. CMakeLists.txt and AetherSDR.rc changes dropped entirely. Two files, two lines changed. No merge conflicts.

@ten9876 ten9876 enabled auto-merge (squash) May 28, 2026 14:13
@ten9876
Copy link
Copy Markdown
Collaborator

ten9876 commented May 28, 2026

Hey @jensenpat@chibondking did the rebase you asked for; current head 8b78e0fa is now just the two-line Qt::UniqueConnection removal (DFNR revert and manifest changes are gone). When you have a moment, would you mind dismissing your CHANGES_REQUESTED or re-reviewing? Auto-merge is enabled and CI is green, so it'll land as soon as the review block clears.

73, Jeremy KK7GWY & Claude (AI dev partner)

@ten9876 ten9876 merged commit ba79909 into aethersdr:main May 28, 2026
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

aetherclaude-eligible Issue approved for AetherClaude automated agent

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants