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 2 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
20 changes: 19 additions & 1 deletion src/dodal/devices/eiger.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
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.

Could: If we're going to wait on each individual status then we don't need to keep the status around, e.g.

Suggested change
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)
self.cam.roi_mode.set(1 if enable else 0, timeout=self.timeouts.general_status_timeout).wait(timeout=self.timeouts.general_status_timeout)

Copy link
Copy Markdown
Contributor

@fajinyuan fajinyuan May 11, 2026

Choose a reason for hiding this comment

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

Sorry for my question if it misses the point here. Is it possibl to improve IOC PV so the set call can rely on callback instead of wait on status, or the status already embedded callback inside?

Copy link
Copy Markdown
Contributor

@DominicOram DominicOram May 11, 2026

Choose a reason for hiding this comment

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

Yes, the status is the callback from the IOC

LOGGER.debug(f"Setting image_height to {detector_dimensions.height}")
status &= self.odin.file_writer.image_height.set(
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: The last status has already been waited on so no need to do &= here, = will do (or just don't keep it around as above)

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
)
Expand All @@ -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
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: The only reason this returns the status is so that we can wait on it, if we've already waited on it then it seems a bit pointless

Copy link
Copy Markdown
Contributor Author

@rtuck99 rtuck99 May 12, 2026

Choose a reason for hiding this comment

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

this all goes into the list of run_functions_without_blocking() so to follow convention we return the status.


def set_odin_number_of_frame_chunks(self) -> Status:
Expand Down Expand Up @@ -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)
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: Can we pull this into a const?


status = self.odin.file_writer.capture.set(
1, timeout=self.timeouts.general_status_timeout
Expand Down
3 changes: 1 addition & 2 deletions tests/devices/test_eiger.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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__")
Expand Down Expand Up @@ -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
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