Skip to content

fix(ui): prevent segfault during init — call _vt_append via Qt signal from background thread - #1

Open
edufortes wants to merge 1 commit into
oe3gas:mainfrom
edufortes:fix/vt-append-thread-safety
Open

fix(ui): prevent segfault during init — call _vt_append via Qt signal from background thread#1
edufortes wants to merge 1 commit into
oe3gas:mainfrom
edufortes:fix/vt-append-thread-safety

Conversation

@edufortes

Copy link
Copy Markdown

Problem

Every connect/init sequence ends in a segfault:

INFO pk232py.comm.params_uploader ParamsUploader: upload complete (59 commands)
[1] 464793 segmentation fault (core dumped) pk232py

Root Cause

_on_verbose_mode_ready spawns a PK232-ParamUpload background thread
that calls _vt_append() directly — including via the echo_callback
passed to ParamsUploader. _vt_append manipulates Qt widgets
(QTextCursor, insertText, ensureCursorVisible), which must only
be called from the GUI thread. Doing so from a background thread is
undefined behaviour in PyQt6 and causes the segfault.

Fix

  • Add _vt_append_signal = pyqtSignal(str, str) to MainWindow
  • Connect it to _vt_append in _connect_signals() (queued → GUI thread)
  • Replace all direct _vt_append() calls inside the _upload() closure
    with a local _vt() wrapper that emits the signal instead
  • Pass _vt as echo_callback to ParamsUploader

Testing

Reproduced segfault on Linux with a real PK-232MBX (v7.1). After this
fix the full init sequence completes without crash and the verbose
terminal displays all uploaded parameters correctly.

  background thread

  PK232-ParamUpload thread was calling _vt_append() (QTextCursor ops)
  directly — undefined behaviour in PyQt6, causes segfault at end of
  params upload. Add _vt_append_signal(str,str) connected to _vt_append
  in the GUI thread; background thread emits the signal instead.

@oe3gas oe3gas left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the report and the fix — the diagnosis is exactly right, and the reproduction with real hardware is much appreciated. I'd never seen this on Windows, but that was timing luck rather than a working code path: the uploader idles ≥120 ms between commands, so the GUI thread rarely collides with it. The bug was always there, just latent. Good catch.

The signal/queued-connection approach is the right one, and keeping the default colour in the _vt() wrapper was the correct call — without it the single-argument uses would have raised a TypeError silently inside a daemon thread.

Two things before I can merge — see the inline comments. Short version: _log_monitor() is still called directly from the worker thread and has the same problem, and the two setFocus() calls were dropped rather than moved.

One suggestion, entirely optional: instead of adding a second bridge signal, it might be cleaner to move the whole post-upload tail into the GUI thread — something like _upload_done_signal = pyqtSignal(int, bool) emitted at the end of _upload(), with a slot that handles setFocus() and enter_host_mode(). That leaves the worker thread doing nothing but blocking serial I/O and no UI knowledge at all. Happy either way — the two-signal version is fine if you prefer the smaller diff.

I'll also add the GUI-thread rule to CLAUDE.md §3 and list the PK232-ParamUpload thread in the state-machine doc, where it's currently missing. That omission is arguably why this slipped through in the first place.

Once the two points above are addressed I'll test on Windows (COM16, PK-232MBX v7.1) — including the fast-init branch, which has its own code path.

if fast_init:
self._vt_append("[SYS] Fast Init — parameter upload skipped\n")
_vt("[SYS] Fast Init — parameter upload skipped\n")
self._log_monitor("[SYS] Fast Init active — no parameter upload")

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_log_monitor() has the same threading problem as _vt_append() — it writes to the self._monitor QTextEdit and reads self._mon_btn_hex.isChecked(), both GUI-thread-only. Since it's still called directly from _upload(), the segfault isn't fully eliminated, just made much rarer (2 calls instead of 61). Could you route it through a signal as well? Something like _log_monitor_signal = pyqtSignal(str) connected to _log_monitor, following the same pattern you used for _vt_append. Same applies to line 955.

else:
self._vt_append("[SYS] Verbose terminal ready (fast init)\n")
self._vt_input.setFocus()
_vt("[SYS] Verbose terminal ready (fast init)\n")

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The self._vt_input.setFocus() call was removed here and at the end of the upload path, but isn't mentioned in the PR description. I understand why — it's a GUI call from a background thread and had to go. But dropping it is a silent behaviour change: after the upload completes, the cursor no longer returns to the command input, which is noticeable if the user clicked elsewhere during the ~7 s upload. Could it be restored via the GUI thread instead of removed?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants