Mercury client: bandwidth toggle and Send CQ Frame

[pedromessetti]

Summary

Adds two features to the Mercury Client (chat) window and its client library:

• Bandwidth toggle — a 500 Hz / 2300 Hz selector (default 2300 Hz) that sends BW500 / BW2300 to the engine. Replaces the previous hard-coded BW2750 sent on connect.
• Send CQ Frame button — queues a one-shot CQFRAME <callsign> <bw> via the control port using the local callsign and current bandwidth. While a CQ frame is transmitting, broadcast and ARQ connect are disabled; they re-enable on the engine's CANCELPENDING.

Changes

• gui_interface/mercury-client/client/client.go
  • Add Config.BandwidthHz (default 2300), send BW<BandwidthHz> on connect.
  • Add Client.SetBandwidth() and Client.SendCQFrame().
• gui_interface/mercury-client/modem/modem.go
  • Add ModemClient.SendCQFrame(src, bwHz).
• gui_interface/fyne-ui/mercury_chat_window.go
  • Add bandwidth selector, Send CQ Frame button, and CQ-in-progress gating of broadcast/ARQ connect.

Test plan

• go build ./... in gui_interface/mercury-client and gui_interface/fyne-ui.

[rafael2k]

Both builds clean (mercury-client and fyne-ui), go vet clean, no C changes. The command vocabulary checks out against the engine: BW500/BW2300 match ARQ_BANDWIDTH_NARROW_HZ/ARQ_BANDWIDTH_FULL_HZ (arq.h:27-28), and CQFRAME %15s %d is accepted at tcp_interfaces.c:516 with the same three-value validation. Good.

Three issues in the CQ gating, one of which I think matters on air.

---

1. CANCELPENDING is not CQ-specific — it can re-enable TX while the CQ is still keying

The engine emits CANCELPENDING from two unrelated places:

// datalink_arq/arq.c:262 — an INCOMING connection was cancelled
static void cb_notify_cancelpending(void) { ... arq_tnc_send_cancelpending(); 
    HLOGI(LOG_COMP, "Incoming connection cancelled"); }

// datalink_arq/arq.c:783 — our CQ transmission finished
void arq_notify_cq_tx_complete(void) { arq_tnc_send_cancelpending();
    HLOGI(LOG_COMP, "CQ transmission completed"); }

The UI treats any CANCELPENDING as "my CQ is done":

case "CANCELPENDING":
    if cw.cqSending { cw.cqSending = false; cw.setCQBusy(false) }

So: someone calls us while our CQ is on the air, then gives up (or the connect attempt is otherwise cancelled) → cb_notify_cancelpending fires → the UI clears cqSending and re-enables ARQ Connect and Broadcast while the CQ frame is still transmitting. The user then has a live button that starts a second transmission on top of the first.

That's the same failure class we spent this week fixing on the modem side (transmitting into our own tail), so I'd rather not hand the user a button that causes it.

The two PENDING/CANCELPENDING pairs are indistinguishable on the wire as things stand. Options, roughly in order of preference:

• give the CQ path its own async status (e.g. CQFRAMESENT) so the client can key off something unambiguous — a small engine change, but it makes the lifecycle explicit rather than inferred;
• or, if we keep VARA-shaped status only, treat the CQ as busy until either CANCELPENDING or a bounded timeout, and don't let a stray one re-enable ARQ Connect while the modem still reports PTT.

Worth deciding deliberately, since the engine header already notes this is VARA-compat shaped ("VARA clients such as varim treat CQFRAME as a pending operation", arq.h:253-256).

2. setCQBusy(false) re-enables ARQ Connect even during an active ARQ session

cw.sendCQ.Enable()
cw.arqConnect.Enable()                     // <- unconditional
if cw.mc != nil && cw.mc.IsConnected() && !cw.mc.IsARQConnected() {
    cw.sendBcastWrap.Enable()              // <- correctly guarded
}

Broadcast gets the IsARQConnected() guard; ARQ Connect doesn't. If a session comes up during the CQ (setARQ(true) disables arqConnect), the subsequent CANCELPENDING re-enables Connect mid-session, contradicting setARQ. Same guard on arqConnect fixes it.

3. cqSending is read and written from two goroutines without synchronisation

onSendCQ, onConnect and onDisconnect write it on the UI goroutine; forwardStatus (started with go cw.forwardStatus(), line 346) reads and writes it directly. That's a plain data race.

The file already has the right convention for this — setARQ mutates cw.bcastDisabledReason inside fyne.Do(...), marshalling onto the UI thread. The new code puts the widget calls inside fyne.Do but leaves the cqSending test and assignment outside it. Moving the flag into the same closure (i.e. let setCQBusy own it) makes it consistent with the surrounding code and removes the race. Worth a go test -race pass on the client packages afterwards.

---

Minor / questions

• Dropping BW2750 (ARQ_BANDWIDTH_TACTICAL_HZ) from the selector loses the previously hard-coded default. Deliberate? It's still accepted by the engine, so it's a UI-only restriction — just want to confirm it's a choice rather than an oversight, since existing users were implicitly on 2750.
• Client.SetBandwidth writes c.cfg.BandwidthHz outside c.mu, while taking that lock to read c.modem two lines earlier; SendCQFrame then reads c.cfg.BandwidthHz unlocked. Both are UI-driven today so it's unlikely to bite, but cfg becoming mutable-after-construction is the kind of thing worth putting under the existing lock while it's still cheap.
• If the CQ never completes (engine restart, modem drop mid-TX), cqSending stays true and Connect/Broadcast stay disabled until a full TCP disconnect. onDisconnect is the only reset. A timeout would close that off — related to Great start #1.

Nothing here is hard to fix; #1 is the one I'd want settled before this goes on air.

[rafael2k]

Correction to my finding #1 above: we're staying with VARA semantics, so scrap the CQFRAMESENT suggestion — no new status on the wire.

The good news is the fix is smaller than either option I gave, and needs no engine change at all. The unambiguous signal already exists and is already VARA-shaped: PTT ON / PTT OFF.

// data_interfaces/tcp_interfaces.c:1297,1307
(void)tnc_queue_line_critical("PTT ON\r");
(void)tnc_queue_line_critical("PTT OFF\r");

Note _critical, not the lossy tnc_queue_line — and the comment above it says exactly why (tcp_interfaces.c:172-176):

A host driving a transmitter interlock or a frequency scanner (BPQ32 INTERLOCK, VARA-style scanning) decides whether the radio is busy from PENDING / PTT ON / PTT OFF; a dropped PTT ON leaves it believing we are receiving while we are keyed, and it will hand the antenna to another port on top of us.

That is precisely the failure this gating is meant to prevent, and the engine already guarantees delivery of the signal for it.

Ordering is exactly what we need. arq_notify_cq_tx_started/complete bracket the modulation call, inside the keyed window:

// modem/modem.c:1288-1305
if (is_cq_frame) arq_notify_cq_tx_started();     // -> PENDING
int rc = send_modulated_data(...);                // PTT ON ... frame ... PTT OFF
if (is_cq_frame) arq_notify_cq_tx_complete();    // -> CANCELPENDING

so a CQ produces PENDING → PTT ON → PTT OFF → CANCELPENDING. Clearing the busy state on PTT OFF instead of CANCELPENDING is both earlier (it's the true end of transmission) and unambiguous — an incoming connection's PENDING/CANCELPENDING pair can no longer clear it, because that path never keys the radio.

What's needed: PTT ON / PTT OFF aren't parsed by the client yet. modem.go:460-484 handles CONNECTED / DISCONNECTED / BUSY / PENDING / CANCELPENDING / CQFRAME but not PTT. Two more cases in that switch, then gate on PTT OFF in forwardStatus. Purely additive, client-side only, no wire change.

Worth considering the slightly larger version: a txActive flag driven by PTT ON/PTT OFF is the right primitive for "don't let the user start a transmission while we're keyed" in general, not just for CQ. It would protect Broadcast and ARQ Connect the same way, and it's the same two status lines. Your call whether that's this PR or a follow-up — for this PR, gating CQ on PTT OFF is enough.

(Incidentally, the CQFRAME status the client already parses is the inbound one — the engine emits it at arq.c:770 when it decodes someone else's CQ. Not our own transmission, so it can't serve here.)

Findings #2 (unguarded arqConnect.Enable()) and #3 (the cqSending race) are unaffected by any of this and still stand.

[rafael2k]

Re-checked at 0bbb58c. All three findings are properly fixed, and the rebase onto mercuryv2 is clean — 3 files, no C changes, both modules build and vet clean.

#1 PTT gating (9dc6912) — PTT ON/PTT OFF added to dispatchControlLine, and forwardStatus now keys off PTT OFF. Correct.

#2 ARQ Connect guard (a0cf81b) — arqConnect.Enable() now sits behind the same IsConnected() && !IsARQConnected() test as Broadcast. Correct.

#3 the race (0bbb58c) — better than what I suggested. forwardStatus no longer touches cqSending at all; setCQBusy owns it inside fyne.Do, and the if !cw.cqSending { return } guard lives inside the closure, so a stray PTT OFF is a no-op rather than a racy read. That's the right shape.

---

Two things the fix surfaces that weren't visible before. Both are about the same gap, and neither is a regression from your changes — the CQ button is simply new surface.

setARQ(true) never disables sendCQ

setARQ(true) disables arqConnect and sendBcastWrap, but not sendCQ. So during a live ARQ session the Send CQ Frame button stays clickable, and onSendCQ only checks IsConnected() (i.e. modem/TCP) and cqSending — neither of which reflects an active session.

The engine won't save us: ARQ_CMD_SEND_CQ (arq.c:517-541) has no conn_state guard at all. It builds the frame and calls cb_send_tx_frame unconditionally, so the CQ really does go out mid-session.

Adding cw.sendCQ.Disable() / .Enable() to the two setARQ branches closes it, matching how sendBcastWrap is already handled.

Narrower: a foreign PTT OFF can clear the flag early

PTT OFF fires at the end of every transmission. cqSending gates it, but the flag is set when the CQFRAME command is queued, not when the CQ actually keys. So if another transmission is already in flight at that moment, its PTT OFF arrives first and clears the busy state before the CQ has been on the air.

Today the main way to reach that is the session case above (ARQ burst keying while the user hits Send CQ), so fixing setARQ removes most of it. A broadcast in flight is the remaining path.

If you want it airtight, arm on PTT ON rather than at queue time:

case "PTT ON":
    cw.cqKeyed = true       // only meaningful while cqSending
case "PTT OFF":
    if cw.cqKeyed { cw.cqKeyed = false; cw.setCQBusy(false) }

Given you now parse PTT ON anyway, that's a couple of lines. Your call whether it's worth it here or noted for later — the setARQ one I'd do in this PR.

Neither blocks; the three original findings are resolved.

[rafael2k]

@pedromessetti I've pushed the setARQ fix myself rather than send you round again for two lines — ca6d307. Good to merge from my side once CI is green.

What I changed, so nothing is a surprise:

setARQ(true) now disables sendCQ, alongside the arqConnect / sendBcastWrap it already handled.

setCQBusy(false) re-enables sendCQ under the same session test as the rest, rather than unconditionally. That second half turned out to be necessary, not tidiness: a session can come up while our own CQ is still on the air, and the unconditional sendCQ.Enable() on PTT OFF would have undone the block a moment after setARQ applied it.

I walked the state paths looking for a button that could get stuck off:

• CQ in flight when a session starts → sendCQ already disabled; PTT OFF clears cqSending but the session test keeps it disabled. Correct.
• CQ in flight when a session ends → setARQ(false) sees cqSending still true and leaves it disabled; the following PTT OFF re-enables it. Correct.
• setTCP(false) and onDisconnect both reset the flag, so a modem drop can't strand it.

gofmt clean, both modules build, go vet clean.

Your three fixes are all good — and the cqSending one is better than what I proposed: moving the guard inside the fyne.Do closure means a stray PTT OFF is a clean no-op rather than a racy read. I took the same shape for the enable path.

Still open, and deliberately not in this PR: cqSending is set when the CQFRAME command is queued rather than when the CQ actually keys, so a broadcast already in flight can clear it early with its own PTT OFF. Blocking CQ during a session removes the main path to that; arming on PTT ON would close the rest, and you already parse it. Worth a follow-up issue rather than growing this one.

Thanks for the fast turnarounds on all three rounds.

[rafael2k]

@pedromessetti — one ask before this lands: please re-review ca6d307.

I pushed it directly to your branch, so it's the one commit here that hasn't had a second pair of eyes. Reviewing your own PR after someone else has written into it is easy to skip, and that's exactly the commit where a mistake would be least likely to get caught.

Two specific things worth your judgement rather than just a glance:

1. The setCQBusy(false) half. I put sendCQ.Enable() inside the existing IsConnected() && !IsARQConnected() block. You wrote that function, so you'll know better than I do whether there's a path where a CQ finishes and sendCQ should come back even though the session test fails — I couldn't find one, but you have the fuller picture of the window's state machine.

2. Whether blocking CQ during a session is the behaviour you actually want. I treated "CQ mid-session transmits on top of the session" as clearly wrong, but that's a product call as much as a technical one. If you intended CQ to stay available — say for a beacon-style use — then the right fix is a guard in the engine instead of a disabled button, and we should do that rather than my version.

If it looks right to you, merge away — CI is still running as I write this, so just let it go green first.

[pedromessetti]

Reviewed ca6d307. Both calls are correct — agree with the implementation as-is.

On the guard: I can't find a path where a CQ finishes and should return while the session test fails. connected && !session only fails when (a) the link is down or (b) a session is up; in (a) PTT OFF can't arrive post-disconnect (and a buffered one is a no-op because cleared , short-circuiting the guard), and in (b) that's precisely the mid-session case you're blocking. The half is symmetric ( defers to setCQBusy), so there's no stuck-button or premature-enable path.

On blocking CQ mid-session: yes, that's the behaviour I want. The button sends a single solicitation frame, not a beacon, so it only makes sense while idle; transmitting on top of a live session is the key-over-our-own-tail class we've been eliminating, and since ARQ_CMD_SEND_CQ has no conn_state guard the disabled button is the right minimal fix and consistent with how Broadcast/Connect are handled in setARQ. If we ever want beacon-style CQ, that's a separate feature — an engine-side guard (reject/queue SEND_CQ unless idle) rather than just un-disabling the button.

Verified go build, go vet , and the quick -race tests pass on top of your commit. LGTM