From 46fe7baa4da9dd389d52c07a008c4d535c6974dd Mon Sep 17 00:00:00 2001 From: menzel Date: Tue, 1 Sep 2026 15:11:47 +0200 Subject: [PATCH 1/2] fix(falcon): set the layout the IOC can actually resolve, and verify it _initialize_detector_backend put a bare "layout.xml" into the HDF5 plugin. The IOC resolves that relative to its own working directory, where no such file exists: the cSAXS SITORO IOC ships the layout as cfg/layout.xml (installed at /ioc/X12SA-CPCL-FALCONX1/cfg/layout.xml), and that is what the IOC configures at startup. BEC was overwriting a correct value with a broken one on every device init. The failure was silent and badly signposted. An unreadable layout does not raise; the plugin simply refuses to open the output file later, reporting "Error opening file ..., status=-1" with the actual cause buried in XMLErrorMsg_RBV. Because a caget taken before a BEC device reload showed the IOC's own valid value, the two readings disagreed and the layout looked innocent. The default is now cfg/layout.xml, overridable per deployment with an xml_file_name key in deviceConfig ("" selects the plugin's built-in layout), and on_connected reads XMLValid_RBV back and logs an error naming the file and the IOC's message if the layout was rejected. The existing on_connected test asserted the broken value; it now asserts the default constant. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01KLnmUurqcNd1FiDY5M2uZr --- csaxs_bec/devices/epics/falcon_csaxs.py | 56 +++++++++++++++++++++- tests/tests_devices/test_falcon_csaxs.py | 59 +++++++++++++++++++++++- 2 files changed, 112 insertions(+), 3 deletions(-) diff --git a/csaxs_bec/devices/epics/falcon_csaxs.py b/csaxs_bec/devices/epics/falcon_csaxs.py index 23a27cf3..521aad58 100644 --- a/csaxs_bec/devices/epics/falcon_csaxs.py +++ b/csaxs_bec/devices/epics/falcon_csaxs.py @@ -19,6 +19,13 @@ from csaxs_bec.devices.utils.utils import fetch_scan_info logger = bec_logger.logger +# NOTE Layout of the written HDF5 file, resolved by the IOC relative to its own working +# directory. The cSAXS SITORO IOC ships it as cfg/layout.xml (installed at +# /ioc/X12SA-CPCL-FALCONX1/cfg/layout.xml). Override per deployment with the +# xml_file_name key in deviceConfig, or set it to "" to fall back to the plugin's +# built-in layout. +DEFAULT_XML_FILE_NAME = "cfg/layout.xml" + class FalconError(Exception): """Base class for exceptions in this module.""" @@ -75,6 +82,17 @@ class FalconcSAXS(PSIDeviceBase, FalconControl): file_event = Cpt(FileEventSignal, name="file_event") + def __init__(self, *args, xml_file_name: str | None = None, **kwargs): + """ + Args: + xml_file_name: HDF5 layout description, as the IOC resolves it. Defaults to + DEFAULT_XML_FILE_NAME; pass "" to use the plugin's built-in layout. + """ + self._xml_file_name_config = ( + DEFAULT_XML_FILE_NAME if xml_file_name is None else xml_file_name + ) + super().__init__(*args, **kwargs) + def on_init(self) -> None: """Initialize Falcon Sitoro detector""" self._lock = threading.RLock() @@ -82,6 +100,7 @@ class FalconcSAXS(PSIDeviceBase, FalconControl): self._value_pixel_per_buffer = 20 self._queue_size = 2000 self._full_path = "" + self._xml_file_name = self._xml_file_name_config self.scan_parameters: ScanServerScanInfo | None = None def on_connected(self): @@ -240,8 +259,13 @@ class FalconcSAXS(PSIDeviceBase, FalconControl): # Enable HDF5 plugin self.hdf5.enable.put(1) - # Use layout.xml file for cSAXS Falcon. FIXME:Should be checked if IOC runs on different host. - self.hdf5.xml_file_name.put("layout.xml") + # Layout of the written file. The IOC resolves this relative to its own working + # directory, so a bare "layout.xml" does not resolve -- the cSAXS SITORO IOC keeps + # it under cfg/. An unreadable layout does not fail here: the plugin refuses to + # open the output file later, reporting only "Error opening file ... status=-1", + # so the value is verified against XMLValid_RBV right away. + self.hdf5.xml_file_name.put(self._xml_file_name) + self._check_xml_layout() # TODO Check if lazy open is needed and wanted! self.hdf5.lazy_open.put(1) @@ -255,6 +279,34 @@ class FalconcSAXS(PSIDeviceBase, FalconControl): # Set nd_array mode to 1: This means segmentation into Spectra within EPICS, 1 is activate, 0 is deactivate self.nd_array_mode.put(1) + def _check_xml_layout(self, timeout: float = 1.0) -> bool: + """Warn if the IOC could not read the configured HDF5 layout description. + + Args: + timeout (float): Time in seconds to allow the IOC to parse the file. + + Returns: + bool: True if the IOC reports the layout as valid. + """ + if not self._xml_file_name: + return True # built-in layout requested; nothing to validate + deadline = time.time() + timeout + valid = "" + while time.time() < deadline: + valid = str(self.hdf5.xml_valid.get()) + if valid.lower() in ("yes", "1"): + return True + time.sleep(0.05) + logger.error( + f"{self.name}: the IOC cannot use the HDF5 layout " + f"{self._xml_file_name!r} (XMLValid={valid!r}, " + f"error={self.hdf5.xml_error_msg.get()!r}). Writing will fail with an " + "'Error opening file' until this is corrected. The path is resolved by the " + 'IOC, not by BEC; set xml_file_name in deviceConfig, or "" for the ' + "plugin's built-in layout." + ) + return False + def on_stage(self): """ This method is called when the detector is staged for acquisition. diff --git a/tests/tests_devices/test_falcon_csaxs.py b/tests/tests_devices/test_falcon_csaxs.py index c7ca8950..97ce39b0 100644 --- a/tests/tests_devices/test_falcon_csaxs.py +++ b/tests/tests_devices/test_falcon_csaxs.py @@ -15,6 +15,7 @@ from ophyd_devices.tests.utils import patched_device from csaxs_bec.devices.epics.falcon_csaxs import ( ACQUIRESTATUS, + DEFAULT_XML_FILE_NAME, FalconcSAXS, FalconError, MappingSource, @@ -84,7 +85,7 @@ def test_falcon_on_connected(mock_det: FalconcSAXS): # Backend default PV values assert falcon.hdf5.enable.get() == "1" # Enabled - assert falcon.hdf5.xml_file_name.get() == "layout.xml" + assert falcon.hdf5.xml_file_name.get() == DEFAULT_XML_FILE_NAME assert falcon.hdf5.lazy_open.get() == "1" # Enabled assert falcon.hdf5.temp_suffix.get() == "" assert falcon.hdf5.queue_size.get() == falcon._queue_size @@ -303,3 +304,59 @@ def test_falcon_prime_restores_config_on_failure(mock_det: FalconcSAXS): assert falcon.pixel_advance_mode.get() == TriggerSource.GATE assert falcon.ignore_gate.get() == 0 assert falcon.pixels_per_buffer.get() == 20 + + +def test_falcon_sets_the_ioc_resolvable_layout(mock_det: FalconcSAXS): + """A bare 'layout.xml' does not resolve for the IOC; cfg/layout.xml does.""" + falcon = mock_det + falcon.hdf5.xml_file_name.put("something-else") + + with mock.patch.object(falcon, "_check_xml_layout", return_value=True): + falcon._initialize_detector_backend() + + assert falcon.hdf5.xml_file_name.get() == DEFAULT_XML_FILE_NAME + assert DEFAULT_XML_FILE_NAME == "cfg/layout.xml" + + +def test_falcon_xml_file_name_from_device_config(): + """deviceConfig can point at a different layout, or disable it with ''.""" + with patched_device( + FalconcSAXS, + name="falcon", + prefix="X12SA-SITORO:", + device_manager=DMMock(), + xml_file_name="", + _mock_pv_initial_value=1, + ) as dev: + try: + assert dev._xml_file_name == "" + # Nothing to validate when the built-in layout is requested. + assert dev._check_xml_layout(timeout=0.01) is True + finally: + dev.destroy() + + +def test_falcon_warns_when_the_ioc_rejects_the_layout(mock_det: FalconcSAXS): + """An unreadable layout is reported at connect, not as a write error much later.""" + falcon = mock_det + + with ( + mock.patch.object(falcon.hdf5.xml_valid, "get", return_value="No"), + mock.patch.object(falcon.hdf5.xml_error_msg, "get", return_value="cannot be opened"), + mock.patch("csaxs_bec.devices.epics.falcon_csaxs.logger") as mock_logger, + ): + assert falcon._check_xml_layout(timeout=0.05) is False + assert mock_logger.error.call_count == 1 + assert "layout" in mock_logger.error.call_args[0][0].lower() + + +def test_falcon_accepts_a_valid_layout_silently(mock_det: FalconcSAXS): + """A layout the IOC accepts produces no error.""" + falcon = mock_det + + with ( + mock.patch.object(falcon.hdf5.xml_valid, "get", return_value="Yes"), + mock.patch("csaxs_bec.devices.epics.falcon_csaxs.logger") as mock_logger, + ): + assert falcon._check_xml_layout(timeout=0.05) is True + mock_logger.error.assert_not_called() -- 2.54.0 From b20afd15eafa69229bf94b02d4c73ea43c5579e1 Mon Sep 17 00:00:00 2001 From: menzel Date: Tue, 1 Sep 2026 21:49:21 +0200 Subject: [PATCH 2/2] fix(falcon): name prefix in __init__ so the device server stops dropping it The __init__ added in the previous commit took (*args, **kwargs). The device server builds a device's init kwargs by intersecting the deviceConfig keys with the NAMED parameters of the class signature (bec_server/device_server/devices/devicemanager.py:469-475), so 'prefix' was silently discarded and the Falcon was constructed with an empty prefix. Every signal then pointed at a bare suffix -- HDF1:FilePath_RBV instead of X12SA-SITORO:HDF1:FilePath_RBV -- and instantiation failed with "TimeoutError: Failed to connect to all signals" listing several hundred PVs. That reads like an unreachable IOC, which is how it was diagnosed at the beamline for two hours, while caget from the same host worked perfectly. The signature now names name, prefix, scan_info, device_manager and xml_file_name explicitly, matching DDG1. Two tests guard it: one asserts every deviceConfig key is a named parameter, the other that a configured prefix reaches the signal PV names. Reported-by: Klaus Wakonig Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01KLnmUurqcNd1FiDY5M2uZr --- csaxs_bec/devices/epics/falcon_csaxs.py | 27 +++++++++++++++++++-- tests/tests_devices/test_falcon_csaxs.py | 30 ++++++++++++++++++++++++ 2 files changed, 55 insertions(+), 2 deletions(-) diff --git a/csaxs_bec/devices/epics/falcon_csaxs.py b/csaxs_bec/devices/epics/falcon_csaxs.py index 521aad58..d818cf10 100644 --- a/csaxs_bec/devices/epics/falcon_csaxs.py +++ b/csaxs_bec/devices/epics/falcon_csaxs.py @@ -4,6 +4,7 @@ import enum import os import threading import time +from typing import TYPE_CHECKING from typing import Literal from bec_lib.file_utils import get_full_path @@ -17,6 +18,9 @@ from ophyd_devices.interfaces.base_classes.psi_device_base import PSIDeviceBase from csaxs_bec.devices.utils.utils import fetch_scan_info +if TYPE_CHECKING: # pragma: no cover + from bec_lib.devicemanager import DeviceManagerBase, ScanInfo + logger = bec_logger.logger # NOTE Layout of the written HDF5 file, resolved by the IOC relative to its own working @@ -82,16 +86,35 @@ class FalconcSAXS(PSIDeviceBase, FalconControl): file_event = Cpt(FileEventSignal, name="file_event") - def __init__(self, *args, xml_file_name: str | None = None, **kwargs): + def __init__( + self, + name: str, + prefix: str = "", + scan_info: "ScanInfo | None" = None, + device_manager: "DeviceManagerBase | None" = None, + xml_file_name: str | None = None, + **kwargs, + ): """ + Every deviceConfig key must appear here by name. The device server builds the + init kwargs by intersecting the config keys with the class signature's named + parameters, so a bare *args/**kwargs signature silently drops 'prefix' and the + device is built with empty PV names. + Args: + name: device name. + prefix: EPICS prefix, e.g. 'X12SA-SITORO:'. + scan_info: scan info object. + device_manager: BEC device manager. xml_file_name: HDF5 layout description, as the IOC resolves it. Defaults to DEFAULT_XML_FILE_NAME; pass "" to use the plugin's built-in layout. """ self._xml_file_name_config = ( DEFAULT_XML_FILE_NAME if xml_file_name is None else xml_file_name ) - super().__init__(*args, **kwargs) + super().__init__( + name=name, prefix=prefix, scan_info=scan_info, device_manager=device_manager, **kwargs + ) def on_init(self) -> None: """Initialize Falcon Sitoro detector""" diff --git a/tests/tests_devices/test_falcon_csaxs.py b/tests/tests_devices/test_falcon_csaxs.py index 97ce39b0..156544f9 100644 --- a/tests/tests_devices/test_falcon_csaxs.py +++ b/tests/tests_devices/test_falcon_csaxs.py @@ -1,4 +1,5 @@ # pylint: skip-file +import inspect import os import threading from typing import Generator @@ -360,3 +361,32 @@ def test_falcon_accepts_a_valid_layout_silently(mock_det: FalconcSAXS): ): assert falcon._check_xml_layout(timeout=0.05) is True mock_logger.error.assert_not_called() + + +def test_falcon_init_names_every_device_config_key(): + """The device server drops config keys that are not named parameters. + + bec_server's devicemanager builds the init kwargs by intersecting the deviceConfig + keys with the class signature's named parameters. A *args/**kwargs signature + therefore silently discards 'prefix', and every PV is built without it -- which + presents as 'failed to connect to all signals' rather than as a config error. + """ + params = inspect.signature(FalconcSAXS).parameters + for key in ("name", "prefix", "scan_info", "device_manager", "xml_file_name"): + assert key in params, f"{key} must be a named parameter or the device server drops it" + + +def test_falcon_keeps_the_configured_prefix(): + """A prefix passed as the device server passes it must reach the signals.""" + with patched_device( + FalconcSAXS, + name="falcon", + prefix="X12SA-SITORO:", + device_manager=DMMock(), + _mock_pv_initial_value=1, + ) as dev: + try: + assert dev.prefix == "X12SA-SITORO:" + assert dev.hdf5.file_path.pvname.startswith("X12SA-SITORO:") + finally: + dev.destroy() -- 2.54.0