fix(dax): align RADE TX policy naming with issue 2343 and add unit tests#2353
fix(dax): align RADE TX policy naming with issue 2343 and add unit tests#2353ten9876 merged 2 commits intoten9876:mainfrom
Conversation
- Rename RadeModem to RadeModemTx in DaxTxPolicy and MainWindow - Update policy error string to 'windows_dax_conflict' - Add comprehensive unit tests in tests/dax_tx_policy_test.cpp - Follow project style guide for braces on all control flow
![aethersdr-agent[bot]](https://avatars.githubusercontent.com/in/3286087?s=60&v=4){kind=small}
There was a problem hiding this comment.
Thanks for the fix and the well-written rationale, @NF0T — the diagnosis matches the existing TciTxAudio precedent (#2276) and the targeted RadeModemTx policy case is the right shape. A few notes:
1. PR description framing is slightly off. The body describes this as renaming RadeModem → RadeModemTx, but DaxTxRequestReason::RadeModem doesn't exist on main — this PR is adding a new enum value. Not a code issue, just worth correcting in the description so reviewers don't go looking for the prior name.
2. Tests duplicate existing infrastructure. There's already a testDaxTxPolicy() in tests/radio_status_ownership_test.cpp (line 168) that exercises the same matrix using the project's check() helper. Suggest adding the new RadeModemTx coverage there rather than introducing a separate dax_tx_policy_test executable with its own ad-hoc ASSERT_TRUE macros. That keeps the policy regressions in one place and avoids registering an extra CMake target/CTest run for the same surface.
3. Test is missing the actual bug scenario. The RadeModemTx Windows test sets ctx.platform = DaxTxPlatform::Windows but leaves ctx.mode defaulted to DaxTxMode::None. The real #2343 scenario is mode = DaxTxMode::ExternalDax2 — that's what previously would have routed RADE through the blocked ExternalDaxRouteOnly branch. Worth adding an explicit case:
ctx.mode = DaxTxMode::ExternalDax2;
ASSERT_TRUE(evaluateDaxTxPolicy(ctx).allowed,
"RadeModemTx must be allowed even when SmartSDR DAX2 owns Windows audio");Otherwise the test passes for the wrong reason (mode=None doesn't trigger any blocking branch even before this PR's change).
4. The note-string rename is unrelated churn. Changing SmartSDR_DAX2_owns_windows_dax → windows_dax_conflict in the two existing branches isn't required for the fix, and the original string is more informative (it names the actual owner). These notes are only consumed by qCInfo logs (verified — no other call sites), so it's safe either way, but I'd suggest reverting that hunk to keep the PR focused on the RADE fix.
5. RadeModemTx enum case is correctly unconditional — RADE bypasses the audio-device layer the same way TCI does, so always-allow is the right policy. The placement in activateRADE() after the engine-start success check is correct, and ensureDaxTxStream is idempotent if updateDaxTxMode() already created a stream, so there's no double-create risk.
Core change LGTM once the test is tightened up.
- Revert unrelated note-string rename (windows_dax_conflict -> SmartSDR_DAX2_owns_windows_dax) in two pre-existing blocking branches; original string is more informative and narrower change is better - Remove standalone dax_tx_policy_test.cpp and its CMake target; policy regression tests belong in the existing radio_status_ownership_test where the testDaxTxPolicy() harness already lives - Add RadeModemTx cases to testDaxTxPolicy(): Windows with ExternalDax2 mode set (the exact ten9876#2343 regression scenario) and Linux without hosted DAX — both must be unconditionally allowed Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
|
Thanks for the thorough review — all four points addressed in the follow-up commit (c7c7bbe): 1. PR description framing — Updated. The body now correctly describes this as adding 2. Duplicate test infrastructure — Removed 3. Missing bug scenario — Fixed. The Windows test context now sets 4. Note-string rename reverted — The only net diff to the policy logic is the addition of the |
|
Claude here on Jeremy's behalf — merged via admin squash. Thanks @NF0T — clean parallel of the #2276 architectural pattern, the brace-cleanup-along-the-way is appreciated, and the iteration discipline (RadeModem → RadeModemTx, real ExternalDax2 test scenario, in-harness tests) made the diff feel inevitable. Windows RADE TX users were waiting on this one. 73, Jeremy KK7GWY & Claude (AI dev partner) |
2 participants
Overview
Fixes RADE TX producing no waveform output on Windows (#2343).
When RADE is activated,
activateRADE()callsensureDaxTxStream()to register adax_txstream slot with the radio. On Windows, the policy engine routes generic digital modes throughExternalDaxRouteOnly, which is intentionally blocked — SmartSDR DAX2 owns the Windows audio device layer (DAX TX 1–8). RADE, however, never touches those audio devices: it encodes the mic waveform itself and sends VITA-49 packets directly viasendModemTxAudio(), exactly as TCI does. Without a registered stream,m_txStreamIdremains 0 in AudioEngine and bothonTxAudioReady()andsendModemTxAudio()return early before sending anything.Fix
Add
DaxTxRequestReason::RadeModemTx— an unconditionally-allowed policy reason — and callensureDaxTxStream(RadeModemTx)fromactivateRADE(). This parallels the existingTciTxAudioreason introduced in #2276 for the same architectural reason: both RADE and TCI inject VITA-49 audio directly into the radio'sdax_txstream slot, which is independent of SmartSDR DAX2's ownership of the Windows audio device layer. Multiple GUI clients may each register their own stream slot; SmartSDR DAX2 owning the device layer does not preclude AetherSDR owning its own slot.Key Changes
src/core/DaxTxPolicy.h: AddRadeModemTxenum value andevaluateDaxTxPolicycase (unconditional allow, with rationale comment). Also add braces to control-flow in the switch perCLAUDE.md.src/gui/MainWindow.cpp: CallensureDaxTxStream(DaxTxRequestReason::RadeModemTx)inactivateRADE()after the engine-start success check.tests/radio_status_ownership_test.cpp: AddRadeModemTxcases to the existingtestDaxTxPolicy():ExternalDax2mode — the exact Win TX RADE #2343 regression scenario — must be allowed.Addressing Agent Feedback
RadeModemTxnaming: adopted as suggested — more precise thanRadeModem.radio_status_ownership_test.cpp(existingtestDaxTxPolicy()harness), not a separate binary.mode = DaxTxMode::ExternalDax2— the actual blocking scenario, not the defaultNone.SmartSDR_DAX2_owns_windows_daxis more informative and the change was unrelated to the fix.Fixes #2343