From bb39833d83f2951e12facaa3f13e170c28084cc9 Mon Sep 17 00:00:00 2001 From: menzel Date: Tue, 1 Sep 2026 14:54:00 +0200 Subject: [PATCH 1/7] feat(ddg2): make the detector-trigger readout time configurable The gap between consecutive detector triggers is exp_time minus a readout time that was a module constant fixed at 0.2 ms. Some detectors need more: FalconcSAXS declares MIN_READOUT = 3 ms, fifteen times longer. Nothing reconciled the two -- DDG2 does not know which detectors are in the scan, and the falcon only validates its exposure time, never the gap -- so a detector that cannot keep up silently dropped frames. There was also no way to change it. on_stage recomputes the pulse width from the constant on every scan, so a value set by hand from the client did not survive to the next acquisition. The readout times are now per-instance, settable two ways: a readout_times key in deviceConfig for a per-deployment default, and set_readout_times() via USER_ACCESS for a change between scans. Raising the value widens the gap and shortens the exposure by the same amount; burst_period stays at exp_time, so the frame rate is unaffected. Channel pair 'ab' is the one multiplexed to the detectors and normally the only one worth changing. A value that would exceed the exposure time is rejected, and a non-default value is logged at on_stage so a scan taken with a widened gap is recoverable from the logs. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01KLnmUurqcNd1FiDY5M2uZr --- .../epics/delay_generator_csaxs/ddg_2.py | 93 ++++++++++++++++++- .../test_delay_generator_csaxs.py | 62 +++++++++++++ 2 files changed, 151 insertions(+), 4 deletions(-) diff --git a/csaxs_bec/devices/epics/delay_generator_csaxs/ddg_2.py b/csaxs_bec/devices/epics/delay_generator_csaxs/ddg_2.py index 0141d5dc..5ee38fe5 100644 --- a/csaxs_bec/devices/epics/delay_generator_csaxs/ddg_2.py +++ b/csaxs_bec/devices/epics/delay_generator_csaxs/ddg_2.py @@ -22,7 +22,10 @@ Burst mode is enabled: - Burst period is set to the exposure time. """ +from __future__ import annotations + import time +from typing import TYPE_CHECKING from bec_lib.logger import bec_logger from bec_server.scan_server.scans.scan_base import ScanInfo as ScanServerScanInfo @@ -42,6 +45,9 @@ from csaxs_bec.devices.epics.delay_generator_csaxs.delay_generator_csaxs import ) 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 ######################## @@ -113,6 +119,77 @@ class DDG2(PSIDeviceBase, DelayGeneratorCSAXS): distributes the trigger to the detectors. The DDG2 is triggered by the DDG1 through the EXT/EN channel. """ + # Methods exposed to the BEC client, i.e. callable as dev.ddg2.() + USER_ACCESS = ["get_readout_times", "set_readout_times"] + + def __init__( + self, + name: str, + prefix: str = "", + scan_info: ScanInfo | None = None, + device_manager: DeviceManagerBase | None = None, + readout_times: dict[str, float] | None = None, + **kwargs, + ): + """ + Args: + readout_times: per-channel-pair readout times in seconds, merged over + DEFAULT_READOUT_TIMES. Only the pairs given are overridden, e.g. + ``{"ab": 3.0e-3}``. See set_readout_times() for what this controls. + """ + super().__init__( + name=name, prefix=prefix, scan_info=scan_info, device_manager=device_manager, **kwargs + ) + self._readout_times: dict[str, float] = dict(DEFAULT_READOUT_TIMES) + if readout_times: + self.set_readout_times(**readout_times) + + def get_readout_times(self) -> dict[str, float]: + """Return the readout times per channel pair, in seconds. + + Returns: + dict[str, float]: a copy of the effective readout times. + """ + return dict(self._readout_times) + + def set_readout_times(self, **channels: float) -> dict[str, float]: + """Set the readout time of one or more channel pairs, in seconds. + + The readout time is subtracted from the exposure time to obtain the pulse + width, while the burst period stays at the exposure time. Raising it therefore + widens the gap between consecutive detector triggers and shortens the exposure + by the same amount; the frame rate is unaffected. + + This matters for detectors that need a minimum dead time between frames. The + default of 0.2 ms is shorter than some detectors can handle -- FalconcSAXS, for + instance, declares MIN_READOUT = 3 ms -- and a value that is too short shows up + as silently dropped frames rather than as an error. + + Channel pair 'ab' is the one multiplexed to the detectors, so it is normally the + only one worth changing. The new value applies from the next on_stage, i.e. the + next scan; it does not alter a scan already staged. + + Args: + **channels: channel pair to readout time in seconds, e.g. ``ab=3.0e-3``. + + Returns: + dict[str, float]: the effective readout times after the update. + + Raises: + ValueError: for an unknown channel pair or a non-positive value. + """ + for channel, value in channels.items(): + if channel not in self._readout_times: + raise ValueError( + f"Unknown channel pair {channel!r}; expected one of " + f"{sorted(self._readout_times)}" + ) + if value <= 0: + raise ValueError(f"readout time for {channel!r} must be > 0, got {value}") + self._readout_times[channel] = float(value) + logger.info(f"DDG {self.name} readout times set to {self._readout_times}") + return self.get_readout_times() + def on_init(self) -> None: """Initialize the device""" self.scan_parameters: ScanServerScanInfo | None = None @@ -193,9 +270,10 @@ class DDG2(PSIDeviceBase, DelayGeneratorCSAXS): # NOTE Check if the exposure time is longer than all readout times. # Raise a ValueError if requested exposure time is too short. - if any(exp_time <= rt for rt in DEFAULT_READOUT_TIMES.values()): + if any(exp_time <= rt for rt in self._readout_times.values()): raise ValueError( - f"Exposure time {exp_time} is too short for the readout times {DEFAULT_READOUT_TIMES}" + f"Exposure time {exp_time} is too short for the readout times " + f"{self._readout_times}" ) ######################################### @@ -208,8 +286,15 @@ class DDG2(PSIDeviceBase, DelayGeneratorCSAXS): if self.burst_period.get() != exp_time: self.burst_period.put(exp_time) - # Calculate the pulse width for the channel pair 'ab' - burst_pulse_width = exp_time - DEFAULT_READOUT_TIMES["ab"] + # Calculate the pulse width for the channel pair 'ab'. The readout time is the + # gap left between consecutive triggers; see set_readout_times(). + burst_pulse_width = exp_time - self._readout_times["ab"] + if self._readout_times["ab"] != DEFAULT_READOUT_TIMES["ab"]: + logger.info( + f"DDG {self.name} using non-default readout time " + f"{self._readout_times['ab']}s for 'ab' (default " + f"{DEFAULT_READOUT_TIMES['ab']}s); pulse width {burst_pulse_width}s" + ) # Trigger detectors with delay 0, and pulse width = exp_time - readout_time self.set_delay_pairs(channel="ab", delay=0, width=burst_pulse_width) diff --git a/tests/tests_devices/test_delay_generator_csaxs.py b/tests/tests_devices/test_delay_generator_csaxs.py index a484dae3..19810450 100644 --- a/tests/tests_devices/test_delay_generator_csaxs.py +++ b/tests/tests_devices/test_delay_generator_csaxs.py @@ -613,3 +613,65 @@ def test_ddg2_on_stop(mock_ddg2: DDG2): # mock_ddg2.burst_mode.put(1) # Enable burst mode # mock_ddg2.stop() # assert mock_ddg2.burst_mode.get() == 0 # Burst mode is disabled + + +def test_ddg2_readout_times_default(mock_ddg2: DDG2): + """Without an override the module defaults are in force.""" + assert mock_ddg2.get_readout_times() == DDG2_DEFAULT_READOUT_TIMES + # A copy, so a caller cannot mutate the device's state by accident. + mock_ddg2.get_readout_times()["ab"] = 1.0 + assert mock_ddg2.get_readout_times()["ab"] == DDG2_DEFAULT_READOUT_TIMES["ab"] + + +def test_ddg2_set_readout_times_validates(mock_ddg2: DDG2): + """Unknown pairs and non-positive values are rejected before they reach a scan.""" + with pytest.raises(ValueError): + mock_ddg2.set_readout_times(xy=1e-3) + with pytest.raises(ValueError): + mock_ddg2.set_readout_times(ab=0) + with pytest.raises(ValueError): + mock_ddg2.set_readout_times(ab=-1e-3) + assert mock_ddg2.get_readout_times() == DDG2_DEFAULT_READOUT_TIMES + + +def test_ddg2_set_readout_times_widens_the_gap(mock_ddg2: DDG2): + """A raised readout time shortens the pulse; the burst period is unchanged.""" + ddg = mock_ddg2 + exp_time = 0.1 + frames_per_trigger = 10 + ddg.on_connected() + ddg.scan_info.msg.info["exp_time"] = exp_time + ddg.scan_info.msg.info["frames_per_trigger"] = frames_per_trigger + + ddg.set_readout_times(ab=3e-3) + assert ddg.get_readout_times()["ab"] == 3e-3 + # Other pairs are untouched. + assert ddg.get_readout_times()["cd"] == DDG2_DEFAULT_READOUT_TIMES["cd"] + + ddg.stage() + assert np.isclose(ddg.ab.width.get(), exp_time - 3e-3) + assert ddg.burst_period.get() == exp_time # frame rate unaffected + ddg.unstage() + + +def test_ddg2_readout_times_from_device_config(): + """deviceConfig can set it per deployment, merged over the defaults.""" + with patched_device( + DDG2, name="ddg2", prefix="test_ddg2:", readout_times={"ab": 3e-3}, _mock_pv_initial_value=0 + ) as ddg: + try: + assert ddg.get_readout_times()["ab"] == 3e-3 + assert ddg.get_readout_times()["gh"] == DDG2_DEFAULT_READOUT_TIMES["gh"] + finally: + ddg.destroy() + + +def test_ddg2_too_long_readout_time_is_caught_at_stage(mock_ddg2: DDG2): + """A readout time exceeding the exposure must fail loudly, not emit a negative pulse.""" + ddg = mock_ddg2 + ddg.on_connected() + ddg.scan_info.msg.info["exp_time"] = 1e-3 + ddg.scan_info.msg.info["frames_per_trigger"] = 1 + ddg.set_readout_times(ab=3e-3) + with pytest.raises(ValueError): + ddg.stage() From 77fc2525a007f14f3fe108d95945166c512f05e5 Mon Sep 17 00:00:00 2001 From: menzel Date: Wed, 2 Sep 2026 17:02:17 +0200 Subject: [PATCH 2/7] feat(eiger,ddg2): acquire over the gated window, not the whole period scan_info's exp_time is the trigger PERIOD. DDG2 gates for exp_time minus a readout time, but the Eiger was sent the full exp_time as image_time_us, so the detector acquired 180 us past the falling edge of its own gate and finished only 20 us before the next trigger -- that 20 us being JungfrauJoch's internal board-readout allowance, and the entire margin available. Measured on a scope as a 172.8 us overhang. Four independent notions of "readout time" existed: 500 us EIGER*_READOUT_TIME_US validation floor only, no effect on anything 200 us DDG2 DEFAULT_READOUT_TIMES["ab"] sets the gate width 20 us JungfrauJoch deployment config applied internally by JFJoch - scan parameter readout_time honoured by the Falcon, ignored by DDG2 They are now one. DDG2 derives the gap from the scan's readout_time, floored by its configured value (scans default it to 0), and exposes effective_readout_times(). The Eiger subtracts the same number from image_time_us. The Falcon already used the scan value, so it needs no change. The Eiger's 500 us constant is split in two, because it was doing two jobs: a MIN_EXP_TIME validation floor keeps today's behaviour exactly, while the new EIGER_READOUT_TIME defaults to 2e-4 to match the delay generator. The _US suffix on constants holding seconds is dropped in all three modules. Both devices log the effective exposure window at on_stage, so a period/exposure mismatch is visible in the logs rather than only on an oscilloscope. NOTE: this shortens the delivered exposure by the readout time -- 200 us, i.e. 0.5% at 40 ms -- for scans that do not set readout_time. Users should be told before this is deployed. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01KLnmUurqcNd1FiDY5M2uZr --- .../epics/delay_generator_csaxs/ddg_2.py | 43 +++++++--- csaxs_bec/devices/jungfraujoch/eiger.py | 46 ++++++++--- csaxs_bec/devices/jungfraujoch/eiger_1_5m.py | 6 +- csaxs_bec/devices/jungfraujoch/eiger_9m.py | 6 +- .../test_delay_generator_csaxs.py | 30 +++++++ tests/tests_devices/test_eiger.py | 79 ++++++++++++++++++- 6 files changed, 185 insertions(+), 25 deletions(-) diff --git a/csaxs_bec/devices/epics/delay_generator_csaxs/ddg_2.py b/csaxs_bec/devices/epics/delay_generator_csaxs/ddg_2.py index 5ee38fe5..533aecd6 100644 --- a/csaxs_bec/devices/epics/delay_generator_csaxs/ddg_2.py +++ b/csaxs_bec/devices/epics/delay_generator_csaxs/ddg_2.py @@ -120,7 +120,7 @@ class DDG2(PSIDeviceBase, DelayGeneratorCSAXS): """ # Methods exposed to the BEC client, i.e. callable as dev.ddg2.() - USER_ACCESS = ["get_readout_times", "set_readout_times"] + USER_ACCESS = ["get_readout_times", "set_readout_times", "effective_readout_times"] def __init__( self, @@ -144,6 +144,25 @@ class DDG2(PSIDeviceBase, DelayGeneratorCSAXS): if readout_times: self.set_readout_times(**readout_times) + def effective_readout_times(self) -> dict[str, float]: + """Return the readout times actually in force for the upcoming scan. + + The scan may request a readout time of its own; it wins when it is larger than + this device's configured value. Scans default readout_time to 0, so the + configured value is the floor rather than a fallback. + + The result defines the exposure window -- period minus gap -- that every + detector in the scan is expected to integrate over, whether it reads the gate + (Falcon) or runs its own timer from the rising edge (Eiger). + + Returns: + dict[str, float]: effective readout time per channel pair, in seconds. + """ + scan_readout = 0.0 + if self.scan_parameters is not None: + scan_readout = float(getattr(self.scan_parameters, "readout_time", 0.0) or 0.0) + return {ch: max(scan_readout, rt) for ch, rt in self._readout_times.items()} + def get_readout_times(self) -> dict[str, float]: """Return the readout times per channel pair, in seconds. @@ -270,10 +289,14 @@ class DDG2(PSIDeviceBase, DelayGeneratorCSAXS): # NOTE Check if the exposure time is longer than all readout times. # Raise a ValueError if requested exposure time is too short. - if any(exp_time <= rt for rt in self._readout_times.values()): + # NOTE The gap is the scan's readout_time when the scan asks for one, otherwise this + # device's configured value. Scans default readout_time to 0, hence the max(). + # Every detector must derive its exposure window from the same number -- see + # effective_readout_times(). + readouts = self.effective_readout_times() + if any(exp_time <= rt for rt in readouts.values()): raise ValueError( - f"Exposure time {exp_time} is too short for the readout times " - f"{self._readout_times}" + f"Exposure time {exp_time} is too short for the readout times {readouts}" ) ######################################### @@ -288,13 +311,11 @@ class DDG2(PSIDeviceBase, DelayGeneratorCSAXS): # Calculate the pulse width for the channel pair 'ab'. The readout time is the # gap left between consecutive triggers; see set_readout_times(). - burst_pulse_width = exp_time - self._readout_times["ab"] - if self._readout_times["ab"] != DEFAULT_READOUT_TIMES["ab"]: - logger.info( - f"DDG {self.name} using non-default readout time " - f"{self._readout_times['ab']}s for 'ab' (default " - f"{DEFAULT_READOUT_TIMES['ab']}s); pulse width {burst_pulse_width}s" - ) + burst_pulse_width = exp_time - readouts["ab"] + logger.info( + f"DDG {self.name} exposure window: period {exp_time}s, gap {readouts['ab']}s, " + f"pulse width {burst_pulse_width}s" + ) # Trigger detectors with delay 0, and pulse width = exp_time - readout_time self.set_delay_pairs(channel="ab", delay=0, width=burst_pulse_width) diff --git a/csaxs_bec/devices/jungfraujoch/eiger.py b/csaxs_bec/devices/jungfraujoch/eiger.py index cdcf382a..27b4a050 100644 --- a/csaxs_bec/devices/jungfraujoch/eiger.py +++ b/csaxs_bec/devices/jungfraujoch/eiger.py @@ -8,10 +8,15 @@ A few notes on setup and operation of the Eiger detectors through the JungfrauJo - Changes in energy may take time, good to implement logic that only resets energy if needed. - For the Eiger, the frame_time_us in DetectorSettings is ignored, only the frame_time_us in the DatasetSettings is relevant -- The bit_depth will be adjusted automatically based on the exp_time. Here, we need to ensure - that subsequent triggers properly consider the readout_time of the boards. For the Eiger detectors - at cSAXS, a readout time of 20us is configured through the JungfrauJoch deployment config. This - setting is sufficiently large for the detectors if they run in parallel mode. +- The bit_depth will be adjusted automatically based on the exp_time (32 bit above 2622 us), + and in 32 bit the detector accumulates ~2.3 ms subframes. JungfrauJoch's own deployment config + budgets 20 us of board readout between frames, which is right for 16 bit parallel mode but far + smaller than the subframe granularity -- so a frame can outlast the trigger period without + anything reporting an error. +- scan_info's exp_time is the trigger PERIOD, not the exposure. The delay generator gates for + exp_time - readout_time, and on_stage sends that same window as image_time_us so the detector + is not still acquiring when the next trigger arrives. The readout time must agree with DDG2's + DEFAULT_READOUT_TIMES["ab"]; a scan-level readout_time overrides both when larger. - beam_center and detector settings are required input arguments, thus, they may be set to wrong values for acquisitions to start. Please keep this in mind. @@ -65,7 +70,14 @@ if TYPE_CHECKING: # pragma no cover logger = bec_logger.logger -EIGER_READOUT_TIME_US = 500e-6 # 500 microseconds in s +# NOTE Two distinct numbers that used to be one. MIN_EXP_TIME is a validation floor +# (its value is inherited from the former EIGER_READOUT_TIME_US and has no documented +# provenance). EIGER_READOUT_TIME is the gap left between the end of one acquisition and +# the next trigger; it must match the gap the delay generator leaves in the pulse train, +# DDG2's DEFAULT_READOUT_TIMES["ab"], or the detector is told to acquire for longer than +# the trigger period allows. +MIN_EXP_TIME = 500e-6 # s +EIGER_READOUT_TIME = 2e-4 # s -- matches DDG2 DEFAULT_READOUT_TIMES["ab"] class EigerError(Exception): @@ -106,7 +118,7 @@ class Eiger(PSIDeviceBase): detector_distance: float = 100.0, beam_center: tuple[int, int] = (0, 0), scan_info: ScanInfo = None, - readout_time: float = EIGER_READOUT_TIME_US, + readout_time: float = EIGER_READOUT_TIME, device_manager=None, **kwargs, ): @@ -344,9 +356,21 @@ class Eiger(PSIDeviceBase): # Setting up exp_time and num_triggers acquisition parameter exp_time = self.scan_parameters.exp_time - if exp_time <= self._readout_time: # Exp_time must be at least the readout time + if exp_time <= MIN_EXP_TIME: raise ValueError( - f"Value error on device {self.name}: Exposure time {exp_time}s is less than readout time {self._readout_time}s." + f"Value error on device {self.name}: Exposure time {exp_time}s is below the " + f"minimum exposure time {MIN_EXP_TIME}s." + ) + # NOTE exp_time is the trigger PERIOD, not the exposure. The delay generator gates + # for exp_time - readout_time, so the detector must acquire for the same window or + # it is still busy when the next trigger arrives. Scans default readout_time to 0, + # hence the max() against this device's configured value. + readout_time = max(self.scan_parameters.readout_time, self._readout_time) + acquisition_time = exp_time - readout_time + if acquisition_time <= 0: + raise ValueError( + f"Value error on device {self.name}: readout time {readout_time}s leaves no " + f"acquisition time within the trigger period {exp_time}s." ) self._num_triggers = int( self.scan_parameters.num_points * self.scan_parameters.frames_per_trigger @@ -369,7 +393,7 @@ class Eiger(PSIDeviceBase): # Create dataset settings for API call. data_settings = DatasetSettings( - image_time_us=int(exp_time * 1e6), + image_time_us=int(acquisition_time * 1e6), ntrigger=self._num_triggers, file_prefix=path, beam_x_pxl=float(self._beam_center[0]), @@ -379,6 +403,10 @@ class Eiger(PSIDeviceBase): ) logger.debug(f"Setting data_settings: {yaml.dump(data_settings.to_dict(), indent=4)}") prep_time = time.time() + logger.info( + f"{self.name} exposure window: period {exp_time}s, readout {readout_time}s, " + f"acquisition {acquisition_time}s" + ) self.jfj_client.wait_for_idle(timeout=10) # Ensure we are in IDLE state self.jfj_client.start(settings=data_settings) # Takes around ~0.6s -> 6s currently. diff --git a/csaxs_bec/devices/jungfraujoch/eiger_1_5m.py b/csaxs_bec/devices/jungfraujoch/eiger_1_5m.py index 22432c7c..bfdb3646 100644 --- a/csaxs_bec/devices/jungfraujoch/eiger_1_5m.py +++ b/csaxs_bec/devices/jungfraujoch/eiger_1_5m.py @@ -11,7 +11,9 @@ from typing import TYPE_CHECKING from csaxs_bec.devices.jungfraujoch.eiger import Eiger -EIGER1_5M_READOUT_TIME_US = 500e-6 # 500 microseconds in s +# 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 DETECTOR_NAME = "EIGER 1.5M" @@ -45,7 +47,7 @@ class Eiger1_5M(Eiger): super().__init__( name=name, detector_name=DETECTOR_NAME, - readout_time=EIGER1_5M_READOUT_TIME_US, + readout_time=EIGER1_5M_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 f2061032..bb305dd5 100644 --- a/csaxs_bec/devices/jungfraujoch/eiger_9m.py +++ b/csaxs_bec/devices/jungfraujoch/eiger_9m.py @@ -20,7 +20,9 @@ if TYPE_CHECKING: # pragma no cover from bec_lib.devicemanager import ScanInfo from bec_server.device_server.device_server import DeviceManagerDS -EIGER9M_READOUT_TIME_US = 500e-6 # 500 microseconds in s +# 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 DETECTOR_NAME = "EIGER 9M" # "EIGER 9M"" @@ -49,7 +51,7 @@ class Eiger9M(Eiger): super().__init__( name=name, detector_name=DETECTOR_NAME, - readout_time=EIGER9M_READOUT_TIME_US, + readout_time=EIGER9M_READOUT_TIME, detector_distance=detector_distance, beam_center=beam_center, scan_info=scan_info, diff --git a/tests/tests_devices/test_delay_generator_csaxs.py b/tests/tests_devices/test_delay_generator_csaxs.py index 19810450..84f0061d 100644 --- a/tests/tests_devices/test_delay_generator_csaxs.py +++ b/tests/tests_devices/test_delay_generator_csaxs.py @@ -675,3 +675,33 @@ def test_ddg2_too_long_readout_time_is_caught_at_stage(mock_ddg2: DDG2): ddg.set_readout_times(ab=3e-3) with pytest.raises(ValueError): ddg.stage() + + +def test_ddg2_effective_readout_uses_the_scan_value_when_larger(mock_ddg2: DDG2): + """A scan-level readout_time widens the gap; the configured value is the floor.""" + ddg = mock_ddg2 + exp_time = 0.1 + ddg.on_connected() + ddg.scan_info.msg.info["exp_time"] = exp_time + ddg.scan_info.msg.info["frames_per_trigger"] = 1 + ddg.scan_info.msg.info["readout_time"] = 3e-3 + + ddg.stage() + assert ddg.effective_readout_times()["ab"] == 3e-3 + assert np.isclose(ddg.ab.width.get(), exp_time - 3e-3) + ddg.unstage() + + +def test_ddg2_effective_readout_falls_back_to_the_configured_value(mock_ddg2: DDG2): + """Scans default readout_time to 0, so the device value must still apply.""" + ddg = mock_ddg2 + exp_time = 0.1 + ddg.on_connected() + ddg.scan_info.msg.info["exp_time"] = exp_time + ddg.scan_info.msg.info["frames_per_trigger"] = 1 + ddg.scan_info.msg.info["readout_time"] = 0.0 + + ddg.stage() + assert ddg.effective_readout_times()["ab"] == DDG2_DEFAULT_READOUT_TIMES["ab"] + assert np.isclose(ddg.ab.width.get(), exp_time - DDG2_DEFAULT_READOUT_TIMES["ab"]) + ddg.unstage() diff --git a/tests/tests_devices/test_eiger.py b/tests/tests_devices/test_eiger.py index 3e94b424..c07920cd 100644 --- a/tests/tests_devices/test_eiger.py +++ b/tests/tests_devices/test_eiger.py @@ -22,6 +22,7 @@ from ophyd import Staged 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 @@ -416,7 +417,8 @@ def test_eiger_on_stage(eiger_1_5m): assert file_msg.hinted_h5_entries == {"data": "entry/data/data"} data_settings = DatasetSettings( - image_time_us=int(scan_msg.scan_parameters["exp_time"] * 1e6), + # exp_time is the trigger period; the detector acquires for the gated window + image_time_us=int((scan_msg.scan_parameters["exp_time"] - EIGER_READOUT_TIME) * 1e6), ntrigger=int(scan_msg.num_points * scan_msg.scan_parameters["frames_per_trigger"]), file_prefix=os.path.relpath(eiger._full_path, start="/sls/x12sa/data").removesuffix( "_master.h5" @@ -460,3 +462,78 @@ def test_eiger_preview_callback(eiger_1_5m): mock_preview_put.assert_called_once_with(msg["data"]["default"]) else: mock_preview_put.assert_not_called() + + +def test_eiger_acquisition_matches_the_gated_window(eiger_1_5m): + """The detector must acquire for exp_time - readout, not for the whole period. + + exp_time is the trigger period. The delay generator gates for exp_time - readout, so + sending the full period leaves the detector still acquiring when the next trigger + arrives -- 20 us of margin, which is what JungfrauJoch subtracts internally. + """ + eiger = eiger_1_5m + exp_time = eiger.scan_info.msg.scan_parameters["exp_time"] + + with ( + mock.patch.object(eiger.jfj_client, "wait_for_idle", return_value=True), + mock.patch.object(eiger.jfj_client, "start") as mock_start, + mock.patch.object( + eiger.jfj_client.api, "status_get", return_value=BrokerStatus(state="Idle") + ), + ): + eiger.stage() + + sent = mock_start.call_args.kwargs["settings"].image_time_us + assert sent == int((exp_time - EIGER_READOUT_TIME) * 1e6) + assert sent < int(exp_time * 1e6) + + +def test_eiger_scan_readout_time_wins_when_larger(eiger_1_5m): + """A scan asking for a wider gap must widen the acquisition window too.""" + eiger = eiger_1_5m + exp_time = eiger.scan_info.msg.scan_parameters["exp_time"] + eiger.scan_info.msg.info["readout_time"] = 3e-3 # > EIGER_READOUT_TIME + + with ( + mock.patch.object(eiger.jfj_client, "wait_for_idle", return_value=True), + mock.patch.object(eiger.jfj_client, "start") as mock_start, + mock.patch.object( + eiger.jfj_client.api, "status_get", return_value=BrokerStatus(state="Idle") + ), + ): + eiger.stage() + + assert mock_start.call_args.kwargs["settings"].image_time_us == int((exp_time - 3e-3) * 1e6) + + +def test_eiger_rejects_a_readout_that_fills_the_period(eiger_1_5m): + """A gap wider than the period leaves nothing to acquire; fail rather than send <= 0.""" + eiger = eiger_1_5m + exp_time = eiger.scan_info.msg.scan_parameters["exp_time"] + eiger.scan_info.msg.info["readout_time"] = exp_time + 1e-3 + + with ( + mock.patch.object(eiger.jfj_client, "wait_for_idle", return_value=True), + mock.patch.object(eiger.jfj_client, "start"), + mock.patch.object( + eiger.jfj_client.api, "status_get", return_value=BrokerStatus(state="Idle") + ), + ): + with pytest.raises(ValueError): + eiger.stage() + + +def test_eiger_minimum_exposure_check_is_unchanged(eiger_1_5m): + """Splitting readout from the validation floor must not relax the minimum exposure.""" + eiger = eiger_1_5m + eiger.scan_info.msg.info["exp_time"] = MIN_EXP_TIME / 2 + + with ( + mock.patch.object(eiger.jfj_client, "wait_for_idle", return_value=True), + mock.patch.object(eiger.jfj_client, "start"), + mock.patch.object( + eiger.jfj_client.api, "status_get", return_value=BrokerStatus(state="Idle") + ), + ): + with pytest.raises(ValueError): + eiger.stage() From 64c2fff6e6e5d6f215c9dab681ea4f090bec5700 Mon Sep 17 00:00:00 2001 From: menzel Date: Wed, 2 Sep 2026 17:42:38 +0200 Subject: [PATCH 3/7] 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 From cfc7271d40f5da8271d20d8b2fd5b3f62dfc14ef Mon Sep 17 00:00:00 2001 From: menzel Date: Wed, 2 Sep 2026 17:54:35 +0200 Subject: [PATCH 4/7] docs(eiger): record that triggering emulates gating, and why The Eiger is triggered rather than gated for stability, and the pulse train is shaped so its internal timer coincides with the gate -- it is meant to behave as if gated, so that every detector in a scan integrates the same window. Nothing in the code said so, which is why sending the full exp_time as image_time_us looked reasonable and stayed wrong until a scope showed the 173 us overhang. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01KLnmUurqcNd1FiDY5M2uZr --- csaxs_bec/devices/jungfraujoch/eiger.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/csaxs_bec/devices/jungfraujoch/eiger.py b/csaxs_bec/devices/jungfraujoch/eiger.py index 60696b4e..bc915928 100644 --- a/csaxs_bec/devices/jungfraujoch/eiger.py +++ b/csaxs_bec/devices/jungfraujoch/eiger.py @@ -13,6 +13,12 @@ A few notes on setup and operation of the Eiger detectors through the JungfrauJo budgets 20 us of board readout between frames, which is right for 16 bit parallel mode but far smaller than the subframe granularity -- so a frame can outlast the trigger period without anything reporting an error. +- The Eiger is TRIGGERED rather than gated (DetectorTiming.TRIGGER), for stability: gating + it proved unreliable. It therefore ignores the gate width and runs its own timer from the + rising edge. The pulse train is deliberately shaped so that this timer coincides with the + gate -- the Eiger is meant to behave *as if* it were gated, so that every detector in the + scan integrates the same window. Sending it anything other than the gated window breaks + that, silently, and is only visible on a scope. - scan_info's exp_time is the trigger PERIOD, not the exposure. The delay generator gates for exp_time - readout_time, and on_stage sends that same window as image_time_us so the detector is not still acquiring when the next trigger arrives. The readout time must agree with DDG2's From 4c53f2cf251bfa2ac4d703a98bc23621d2077c51 Mon Sep 17 00:00:00 2001 From: wakonig_k Date: Sat, 5 Sep 2026 12:56:52 +0200 Subject: [PATCH 5/7] fix(mcs_card): handle excess data points and improve MCA callback suppression --- .../devices/epics/mcs_card/mcs_card_csaxs.py | 24 +++++++++--- tests/tests_devices/test_mcs_card.py | 37 ++++++++++++++++++- 2 files changed, 53 insertions(+), 8 deletions(-) diff --git a/csaxs_bec/devices/epics/mcs_card/mcs_card_csaxs.py b/csaxs_bec/devices/epics/mcs_card/mcs_card_csaxs.py index e2b87184..70511295 100644 --- a/csaxs_bec/devices/epics/mcs_card/mcs_card_csaxs.py +++ b/csaxs_bec/devices/epics/mcs_card/mcs_card_csaxs.py @@ -70,11 +70,14 @@ def suppress_mca_callbacks(mcs_card: MCSCard, restore_after_timeout: None | floa """ with mcs_card._rlock: mcs_card._omit_mca_callbacks.set() # pylint: disable=protected-access - try: - yield - finally: - if restore_after_timeout is not None: - time.sleep(restore_after_timeout) + try: + # MCA callbacks must be able to acquire the data lock while suppression + # is active so they are discarded instead of queued until afterward. + yield + finally: + if restore_after_timeout is not None: + time.sleep(restore_after_timeout) + with mcs_card._rlock: mcs_card._omit_mca_callbacks.clear() # pylint: disable=protected-access @@ -467,7 +470,16 @@ class MCSCardCSAXS(PSIDeviceBase, MCSCard): logger.info( f"Software triggered scan: {self._current_data_index}/{self.scan_parameters.num_points} points received." ) - if self._current_data_index == self.scan_parameters.num_points: + if self._current_data_index > self.scan_parameters.num_points: + exception = RuntimeError( + f"MCS card {self.name} emitted {self._current_data_index} " + f"data points, but the scan requested " + f"{self.scan_parameters.num_points}." + ) + logger.error(str(exception)) + for callback in self._scan_done_callbacks: + callback(exception=exception) + elif self._current_data_index == self.scan_parameters.num_points: for callback in self._scan_done_callbacks: callback(exception=None) else: diff --git a/tests/tests_devices/test_mcs_card.py b/tests/tests_devices/test_mcs_card.py index 2f8f6b07..0bfa70aa 100644 --- a/tests/tests_devices/test_mcs_card.py +++ b/tests/tests_devices/test_mcs_card.py @@ -24,7 +24,7 @@ from csaxs_bec.devices.epics.mcs_card.mcs_card import ( READMODE, MCSCard, ) -from csaxs_bec.devices.epics.mcs_card.mcs_card_csaxs import MCSCardCSAXS +from csaxs_bec.devices.epics.mcs_card.mcs_card_csaxs import MCSCardCSAXS, suppress_mca_callbacks from csaxs_bec.devices.utils.utils import fetch_scan_info @@ -167,9 +167,10 @@ def test_mcs_card_csaxs_complete_and_stop(mock_mcs_csaxs: MCSCardCSAXS): """ Test complete method of MCSCarcCSAXS. - Two use cases: + Three use cases: I. Acquisition is stopped externally II. Acquisition completes normally + III. Acquisition emits more points than requested """ mcs = mock_mcs_csaxs mcs.scan_parameters = fetch_scan_info(mcs.scan_info) @@ -219,6 +220,38 @@ def test_mcs_card_csaxs_complete_and_stop(mock_mcs_csaxs: MCSCardCSAXS): mcs._start_monitor_async_data_emission.wait(2) assert not mcs._start_monitor_async_data_emission.is_set() + ############################## + # III. Extra data fails fast # + ############################## + mcs._current_data_index = 11 + st = mcs.complete() + with pytest.raises(RuntimeError, match="emitted 11 data points, but the scan requested 10"): + st.wait(timeout=1) + + +def test_suppress_mca_callbacks_discards_callbacks_without_blocking(mock_mcs_csaxs: MCSCardCSAXS): + """A complete MCA callback batch is discarded while suppression is active.""" + mcs = mock_mcs_csaxs + callback_finished = threading.Event() + + def emit_callbacks(): + for index in range(mcs.NUM_MCA_CHANNELS): + counter = getattr(mcs.counters, f"mca{index + 1}") + mcs._on_counter_update(index, obj=counter, timestamp=1.0) + callback_finished.set() + + worker = threading.Thread(target=emit_callbacks) + with mock.patch.object(mcs.mca, "put") as mca_put: + with suppress_mca_callbacks(mcs, restore_after_timeout=0): + worker.start() + assert callback_finished.wait(timeout=1) + + worker.join(timeout=1) + assert not worker.is_alive() + assert mcs._current_data == {} + assert mcs._current_data_index == 0 + mca_put.assert_not_called() + def test_mcs_on_stop(mock_mcs_csaxs: MCSCardCSAXS): """Test that on stop sets the omit_mca_callbacks flag. Also test that on stage clears the omit_mca_callbacks flag.""" From 653351fc15630b789dcee84754d880cd25e4f0b6 Mon Sep 17 00:00:00 2001 From: menzel Date: Tue, 8 Sep 2026 12:39:15 +0200 Subject: [PATCH 6/7] feat(eiger): make the missing-packet tolerance per-detector and switchable The JungfrauJoch broker reports packet loss during data collection as a plain error, which fails the scan. Since 2026-09-03 that error has been suppressed on the beamline by an uncommitted edit, through a hardcoded flag on the Eiger base class: it applied to every Eiger at once, it could be reached neither from the client nor from deviceConfig, and it left nothing in the log. The tolerance is now a real parameter, raise_on_missing_packets: - named in Eiger, Eiger9M and Eiger1_5M, so that a deviceConfig key actually reaches the device. bec_server intersects config keys with the named parameters of the class, so a flag reachable only through **kwargs is silently dropped -- the same trap as readout_time (f450f29) and prefix (10be2b5). A test pins the signatures. - exposed through USER_ACCESS as get_/set_raise_on_missing_packets, so a beamtime can change its mind without a redeployment. Like every runtime value it is shared between clients and does not survive a server restart; deviceConfig is what makes a choice stick. - counted, and logged at warning level whenever an error is let through, so that "which scans were affected?" has an answer. get_missing_packet_events() returns the count. What is tolerated is narrower than it looks: the frame-count check below still raises when statistics.images_collected falls short of the trigger count. Only "the broker flagged packet loss but delivered the expected number of images" gets through, and a test pins that a short acquisition still fails. The default stays False, i.e. tolerate, so the running beamtime is unaffected. It should become True once the 9M's packet loss is understood, with raise_on_missing_packets: false in that detector's deviceConfig if it still needs it. That is one constant to change, RAISE_ON_MISSING_PACKETS. The wording of the broker message is the only handle available, as there is no error code for it. If JungfrauJoch rephrases it the match stops working and the error raises again, which is the safe direction to fail in. test_eiger_on_complete_error_message was skipped as failing "because the error should be skipped for now due to HW issues". With the tolerance scoped to the missing-packet message it passes again, and is no longer skipped. Co-Authored-By: Claude Opus 5 (1M context) --- csaxs_bec/devices/jungfraujoch/eiger.py | 89 +++++++++++- csaxs_bec/devices/jungfraujoch/eiger_1_5m.py | 4 +- csaxs_bec/devices/jungfraujoch/eiger_9m.py | 4 +- tests/tests_devices/test_eiger.py | 138 ++++++++++++++++++- 4 files changed, 221 insertions(+), 14 deletions(-) diff --git a/csaxs_bec/devices/jungfraujoch/eiger.py b/csaxs_bec/devices/jungfraujoch/eiger.py index bc915928..789168af 100644 --- a/csaxs_bec/devices/jungfraujoch/eiger.py +++ b/csaxs_bec/devices/jungfraujoch/eiger.py @@ -84,6 +84,19 @@ 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 The JungfrauJoch broker reports packet loss as a plain error with no error code, +# so the only way to recognise it is the wording of its status message. If JFJoch ever +# rephrases this, the match stops working and the error raises again -- which is the safe +# direction to fail in. +MISSING_PACKETS_MESSAGE = "Missing packets in data collection" + +# NOTE Default for raise_on_missing_packets, i.e. whether a missing-packet error from the +# broker fails the scan. False while the 9M is losing packets during the 2026-09 beamtime +# and scans must not die on it. Set it back to True once that is resolved, and put +# "raise_on_missing_packets: false" in the deviceConfig of the detector that still needs +# it -- per detector, rather than for every Eiger at once. +RAISE_ON_MISSING_PACKETS = False # 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 @@ -106,6 +119,8 @@ class Eiger(PSIDeviceBase): port (int): Port of the Jungfrau Joch server. scan_info (ScanInfo): The scan info to use. device_manager (DeviceManagerDS): The device manager to use. + raise_on_missing_packets (bool): Whether a 'missing packets' error from the + JungfrauJoch broker fails the scan. See RAISE_ON_MISSING_PACKETS. **kwargs: Additional keyword arguments. """ @@ -115,6 +130,9 @@ class Eiger(PSIDeviceBase): "print_detector_distance_and_center", "set_azi_settings", "get_azi_settings", + "get_raise_on_missing_packets", + "set_raise_on_missing_packets", + "get_missing_packet_events", ] file_event = Cpt(FileEventSignal, name="file_event") @@ -130,6 +148,7 @@ class Eiger(PSIDeviceBase): beam_center: tuple[int, int] = (0, 0), scan_info: ScanInfo = None, readout_time: float = EIGER_READOUT_TIME, + raise_on_missing_packets: bool = RAISE_ON_MISSING_PACKETS, device_manager=None, **kwargs, ): @@ -153,6 +172,47 @@ class Eiger(PSIDeviceBase): if self.device_manager is not None: self.device_manager: DeviceManagerDS self.scan_parameters: ScanServerScanInfo | None = None + self._raise_on_missing_packets = bool(raise_on_missing_packets) + self._missing_packet_events = 0 + + def get_raise_on_missing_packets(self) -> bool: + """ + Whether a 'missing packets' error from the JungfrauJoch broker fails the scan. + + Returns: + bool: True if missing packets raise an EigerError, False if they are tolerated. + """ + return self._raise_on_missing_packets + + def set_raise_on_missing_packets(self, value: bool) -> None: + """ + Set whether a 'missing packets' error from the JungfrauJoch broker fails the scan. + + NOTE This is shared by every client of this device server, and it does NOT survive a + server restart: '__init__' runs again and restores the value from deviceConfig. To make + a choice stick for a beamtime, set 'raise_on_missing_packets' in the deviceConfig of the + detector instead. + + Args: + value (bool): True to raise on missing packets, False to tolerate them. + """ + self._raise_on_missing_packets = bool(value) + logger.warning( + f"Device {self.name}: raise_on_missing_packets is now " + f"{self._raise_on_missing_packets}." + ) + + def get_missing_packet_events(self) -> int: + """ + How many missing-packet errors have been tolerated since this device was created. + + Reset by a server restart, like the flag itself. Non-zero means acquisitions completed + that the broker had flagged, so the data of those scans may be incomplete. + + Returns: + int: Number of tolerated missing-packet errors. + """ + return self._missing_packet_events def _preview_callback(self, message: dict) -> None: """ @@ -477,13 +537,28 @@ class Eiger(PSIDeviceBase): logger.info( f"JungfrauJoch broker status: {yaml.dump(broker_status.to_dict(), indent=4)}" ) - if broker_status.message_severity == "error": # Raise on error - raise EigerError( - f"Device {self.name} acquisition completed with error status from JungfrauJoch broker: {yaml.dump(broker_status.to_dict(), indent=4)}" - ) - # logger.warning( - # f"Device {self.name} acquisition completed with error status from JungfrauJoch broker: {yaml.dump(broker_status.to_dict(), indent=4)}" - # ) + if broker_status.message_severity == "error": + # NOTE Missing packets can be tolerated per detector, because the frame + # count is still checked below: what is being let through here is "the + # broker flagged packet loss but delivered the expected number of + # images", not a short acquisition. + if ( + MISSING_PACKETS_MESSAGE in (broker_status.message or "") + and not self._raise_on_missing_packets + ): + self._missing_packet_events += 1 + logger.warning( + f"Device {self.name} reported missing packets and the error is " + f"being tolerated (raise_on_missing_packets is False), so the " + f"data of this scan may be incomplete. That is tolerated event " + f"{self._missing_packet_events} for this device since it was " + f"created. Broker status: " + f"{yaml.dump(broker_status.to_dict(), indent=4)}" + ) + else: + raise EigerError( + f"Device {self.name} acquisition completed with error status from JungfrauJoch broker: {yaml.dump(broker_status.to_dict(), indent=4)}" + ) # Call API endpoint to get statistics statistics: MeasurementStatistics = ( self.jfj_client.api.statistics_data_collection_get(_request_timeout=5) diff --git a/csaxs_bec/devices/jungfraujoch/eiger_1_5m.py b/csaxs_bec/devices/jungfraujoch/eiger_1_5m.py index 92608502..f5bbcbf5 100644 --- a/csaxs_bec/devices/jungfraujoch/eiger_1_5m.py +++ b/csaxs_bec/devices/jungfraujoch/eiger_1_5m.py @@ -9,7 +9,7 @@ from __future__ import annotations from typing import TYPE_CHECKING -from csaxs_bec.devices.jungfraujoch.eiger import Eiger +from csaxs_bec.devices.jungfraujoch.eiger import RAISE_ON_MISSING_PACKETS, Eiger # 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 @@ -45,12 +45,14 @@ class Eiger1_5M(Eiger): scan_info: ScanInfo = None, device_manager: DeviceManagerDS = None, readout_time: float = EIGER1_5M_READOUT_TIME, + raise_on_missing_packets: bool = RAISE_ON_MISSING_PACKETS, **kwargs, ) -> None: super().__init__( name=name, detector_name=DETECTOR_NAME, readout_time=readout_time, + raise_on_missing_packets=raise_on_missing_packets, 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 13f1a4c6..4419c61f 100644 --- a/csaxs_bec/devices/jungfraujoch/eiger_9m.py +++ b/csaxs_bec/devices/jungfraujoch/eiger_9m.py @@ -14,7 +14,7 @@ from __future__ import annotations from typing import TYPE_CHECKING -from csaxs_bec.devices.jungfraujoch.eiger import Eiger +from csaxs_bec.devices.jungfraujoch.eiger import RAISE_ON_MISSING_PACKETS, Eiger if TYPE_CHECKING: # pragma no cover from bec_lib.devicemanager import ScanInfo @@ -49,12 +49,14 @@ class Eiger9M(Eiger): scan_info: ScanInfo = None, device_manager: DeviceManagerDS = None, readout_time: float = EIGER9M_READOUT_TIME, + raise_on_missing_packets: bool = RAISE_ON_MISSING_PACKETS, **kwargs, ) -> None: super().__init__( name=name, detector_name=DETECTOR_NAME, readout_time=readout_time, + raise_on_missing_packets=raise_on_missing_packets, 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 20dcf483..285dff5b 100644 --- a/tests/tests_devices/test_eiger.py +++ b/tests/tests_devices/test_eiger.py @@ -23,7 +23,12 @@ from ophyd import Staged 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 import ( + EIGER_READOUT_TIME, + MIN_EXP_TIME, + MISSING_PACKETS_MESSAGE, + RAISE_ON_MISSING_PACKETS, +) 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 @@ -329,11 +334,12 @@ def test_eiger_on_complete(eiger_1_5m, raise_timeout): assert status.success == False if raise_timeout else True -@pytest.mark.skip( - reason="This test currently fails because the error should be skipped for now due to HW issues." -) def test_eiger_on_complete_error_message(eiger_1_5m): - """Test that on_complete raises if the message severity is error.""" + """Test that on_complete raises if the message severity is error. + + Unrelated to the missing-packet tolerance: this message is a different error, so it + raises whatever raise_on_missing_packets is set to. + """ # Test that on_complete raises if message_severity is error eiger = eiger_1_5m unblock_wait_for_idle = threading.Event() @@ -571,3 +577,125 @@ def test_eiger_subclasses_name_readout_time_for_the_device_server(): """ for cls in (Eiger1_5M, Eiger9M): assert "readout_time" in inspect.signature(cls).parameters + + +def _run_on_complete(eiger, broker_status, images_collected=0, timeout=5): + """Drive on_complete to a verdict against a given broker status. + + Returns the status once it resolves; raises whatever on_complete raised. + """ + eiger._wait_for_on_complete = 1 # keep the loop short + unblock_wait_for_idle = threading.Event() + + def mock_wait_for_idle(timeout: float, raise_on_timeout: bool) -> bool: + return unblock_wait_for_idle.wait(timeout) + + with ( + mock.patch.object(eiger.jfj_client.api, "status_get", return_value=broker_status), + mock.patch.object(eiger.jfj_client, "wait_for_idle", side_effect=mock_wait_for_idle), + mock.patch.object( + eiger.jfj_client.api, + "statistics_data_collection_get", + return_value=MeasurementStatistics(run_number=1, images_collected=images_collected), + ), + ): + status = eiger.complete() + unblock_wait_for_idle.set() + status.wait(timeout=timeout) + return status + + +def _missing_packets_status(): + """A broker status carrying the packet-loss error JungfrauJoch actually emits.""" + return BrokerStatus( + state="Idle", message_severity="error", message=f"{MISSING_PACKETS_MESSAGE} (run 42)" + ) + + +def test_eiger_tolerates_missing_packets_when_configured_to(eiger_1_5m): + """With the tolerance on, a missing-packet error completes the scan and is counted.""" + eiger = eiger_1_5m + eiger.set_raise_on_missing_packets(False) + + status = _run_on_complete(eiger, _missing_packets_status()) + + assert status.done is True + assert status.success is True + assert eiger.get_missing_packet_events() == 1 + + +def test_eiger_raises_on_missing_packets_when_asked_to(eiger_1_5m): + """With the tolerance off, the same error fails the scan and is not counted.""" + eiger = eiger_1_5m + eiger.set_raise_on_missing_packets(True) + + with pytest.raises(EigerError): + _run_on_complete(eiger, _missing_packets_status()) + + assert eiger.get_missing_packet_events() == 0 + + +def test_eiger_tolerance_does_not_extend_to_other_broker_errors(eiger_1_5m): + """Tolerating packet loss must not tolerate every error the broker reports.""" + eiger = eiger_1_5m + eiger.set_raise_on_missing_packets(False) + + with pytest.raises(EigerError): + _run_on_complete( + eiger, BrokerStatus(state="Idle", message_severity="error", message="Detector on fire") + ) + + assert eiger.get_missing_packet_events() == 0 + + +def test_eiger_tolerance_does_not_hide_a_short_acquisition(eiger_1_5m): + """The frame-count check still fires: only flagged-but-complete data is let through. + + This is what makes the tolerance defensible -- a scan that actually lost frames still + fails, whatever raise_on_missing_packets says. + """ + eiger = eiger_1_5m + eiger.set_raise_on_missing_packets(False) + eiger._num_triggers = 10 + + with pytest.raises(EigerError, match="acquisition incomplete"): + _run_on_complete(eiger, _missing_packets_status(), images_collected=9) + + +def test_eiger_missing_packet_tolerance_is_switchable_at_runtime(eiger_1_5m): + """The flag is reachable from the client, so a beamtime can change its mind.""" + eiger = eiger_1_5m + assert eiger.get_raise_on_missing_packets() is RAISE_ON_MISSING_PACKETS + + eiger.set_raise_on_missing_packets(True) + assert eiger.get_raise_on_missing_packets() is True + eiger.set_raise_on_missing_packets(False) + assert eiger.get_raise_on_missing_packets() is False + + for name in ( + "get_raise_on_missing_packets", + "set_raise_on_missing_packets", + "get_missing_packet_events", + ): + assert name in type(eiger).USER_ACCESS + + +def test_eiger_missing_packet_tolerance_is_overridable_per_deployment(mock_scan_info): + """deviceConfig's raise_on_missing_packets wins over the default, per detector.""" + dev = Eiger1_5M( + name="eiger_1_5m", + beam_center=(256, 256), + detector_distance=100.0, + raise_on_missing_packets=True, + ) + try: + assert dev.get_raise_on_missing_packets() is True + finally: + dev.destroy() + + +def test_eiger_subclasses_name_raise_on_missing_packets_for_the_device_server(): + """Same rule as readout_time: a deviceConfig key only reaches a device if the + class signature names it, so **kwargs alone would silently drop it.""" + for cls in (Eiger1_5M, Eiger9M): + assert "raise_on_missing_packets" in inspect.signature(cls).parameters From 376e5b1661cff2b939da8164ce8c9273fee1a853 Mon Sep 17 00:00:00 2001 From: wakonig_k Date: Tue, 8 Sep 2026 10:10:52 +0200 Subject: [PATCH 7/7] fix(camera): stop live mode before disconnecting to prevent polling --- .../allied_vision_cameras/allied_vision_aravis_camera.py | 2 ++ csaxs_bec/devices/ids_cameras/ids_camera.py | 2 ++ tests/tests_devices/test_allied_vision_aravis_camera.py | 1 + tests/tests_devices/test_ids_camera.py | 1 + 4 files changed, 6 insertions(+) diff --git a/csaxs_bec/devices/allied_vision_cameras/allied_vision_aravis_camera.py b/csaxs_bec/devices/allied_vision_cameras/allied_vision_aravis_camera.py index 5e41faf0..92a41c7f 100644 --- a/csaxs_bec/devices/allied_vision_cameras/allied_vision_aravis_camera.py +++ b/csaxs_bec/devices/allied_vision_cameras/allied_vision_aravis_camera.py @@ -247,6 +247,8 @@ class AlliedVisionAravisCamera(PSIDeviceBase): def on_destroy(self): """Clean up resources when the device is destroyed.""" + # Stop acquisition before disconnecting so the live thread cannot keep polling. + self.stop_live_mode() self.cam.on_disconnect() super().on_destroy() diff --git a/csaxs_bec/devices/ids_cameras/ids_camera.py b/csaxs_bec/devices/ids_cameras/ids_camera.py index e429c914..7f63241a 100644 --- a/csaxs_bec/devices/ids_cameras/ids_camera.py +++ b/csaxs_bec/devices/ids_cameras/ids_camera.py @@ -289,6 +289,8 @@ class IDSCamera(PSIDeviceBase): def on_destroy(self): """Clean up resources when the device is destroyed.""" + # Stop acquisition before disconnecting so the live thread cannot keep polling. + self.stop_live_mode() self.cam.on_disconnect() super().on_destroy() diff --git a/tests/tests_devices/test_allied_vision_aravis_camera.py b/tests/tests_devices/test_allied_vision_aravis_camera.py index b3134d06..d5951284 100644 --- a/tests/tests_devices/test_allied_vision_aravis_camera.py +++ b/tests/tests_devices/test_allied_vision_aravis_camera.py @@ -23,6 +23,7 @@ def allied_vision_camera(): camera.cam.width = 2 camera.cam.height = 2 yield camera + camera.stop_live_mode() def test_mask_setter_getter(allied_vision_camera): diff --git a/tests/tests_devices/test_ids_camera.py b/tests/tests_devices/test_ids_camera.py index 1c639b4b..2279e10e 100644 --- a/tests/tests_devices/test_ids_camera.py +++ b/tests/tests_devices/test_ids_camera.py @@ -28,6 +28,7 @@ def ids_camera(): camera.cam.cam.width.value = 2 camera.cam.cam.height.value = 2 yield camera + camera.stop_live_mode() def test_mask_setter_getter(ids_camera):