Skip to content

[aethercore] Vendor WDSP 2.00 for Hermes-Lite RX#4278

Open
jensenpat wants to merge 1 commit into
aethersdr:mainfrom
jensenpat:aether/vendor-wdsp-hl2
Open

[aethercore] Vendor WDSP 2.00 for Hermes-Lite RX#4278
jensenpat wants to merge 1 commit into
aethersdr:mainfrom
jensenpat:aether/vendor-wdsp-hl2

Conversation

@jensenpat

Copy link
Copy Markdown
Collaborator

Summary

  • vendor the exact TAPR/OpenHPSDR-wdsp 2.00 source at commit 584e8aca5ba1c4c6bc66fc0cc164ce567c8ba1e3
  • build WDSP as the isolated aether::wdsp static library, linked privately only by aethercore
  • add a narrow portability boundary for threads, synchronization, aligned allocation, exports, and SIMD flush-to-zero behavior
  • hide WDSP's process-global numeric channel table behind the non-copyable RAII WdspChannel C++ owner
  • keep channel construction, FFT planning, allocation, reconfiguration, and teardown on the control path; processIq() is the fixed-buffer real-time entry point
  • add deterministic RX and TX vectors plus underrun, reconfiguration, concurrent-ID, callback-allocation, and repeated-teardown coverage
  • update the Hermes-Lite 2 design around a complete backend-owned WDSP RX channel rather than mixing individual WDSP stages into AudioEngine

Why

Hermes-Lite 2 sends raw IQ rather than radio-demodulated audio. The aetherd backend therefore needs an engine-side modulation chain with explicit ownership and real-time constraints. This PR establishes that foundation before the live Metis RX path arrives: one complete WDSP RX channel, contained below the radio seam and exposed through a narrow C++ boundary.

The planned live path is:

Metis UDP RX -> bounded IQ queue -> HL2 DSP worker -> WDSP RX channel -> PCM outlet

A read-only spectrum tap may observe IQ, but it does not participate in the audio DSP chain. Rate changes will prebuild a replacement channel off the real-time thread and swap it at a block boundary.

Safety and scope

  • No UI or current radio behavior changes.
  • No Hermes-Lite network code is enabled by this PR.
  • No path can key hardware; live TX remains deferred and canTransmit remains false.
  • The TX channel exists only to validate the ownership boundary and future audio-to-IQ direction deterministically.
  • The optional upstream binary calculus table and PDF are intentionally omitted.

The lifecycle tests found three WDSP 2.00 allocation leaks: the notchdb owner, NURBS owners, and a temporary CFIR transition table. The three minimal fixes are documented in third_party/wdsp/AETHERSDR-PATCHES.md for future upstream refreshes.

Validation

  • cmake --build build -j8
  • ./build/wdsp_channel_test
  • ctest --test-dir build -R '^wdsp_channel_test$' --output-on-failure
  • python3 tools/check_engine_boundary.py --strict — zero blocking findings
  • git diff --check
  • compared the imported snapshot against the pinned upstream tree; only the omitted optional binary and three documented teardown fixes differ
  • signed commit verified with git log --show-signature -1

Validated locally on macOS ARM. Windows and Linux portability paths are included for CI coverage; the multi-platform performance matrix remains follow-up work after live RX correctness.

Screenshots

Not applicable — this is an engine-only dependency and DSP ownership boundary with no visible UI change.

👨🏼‍💻 Generated with OpenAI Codex (GPT-5.6 Sol) and tested by @jensenpat

@jensenpat jensenpat self-assigned this Jul 17, 2026
@jensenpat jensenpat changed the title Vendor WDSP 2.00 for aethercore RX [aethercore] Vendor WDSP 2.00 for Hermes-Lite RX Jul 17, 2026
@jensenpat
jensenpat marked this pull request as ready for review July 17, 2026 02:27
@jensenpat
jensenpat requested review from a team as code owners July 17, 2026 02:27
Comment thread third_party/wdsp/upstream/emnr.c Dismissed
Comment thread third_party/wdsp/upstream/emnr.c Dismissed
@jensenpat
jensenpat marked this pull request as draft July 17, 2026 16:49
@jensenpat

Copy link
Copy Markdown
Collaborator Author

Holding for next week's release

@jensenpat
jensenpat marked this pull request as ready for review July 19, 2026 04:28

@aethersdr-agent aethersdr-agent Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Really nice foundational PR. The vendoring boundary is clean: WDSP stays private to aethercore, the Win32/POSIX shim is contained, and the WdspChannel RAII owner with its Dekker-style m_controlOperation/m_callbacksInFlight handshake between processIq() and the control operations is a careful, correct design. Mode enum ↔ RXA_*/TXA_* ordinals line up (and TX+WBFM is validated out, which is right since TXA has no WBFM slot). Test coverage — leak checking, underrun, reconfig, concurrent IDs, callback-alloc, repeated teardown — is exactly what this layer needs. No blocking issues; everything below is forward-looking.

Since all changes are behind the radio seam with no live path wired, I'd merge this once the notes are considered.

Non-blocking notes

  • processIq() allocation guard reads a process-global sequence, so once you wire the planned prebuild-and-swap rate change, a replacement channel's OpenChannel on another thread will bump the same counter and trip a spurious AllocationViolation on a live channel. Worth a per-channel scheme (or documenting the constraint) before the live path lands.
  • Vendored WDSP makes FFTW3 a hard configure-time requirement for the entire build, removing the previous NR2 soft-fallback. Confirm every build/CI env provides it.
  • Destructor busy-waits on in-flight callbacks with no pause hint — fine given the documented contract, minor polish.
  • The CodeQL emnr.c overflow alerts are on vendored upstream under third_party/**, which .github/codeql/codeql-config.yml already declares out of scope. Keeping the snapshot pristine is the correct call and consistent with repo policy — these can be dismissed rather than patched (CodeQL's paths-ignore is unreliable for compiled C/C++, which is why they still surfaced).

🤖 aethersdr-agent · cost: $3.4770 · model: claude-opus-4-8

Comment on lines +112 to +122
const uint64_t allocationsBefore = wdspPortAllocationSequence();
int wdspError = 0;
fexchange2(m_channelId,
const_cast<float*>(inputI.data()),
const_cast<float*>(inputQ.data()),
outputLeft.data(), outputRight.data(), &wdspError);
const uint64_t allocationsAfter = wdspPortAllocationSequence();
m_callbacksInFlight.fetch_sub(1, std::memory_order_release);

if (allocationsAfter != allocationsBefore) {
return ProcessResult::AllocationViolation;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

wdspPortAllocationSequence() is a single process-global counter, so this only reliably detects RT-path allocations when nothing else touches WDSP concurrently. The PR body describes the future live path as "prebuild a replacement channel off the real-time thread and swap it at a block boundary" — that prebuild's OpenChannel (control path, allocates heavily) runs on another thread and increments the same global sequence, which would make this comparison report AllocationViolation on an unrelated live channel that did nothing wrong.

Not a problem today since no live path feeds processIq(), but before that lands consider a per-channel allocation counter, or gating the guard so it only arms when no control operation is in flight on any channel. Flagging now so the swap design accounts for it.

WdspChannel::~WdspChannel()
{
m_controlOperation.store(true, std::memory_order_release);
while (m_callbacksInFlight.load(std::memory_order_acquire) != 0) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Tight spin with no relax hint. The contract ("caller must stop feeding processIq() before a control operation") means this is normally already 0, so this is pure polish, but a std::this_thread::yield() in the loop body avoids a pathological busy-burn if a stray processIq() is still unwinding during teardown.

cmake_minimum_required(VERSION 3.25)

if(NOT FFTW3_FOUND)
message(FATAL_ERROR "WDSP 2.00 requires FFTW3")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Because aethercore links aether::wdsp unconditionally and add_subdirectory() processes this file at configure time regardless of EXCLUDE_FROM_ALL, this FATAL_ERROR now makes FFTW3 mandatory for the whole build — the previous NR2 behavior only warned and fell back to an internal FFT when FFTW was absent. That's a reasonable tightening, just worth confirming every dev/CI environment (esp. Linux without libfftw3-dev) has it, since the failure mode moves from a soft warning to a hard configure error for everyone, including builds that never touch HL2.

@aethersdr-agent aethersdr-agent Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for the exceptionally disciplined vendoring here, @jensenpat. The isolation is textbook: aether::wdsp is a private static lib linked only by aethercore, the Win32 surface is confined to a single documented port boundary, the three upstream teardown leaks are captured in AETHERSDR-PATCHES.md rather than smeared through the snapshot, and the RAII WdspChannel owner keeps all allocation/planning on the control path with a genuinely allocation-free processIq(). I verified the direct Modeint cast against the vendored RXA.h/TXA.h: RX modes 0–12 line up exactly (WBFM=12), TX diverges at 12 (AM_LSB), and validateConfig correctly rejects TX+Wbfm — nicely defensive. The deterministic RX/TX/underrun/reconfig/leak coverage is a real asset. canTransmit stays false and no network path is enabled, so scope matches the description.

On the Copilot/CodeQL flags at emnr.c:388-389 (int multiplication widened to unsigned long): these are valid observations but land in pristine vendored upstream, where the multiplicands are small FFT-sizing ints — no practical overflow. Deferring them per your AETHERSDR-PATCHES.md discipline is the right call; patching upstream math outside a documented minimal fix would undermine the "pinned snapshot" guarantee. No action needed there.

Non-blocking notes:

  • processIq() allocation guard uses a process-global counter — spurious AllocationViolation possible once a second channel runs concurrently. Latent only; one channel is wired today.
  • add_subdirectory(third_party/wdsp) makes FFTW a hard configure-time requirement across all platforms (previously optional with an NR2 fallback). Worth guarding or documenting.

Nothing blocking from me — this is a strong foundation.


🤖 aethersdr-agent · cost: $4.1409 · model: claude-opus-4-8

Comment on lines +112 to +121
const uint64_t allocationsBefore = wdspPortAllocationSequence();
int wdspError = 0;
fexchange2(m_channelId,
const_cast<float*>(inputI.data()),
const_cast<float*>(inputQ.data()),
outputLeft.data(), outputRight.data(), &wdspError);
const uint64_t allocationsAfter = wdspPortAllocationSequence();
m_callbacksInFlight.fetch_sub(1, std::memory_order_release);

if (allocationsAfter != allocationsBefore) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The allocation-freeness check reads wdspPortAllocationSequence(), which is a process-global counter shared across every WdspChannel and every thread. With a single RX channel (today's wiring) this is fine, but the class explicitly supports 32 channels and you test concurrent IDs — so once a second channel is live, a control-path allocation on channel B (e.g. a reconfigure()/OpenChannel on another thread) bumps the global sequence while channel A is mid-fexchange2(), and A will spuriously return AllocationViolation and drop a valid audio block.

Since the goal is to detect allocation by this channel's own fexchange2() call, a per-instance counter (or a thread-local delta captured around the call) would be race-free. Non-blocking given only one channel is active now, but worth tightening before the multi-channel path lands.

Comment thread CMakeLists.txt

# WDSP 2.00 is an engine-only dependency for raw-IQ radio backends. Its C API,
# platform shims, and FFTW dependency remain private to aethercore.
add_subdirectory(third_party/wdsp EXCLUDE_FROM_ALL)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This add_subdirectory is unconditional, and third_party/wdsp/CMakeLists.txt does if(NOT FFTW3_FOUND) message(FATAL_ERROR ...). Combined with the unconditional target_link_libraries(aethercore PRIVATE aether::wdsp), this promotes FFTW from optional-with-fallback (the old NR2 will use fallback FFT warning path) to a hard configure-time requirement on all platforms — a machine without FFTW that used to configure and build now fails at CMake configure.

That's a reasonable consequence since WDSP genuinely needs FFTW, but it's a behavior change worth making deliberate: either guard this on if(FFTW3_FOUND) (and skip the raw-IQ backend when absent), or update the build docs to state FFTW is now mandatory. CI runners provide it, so this won't block here.

@ten9876

ten9876 commented Jul 19, 2026

Copy link
Copy Markdown
Collaborator

First-pass triage review (large vendored-dependency PR — focusing on provenance/licensing/build, not the vendored source line-by-line):

Blockers

  • Licensing / provenance: clean — no blocker. This is model vendoring hygiene. third_party/wdsp/LICENSE (GPL-2.0), COMMIT (pinned 584e8ac…), README.md (upstream URL, revision, imported path, refresh procedure), and AETHERSDR-PATCHES.md are all present, and THIRD_PARTY_LICENSES carries a full WDSP entry. Compatibility checks out: the project is GPL-3.0 and WDSP is GPL-2.0-or-later, so the "or-later" clause makes it GPL-3 compatible. Nothing to fix here.
  • Unconditional build + FFTW3 becomes a hard requirement — please confirm this is intended. add_subdirectory(third_party/wdsp EXCLUDE_FROM_ALL) is paired with target_link_libraries(aethercore PRIVATE aether::wdsp), and since aethercore always builds, the EXCLUDE_FROM_ALL is effectively moot — every build now compiles ~206k lines of WDSP, including for pure-FLEX users who will never touch Hermes-Lite. More importantly, third_party/wdsp/CMakeLists.txt does if(NOT FFTW3_FOUND) message(FATAL_ERROR "WDSP 2.00 requires FFTW3"), whereas FFTW3 was previously optional (NR2 fell back to an internal FFT). The root CMakeLists.txt still only emits a WARNING when FFTW3 is missing on Windows — so a Windows configure without setup-fftw will warn-and-continue straight into the WDSP subdir's FATAL_ERROR. That's a default-build break for anyone who hasn't fetched FFTW3. Either gate WDSP behind an option (an aetherd/HL2 feature flag) or make FFTW3 a documented hard prereq and fix the root Windows path so it fails cleanly rather than warning.

Design questions

  • Dead-vendored ahead of its consumer. WdspChannel and aether_wdsp.h are referenced only by WdspChannel.cpp and wdsp_channel_test — no runtime path uses the channel yet (confirmed; the PR is upfront that this lands "before the live Metis RX path arrives"). Reasonable as staged foundation, but it does land a large unused code + security surface ahead of the code that exercises it. Worth a maintainer call on whether to land the consumer in the same PR or accept the staging.
  • In-file GPL change notices. Three upstream files are modified (nbp.c, nurbs.c, cfir.c teardown leak fixes), documented centrally in AETHERSDR-PATCHES.md — good. GPL-2 §2(a) also asks that modified files themselves carry a notice of the change and date; those three files don't appear to. Consider a one-line marker in each for strict compliance.
  • Vendor vs. submodule. Vendoring is consistent with project convention (rnnoise/r8brain/etc. are all in-tree), and since three local patches are required a submodule would be awkward. The pinned COMMIT + documented refresh procedure is a solid answer — I'd keep vendoring.
  • Branch is 20 commits behind main (merge-base predates the v26.7.3 release; the branch still reads VERSION 26.7.2). The CMakeLists test-block region has churned heavily on main since — a rebase before merge would avoid a likely conflict there and confirm CI runs against current main.

Provenance and licensing are in great shape; the one thing I'd want resolved before merge is the unconditional-build / hard-FFTW3 question. Not a line-by-line review of the vendored tree.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants