Review: FreeDV Reporter station reporting with RADE integration

Nice contribution, @NF0T — this is a well-scoped feature with a thorough PR description and solid test plan. The architecture of routing SNR/sync signals through MainWindow to the FreeDV worker thread is the right approach. A few items to consider:

Design / correctness

1. m_authNeedsRefresh reconnect race — In onWsDisconnected(), the new m_authNeedsRefresh path calls m_ws->open() directly without going through the reconnect timer. If the server rejects the new auth or the open fails synchronously, onWsDisconnected would fire again with m_authNeedsRefresh == false, falling into the exponential backoff path. This is fine for resilience, but it means the user's toggle won't have immediate effect — worth a debug log at minimum. Also, if enableReporting() is called twice rapidly before the first reconnect completes, m_authNeedsRefresh could be set again while the socket is mid-close. Consider guarding against that (e.g. checking m_ws->state() before calling close() in enableReporting()).

2. disableReporting() sends events then immediately closes — sendEvent("message_update", ...) and sendEvent("freq_change", {freq: 0}) are followed by m_ws->close(). Since QWebSocket::sendTextMessage() buffers to the OS, this should flush before the close handshake, but it's worth noting this relies on the WebSocket close handshake not racing the sends. In practice this is fine with Qt's implementation, but a comment documenting the assumption would help future readers.

3. rx_report with empty callsign — The onSnrTimer() sends rx_report with callsign: "". The PR description notes the server may silently drop these. If the server does process them, this could create phantom entries in the reporter's "heard" table. Consider either not sending rx_report at all until callsign identification is available, or gating it behind a config flag. The infrastructure being "in place" is good forward thinking, but sending empty data to a production server is a risk.

4. hasGpsHardware() helper — The diff adds this to RadioModel.h but I don't see it in the current base. It follows the existing model-detection pattern (m_model.contains(...)) which is good. However, the PR description mentions "FLEX-8000 class and Aurora" — make sure the string matching covers all 8000-series variants (8400, 8600) and the Aurora model string correctly. The existing code uses patterns like m_model.contains("8600") — does the 8400 have GPS? If not, the helper name hasGpsHardware should be documented to clarify which models qualify.

Thread safety

5. updateRxSnr() / updateRxSynced() called cross-thread — These are called from RADEEngine signals (worker thread) and store to m_lastSnr / m_radeSynced which are read by onSnrTimer() (FreeDV worker thread). The PR description mentions QMetaObject::invokeMethod for thread safety in MainWindow, but the FreeDvClient members m_lastSnr (float) and m_radeSynced (bool) are written from one thread and read from another without synchronization. On x86 this is benign for aligned floats/bools, but it's technically a data race. Consider either:

   • Making these std::atomic<float> / std::atomic<bool>, or
   • Ensuring the signals are connected with Qt::QueuedConnection so writes happen on the FreeDV thread

   If the connections from RADEEngine → FreeDvClient are queued (both live on different threads with event loops), this is already safe. Just want to confirm that's the case.

Scope

6. .gitignore change — Adding .vscode/c_cpp_properties.json is fine but tangential to this PR. Not blocking, just noting it's unrelated to the feature.

Minor

7. SNR timer interval — The 1-second SNR reporting interval (m_snrTimer->setInterval(1000)) is reasonable, but the FreeDV Reporter server may have its own rate limits. The PR description mentions a 5-minute keepalive for station_update — is the 1-second rx_report rate documented as acceptable by the server? If the server throttles or drops high-rate reports, the timer could be bumped to 5s without losing much value.

8. deactivateRADE() "bug fix" — The PR claims m_radeSliceId was zeroed before teardown read it, but looking at the existing code, nothing after m_radeSliceId = -1 (line 10691) references the slice ID — the remaining teardown uses m_radeEngine directly. The fix is correct in the context of the new stopFreeDvReporting() code that needs the slice ID, but characterizing it as a "pre-existing bug" is slightly misleading. It's more accurately "capture slice ID before the block that resets it, since we now need it for reporting teardown."

Overall

This is solid work. The opt-in design, AppSettings persistence, and GPS/callsign sync from the radio are all well thought out. The thread-safety question (item 5) is the most important to resolve before merge. Everything else is minor or informational.

Thanks for the contribution! 73