Fix TCI volume / drive / rx_volume command dispatch (#1764)#2463
Conversation
Resolves long-standing TCI bugs where DRIVE, VOLUME and RX_VOLUME
SET commands sent by clients had no effect on the radio:
1. **rx_volume dispatched before isSet was computed.**
`isSet` was initialised to `false` at the top of `handleCommand` and
only assigned its real value (`args.size() >= 2`) AFTER the first
dispatch block — so every command in that block (`rx_volume`,
`cw_keyer_speed`, `mon_volume`, `rx_mute`, `rx_balance`, `mon_enable`,
`rx_nb_param`, `rx_bin_enable`, `rx_anc_enable`, `rx_dse_enable`,
`rx_nf_enable`, `digl_offset`, `digu_offset`, `vfo_lock`) silently
ran the GET path and ignored client SET values.
Fix: compute `isSet` once at the top of the function so all
dispatched commands see the correct value.
2. **`cmdDrive` / `cmdTuneDrive` read power from `args[0]`** (the trx
index) instead of `args[1]`, so `drive:0,50;` set power to 0.
Both commands are global per the TCI v2.0 spec — fix to treat any
args at all as SET, take args[0] as the value (or args[1] if a
client sends the legacy trx-prefixed form).
3. **`cmdVolume` confused vol and trx indices and didn't reach the
master output.** Per the TCI v2.0 spec, `volume:N;` is the GLOBAL
master volume command — not a per-slice gain. Old SET path used
args[0] for both vol and trx, and applied to slice audioGain
instead of the actual master output the operator hears.
Rewritten as a global master-volume command:
- GET reads from AppSettings("MasterVolume") — the same store the
title bar slider uses.
- SET stashes the requested level in `m_pendingMasterVolume`;
`TciServer` reads it after `handleCommand` and emits a new
`masterVolumeRequested(int)` signal that `MainWindow` handles
via the same `applyMasterVolume()` slot the title bar slider uses
(AudioEngine::setRxVolume or RadioModel lineout, depending on
PcAudioEnabled, plus persistence to AppSettings).
- Per-receiver volume continues to live on the separate
`rx_volume:trx,N;` command (cmdRxVolume, already correctly
written — args[0]=trx, args[1]=value).
4. **GUI master-volume changes were invisible to TCI clients.**
`applyMasterVolume()` now calls `m_tciServer->broadcastMasterVolume(pct)`
so remote controllers (e.g. aether_pad) stay in sync when the
operator drags the title bar slider.
5. **Per-slice `audioGainChanged` was not broadcast.**
Added a `SliceModel::audioGainChanged` connection in
`TciServer::wireSlice()` that broadcasts `rx_volume:trx,N;` so
GUI changes to slice audio level reach connected TCI clients.
Tested on Windows (Qt 6.10.3 + MSVC), Linux (Qt 6.2.4), macOS
(Qt 6.11). Verified end-to-end with the aether_pad standalone
controller (G0JKN/W3) — bidirectional sync between the pad's VOLUME
chip and AetherSDR's title bar slider, audio actually changes when
either side moves.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Thanks @nigelfenton — this is a thorough fix with great diagnostic detail. The root-cause analysis (the isSet initialization order bug silently turning many SETs into GETs) is exactly right, and the spec-aligned rewrites of cmdDrive/cmdTuneDrive/cmdVolume look correct. AetherSDR conventions are followed (AppSettings, not QSettings), and TitleBar::setMasterVolume uses QSignalBlocker so the new masterVolumeRequested lambda calling setMasterVolume(pct) then applyMasterVolume(pct) won't double-apply via the slider signal. Nice.
A couple of small things worth a look:
1. cmdVolume SET silently mutes on garbage input — src/core/TciProtocol.cpp:780
int vol = args[args.size() == 1 ? 0 : 1].toInt();Unlike the parallel cmdDrive which uses toInt(&ok) and bails on parse failure, cmdVolume does not check ok. A malformed volume:abc; from a misbehaving client coerces to 0 and silently mutes the radio. This is a system boundary, so it's worth validating:
bool ok;
int vol = args[args.size() == 1 ? 0 : 1].toInt(&ok);
if (!ok) return {};2. Doubled broadcasts on rx_volume SETs from a TCI client — src/core/TciServer.cpp:1175 (new audioGainChanged handler)
The new wireSlice handler is exactly right for the GUI-change → broadcast direction. But on the TCI-SET direction, cmdRxVolume already sets m_pendingNotification = "rx_volume:trx,N;" (TciProtocol.cpp:1260), and the SET path calls s->setAudioGain(gain) which then fires audioGainChanged, which this new connection broadcasts a second time. Net effect: every TCI rx_volume SET produces two identical broadcasts.
It's idempotent so behaviorally harmless, but if you wanted to suppress the redundant emit, the cleanest path is dropping the m_pendingNotification assignment in cmdRxVolume's SET branch and letting the model→signal path be the sole source of truth (matches how Qt models normally fan out state changes). The same pattern applies to the master-volume path: cmdVolume sets m_pendingNotification and applyMasterVolume ends with broadcastMasterVolume(pct), so a TCI volume:N; SET also broadcasts twice. Not a blocker — just worth a thought before this lands as the canonical wiring pattern for the rest of the protocol.
Otherwise this looks good to merge. Thanks for the careful diagnosis and the comments pointing future readers at #1764 — those will save someone real time.
|
Claude here — really excellent investigation, Nigel, and a great example of how the aether_pad ground-truth approach surfaces bugs pure code review wouldn't catch. The isSet initialization bug is exactly the kind of latent issue that lives in code forever until someone sends an actual SET and watches nothing happen. Five distinct fixes, properly scoped, fully tested, well-commented with TCI v2.0 spec references throughout. The applyMasterVolume single-source-of-truth refactor is the right shape — title bar and TCI now route through one slot so audio path / persistence / broadcast stay in lockstep regardless of who initiated the change. Backward compat for legacy 'volume:trx,N;' clients is appreciated too. Thanks for the clean follow-up after the scope feedback on #2456 — exactly how iteration should go. Merged. 73, Jeremy KK7GWY & Claude (AI dev partner) |
Closes #1764.
Resolves long-standing TCI bugs where
DRIVE,VOLUMEandRX_VOLUMESET commands had no effect on the radio. Diagnosed independently while building a standalone TCI control surface (aether_pad on Arduino Giga R1) — sendingvolume:N;from the pad caused the displayed value to flicker but the actual audio never moved. Tracing throughTciProtocol.cppconfirmed the same five issues the existing #1764 triage flagged.What's fixed
isSetwas initialised tofalseand only assigned its real value AFTER the first dispatch block.Every command dispatched in lines 211–247 (
rx_volume,cw_keyer_speed,mon_volume,rx_mute,rx_balance,mon_enable,rx_nb_param,rx_bin_enable,rx_anc_enable,rx_dse_enable,rx_nf_enable,digl_offset,digu_offset,vfo_lock) silently took the GET path forever, ignoring client SET values.Fix: compute
isSet = (args.size() >= 2)once at the top ofhandleCommand.cmdDrive/cmdTuneDriveread power fromargs[0](the trx index) instead ofargs[1]—drive:0,50;set power to 0. DRIVE is a global command per TCI v2.0; rewritten to treat any args as SET, takeargs[0]as the value (orargs[1]for legacy trx-prefixed form).cmdVolumeconfused vol and trx indices and applied to per-slice gain instead of the master output the operator hears. Per spec,volume:N;is GLOBAL master volume.Rewritten as a global master-volume command with proper plumbing to
AudioEngine::setRxVolume()(orRadioModel::setLineoutGainwhen PC audio is off) — the same path the title bar slider uses.TciProtocoldoesn't ownAudioEngine, so the requested level is stashed in a newm_pendingMasterVolumemember;TciServerreads it afterhandleCommandand emitsmasterVolumeRequested(int)forMainWindowto handle.applyMasterVolume()factored into a private slot shared by both the title bar slider and the new TCI signal.GUI master-volume changes were invisible to TCI clients.
applyMasterVolume()now callsTciServer::broadcastMasterVolume(pct)so remote controllers stay in sync when the operator drags the title bar slider.Per-slice
audioGainChangedwas never broadcast.TciServer::wireSlice()now emitsrx_volume:trx,N;on slice audio-gain changes — GUI changes reach connected TCI clients.Files
src/core/TciProtocol.hpendingMasterVolume()accessor andm_pendingMasterVolumemembersrc/core/TciProtocol.cppisSetonce at top; rewritecmdDrive/cmdTuneDrive/cmdVolumeper TCI v2.0 specsrc/core/TciServer.hmasterVolumeRequested(int)signal +broadcastMasterVolume(int)methodsrc/core/TciServer.cpppendingMasterVolumeto MainWindow via signal; broadcastrx_volumeon slice gain changes; implementbroadcastMasterVolumesrc/gui/MainWindow.happlyMasterVolume(int)private slotsrc/gui/MainWindow.cppapplyMasterVolume(); wire title bar + TCI signals through it; broadcast on every changeTest plan
volume:N;from a TCI client moves the title bar slider AND changes audible output (via either AudioEngine or radio lineout depending onPcAudioEnabled)volume:N;to all connected TCI clientsrx_volume:trx,N;continues to control per-slice audio gain (was already working post-dispatch fix)rx_volume:trx,N;to TCI clientsdrive:N;(1 arg, spec form) ANDdrive:0,N;(legacy trx-prefixed) both correctly set TX powertune_drive:N;same — both 1-arg and 2-arg forms accepted🤖 Generated with Claude Code