From f450f29e39012f2eb30ca02fceb1c24811894f54 Mon Sep 17 00:00:00 2001 From: menzel Date: Wed, 2 Sep 2026 17:42:38 +0200 Subject: [PATCH] fix(eiger): let each model keep its own readout time, and make it configurable Readout is a property of the detector, not a beamline constant: a 9M has more modules than a 1.5M, and the Falcon needs 3 ms against the delay generator's 200 us. The per-model constants therefore stay, with comments saying the duplication is deliberate so nobody consolidates them again. They all hold 2e-4 today only because no measured per-model value exists yet. Writing a test for the deviceConfig override exposed that it never worked. Both subclasses passed readout_time to super() while also forwarding **kwargs, so supplying it raised "got multiple values for keyword argument" -- and through BEC it never even got that far, because readout_time was not a named parameter of the subclass signature and the device server drops config keys it cannot see (the same rule behind the recent prefix incident). Both subclasses now name it with the model constant as default, and a test asserts the signatures keep it. Also documents frame_time_us in DetectorSettings as required-but-ignored for the Eiger, and warns that its 500 is microseconds while every other time in the module is seconds. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01KLnmUurqcNd1FiDY5M2uZr --- csaxs_bec/devices/jungfraujoch/eiger.py | 9 +++++ csaxs_bec/devices/jungfraujoch/eiger_1_5m.py | 11 +++--- csaxs_bec/devices/jungfraujoch/eiger_9m.py | 11 +++--- tests/tests_devices/test_eiger.py | 38 ++++++++++++++++++-- 4 files changed, 59 insertions(+), 10 deletions(-) diff --git a/csaxs_bec/devices/jungfraujoch/eiger.py b/csaxs_bec/devices/jungfraujoch/eiger.py index 27b4a050..60696b4e 100644 --- a/csaxs_bec/devices/jungfraujoch/eiger.py +++ b/csaxs_bec/devices/jungfraujoch/eiger.py @@ -78,6 +78,11 @@ logger = bec_logger.logger # the trigger period allows. MIN_EXP_TIME = 500e-6 # s EIGER_READOUT_TIME = 2e-4 # s -- matches DDG2 DEFAULT_READOUT_TIMES["ab"] +# NOTE Each model declares its own value and passes it here; that duplication is +# deliberate, since readout is a property of the detector (a 9M has more modules than a +# 1.5M, and the Falcon needs 3 ms). They all happen to be 2e-4 today only because no +# measured per-model number exists yet. This constant is the fallback for a model that +# does not state one; deviceConfig's readout_time overrides it per deployment. class EigerError(Exception): @@ -303,6 +308,10 @@ class Eiger(PSIDeviceBase): # self.jfj_client.connect_and_initialise(timeout=10) # Setup Detector settings, here we may also set the energy already as this might be time consuming + # NOTE frame_time_us is required by the API but ignored for the Eiger ("For EIGER + # detector this is default frame time, not used otherwise"); the exposure comes + # from image_time_us in DatasetSettings. The 500 is a placeholder -- it is NOT the + # readout time, and it is in microseconds, unlike every other time in this module. settings = DetectorSettings(frame_time_us=int(500), timing=DetectorTiming.TRIGGER) self.jfj_client.set_detector_settings(settings, timeout=5) diff --git a/csaxs_bec/devices/jungfraujoch/eiger_1_5m.py b/csaxs_bec/devices/jungfraujoch/eiger_1_5m.py index bfdb3646..92608502 100644 --- a/csaxs_bec/devices/jungfraujoch/eiger_1_5m.py +++ b/csaxs_bec/devices/jungfraujoch/eiger_1_5m.py @@ -11,9 +11,11 @@ from typing import TYPE_CHECKING from csaxs_bec.devices.jungfraujoch.eiger import Eiger -# Gap between the end of an acquisition and the next trigger; must match the -# delay generator's gap (DDG2 DEFAULT_READOUT_TIMES["ab"]). -EIGER1_5M_READOUT_TIME = 2e-4 # s +# Gap this model needs between the end of an acquisition and the next trigger. Kept +# per model on purpose -- readout is a detector property, not a beamline constant. The +# delay generator's gap (DDG2 DEFAULT_READOUT_TIMES["ab"]) must be at least as large as +# the largest value among the detectors in a scan, or the slowest one overruns. +EIGER1_5M_READOUT_TIME = 2e-4 # s -- placeholder until a measured value exists DETECTOR_NAME = "EIGER 1.5M" @@ -42,12 +44,13 @@ class Eiger1_5M(Eiger): beam_center: tuple[float, float] = (0.0, 0.0), scan_info: ScanInfo = None, device_manager: DeviceManagerDS = None, + readout_time: float = EIGER1_5M_READOUT_TIME, **kwargs, ) -> None: super().__init__( name=name, detector_name=DETECTOR_NAME, - readout_time=EIGER1_5M_READOUT_TIME, + readout_time=readout_time, detector_distance=detector_distance, beam_center=beam_center, scan_info=scan_info, diff --git a/csaxs_bec/devices/jungfraujoch/eiger_9m.py b/csaxs_bec/devices/jungfraujoch/eiger_9m.py index bb305dd5..13f1a4c6 100644 --- a/csaxs_bec/devices/jungfraujoch/eiger_9m.py +++ b/csaxs_bec/devices/jungfraujoch/eiger_9m.py @@ -20,9 +20,11 @@ if TYPE_CHECKING: # pragma no cover from bec_lib.devicemanager import ScanInfo from bec_server.device_server.device_server import DeviceManagerDS -# Gap between the end of an acquisition and the next trigger; must match the -# delay generator's gap (DDG2 DEFAULT_READOUT_TIMES["ab"]). -EIGER9M_READOUT_TIME = 2e-4 # s +# Gap this model needs between the end of an acquisition and the next trigger. Kept +# per model on purpose -- readout is a detector property, not a beamline constant. The +# delay generator's gap (DDG2 DEFAULT_READOUT_TIMES["ab"]) must be at least as large as +# the largest value among the detectors in a scan, or the slowest one overruns. +EIGER9M_READOUT_TIME = 2e-4 # s -- placeholder until a measured value exists DETECTOR_NAME = "EIGER 9M" # "EIGER 9M"" @@ -46,12 +48,13 @@ class Eiger9M(Eiger): beam_center: tuple[float, float] = (0.0, 0.0), scan_info: ScanInfo = None, device_manager: DeviceManagerDS = None, + readout_time: float = EIGER9M_READOUT_TIME, **kwargs, ) -> None: super().__init__( name=name, detector_name=DETECTOR_NAME, - readout_time=EIGER9M_READOUT_TIME, + readout_time=readout_time, detector_distance=detector_distance, beam_center=beam_center, scan_info=scan_info, diff --git a/tests/tests_devices/test_eiger.py b/tests/tests_devices/test_eiger.py index c07920cd..20dcf483 100644 --- a/tests/tests_devices/test_eiger.py +++ b/tests/tests_devices/test_eiger.py @@ -1,4 +1,5 @@ # pylint: skip-file +import inspect import os import threading import time @@ -23,8 +24,8 @@ from ophyd_devices.utils.psi_device_base_utils import DeviceStatus from csaxs_bec.devices.jungfraujoch.eiger import EigerError from csaxs_bec.devices.jungfraujoch.eiger import EIGER_READOUT_TIME, MIN_EXP_TIME -from csaxs_bec.devices.jungfraujoch.eiger_1_5m import Eiger1_5M -from csaxs_bec.devices.jungfraujoch.eiger_9m import Eiger9M +from csaxs_bec.devices.jungfraujoch.eiger_1_5m import EIGER1_5M_READOUT_TIME, Eiger1_5M +from csaxs_bec.devices.jungfraujoch.eiger_9m import EIGER9M_READOUT_TIME, Eiger9M if TYPE_CHECKING: # pragma no cover from bec_lib.messages import FileMessage @@ -537,3 +538,36 @@ def test_eiger_minimum_exposure_check_is_unchanged(eiger_1_5m): ): with pytest.raises(ValueError): eiger.stage() + + +def test_eiger_models_may_declare_their_own_readout_time(eiger_1_5m, eiger_9m): + """Readout is a detector property, so each model states its own value. + + They coincide today, but nothing should force them to: a 9M has more modules + than a 1.5M, and the Falcon needs 3 ms. + """ + assert eiger_1_5m._readout_time == EIGER1_5M_READOUT_TIME + assert eiger_9m._readout_time == EIGER9M_READOUT_TIME + + +def test_eiger_readout_time_is_overridable_per_deployment(mock_scan_info): + """deviceConfig's readout_time wins over the model's default.""" + dev = Eiger1_5M( + name="eiger_1_5m", beam_center=(256, 256), detector_distance=100.0, readout_time=1.5e-3 + ) + try: + assert dev._readout_time == 1.5e-3 + finally: + dev.destroy() + + +def test_eiger_subclasses_name_readout_time_for_the_device_server(): + """deviceConfig keys reach a device only if the class signature names them. + + bec_server intersects the config keys with the named parameters of the class + (device_server/devices/devicemanager.py), so a subclass that only forwards + **kwargs silently drops readout_time -- and passing it explicitly used to raise + 'got multiple values for keyword argument'. + """ + for cls in (Eiger1_5M, Eiger9M): + assert "readout_time" in inspect.signature(cls).parameters