Radio control self-deadlock + cross-thread GTK calls on rigctld connection errors

[N6RFM]

Hi Alex,

Further investigation of rotor control and GTK updates revealed the following. Again, the trouble shooting was done very interactively using the Claude LLM with results reported below. Implementation of the suggested fixes in these three issues has solved the problems I was trying to address.

Thank Again,

Bob
N6RFM

Summary

When the rigctld connection's error count reaches MAX_ERROR_COUNT,
the background worker thread (rigctl_run) calls
gtk_toggle_button_set_active() on the Engage button directly, from
the worker thread itself. This has two consequences:

1. It synchronously re-enters the same thread via rig_engaged_cb's
   disengage branch, which waits on a condition variable that only
   code further up the very same function (unreachable until this
   call returns) can ever signal — a permanent self-deadlock.
2. More generally, gtk_toggle_button_set_active() and
   gtk_widget_set_sensitive() are GTK widget functions being called
   from a non-main thread, which is undefined behavior in GTK3 (GTK
   is not thread-safe).

Environment

• Reproduced on current master
• GTK 3.24.41, GLib 2.80.0
• Reproduced in a headless sandbox using a minimal fake
  rigctld-protocol TCP server (standing in for a real rig or a GRC
  flowgraph exposing a rigctld-compatible socket), so it is not
  specific to any particular radio or flowgraph.

Steps to reproduce

1. Configure a radio device pointing at a rigctld-compatible TCP
   endpoint (real hardware, rigctld, or a flowgraph implementing
   the same text protocol).
2. Engage Radio Control and let it run a few normal cycles.
3. Kill/stop the process on the other end of the connection (e.g.
   restart a GNU Radio Companion flowgraph that's providing the
   socket) without Gpredict itself being told to disengage.
4. Wait for the error count to reach MAX_ERROR_COUNT (5 cycles).
5. Restart the remote endpoint, then click Engage again in Gpredict.

Expected: Gpredict disengages cleanly after the error threshold,
and a subsequent Engage opens a fresh connection to the restarted
endpoint.

Actual: The worker thread deadlocks permanently at step 4 (see
"Root cause"). ctrl->engaged is set to FALSE and the toggle
button visually reflects "disengaged" (since that line runs before
the deadlocking call), but the thread itself never exits, and
ctrl->sock is never reset. Any later Engage reuses the dead,
stale socket instead of reopening a connection — confirmed directly:
after simulating a restart, the new listening endpoint received zero
connection attempts, while Gpredict kept logging errors against the
old, already-closed socket.

Root cause

In rigctl_run() (src/gtk-rig-ctrl.c):

if (t_ctrl->errcnt >= MAX_ERROR_COUNT)
{
    /* disengage device */
    gtk_toggle_button_set_active(GTK_TOGGLE_BUTTON(t_ctrl->LockBut),
                                 FALSE);
    t_ctrl->engaged = FALSE;
    t_ctrl->errcnt = 0;
    sat_log_log(...);
}

gtk_toggle_button_set_active() synchronously fires the button's
"toggled" signal, invoking rig_engaged_cb() — in the same call
stack, same thread:

if (!gtk_toggle_button_get_active(button))
{
    gtk_widget_set_sensitive(ctrl->DevSel, TRUE);
    gtk_widget_set_sensitive(ctrl->DevSel2, TRUE);
    ctrl->engaged = FALSE;

    g_mutex_lock(&ctrl->widgetsync);
    setconfig(ctrl);
    g_cond_wait(&ctrl->widgetready, &ctrl->widgetsync);   /* <-- hangs here */
    g_mutex_unlock(&ctrl->widgetsync);
    ctrl->rigctl_thread = NULL;
}

The only code that ever calls g_cond_signal(&ctrl->widgetready) is
rigctl_run's own else branch (the normal, non-error disengage
path), reached only at the top of its while (1) loop when
t_ctrl->engaged is false — which cannot happen until the current,
nested call returns. It never does. The thread parks in g_cond_wait
permanently, and rigctrl_close() (which resets ctrl->sock to 0)
— also only reachable from that same unreached branch — never runs.

This is a real (not theoretical) violation reproduced deterministically
in a sandbox: the log line "MAX_ERROR_COUNT (%d) reached. Disengaging device!", which is logged after the deadlocking call in the current
code, never appeared in any test run, confirming the thread never got
past that point.

Independently of the deadlock, calling GTK widget functions
(gtk_toggle_button_set_active, gtk_widget_set_sensitive) from a
non-main thread is undefined behavior in GTK3 regardless of whether a
clean deadlock results — a plausible explanation for reports of
intermittent full-application freezes in real-world use that may not
reproduce identically every time, since UB doesn't have to fail the
same way twice.

Suggested fix

Perform the actual cleanup (engaged flag, error count reset, socket
close, timer removal) directly on the worker thread — where it's
already running and already safe to do — then marshal only the GTK
widget updates onto the main thread via g_idle_add(). Block the
"toggled" signal while updating the button so rig_engaged_cb's own
disengage branch (meant for user-initiated Engage/Disengage on the
main thread) doesn't run a second time:

static gboolean rig_auto_disengage_ui_cb(gpointer data)
{
    GtkRigCtrl *ctrl = GTK_RIG_CTRL(data);

    g_signal_handlers_block_by_func(ctrl->LockBut, rig_engaged_cb, ctrl);
    gtk_toggle_button_set_active(GTK_TOGGLE_BUTTON(ctrl->LockBut), FALSE);
    g_signal_handlers_unblock_by_func(ctrl->LockBut, rig_engaged_cb, ctrl);

    gtk_widget_set_sensitive(ctrl->DevSel, TRUE);
    gtk_widget_set_sensitive(ctrl->DevSel2, TRUE);
    ctrl->rigctl_thread = NULL;

    return FALSE;
}

And in rigctl_run's error-count check:

if (t_ctrl->errcnt >= MAX_ERROR_COUNT)
{
    sat_log_log(SAT_LOG_LEVEL_ERROR,
                _("%s:%s: MAX_ERROR_COUNT (%d) reached. Disengaging device!"),
                __FILE__, __func__, MAX_ERROR_COUNT);

    t_ctrl->engaged = FALSE;
    t_ctrl->errcnt = 0;

    if (t_ctrl->sock > 0)
        rigctrl_close(t_ctrl);
    if (t_ctrl->timerid)
        remove_timer(t_ctrl);

    g_idle_add(rig_auto_disengage_ui_cb, t_ctrl);
    break;
}

Verified in sandbox: with this change, the MAX_ERROR_COUNT log line
correctly appears (previously unreachable), and a subsequent Engage
successfully opens a fresh connection to a restarted endpoint, with a
full handshake and normal command cycling — confirmed by the new
endpoint actually receiving and completing a connection, which never
happened before.

Happy to open a PR with this change if useful.

[N6RFM]

Related: #423 and #424 report similar timing/state-handling issues in the sibling rotor control code.