[aethercore] Vendor WDSP 2.00 for Hermes-Lite RX#4278
Conversation
|
Holding for next week's release |
There was a problem hiding this comment.
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'sOpenChannelon another thread will bump the same counter and trip a spuriousAllocationViolationon 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.coverflow alerts are on vendored upstream underthird_party/**, which.github/codeql/codeql-config.ymlalready 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
| 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; |
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
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") |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 Mode→int 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 — spuriousAllocationViolationpossible 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
| 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) { |
There was a problem hiding this comment.
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.
|
|
||
| # 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) |
There was a problem hiding this comment.
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.
|
First-pass triage review (large vendored-dependency PR — focusing on provenance/licensing/build, not the vendored source line-by-line): Blockers
Design questions
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. |
Summary
584e8aca5ba1c4c6bc66fc0cc164ce567c8ba1e3aether::wdspstatic library, linked privately only byaethercoreWdspChannelC++ ownerprocessIq()is the fixed-buffer real-time entry pointAudioEngineWhy
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 outletA 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
canTransmitremains false.The lifecycle tests found three WDSP 2.00 allocation leaks: the
notchdbowner, NURBS owners, and a temporary CFIR transition table. The three minimal fixes are documented inthird_party/wdsp/AETHERSDR-PATCHES.mdfor future upstream refreshes.Validation
cmake --build build -j8./build/wdsp_channel_testctest --test-dir build -R '^wdsp_channel_test$' --output-on-failurepython3 tools/check_engine_boundary.py --strict— zero blocking findingsgit diff --checkgit log --show-signature -1Validated 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