Repository navigation
fix: run ExposureWidget and PropertyWidget core-event handlers on the main thread - #563
Draft
hinderling wants to merge 2 commits into
Draft
hinderling wants to merge 2 commits into
hinderling wants to merge 2 commits into
Conversation
With the psygnal signal backend, core events run their handlers on the emitting thread. During an acquisition that is the MDA thread, so _on_exp_changed / _on_load mutated the spinbox from a non-GUI thread. Cross-thread widget mutation is undefined behavior, and in particular the signals_blocked guard is released before Qt processes the cross-thread setValue, so the spinbox can later emit valueChanged (which is wired to mmc.setExposure) with a stale value, overriding the exposure of frames in a running acquisition. Observed in the field as episodic two-state brightness flicker in multichannel timelapses: random frames acquired at the widget's exposure instead of the event's, with per-channel brightness ratios matching one stray exposure value. Both handlers now use superqt's ensure_main_thread, so the update is queued to the GUI thread and applied inside the signals_blocked guard. The added test asserts that a worker-thread event does not mutate the spinbox synchronously and lands without emitting valueChanged.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #563 +/- ##
==========================================
+ Coverage 87.36% 87.42% +0.06%
==========================================
Files 115 115
Lines 14098 14103 +5
==========================================
+ Hits 12317 12330 +13
+ Misses 1781 1773 -8 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
hinderling
marked this pull request as draft
September 1, 2026 22:19
Core events can arrive on a worker thread. Setting the compound slider/spinbox from there lets the inner widgets' valueChanged reach the main thread after blockSignals is released, and the widget writes the value back to the core. Wrap _on_core_changed and _on_config_loaded in ensure_main_thread, add a test, drop the unused type-ignore comments and trim the comments in ExposureWidget.
hinderling
changed the title
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Core events can be delivered on a thread other than the GUI thread (for example with the psygnal signal backend during an acquisition). Two widgets update Qt widgets directly inside those handlers.
PropertyWidget._on_core_changedsets its slider/spinbox value underblockSignals. When called from a worker thread, the inner slider and spinboxvalueChangedsignals are delivered to the main thread after the block is released, so_on_widget_changedwrites the value back into the core. With the Device Property Browser open during an acquisition, this rewrote the SLMExposureTimeseveral hundred times per run on a microscope and changed the illumination of frames. Reproducible with the demo config: 6 core writes from a worker thread produce 12 write-backs from the main thread.ExposureWidget._on_exp_changedand_on_loadset a spinbox from the worker thread. That is not thread safe and crashes the process under load (demo config, 6 of 9 runs).Fix
Wrap the four handlers in superqt's
ensure_main_thread. Under the Qt backend these calls already arrive queued on the main thread, so there is no behavior change there.Tests
One test per widget calls the handler from a worker thread and asserts that the widget updates and nothing is written back to the core. Both fail on
main.