Refactor Phase 2: extract SwrSweepController + FreeDvSessionController from MainWindow (header member reduction)

[ten9876]

Follow-up to #3351 (now closed — Phases 0–1 + RadioSession delivered). This is the issue's original Phase 2, carved out as its own scoped effort because it's a different kind of change from the Phase 0–1 work that landed.

What Phase 1 did vs. what Phase 2 needs

Phase 1 (#3508–#3541) moved subsystem code into sibling TUs — MainWindow_SwrSweep.cpp, MainWindow_DigitalModes.cpp, etc. — but the subsystems' state still lives in MainWindow.h. The header is still ~158 member variables. Phase 2 finishes the job the original issue named:

"Extracting a SwrSweepController / FreeDvSessionController that own their own timers and state would pull ~1,000+ lines and their member variables out of the 190-member header."

This is not behavior-preserving code motion. It moves state ownership and object lifetimes across a new class boundary — so it needs careful review and bench QA, unlike the Phase 1 TU splits.

Target 1 — SwrSweepController

The sweep is already a self-contained state machine; the state is even pre-grouped in a struct. Members to extract from MainWindow.h:

• SwrSweepState m_swrSweep; (the whole struct — running/phase/meterSource/sliceId/panId/frequencies/samples/timing/power fields, ~20 fields at MainWindow.h:910)
• QTimer m_swrSweepTimer;
• The 14 methods already in MainWindow_SwrSweep.cpp (startSwrSweep, advanceSwrSweep, finishSwrSweep, …) become SwrSweepController methods.
• The kSwrSweep* constants (currently in MainWindowHelpers.h) move to the controller.

Coupling to resolve: the sweep methods touch m_radioModel (commands), spectrumForSlice()/pan overlay (UI), and the status bar. The controller takes a RadioModel& + a small callback/signal interface for the UI bits — it should NOT depend on MainWindow's widgets directly (same model-vs-GUI-layer discipline as the RadioSession finding in #3555).

Target 2 — FreeDvSessionController

RADE + classic FreeDV state, ~15 members at MainWindow.h:958–977 (m_radeEngine, m_radeThread, m_radeSliceId, m_radeDaxStreamId, the RADE/FDV connection handles, m_fdvDisplaySliceId, m_fdvSnrMeterIndex, m_fdvSynced, …). Methods already in MainWindow_DigitalModes.cpp (activateRADE/deactivateRADE/activateFdvDisplay/…) become controller methods.

Caveat: more UI-coupled than the sweep (touches applet panel, VFO widgets, meters) — so the extraction boundary needs more care, or this target stays a "thin controller owning state + lifecycle, UI wiring stays in the GUI layer" split. Worth a design note before coding.

Acceptance

• MainWindow.h member count drops by ~35 (the SWR + RADE/FDV members)
• Both controllers live in the GUI layer but depend on RadioModel/AudioEngine, not on MainWindow's widgets
• Behavior identical — full SWR-sweep + RADE + FreeDV bench QA
• No new -Wreorder; build clean on the Qt 6.4.2 floor

Not in scope

Phase 3 (table-driven menu/shortcut builders) — separate follow-up.

Refs #3351 #3445

[aethersdr-agent]

Triage — maintainer-review

Well-scoped maintainer follow-up to #3351, and the source matches the issue's description exactly. No ambiguity to resolve with the reporter — this is ready for a go/no-go on implementation. Verified both targets against the current tree.

Target 1 — SwrSweepController (clean, recommend doing first)

State in MainWindow.h:894–944: the nested types SwrSweepSample / SwrSweepPhase / SwrSweepMeterSource / SwrSweepState, the SwrSweepState m_swrSweep member, and QTimer m_swrSweepTimer. The 14 methods are all in MainWindow_SwrSweep.cpp, and the kSwrSweep* constants are isolated at MainWindowHelpers.h:92–104.

The sweep is genuinely a self-contained state machine (the phase enum + the advanceSwrSweep() switch at MainWindow_SwrSweep.cpp:373–507 drive everything off the timer). The non-RadioModel coupling is small and enumerable — the controller needs a callback/signal seam for exactly these, none of which should be a direct widget dependency:

• applyPanRangeRequest(panId, center, bw, tag) — MainWindow.h:250
• spectrumForSlice() + per-applet SpectrumWidget::setSwrSweepPoints/clearSwrSweepPoints (the overlay, updateSwrSweepOverlay)
• swrSweepTargetSlice() — MainWindow.h:453
• statusBar()->showMessage(...) (many sites)
• input-lock: enable/disable m_appletPanel + m_panStack (setSwrSweepInputsLocked)
• read access to m_splitActive and m_bandPlanMgr

So SwrSweepController(RadioModel&, <ui-callback-iface>) is achievable and matches the model-vs-GUI discipline from the #3555 RadioSession note. Net header drop: ~20 declarations (2 data members + 4 nested types + 14 methods).

Target 2 — FreeDvSessionController (agree with the issue's caveat)

State at MainWindow.h:957–981 (the HAVE_RADE block): m_radeEngine, m_radeThread, m_radeSliceId, m_radePrevMute, m_radeDaxStreamId, three QMetaObject::Connection handles, m_lastRadeRxCallsign, m_radeEooPending, m_radeTxActive, plus the FDV trio m_fdvDisplaySliceId / m_fdvSnrMeterIndex / m_fdvSynced. Methods are in MainWindow_DigitalModes.cpp.

This one is materially more UI-coupled than the sweep — activateRADE() alone (:188–497) wires VfoWidget, RadeApplet, PhoneCwApplet, AudioEngine, m_freedvClient, m_radioModel.panStream(), and the three-layer PTT/EOO orchestration. I'd push back on extracting the connect-wiring: per the RadioSession v3 scope note (RadioSession.h:38–46), model→UI glue is application-layer by nature and moving it onto a controller inverts the dependency. Recommend the issue's fallback boundary: the controller owns the m_radeEngine/m_radeThread lifecycle + the state fields + the EOO/PTT hook logic (the parts that are pure RADE/audio), and the VfoWidget/applet wiring stays in the GUI layer behind a signal forwarded from the controller. Worth the design note the issue asks for before coding. Net header drop here: ~16 data members + methods.

Risks to flag for the implementer

• -Wreorder: removing members shifts ctor init-list order — re-sort init lists after extraction (this is on the acceptance list for good reason).
• Teardown order: deactivateRADE() (:499–605) has a precise sequence (disconnect → stop on worker thread → m_radeThread->wait(2000) → null engine) and the DAX-stream-remove guards depend on tciServer()/m_daxBridge liveness. The controller's dtor must preserve that ordering — same class of crash-on-quit the Crash on quit — TciServer::releaseDaxForTci() dereferences null/dangling RadioModel during MainWindow teardown #2385 work guarded against.
• Not behavior-preserving (unlike the Phase 1 TU splits): object lifetimes cross a new boundary. The acceptance criteria correctly call for full SWR-sweep + RADE + FreeDV bench QA, including the TGXL-bypass sweep path and the EOO-on-unkey timing.
• Both controllers go in src/gui/ (they need GUI callbacks), distinct from RadioSession which lives in src/models/.

Recommendation

Land as two separate PRs — SwrSweepController first (low-coupling, high member-count payoff), then FreeDvSessionController after a design note settles the UI-wiring boundary. Awaiting aetherclaude-eligible to proceed.

---

_{🤖 aethersdr-agent · cost: $2.5784 · model: claude-opus-4-8}

---

_{📍 Citation health check (Foundry Constitution Principle I): 0 resolved, 7 unresolved against /Users/aetherclaude/workspace/AetherSDR at origin/main.}

Unresolved citations (7)

Citation	Reason
MainWindow.h:250	file not found in workspace
MainWindow.h:453	file not found in workspace
MainWindow.h:894	file not found in workspace
MainWindow.h:957	file not found in workspace
MainWindowHelpers.h:92	file not found in workspace
MainWindow_SwrSweep.cpp:373	file not found in workspace
RadioSession.h:38	file not found in workspace

[ten9876]

How this plan was produced. A multi-agent analysis workflow surveyed all 9 candidate MainWindow subsystems (the 2 named in this issue + 7 others), with an adversarial-verification pass per candidate, then synthesized the ranking below. I (maintainer) reviewed the output against the source before posting and corrected two findings the agents got wrong: both claimed "race conditions" (m_spotDedup, m_sHistoryData) are not races — those handlers run on the GUI thread because their worker-thread signals connect via the connect(sender, sig, this, functor) receiver-context form, which Qt::AutoConnection marshals to MainWindow's thread. The corrected text is inline below; flagging the correction transparently because the original "fix this race" instruction would have sent someone to add a needless mutex. Everything else (the layering analysis, the extract/leave verdicts, and especially the two reversals — see next paragraph) checked out against the code.

Bottom line up front: the analysis recommends renegotiating this issue's scope. Both candidates named in the issue body shrank under scrutiny — FreeDvSession should not be extracted at all (thread-destructor ordering + BlockingQueuedConnection patterns require MainWindow membership), and SpotSubsystem yields ~8 members, not the full subsystem (the rest is model-layer signal analysis that a src/gui/ move would invert). The genuine headline win is a candidate the issue didn't name: AudioEngineController. Realistic deliverable is ~16 members across 2 PRs, not a sweeping header purge.

---

#3557 Implementation Plan — Extract Controller Classes from MainWindow

Goal & Headline Finding

Shrink MainWindow's oversized header (158 declared members on the original count; 265 raw m_ matches today) by extracting cohesive subsystems into dedicated controller classes, following up on the completed #3351 TU-split decomposition. #3557 named two candidates (FreeDvSession, SpotSubsystem); this plan evaluates eight and applies the adversarial-verification corrections.

Headline: Of the ~107 header members spanning all eight candidates, only ~16 members across two subsystems are genuinely worth extracting now (AudioEngineThread fully + a thin spot-batching slice of SpotSubsystem). Both #3557-named candidates were corrected downward by verification: FreeDvSession flips to leave, and SpotSubsystem flips from a 21-member GUI extraction to a ~8-member partial (the rest is model-layer signal analysis that would invert layers, ). The biggest "clean" win is the one candidate verification upheld: AudioEngineThread. The realistic, low-drift deliverable for #3557 is one solid extraction (AudioEngine) plus one scoped partial (spot-batching), not a sweeping header purge.

Ranked Candidates (post-verification)

Ranked by impact ÷ risk — clean low-risk wins first.

#	Subsystem	Verdict (post-verify)	Members removed	UI coupling	Behavior risk	Suggested order
1	AudioEngineThread	extract-gui-controller (upheld)	~9	high	high	PR 1
2	SpotSubsystem (batching slice only)	partial (corrected from full)	~8 of 21	high	medium	PR 2
3	DaxBridge	partial (corrected)	~2 of 4	high	high	PR 3 (optional)
4	ExternalControllers	partial / internals-only (corrected)	~0–8 of 42	high	high	defer
5	AudioDeviceMonitor	partial (corrected)	(subset of #1)	high	medium	folded into PR 1
6	FreeDvSession	leave (corrected from extract)	0	high	high	do not extract
7	SHistoryMarkers	leave	0	high	high	do not extract
8	PanLifecycle	leave	0	high	high	do not extract
9	SwrSweep	leave	0	high	high	do not extract

Note: AudioDeviceMonitor (#5) and AudioEngineThread (#1) overlap on the same 7 device-tracking members; they are one PR, not two.

Top Recommended Extractions

1. AudioEngineController — src/gui/

The only candidate verification upheld as extract-gui-controller. AudioEngine itself is pure audio (no GUI deps), and the surrounding MainWindow management code is cohesive.

• Members owned (~9): m_audio (AudioEngine*), m_audioThread (QThread*), m_audioDeviceMonitor (QMediaDevices*), m_audioDeviceChangeTimer, m_knownAudioInputIds, m_knownAudioOutputIds, m_knownDefaultAudioInputId, m_knownDefaultAudioOutputId, m_audioDeviceDialogOpen.
• Methods absorbed: AudioEngine init/thread setup (MainWindow.cpp:920–945), device hot-plug detection + change handling (MainWindow.cpp:266–330, 3311–3565), and the handleAudioDeviceListChanged() dialog flow.
• Coupling boundary: Lives in src/gui/ (mandatory — QDialog::exec() on AudioDeviceChangeDialog at MainWindow.cpp:3473 forbids the model layer). Reaches m_radioModel.transmitModel().micSelection() / radioAddress() (MainWindow.cpp:3435) and m_audio (3421–3422, 3504–3505) via signal-based access, not a back-pointer to MainWindow — the verification flagged that a direct m_radioModel reference from the new class would invert layers. Inject AudioEngine and a RadioModel reference; surface device-change events as signals MainWindow connects to for TitleBar status updates.
• Behavior-preservation watch-items:
  • Destructor uses BlockingQueuedConnection for thread shutdown (MainWindow.cpp:1927), followed by m_audioThread->wait(3000) — a wedged audio thread blocks teardown for up to 3s (bounded, not a true deadlock, but disruptive). Preserve this exact teardown ordering inside the controller's destructor; do not reorder relative to dialog teardown.
  • m_audioDeviceChangeTimer thread affinity + moveToThread coordination is a silent-break hazard — keep the timer on the main thread.
  • The m_shuttingDown guard (MainWindow.cpp:3380, 3387) prevents UAF — expose it to the controller via constructor reference or a setShuttingDown() call, not an implicit pointer cycle.
  • Design-note required: modal device-dialog ordering vs. cross-thread audio signal emission. Write this down before coding (verification confidence on this candidate was only 0.65).

2. SpotBatchingController — src/gui/ (PARTIAL — batching slice only)

Verification corrected the full 21-member GUI extraction to partial: ~13 of those members (m_sHistoryData, m_sHistoryPanState, m_spectrogramBuffers, m_signalClassifier, m_smartSpotFilterEnabled, the SHistory enable flags) are model-layer signal-analysis concerns (CNN classification, voice/QRM dedup). Moving them to src/gui/ inverts the dependency. Extract only the spot-batching machinery.

• Members owned (~8): m_dxCluster, m_rbnClient, m_wsjtxClient, m_spotCollectorClient, m_potaClient, m_spotThread, m_spotCmdBatch, m_spotDedup (plus m_nextPassiveSpotId, m_passiveSpotExpiryMs as batch-adjacent state).
• Members explicitly NOT moved: m_sHistoryData, m_sHistoryPanState, m_sHistoryExpireTimer, m_sHistoryEnabled, m_sHistoryQrmEnabled, m_spectrogramBuffers, m_signalClassifier, m_propForecast, m_freedvClient — these stay in MainWindow (SHistory subsystem, see leave-list).
• Methods absorbed: DX-Cluster/RBN/WSJT-X/SpotCollector/POTA client coordination, the 1/sec batch flush, and passive-spot expiry from MainWindow_Spots.cpp. Leave applySHistoryEnabled, rebuildSHistoryForPan, expireSHistoryMarkers, onSpectrumReadyForSHistory in MainWindow.
• Coupling boundary: GUI-layer controller owning the spot worker thread + clients, forwarding batched spots to the radio via a thin facade. No SpectrumWidget push (that's the SHistory half staying behind).
• Behavior-preservation watch-items:
  • No race, despite first appearances (the workflow's verification agents flagged one here; maintainer review refuted it): m_spotDedup is unguarded, but isDuplicateSpot runs on the GUI thread, not a worker thread. The clients live on m_spotThread, but connect(client, &…::spotReceived, this, functor) (MainWindow_Spots.cpp:423–525) uses the receiver-context form — sender on worker thread, receiver context = this (MainWindow, GUI thread) — so Qt::AutoConnection marshals the functor to the GUI event loop. All dedup is serial on the GUI thread. The extraction must preserve that connection form (keep the controller QObject on the GUI thread, connect with this/controller as receiver context) — that's the actual invariant to protect, not a mutex.
  • Thread-shutdown sequencing for m_spotThread must precede client deletion.
  • Design-note required: the exact GUI/model boundary line between batching (extract) and SHistory analysis (stay). Document which members cross and why, so a future Refactor Phase 3 (optional): table-drive the menu + shortcut builders #3558 can lift the analysis half into src/models/ cleanly.

3. DaxBridgeController — src/gui/ (PARTIAL — optional, lower priority)

Verification corrected from full extraction to partial: the DAX stream lifecycle is extractable, but startDax/stopDax contain QMessageBox::warning(this, ...) dialogs (MainWindow_DigitalModes.cpp:877–890) and m_appletPanel->daxApplet()/daxIqApplet() wiring (1016–1037) that are inherently GUI-layer and parent-bound.

• Members owned (~2 of 4): m_daxSliceConns, m_daxSliceLastCh (the stream-tracking state). m_daxBridge and m_savedMicSelection stay closely tied to MainWindow's applet/mic context.
• Methods absorbed: the DAX stream create/remove + ownership tracking from onDaxChannelChanged (MainWindow_DigitalModes.cpp:1163–1189).
• Methods that STAY: startDax/stopDax (error dialogs + applet wiring), wireDaxSlice.
• Coupling boundary: extract the stream-lifecycle state machine; keep UI wiring and dialogs in MainWindow.
• Behavior-preservation watch-items: feedDaxAudio is called via DirectConnection from the network thread with m_open/m_channelGain accessed lock-free (VirtualAudioBridge.h:84–88) — do not perturb this lock-free contract. DAX virtual audio: only slice 0 gets audio client on Linux PipeWire headless (RPi 5, Ubuntu 24.04, FLEX-8600) #2895 stream-ownership guards (TCI/RADE sharing detection) must survive intact. Only do this PR if PRs 1–2 land cleanly; payoff is the smallest of the three.

Leave Alone (do NOT extract)

• FreeDvSession — Refactor Phase 2: extract SwrSweepController + FreeDvSessionController from MainWindow (header member reduction) #3557-named, but verification inverts the verdict to leave. RADEEngine is a core codec engine, not GUI; extraction breaks thread destructor ordering (m_radeThread must outlive m_radeEngine slots) and creates models→GUI-event-loop dependencies via QTimer::singleShot([this]...) at MainWindow_DigitalModes.cpp:361. BlockingQueuedConnection patterns (261, 592) require MainWindow membership. Containment is the correct design; revisit under Refactor Phase 3 (optional): table-drive the menu + shortcut builders #3558.
• SHistoryMarkers (the 13 members carved out of SpotSubsystem) — all 5 methods push directly into SpectrumWidget, so a src/gui/ move buys little and a src/models/ move would invert layers. (The workflow claimed onSpectrumReadyForSHistory races on m_sHistoryData from a network thread; refuted on review — it's connected panStream→this so AutoConnection runs it on the GUI thread, MainWindow_Session.cpp:860. Not a race, but still a leave on the layering grounds above.) Revisit as a model-layer analysis component under Refactor Phase 3 (optional): table-drive the menu + shortcut builders #3558.
• PanLifecycle — 1016-line wirePanadapter (~70 connects binding SpectrumWidget/overlay/applet to MainWindow lambdas). Already well-modularized across TUs by Refactor: break up the 15k-line MainWindow.cpp monolith (phased, behavior-preserving) #3351; extraction needs MainWindow* (defeats decoupling) or bloats widgets. Model-layer move impossible (RadioSession must not import GUI).
• SwrSweep — 6-phase state machine already isolated in its own TU; tight TGXL hardware-state + keyboard-interception coupling makes it a transient application-layer controller, not a service. No refactor needed.
• ExternalControllers — verification corrected to partial/internals-only. ~20+ of the 42 members (coalesce/idle timers, RC-28 hold state machines at 2149–2235, TMate2 overlay/idle, m_extCtrlThread) must stay in MainWindow. The architecture is already sound GUI-layer integration. At most, extract the worker-thread setup into a helper method; full extraction adds complexity with no modularity benefit. Defer.

Sequencing

1. PR 1 — AudioEngineController (src/gui/). The upheld, highest-ratio win. Folds in AudioDeviceMonitor's 7 device-tracking members (same subsystem). Delivers ~9 members removed and a testable audio/device-hotplug component. Requires the modal-ordering design note first. This is the keystone PR.
2. PR 2 — SpotBatchingController (src/gui/, partial). Removes ~8 members. Requires the GUI/model boundary design note. Leaves SHistory analysis in place for a future Refactor Phase 3 (optional): table-drive the menu + shortcut builders #3558.
3. PR 3 — DaxBridgeController (src/gui/, partial, optional). Removes ~2 members. Lowest payoff; only attempt if 1–2 are clean and time remains.

Natural stopping point: after PR 2. That captures the genuinely extractable surface (~16–17 members) at acceptable risk. Everything beyond is either a leave verdict, a partial with poor cost/benefit (DaxBridge, ExternalControllers), or blocked on the #3558 integration framework.

Honest Caveats

• Both Refactor Phase 2: extract SwrSweepController + FreeDvSessionController from MainWindow (header member reduction) #3557-named candidates shrank under scrutiny. FreeDvSession should not be touched at all, and SpotSubsystem yields a third of its advertised members. If Refactor Phase 2: extract SwrSweepController + FreeDvSessionController from MainWindow (header member reduction) #3557's acceptance criteria assume those two as large wins, renegotiate scope before coding — AudioEngine is the real headline.
• AudioEngineController carries real teardown risk (BlockingQueuedConnection + bounded 3s wait on a possibly-hung audio thread) and only 0.65 verification confidence. It needs a written design note on modal-dialog vs. cross-thread-signal ordering before implementation.
• The two "race conditions" the analysis flagged are not races (maintainer-verified before posting — see preamble). Both m_spotDedup and m_sHistoryData mutate on the GUI thread, because their producing signals use connect(workerSender, sig, this, …) and Qt::AutoConnection marshals to MainWindow's thread. No mutex needed; the invariant to preserve is that the extracted controllers stay GUI-thread objects connected via receiver-context. Do not add synchronization — it would be cargo-cult.
• Layer inversion is the dominant failure mode. Multiple verdicts were over-extractions that would have pushed model-layer logic (RADE codec, CNN classifier, signal dedup) into src/gui/, or required a MainWindow* back-pointer that defeats the decoupling. Every extracted controller must reach RadioModel/AudioEngine via injected references + signals, never a back-pointer.
• Design notes required before coding: (1) AudioEngineController modal/thread ordering; (2) SpotBatchingController GUI/model boundary + dedup synchronization. Do not start either PR without its note landed.