Follow-up fixes for fyne-ui-v2 (PR #174 review comments)

[pedromessetti]

Summary

Follow-up fixes addressing the review comments raised on #174.

Client / modem (goroutine & socket lifecycle)

• client: guard forwarder sends — every forward now selects on the done channel, so a goroutine parked in a blocked send always observes disconnect (last sliver of the goroutine leak).
• client: reap the five event goroutines — add a done channel to Client, created on Connect and closed on Disconnect.
• modem: stop sending on LogCh while holding mc.mu — Connect/SendCommand now flush logs after unlocking.
• modem: make ConnectARQ cancellable — select on mc.quit, so a disconnect mid-handshake doesn't leave a goroutine alive for 120 s.
• modem: snapshot control conn before nilling — fixes dead alias guard and double-close in Connect/Disconnect.
• modem: close sockets on partial Connect failure — deferred cleanup closes any conns opened before the failing dial.

Chat window

• guard onConnect against double-tap — synchronous button disable + disconnect existing client before reconnecting.
• singleton window — reuse the open chat window instead of evicting the incumbent control client.
• ring-buffer the log — cap at 200 lines, no re-splitting of the widget text.
• validate ports — surface strconv.Atoi errors instead of silently defaulting.
• read real engine ports — new CGo bridge (mercury_ui_get_tcp_ports) reads arq_tcp_base_port / broadcast_tcp_port from config so -p/-b overrides are respected.
• capture cw.mc locally in onARQDisconnect — consistency.

Engine link

• retry device-list refresh — replace the one-shot 100 ms AfterFunc with ctx-cancellable retries (200 ms × 5) so slow audioio/hamlib restarts aren't reported as stale.

ui_communication.c

• tx_gain_db under cfg_mutex — move the write inside the lock.
• stop spectrum publisher on disable — atomic spec_run flag + pthread_join (thread now joinable); spec_tid = 0 on create failure so re-enable isn't blocked.
• drop pthread_mutex_destroy — avoids racing a live ui_comm_set_waterfall at shutdown.

[rafael2k]

Reviewed all 10 commits. This is merge-ready — go ahead and merge it, @pedromessetti. Every item from the #174 follow-up list is addressed, and the two most subtle ones (the deferred-flush ordering and the alias snapshot) are done correctly.

Verified

Item	Verdict
sendOrStop on all five forwarders	Correct — and updateRemoteCall threads done through and returns bool rather than swallowing it
Connect() deferred log flush	Correct LIFO: the unlock+flush defer is registered first so it runs last, and the conn-cleanup defer still runs under mc.mu. The comments say exactly this
SendCommand log outside the lock	Correct. Conn can be closed between unlock and Write, which just returns an error — benign
Control-conn snapshot	Fixes the real one: the guard compared against a pointer nil'd three lines above, so it was always true and the aliased conn was double-closed. Both Connect() cleanup and Disconnect() fixed
ConnectARQ cancellable	quit := mc.quit captured under the lock — right, since a later Connect() reassigns it
onConnect double-tap	Synchronous Disable() at the top, re-enabled on all three error paths
cw.done vs c.done	Distinct channels; every close is paired with a nil assignment on the Fyne main goroutine. No double-close
spec_tid = 0 on create failure	Correct — a failed start no longer wedges re-enable
Not destroying cfg_mutex	Right call, and the comment explains why

I also checked the join for deadlock: spectrum_publisher_thread never takes cfg_mutex, so joining under it is safe. Builds clean here — make fyne-ui, mercury-client, and go vet on both the plain and mercury_embedded tags.

Two follow-ups (not blockers)

1. ui_comm_shutdown doesn't stop or join the thread it just made joinable (ui_communication.c:739). You removed the pthread_detach and join on the disable path, but shutdown still only does g_ui_ctx = NULL + ws_shutdown. Nothing sets spec_run = false there, so the thread is joinable and never joined.

Impact today is nil — g_ui_ctx is a static ui_ctx_t that outlives everything, and ws_broadcast_binary guards on ctx->running, which ws_shutdown clears first, so the post-teardown broadcasts are no-ops. But it's free to close, and this codebase has already paid for teardown ordering once (396568a, rings freed under live modem threads):

atomic_store_explicit(&ctx->spec_run, false, memory_order_relaxed);
if (ctx->spec_tid != 0) { pthread_join(ctx->spec_tid, NULL); ctx->spec_tid = 0; }
ws_shutdown(&ctx->ws);

2. The log flush is the one unguarded blocking send left (modem.go:122, and :199 in SendCommand). Moving it out from under mc.mu fixed the part that mattered — a full LogCh no longer stalls every reader, IsConnected, SendARQData and Disconnect. But the send itself can still park forever if nothing drains the channel, which is the same class sendOrStop exists for on the client side. mc.quit is already captured under the lock in ConnectARQ, so the same shape works here.

Note for #175

This changes the switch my TX-waterfall PR sits on, in a good way. With #177 in, turning the waterfall off stops the RX FFT, the TX FFT (I rebased #175 to gate on your modem_set_spectrum_enabled and dropped my separate setter), and the publisher thread — the whole chain, one control. I'll re-run #175's gate once this lands.