Skip to content

Fix misc eiger arming issues #2052

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
rtuck99 wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Fix misc eiger arming issues #2052

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
6f756e4
move CONFIG_SERVER_URL environment variable name to a constant
rtuck99 May 7, 2026
1b68b6a
Fix eiger arming/disarming issues by serialising set operations
rtuck99 May 7, 2026
96660ea
Update with changes from PR comments
rtuck99 May 12, 2026
710c63a
Update comment
rtuck99 May 12, 2026
f94bdef
Make type-checking happy
rtuck99 May 12, 2026
57a35f5
Fix broken test
rtuck99 May 13, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion src/dodal/beamlines/i03.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
3 changes: 3 additions & 0 deletions src/dodal/common/beamlines/beamline_parameters.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

Expand Down
55 changes: 33 additions & 22 deletions src/dodal/devices/eiger.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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):
Expand Down Expand Up @@ -225,46 +226,54 @@ def change_roi_mode(self, enable: bool) -> StatusBase:
if enable
else self.detector_params.detector_size_constants.det_size_pixels
)
status = self.cam.roi_mode.set(
LOGGER.info(f"Setting cam.roi_mode to {1 if enable else 0}")
self.cam.roi_mode.set(
1 if enable else 0, timeout=self.timeouts.general_status_timeout
)
status &= self.odin.file_writer.image_height.set(
).wait(timeout=self.timeouts.general_status_timeout)
LOGGER.debug(f"Setting image_height to {detector_dimensions.height}")
self.odin.file_writer.image_height.set(
detector_dimensions.height, timeout=self.timeouts.general_status_timeout
)
status &= self.odin.file_writer.image_width.set(
).wait(timeout=self.timeouts.general_status_timeout)
LOGGER.debug(f"Setting image_width to {detector_dimensions.width}")
self.odin.file_writer.image_width.set(
detector_dimensions.width, timeout=self.timeouts.general_status_timeout
)
status &= self.odin.file_writer.num_row_chunks.set(
).wait(timeout=self.timeouts.general_status_timeout)
LOGGER.debug(f"Setting num_row_chunks to {detector_dimensions.height}")
self.odin.file_writer.num_row_chunks.set(
detector_dimensions.height, timeout=self.timeouts.general_status_timeout
)
status &= self.odin.file_writer.num_col_chunks.set(
).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
)
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
status = self.cam.acquire_time.set(
self.cam.acquire_time.set(
self.detector_params.exposure_time_s,
timeout=self.timeouts.general_status_timeout,
)
status &= self.cam.acquire_period.set(
).wait(timeout=self.timeouts.general_status_timeout)
LOGGER.debug("Setting cam acquire_period...")
self.cam.acquire_period.set(
self.detector_params.exposure_time_s,
timeout=self.timeouts.general_status_timeout,
)
status &= self.cam.num_exposures.set(
).wait(timeout=self.timeouts.general_status_timeout)
LOGGER.debug("Setting cam num_exposures...")
self.cam.num_exposures.set(
1, timeout=self.timeouts.general_status_timeout
)
status &= self.cam.image_mode.set(
).wait(timeout=self.timeouts.general_status_timeout)
LOGGER.debug("Setting cam image_mode...")
self.cam.image_mode.set(
self.cam.ImageMode.MULTIPLE, # pyright: ignore[reportAttributeAccessIssue]
timeout=self.timeouts.general_status_timeout,
)
status &= self.cam.trigger_mode.set(
).wait(timeout=self.timeouts.general_status_timeout)
LOGGER.debug("Setting cam trigger_mode...")
return self.cam.trigger_mode.set(
InternalEigerTriggerMode.EXTERNAL_SERIES.value,
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...")
Expand Down Expand Up @@ -379,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(self.timeouts.general_status_timeout)
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
Expand Down
60 changes: 21 additions & 39 deletions tests/devices/test_eiger.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -275,44 +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()
LOGGER.error.assert_called()


@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")
Expand Down Expand Up @@ -354,8 +336,8 @@ 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
assert len(caplog.messages) == 14
# One log message kicking off arming, then one for each of the 17 arming stages
assert len(caplog.messages) == 18
Copy link
Copy Markdown
Contributor

@DominicOram DominicOram May 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should: This no longer matches the comment above



def test_given_stale_parameters_goes_high_before_callbacks_then_stale_parameters_waited_on(
Expand Down
Loading