perf(wdsp): build the minimum-phase workspace only when minimum phase is on

[on8st]

plan_fircore() ends with an unconditional a->pminphase = create_minphase(a->nc, a->pfactor), with no reference to a->mp. calc_fircore() only uses that workspace under if (a->mp). So every fircore in the tree builds it, and almost none of them ever reads it.

This is shared DSP. Every backend pays it — Flex, Icom, ANAN, RTL-SDR and the HL2 alike, and every one of them for a code path that does not execute.

The cost is plan construction, not bytes

create_minphase() builds four FFTW_PATIENT plans per instance. FFTW_PATIENT measures many transform strategies at plan time; that is where the seconds go, and it is why this is worth more than its allocation figure suggests.

One RX channel open at 8192 taps makes 19 create_minphase() calls, 18 of them dead — 72 never-executed FFTW_PATIENT plans, at lengths 131072 (×3), 65536, 32768 (×8), 8192 (×5) and 4096.

Related: #5456 — this PR does not establish that link. It is a candidate mechanism for the cold-connect times recorded there, and connecting the two needs a cold-wisdom connect-path timing that nobody has taken. I am not claiming it.

(Worded this way deliberately. The sentence that stood here read "This does not fix" followed directly by the issue number — and GitHub's closing-keyword parser does not read negation. It matched the keyword against the number that followed it and set this PR to close that issue on merge. The disclaimer created the link it disclaimed. Keeping the reference is fine; keeping it away from fix / close / resolve is the part that matters.)

The inventory, counted rather than assumed

Counted by instrumenting create_fircore() and create_minphase() and opening one channel, then confirmed by reading the call sites.

An RX channel builds 13 fircores; 12 are mp == 0.

• Six scale with the channel's tap count, reached by RXASetNC: rxa[].nbp0, the nbp inside rxa[].bpsnba, rxa[].bp1, rxa[].fmsq, fmd->pde, fmd->paud.
• Six have a fixed nc and pass mp as a literal 0 at their call site: rxa[].doublepole, rxa[].matched, rxa[].gaussian, and rxa[].wbfm's pfil0_15, pfil19 and pfil23_53.
• The exception is rxa[].eqp, which is created with the minimum-phase flag set (nc 16384, pfactor 4). Its workspace is real and the patch keeps it. RXASetMP never touches it.

A TX channel builds 7 fircores, 6 of them mp == 0.

Figures

Arithmetic on a code reading, not measurements. 643,072 elements × 104 B (six complex at 16 B plus one real at 8 B):

	never-executed workspace
RX channel at 8192 taps	≈ 66.9 MB
RX channel at WDSP's default	≈ 28.5 MB
TX channel	≈ 16.2 MB
four receivers	≈ 268 MB

What was measured, through WDSP's own wdspPortOutstandingAllocations() / wdspPortAllocationSequence(), macOS arm64, one RX open at 8192 taps with mp == 0: live allocations 1300 → 1204, allocations made during open 1518 → 1374. create_minphase() makes 8 allocations, so that is exactly 12 fewer held and 18 fewer built — matching the counted inventory independently.

The patch

plan_fircore() builds the workspace only when a->mp is set. calc_fircore() builds it on first use through a new static ensure_minphase(). deplan_fircore() tolerates its absence.

The laziness is load-bearing, not tidiness. setMp_fircore() can turn mp on at any time, and WdspChannel::open() calls RXASetNC() before RXASetMP() — so with minimum phase requested, the six re-planned cores are planned while mp == 0 and flipped on afterwards. Building only in plan_fircore() would be a null dereference in mp_imp_exec().

Test

tests/wdsp_channel_test.cpp gains runMinimumPhaseWorkspaceTest — the first test in this tree to set Config::minimumPhase. It asserts audio out of both phase modes, and that minimum phase costs strictly more live WDSP allocations than linear. A relation, not a hardcoded count.

It goes red with the patch reverted (linear=2069 minimum=2069), which is the only thing that stops a no-op passing.

Full ctest on the rebased branch (macOS arm64, RelWithDebInfo, -j 4): 449 tests, 447 ran, 446 passed, 1 failed. Two skipped by their own guards (crdv_quarantined_test, weather_radar_texture_gl_test). wdsp_channel_test passes, 3.51 s. Ownership of the failure established from the target's own link line and sources rather than assumed:

• tgxl_docked_parity_test — deterministic, 7 of 7 (both full runs plus 5 of 5 solo reruns). It fails the QFontMetrics(drawn).horizontalAdvance(btn->text()) <= btn->width() - 6 check in tests/tgxl_docked_parity_test.cpp, with Qt logging a missing "Sans Serif" family under QT_QPA_PLATFORM=offscreen and ThemeManager falling back to compiled-in defaults. Its link line carries neither libaethercore.a nor third_party/wdsp/libaether_wdsp.a, the file contains no reference to WDSP, fircore or minimum phase, and its only commit is 43687413 (feat(tuner): TGXL front-panel presentation, tune abort, and tuner alerts #5676). Not this PR's.

vkamp_connection_test also failed once, in an earlier full run, and passed the second full run and 4 of 5 solo reruns — 2 failures in 7 observations. The check that fails is amp.commands.isEmpty() after a 400 ms spinFor() in tests/vkamp_connection_test.cpp: a time-boxed socket assertion, which is the shape of a timing flake rather than a regression. Same link line — no aethercore, no WDSP. Last touched by 13ba5790 (#4919).

AddressSanitizer

AETHERSDR-PATCHES.md asks for the lifecycle test under ASan on every supported platform. This patch is the only one of the seven that makes a pointer legitimately null, so ASan is exactly its gate.

An ASan build of the vendored library alone (all 78 translation units, -fsanitize=address), cycling open / RXASetNC / RXASetMP(1) / RXASetNC / RXASetMP(0) / close twice plus a TX open: clean, on macOS arm64. The Linux sanitizers.yml lane has not signed this off, and that is the same lane whose applicability to the 2.10 refresh is still open on #5686.

Numbering

This is patch 7. It was written as patch 5 against a main that listed four. #5687 landed while this branch was in review and added two — upstream/nnr.c / upstream/nnr.h and upstream/nnet.c — taking 5 and 6, so the entry is renumbered on the rebase.

The in-source markers no longer carry the ordinal at all. Per the reviewer's nit, the five comments in firmin.c and tests/wdsp_channel_test.cpp now name the change and point at AETHERSDR-PATCHES.md instead of citing a number, so the ordinal appears in exactly one place: the registry entry. grep AetherSDR still finds all five.

#5628 is still open and also touches AETHERSDR-PATCHES.md and tests/wdsp_channel_test.cpp (it inserts after runNotchAttenuationTest() and appends to the same runLeakChecked chain in main()). Whichever of the two lands second conflicts in both files; the ordinal is no longer part of that conflict.

Not measured

Nothing was timed. Nothing touched hardware and nothing keyed a transmitter. The byte figures are arithmetic; the allocation deltas are measured on one platform.

🤖 Generated with Claude Code

[aethersdr-agent]

Issue fit

No fixes/closes reference and no linked issue — I checked the PR body and searched; this stands on its own stated intent, which is a performance change to a vendored file plus the test that makes the new lazy path safe. Per GOVERNANCE.md that's fine: it is not an architecture change, a new dependency, or a UX change, and it is a local patch to an already-enumerated vendor patch set. The change does what it says: plan_fircore() builds the minimum-phase workspace only at mp != 0, calc_fircore() builds it on first use, deplan_fircore() tolerates the null, and AETHERSDR-PATCHES.md records it as patch 5 with a drop condition.

Scope

File	What it changes	Claimed?	Verdict
third_party/wdsp/upstream/firmin.c	The patch itself: conditional build in plan_fircore(), new static ensure_minphase(), guard in calc_fircore(), null-tolerant deplan_fircore()	Yes	In scope
third_party/wdsp/AETHERSDR-PATCHES.md	Four→five, patch 5 entry with rationale, counts and drop condition	Yes	In scope — this is the required provenance record (Principle IV)
tests/wdsp_channel_test.cpp	runMinimumPhaseWorkspaceTest, registered in main()'s runLeakChecked chain	Yes	In scope

Everything in the diff is explained by the stated change. No new public surface, no settings, no UI, no CHANGELOG entry (correct — do not add one). No deleted guard: the only - lines in firmin.c are the unconditional create_minphase and the unconditional destroy_minphase, both replaced by strictly more defensive forms. No sibling copies — firmin.c is the only definition of plan_fircore/deplan_fircore in the tree, and pminphase is read in exactly the four places this diff touches (grep pminphase third_party/wdsp/upstream/ returns only firmin.c and the firmin.h struct member).

CI on e3235a35 is green across build / check-macos / check-windows / Static checks / Sanitizer option configures, against base 85e4a1ff which is current main.

Blockers

None.

Nits (non-blocking)

• The ~66.9 MB figure doesn't reconcile with the patch note's own inventory (AETHERSDR-PATCHES.md:121, inline). By the note's own numbers — 104 bytes/element and lengths 131072×3, 65536, 32768×8, 8192×5, 4096 — I get 765,952 elements × 104 = 79.7 MB (76.0 MiB), not 66.9. It's an understatement rather than an oversell, and the note correctly labels it "arithmetic on those counts, not a measurement", but it would be good to show the working or correct it. The allocation counts in the same paragraph (1300→1204 = 96 = 12×8; 1518→1374 = 144 = 18×8; mp==1 at 1252 = 1204+48 = 6×8) are all internally consistent and I could not fault any of them.
• The "plan-construction time first" framing understates the existing wisdom cache (AETHERSDR-PATCHES.md:98, inline). WdspChannel::open() calls loadWisdomOnce() before the first plan and exportWisdomNow() after (src/core/dsp/WdspChannel.cpp:881, :919), so on a warm cache FFTW_PATIENT does not re-measure and the planner cost is largely already amortised. The durable win is the one the counts prove: 12 fewer live objects and tens of MB of resident workspace per RX channel, on every open, on every backend that opens a WdspChannel. That's a good enough case on its own.
• AETHERSDR-PATCHES.md:5 now reads "five fixes", but patch 5 is a performance change, not a correctness fix — the closing paragraph already draws that distinction ("changes no filter output"). "five local changes" or similar would read truer.

What I tried to break

Findings below are reasoned from the code in /tmp/aetherclaude/pr-5697; I have no build or test run here, so nothing was reproduced at runtime.

• Leak on a plan_fircore() that runs with a live workspace already attached. Every caller of plan_fircore() is either create_fircore() (on a malloc0 struct, so pminphase == 0) or is immediately preceded by deplan_fircore() — setBuffers_fircore (firmin.c:471), setSize_fircore (:479), setNc_fircore (:493). Since the patched deplan_fircore() now nulls the pointer after destroying it, plan_fircore() never overwrites a live MINPHASE. plan_fircore is declared in firmin.h but has no caller outside firmin.c. Survives.
• mp flipped to 1 on a path that never reaches calc_fircore() — that would leave mp_imp_exec() dereferencing null on the next call. Every write to a FIRCORE's mp is either create_fircore()'s initialiser or setMp_fircore() (:506), which unconditionally calls calc_fircore(). The module-level a->mp = mp writes in bandpass.c, nbp.c, emph.c, fmd.c, fmmod.c, fmsq.c are on the module struct and each is immediately followed by setMp_fircore()/setMp_nbp(). No path reaches mp_imp_exec() without passing ensure_minphase() first. Survives.
• A stale workspace sized for the wrong nc. ensure_minphase() is a no-op when pminphase != 0, so if nc could change without a deplan/plan cycle you'd get mp_imp_exec() with a mismatched N. nc only changes in setNc_fircore(), which deplans (→ null) and re-plans; pfactor has no setter in this tree. Staleness properties are identical to upstream. Survives.
• Double teardown. Upstream's deplan_fircore() called destroy_minphase(a->pminphase) unconditionally, so a double deplan was a double free; the patched form nulls it and is now the only member of that function that is safe to run twice. Strictly an improvement, not a regression. (crev, accum, fftout etc. are still left dangling — pre-existing, not this PR's problem.)
• New FFTW planning on a thread that didn't do any before. create_minphase() builds four FFTW_PATIENT plans, and planning is not thread-safe. Moving it into calc_fircore() means setMp_fircore() can now plan, which it never could before. In this tree the only caller of RXASetMP is WdspChannel::open() (WdspChannel.cpp:908), which holds g_setupMutex — so no new concurrent-planning site, and setNc_fircore() already planned from the same place. Survives, but it is a new property of setMp_fircore() worth remembering if anything ever starts toggling minimum phase at runtime.
• The test asserting the implementation back to itself. The allocation assertion is the one that has teeth: the body reports that before the patch mp == 1 and mp == 0 both held 1300, so minimumAllocations <= linearAllocations fails on the unpatched library — the test does not pass against the code it is meant to guard. The audio assertion walks the lazy path for real, because open() calls RXASetNC() at line 907 before RXASetMP() at 908, so all six cores are planned at mp == 0 and only then flipped, which is exactly the pminphase == 0 case ensure_minphase() exists for. RXASetMP fans out to six setters (RXA.c:1056) and Config::minimumPhase has no other setter anywhere in src/, so the claim that nothing else exercises mp == 1 holds.
• The "12 of 13 are mp == 0, the exception is eqp" claim. create_fircore call sites confirm the shape: dexp.c:163 is the only other hardcoded mp = 1, and create_dexp is instantiated in neither RXA.c nor TXA.c. eq.c:282 passes a->mp and RXA.c:266 creates it — consistent. I could not independently count all 13 without running the instrumented build; the allocation deltas quoted are self-consistent with it.
• Test runtime. wdsp_channel_test has no TIMEOUT property in tests.cmake:603-605 (default 1500 s) and is noted elsewhere in that file at ~22 s. The new case adds two 8192-tap opens but the patch also removes 18 create_minphase plan-builds from every other open in the file, so I expect this to be net-neutral or faster. I cannot measure it; CI passing at this SHA is the only evidence either way.
• Socket tests: none added, modified or removed. tests/wdsp_channel_test.cpp contains no socket or fake-peer code; it is a socket-free CTest, which is the correct layer for DSP per AGENTS.md's test-layer boundary. No tests.cmake registration change was needed — the target already exists.

Recommendation

Approve with nits. This is a tightly scoped vendor patch with the right shape: the removal is provably unobservable at mp == 0, the one new null it introduces is closed by ensure_minphase() at the single site that dereferences it, and the accompanying test is the first thing in the tree to enter the mp == 1 branch — it would fail both against the unpatched library (the allocation relation) and against a botched lazy build (the audio assertion). The three nits are all in AETHERSDR-PATCHES.md and none of them change the code. Next step: fix or show the working for the MB figure, and a maintainer should confirm they're happy carrying a performance divergence in the vendored tree alongside the four correctness ones — that's a policy call, not a code defect, and the note's drop condition is already written.

Nice work on the provenance section — counting the cores rather than assuming them, and separating the measured counts from the derived bytes, is what made this reviewable without a build.

---

_{🤖 aethersdr-agent · cost: $7.9734 · model: claude-opus-5}

[aethersdr-agent]

Antares Detector — candidate vulnerable file(s)

• src/core/WaveformInstaller.cpp

The ssdr loader reads packet lengths from untrusted network input and uses them to allocate or copy memory (e.g., buildVitaTxPacket in src/core/AudioEngine.cpp) without validating the length field, enabling buffer-overrun vulnerabilities.

_{Localized by Cisco Foundation AI Antares-1B running locally in the AetherClaude sandbox, seeded by the Cartographer security map. Advisory only — please verify before acting.}

---

_{🤖 aethersdr-agent · cost: $8.1307 · model: claude-opus-5}

[ten9876]

Issue fit

The PR body's own framing is the right one: this is a self-standing performance change to a vendored file, plus the test that makes the new lazy path safe. Per GOVERNANCE.md that needs no prior RFC — it is not an architecture change, a new dependency, or a UX change.

The change does what it says, and I verified it rather than taking it on faith. plan_fircore() builds the workspace only at mp != 0, calc_fircore() builds it on first use, deplan_fircore() tolerates the null, and AETHERSDR-PATCHES.md records it as patch 5 with a drop condition. The C change itself is sound — see "What held up" below for the attacks it survived.

But the PR is currently linked to close #5456, which its own body says in bold it does not fix. That is blocker 1, and it is the one that does real damage at merge.

Scope

File	What it changes	Claimed?	Verdict
third_party/wdsp/upstream/firmin.c (+27/-2)	Conditional build in plan_fircore(), new static ensure_minphase(), guard in calc_fircore(), null-tolerant deplan_fircore()	Yes	In scope
third_party/wdsp/AETHERSDR-PATCHES.md (+113/-5)	four→five, patch 5 entry, reconciled MB tables, thread-safety caveat, rewritten refresh instruction	Yes	In scope (Principle IV provenance record) — but the rewritten refresh instruction introduces a factual error, blocker 3
tests/wdsp_channel_test.cpp (+114/-0)	runMinimumPhaseWorkspaceTest, registered in main()'s runLeakChecked chain	Yes	In scope

All three commits are authored 2026-09-14 (authoredDate); no date outlier, no commit unrelated to the stated change. No new public surface, no settings key, no UI, no CHANGELOG entry (correct — do not add one).

No deleted guard. The only - lines in firmin.c are the unconditional create_minphase and the unconditional destroy_minphase, both replaced by strictly more defensive forms. pminphase is read in exactly the four places this diff touches, and firmin.c holds the only definition of plan_fircore/deplan_fircore in the tree — there is no sibling copy left unfixed.

What is missing from the diff is the finding. This PR changes the count of local divergences from four to five, and two other documents still state four. That is blocker 2.

Blockers

1. The PR is set to auto-close #5456, which the body explicitly disclaims.

The body says, in bold: "This does not fix #5456. It is a candidate mechanism for the cold-connect times recorded there, and connecting the two needs a cold-wisdom connect-path timing that nobody has taken. I am not claiming it."

That restraint is exactly right, and I agree with it. But GitHub disagrees:

$ gh api graphql -f query='{repository(owner:"aethersdr",name:"AetherSDR"){
    pullRequest(number:5697){closingIssuesReferences(first:10){nodes{number title state}}}}}'
{"data":{"repository":{"pullRequest":{"closingIssuesReferences":{"nodes":[
  {"number":5456,
   "title":"docs/HERMES.md §22.3's cold FFTW planning figures do not reproduce: ...",
   "state":"OPEN"}]}}}}}

There is no fixes/closes keyword in the body, so this was linked manually in the Development sidebar. Either way it is the metadata GitHub acts on: merging this PR closes #5456.

#5456 is an open, maintainer-review-labelled documentation issue about docs/HERMES.md §22.3's cold-planning magnitudes being 5–19× out, measured against a documented 20.2 s cold connect that reproduced at 257.86 s. This PR touches no documentation in docs/ at all, and by its own account does not establish the causal link. Merging it would silently retire an unresolved measurement discrepancy.

Principle XI frames a Fixes #NNNN as "the implementing agent's hypothesis". The prose here correctly declines to make that hypothesis; the metadata makes it anyway. Unlink #5456 from the Development sidebar (a plain refs #5456 mention in the body carries the context without the auto-close), and the PR's stated position and its behaviour will agree.

2. Two documents still say the snapshot carries four local changes.

The diff updates AETHERSDR-PATCHES.md:5 to "five local changes", but leaves both other places that state the count:

• third_party/wdsp/README.md:23-24 — "upstream/ matches that source snapshot except for the four fixes recorded in AETHERSDR-PATCHES.md — three teardown corrections and one use-after-free". After this PR there are five, and the fifth is neither a teardown correction nor a use-after-free.
• docs/architecture/wdsp-integration.md:50 — "The source snapshot has four documented corrections."

This is not cosmetic. README.md is the boundary statement a maintainer reads at refresh time — the document whose whole job is to enumerate what diverges from upstream. Someone refreshing WDSP reads "four", verifies four, and drops the fifth on the floor. That is precisely the failure the AETHERSDR-PATCHES.md procedure exists to prevent, and this PR is the change that invalidates the claim. Both lines need the same edit as AETHERSDR-PATCHES.md:5. Inline comment on that line.

3. The rewritten refresh instruction mischaracterises patch 3, and the old text did not.

AETHERSDR-PATCHES.md:197-198 (added by this diff) now reads "the equivalent frees for patches 1-3". Patch 3 is not a free. This same document, at lines 12-31, describes it as a->pfcimp = build_fcimp (...) — a discarded return pointer causing a use-after-free on a live path, fixed by an assignment, and reported as TAPR/OpenHPSDR-wdsp#2. A future refresher grepping a new upstream release for an added _aligned_free will conclude the fix is absent when it has landed, or reapply a patch upstream already carries.

The text this replaced ("first check whether upstream contains equivalent frees") was vague; the new text is specific and wrong, which is worse. Suggestion inline.

Nits (all explicitly non-blocking)

• The PR body still leads with the argument the patch note retracts. The body's second heading is "## The cost is plan construction, not bytes" — "that is where the seconds go, and it is why this is worth more than its allocation figure suggests." Commit c6da5cc9c then added, at AETHERSDR-PATCHES.md:109: "The standing cost is the resident workspace, not the planning. An earlier draft of this note led with plan-construction time; that overstated it ... on a warm cache FFTW_PATIENT imports instead of re-measuring." The note was corrected; the body was not. Since this is a discretionary vendor divergence with its own drop condition, the maintainer deciding whether to carry it reads the body — and gets the superseded case as the headline. Worth the same edit the note received.
• setMp_fircore() can now plan on a channel that is running, not just outside a mutex. The note (AETHERSDR-PATCHES.md:88-95) records the thread-safety half of this. The other half is that RXASetNC() brackets its work in SetChannelState(channel, 0, 1) (RXA.c:1045,1052) and RXASetMP() (RXA.c:1056-1064) does not. Today that is harmless — WdspChannel::open() calls SetChannelState(m_channelId, 1, 0) at line 926, after RXASetMP() at 908 — but a future runtime toggle would build four FFTW_PATIENT plans at up to 131072 points with audio flowing, i.e. a cold-cache stall mid-stream, not only a data race. Same line to revisit; worth one clause in the same paragraph. Inline.
• Teardown is not symmetric with construction. setMp_fircore(a, 0) leaves a->pminphase allocated until the next deplan_fircore(). No effect today (the only caller passes the config value once, at open, when nothing has been built yet), but it means the saving is one-way if minimum phase ever becomes togglable. Worth a sentence in the drop-condition paragraph.
• The test cannot tell "minimum phase applied" from "workspace built, output discarded." It asserts both modes produce energy > 1e-4 and that minimum phase costs strictly more allocations. Neither fires if mp_imp_exec() runs and its result is silently ignored — the allocation relation proves objects were created, not used. A minimum-phase filter has markedly different group delay, so the two outputs must differ; maximumDifference() already exists at tests/wdsp_channel_test.cpp:47 and both buffers are in hand. One assertion closes it. Inline.
• AetherSDR patch 5 is hardcoded in five places (firmin.c:320,331,347,390 and tests/wdsp_channel_test.cpp:413) while the body itself notes the number becomes 7 if #5628 lands first. Merge order alone would make five in-tree comments cite the wrong registry entry. Anchoring on the change instead — "AetherSDR: minimum-phase workspace is built lazily — see AETHERSDR-PATCHES.md" — drops the ordering dependency. Inline.
• toneRms receives a sum, not an RMS (tests/wdsp_channel_test.cpp:488): energy += rms(outputLeft) over 40 blocks, then *toneRms = energy. Compared against a fixed 1e-4 floor, so harmless now, but it scales with the block count. toneEnergy matches the arithmetic. Inline.
• The #5628 conflict is on two files, not one. The body flags AETHERSDR-PATCHES.md. #5628 also inserts after runNotchAttenuationTest() at tests/wdsp_channel_test.cpp:404 and appends to the same main() runLeakChecked chain — both PRs collide there too, and both claim the number "5". Worth naming in the body so whoever rebases second knows the full surface.
• No commit subject cites a principle, which AGENTS.md:95-96 asks for ("Commit messages cite the most-load-bearing principle as Principle <N>. at the end of the subject line"). Principle VIII would fit. Genuinely minor: the squash-merge subject is the maintainer's to compose, and main's own history is inconsistent here — #5686, the WDSP 2.10 refresh this patch sits on, carries no principle either.

What held up

I tried to break this and mostly could not. The claims I tested:

1. "It goes red with the patch reverted (linear=2069 minimum=2069)" — reproduced exactly. Reverted firmin.c alone to the merge base, rebuilt, ran: FAIL: minimum phase cost no extra WDSP allocations (linear=2069 minimum=2069). Byte-for-byte the figure in the body. The regression test pins something real.
2. "Getting the laziness wrong is a null dereference inside mp_imp_exec()" — reproduced. I neutered ensure_minphase() to a no-op on the patched tree; wdsp_channel_test exits 139 (SIGSEGV). So the new test genuinely walks the lazy-construction path, and the laziness is load-bearing exactly as claimed.
3. The counted inventory reproduces on a second architecture. The body's figures are macOS arm64. On Linux x86-64 (Arch, RelWithDebInfo) the same test yields patched linear=1973 minimum=2021 against reverted 2069/2069 — i.e. 96 fewer allocations held at mp == 0 (12 cores × 8) and 48 fewer at mp == 1 (6 × 8). That is the body's "exactly 12 fewer objects held" and "mp == 1 holds 1204+48", arrived at independently on different hardware.
4. ASan on Linux, which the body says has not signed this off — it does now. -DAETHERSDR_SANITIZER=address, detect_leaks=1: no new finding on the minimum-phase path. The run does report 1120 bytes in 10 allocations, but I built the merge base with the same 3 files reverted and it reports the identical leak at the identical site (createWaitable ← create_calcc ← create_txa, calcc.c:1079), reached from the pre-existing runVector test. Pre-existing, not this PR, and worth its own issue.
5. Every plan_fircore() caller checked for a stale or null workspace. All four (create_fircore, setBuffers_fircore, setSize_fircore, setNc_fircore) are deplan → plan → calc, and create_fircore assigns a->mp before plan_fircore(a). setNc_fircore deplans before mutating a->nc, so ensure_minphase() can never size against a stale nc. I could not construct a path that reaches mp_imp_exec() with a null or wrongly-sized pminphase.
6. One refutation, in the PR's favour. I checked whether the note's TX figures describe an unreachable configuration, since WdspChannel::open()'s TX branch never calls TXASetNC/TXASetMP and nothing in the tree sets Config::minimumPhase. They are reachable: TXA.c builds its bandpass cores through create_bandpass → create_fircore(..., a->mp, 16, impulse) with the flag fixed at construction, so the ~16.2 MB saving lands on every TX open regardless. The note is right.
7. Considered and rejected: collapsing the ternary. a->pminphase = 0; in plan_fircore() would be exactly equivalent, since all four callers reach calc_fircore() before anything reads the pointer. I think the author's a->mp ? create_minphase(...) : 0 is the better call anyway: it keeps the mp == 1 path byte-identical to upstream, which is the right instinct in a vendored file.

Not driven through the app. This is a headless DSP change with no user-visible surface — nothing in the tree sets Config::minimumPhase, so there is no control to click and no pixel to grab. A bridge session against the demo backend would have added nothing the allocation counters and the two mutations did not already settle. Windows ASan not run (no Windows box here); AETHERSDR-PATCHES.md asks for every supported platform, and macOS + Linux are now covered.

CI is green on the current head 079f7463 across all five checks (build, check-macos, check-windows, Static checks, Sanitizer option configures).

---

The three blockers are four lines of edits between them and none of them is in the C. The patch is good work — the counted inventory, the mutation-proof test, and the drop condition are all the right shape, and the note's own self-correction on plan time versus resident memory is the kind of thing most PRs would have quietly left standing.

🤖 Generated with Claude Code

[on8st]

Three blockers, all correct, all cleared. Rebased onto main @ 26998942; head is aa3e9163 and the PR is mergeable again.

Blocker 1 — your finding is right, but the cause is not the one in your review, and that matters. There was never a Development-sidebar link. Two proofs: the PR timeline contains no ConnectedEvent (a manual link always emits one), and I cleared the reference by editing only the body, which would not have touched a sidebar link.

The link came from our own disclaimer. The body read "This does not fix" immediately followed by the issue number. GitHub's closing-keyword parser does not read negation — it matched the keyword against the number that followed it and set the PR to close on merge. The sentence disclaiming the link is the sentence that created it.

I am flagging this rather than just fixing it because your remedy — unlink it in the sidebar — would have sent the next person to a sidebar with nothing in it, and the auto-close would have survived. The fix is to keep the reference away from fix / close / resolve. The body now reads Related: #5456 — this PR does not establish that link, and closingIssuesReferences is empty. My first draft of the explanation quoted the old sentence verbatim and re-created the link; the post-edit keyword scan caught it. The trap is easy to fall into twice.

Blocker 2 — fixed, and the count was wrong in one more place than you found. All three statements now say seven. docs/architecture/wdsp-integration.md was already one short on main before this branch: #5687 added two entries and bumped README.md four→six, but wdsp-integration.md four→five. So your blocker landed on your own PR four hours after you wrote it. That is not a dig — it is the best possible argument that the problem is structural rather than anyone's carelessness. Corrected here.

Blocker 3 — fixed, and wider than patch 3. You are right that "the equivalent frees for patches 1-3" is specific and wrong, which is worse than vague. It was also wrong for 4 through 7 — an exit handshake, an accessor set, a null guard and a conditional construction are none of them frees. The paragraph is now a shape-per-patch list, with patch 3 named as the a->pfcimp = build_fcimp (...) assignment and an explicit "not a free, and searching for one will miss it in both directions."

Your nit predicted the renumber, so I have taken it. This entry is patch 7 now, not 5 — #5687 took 5 and 6 while the branch was in review. You wrote that merge order alone would leave five in-tree comments citing the wrong registry entry; four hours later merge order did exactly that. The five markers in firmin.c and tests/wdsp_channel_test.cpp no longer carry an ordinal: they read // AetherSDR: the minimum-phase workspace is built lazily -- see AETHERSDR-PATCHES.md. grep AetherSDR still finds all five, and the number now lives in exactly one place. Patch 4's ten markers have the same exposure; left alone as out of scope.

An observation, not a request. Four separate conflict waves hit our branches today and every one landed in one of two files. Measured: docs/architecture/aetherd-touchpoints.md — 4 open PRs across 2 authors, 3 of them conflicting, plus 6 merged in the last 48 hours. third_party/wdsp/AETHERSDR-PATCHES.md — 2 open PRs plus 2 merged today. Both carry a running count in prose, so any change to either conflicts with any other. The touchpoint manifest is additionally generated, committed and verified in static-checks.yml with --check, so a stale copy fails CI and a fresh one conflicts — there is no third option for two concurrent PRs. Detail and a suggested remedy are on #5642.

You have been on both sides of this in one day: blocker 2 warned about a stale count, and #5687 moved it. That is the system working on you, not you working badly.

Full ctest: 449 tests, 447 ran, 446 passed, 1 failed. wdsp_channel_test passes.

• tgxl_docked_parity_test — deterministic, 7 of 7. QFontMetrics width check, missing "Sans Serif" under offscreen QPA. Link edge carries neither libaethercore.a nor libaether_wdsp.a; sole commit 43687413 (feat(tuner): TGXL front-panel presentation, tune abort, and tuner alerts #5676).
• vkamp_connection_test — 2 failures in 7 runs. A time-boxed socket assertion. Same link edge.

A build break I hit on the way, now filed as #5706: aethercore does not compile with ENABLE_DFNR=OFF, because #5687 put the NNR includes inside #ifdef HAVE_DFNR while their users are unconditional. Reproduced independently by two of my runs on different branches. Not CI's miss — no lane builds that configuration at all.

Not measured: nothing was timed, nothing touched hardware, nothing keyed a transmitter. Byte figures are still arithmetic; allocation counts are still macOS arm64 only. Your Linux x86-64 reproduction and your Linux ASan run remain the only Linux evidence, and thank you for both — the SIGSEGV mutation in particular is the check I could not run myself.

🤖 Generated with Claude Code

[on8st]

Cross-reference: this PR and #5628 edit the same two files — third_party/wdsp/AETHERSDR-PATCHES.md and tests/wdsp_channel_test.cpp — so whichever lands first displaces the other's patch number and forces a second rebase. That already happened once today when #5687 landed two patches mid-review and both branches renumbered from 5/6 to 7/8/9.

The grouping, and two open issues that sit in the same area, are laid out on #5628 rather than repeated here.

🤖 Generated with Claude Code

[on8st]

@ten9876 — the blocker is answered and two cosmetic threads are still open; I
will reply to those separately. Flagging only because the review state now
predates the branch head.

🤖 Generated with Claude Code

[on8st]

@ten9876 — this one is answered and rebased, and the only thing holding it is a review state no action of ours reaches.

All twelve unresolved threads end with our reply, posted 2026-09-14. The two you raised last round are both taken: the RXASetMP asymmetry is now its own clause in patch 4's registry entry rather than a widening of the thread-safety sentence — RXASetNC brackets its work in SetChannelState and RXASetMP does not, and setMp_fircore contains no teardown, so the saving is one-way per fircore. And toneRms was a sum of forty per-block RMS values rather than an RMS; you were right and the name was what misled.

Rebased onto main today at fd49a4dd, after #5628 landed. Two things in that rebase are worth a reviewer's eye rather than a glance, and I would rather point at them than have them found:

• The divergence count went seven → ten, and the minimum-phase patch is now entry 10, not 7. Commit 24c3b387 applied cleanly while writing "seven" into third_party/wdsp/README.md and docs/architecture/wdsp-integration.md — a clean apply hiding stale content, which is exactly the shape you flagged on fix(hl2): make the ALC reduction-only and widen the mic slider (#5463 change 3) #5646's Hl2TxLevelPolicy.h. Corrected in its own labelled commit.
• I wrote three new bullets for patches 7, 8 and 9 in the "shape per patch" refresh list, summarised from main's own entries. That is authored prose inside a rebase. It is the item I would most want checked, because a rebase is not supposed to contain new sentences.

The marker convention is the one exception I did not resolve: #5628 established // AetherSDR patch N:, and the minphase change to firmin.c uses an unnumbered // AetherSDR:. I noted it in the registry rather than editing vendored source, on the grounds that renumbering a marker is a change to the snapshot and this PR has no business making one.

Not measured: nothing on hardware. wdsp_channel_test passes on the rebased head; the only failure in the full suite is tgxl_docked_parity_test, the #5676 font assertion.

No rush on my side — flagging it because the review state, not the code, is what it is waiting on.

🤖 Generated with Claude Code

[jensenpat]

Issue fit

Self-standing WDSP performance patch. No closing-keyword link; closingIssuesReferences is empty. Related mention of #5456 is correctly kept away from fix/close/resolve. Scope is the lazy create_minphase() path plus registry/docs/test. Shared DSP; default Config::minimumPhase remains false, so live backends keep linear-phase audio.

Group	Role
third_party/wdsp/upstream/firmin.c	Guard plan_fircore; ensure_minphase in calc_fircore; null-safe deplan_fircore
tests/wdsp_channel_test.cpp	First minimumPhase=true case; allocation relation + energy floor
AETHERSDR-PATCHES.md / README / wdsp-integration.md	Patch 10 registry; count ten

Findings

No blockers.

Prior ten9876 blockers (auto-close wording, stale patch counts, ordinal-in-comments) are addressed on this SHA. Remaining items from that review are nits: toneRms is still a 40-block energy sum; the test does not assert the two streams differ; setMp_fircore(a,0) does not release the workspace. None of those reintroduce the dead workspace on the default path.

Verification

• Socket preflight: wdsp_channel_test has no Qt sockets / fake radio peers. Registered in tests/tests.cmake.
• Local RelWithDebInfo Ninja at fd49a4dd: wdsp_channel_test passed (7.29 s, then 7.24 s after restore).
• Mutation: unconditional create_minphase in plan_fircore → FAIL: ... (linear=2069 minimum=2069) — same figure as the PR body. Restored; test passed again.
• CI on fd49a4dd: build, check-macos, check-windows, Static checks, Sanitizer option configures — all success.
• App not built (DSP-only target). No hardware / no TX.

Verdict: Approve. Merge authorized if live gates pass.

Addressed on fd49a4d: closing-keyword trap, patch count, and ordinal comments. Remaining items are nits; CODEOWNER approved current head.