From 0467d88010624a01b1bc73b52a85295a4f73f91f Mon Sep 17 00:00:00 2001 From: appel_c Date: Thu, 8 Jan 2026 07:52:07 +0100 Subject: [PATCH] fix(device-form-dialog): Adapt device-form-dialog ophyd validation test --- .../device_form_dialog.py | 55 +++++++++++++++++-- .../device_manager_display_widget.py | 10 ++-- .../components/device_table/device_table.py | 35 +++++++----- .../ophyd_validation/ophyd_validation.py | 27 ++++++++- 4 files changed, 102 insertions(+), 25 deletions(-) diff --git a/bec_widgets/applications/views/device_manager_view/device_manager_dialogs/device_form_dialog.py b/bec_widgets/applications/views/device_manager_view/device_manager_dialogs/device_form_dialog.py index 7acc7a74..c0453f93 100644 --- a/bec_widgets/applications/views/device_manager_view/device_manager_dialogs/device_form_dialog.py +++ b/bec_widgets/applications/views/device_manager_view/device_manager_dialogs/device_form_dialog.py @@ -1,5 +1,7 @@ """Dialogs for device configuration forms and ophyd testing.""" +from typing import Any, Iterable, Tuple + from bec_lib.atlas_models import Device as DeviceModel from bec_lib.logger import bec_logger from ophyd_devices.interfaces.device_config_templates.ophyd_templates import OPHYD_DEVICE_TEMPLATES @@ -20,6 +22,7 @@ from bec_widgets.widgets.control.device_manager.components.ophyd_validation impo ) DEFAULT_DEVICE = "CustomDevice" +_ValidationResultIter = Iterable[Tuple[dict[str, Any], ConfigStatus, ConnectionStatus, str]] logger = bec_logger.logger @@ -194,6 +197,9 @@ class DeviceFormDialog(QtWidgets.QDialog): for widget in self._control_widgets.values(): widget.close() widget.deleteLater() + if self._wait_dialog is not None: + self._wait_dialog.close() + self._wait_dialog.deleteLater() @property def config_validation_result(self) -> tuple[dict, int, int, str]: @@ -274,14 +280,36 @@ class DeviceFormDialog(QtWidgets.QDialog): Create and show a validation progress dialog while validating the device configuration. The dialog will be modal and prevent user interaction until validation is complete. """ - wait_dialog = QtWidgets.QProgressDialog("Validating… please wait", None, 0, 0, parent=self) + wait_dialog = QtWidgets.QProgressDialog( + "Validating config… please wait", None, 0, 0, parent=self + ) wait_dialog.setWindowModality(QtCore.Qt.WindowModality.ApplicationModal) wait_dialog.setCancelButton(None) wait_dialog.setMinimumDuration(0) return wait_dialog + @SafeSlot(list) + def _handle_devices_already_in_session_results( + self, validation_results: _ValidationResultIter + ) -> None: + """Handle completion if device is already in session.""" + if len(validation_results) != 1: + logger.error( + "Expected a single device validation result, but got multiple. Using first result." + ) + result = validation_results[0] if len(validation_results) > 0 else None + if result is None: + logger.error( + f"Received validation results: {validation_results} of unexpected length 0. Returning." + ) + return + device_config, config_status, connection_status, validation_msg = result + self._handle_validation_result( + device_config, config_status, connection_status, validation_msg + ) + @SafeSlot(dict, int, int, str) - def _validation_complete( + def _handle_validation_result( self, device_config: dict, config_status: int, connection_status: int, validation_msg: str ): """Handle completion of validation.""" @@ -297,8 +325,8 @@ class DeviceFormDialog(QtWidgets.QDialog): f"Device config validation changed for config: {device_config} compared to previous validation. Using status from recent validation ." ) self._validation_result = (device_config, config_status, connection_status, validation_msg) - self._wait_dialog.finished.emit(0) if self._wait_dialog is not None: + self._wait_dialog.accept() self._wait_dialog.close() self._wait_dialog.deleteLater() self._wait_dialog = None @@ -327,10 +355,25 @@ class DeviceFormDialog(QtWidgets.QDialog): self._wait_dialog = self._create_validation_dialog() ophyd_validation = OphydValidation() - ophyd_validation.validation_completed.connect(self._validation_complete) - ophyd_validation.change_device_configs([config], True, False) + ophyd_validation.validation_completed.connect(self._handle_validation_result) + ophyd_validation.multiple_validations_completed.connect( + self._handle_devices_already_in_session_results + ) - res = self._wait_dialog.exec() # This will block until the validation is complete + # NOTE Use singleShot here to ensure that the signal is emitted after all other scheduled + # tasks in the event loop are processed. This avoids potential deadlocks. In particular, + # this is relevant for the _wait_dialog exec which opens a modal dialog during validation + # and therefore must not have the signal emitted immediately in the same event loop iteration. + # Otherwise, the callback may be scheduled before the dialog is shown resulting in a deadlock. + QtCore.QTimer.singleShot( + 0, lambda: ophyd_validation.change_device_configs([config], True, False) + ) + + # NOTE If dialog was already close, this means that a validation callback was already received + # which closed the dialog. In this case, we skip exec to avoid deadlock. With the singleShot above, + # this should not happen, but we keep the check for safety. + if self._wait_dialog is not None: + self._wait_dialog.exec() # This will block until the validation is complete config, config_status, connection_status, validation_msg = self._validation_result diff --git a/bec_widgets/applications/views/device_manager_view/device_manager_display_widget.py b/bec_widgets/applications/views/device_manager_view/device_manager_display_widget.py index d33abd29..34031264 100644 --- a/bec_widgets/applications/views/device_manager_view/device_manager_display_widget.py +++ b/bec_widgets/applications/views/device_manager_view/device_manager_display_widget.py @@ -104,10 +104,10 @@ class DeviceManagerDisplayWidget(DockAreaWidget): self.ophyd_test_view.multiple_validations_completed, (self.device_table_view.update_multiple_device_validations,), ), - (self.request_ophyd_validation, (self.ophyd_test_view.change_device_configs,)), + (self.request_ophyd_validation, (self.ophyd_test_view.device_table_config_changed,)), ( self.device_table_view.device_configs_changed, - (self.ophyd_test_view.change_device_configs,), + (self.ophyd_test_view.device_table_config_changed,), ), ( self.device_table_view.device_config_in_sync_with_redis, @@ -591,7 +591,8 @@ class DeviceManagerDisplayWidget(DockAreaWidget): ): if old_device_name and old_device_name != data.get("name", ""): self.device_table_view.remove_device(old_device_name) - self.device_table_view.update_device_configs([data]) + self.device_table_view.update_device_configs([data], skip_validation=True) + self.device_table_view.update_device_validation(data, config_status, connection_status, msg) @SafeSlot(dict, int, int, str, str) def _add_to_table_from_dialog( @@ -602,7 +603,8 @@ class DeviceManagerDisplayWidget(DockAreaWidget): msg: str, old_device_name: str = "", ): - self.device_table_view.add_device_configs([data]) + self.device_table_view.add_device_configs([data], skip_validation=True) + self.device_table_view.update_device_validation(data, config_status, connection_status, msg) @SafeSlot() def _remove_device_action(self): diff --git a/bec_widgets/widgets/control/device_manager/components/device_table/device_table.py b/bec_widgets/widgets/control/device_manager/components/device_table/device_table.py index 5e7b665c..c930b582 100644 --- a/bec_widgets/widgets/control/device_manager/components/device_table/device_table.py +++ b/bec_widgets/widgets/control/device_manager/components/device_table/device_table.py @@ -199,7 +199,8 @@ class DeviceTable(BECWidget, QtWidgets.QWidget): # Signal emitted if devices are added (updated) or removed # - device_configs: List of device configurations. # - added: True if devices were added/updated, False if removed. - device_configs_changed = QtCore.Signal(list, bool) + # - skip validation: True if validation should be skipped for added/updated devices. + device_configs_changed = QtCore.Signal(list, bool, bool) # Signal emitted when device selection changes, emits list of selected device configs selected_devices = QtCore.Signal(list) # Signal emitted when a device row is double-clicked, emits the device config @@ -823,7 +824,7 @@ class DeviceTable(BECWidget, QtWidgets.QWidget): # ------------------------------------------------------------------------- @SafeSlot(list) - def set_device_config(self, device_configs: _DeviceCfgIter): + def set_device_config(self, device_configs: _DeviceCfgIter, skip_validation: bool = False): """ Set the device config. This will clear any existing configs. @@ -837,27 +838,31 @@ class DeviceTable(BECWidget, QtWidgets.QWidget): for cfg in device_configs: self._add_row(cfg, ConfigStatus.UNKNOWN, ConnectionStatus.UNKNOWN) cfgs_added.append(cfg) - self.device_configs_changed.emit(cfgs_added, True) + self.device_configs_changed.emit(cfgs_added, True, skip_validation) in_sync_with_redis = self._is_config_in_sync_with_redis() self.device_config_in_sync_with_redis.emit(in_sync_with_redis) self.set_busy(False, text="") @SafeSlot() def clear_device_configs(self): - """Clear the device configs.""" + """Clear the device configs. Skips validation per default.""" self.set_busy(True, text="Clearing device configurations...") device_configs = self.get_device_config() with self.table_sort_on_hold: self._clear_table() - self.device_configs_changed.emit(device_configs, False) + self.device_configs_changed.emit( + device_configs, False, True + ) # Skip validation for removals in_sync_with_redis = self._is_config_in_sync_with_redis() self.device_config_in_sync_with_redis.emit(in_sync_with_redis) self.set_busy(False, text="") @SafeSlot(list) - def add_device_configs(self, device_configs: _DeviceCfgIter): + def add_device_configs(self, device_configs: _DeviceCfgIter, skip_validation: bool = False): """ - Add devices to the config. If a device already exists, it will be replaced. + Add devices to the config. If a device already exists, it will be replaced. If the validation is + skipped, the device will be added with UNKNOWN state to the table and has to be manually adjusted + by the user later on. Args: device_configs (Iterable[dict[str, Any]]): The device configs to add. @@ -875,20 +880,22 @@ class DeviceTable(BECWidget, QtWidgets.QWidget): # Remove existing rows first if len(already_in_table) > 0: self._remove_rows_by_name([cfg["name"] for cfg in already_in_table]) - self.device_configs_changed.emit(already_in_table, False) + self.device_configs_changed.emit( + already_in_table, False, True + ) # Skip validation for removals all_configs = already_in_table + not_in_table if len(all_configs) > 0: for cfg in already_in_table + not_in_table: self._add_row(cfg, ConfigStatus.UNKNOWN, ConnectionStatus.UNKNOWN) - self.device_configs_changed.emit(already_in_table + not_in_table, True) + self.device_configs_changed.emit(already_in_table + not_in_table, True, skip_validation) in_sync_with_redis = self._is_config_in_sync_with_redis() self.device_config_in_sync_with_redis.emit(in_sync_with_redis) self.set_busy(False, text="") @SafeSlot(list) - def update_device_configs(self, device_configs: _DeviceCfgIter): + def update_device_configs(self, device_configs: _DeviceCfgIter, skip_validation: bool = False): """ Update devices in the config. If a device does not exist, it will be added. @@ -907,7 +914,7 @@ class DeviceTable(BECWidget, QtWidgets.QWidget): row = self._update_row(cfg) if row is not None: cfgs_updated.append(cfg) - self.device_configs_changed.emit(cfgs_updated, True) + self.device_configs_changed.emit(cfgs_updated, True, skip_validation) in_sync_with_redis = self._is_config_in_sync_with_redis() self.device_config_in_sync_with_redis.emit(in_sync_with_redis) self.set_busy(False, text="") @@ -924,7 +931,9 @@ class DeviceTable(BECWidget, QtWidgets.QWidget): cfgs_to_be_removed = list(device_configs) with self.table_sort_on_hold: self._remove_rows_by_name([cfg["name"] for cfg in cfgs_to_be_removed]) - self.device_configs_changed.emit(cfgs_to_be_removed, False) # + self.device_configs_changed.emit( + cfgs_to_be_removed, False, True + ) # Skip validation for removals in_sync_with_redis = self._is_config_in_sync_with_redis() self.device_config_in_sync_with_redis.emit(in_sync_with_redis) self.set_busy(False, text="") @@ -946,7 +955,7 @@ class DeviceTable(BECWidget, QtWidgets.QWidget): with self.table_sort_on_hold: self._remove_rows_by_name([row_data.data["name"]]) cfgs = [{"name": device_name, **row_data.data}] - self.device_configs_changed.emit(cfgs, False) + self.device_configs_changed.emit(cfgs, False, True) # Skip validation for removals in_sync_with_redis = self._is_config_in_sync_with_redis() self.device_config_in_sync_with_redis.emit(in_sync_with_redis) self.set_busy(False, text="") diff --git a/bec_widgets/widgets/control/device_manager/components/ophyd_validation/ophyd_validation.py b/bec_widgets/widgets/control/device_manager/components/ophyd_validation/ophyd_validation.py index 9838720d..eb2acfd9 100644 --- a/bec_widgets/widgets/control/device_manager/components/ophyd_validation/ophyd_validation.py +++ b/bec_widgets/widgets/control/device_manager/components/ophyd_validation/ophyd_validation.py @@ -470,9 +470,19 @@ class OphydValidation(BECWidget, QtWidgets.QWidget): widgets: list[ValidationListItem] = self.list_widget.get_widgets() return [widget.device_model.device_config for widget in widgets] + @SafeSlot(list, bool, bool) + def device_table_config_changed( + self, device_configs: list[dict[str, Any]], added: bool, skip_validation: bool + ) -> None: + """Slot to handle device config changes in the device table.""" + self.change_device_configs( + device_configs=device_configs, added=added, skip_validation=skip_validation + ) + @SafeSlot(list, bool) @SafeSlot(list, bool, bool) @SafeSlot(list, bool, bool, bool, float) + @SafeSlot(list, bool, bool, bool, float, bool) def change_device_configs( self, device_configs: list[dict[str, Any]], @@ -480,11 +490,17 @@ class OphydValidation(BECWidget, QtWidgets.QWidget): connect: bool = False, force_connect: bool = False, timeout: float = 5.0, + skip_validation: bool = False, ) -> None: """ Change the device configuration to test. If added is False, existing devices are removed. Device tests will be removed based on device names. No duplicates are allowed. + For validation runs, results are emitted via the validation_completed signal. Unless devices + are already in the running session with the same config, in which case the combined results + of all such devices are emitted via the multiple_validations_completed signal. NOTE Please make + sure to connect to both signals if you want to capture all results. + Args: device_configs (list[dict[str, Any]]): List of device configurations. added (bool): Whether the devices are added to the existing list. @@ -504,7 +520,7 @@ class OphydValidation(BECWidget, QtWidgets.QWidget): if device_name is None: # Config missing name, will be skipped.. logger.error(f"Device config missing 'name': {cfg}. Config will be skipped.") continue - if not added: # Remove requested + if not added or skip_validation is True: # Remove requested self._remove_device_config(cfg) continue if self._is_device_in_redis_session(cfg.get("name"), cfg): @@ -533,7 +549,14 @@ class OphydValidation(BECWidget, QtWidgets.QWidget): ) # Send out batch of updates for devices already in session if devices_already_in_session: - self.multiple_validations_completed.emit(devices_already_in_session) + # NOTE: Use singleShot here to ensure that the signal is emitted after all other scheduled + # tasks in the event loop are processed. This avoids potential deadlocks. In particular, + # this is relevant for the DeviceFormDialog which opens a modal dialog during validation + # and therefore must not have the signal emitted immediately in the same event loop iteration. + # Otherwise, the dialog would block signal processing. + QtCore.QTimer.singleShot( + 0, lambda: self.multiple_validations_completed.emit(devices_already_in_session) + ) def cancel_validation(self, device_name: str) -> None: """Cancel a running validation for a specific device.