Fix firmware version display in Sonication Control page by alkagan · Pull Request #562 · OpenwaterHealth/SlicerOpenLIFU

alkagan · 2026-03-26T18:46:09Z

This closes #482 and replaces/closes #552

Copilot

Pull request overview

Adds firmware/SDK version display to the Sonication Control module and improves thread-safety around hardware monitor events/logging, aligning with #482’s request to surface connected device version information in the UI.

Changes:

Add three new UI labels to display SDK, console FW, and TX module FW versions.
Populate/clear those labels on device connect/disconnect via a new updateVersionLabels() helper.
Refactor hardware monitor callbacks to cross the monitor-thread → main-thread boundary via Qt signals (and prevent Slicer UI logging from background threads).

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.

File	Description
`OpenLIFUSonicationControl/Resources/UI/OpenLIFUSonicationControl.ui`	Adds SDK/console/TX version `QLabel`s in the connection status frame.
`OpenLIFUSonicationControl/OpenLIFUSonicationControl.py`	Implements version label updates and adjusts device-connect plumbing (Qt signals, reinit logic, logging).
`OpenLIFULib/OpenLIFULib/util.py`	Guards Slicer UI log handler against non-main-thread emission to avoid Qt cross-thread crashes.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-03-26T18:50:45Z

+    def updateVersionLabels(self):
+        """Populate SDK / console / TX firmware version labels when both devices are connected."""
+        if self._cur_device_connected_state == DeviceConnectedState.CONNECTED:


Version labels are only refreshed in onDeviceConnected/onDeviceDisconnected. In setup() you already call updateDeviceConnectedStateFromDevice(), but you never call updateVersionLabels(), so opening the module while the device is already connected will leave these labels blank until a reconnect event happens. Consider calling updateVersionLabels() after the initial connection-state sync (and after reinitialize/enter as well).

Copilot · 2026-03-26T18:50:46Z

+            self.monitoring_timer.stop()
+            self.stop_monitoring()
+            self.cur_lifu_interface.close()
+        except Exception as e:
+            logging.warning("[LIFU] Error during interface cleanup: %s", e)
+


reinitialize_lifu_interface() replaces _monitor_loop/_monitor_thread without joining the prior monitor thread or closing the prior event loop. If reinitialize is triggered repeatedly, this can leak resources and may leave old monitor threads running briefly against stale state. Consider waiting for the old thread to exit (join with timeout) and explicitly closing the old event loop once stopped.

Copilot · 2026-03-26T18:50:46Z

    def emit(self, record):
+        # Qt UI calls are only safe on the main thread. If this log record was emitted
+        # from a background thread (e.g. UART monitor/read threads), skip Qt interactions
+        # entirely to avoid QObject::setParent and processEvents cross-thread crashes.
+        if qt.QThread.currentThread() is not slicer.app.thread():
+            return


This early return drops all Slicer UI surfacing for log records emitted off the main thread (including warnings/errors). If this handler is the primary feedback path for these loggers, important messages may be silently lost. Consider queuing the UI work onto the main thread (e.g., via a singleShot/invokeMethod) or at least falling back to non-Qt output when off-thread, instead of returning.

maybe at least log something so that we know it is dropping

georgevigelette · 2026-03-26T19:12:53Z

this needs to be fixed i had it hardcoded to my path for testing

georgevigelette · 2026-03-26T19:13:29Z

    def emit(self, record):
+        # Qt UI calls are only safe on the main thread. If this log record was emitted
+        # from a background thread (e.g. UART monitor/read threads), skip Qt interactions
+        # entirely to avoid QObject::setParent and processEvents cross-thread crashes.
+        if qt.QThread.currentThread() is not slicer.app.thread():
+            return


maybe at least log something so that we know it is dropping

ebrahimebrahim

This looks good! Just a couple of questions and we should be good to go

@alkagan Can you confirm that this works with openlifu v0.18.0 specifically? Or were you using some other test version of openlifu that fixed the crash? Just checking whether we need to update the python requirements within this PR.

minor: updateVersionLabels was not being called in setup and so it's not shown initially if there's nothing to trigger it to be called (e.g. if there's no device connected at all). I just added this myself.

alkagan · 2026-04-08T21:39:31Z

This looks good! Just a couple of questions and we should be good to go

@alkagan Can you confirm that this works with openlifu v0.18.0 specifically? Or were you using some other test version of openlifu that fixed the crash? Just checking whether we need to update the python requirements within this PR.

minor: updateVersionLabels was not being called in setup and so it's not shown initially if there's nothing to trigger it to be called (e.g. if there's no device connected at all). I just added this myself.

Yes, openlifu will need to be updated because we also need to make a PR for the feature/bootloader-slave-enablement branch in order for this to work properly. Thanks for the addition!

Edit: so with this change we expect updateVersionLabels() to be called in setup and there to be a display regardless if a device is connected or not? Because if so I just tested it out and it still does not show anything if it does not detect a connected device.

ebrahimebrahim · 2026-04-08T23:01:52Z

Edit: so with this change we expect updateVersionLabels() to be called in setup and there to be a display regardless if a device is connected or not? Because if so I just tested it out and it still does not show anything if it does not detect a connected device.

I suppose it doesn't. updateVersionLabels needs to be updated to operate this way. It's still good to call it in setup in case someone ever modifies updateVersionLabels. Currently updateVersionLabels gives up on putting any labels in the event that no device is connected. That's okay, we can change that behavior later if we want to.

alkagan requested review from arhowe00, Copilot, georgevigelette and sadhana-r March 26, 2026 18:46

Copilot started reviewing on behalf of alkagan March 26, 2026 18:46 View session

alkagan force-pushed the feature/sonication-controller branch from b1bf44b to 3c3fc80 Compare March 26, 2026 18:50

Copilot AI reviewed Mar 26, 2026

View reviewed changes

georgevigelette reviewed Mar 26, 2026

View reviewed changes

georgevigelette force-pushed the feature/sonication-controller branch 3 times, most recently from 220ff5d to 303db1b Compare March 26, 2026 22:37

georgevigelette self-requested a review March 26, 2026 22:38

georgevigelette approved these changes Mar 26, 2026

View reviewed changes

alkagan force-pushed the feature/sonication-controller branch from 303db1b to 35f03a5 Compare March 27, 2026 22:58

ebrahimebrahim mentioned this pull request Apr 1, 2026

482 add firmware version display to sonication control module #552

Closed

ebrahimebrahim requested changes Apr 2, 2026

View reviewed changes

Comment thread OpenLIFUSonicationControl/OpenLIFUSonicationControl.py

Comment thread OpenLIFUSonicationControl/OpenLIFUSonicationControl.py Outdated

georgevigelette and others added 5 commits April 8, 2026 18:58

sonication control get versions (#482)

88cf665

update (#482)

8b8c3e7

Add firmware display for multiple TX modules, debug logging (#482)

166c61a

Show version labels on module open (#482)

63b272a

Move logging to after docstring (#482)

349ae13

ebrahimebrahim force-pushed the feature/sonication-controller branch from 42ea1bb to 349ae13 Compare April 8, 2026 23:02

ebrahimebrahim approved these changes Apr 8, 2026

View reviewed changes

ebrahimebrahim merged commit dd74c60 into main Apr 8, 2026
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix firmware version display in Sonication Control page#562

Fix firmware version display in Sonication Control page#562
ebrahimebrahim merged 5 commits into
mainfrom
feature/sonication-controller

alkagan commented Mar 26, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI Mar 26, 2026

Uh oh!

Copilot AI Mar 26, 2026

Uh oh!

Copilot AI Mar 26, 2026

Uh oh!

georgevigelette Mar 26, 2026

Uh oh!

georgevigelette Mar 26, 2026

Uh oh!

georgevigelette Mar 26, 2026

Uh oh!

ebrahimebrahim left a comment

Uh oh!

Uh oh!

Uh oh!

alkagan commented Apr 8, 2026 •

edited

Loading

Uh oh!

ebrahimebrahim commented Apr 8, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

alkagan commented Mar 26, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Copilot AI Mar 26, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 26, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 26, 2026

Choose a reason for hiding this comment

Uh oh!

georgevigelette Mar 26, 2026

Choose a reason for hiding this comment

Uh oh!

georgevigelette Mar 26, 2026

Choose a reason for hiding this comment

Uh oh!

georgevigelette Mar 26, 2026

Choose a reason for hiding this comment

Uh oh!

ebrahimebrahim left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

alkagan commented Apr 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ebrahimebrahim commented Apr 8, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

alkagan commented Apr 8, 2026 •

edited

Loading