fyne-ui-v2: Mercury Client chat window, waterfall palette & toggle, UI polish

[pedromessetti]

Summary

This branch bundles the vendored Mercury Client into the Fyne UI and adds waterfall enhancements.

Mercury Client (vendored)

• Added gui_interface/mercury-client/ Go module replacing the exec-based binary launch. The client is compiled directly into mercury-ui via a Go module replace directive.
• Launch Mercury Client button opens a chat window in the same Fyne app — no external binary, no PATH lookup.
• Chat window: TCP connection, ARQ session controls, send/receive ARQ and broadcast messages. Callsigns rendered in bold via RichText.
• UI polish: labels aligned with Mercury Client conventions, placeholders updated.

Waterfall

• Settings > Waterfall dialog with color palette selector (Turbo, Hot, Grayscale, Blackblue) and an on/off toggle.
• Palette persists via Fyne preferences; waterfall state persists to mercury.ini so it survives restarts.
• Toggle uses a direct CGo call (mercury_ui_set_waterfall) — no websocket, no strcmp dispatch — saving CPU by stopping the engine's FFT pipeline at runtime.
• When disabled the waterfall/spectrum card vanishes from the layout; re-enabling restores it.

Telemetry labels

• Renamed User Callsign → My callsign, Dest Callsign → Target callsign.

[rafael2k]

Review of the full diff (14 files, +1505/−87) against mercuryv2. Build checks
pass: go build ./... clean, go vet ./... clean on the new module,
ui_communication.c compiles clean. Nice piece of work — the vendored client
removes a whole class of PATH/exec problems.

11 findings, ordered by severity. Numbers 1, 3 and 5 are the ones I would fix
before merge; the rest are safe to land as follow-ups if you prefer.

---

1. Re-enabling the waterfall never restarts the publisher thread — ui_communication.c:213 (medium)

ui_comm_set_waterfall() flips the FFT gate and the config flag, but
spectrum_publisher_thread is only created by ui_comm_init() when
waterfall_enabled was true at boot (:660).

Start with waterfall_enabled = false (or turn it off once — it now persists),
then re-enable from Settings → Waterfall: the engine pays the full FFT cost
again and status reports waterfall: true, but no ws_broadcast_binary frames
are ever sent. mercury-qt and docs/app show a permanently blank waterfall
until restart. The embedded Fyne UI hides this because it polls
mercury_ui_get_spectrum directly.

2. cfg_write() is now reachable from two threads — ui_communication.c:221 (low)

The Fyne UI thread reaches it via mercury_ui_set_waterfall, while the
websocket thread can be inside ui_comm_handle_command doing its own
cfg_write for set_audio_config/set_radio_config. cfg_write does
fopen(path, "w") (truncate) with no lock and no temp-file+rename, so a remote
client applying a radio config while the local operator toggles the waterfall
can truncate or interleave mercury.ini. It also races on ctx->cfg itself.

3. SetChecked fires OnChanged, so opening the dialog re-toggles — main.go:1029 (medium)

enabledCheck.SetChecked(state.telemetry.Waterfall) invokes the callback (fyne
v2.8 widget/check.go:75-86 fires whenever the value changes from the zero
value, and the check starts unchecked). So every opening of Settings →
Waterfall while it is on re-issues the toggle: locally a CGo call that rewrites
mercury.ini and logs "Waterfall turned on"; on a remote link it pushes
set_waterfall to the other station; with no link it logs a bogus "Failed to
toggle waterfall: not connected" when the operator only opened the dialog.

Set enabledCheck.Checked directly, or assign OnChanged after SetChecked.

4. state.link read without the mutex — main.go:1019 (low)

Written under state.mu from the connect goroutine (:693, :702) and
disconnectLink (:628-630). Toggling the waterfall during a connect or
disconnect is a race on a two-word interface value — reported under -race,
and a torn read would hit the type assertion.

5. cw.mc nil-deref panics the whole process — mercury_chat_window.go:273 (medium)

cw.mc is read inside the goroutine (cw.mc.ConnectARQ()) but written on the
UI goroutine by onDisconnect (:260, cw.mc = nil). Press "Connect ARQ" then
"Disconnect modem" before the goroutine is scheduled: the read yields nil,
Client.ConnectARQ does c.mu.Lock() on a nil receiver, and the panic takes
down mercury-ui — engine and radio with it. The arqAbort closure and
onSendARQ share the same unsynchronized field.

This is the one I would fix first: it is operator-reachable with two clicks and
the blast radius is the whole station.

6. Forwarder goroutines are never reaped — mercury_chat_window.go:322 (low)

forwardLog/forwardARQChat/forwardBroadcastChat/forwardStatus range over
channels that are never closed — neither Client.Disconnect nor
ModemClient.Disconnect closes LogCh/ARQChatCh/BroadcastChatCh/StatusCh.
Each connect/disconnect cycle strands four more goroutines, holding the old
Client alive and calling fyne.Do against widgets of a possibly-closed
window. They also evaluate cw.mc.LogCh at goroutine start, carrying the same
nil race as #5.

7. Second "Launch Mercury Client" window evicts the first — mercury_chat_window.go:18 (low)

The button is never disabled and openMercuryClientWindow keeps no singleton.
A second click dials the ARQ control port again; the engine accepts and evicts
the previous control client (data_interfaces/tcp_interfaces.c:764 →
close_ctl_client(..., notify_arq=true)), which submits
ARQ_CMD_CLIENT_DISCONNECT and tears down any ARQ session in progress. The
first window keeps its buttons enabled and gives no sign it is dead.

8. Unsynchronized conn pointers in the readers — modem/modem.go:442 (low)

readARQData (:442) and readBroadcast (:468) dereference
mc.ARQDataConn/mc.BroadcastConn with no lock while Disconnect()
(:339-357) closes and nils them under mc.mu. It survives today only because
a nil *net.TCPConn receiver returns EINVAL instead of panicking. Capture the
conn once at goroutine start, as readARQControl does at :377.

9. quit channel swap races, and LogCh sends under the mutex — modem/modem.go:361 (low)

Disconnect() does close(mc.quit); mc.quit = make(chan struct{}) while three
readers select on it unsynchronized. A reader that had not yet observed the
close now selects on the fresh channel and can only exit via socket error. The
same function sends up to three messages on mc.LogCh (cap 100) while holding
mc.mu, so a stalled consumer blocks the Fyne UI goroutine with the modem mutex
held.

10. Double-click on Connect ARQ strands both waiters — modem/modem.go:189 (low)

ConnectARQ blocks up to 120 s but onARQConnect does not disable the button.
A second click assigns a new mc.connectRespCh; the first waiter never sees
CONNECTED and hangs to its full timeout, and its deferred
mc.connectRespCh = nil clears the second call's channel too, stranding that
one as well. A second CONNECT frame also goes to the TNC mid-handshake.

11. Partial Connect() failure leaks the sockets — modem/modem.go:146 (low)

If ARQ control dials OK and then the data or broadcast dial fails (:129/:146),
the already-open conns are neither closed nor recorded — client.Connect
(client.go:82) returns the error and drops the ModemClient. The socket
lingers until the GC finalizer, and meanwhile the engine reports
client_tcp_connected: true with nothing reading it.

---

Note on overlap with #175

#175 (TX waterfall) adds modem_set_tx_spectrum_enabled(), which is a second
setter for the same user-facing switch your modem_set_spectrum_enabled()
already drives. Merging both as-is gives a toggle that stops the RX FFT at
runtime but leaves the TX FFT running, since mine is only wired at init.

That is mine to fix, not yours — the plan is to merge this first, then rebase
#175 on top and drop my setter so yours gates both. Finding 1 above applies to
that combined path too.

Also: README.md here may need a refresh — trunk gained a Go/fyne
prerequisites section in 59fa549.

[rafael2k]

Re-reviewed after your fixes. All ten are real — I checked each diff rather
than the commit titles, and traced the paths rather than assuming. Nice work,
that was fast.

Verified green: go vet ./... (websocket build), go vet -tags mercury_embedded ./... (cgo build), go vet ./... in mercury-client, and gcc -fsyntax-only
on ui_communication.c.

I also went looking for problems the fixes could have introduced, and found
none: cfg_mutex is only ever taken leaf-wise (no nesting, and the
pthread_create inside it doesn't re-enter), the done channel has no
double-close on any path (connect / disconnect / window close — onDisconnect
nils cw.done and fyne guards SetOnClosed), and fyne v2.8's funcQueue is
unbounded so the new forwarder structure can't wedge the GL loop.

Verdict: approve with changes. Nothing below is a structural problem with
the branch. Four things I would fix before merge, then it is good to go.

---

Fix before merge

A. modem.go:392 — can still panic the whole process

4b3013a moved the reads to the top of the goroutine, but they are still
unsynchronized (readARQControl reads mc.ARQControlConn and mc.quit,
readBroadcast at :488 reads mc.BroadcastConn) while Disconnect() nils
them under mc.mu.

Two consequences. It is still a real data race under -race; and
readARQData has a nil guard while readARQControl and readBroadcast do
not — so if Disconnect() runs before the goroutine is first scheduled,
bufio.NewReader(nil).ReadString dereferences a nil *net.TCPConn receiver in
(*conn).ok() and takes the UI process down. Capturing under mc.mu plus the
nil check closes both.

B. mercury_chat_window.go:18 — prior finding 7, still open

Confirmed no fix attempted, and it is worse than I first described.
openMercuryClientWindow always builds a new window; the engine evicts the
incumbent control client (data_interfaces/tcp_interfaces.c:762,
close_ctl_client(..., true)), tearing down window A's ARQ session. Window A
never finds out: its reader logs a read error, but ARQControlConn stays
non-nil so IsConnected() keeps returning true and setTCP(false) is never
called — the buttons still say connected.

A singleton (or disabling the launch button while a window is open) fixes it.

C. client.go:97 — the goroutine leak moved down a layer

2ecb26d reaped the window-level forwarders, but the five client.Client
event goroutines (updateLog, handleIncomingARQ, handleIncomingARQData,
handleIncomingBroadcast, handleStatus) still for … range over modem
channels that are never closed. Disconnect() closes the sockets and swaps
quit, so the modem readers exit — these five block forever. Every
Connect→Disconnect→Connect cycle strands five more, holding both the Client
and ModemClient alive.

D. mercury_chat_window.go:154 — the log grows without bound

logMsg does SetText(line + "\n" + cur) — a full rebuild every message, never
trimmed — and appendRichChat (:168) prepends a new RichText to the VBox
forever. client.handleIncomingARQData emits a line per RX chunk
(ARQ Data RX: %d bytes: %q), so a modest ARQ transfer produces thousands:
each append is O(n) plus a full re-wrap of a TextWrapBreak Entry.

The window becomes unusable during exactly the long transfers it exists to
drive. A ring buffer (cap the last N lines) fixes it.

---

Follow-ups (fine to land as-is)

• ui_communication.c:204 — the cfg_mutex fix missed one field:
  ctx->cfg.tx_gain_db = db; is outside the lock added two lines below,
  unlike the audio/radio branches where the writes were moved inside.
• ui_communication.c:219 — disabling never stops the publisher; it keeps
  waking at 20 Hz calling modem_get_rx_spectrum_seq for the process lifetime.
  Harmless output-wise (seq stops advancing) but a permanent timer on a Pi.
  Also: POSIX leaves spec_tid unspecified when pthread_create fails, so set
  ctx->spec_tid = 0 in the error branch or a failed start blocks re-enable
  forever.
• ui_communication.c:717 — pthread_mutex_destroy(&ctx->cfg_mutex) in
  ui_comm_shutdown has no barrier against ui_comm_set_waterfall, which can
  pass its g_ui_ctx NULL check just before shutdown NULLs it and then lock a
  destroyed mutex. Shutdown-only and narrow, but newly introduced by the mutex.
• mercury_chat_window.go:298 — onARQDisconnect still uses the
  if cw.mc != nil { cw.mc.… } two-read pattern 6797a7d removed everywhere
  else. Not exploitable (all callbacks are on the fyne main goroutine, the only
  writer) but worth matching.
• modem.go:181 — cbf8c0f moved the LogCh sends out of the lock in
  Disconnect() only. SendCommand still sends under mc.mu, and Connect()
  does it three times at :143. If LogCh (cap 100) fills — which it can once
  the leaked goroutines from C stop draining — SendCommand blocks forever
  holding mc.mu, wedging every other method including Disconnect().
• mercury_chat_window.go:236 — onConnect discards both strconv.Atoi
  errors, so a typo'd port becomes 0 and client.New silently substitutes
  8300/8100. Those defaults are also hardcoded rather than read from
  arq_tcp_base_port/broadcast_tcp_port, so they are wrong when mercury runs
  with -p/-b.

[rafael2k]

Verdict: safe to merge — go ahead and merge it, @pedromessetti

Third pass, verified from the diffs rather than the commit titles. All four
blockers are genuinely closed, and the earlier ten were re-checked in the
current tree for regressions — all hold.

Green on both build tags:

• go build ./... && go vet ./... (non-embedded) — clean
• make fyne-ui + go vet -tags mercury_embedded ./... (CGo, links
  libmercury_core.a) — clean
• gui_interface/mercury-client: go vet ./... — clean

blocker	commit	result
A — nil-deref + race in readers	fe23cbc	Fixed, all three. readARQControl/readARQData/readBroadcast each take mc.mu, snapshot conn+quit, unlock, and return early on nil. readARQData also snapshots ARQControlConn under the same lock, so the aliasing test is race-free.
B — second window evicts first	acac096	Fixed. Singleton set after build(), cleared in SetOnClosed, relaunch works; no race, both paths are on the Fyne main goroutine.
C — five leaked event goroutines	d8a051a	Mostly. done plumbing is correct — created in Connect (closing any stale one), read under c.mu paired with mc, closed and nil'd in Disconnect. No double-close, no send-on-closed. See follow-up 1.
D — unbounded log growth	93708b3	Fixed. Both caps enforced, newest entries kept. See follow-up 2 for the remaining cost.

No panic, crash or wire/protocol regression remains. Merge it — the rest is
follow-up material and should not hold up the branch.

---

Worth a follow-up PR

The first two are refinements of what you just fixed, not new problems.

1. client.go:255 — the last sliver of the goroutine leak (medium). The
five goroutines only observe done while parked in the receive select; every
forward is an unguarded blocking send (c.LogCh <- at 255/277/328/377/421,
ARQChatCh 352, BroadcastChatCh 397, StatusCh 422). During a sustained ARQ
RX, handleIncomingARQData emits a line per chunk and can fill the 256 buffer
while forwardLog is briefly behind; if the user hits "Disconnect modem" right
then, onDisconnect closes c.done and then cw.done, killing the only
drainer — a goroutine parked in that send never re-reaches the select.

select { case ch <- v: case <-done: return } closes it.

2. mercury_chat_window.go:265 — B's symptom via a different door (medium).
onConnect has no synchronous button guard, unlike onARQConnect, which does
cw.arqConnect.Disable() inline at :314 — that one is right. setTCP(true)
disables inside fyne.Do, i.e. a later main-loop iteration, while
Button.Tapped fires synchronously in d.pollEvents(). Two taps in one poll
batch (easy if the main loop lags, or key-repeat on a focused button) both run
onConnect: a second client opens three more sockets and evicts the first
control client — exactly what (B) was filed for — and client 1 is never
Disconnect()ed, leaking its 5 event goroutines, 3 readers and 3 TCP conns.

cw.connectBtn.Disable() synchronously at the top, and Disconnect() any
existing cw.mc before overwriting it.

3. modem.go:187 — SendCommand/Connect still send on LogCh under
mc.mu (medium). cbf8c0f fixed Disconnect only. Client.Connect fires
five SendCommands before its consumers exist; the 100-slot buffer absorbs it
today, but a full LogCh becomes a held mc.mu, which blocks every reader's
startup lock, IsConnected(), SendARQData() and Disconnect() — a
subsystem-wide stall driven from the UI thread. Same treatment as Disconnect:
collect, then send after unlock.

4. mercury_chat_window.go:183 — cost per line (medium). The cap bounds
memory, but each line still does strings.Split of ~1000 lines + Join + a
full Entry.SetText/Refresh on the main goroutine, and fyne.Do queues onto
an unbounded funcQueue. If chunk rate exceeds rebuild rate during a multi-kB
transfer the queue grows and the UI falls further behind for the whole transfer
(and feeds 2). Suggest maxLogLines ≈ 200 and a []string ring rather than
re-splitting the widget's own text.

5. modem.go:129 — 7731178's alias guard is dead code (low, new).
mc.ARQControlConn is nil'd three lines above, so
mc.ARQDataConn != mc.ARQControlConn is always true. When the data conn is an
alias (the ARQDataAddr == "" branch at :161) and the broadcast dial fails,
the same *net.TCPConn is closed twice. Same at :370 in Disconnect, which
also logs a bogus "Disconnected from ARQ Data." Not reachable from this UI
(client.Connect always uses ARQPort+1), but modem is standalone. Snapshot
the control pointer before nilling it.

6. modem.go:229 — ConnectARQ can't be cancelled (low). The wait selects
only on respCh and a 120 s timeout, never mc.quit. "Connect ARQ" to a silent
peer then "Disconnect modem" leaves a goroutine alive up to two minutes, which
then calls cw.setARQ(false) — re-enabling the button on a downed modem — and
cw.logMsg against a possibly-closed window.

7. link_engine.go:150 — the 100 ms time.AfterFunc papers over the engine
race (low). set_audio_config restarts audioio; if that exceeds 100 ms the
refresh reads the pre-change device list and the dialog shows a stale selection,
with no retry. The timer also fires after Close().

Plus the earlier follow-ups still open, all low: onARQDisconnect's two-read
pattern (:327, the last instance), discarded strconv.Atoi errors (:266),
ctx->cfg.tx_gain_db written outside cfg_mutex (ui_communication.c:204),
spec_tid not zeroed on pthread_create failure and disable never stopping the
publisher (:231), and ui_comm_set_waterfall loading g_ui_ctx before
locking a mutex ui_comm_shutdown may already have destroyed (:717).

---

Once this lands I will rebase #175 (TX waterfall) on top and drop my
modem_set_tx_spectrum_enabled() so your modem_set_spectrum_enabled() gates
both the RX and TX FFTs — one switch, one setter. Note the disable direction
(:231 above) applies to that combined path too.

Also: README.md here may want a refresh — trunk gained a Go/fyne
prerequisites section in 59fa549.

Good, fast work on both rounds.