From 6f756e4461acfc06db4b9dcbf96a3ac53e95a94a Mon Sep 17 00:00:00 2001 From: Robert Tuck Date: Thu, 7 May 2026 15:29:58 +0100 Subject: [PATCH 1/6] move CONFIG_SERVER_URL environment variable name to a constant --- src/dodal/beamlines/i03.py | 5 ++++- src/dodal/common/beamlines/beamline_parameters.py | 3 +++ 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/src/dodal/beamlines/i03.py b/src/dodal/beamlines/i03.py index 64aa23b1c85..adcd92d4696 100644 --- a/src/dodal/beamlines/i03.py +++ b/src/dodal/beamlines/i03.py @@ -7,6 +7,7 @@ from ophyd_async.fastcs.panda import HDFPanda from yarl import URL +from dodal.common.beamlines.beamline_parameters import CONFIG_SERVER_URL_ENV_VAR from dodal.common.beamlines.beamline_utils import set_beamline as set_utils_beamline from dodal.common.beamlines.beamline_utils import set_config_client, set_path_provider from dodal.common.beamlines.commissioning_mode import set_commissioning_signal @@ -100,7 +101,9 @@ def path_provider() -> PathProvider: @devices.fixture @cache def config_client() -> ConfigClient: - config_server_url = getenv("CONFIG_SERVER_URL", DEFAULT_CONFIG_SERVER_ENDPOINT) + config_server_url = getenv( + CONFIG_SERVER_URL_ENV_VAR, DEFAULT_CONFIG_SERVER_ENDPOINT + ) client = ConfigClient(config_server_url) set_config_client(client) return client diff --git a/src/dodal/common/beamlines/beamline_parameters.py b/src/dodal/common/beamlines/beamline_parameters.py index f0645a85eb4..7f5ebd0f1a9 100644 --- a/src/dodal/common/beamlines/beamline_parameters.py +++ b/src/dodal/common/beamlines/beamline_parameters.py @@ -8,6 +8,9 @@ } +CONFIG_SERVER_URL_ENV_VAR = "CONFIG_SERVER_URL" + + def get_beamline_parameters(beamline: str) -> dict[str, Any]: """Loads the beamline parameters for a specified beamline from the config server. From 1b68b6af2fa0f541d7e76cefd18b78ebf5407f2d Mon Sep 17 00:00:00 2001 From: Robert Tuck Date: Thu, 7 May 2026 16:25:53 +0100 Subject: [PATCH 2/6] Fix eiger arming/disarming issues by serialising set operations --- src/dodal/devices/eiger.py | 20 +++++++++++++++++++- tests/devices/test_eiger.py | 3 +-- 2 files changed, 20 insertions(+), 3 deletions(-) diff --git a/src/dodal/devices/eiger.py b/src/dodal/devices/eiger.py index 1662cc86a15..4d649dccf41 100644 --- a/src/dodal/devices/eiger.py +++ b/src/dodal/devices/eiger.py @@ -225,18 +225,27 @@ def change_roi_mode(self, enable: bool) -> StatusBase: if enable else self.detector_params.detector_size_constants.det_size_pixels ) + LOGGER.info(f"Setting cam.roi_mode to {1 if enable else 0}") status = self.cam.roi_mode.set( 1 if enable else 0, timeout=self.timeouts.general_status_timeout ) + status.wait(timeout=self.timeouts.general_status_timeout) + LOGGER.debug(f"Setting image_height to {detector_dimensions.height}") status &= self.odin.file_writer.image_height.set( detector_dimensions.height, timeout=self.timeouts.general_status_timeout ) + status.wait(timeout=self.timeouts.general_status_timeout) + LOGGER.debug(f"Setting image_width to {detector_dimensions.width}") status &= self.odin.file_writer.image_width.set( detector_dimensions.width, timeout=self.timeouts.general_status_timeout ) + status.wait(timeout=self.timeouts.general_status_timeout) + LOGGER.debug(f"Setting num_row_chunks to {detector_dimensions.height}") status &= self.odin.file_writer.num_row_chunks.set( detector_dimensions.height, timeout=self.timeouts.general_status_timeout ) + status.wait(timeout=self.timeouts.general_status_timeout) + LOGGER.debug(f"Setting num_col_chunks to {detector_dimensions.width}") status &= self.odin.file_writer.num_col_chunks.set( detector_dimensions.width, timeout=self.timeouts.general_status_timeout ) @@ -249,21 +258,30 @@ def set_cam_pvs(self) -> AndStatus: self.detector_params.exposure_time_s, timeout=self.timeouts.general_status_timeout, ) + status.wait(timeout=self.timeouts.general_status_timeout) + LOGGER.debug("Setting cam acquire_period...") status &= self.cam.acquire_period.set( self.detector_params.exposure_time_s, timeout=self.timeouts.general_status_timeout, ) + status.wait(timeout=self.timeouts.general_status_timeout) + LOGGER.debug("Setting cam num_exposures...") status &= self.cam.num_exposures.set( 1, timeout=self.timeouts.general_status_timeout ) + status.wait(timeout=self.timeouts.general_status_timeout) + LOGGER.debug("Setting cam image_mode...") status &= self.cam.image_mode.set( self.cam.ImageMode.MULTIPLE, # pyright: ignore[reportAttributeAccessIssue] timeout=self.timeouts.general_status_timeout, ) + status.wait(timeout=self.timeouts.general_status_timeout) + LOGGER.debug("Setting cam trigger_mode...") status &= self.cam.trigger_mode.set( InternalEigerTriggerMode.EXTERNAL_SERIES.value, timeout=self.timeouts.general_status_timeout, ) + status.wait(timeout=self.timeouts.general_status_timeout) return status def set_odin_number_of_frame_chunks(self) -> Status: @@ -379,7 +397,7 @@ def set_num_triggers_and_captures(self) -> StatusBase: def _wait_for_odin_status(self) -> StatusBase: self.forward_bit_depth_to_filewriter() - await_value(self.odin.meta.active, 1).wait(self.timeouts.general_status_timeout) + await_value(self.odin.meta.active, 1).wait(60) status = self.odin.file_writer.capture.set( 1, timeout=self.timeouts.general_status_timeout diff --git a/tests/devices/test_eiger.py b/tests/devices/test_eiger.py index e42dd11f9d3..438f870768d 100644 --- a/tests/devices/test_eiger.py +++ b/tests/devices/test_eiger.py @@ -293,7 +293,6 @@ def test_unsuccessful_true_roi_mode_change_results_in_callback_error( ] with pytest.raises(StatusError): run_functions_without_blocking(unwrapped_funcs).wait() - LOGGER.error.assert_called() @patch("ophyd.status.Status.__and__") @@ -355,7 +354,7 @@ def test_stage_runs_successfully( fake_eiger.stage() fake_eiger.arming_status.wait(1) # This should complete long before 1s # One log message kicking off arming, then one for each of the 13 arming stages - assert len(caplog.messages) == 14 + assert len(caplog.messages) == 18 def test_given_stale_parameters_goes_high_before_callbacks_then_stale_parameters_waited_on( From 96660eaf37cd69acb4488861818a141f76f85a00 Mon Sep 17 00:00:00 2001 From: Robert Tuck Date: Tue, 12 May 2026 14:43:04 +0100 Subject: [PATCH 3/6] Update with changes from PR comments --- src/dodal/devices/eiger.py | 51 ++++++++++++++++---------------------- 1 file changed, 22 insertions(+), 29 deletions(-) diff --git a/src/dodal/devices/eiger.py b/src/dodal/devices/eiger.py index 4d649dccf41..c574903c4fa 100644 --- a/src/dodal/devices/eiger.py +++ b/src/dodal/devices/eiger.py @@ -24,6 +24,7 @@ class EigerTimeouts: all_frames_timeout: int = 120 arming_timeout: int = 60 odin_stop_timeout: int = 30 + odin_meta_active_timeout: int = 60 class InternalEigerTriggerMode(Enum): @@ -226,27 +227,23 @@ def change_roi_mode(self, enable: bool) -> StatusBase: else self.detector_params.detector_size_constants.det_size_pixels ) LOGGER.info(f"Setting cam.roi_mode to {1 if enable else 0}") - status = self.cam.roi_mode.set( + self.cam.roi_mode.set( 1 if enable else 0, timeout=self.timeouts.general_status_timeout - ) - status.wait(timeout=self.timeouts.general_status_timeout) + ).wait(timeout=self.timeouts.general_status_timeout) LOGGER.debug(f"Setting image_height to {detector_dimensions.height}") - status &= self.odin.file_writer.image_height.set( + self.odin.file_writer.image_height.set( detector_dimensions.height, timeout=self.timeouts.general_status_timeout - ) - status.wait(timeout=self.timeouts.general_status_timeout) + ).wait(timeout=self.timeouts.general_status_timeout) LOGGER.debug(f"Setting image_width to {detector_dimensions.width}") - status &= self.odin.file_writer.image_width.set( + self.odin.file_writer.image_width.set( detector_dimensions.width, timeout=self.timeouts.general_status_timeout - ) - status.wait(timeout=self.timeouts.general_status_timeout) + ).wait(timeout=self.timeouts.general_status_timeout) LOGGER.debug(f"Setting num_row_chunks to {detector_dimensions.height}") - status &= self.odin.file_writer.num_row_chunks.set( + self.odin.file_writer.num_row_chunks.set( detector_dimensions.height, timeout=self.timeouts.general_status_timeout - ) - status.wait(timeout=self.timeouts.general_status_timeout) + ).wait(timeout=self.timeouts.general_status_timeout) LOGGER.debug(f"Setting num_col_chunks to {detector_dimensions.width}") - status &= self.odin.file_writer.num_col_chunks.set( + status = self.odin.file_writer.num_col_chunks.set( detector_dimensions.width, timeout=self.timeouts.general_status_timeout ) return status @@ -254,35 +251,29 @@ def change_roi_mode(self, enable: bool) -> StatusBase: def set_cam_pvs(self) -> AndStatus: LOGGER.info("Eiger arming: Setting eiger camera PVs...") assert self.detector_params is not None - status = self.cam.acquire_time.set( + self.cam.acquire_time.set( self.detector_params.exposure_time_s, timeout=self.timeouts.general_status_timeout, - ) - status.wait(timeout=self.timeouts.general_status_timeout) + ).wait(timeout=self.timeouts.general_status_timeout) LOGGER.debug("Setting cam acquire_period...") - status &= self.cam.acquire_period.set( + self.cam.acquire_period.set( self.detector_params.exposure_time_s, timeout=self.timeouts.general_status_timeout, - ) - status.wait(timeout=self.timeouts.general_status_timeout) + ).wait(timeout=self.timeouts.general_status_timeout) LOGGER.debug("Setting cam num_exposures...") - status &= self.cam.num_exposures.set( + self.cam.num_exposures.set( 1, timeout=self.timeouts.general_status_timeout - ) - status.wait(timeout=self.timeouts.general_status_timeout) + ).wait(timeout=self.timeouts.general_status_timeout) LOGGER.debug("Setting cam image_mode...") - status &= self.cam.image_mode.set( + self.cam.image_mode.set( self.cam.ImageMode.MULTIPLE, # pyright: ignore[reportAttributeAccessIssue] timeout=self.timeouts.general_status_timeout, - ) - status.wait(timeout=self.timeouts.general_status_timeout) + ).wait(timeout=self.timeouts.general_status_timeout) LOGGER.debug("Setting cam trigger_mode...") - status &= self.cam.trigger_mode.set( + return self.cam.trigger_mode.set( InternalEigerTriggerMode.EXTERNAL_SERIES.value, timeout=self.timeouts.general_status_timeout, ) - status.wait(timeout=self.timeouts.general_status_timeout) - return status def set_odin_number_of_frame_chunks(self) -> Status: LOGGER.info("Eiger arming: Setting odin number of frames chunks...") @@ -397,7 +388,9 @@ def set_num_triggers_and_captures(self) -> StatusBase: def _wait_for_odin_status(self) -> StatusBase: self.forward_bit_depth_to_filewriter() - await_value(self.odin.meta.active, 1).wait(60) + await_value(self.odin.meta.active, 1).wait( + self.timeouts.odin_meta_active_timeout + ) status = self.odin.file_writer.capture.set( 1, timeout=self.timeouts.general_status_timeout From 710c63ac14352ac33833d6fa433eb832998c044b Mon Sep 17 00:00:00 2001 From: Robert Tuck Date: Tue, 12 May 2026 14:44:36 +0100 Subject: [PATCH 4/6] Update comment --- tests/devices/test_eiger.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/devices/test_eiger.py b/tests/devices/test_eiger.py index 438f870768d..a24fe8c5454 100644 --- a/tests/devices/test_eiger.py +++ b/tests/devices/test_eiger.py @@ -353,7 +353,7 @@ def test_stage_runs_successfully( set_up_eiger_to_stage_happily(fake_eiger) fake_eiger.stage() fake_eiger.arming_status.wait(1) # This should complete long before 1s - # One log message kicking off arming, then one for each of the 13 arming stages + # One log message kicking off arming, then one for each of the 17 arming stages assert len(caplog.messages) == 18 From f94bdef1ff78d7d0ecb979879e767fa73de5cc3e Mon Sep 17 00:00:00 2001 From: Robert Tuck Date: Tue, 12 May 2026 14:46:27 +0100 Subject: [PATCH 5/6] Make type-checking happy --- src/dodal/devices/eiger.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/dodal/devices/eiger.py b/src/dodal/devices/eiger.py index c574903c4fa..9a4b2899a8b 100644 --- a/src/dodal/devices/eiger.py +++ b/src/dodal/devices/eiger.py @@ -5,7 +5,7 @@ from ophyd import Component, Device, EpicsSignalRO, Signal from ophyd.areadetector.cam import EigerDetectorCam from ophyd.signal import AttributeSignal -from ophyd.status import AndStatus, Status, StatusBase, SubscriptionStatus +from ophyd.status import Status, StatusBase, SubscriptionStatus from dodal.devices.detector import DetectorParams, TriggerMode from dodal.devices.eiger_odin import EigerOdin @@ -248,7 +248,7 @@ def change_roi_mode(self, enable: bool) -> StatusBase: ) return status - def set_cam_pvs(self) -> AndStatus: + def set_cam_pvs(self) -> StatusBase: LOGGER.info("Eiger arming: Setting eiger camera PVs...") assert self.detector_params is not None self.cam.acquire_time.set( From 57a35f5c7a9aab76177f2538480c956280192531 Mon Sep 17 00:00:00 2001 From: Robert Tuck Date: Wed, 13 May 2026 10:16:58 +0100 Subject: [PATCH 6/6] Fix broken test --- tests/devices/test_eiger.py | 55 +++++++++++++------------------------ 1 file changed, 19 insertions(+), 36 deletions(-) diff --git a/tests/devices/test_eiger.py b/tests/devices/test_eiger.py index a24fe8c5454..aa644c7988e 100644 --- a/tests/devices/test_eiger.py +++ b/tests/devices/test_eiger.py @@ -275,43 +275,26 @@ def test_change_roi_mode_sets_cam_roi_mode_correctly( ) -# Also tests transition from change ROI to set_detector_threshold -@patch("ophyd.status.Status.__and__") -def test_unsuccessful_true_roi_mode_change_results_in_callback_error( - mock_and, fake_eiger: EigerDetector -): - bad_status = Status() - bad_status.set_exception(StatusError("Failed setting ROI mode True")) - mock_and.return_value = bad_status - LOGGER.error = MagicMock() - - unwrapped_funcs = [ - lambda: fake_eiger.change_roi_mode(enable=True), - lambda: fake_eiger.set_detector_threshold( - energy=fake_eiger.detector_params.expected_energy_ev - ), - ] - with pytest.raises(StatusError): - run_functions_without_blocking(unwrapped_funcs).wait() - - -@patch("ophyd.status.Status.__and__") -def test_unsuccessful_false_roi_mode_change_results_in_callback_error( - mock_and, fake_eiger: EigerDetector +def test_unsuccessful_roi_mode_change_results_in_callback_error( + fake_eiger: EigerDetector, ): - bad_status = Status() - bad_status.set_exception(StatusError("Failed setting ROI mode False")) - mock_and.return_value = bad_status - LOGGER.error = MagicMock() - - unwrapped_funcs = [ - lambda: fake_eiger.change_roi_mode(enable=False), - lambda: fake_eiger.set_detector_threshold( - energy=fake_eiger.detector_params.expected_energy_ev - ), - ] - with pytest.raises(StatusError): - run_functions_without_blocking(unwrapped_funcs).wait() + for signal in [ + fake_eiger.odin.file_writer.num_col_chunks, + fake_eiger.odin.file_writer.num_row_chunks, + fake_eiger.odin.file_writer.image_width, + fake_eiger.odin.file_writer.image_height, + fake_eiger.cam.roi_mode, + ]: + bad_status = Status() + bad_status.set_exception(StatusError(f"Failed setting {signal.name}")) + signal.set = MagicMock(return_value=bad_status) + LOGGER.error = MagicMock() + + unwrapped_funcs = [ + lambda: fake_eiger.change_roi_mode(enable=True), + ] + with pytest.raises(StatusError, match=f"Failed setting {signal.name}"): + run_functions_without_blocking(unwrapped_funcs).wait() @patch("dodal.devices.eiger.EigerOdin.check_and_wait_for_odin_state")