From caf0f2980ff991d84c4199e561d929e1ab04f0ee Mon Sep 17 00:00:00 2001 From: Robert Tuck Date: Wed, 14 Jan 2026 10:53:53 +0000 Subject: [PATCH 01/16] Initial changes to aperture_scatterguard and scintillator to support configuration --- src/dodal/beamlines/i03.py | 9 +- src/dodal/beamlines/i04.py | 7 +- src/dodal/devices/aperturescatterguard.py | 44 +++++++--- src/dodal/devices/scintillator.py | 26 +++++- tests/devices/conftest.py | 14 ++-- tests/devices/test_aperture_scatterguard.py | 93 ++++++++++++--------- tests/devices/test_scintillator.py | 18 ++++ 7 files changed, 147 insertions(+), 64 deletions(-) diff --git a/src/dodal/beamlines/i03.py b/src/dodal/beamlines/i03.py index 3987b4563d..cb4ba71ac7 100644 --- a/src/dodal/beamlines/i03.py +++ b/src/dodal/beamlines/i03.py @@ -12,7 +12,7 @@ from dodal.devices.aperturescatterguard import ( AperturePosition, ApertureScatterguard, - load_positions_from_beamline_parameters, + load_configuration, ) from dodal.devices.attenuator.attenuator import BinaryFilterAttenuator from dodal.devices.backlight import Backlight @@ -99,7 +99,7 @@ def aperture_scatterguard() -> ApertureScatterguard: return ApertureScatterguard( aperture_prefix=f"{PREFIX.beamline_prefix}-MO-MAPT-01:", scatterguard_prefix=f"{PREFIX.beamline_prefix}-MO-SCAT-01:", - loaded_positions=load_positions_from_beamline_parameters(params), + config=load_configuration(params), tolerances=AperturePosition.tolerances_from_gda_params(params), ) @@ -342,10 +342,13 @@ def fluorescence_det_motion() -> FluorescenceDetector: @devices.factory() -def scintillator(aperture_scatterguard: ApertureScatterguard) -> Scintillator: +def scintillator( + aperture_scatterguard: ApertureScatterguard, beamstop: Beamstop +) -> Scintillator: return Scintillator( f"{PREFIX.beamline_prefix}-MO-SCIN-01:", Reference(aperture_scatterguard), + Reference(beamstop), get_beamline_parameters(), ) diff --git a/src/dodal/beamlines/i04.py b/src/dodal/beamlines/i04.py index 18cc611344..1a82cf3b80 100644 --- a/src/dodal/beamlines/i04.py +++ b/src/dodal/beamlines/i04.py @@ -6,7 +6,7 @@ from dodal.devices.aperturescatterguard import ( AperturePosition, ApertureScatterguard, - load_positions_from_beamline_parameters, + load_configuration, ) from dodal.devices.attenuator.attenuator import BinaryFilterAttenuator from dodal.devices.backlight import Backlight @@ -152,7 +152,7 @@ def aperture_scatterguard() -> ApertureScatterguard: return ApertureScatterguard( aperture_prefix=f"{PREFIX.beamline_prefix}-MO-MAPT-01:", scatterguard_prefix=f"{PREFIX.beamline_prefix}-MO-SCAT-01:", - loaded_positions=load_positions_from_beamline_parameters(params), + config=load_configuration(params), tolerances=AperturePosition.tolerances_from_gda_params(params), ) @@ -277,10 +277,11 @@ def pin_tip_detection() -> PinTipDetection: @devices.factory() -def scintillator(aperture_scatterguard: ApertureScatterguard) -> Scintillator: +def scintillator(aperture_scatterguard: ApertureScatterguard, beamstop: Beamstop) -> Scintillator: return Scintillator( f"{PREFIX.beamline_prefix}-MO-SCIN-01:", Reference(aperture_scatterguard), + Reference(beamstop), get_beamline_parameters(), ) diff --git a/src/dodal/devices/aperturescatterguard.py b/src/dodal/devices/aperturescatterguard.py index 403000fb7d..d59e5eca19 100644 --- a/src/dodal/devices/aperturescatterguard.py +++ b/src/dodal/devices/aperturescatterguard.py @@ -1,6 +1,7 @@ from __future__ import annotations import asyncio +import dataclasses from math import inf from bluesky.protocols import Preparable @@ -12,6 +13,7 @@ derived_signal_r, derived_signal_rw, ) +from ophyd_async.epics.motor import Motor from pydantic import BaseModel, Field from dodal.common.beamlines.beamline_parameters import GDABeamlineParameters @@ -31,6 +33,7 @@ class _GDAParamApertureValue(StrictEnum): MEDIUM = "MEDIUM_APERTURE" LARGE = "LARGE_APERTURE" MANUAL_LOAD = "MANUAL_LOAD" + SCIN_MOVE = "SCIN_MOVE" class AperturePosition(BaseModel): @@ -108,10 +111,18 @@ def __str__(self): return self.name.capitalize() -def load_positions_from_beamline_parameters( +@dataclasses.dataclass +class ApertureScatterguardConfiguration: + aperture_positions: dict[ApertureValue, AperturePosition] + # The SCIN MOVE position is defined only for x-axis + scintillator_move_miniap_x: float + scintillator_move_sg_x: float + + +def load_configuration( params: GDABeamlineParameters, -) -> dict[ApertureValue, AperturePosition]: - return { +) -> ApertureScatterguardConfiguration: + positions = { ApertureValue.OUT_OF_BEAM: AperturePosition.from_gda_params( _GDAParamApertureValue.ROBOT_LOAD, inf, params ), @@ -128,6 +139,11 @@ def load_positions_from_beamline_parameters( _GDAParamApertureValue.MANUAL_LOAD, inf, params ), } + return ApertureScatterguardConfiguration( + aperture_positions=positions, + scintillator_move_miniap_x=params["miniap_x_SCIN_MOVE"], + scintillator_move_sg_x=params["sg_x_SCIN_MOVE"], + ) class ApertureScatterguard(StandardReadable, Preparable): @@ -166,13 +182,13 @@ def __init__( self, aperture_prefix: str, scatterguard_prefix: str, - loaded_positions: dict[ApertureValue, AperturePosition], + config: ApertureScatterguardConfiguration, tolerances: AperturePosition, name: str = "", ) -> None: self.aperture = Aperture(aperture_prefix) self.scatterguard = XYStage(scatterguard_prefix) - self._loaded_positions = loaded_positions + self._config = config self._tolerances = tolerances with self.add_children_as_readables(StandardReadableFormat.HINTED_SIGNAL): self.selected_aperture = derived_signal_rw( @@ -208,7 +224,7 @@ async def _unpark(self, position_to_move_to: ApertureValue): """When the aperture is parked it is under the collimation table. It needs to be moved out from under the table before it is moved up to beam height. """ - position = self._loaded_positions[position_to_move_to] + position = self._config.aperture_positions[position_to_move_to] await self.aperture.z.set(position.aperture_z) async def _set_current_aperture_position(self, value: ApertureValue) -> None: @@ -217,7 +233,7 @@ async def _set_current_aperture_position(self, value: ApertureValue) -> None: "Currently not able to park aperture/scatterguard, see https://github.com/DiamondLightSource/mx-bluesky/issues/1197" ) - position = self._loaded_positions[value] + position = self._config.aperture_positions[value] current_ap_y = await self.aperture.y.user_readback.get_value() current_ap_z = await self.aperture.z.user_readback.get_value() @@ -263,13 +279,13 @@ async def _check_safe_to_move(self, expected_z_position: float): ) def _get_current_diameter(self, current_aperture: ApertureValue) -> float: - return self._loaded_positions[current_aperture].diameter + return self._config.aperture_positions[current_aperture].diameter def _is_in_position( self, position: ApertureValue, current_ap_y: float, current_ap_z: float ) -> bool: - position_y = self._loaded_positions[position].aperture_y - position_z = self._loaded_positions[position].aperture_z + position_y = self._config.aperture_positions[position].aperture_y + position_z = self._config.aperture_positions[position].aperture_z y_matches = abs(current_ap_y - position_y) <= self._tolerances.aperture_y z_matches = abs(current_ap_z - position_z) <= self._tolerances.aperture_z return y_matches and z_matches @@ -362,7 +378,7 @@ async def prepare(self, value: ApertureValue): current_z = await self.aperture.z.user_readback.get_value() if self._is_in_position(ApertureValue.OUT_OF_BEAM, current_y, current_z): aperture_x, _, aperture_z, scatterguard_x, scatterguard_y = ( - self._loaded_positions[value].values + self._config.aperture_positions[value].values ) await asyncio.gather( @@ -373,3 +389,9 @@ async def prepare(self, value: ApertureValue): ) else: await self.selected_aperture.set(value) + + def get_scin_move_position(self) -> dict[Motor, float]: + return { + self.aperture.x: self._config.scintillator_move_miniap_x, + self.scatterguard.x: self._config.scintillator_move_sg_x, + } diff --git a/src/dodal/devices/scintillator.py b/src/dodal/devices/scintillator.py index bfa4506b1e..dbf56d1d77 100644 --- a/src/dodal/devices/scintillator.py +++ b/src/dodal/devices/scintillator.py @@ -5,6 +5,7 @@ from dodal.common.beamlines.beamline_parameters import GDABeamlineParameters from dodal.devices.aperturescatterguard import ApertureScatterguard, ApertureValue +from dodal.devices.mx_phase1.beamstop import Beamstop class InOut(StrictEnum): @@ -29,6 +30,7 @@ def __init__( self, prefix: str, aperture_scatterguard: Reference[ApertureScatterguard], + beamstop: Reference[Beamstop], beamline_parameters: GDABeamlineParameters, name: str = "", ): @@ -43,6 +45,7 @@ def __init__( ) self._aperture_scatterguard = aperture_scatterguard + self._beamstop = beamstop self._scintillator_out_yz_mm = [ float(beamline_parameters[f"scin_{axis}_SCIN_OUT"]) for axis in ("y", "z") ] @@ -94,8 +97,13 @@ async def _set_selected_position(self, position: InOut) -> None: if self._get_selected_position(current_y, current_z) == InOut.OUT: return await self._check_aperture_parked() - await self.y_mm.set(self._scintillator_out_yz_mm[0]) - await self.z_mm.set(self._scintillator_out_yz_mm[1]) + + async def move_scin_out(): + await self.y_mm.set(self._scintillator_out_yz_mm[0]) + await self.z_mm.set(self._scintillator_out_yz_mm[1]) + + await self.do_with_ap_sg_in_safe_pos(move_scin_out) + case InOut.IN: current_y = await self.y_mm.user_readback.get_value() current_z = await self.z_mm.user_readback.get_value() @@ -106,3 +114,17 @@ async def _set_selected_position(self, position: InOut) -> None: await self.y_mm.set(self._scintillator_in_yz_mm[0]) case _: raise ValueError(f"Cannot set scintillator to position {position}") + + async def do_with_ap_sg_in_safe_pos(self, func): + scin_move_positions = self._aperture_scatterguard().get_scin_move_position() + saved_positions: dict[Motor, float] = { + motor: await motor.user_readback.get_value() + for motor in scin_move_positions + } + for motor, pos in scin_move_positions.items(): + await motor.set(pos) + + await func() + # If func() fails then do not restore motors back to avoid potential collision + for motor, pos in saved_positions.items(): + await motor.set(pos) diff --git a/tests/devices/conftest.py b/tests/devices/conftest.py index 33c2a24121..9e7b0e631d 100644 --- a/tests/devices/conftest.py +++ b/tests/devices/conftest.py @@ -7,14 +7,14 @@ from dodal.devices.aperturescatterguard import ( AperturePosition, ApertureScatterguard, - ApertureValue, - load_positions_from_beamline_parameters, + ApertureScatterguardConfiguration, + load_configuration, ) @pytest.fixture -def aperture_positions() -> dict[ApertureValue, AperturePosition]: - return load_positions_from_beamline_parameters( +def ap_sg_configuration() -> ApertureScatterguardConfiguration: + return load_configuration( GDABeamlineParameters( params={ "miniap_x_LARGE_APERTURE": 2.389, @@ -42,6 +42,8 @@ def aperture_positions() -> dict[ApertureValue, AperturePosition]: "miniap_z_MANUAL_LOAD": -10.0, "sg_x_MANUAL_LOAD": -4.7, "sg_y_MANUAL_LOAD": 1.8, + "miniap_x_SCIN_MOVE": -4.91, + "sg_x_SCIN_MOVE": -4.75, } ) ) @@ -64,7 +66,7 @@ def aperture_tolerances(): @pytest.fixture async def ap_sg( - aperture_positions: dict[ApertureValue, AperturePosition], + ap_sg_configuration: ApertureScatterguardConfiguration, aperture_tolerances: AperturePosition, ) -> ApertureScatterguard: async with init_devices(mock=True): @@ -72,7 +74,7 @@ async def ap_sg( aperture_prefix="-MO-MAPT-01:", scatterguard_prefix="-MO-SCAT-01:", name="test_ap_sg", - loaded_positions=aperture_positions, + config=ap_sg_configuration, tolerances=aperture_tolerances, ) return ap_sg diff --git a/tests/devices/test_aperture_scatterguard.py b/tests/devices/test_aperture_scatterguard.py index 985ac20827..51bee788c1 100644 --- a/tests/devices/test_aperture_scatterguard.py +++ b/tests/devices/test_aperture_scatterguard.py @@ -16,6 +16,7 @@ from dodal.devices.aperturescatterguard import ( AperturePosition, ApertureScatterguard, + ApertureScatterguardConfiguration, ApertureValue, InvalidApertureMoveError, ) @@ -25,9 +26,11 @@ @pytest.fixture async def aperture_in_medium_pos( ap_sg: ApertureScatterguard, - aperture_positions: dict[ApertureValue, AperturePosition], + ap_sg_configuration: ApertureScatterguardConfiguration, ) -> AsyncGenerator[ApertureScatterguard, None]: - await set_to_position(ap_sg, aperture_positions[ApertureValue.MEDIUM]) + await set_to_position( + ap_sg, ap_sg_configuration.aperture_positions[ApertureValue.MEDIUM] + ) set_mock_value(ap_sg.aperture.medium, 1) @@ -194,7 +197,7 @@ def set_underlying_motors(ap_sg: ApertureScatterguard, position: AperturePositio ) async def test_aperture_positions( ap_sg: ApertureScatterguard, - aperture_positions: dict[ApertureValue, AperturePosition], + ap_sg_configuration: ApertureScatterguardConfiguration, read_pv: str, aperture: ApertureValue, ): @@ -203,19 +206,19 @@ async def test_aperture_positions( assert isinstance(reading, dict) assert ( reading[f"{ap_sg.name}-diameter"]["value"] - == aperture_positions[aperture].diameter + == ap_sg_configuration.aperture_positions[aperture].diameter ) assert reading[f"{ap_sg.name}-selected_aperture"]["value"] == aperture async def test_aperture_positions_robot_load( ap_sg: ApertureScatterguard, - aperture_positions: dict[ApertureValue, AperturePosition], + ap_sg_configuration: ApertureScatterguardConfiguration, ): set_mock_value(ap_sg.aperture.large, 0) set_mock_value(ap_sg.aperture.medium, 0) set_mock_value(ap_sg.aperture.small, 0) - robot_load = aperture_positions[ApertureValue.OUT_OF_BEAM] + robot_load = ap_sg_configuration.aperture_positions[ApertureValue.OUT_OF_BEAM] await ap_sg.aperture.y.set(robot_load.aperture_y) await ap_sg.aperture.z.set(robot_load.aperture_z) reading = await ap_sg.read() @@ -228,9 +231,9 @@ async def test_aperture_positions_robot_load( async def test_aperture_positions_robot_load_within_tolerance( ap_sg: ApertureScatterguard, - aperture_positions: dict[ApertureValue, AperturePosition], + ap_sg_configuration: ApertureScatterguardConfiguration, ): - robot_load = aperture_positions[ApertureValue.OUT_OF_BEAM] + robot_load = ap_sg_configuration.aperture_positions[ApertureValue.OUT_OF_BEAM] robot_load_ap_y = robot_load.aperture_y tolerance = ap_sg._tolerances.aperture_y - 0.001 set_mock_value(ap_sg.aperture.large, 0) @@ -248,9 +251,9 @@ async def test_aperture_positions_robot_load_within_tolerance( async def test_aperture_positions_robot_load_outside_tolerance( ap_sg: ApertureScatterguard, - aperture_positions: dict[ApertureValue, AperturePosition], + ap_sg_configuration: ApertureScatterguardConfiguration, ): - robot_load = aperture_positions[ApertureValue.OUT_OF_BEAM] + robot_load = ap_sg_configuration.aperture_positions[ApertureValue.OUT_OF_BEAM] robot_load_ap_y = robot_load.aperture_y tolerance = ap_sg._tolerances.aperture_y + 0.01 set_mock_value(ap_sg.aperture.large, 0) @@ -264,12 +267,12 @@ async def test_aperture_positions_robot_load_outside_tolerance( async def test_aperture_positions_parked( ap_sg: ApertureScatterguard, - aperture_positions: dict[ApertureValue, AperturePosition], + ap_sg_configuration: ApertureScatterguardConfiguration, ): set_mock_value(ap_sg.aperture.large, 0) set_mock_value(ap_sg.aperture.medium, 0) set_mock_value(ap_sg.aperture.small, 0) - parked = aperture_positions[ApertureValue.PARKED] + parked = ap_sg_configuration.aperture_positions[ApertureValue.PARKED] await ap_sg.aperture.y.set(parked.aperture_y) await ap_sg.aperture.z.set(parked.aperture_z) reading = await ap_sg.read() @@ -280,9 +283,9 @@ async def test_aperture_positions_parked( async def test_aperture_positions_parked_within_tolerance( ap_sg: ApertureScatterguard, - aperture_positions: dict[ApertureValue, AperturePosition], + ap_sg_configuration: ApertureScatterguardConfiguration, ): - parked = aperture_positions[ApertureValue.PARKED] + parked = ap_sg_configuration.aperture_positions[ApertureValue.PARKED] parked_z = parked.aperture_z tolerance = ap_sg._tolerances.aperture_z - 0.01 set_mock_value(ap_sg.aperture.large, 0) @@ -298,9 +301,9 @@ async def test_aperture_positions_parked_within_tolerance( async def test_aperture_positions_parked_outside_tolerance( ap_sg: ApertureScatterguard, - aperture_positions: dict[ApertureValue, AperturePosition], + ap_sg_configuration: ApertureScatterguardConfiguration, ): - parked = aperture_positions[ApertureValue.PARKED] + parked = ap_sg_configuration.aperture_positions[ApertureValue.PARKED] parked_z = parked.aperture_z tolerance = ap_sg._tolerances.aperture_z + 0.01 set_mock_value(ap_sg.aperture.large, 0) @@ -325,14 +328,14 @@ async def test_aperture_positions_unsafe( async def test_given_aperture_not_set_through_device_but_motors_in_position_when_device_read_then_position_returned( aperture_in_medium_pos: ApertureScatterguard, - aperture_positions: dict[ApertureValue, AperturePosition], + ap_sg_configuration: ApertureScatterguardConfiguration, ): reading = await aperture_in_medium_pos.read() assert isinstance(reading, dict) _assert_position_in_reading( reading, ApertureValue.MEDIUM, - aperture_positions[ApertureValue.MEDIUM], + ap_sg_configuration.aperture_positions[ApertureValue.MEDIUM], aperture_in_medium_pos.name, ) @@ -348,14 +351,14 @@ async def test_given_aperture_not_set_through_device_but_motors_in_position_when async def test_when_aperture_set_and_device_read_then_position_returned( aperture: ApertureValue, aperture_in_medium_pos: ApertureScatterguard, - aperture_positions: dict[ApertureValue, AperturePosition], + ap_sg_configuration: ApertureScatterguardConfiguration, ): await aperture_in_medium_pos.selected_aperture.set(aperture) reading = await aperture_in_medium_pos.read() _assert_position_in_reading( reading, aperture, - aperture_positions[aperture], + ap_sg_configuration.aperture_positions[aperture], aperture_in_medium_pos.name, ) @@ -363,11 +366,11 @@ async def test_when_aperture_set_and_device_read_then_position_returned( async def test_ap_sg_in_runengine( aperture_in_medium_pos: ApertureScatterguard, run_engine: RunEngine, - aperture_positions: dict[ApertureValue, AperturePosition], + ap_sg_configuration: ApertureScatterguardConfiguration, ): ap = aperture_in_medium_pos.aperture sg = aperture_in_medium_pos.scatterguard - test_loc = aperture_positions[ApertureValue.SMALL] + test_loc = ap_sg_configuration.aperture_positions[ApertureValue.SMALL] def set_small_readback_pv(value, *args, **kwargs): set_mock_value(ap.small, 1) @@ -413,11 +416,15 @@ async def assert_all_positions_other_than_y( async def test_given_aperture_out_when_new_aperture_selected_then_aperture_not_moved_in( ap_sg: ApertureScatterguard, - aperture_positions: dict[ApertureValue, AperturePosition], + ap_sg_configuration: ApertureScatterguardConfiguration, ): ap = ap_sg.aperture - y_set_point = aperture_positions[ApertureValue.OUT_OF_BEAM].aperture_y - z_set_point = aperture_positions[ApertureValue.OUT_OF_BEAM].aperture_z + y_set_point = ap_sg_configuration.aperture_positions[ + ApertureValue.OUT_OF_BEAM + ].aperture_y + z_set_point = ap_sg_configuration.aperture_positions[ + ApertureValue.OUT_OF_BEAM + ].aperture_z ap.y.set(y_set_point) ap.z.set(z_set_point) set_mock_value(ap.y.user_readback, y_set_point) @@ -427,18 +434,18 @@ async def test_given_aperture_out_when_new_aperture_selected_then_aperture_not_m assert await ap.y.user_setpoint.get_value() == y_set_point await assert_all_positions_other_than_y( - ap_sg, aperture_positions[ApertureValue.SMALL] + ap_sg, ap_sg_configuration.aperture_positions[ApertureValue.SMALL] ) async def test_given_aperture_in_when_new_aperture_set_then_aperture_moved_safely( aperture_in_medium_pos: ApertureScatterguard, - aperture_positions: dict[ApertureValue, AperturePosition], + ap_sg_configuration: ApertureScatterguardConfiguration, ): aperture_in_medium_pos._safe_move_whilst_in_beam = AsyncMock() await aperture_in_medium_pos.prepare(ApertureValue.SMALL) aperture_in_medium_pos._safe_move_whilst_in_beam.assert_called_once_with( - aperture_positions[ApertureValue.SMALL] + ap_sg_configuration.aperture_positions[ApertureValue.SMALL] ) @@ -449,10 +456,10 @@ async def test_given_aperture_in_when_new_aperture_set_then_aperture_moved_safel async def test_given_in_and_aperture_selected_when_move_out_then_only_aperture_y_moves( selected_aperture: ApertureValue, ap_sg: ApertureScatterguard, - aperture_positions: dict[ApertureValue, AperturePosition], + ap_sg_configuration: ApertureScatterguardConfiguration, ): y_setpoint = ap_sg.aperture.y.user_setpoint - aperture_position = aperture_positions[selected_aperture] + aperture_position = ap_sg_configuration.aperture_positions[selected_aperture] await set_to_position(ap_sg, aperture_position) assert await y_setpoint.get_value() == aperture_position.aperture_y @@ -462,7 +469,7 @@ async def test_given_in_and_aperture_selected_when_move_out_then_only_aperture_y assert ( await y_setpoint.get_value() - == aperture_positions[ApertureValue.OUT_OF_BEAM].aperture_y + == ap_sg_configuration.aperture_positions[ApertureValue.OUT_OF_BEAM].aperture_y ) @@ -473,10 +480,10 @@ async def test_given_in_and_aperture_selected_when_move_out_then_only_aperture_y async def test_given_out_and_aperture_selected_when_move_in_then_correct_y_selected( selected_aperture: ApertureValue, ap_sg: ApertureScatterguard, - aperture_positions: dict[ApertureValue, AperturePosition], + ap_sg_configuration: ApertureScatterguardConfiguration, ): y_setpoint = ap_sg.aperture.y.user_setpoint - aperture_position = aperture_positions[selected_aperture] + aperture_position = ap_sg_configuration.aperture_positions[selected_aperture] set_mock_value(ap_sg.aperture.z.user_readback, aperture_position.aperture_z) await ap_sg.selected_aperture.set(ApertureValue.OUT_OF_BEAM) @@ -499,9 +506,9 @@ async def test_given_out_and_aperture_selected_when_move_in_then_correct_y_selec async def test_given_parked_and_aperture_selected_when_move_in_then_z_moved_out_first( selected_aperture: ApertureValue, ap_sg: ApertureScatterguard, - aperture_positions: dict[ApertureValue, AperturePosition], + ap_sg_configuration: ApertureScatterguardConfiguration, ): - parked_position = aperture_positions[ApertureValue.PARKED] + parked_position = ap_sg_configuration.aperture_positions[ApertureValue.PARKED] set_mock_value(ap_sg.aperture.y.user_readback, parked_position.aperture_y) set_mock_value(ap_sg.aperture.z.user_readback, parked_position.aperture_z) @@ -514,7 +521,7 @@ async def test_given_parked_and_aperture_selected_when_move_in_then_z_moved_out_ selected_aperture, wait=True ) assert parent_mock.method_calls[1] == call.aperture.z.user_setpoint.put( - aperture_positions[selected_aperture].aperture_z, wait=True + ap_sg_configuration.aperture_positions[selected_aperture].aperture_z, wait=True ) @@ -530,9 +537,9 @@ async def test_given_parked_and_aperture_selected_when_move_in_then_z_moved_out_ async def test_given_parked_and_ap_sg_prepared_when_move_in_then_z_moved_out_first( selected_aperture: ApertureValue, ap_sg: ApertureScatterguard, - aperture_positions: dict[ApertureValue, AperturePosition], + ap_sg_configuration: ApertureScatterguardConfiguration, ): - parked_position = aperture_positions[ApertureValue.PARKED] + parked_position = ap_sg_configuration.aperture_positions[ApertureValue.PARKED] set_mock_value(ap_sg.aperture.y.user_readback, parked_position.aperture_y) set_mock_value(ap_sg.aperture.z.user_readback, parked_position.aperture_z) @@ -545,7 +552,7 @@ async def test_given_parked_and_ap_sg_prepared_when_move_in_then_z_moved_out_fir selected_aperture, wait=True ) assert parent_mock.method_calls[1] == call.aperture.z.user_setpoint.put( - aperture_positions[selected_aperture].aperture_z, wait=True + ap_sg_configuration.aperture_positions[selected_aperture].aperture_z, wait=True ) @@ -569,3 +576,11 @@ async def set_motor_moving(value, *args, **kwargs): with pytest.raises(InvalidApertureMoveError): await aperture_in_medium_pos.selected_aperture.set(ApertureValue.SMALL) + + +def test_restore_ap_sg_from_scin_move_position(): + pass + + +def test_ap_sg_moves_to_scin_move_position_from_current_pos(): + pass diff --git a/tests/devices/test_scintillator.py b/tests/devices/test_scintillator.py index ce1579bc1b..397fe4c244 100644 --- a/tests/devices/test_scintillator.py +++ b/tests/devices/test_scintillator.py @@ -31,10 +31,12 @@ async def scintillator_and_ap_sg( mock_ap_sg = MagicMock() mock_ap_sg.return_value.selected_aperture.set = AsyncMock() mock_ap_sg.return_value.selected_aperture.get_value = AsyncMock() + mock_beamstop = MagicMock() scintillator = Scintillator( prefix="", name="test_scin", aperture_scatterguard=mock_ap_sg, + beamstop=mock_beamstop, beamline_parameters=mock_beamline_parameters, ) await scintillator.y_mm.set(5) @@ -134,3 +136,19 @@ async def test_given_scintillator_already_out_when_moved_in_or_out_then_does_not get_mock_put(scintillator.y_mm.user_setpoint).assert_not_called() get_mock_put(scintillator.z_mm.user_setpoint).assert_not_called() + + +def test_move_scintillator_out_moves_beamstop_to_data_collection_if_needed(): + pass + + +def test_move_scintillator_out_does_not_move_beamstop_if_already_in_dc(): + pass + + +def test_move_scintillator_out_moves_ap_sg_to_parked_if_needed(): + pass + + +def test_move_scintillator_out_does_not_move_ap_sg_if_already_parked(): + pass From b72ee3e95182f5d290a93aa5d0421dcbb2730b58 Mon Sep 17 00:00:00 2001 From: Robert Tuck Date: Wed, 14 Jan 2026 14:13:33 +0000 Subject: [PATCH 02/16] 1540 Add docs for aperture-scatterguard --- docs/images/aperture_scatterguard.svg | 4 ++++ docs/reference/Aperture-Scatterguard-Collisions.md | 6 ++++++ 2 files changed, 10 insertions(+) create mode 100644 docs/images/aperture_scatterguard.svg create mode 100644 docs/reference/Aperture-Scatterguard-Collisions.md diff --git a/docs/images/aperture_scatterguard.svg b/docs/images/aperture_scatterguard.svg new file mode 100644 index 0000000000..a7fab2dc88 --- /dev/null +++ b/docs/images/aperture_scatterguard.svg @@ -0,0 +1,4 @@ + + + +
Cryostream
Cryostream
Aperture (with 3 positions)
Aperture (wit...
Scatterguard
(has additional independent axis)
Scatterguard...
Moves together on miniapt axis
Moves together on...
+ y
+ yText is not SVG - cannot display diff --git a/docs/reference/Aperture-Scatterguard-Collisions.md b/docs/reference/Aperture-Scatterguard-Collisions.md new file mode 100644 index 0000000000..8f3b2fc8ed --- /dev/null +++ b/docs/reference/Aperture-Scatterguard-Collisions.md @@ -0,0 +1,6 @@ +If the aperture/scatterguard are not moved sensibly in y it is possible for the scatterguard to collide with the cryostream. See the below diagram... + +```{raw} html +:file: ../images/aperture_scatterguard.svg +``` +If for example the top aperture is to be chosen then the whole assembly must move down so that the top aperture matches the beam then the scatterguard up to match the top aperture. If instead the bottom aperture is chosen then the scatterguard must move down then the whole assembly moved up. From a36a0987f665474a44d0bda49a2dd0658f709f76 Mon Sep 17 00:00:00 2001 From: Robert Tuck Date: Thu, 15 Jan 2026 12:05:43 +0000 Subject: [PATCH 03/16] Add beamstop check to scintillator move --- .../Aperture-Scatterguard-Collisions.md | 9 ++ src/dodal/devices/scintillator.py | 24 ++-- tests/devices/test_aperture_scatterguard.py | 36 +++-- tests/devices/test_scintillator.py | 131 +++++++++++++----- 4 files changed, 145 insertions(+), 55 deletions(-) diff --git a/docs/reference/Aperture-Scatterguard-Collisions.md b/docs/reference/Aperture-Scatterguard-Collisions.md index 8f3b2fc8ed..76f2e998ae 100644 --- a/docs/reference/Aperture-Scatterguard-Collisions.md +++ b/docs/reference/Aperture-Scatterguard-Collisions.md @@ -1,6 +1,15 @@ +# Aperture Scatterguard and Scintillator Collisions + If the aperture/scatterguard are not moved sensibly in y it is possible for the scatterguard to collide with the cryostream. See the below diagram... ```{raw} html :file: ../images/aperture_scatterguard.svg ``` If for example the top aperture is to be chosen then the whole assembly must move down so that the top aperture matches the beam then the scatterguard up to match the top aperture. If instead the bottom aperture is chosen then the scatterguard must move down then the whole assembly moved up. + +The aperture/scatterguard may also collide with the scintillator as described by the diagram below. + +Normally the scintillator is stowed behind the aperture/scatterguard in the z direction, to move it into position it must be moved in front; to do this the aperture/scatterguard must be moved sideways in the -ve x direction out of the way. + +The aperture/scatterguard must be moved in the -ve x direction in order to avoid colliding with the scintillator when it is +moved in / out (scintillator only has motion in the y, z axes). diff --git a/src/dodal/devices/scintillator.py b/src/dodal/devices/scintillator.py index dbf56d1d77..fe6f55e869 100644 --- a/src/dodal/devices/scintillator.py +++ b/src/dodal/devices/scintillator.py @@ -4,8 +4,8 @@ from ophyd_async.epics.motor import Motor from dodal.common.beamlines.beamline_parameters import GDABeamlineParameters -from dodal.devices.aperturescatterguard import ApertureScatterguard, ApertureValue -from dodal.devices.mx_phase1.beamstop import Beamstop +from dodal.devices.aperturescatterguard import ApertureScatterguard +from dodal.devices.mx_phase1.beamstop import Beamstop, BeamstopPositions class InOut(StrictEnum): @@ -80,23 +80,24 @@ def _get_selected_position(self, y: float, z: float) -> InOut: else: return InOut.UNKNOWN - async def _check_aperture_parked(self): - if ( - await self._aperture_scatterguard().selected_aperture.get_value() - != ApertureValue.PARKED - ): - raise ValueError( - f"Cannot move scintillator if aperture/scatterguard is not parked. Position is currently {await self._aperture_scatterguard().selected_aperture.get_value()}" - ) + async def _check_beamstop_position(self): + position = await self._beamstop().selected_pos.get_value() + match position: + case BeamstopPositions.OUT_OF_BEAM | BeamstopPositions.DATA_COLLECTION: + return + case _: + raise ValueError( + f"Scintillator cannot be moved due to beamstop position {position}, must be in either in DATA_COLLECTION or OUT_OF_BEAM position." + ) async def _set_selected_position(self, position: InOut) -> None: + await self._check_beamstop_position() match position: case InOut.OUT: current_y = await self.y_mm.user_readback.get_value() current_z = await self.z_mm.user_readback.get_value() if self._get_selected_position(current_y, current_z) == InOut.OUT: return - await self._check_aperture_parked() async def move_scin_out(): await self.y_mm.set(self._scintillator_out_yz_mm[0]) @@ -109,7 +110,6 @@ async def move_scin_out(): current_z = await self.z_mm.user_readback.get_value() if self._get_selected_position(current_y, current_z) == InOut.IN: return - await self._check_aperture_parked() await self.z_mm.set(self._scintillator_in_yz_mm[1]) await self.y_mm.set(self._scintillator_in_yz_mm[0]) case _: diff --git a/tests/devices/test_aperture_scatterguard.py b/tests/devices/test_aperture_scatterguard.py index 51bee788c1..70ba922e0a 100644 --- a/tests/devices/test_aperture_scatterguard.py +++ b/tests/devices/test_aperture_scatterguard.py @@ -178,13 +178,17 @@ async def test_aperture_scatterguard_returns_status_if_within_tolerance( await ap_sg._safe_move_whilst_in_beam(pos) -def set_underlying_motors(ap_sg: ApertureScatterguard, position: AperturePosition): +async def set_underlying_motors( + ap_sg: ApertureScatterguard, + config: ApertureScatterguardConfiguration, + aperture: ApertureValue, +): for motor, pos in zip( get_all_motors(ap_sg), - position.values, + dict(config.aperture_positions[aperture]).values(), strict=False, ): - motor.set(pos) + await motor.set(pos) @pytest.mark.parametrize( @@ -578,9 +582,25 @@ async def set_motor_moving(value, *args, **kwargs): await aperture_in_medium_pos.selected_aperture.set(ApertureValue.SMALL) -def test_restore_ap_sg_from_scin_move_position(): - pass - +@pytest.mark.parametrize( + "selected_aperture", + [ + ApertureValue.SMALL, + ApertureValue.MEDIUM, + ApertureValue.LARGE, + ApertureValue.OUT_OF_BEAM, + ApertureValue.PARKED, + ], +) +async def test_restore_ap_sg_from_scin_move_position( + aperture_in_medium_pos: ApertureScatterguard, + ap_sg_configuration: ApertureScatterguardConfiguration, + selected_aperture: ApertureValue, + run_engine: RunEngine, +): + ap_sg = aperture_in_medium_pos + await set_underlying_motors(ap_sg, ap_sg_configuration, selected_aperture) -def test_ap_sg_moves_to_scin_move_position_from_current_pos(): - pass + positions = ap_sg.get_scin_move_position() + assert positions[ap_sg.aperture.x] == ap_sg_configuration.scintillator_move_miniap_x + assert positions[ap_sg.scatterguard.x] == ap_sg_configuration.scintillator_move_sg_x diff --git a/tests/devices/test_scintillator.py b/tests/devices/test_scintillator.py index 397fe4c244..d0fd61ddc4 100644 --- a/tests/devices/test_scintillator.py +++ b/tests/devices/test_scintillator.py @@ -1,11 +1,16 @@ -from unittest.mock import AsyncMock, MagicMock +from contextlib import nullcontext +from unittest.mock import AsyncMock, MagicMock, call import pytest -from ophyd_async.core import get_mock_put, init_devices +from ophyd_async.core import Reference, get_mock_put, init_devices, set_mock_value from ophyd_async.testing import assert_value from dodal.common.beamlines.beamline_parameters import GDABeamlineParameters -from dodal.devices.aperturescatterguard import ApertureScatterguard, ApertureValue +from dodal.devices.aperturescatterguard import ( + ApertureScatterguard, + ApertureValue, +) +from dodal.devices.mx_phase1.beamstop import Beamstop, BeamstopPositions from dodal.devices.scintillator import InOut, Scintillator @@ -19,29 +24,47 @@ def mock_beamline_parameters() -> GDABeamlineParameters: "scin_z_SCIN_OUT": 0.1, "scin_y_tolerance": 0.1, "scin_z_tolerance": 0.12, + "in_beam_x_STANDARD": 1.21, + "in_beam_y_STANDARD": 45.4, + "in_beam_z_STANDARD": 30.0, + "bs_x_tolerance": 0.02, + "bs_y_tolerance": 0.005, + "bs_z_tolerance": 0.3, } ) +@pytest.fixture +def beamstop(mock_beamline_parameters) -> Beamstop: + beamstop = Beamstop("-MO-BS-01:", mock_beamline_parameters, name="beamstop") + beamstop.selected_pos.get_value = AsyncMock( + return_value=BeamstopPositions.DATA_COLLECTION + ) + return beamstop + + @pytest.fixture async def scintillator_and_ap_sg( mock_beamline_parameters: GDABeamlineParameters, + beamstop: Beamstop, + ap_sg: ApertureScatterguard, ) -> tuple[Scintillator, MagicMock]: async with init_devices(mock=True): - mock_ap_sg = MagicMock() - mock_ap_sg.return_value.selected_aperture.set = AsyncMock() - mock_ap_sg.return_value.selected_aperture.get_value = AsyncMock() - mock_beamstop = MagicMock() + ap_sg.selected_aperture.set = AsyncMock() + ap_sg.selected_aperture.get_value = AsyncMock() + ap_sg.get_scin_move_position = MagicMock() scintillator = Scintillator( prefix="", name="test_scin", - aperture_scatterguard=mock_ap_sg, - beamstop=mock_beamstop, + aperture_scatterguard=Reference(ap_sg), + beamstop=Reference(beamstop), beamline_parameters=mock_beamline_parameters, ) + set_mock_value(ap_sg.aperture.x.user_readback, 1.0) + set_mock_value(ap_sg.scatterguard.x.user_readback, 2.0) await scintillator.y_mm.set(5) await scintillator.z_mm.set(5) - return scintillator, mock_ap_sg + return scintillator, ap_sg @pytest.mark.parametrize( @@ -79,7 +102,7 @@ async def test_given_aperture_scatterguard_parked_when_set_to_out_position_then_ scintillator_and_ap_sg: tuple[Scintillator, ApertureScatterguard], ): scintillator, ap_sg = scintillator_and_ap_sg - ap_sg.return_value.selected_aperture.get_value.return_value = ApertureValue.PARKED # type: ignore + ap_sg.selected_aperture.get_value.return_value = ApertureValue.PARKED # type: ignore await scintillator.selected_pos.set(InOut.OUT) @@ -91,7 +114,7 @@ async def test_given_aperture_scatterguard_parked_when_set_to_in_position_then_r scintillator_and_ap_sg: tuple[Scintillator, ApertureScatterguard], ): scintillator, ap_sg = scintillator_and_ap_sg - ap_sg.return_value.selected_aperture.get_value.return_value = ApertureValue.PARKED # type: ignore + ap_sg.selected_aperture.get_value.return_value = ApertureValue.PARKED # type: ignore await scintillator.selected_pos.set(InOut.IN) @@ -99,18 +122,6 @@ async def test_given_aperture_scatterguard_parked_when_set_to_in_position_then_r await assert_value(scintillator.z_mm.user_setpoint, 101.5115) -@pytest.mark.parametrize("scint_pos", [InOut.OUT, InOut.IN]) -async def test_given_aperture_scatterguard_not_parked_when_set_to_in_or_out_position_then_exception_raised( - scintillator_and_ap_sg: tuple[Scintillator, ApertureScatterguard], scint_pos -): - for position in ApertureValue: - if position != ApertureValue.PARKED: - scintillator, ap_sg = scintillator_and_ap_sg - ap_sg.return_value.selected_aperture.get_value.return_value = position # type: ignore - with pytest.raises(ValueError): - await scintillator.selected_pos.set(scint_pos) - - @pytest.mark.parametrize( "y, z, expected_position", [ @@ -131,24 +142,74 @@ async def test_given_scintillator_already_out_when_moved_in_or_out_then_does_not get_mock_put(scintillator.y_mm.user_setpoint).reset_mock() get_mock_put(scintillator.z_mm.user_setpoint).reset_mock() - ap_sg.return_value.selected_aperture.get_value.return_value = ApertureValue.LARGE # type: ignore + ap_sg.selected_aperture.get_value.return_value = ApertureValue.LARGE # type: ignore await scintillator.selected_pos.set(expected_position) get_mock_put(scintillator.y_mm.user_setpoint).assert_not_called() get_mock_put(scintillator.z_mm.user_setpoint).assert_not_called() -def test_move_scintillator_out_moves_beamstop_to_data_collection_if_needed(): - pass - - -def test_move_scintillator_out_does_not_move_beamstop_if_already_in_dc(): - pass +@pytest.mark.parametrize( + "beamstop_position, expected_good", + [ + [BeamstopPositions.DATA_COLLECTION, True], + [BeamstopPositions.OUT_OF_BEAM, True], + [BeamstopPositions.UNKNOWN, False], + ], +) +async def test_beamstop_check_in_known_good_position( + scintillator_and_ap_sg: tuple[Scintillator, ApertureScatterguard], + beamstop: Beamstop, + beamstop_position: BeamstopPositions, + expected_good: bool, +): + beamstop.selected_pos.get_value.return_value = beamstop_position + scintillator, _ = scintillator_and_ap_sg + with ( + pytest.raises( + ValueError, match="Scintillator cannot be moved due to beamstop position" + ) + if not expected_good + else nullcontext() + ): + await scintillator.selected_pos.set(InOut.OUT) -def test_move_scintillator_out_moves_ap_sg_to_parked_if_needed(): - pass +async def test_move_scintillator_out_moves_ap_sg_to_scin_move_and_back( + scintillator_and_ap_sg: tuple[Scintillator, ApertureScatterguard], +): + scintillator, ap_sg = scintillator_and_ap_sg + ap_sg.get_scin_move_position.return_value = { + ap_sg.aperture.x: -1.0, + ap_sg.scatterguard.x: -1.5, + } + + parent = MagicMock() + parent.aperture.attach_mock(get_mock_put(ap_sg.aperture.x.user_setpoint), "x") + parent.aperture.attach_mock(get_mock_put(ap_sg.aperture.y.user_setpoint), "y") + parent.aperture.attach_mock(get_mock_put(ap_sg.aperture.z.user_setpoint), "z") + parent.scatterguard.attach_mock( + get_mock_put(ap_sg.scatterguard.x.user_setpoint), "x" + ) + parent.scatterguard.attach_mock( + get_mock_put(ap_sg.scatterguard.y.user_setpoint), "y" + ) + parent.scintillator.attach_mock( + get_mock_put(scintillator.y_mm.user_setpoint), "y_mm" + ) + parent.scintillator.attach_mock( + get_mock_put(scintillator.z_mm.user_setpoint), "z_mm" + ) + await scintillator.selected_pos.set(InOut.OUT) -def test_move_scintillator_out_does_not_move_ap_sg_if_already_parked(): - pass + parent.assert_has_calls( + [ + call.aperture.x(-1.0, wait=True), + call.scatterguard.x(-1.5, wait=True), + call.scintillator.y_mm(-0.02, wait=True), + call.scintillator.z_mm(0.1, wait=True), + call.aperture.x(1.0, wait=True), + call.scatterguard.x(2.0, wait=True), + ] + ) From e685f8edc8dde2ec8aa68783057e826f2064bdc6 Mon Sep 17 00:00:00 2001 From: Robert Tuck Date: Thu, 15 Jan 2026 13:25:54 +0000 Subject: [PATCH 04/16] Implement aperture scatterguard move for reverse direction --- src/dodal/devices/scintillator.py | 33 ++++++++++---------- tests/devices/test_scintillator.py | 48 ++++++++++++++++++++++++------ 2 files changed, 54 insertions(+), 27 deletions(-) diff --git a/src/dodal/devices/scintillator.py b/src/dodal/devices/scintillator.py index fe6f55e869..999616d592 100644 --- a/src/dodal/devices/scintillator.py +++ b/src/dodal/devices/scintillator.py @@ -91,27 +91,24 @@ async def _check_beamstop_position(self): ) async def _set_selected_position(self, position: InOut) -> None: + current_y = await self.y_mm.user_readback.get_value() + current_z = await self.z_mm.user_readback.get_value() + if self._get_selected_position(current_y, current_z) == position: + return + await self._check_beamstop_position() - match position: - case InOut.OUT: - current_y = await self.y_mm.user_readback.get_value() - current_z = await self.z_mm.user_readback.get_value() - if self._get_selected_position(current_y, current_z) == InOut.OUT: - return - - async def move_scin_out(): - await self.y_mm.set(self._scintillator_out_yz_mm[0]) - await self.z_mm.set(self._scintillator_out_yz_mm[1]) - - await self.do_with_ap_sg_in_safe_pos(move_scin_out) - - case InOut.IN: - current_y = await self.y_mm.user_readback.get_value() - current_z = await self.z_mm.user_readback.get_value() - if self._get_selected_position(current_y, current_z) == InOut.IN: - return + + async def move_to_new_position(): + if position == InOut.OUT: + await self.y_mm.set(self._scintillator_out_yz_mm[0]) + await self.z_mm.set(self._scintillator_out_yz_mm[1]) + elif position == InOut.IN: await self.z_mm.set(self._scintillator_in_yz_mm[1]) await self.y_mm.set(self._scintillator_in_yz_mm[0]) + + match position: + case InOut.OUT | InOut.IN: + await self.do_with_ap_sg_in_safe_pos(move_to_new_position) case _: raise ValueError(f"Cannot set scintillator to position {position}") diff --git a/tests/devices/test_scintillator.py b/tests/devices/test_scintillator.py index d0fd61ddc4..93a43a15cb 100644 --- a/tests/devices/test_scintillator.py +++ b/tests/devices/test_scintillator.py @@ -94,6 +94,8 @@ async def test_when_set_to_unknown_position_then_error_raised( scintillator_and_ap_sg: tuple[Scintillator, ApertureScatterguard], ): scintillator, _ = scintillator_and_ap_sg + await scintillator.y_mm.set(100.855) + await scintillator.z_mm.set(101.5115) with pytest.raises(ValueError): await scintillator.selected_pos.set(InOut.UNKNOWN) @@ -131,11 +133,17 @@ async def test_given_aperture_scatterguard_parked_when_set_to_in_position_then_r ) async def test_given_scintillator_already_out_when_moved_in_or_out_then_does_nothing( scintillator_and_ap_sg: tuple[Scintillator, ApertureScatterguard], + beamstop: Beamstop, expected_position, y, z, ): scintillator, ap_sg = scintillator_and_ap_sg + ap_sg.get_scin_move_position.return_value = { + ap_sg.aperture.x: -1.0, + ap_sg.scatterguard.x: -1.5, + } + beamstop.selected_pos.get_value.return_value = BeamstopPositions.UNKNOWN await scintillator.y_mm.set(y) await scintillator.z_mm.set(z) @@ -147,6 +155,8 @@ async def test_given_scintillator_already_out_when_moved_in_or_out_then_does_not get_mock_put(scintillator.y_mm.user_setpoint).assert_not_called() get_mock_put(scintillator.z_mm.user_setpoint).assert_not_called() + get_mock_put(ap_sg.aperture.x.user_setpoint).assert_not_called() + get_mock_put(ap_sg.scatterguard.x.user_setpoint).assert_not_called() @pytest.mark.parametrize( @@ -176,14 +186,29 @@ async def test_beamstop_check_in_known_good_position( await scintillator.selected_pos.set(InOut.OUT) -async def test_move_scintillator_out_moves_ap_sg_to_scin_move_and_back( +@pytest.mark.parametrize( + "initial_y, initial_z, final_y, final_z, swap_order, final_position", + [ + [100.855, 101.5115, -0.02, 0.1, False, InOut.OUT], + [-0.02, 0.1, 100.855, 101.5115, True, InOut.IN], + ], +) +async def test_move_scintillator_moves_ap_sg_to_scin_move_and_back( scintillator_and_ap_sg: tuple[Scintillator, ApertureScatterguard], + initial_y: float, + initial_z: float, + final_y: float, + final_z: float, + swap_order: bool, + final_position: InOut, ): scintillator, ap_sg = scintillator_and_ap_sg ap_sg.get_scin_move_position.return_value = { ap_sg.aperture.x: -1.0, ap_sg.scatterguard.x: -1.5, } + set_mock_value(scintillator.y_mm.user_readback, initial_y) + set_mock_value(scintillator.z_mm.user_readback, initial_z) parent = MagicMock() parent.aperture.attach_mock(get_mock_put(ap_sg.aperture.x.user_setpoint), "x") @@ -201,15 +226,20 @@ async def test_move_scintillator_out_moves_ap_sg_to_scin_move_and_back( parent.scintillator.attach_mock( get_mock_put(scintillator.z_mm.user_setpoint), "z_mm" ) - await scintillator.selected_pos.set(InOut.OUT) - - parent.assert_has_calls( - [ - call.aperture.x(-1.0, wait=True), - call.scatterguard.x(-1.5, wait=True), - call.scintillator.y_mm(-0.02, wait=True), - call.scintillator.z_mm(0.1, wait=True), + await scintillator.selected_pos.set(final_position) + + expected_scintillator_move = [ + call.scintillator.y_mm(final_y, wait=True), + call.scintillator.z_mm(final_z, wait=True), + ] + if swap_order: + expected_scintillator_move = expected_scintillator_move[::-1] + expected_calls = ( + [call.aperture.x(-1.0, wait=True), call.scatterguard.x(-1.5, wait=True)] + + expected_scintillator_move + + [ call.aperture.x(1.0, wait=True), call.scatterguard.x(2.0, wait=True), ] ) + parent.assert_has_calls(expected_calls) From e74e11bc9fa38284ecdd62046bf48579e0ad59f5 Mon Sep 17 00:00:00 2001 From: Robert Tuck Date: Thu, 15 Jan 2026 13:28:29 +0000 Subject: [PATCH 05/16] Make type-checking happy --- tests/devices/test_scintillator.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/tests/devices/test_scintillator.py b/tests/devices/test_scintillator.py index 93a43a15cb..33949610e7 100644 --- a/tests/devices/test_scintillator.py +++ b/tests/devices/test_scintillator.py @@ -48,7 +48,7 @@ async def scintillator_and_ap_sg( mock_beamline_parameters: GDABeamlineParameters, beamstop: Beamstop, ap_sg: ApertureScatterguard, -) -> tuple[Scintillator, MagicMock]: +) -> tuple[Scintillator, ApertureScatterguard]: async with init_devices(mock=True): ap_sg.selected_aperture.set = AsyncMock() ap_sg.selected_aperture.get_value = AsyncMock() @@ -139,11 +139,11 @@ async def test_given_scintillator_already_out_when_moved_in_or_out_then_does_not z, ): scintillator, ap_sg = scintillator_and_ap_sg - ap_sg.get_scin_move_position.return_value = { + ap_sg.get_scin_move_position.return_value = { # type: ignore ap_sg.aperture.x: -1.0, ap_sg.scatterguard.x: -1.5, } - beamstop.selected_pos.get_value.return_value = BeamstopPositions.UNKNOWN + beamstop.selected_pos.get_value.return_value = BeamstopPositions.UNKNOWN # type: ignore await scintillator.y_mm.set(y) await scintillator.z_mm.set(z) @@ -173,7 +173,7 @@ async def test_beamstop_check_in_known_good_position( beamstop_position: BeamstopPositions, expected_good: bool, ): - beamstop.selected_pos.get_value.return_value = beamstop_position + beamstop.selected_pos.get_value.return_value = beamstop_position # type: ignore scintillator, _ = scintillator_and_ap_sg with ( @@ -203,7 +203,7 @@ async def test_move_scintillator_moves_ap_sg_to_scin_move_and_back( final_position: InOut, ): scintillator, ap_sg = scintillator_and_ap_sg - ap_sg.get_scin_move_position.return_value = { + ap_sg.get_scin_move_position.return_value = { # type: ignore ap_sg.aperture.x: -1.0, ap_sg.scatterguard.x: -1.5, } From 483ab941eaa4b059314b9a8478929da966b4f990 Mon Sep 17 00:00:00 2001 From: Robert Tuck Date: Thu, 15 Jan 2026 16:36:53 +0000 Subject: [PATCH 06/16] Add scintillator motion diagram --- docs/images/scintillator-motion.svg | 522 ++++++++++++++++++ ...md => aperture-scatterguard-collisions.md} | 4 + 2 files changed, 526 insertions(+) create mode 100644 docs/images/scintillator-motion.svg rename docs/reference/{Aperture-Scatterguard-Collisions.md => aperture-scatterguard-collisions.md} (94%) diff --git a/docs/images/scintillator-motion.svg b/docs/images/scintillator-motion.svg new file mode 100644 index 0000000000..7780066a71 --- /dev/null +++ b/docs/images/scintillator-motion.svg @@ -0,0 +1,522 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + image/svg+xml + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + aperture-scatterguardmust move in -ve x directionin order for scintillator to come forward and up + + scintillator has y, z axis motion only + z + y + + beam + + + diff --git a/docs/reference/Aperture-Scatterguard-Collisions.md b/docs/reference/aperture-scatterguard-collisions.md similarity index 94% rename from docs/reference/Aperture-Scatterguard-Collisions.md rename to docs/reference/aperture-scatterguard-collisions.md index 76f2e998ae..c94dc21027 100644 --- a/docs/reference/Aperture-Scatterguard-Collisions.md +++ b/docs/reference/aperture-scatterguard-collisions.md @@ -13,3 +13,7 @@ Normally the scintillator is stowed behind the aperture/scatterguard in the z di The aperture/scatterguard must be moved in the -ve x direction in order to avoid colliding with the scintillator when it is moved in / out (scintillator only has motion in the y, z axes). + +```{raw} html +:file: ../images/scintillator-motion.svg +``` From 2a3a39c5c9cf0cfa421c14d05ed3fe41ad906fdb Mon Sep 17 00:00:00 2001 From: Robert Tuck Date: Mon, 19 Jan 2026 10:10:59 +0000 Subject: [PATCH 07/16] Correct annotation of scintillator motion diagram --- docs/images/scintillator-motion.svg | 44 ++++++++++++++--------------- 1 file changed, 22 insertions(+), 22 deletions(-) diff --git a/docs/images/scintillator-motion.svg b/docs/images/scintillator-motion.svg index 7780066a71..0f54e83222 100644 --- a/docs/images/scintillator-motion.svg +++ b/docs/images/scintillator-motion.svg @@ -143,18 +143,18 @@ borderopacity="1.0" inkscape:pageopacity="0.0" inkscape:pageshadow="2" - inkscape:zoom="0.7" - inkscape:cx="-100.78553" - inkscape:cy="440.79498" + inkscape:zoom="0.98994949" + inkscape:cx="21.357968" + inkscape:cy="384.80054" inkscape:document-units="mm" - inkscape:current-layer="layer1" + inkscape:current-layer="g2362" inkscape:document-rotation="0" showgrid="false" inkscape:snap-page="true" inkscape:window-width="1682" inkscape:window-height="863" inkscape:window-x="238" - inkscape:window-y="314" + inkscape:window-y="203" inkscape:window-maximized="0" fit-margin-top="0" fit-margin-left="0" @@ -242,15 +242,15 @@ id="path155-9" style="fill:#e9e9ff;fill-rule:evenodd;stroke:none;stroke-linejoin:round" inkscape:box3dsidetype="11" - points="130.0731,77.855811 130.0731,165.40224 126.21895,163.76541 126.21895,76.218975 " - d="m 126.21895,76.218975 3.85415,1.636836 v 87.546429 l -3.85415,-1.63683 z" /> + points="130.0731,77.855811 130.0731,165.40224 126.21895,163.76541 126.21895,76.218974 " + d="m 126.21895,76.218974 3.85415,1.636837 v 87.546429 l -3.85415,-1.63683 z" /> + points="116.35042,169.46301 126.21895,163.76541 126.21895,76.218974 116.35042,81.916576 " + d="m 116.35042,81.916576 v 87.546434 l 9.86853,-5.6976 V 76.218974 Z" /> + points="120.20457,83.553413 130.0731,77.855811 126.21895,76.218974 116.35042,81.916576 " + d="m 116.35042,81.916576 3.85415,1.636837 9.86853,-5.697602 -3.85415,-1.636837 z" /> + d="m 130.18779,77.789597 2.95948,1.256879 v 35.811694 l -2.95948,-1.25688 z" + points="133.14727,79.046476 133.14727,114.85817 130.18779,113.60129 130.18779,77.789597 " /> + d="M 120.20457,83.553413 V 119.3651 l 9.98322,-5.76381 V 77.789597 Z" + points="120.20457,119.3651 130.18779,113.60129 130.18779,77.789597 120.20457,83.553413 " /> + d="m 120.20457,83.553413 2.95948,1.256878 9.98322,-5.763815 -2.95948,-1.256879 z" + points="123.16405,84.810291 133.14727,79.046476 130.18779,77.789597 120.20457,83.553413 " /> + d="m 123.16405,84.810291 v 35.811689 l 9.98322,-5.76381 V 79.046476 Z" + points="123.16405,120.62198 133.14727,114.85817 133.14727,79.046476 123.16405,84.810291 " /> + d="m 120.20457,83.553413 2.95948,1.256878 v 35.811689 l -2.95948,-1.25688 z" + points="123.16405,84.810291 123.16405,120.62198 120.20457,119.3651 120.20457,83.553413 " /> Date: Mon, 19 Jan 2026 10:15:17 +0000 Subject: [PATCH 08/16] Make type-checking happy --- src/dodal/beamlines/i04.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/dodal/beamlines/i04.py b/src/dodal/beamlines/i04.py index 1a82cf3b80..3cfcb30acd 100644 --- a/src/dodal/beamlines/i04.py +++ b/src/dodal/beamlines/i04.py @@ -277,7 +277,9 @@ def pin_tip_detection() -> PinTipDetection: @devices.factory() -def scintillator(aperture_scatterguard: ApertureScatterguard, beamstop: Beamstop) -> Scintillator: +def scintillator( + aperture_scatterguard: ApertureScatterguard, beamstop: Beamstop +) -> Scintillator: return Scintillator( f"{PREFIX.beamline_prefix}-MO-SCIN-01:", Reference(aperture_scatterguard), From d09522cfb619989645d50bbef2cb28bb135b5004 Mon Sep 17 00:00:00 2001 From: Robert Tuck Date: Mon, 26 Jan 2026 17:19:32 +0000 Subject: [PATCH 09/16] Response to PR comments --- docs/images/scintillator-motion.svg | 10 ++++---- src/dodal/devices/aperturescatterguard.py | 21 ++++++++++------ src/dodal/devices/scintillator.py | 27 +++++++++++++-------- tests/devices/test_aperture_scatterguard.py | 11 ++++++--- 4 files changed, 44 insertions(+), 25 deletions(-) diff --git a/docs/images/scintillator-motion.svg b/docs/images/scintillator-motion.svg index 0f54e83222..ab4e78e7e7 100644 --- a/docs/images/scintillator-motion.svg +++ b/docs/images/scintillator-motion.svg @@ -141,10 +141,10 @@ pagecolor="#ffffff" bordercolor="#666666" borderopacity="1.0" - inkscape:pageopacity="0.0" + inkscape:pageopacity="1" inkscape:pageshadow="2" inkscape:zoom="0.98994949" - inkscape:cx="21.357968" + inkscape:cx="-102.38572" inkscape:cy="384.80054" inkscape:document-units="mm" inkscape:current-layer="g2362" @@ -153,8 +153,8 @@ inkscape:snap-page="true" inkscape:window-width="1682" inkscape:window-height="863" - inkscape:window-x="238" - inkscape:window-y="203" + inkscape:window-x="2112" + inkscape:window-y="174" inkscape:window-maximized="0" fit-margin-top="0" fit-margin-left="0" @@ -168,7 +168,7 @@ image/svg+xml - + diff --git a/src/dodal/devices/aperturescatterguard.py b/src/dodal/devices/aperturescatterguard.py index d59e5eca19..5c7556e673 100644 --- a/src/dodal/devices/aperturescatterguard.py +++ b/src/dodal/devices/aperturescatterguard.py @@ -114,9 +114,16 @@ def __str__(self): @dataclasses.dataclass class ApertureScatterguardConfiguration: aperture_positions: dict[ApertureValue, AperturePosition] - # The SCIN MOVE position is defined only for x-axis - scintillator_move_miniap_x: float - scintillator_move_sg_x: float + # Note on scintillator move configuration: + # The aperture scatterguard must move out of the way of the scintillator when + # the scintillator moves. + # (see "Aperture Scatterguard and Scintillator Collisions" in the documentation) + # The SCIN MOVE position is defined only for the x-axis; the y and z coordinates + # are preserved at the current aperture-scatterguard position. + # Both the aperture (miniap) and scatterguard(sg) are moved, and then subsequently restored. + # As this is a transient move, it is not represented in the ApertureValue enumeration. + scintillator_move_aperture_x: float + scintillator_move_scatterguard_x: float def load_configuration( @@ -141,8 +148,8 @@ def load_configuration( } return ApertureScatterguardConfiguration( aperture_positions=positions, - scintillator_move_miniap_x=params["miniap_x_SCIN_MOVE"], - scintillator_move_sg_x=params["sg_x_SCIN_MOVE"], + scintillator_move_aperture_x=params["miniap_x_SCIN_MOVE"], + scintillator_move_scatterguard_x=params["sg_x_SCIN_MOVE"], ) @@ -392,6 +399,6 @@ async def prepare(self, value: ApertureValue): def get_scin_move_position(self) -> dict[Motor, float]: return { - self.aperture.x: self._config.scintillator_move_miniap_x, - self.scatterguard.x: self._config.scintillator_move_sg_x, + self.aperture.x: self._config.scintillator_move_aperture_x, + self.scatterguard.x: self._config.scintillator_move_scatterguard_x, } diff --git a/src/dodal/devices/scintillator.py b/src/dodal/devices/scintillator.py index 999616d592..5ed17d2551 100644 --- a/src/dodal/devices/scintillator.py +++ b/src/dodal/devices/scintillator.py @@ -1,3 +1,4 @@ +from functools import partial from math import isclose from ophyd_async.core import Reference, StandardReadable, StrictEnum, derived_signal_rw @@ -98,21 +99,27 @@ async def _set_selected_position(self, position: InOut) -> None: await self._check_beamstop_position() - async def move_to_new_position(): - if position == InOut.OUT: - await self.y_mm.set(self._scintillator_out_yz_mm[0]) - await self.z_mm.set(self._scintillator_out_yz_mm[1]) - elif position == InOut.IN: - await self.z_mm.set(self._scintillator_in_yz_mm[1]) - await self.y_mm.set(self._scintillator_in_yz_mm[0]) - match position: case InOut.OUT | InOut.IN: - await self.do_with_ap_sg_in_safe_pos(move_to_new_position) + await self.do_with_aperture_scatterguard_in_safe_pos( + partial(self._move_to_new_position, position) + ) case _: raise ValueError(f"Cannot set scintillator to position {position}") - async def do_with_ap_sg_in_safe_pos(self, func): + async def _move_to_new_position(self, position): + if position == InOut.OUT: + await self.y_mm.set(self._scintillator_out_yz_mm[0]) + await self.z_mm.set(self._scintillator_out_yz_mm[1]) + elif position == InOut.IN: + await self.z_mm.set(self._scintillator_in_yz_mm[1]) + await self.y_mm.set(self._scintillator_in_yz_mm[0]) + + async def do_with_aperture_scatterguard_in_safe_pos(self, func): + """Perform the supplied function with the aperture-scatterguard to the SCIN_MOVE position, + then restore to its previous position. + Args: + func: The async function to be applied""" scin_move_positions = self._aperture_scatterguard().get_scin_move_position() saved_positions: dict[Motor, float] = { motor: await motor.user_readback.get_value() diff --git a/tests/devices/test_aperture_scatterguard.py b/tests/devices/test_aperture_scatterguard.py index 70ba922e0a..d0d329d245 100644 --- a/tests/devices/test_aperture_scatterguard.py +++ b/tests/devices/test_aperture_scatterguard.py @@ -592,7 +592,7 @@ async def set_motor_moving(value, *args, **kwargs): ApertureValue.PARKED, ], ) -async def test_restore_ap_sg_from_scin_move_position( +async def test_get_scin_move_position_returns_expected( aperture_in_medium_pos: ApertureScatterguard, ap_sg_configuration: ApertureScatterguardConfiguration, selected_aperture: ApertureValue, @@ -602,5 +602,10 @@ async def test_restore_ap_sg_from_scin_move_position( await set_underlying_motors(ap_sg, ap_sg_configuration, selected_aperture) positions = ap_sg.get_scin_move_position() - assert positions[ap_sg.aperture.x] == ap_sg_configuration.scintillator_move_miniap_x - assert positions[ap_sg.scatterguard.x] == ap_sg_configuration.scintillator_move_sg_x + assert ( + positions[ap_sg.aperture.x] == ap_sg_configuration.scintillator_move_aperture_x + ) + assert ( + positions[ap_sg.scatterguard.x] + == ap_sg_configuration.scintillator_move_scatterguard_x + ) From db024457fa88159a34fa53b16ddcdbef40b41f7e Mon Sep 17 00:00:00 2001 From: Robert Tuck Date: Mon, 26 Jan 2026 17:44:18 +0000 Subject: [PATCH 10/16] Make diagram background opaque but even more so --- docs/images/scintillator-motion.svg | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/docs/images/scintillator-motion.svg b/docs/images/scintillator-motion.svg index ab4e78e7e7..2230fa73ab 100644 --- a/docs/images/scintillator-motion.svg +++ b/docs/images/scintillator-motion.svg @@ -151,10 +151,10 @@ inkscape:document-rotation="0" showgrid="false" inkscape:snap-page="true" - inkscape:window-width="1682" - inkscape:window-height="863" - inkscape:window-x="2112" - inkscape:window-y="174" + inkscape:window-width="1877" + inkscape:window-height="1160" + inkscape:window-x="2109" + inkscape:window-y="153" inkscape:window-maximized="0" fit-margin-top="0" fit-margin-left="0" @@ -180,6 +180,13 @@ + Date: Tue, 3 Feb 2026 15:23:57 +0000 Subject: [PATCH 11/16] Clarify scintillator motion diagram --- docs/images/scintillator-motion.svg | 304 ++++++++++++---------------- 1 file changed, 128 insertions(+), 176 deletions(-) diff --git a/docs/images/scintillator-motion.svg b/docs/images/scintillator-motion.svg index 2230fa73ab..29edf9349a 100644 --- a/docs/images/scintillator-motion.svg +++ b/docs/images/scintillator-motion.svg @@ -63,7 +63,7 @@ inkscape:vp_x="-95.254798 : 40.454221 : 0" inkscape:vp_y="0 : 253.16582 : 0" inkscape:vp_z="39.932888 : 23.055262 : 0" - inkscape:persp3d-origin="136.82452 : 94.15818 : 1" + inkscape:persp3d-origin="130.6413 : 75.386411 : 1" id="perspective10-3-2" /> image/svg+xml - + @@ -238,182 +238,134 @@ points="93.499976,94.721103 93.499976,182.56201 89.245379,180.7551 89.245379,92.914198 " /> - - - - - - - - - - - - - - + id="g1033" + transform="translate(-6.1832179,18.771769)"> + + + + + + + + + + + + + + + + + + + - - - - - - - - - - - Date: Tue, 3 Feb 2026 15:26:57 +0000 Subject: [PATCH 12/16] Update docstring in src/dodal/devices/scintillator.py Co-authored-by: olliesilvester <122091460+olliesilvester@users.noreply.github.com> --- src/dodal/devices/scintillator.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/dodal/devices/scintillator.py b/src/dodal/devices/scintillator.py index 5ed17d2551..5ad1d6ed3b 100644 --- a/src/dodal/devices/scintillator.py +++ b/src/dodal/devices/scintillator.py @@ -116,8 +116,8 @@ async def _move_to_new_position(self, position): await self.y_mm.set(self._scintillator_in_yz_mm[0]) async def do_with_aperture_scatterguard_in_safe_pos(self, func): - """Perform the supplied function with the aperture-scatterguard to the SCIN_MOVE position, - then restore to its previous position. + """Move the aperture-scatterguard to SCIN_MOVE position, do the supplied function, + then restore aperture-scatterguard to its previous position. Args: func: The async function to be applied""" scin_move_positions = self._aperture_scatterguard().get_scin_move_position() From 5f241d3299dd18fff5902bcfee83cc427f3d29af Mon Sep 17 00:00:00 2001 From: Robert Tuck Date: Tue, 3 Feb 2026 15:29:54 +0000 Subject: [PATCH 13/16] Simplify get_scin_move_position test --- tests/devices/test_aperture_scatterguard.py | 12 ------------ 1 file changed, 12 deletions(-) diff --git a/tests/devices/test_aperture_scatterguard.py b/tests/devices/test_aperture_scatterguard.py index d0d329d245..f60b8a7e7b 100644 --- a/tests/devices/test_aperture_scatterguard.py +++ b/tests/devices/test_aperture_scatterguard.py @@ -582,24 +582,12 @@ async def set_motor_moving(value, *args, **kwargs): await aperture_in_medium_pos.selected_aperture.set(ApertureValue.SMALL) -@pytest.mark.parametrize( - "selected_aperture", - [ - ApertureValue.SMALL, - ApertureValue.MEDIUM, - ApertureValue.LARGE, - ApertureValue.OUT_OF_BEAM, - ApertureValue.PARKED, - ], -) async def test_get_scin_move_position_returns_expected( aperture_in_medium_pos: ApertureScatterguard, ap_sg_configuration: ApertureScatterguardConfiguration, - selected_aperture: ApertureValue, run_engine: RunEngine, ): ap_sg = aperture_in_medium_pos - await set_underlying_motors(ap_sg, ap_sg_configuration, selected_aperture) positions = ap_sg.get_scin_move_position() assert ( From f05343f0e95d8c4a1ce07bdca4c3f79ace99e3bd Mon Sep 17 00:00:00 2001 From: Robert Tuck Date: Wed, 4 Feb 2026 17:42:27 +0000 Subject: [PATCH 14/16] Refactor of aperture-scatterguard, scintillator as suggested in PR review --- src/dodal/beamlines/i03.py | 20 +- src/dodal/beamlines/i04.py | 20 +- src/dodal/devices/beamlines/i03/beamsize.py | 2 +- src/dodal/devices/beamlines/i04/beamsize.py | 2 +- src/dodal/devices/fast_grid_scan.py.orig | 484 ++++++++++++++++++ .../{ => mx_phase1}/aperturescatterguard.py | 23 + .../devices/{ => mx_phase1}/scintillator.py | 105 ++-- src/dodal/devices/smargon.py.orig | 134 +++++ tests/devices/beamlines/i03/test_beamsize.py | 4 +- tests/devices/beamlines/i04/test_beamsize.py | 6 +- tests/devices/conftest.py | 19 +- tests/devices/test_aperture_scatterguard.py | 67 ++- tests/devices/test_scintillator.py | 79 ++- 13 files changed, 844 insertions(+), 121 deletions(-) create mode 100644 src/dodal/devices/fast_grid_scan.py.orig rename src/dodal/devices/{ => mx_phase1}/aperturescatterguard.py (94%) rename src/dodal/devices/{ => mx_phase1}/scintillator.py (57%) create mode 100644 src/dodal/devices/smargon.py.orig diff --git a/src/dodal/beamlines/i03.py b/src/dodal/beamlines/i03.py index 52ff86dcd3..3897e2ffbd 100644 --- a/src/dodal/beamlines/i03.py +++ b/src/dodal/beamlines/i03.py @@ -1,6 +1,6 @@ from functools import cache -from ophyd_async.core import PathProvider, Reference +from ophyd_async.core import PathProvider from ophyd_async.fastcs.eiger import EigerDetector as FastEiger from ophyd_async.fastcs.panda import HDFPanda from yarl import URL @@ -9,11 +9,6 @@ from dodal.common.beamlines.beamline_utils import set_beamline as set_utils_beamline from dodal.common.udc_directory_provider import PandASubpathProvider from dodal.device_manager import DeviceManager -from dodal.devices.aperturescatterguard import ( - AperturePosition, - ApertureScatterguard, - load_configuration, -) from dodal.devices.attenuator.attenuator import BinaryFilterAttenuator from dodal.devices.backlight import Backlight from dodal.devices.baton import Baton @@ -37,13 +32,18 @@ from dodal.devices.hutch_shutter import HutchShutter from dodal.devices.ipin import IPin from dodal.devices.motors import XYZStage +from dodal.devices.mx_phase1.aperturescatterguard import ( + AperturePosition, + ApertureScatterguard, + load_configuration, +) +from dodal.devices.mx_phase1.scintillator import Scintillator from dodal.devices.oav.oav_detector import OAVBeamCentreFile from dodal.devices.oav.oav_parameters import OAVConfigBeamCentre from dodal.devices.oav.pin_image_recognition import PinTipDetection from dodal.devices.qbpm import QBPM from dodal.devices.robot import BartRobot from dodal.devices.s4_slit_gaps import S4SlitGaps -from dodal.devices.scintillator import Scintillator from dodal.devices.smargon import Smargon from dodal.devices.synchrotron import Synchrotron from dodal.devices.thawer import Thawer @@ -342,13 +342,9 @@ def fluorescence_det_motion() -> FluorescenceDetector: @devices.factory() -def scintillator( - aperture_scatterguard: ApertureScatterguard, beamstop: Beamstop -) -> Scintillator: +def scintillator() -> Scintillator: return Scintillator( f"{PREFIX.beamline_prefix}-MO-SCIN-01:", - Reference(aperture_scatterguard), - Reference(beamstop), get_beamline_parameters(), ) diff --git a/src/dodal/beamlines/i04.py b/src/dodal/beamlines/i04.py index 56c8fb834c..6eaf32b024 100644 --- a/src/dodal/beamlines/i04.py +++ b/src/dodal/beamlines/i04.py @@ -1,13 +1,6 @@ -from ophyd_async.core import Reference - from dodal.common.beamlines.beamline_parameters import get_beamline_parameters from dodal.common.beamlines.beamline_utils import set_beamline as set_utils_beamline from dodal.device_manager import DeviceManager -from dodal.devices.aperturescatterguard import ( - AperturePosition, - ApertureScatterguard, - load_configuration, -) from dodal.devices.attenuator.attenuator import BinaryFilterAttenuator from dodal.devices.backlight import Backlight from dodal.devices.baton import Baton @@ -25,7 +18,13 @@ from dodal.devices.flux import Flux from dodal.devices.ipin import IPin from dodal.devices.motors import XYZStage +from dodal.devices.mx_phase1.aperturescatterguard import ( + AperturePosition, + ApertureScatterguard, + load_configuration, +) from dodal.devices.mx_phase1.beamstop import Beamstop +from dodal.devices.mx_phase1.scintillator import Scintillator from dodal.devices.oav.oav_detector import ( OAVBeamCentrePV, ZoomControllerWithBeamCentres, @@ -35,7 +34,6 @@ from dodal.devices.oav.pin_image_recognition import PinTipDetection from dodal.devices.robot import BartRobot from dodal.devices.s4_slit_gaps import S4SlitGaps -from dodal.devices.scintillator import Scintillator from dodal.devices.smargon import Smargon from dodal.devices.synchrotron import Synchrotron from dodal.devices.thawer import Thawer @@ -277,13 +275,9 @@ def pin_tip_detection() -> PinTipDetection: @devices.factory() -def scintillator( - aperture_scatterguard: ApertureScatterguard, beamstop: Beamstop -) -> Scintillator: +def scintillator() -> Scintillator: return Scintillator( f"{PREFIX.beamline_prefix}-MO-SCIN-01:", - Reference(aperture_scatterguard), - Reference(beamstop), get_beamline_parameters(), ) diff --git a/src/dodal/devices/beamlines/i03/beamsize.py b/src/dodal/devices/beamlines/i03/beamsize.py index 40224587da..8e29f65c25 100644 --- a/src/dodal/devices/beamlines/i03/beamsize.py +++ b/src/dodal/devices/beamlines/i03/beamsize.py @@ -1,8 +1,8 @@ from ophyd_async.core import Reference, derived_signal_r -from dodal.devices.aperturescatterguard import ApertureScatterguard from dodal.devices.beamlines.i03.constants import BeamsizeConstants from dodal.devices.beamsize.beamsize import BeamsizeBase +from dodal.devices.mx_phase1.aperturescatterguard import ApertureScatterguard class Beamsize(BeamsizeBase): diff --git a/src/dodal/devices/beamlines/i04/beamsize.py b/src/dodal/devices/beamlines/i04/beamsize.py index c1de9cc7ef..9935c2592c 100644 --- a/src/dodal/devices/beamlines/i04/beamsize.py +++ b/src/dodal/devices/beamlines/i04/beamsize.py @@ -1,8 +1,8 @@ from ophyd_async.core import Reference, derived_signal_r -from dodal.devices.aperturescatterguard import ApertureScatterguard from dodal.devices.beamlines.i04.transfocator import Transfocator from dodal.devices.beamsize.beamsize import BeamsizeBase +from dodal.devices.mx_phase1.aperturescatterguard import ApertureScatterguard class Beamsize(BeamsizeBase): diff --git a/src/dodal/devices/fast_grid_scan.py.orig b/src/dodal/devices/fast_grid_scan.py.orig new file mode 100644 index 0000000000..0bda3bef47 --- /dev/null +++ b/src/dodal/devices/fast_grid_scan.py.orig @@ -0,0 +1,484 @@ +import asyncio +from abc import ABC, abstractmethod +from functools import partial +from math import isclose +from typing import Generic, TypeVar + +import numpy as np +from bluesky.plan_stubs import prepare +from bluesky.protocols import Flyable, Preparable +from numpy import ndarray +from ophyd_async.core import ( + AsyncStatus, + Device, + Signal, + SignalR, + SignalRW, + StandardReadable, + derived_signal_r, + set_and_wait_for_value, + soft_signal_r_and_setter, + wait_for_value, +) +from ophyd_async.epics.core import ( + epics_signal_r, + epics_signal_rw, + epics_signal_rw_rbv, + epics_signal_x, +) +from pydantic import BaseModel, field_validator +from pydantic.dataclasses import dataclass + +from dodal.log import LOGGER +from dodal.parameters.experiment_parameter_base import AbstractExperimentWithBeamParams + + +class GridScanInvalidError(RuntimeError): + """Raised when the gridscan parameters are not valid.""" + + +@dataclass +class GridAxis: + start: float + step_size_mm: float + full_steps: int + + def steps_to_motor_position(self, steps): + """Gives the motor position based on steps, where steps are 0 indexed.""" + return self.start + self.step_size_mm * steps + + @property + def end(self): + """Gives the point where the final frame is taken.""" + # Note that full_steps is one indexed e.g. if there is one step then the end is + # refering to the first position + return self.steps_to_motor_position(self.full_steps - 1) + + def is_within(self, steps: float): + """Determine whether a single axis coordinate is within the grid. + The coordinate is from a continuous coordinate space based on the + XRC grid where the origin corresponds to the centre of the first grid box. + + Args: + steps (float): The coordinate to check. + + Returns: + bool: True if the coordinate falls within the grid. + """ + return -0.5 <= steps <= self.full_steps - 0.5 + + +class GridScanParamsCommon(AbstractExperimentWithBeamParams): + """Common holder class for the parameters of a grid scan in a similar + layout to EPICS. The parameters and functions of this class are common + to both the zebra and panda triggered fast grid scans in 2d or 3d. + + The grid specified is where data is taken e.g. it can be assumed the first frame is + at x_start, y1_start, z1_start and subsequent frames are N*step_size away. + """ + + x_steps: int = 1 + y_steps: int = 1 + x_step_size_mm: float = 0.1 + y_step_size_mm: float = 0.1 + x_start_mm: float = 0.1 + y1_start_mm: float = 0.1 + z1_start_mm: float = 0.1 + + # Whether to set the stub offsets after centering + set_stub_offsets: bool = False + + @property + def x_axis(self) -> GridAxis: + return GridAxis(self.x_start_mm, self.x_step_size_mm, self.x_steps) + + @property + def y_axis(self) -> GridAxis: + return GridAxis(self.y1_start_mm, self.y_step_size_mm, self.y_steps) + + # In 2D grid scans, z axis is just the start position + @property + def z_axis(self) -> GridAxis: + return GridAxis(self.z1_start_mm, 0, 1) + + def grid_position_to_motor_position(self, grid_position: ndarray) -> ndarray: + """Converts a grid position, given as steps in the x, y, z grid, + to a real motor position. + + Args: + grid_position (ndarray): The x, y, z position in grid steps. The origin is + at the centre of the first grid box + + Returns: + ndarray: The motor position this corresponds to. + + Raises: + IndexError if the desired position is outside the grid. + """ + for position, axis in zip( + grid_position, [self.x_axis, self.y_axis, self.z_axis], strict=False + ): + if not axis.is_within(position): + raise IndexError(f"{grid_position} is outside the bounds of the grid") + + return np.array( + [ + self.x_axis.steps_to_motor_position(grid_position[0]), + self.y_axis.steps_to_motor_position(grid_position[1]), + self.z_axis.steps_to_motor_position(grid_position[2]), + ] + ) + + +class GridScanParamsThreeD(GridScanParamsCommon): + """Additional parameters required to do a 3 dimensional gridscan. + + A 3D gridscan works by doing two 2D gridscans. The first of these grids is x_steps + by y_steps. The sample is then rotated by 90 degrees, and then the second grid is + x_steps by z_steps. + """ + + # Start position for z and y during the second gridscan + z2_start_mm: float = 0.1 + y2_start_mm: float = 0.1 + + z_step_size_mm: float = 0.1 + + # Number of vertical steps during the second grid scan, after the rotation in omega + z_steps: int = 1 + + @property + def z_axis(self) -> GridAxis: + return GridAxis(self.z2_start_mm, self.z_step_size_mm, self.z_steps) + + +ParamType = TypeVar("ParamType", bound=GridScanParamsCommon) + + +class WithDwellTime(BaseModel): + dwell_time_ms: float = 213 + + @field_validator("dwell_time_ms") + @classmethod + def non_integer_dwell_time(cls, dwell_time_ms: float) -> float: + dwell_time_floor_rounded = np.floor(dwell_time_ms) + dwell_time_is_close = np.isclose( + dwell_time_ms, dwell_time_floor_rounded, rtol=1e-3 + ) + if not dwell_time_is_close: + raise ValueError( + f"Dwell time of {dwell_time_ms}ms is not an integer value. Fast Grid Scan only accepts integer values" + ) + return dwell_time_ms + + +class ZebraGridScanParamsThreeD(GridScanParamsThreeD, WithDwellTime): + """Params for standard Zebra FGS. Adds on the dwell time, which is really the time + between trigger positions. + """ + + +class PandAGridScanParams(GridScanParamsThreeD): + """Params for panda constant-motion scan. Adds on the goniometer run-up distance.""" + + run_up_distance_mm: float = 0.17 + + +class MotionProgram(Device): + def __init__(self, prefix: str, name: str = "", has_prog_num=True) -> None: + super().__init__(name) + self.running = epics_signal_r(int, prefix + "PROGBITS") + if has_prog_num: + self.program_number = epics_signal_r(float, prefix + "CS1:PROG_NUM") + else: + # Prog number PV doesn't currently exist for i02-1 + self.program_number = soft_signal_r_and_setter(float, -1)[0] + + +class FastGridScanCommon( + StandardReadable, Flyable, ABC, Preparable, Generic[ParamType] +): + """Device containing the minimal signals for a general fast grid scan. + + When the motion program is started, the goniometer will move in a snake-like grid + trajectory, with X as the fast axis and Y as the slow axis. + + See ZebraFastGridScanThreeD as an example of how to implement. + """ + + def __init__( + self, prefix: str, motion_controller_prefix: str, name: str = "" + ) -> None: + super().__init__(name) + self.x_steps = epics_signal_rw_rbv(int, f"{prefix}X_NUM_STEPS") + self.y_steps = epics_signal_rw_rbv( + int, f"{prefix}Y_NUM_STEPS" + ) # Number of vertical steps during the first grid scan + self.x_step_size = epics_signal_rw_rbv(float, f"{prefix}X_STEP_SIZE") + self.y_step_size = epics_signal_rw_rbv(float, f"{prefix}Y_STEP_SIZE") + self.x_start = epics_signal_rw_rbv(float, f"{prefix}X_START") + self.y1_start = epics_signal_rw_rbv(float, f"{prefix}Y_START") + self.z1_start = epics_signal_rw_rbv(float, f"{prefix}Z_START") + + # This can be created like a regular signal instead of an abstract method + # once https://github.com/DiamondLightSource/mx-bluesky/issues/1203 is done + self.scan_invalid = self._create_scan_invalid_signal(prefix) + + self.run_cmd = epics_signal_x(f"{prefix}RUN.PROC") + self.stop_cmd = epics_signal_x(f"{prefix}STOP.PROC") + self.status = epics_signal_r(int, f"{prefix}SCAN_STATUS") + + self.expected_images = self._create_expected_images_signal() + + self.motion_program = self._create_motion_program(motion_controller_prefix) + + self.position_counter = self._create_position_counter(prefix) + + # Kickoff timeout in seconds + self.KICKOFF_TIMEOUT: float = 5.0 + + self.COMPLETE_STATUS: float = 60.0 + self.VALIDITY_CHECK_TIMEOUT = 0.5 + + self._movable_params: dict[str, Signal] = { + "x_steps": self.x_steps, + "y_steps": self.y_steps, + "x_step_size_mm": self.x_step_size, + "y_step_size_mm": self.y_step_size, + "x_start_mm": self.x_start, + "y1_start_mm": self.y1_start, + "z1_start_mm": self.z1_start, + } + + @AsyncStatus.wrap + async def kickoff(self): + curr_prog = await self.motion_program.program_number.get_value() + running = await self.motion_program.running.get_value() + if running: + LOGGER.info(f"Motion program {curr_prog} still running, waiting...") + await wait_for_value(self.motion_program.running, 0, self.KICKOFF_TIMEOUT) + + LOGGER.debug("Running scan") + await self.run_cmd.trigger() + LOGGER.info("Waiting for FGS to start") + await wait_for_value(self.status, 1, self.KICKOFF_TIMEOUT) + LOGGER.debug("FGS kicked off") + + @AsyncStatus.wrap + async def complete(self): + try: + await wait_for_value(self.status, 0, self.COMPLETE_STATUS) + except TimeoutError: + LOGGER.error( + "Hyperion timed out waiting for FGS motion to complete. This may have been caused by a goniometer stage getting stuck.\n\ + Forcibly stopping the FGS motion program..." + ) + await self.stop_cmd.trigger() + raise + + @abstractmethod + def _create_expected_images_signal(self) -> SignalR[int]: ... + + @abstractmethod + def _create_position_counter(self, prefix: str) -> SignalRW[int]: ... + + # This can be created within init rather than as a separate method after https://github.com/DiamondLightSource/mx-bluesky/issues/1203 + @abstractmethod + def _create_scan_invalid_signal(self, prefix: str) -> SignalR[float]: ... + + # This can be created within init rather than as a separate method after https://github.com/DiamondLightSource/mx-bluesky/issues/1203 + @abstractmethod + def _create_motion_program( + self, motion_controller_prefix: str + ) -> MotionProgram: ... + + @AsyncStatus.wrap + async def prepare(self, value: ParamType): + """Submit the gridscan parameters to the device for validation prior to + gridscan kickoff. + + Args: + value (ParamType): The gridscan parameters. + + Raises: + GridScanInvalidError: If the gridscan parameters were not valid. + """ + set_statuses = [] + + LOGGER.info("Applying gridscan parameters...") + + # Create arguments for bps.mv + for key, signal in self._movable_params.items(): + param_value = value.__dict__[key] + + matcher = partial(isclose, param_value, abs_tol=0.001) + + set_statuses.append( + set_and_wait_for_value( + signal, # type: ignore + param_value, + match_value=matcher, + ) + ) + + # Counter should always start at 0 + set_statuses.append(set_and_wait_for_value(self.position_counter, 0)) + + LOGGER.info("Gridscan parameters applied, waiting for sets to complete...") + + # wait for parameter sets to complete + await asyncio.gather(*set_statuses) + + LOGGER.info("Sets confirmed, waiting for validity checks to pass...") + try: + await wait_for_value( + self.scan_invalid, 0.0, timeout=self.VALIDITY_CHECK_TIMEOUT + ) + except TimeoutError as e: + raise GridScanInvalidError( + f"Gridscan parameters not validated after {self.VALIDITY_CHECK_TIMEOUT}s" + ) from e + + LOGGER.info("Gridscan validity confirmed, gridscan is now prepared.") + + +class FastGridScanThreeD(FastGridScanCommon[ParamType]): + """Device for standard 3D FGS. + + After completeing the first grid, if Z steps isn't 0, the goniometer will + rotate in the omega direction such that it moves from the X-Y, to the X-Z plane then + do a second grid scan. The detector is triggered after every x step. + See https://github.com/DiamondLightSource/hyperion/wiki/Coordinate-Systems for more. + + Subclasses must implement _create_position_counter. + """ + + def __init__(self, prefix: str, infix: str, name: str = "") -> None: + full_prefix = prefix + infix + + # Number of vertical steps during the second grid scan, after the rotation in omega + self.z_steps = epics_signal_rw_rbv(int, f"{full_prefix}Z_NUM_STEPS") + self.z_step_size = epics_signal_rw_rbv(float, f"{full_prefix}Z_STEP_SIZE") + self.z2_start = epics_signal_rw_rbv(float, f"{full_prefix}Z2_START") + self.y2_start = epics_signal_rw_rbv(float, f"{full_prefix}Y2_START") + # panda does not have x counter + self.y_counter = epics_signal_r(int, f"{full_prefix}Y_COUNTER") + + super().__init__(full_prefix, prefix, name) + + self._movable_params["z_step_size_mm"] = self.z_step_size + self._movable_params["z2_start_mm"] = self.z2_start + self._movable_params["y2_start_mm"] = self.y2_start + self._movable_params["z_steps"] = self.z_steps + + def _create_expected_images_signal(self): + return derived_signal_r( + self._calculate_expected_images, + x=self.x_steps, + y=self.y_steps, + z=self.z_steps, + ) + + def _calculate_expected_images(self, x: int, y: int, z: int) -> int: + LOGGER.info(f"Reading num of images found {x, y, z} images in each axis") + first_grid = x * y + second_grid = x * z + return first_grid + second_grid + + def _create_scan_invalid_signal(self, prefix: str) -> SignalR[float]: + self.x_scan_valid = epics_signal_r(float, f"{prefix}X_SCAN_VALID") + self.y_scan_valid = epics_signal_r(float, f"{prefix}Y_SCAN_VALID") + self.z_scan_valid = epics_signal_r(float, f"{prefix}Z_SCAN_VALID") + self.device_scan_invalid = epics_signal_r(float, f"{prefix}SCAN_INVALID") + + def compute_derived_value( + x_scan_valid: float, + y_scan_valid: float, + z_scan_valid: float, + device_scan_invalid: float, + ) -> float: + return ( + 1.0 + if not ( + x_scan_valid + and y_scan_valid + and z_scan_valid + and not device_scan_invalid + ) + else 0.0 + ) + + return derived_signal_r( + compute_derived_value, + x_scan_valid=self.x_scan_valid, + y_scan_valid=self.y_scan_valid, + z_scan_valid=self.z_scan_valid, + device_scan_invalid=self.device_scan_invalid, + ) + + def _create_motion_program(self, motion_controller_prefix: str): + return MotionProgram(motion_controller_prefix) + + +class ZebraFastGridScanThreeD(FastGridScanThreeD[ZebraGridScanParamsThreeD]): + """Device for standard Zebra 3D FGS. + + In this scan, the goniometer's velocity profile follows a parabolic shape between X + steps, with the slowest points occuring at each X step. + """ + + def __init__(self, prefix: str, name: str = "") -> None: + infix = "FGS:" + full_prefix = prefix + infix + # Time taken to travel between X steps + self.dwell_time_ms = epics_signal_rw_rbv(float, f"{full_prefix}DWELL_TIME") + self.x_counter = epics_signal_r(int, f"{full_prefix}X_COUNTER") + super().__init__(prefix, infix, name) + self._movable_params["dwell_time_ms"] = self.dwell_time_ms + + def _create_position_counter(self, prefix: str): + return epics_signal_rw( + int, f"{prefix}POS_COUNTER", write_pv=f"{prefix}POS_COUNTER_WRITE" + ) + + +class PandAFastGridScan(FastGridScanThreeD[PandAGridScanParams]): + """Device for panda constant-motion scan. + + In this scan, the goniometer's velocity is constant through each row. It doesn't + slow down when going through trigger points. + """ + + def __init__(self, prefix: str, name: str = "") -> None: + infix = "PGS:" + full_prefix = prefix + infix + self.time_between_x_steps_ms = ( + epics_signal_rw_rbv( # Used by motion controller to set goniometer velocity + float, f"{full_prefix}TIME_BETWEEN_X_STEPS" + ) + ) + + # Distance before and after the grid given to allow goniometer to reach desired speed while it is within the + # grid + self.run_up_distance_mm = epics_signal_rw_rbv( + float, f"{full_prefix}RUNUP_DISTANCE" + ) + super().__init__(prefix, infix, name) + + self._movable_params["run_up_distance_mm"] = self.run_up_distance_mm + + def _create_position_counter(self, prefix: str): + return epics_signal_rw(int, f"{prefix}Y_COUNTER") + + +def set_fast_grid_scan_params(scan: FastGridScanCommon[ParamType], params: ParamType): + """Apply the fast grid scan parameters to the grid scan device and validate them. + + Args: + scan (FastGridScancommon[ParamType]): The fast grid scan device. + params (ParamType): The parameters to set. + + Raises: + GridScanInvalidError: if the grid scan parameters are not valid. + """ + yield from prepare(scan, params, wait=True) diff --git a/src/dodal/devices/aperturescatterguard.py b/src/dodal/devices/mx_phase1/aperturescatterguard.py similarity index 94% rename from src/dodal/devices/aperturescatterguard.py rename to src/dodal/devices/mx_phase1/aperturescatterguard.py index 685909753a..7fa6957dcb 100644 --- a/src/dodal/devices/aperturescatterguard.py +++ b/src/dodal/devices/mx_phase1/aperturescatterguard.py @@ -2,9 +2,13 @@ import asyncio import dataclasses +from collections.abc import Callable from math import inf +import bluesky.plan_stubs as bps +from bluesky.preprocessors import contingency_wrapper from bluesky.protocols import Preparable +from bluesky.utils import MsgGenerator from ophyd_async.core import ( AsyncStatus, StandardReadable, @@ -401,3 +405,22 @@ def get_scin_move_position(self) -> dict[Motor, float]: self.aperture.x: self._config.scintillator_move_aperture_x, self.scatterguard.x: self._config.scintillator_move_scatterguard_x, } + + +def do_with_aperture_scatterguard_in_scin_move_position( + aperture_scatterguard: ApertureScatterguard, inner_plan: Callable[[], MsgGenerator] +) -> MsgGenerator: + motors_and_safe_moves = aperture_scatterguard.get_scin_move_position() + saved_positions = {} + for motor in motors_and_safe_moves: + readback_ = yield from bps.rd(motor.user_readback) + saved_positions[motor] = readback_ + + for motor, position in motors_and_safe_moves.items(): + yield from bps.mv(motor, position) + + def restore_previous_position(): + for motor, position in saved_positions.items(): + yield from bps.mv(motor, position) + + yield from contingency_wrapper(inner_plan(), else_plan=restore_previous_position) diff --git a/src/dodal/devices/scintillator.py b/src/dodal/devices/mx_phase1/scintillator.py similarity index 57% rename from src/dodal/devices/scintillator.py rename to src/dodal/devices/mx_phase1/scintillator.py index 5ad1d6ed3b..5797ad0408 100644 --- a/src/dodal/devices/scintillator.py +++ b/src/dodal/devices/mx_phase1/scintillator.py @@ -1,11 +1,15 @@ -from functools import partial from math import isclose -from ophyd_async.core import Reference, StandardReadable, StrictEnum, derived_signal_rw +import bluesky.plan_stubs as bps +from bluesky.utils import MsgGenerator +from ophyd_async.core import StandardReadable, StrictEnum, derived_signal_r from ophyd_async.epics.motor import Motor from dodal.common.beamlines.beamline_parameters import GDABeamlineParameters -from dodal.devices.aperturescatterguard import ApertureScatterguard +from dodal.devices.mx_phase1.aperturescatterguard import ( + ApertureScatterguard, + do_with_aperture_scatterguard_in_scin_move_position, +) from dodal.devices.mx_phase1.beamstop import Beamstop, BeamstopPositions @@ -25,28 +29,27 @@ class Scintillator(StandardReadable): When moved out of the beam it is parked under the table. This parking has a potential to collide with the aperture/scatterguard if that is not correctly parked already. + + The scintillator does not supports reading the current position but not writing to it; + due to potential collisions with the aperture-scatterguard. Instead a bluesky plan is provided + which moves the aperture-scatterguard to a safe position for the duration of the move. """ def __init__( self, prefix: str, - aperture_scatterguard: Reference[ApertureScatterguard], - beamstop: Reference[Beamstop], beamline_parameters: GDABeamlineParameters, name: str = "", ): with self.add_children_as_readables(): self.y_mm = Motor(f"{prefix}Y") self.z_mm = Motor(f"{prefix}Z") - self.selected_pos = derived_signal_rw( + self.selected_pos = derived_signal_r( self._get_selected_position, - self._set_selected_position, y=self.y_mm, z=self.z_mm, ) - self._aperture_scatterguard = aperture_scatterguard - self._beamstop = beamstop self._scintillator_out_yz_mm = [ float(beamline_parameters[f"scin_{axis}_SCIN_OUT"]) for axis in ("y", "z") ] @@ -59,6 +62,44 @@ def __init__( super().__init__(name) + def move_scintillator_safely( + self, + aperture_scatterguard: ApertureScatterguard, + beamstop: Beamstop, + position: InOut, + ) -> MsgGenerator: + """Bluesky plan to move the scintillator which moves the aperture-scatterguard out of the way + for the duration of the move and then restores it. + + Args: + aperture_scatterguard (ApertureScatterguard): + The aperture-scatterguard device which will be moved + beamstop (Beamstop): + The beamstop device which will be checked for potential collisions + position (InOut): + The scintillator position to move to + """ + current_pos = yield from bps.rd(self.selected_pos) + if current_pos == position: + return + + def _move_to_new_position(): + if position == InOut.OUT: + yield from bps.mv(self.y_mm, self._scintillator_out_yz_mm[0]) + yield from bps.mv(self.z_mm, self._scintillator_out_yz_mm[1]) + elif position == InOut.IN: + yield from bps.mv(self.z_mm, self._scintillator_in_yz_mm[1]) + yield from bps.mv(self.y_mm, self._scintillator_in_yz_mm[0]) + + match position: + case InOut.OUT | InOut.IN: + yield from self._check_beamstop_position(beamstop) + yield from do_with_aperture_scatterguard_in_scin_move_position( + aperture_scatterguard, _move_to_new_position + ) + case _: + raise ValueError(f"Cannot set scintillator to position {position}") + def _check_position(self, current_pos: list[float], pos_to_check: list[float]): return all( isclose(axis_pos, axis_in_beam, abs_tol=axis_tolerance) @@ -81,8 +122,8 @@ def _get_selected_position(self, y: float, z: float) -> InOut: else: return InOut.UNKNOWN - async def _check_beamstop_position(self): - position = await self._beamstop().selected_pos.get_value() + def _check_beamstop_position(self, beamstop: Beamstop) -> MsgGenerator: + position = yield from bps.rd(beamstop.selected_pos) match position: case BeamstopPositions.OUT_OF_BEAM | BeamstopPositions.DATA_COLLECTION: return @@ -90,45 +131,3 @@ async def _check_beamstop_position(self): raise ValueError( f"Scintillator cannot be moved due to beamstop position {position}, must be in either in DATA_COLLECTION or OUT_OF_BEAM position." ) - - async def _set_selected_position(self, position: InOut) -> None: - current_y = await self.y_mm.user_readback.get_value() - current_z = await self.z_mm.user_readback.get_value() - if self._get_selected_position(current_y, current_z) == position: - return - - await self._check_beamstop_position() - - match position: - case InOut.OUT | InOut.IN: - await self.do_with_aperture_scatterguard_in_safe_pos( - partial(self._move_to_new_position, position) - ) - case _: - raise ValueError(f"Cannot set scintillator to position {position}") - - async def _move_to_new_position(self, position): - if position == InOut.OUT: - await self.y_mm.set(self._scintillator_out_yz_mm[0]) - await self.z_mm.set(self._scintillator_out_yz_mm[1]) - elif position == InOut.IN: - await self.z_mm.set(self._scintillator_in_yz_mm[1]) - await self.y_mm.set(self._scintillator_in_yz_mm[0]) - - async def do_with_aperture_scatterguard_in_safe_pos(self, func): - """Move the aperture-scatterguard to SCIN_MOVE position, do the supplied function, - then restore aperture-scatterguard to its previous position. - Args: - func: The async function to be applied""" - scin_move_positions = self._aperture_scatterguard().get_scin_move_position() - saved_positions: dict[Motor, float] = { - motor: await motor.user_readback.get_value() - for motor in scin_move_positions - } - for motor, pos in scin_move_positions.items(): - await motor.set(pos) - - await func() - # If func() fails then do not restore motors back to avoid potential collision - for motor, pos in saved_positions.items(): - await motor.set(pos) diff --git a/src/dodal/devices/smargon.py.orig b/src/dodal/devices/smargon.py.orig new file mode 100644 index 0000000000..6ca3e9dde9 --- /dev/null +++ b/src/dodal/devices/smargon.py.orig @@ -0,0 +1,134 @@ +import asyncio +from enum import Enum +from math import isclose +from typing import TypedDict, cast + +from bluesky.protocols import Movable +from ophyd_async.core import ( + AsyncStatus, + Device, + StrictEnum, + set_and_wait_for_value, + wait_for_value, +) +from ophyd_async.epics.core import epics_signal_r, epics_signal_rw +from ophyd_async.epics.motor import Motor + +from dodal.devices.motors import XYZOmegaStage +from dodal.devices.util.epics_util import SetWhenEnabled + + +class StubPosition(Enum): + CURRENT_AS_CENTER = 0 + RESET_TO_ROBOT_LOAD = 1 + + +def approx_equal_to(target, deadband: float = 1e-9): + def approx_equal_to_target(value): + return isclose(target, value, rel_tol=0, abs_tol=deadband) + + return approx_equal_to_target + + +class StubOffsets(Device): + """Stub offsets are used to change the internal co-ordinate system of the smargon by + adding an offset to x, y, z. + This is useful as the smargon's centre of rotation is around (0, 0, 0). As such + changing stub offsets will change the centre of rotation. + In practice we don't need to change them manually, instead there are helper PVs to + set them so that the current position is zero or to pre-defined positions. + """ + + def __init__(self, prefix: str, name: str = ""): + self.center_at_current_position = SetWhenEnabled(prefix=prefix + "CENTER_CS") + self.to_robot_load = SetWhenEnabled(prefix=prefix + "SET_STUBS_TO_RL") + super().__init__(name) + + @AsyncStatus.wrap + async def set(self, value: StubPosition): + if value == StubPosition.CURRENT_AS_CENTER: + await self.center_at_current_position.set(1) + smargon = cast(Smargon, self.parent) + await wait_for_value( + smargon.x.user_readback, approx_equal_to(0.0, 0.1), None + ) + await wait_for_value( + smargon.y.user_readback, approx_equal_to(0.0, 0.1), None + ) + await wait_for_value( + smargon.z.user_readback, approx_equal_to(0.0, 0.1), None + ) + else: + await self.to_robot_load.set(1) + + +class DeferMoves(StrictEnum): + ON = "Defer On" + OFF = "Defer Off" + + +class CombinedMove(TypedDict, total=False): + """A move on multiple axes at once using a deferred move.""" + + x: float | None + y: float | None + z: float | None + omega: float | None + phi: float | None + chi: float | None + + +class Smargon(XYZOmegaStage, Movable): + """Real motors added to allow stops following pin load (e.g. real_x1.stop() ) + X1 and X2 real motors provide compound chi motion as well as the compound X travel, + increasing the gap between x1 and x2 changes chi, moving together changes virtual x. + Robot loading can nudge these and lead to errors. + """ + + DEFERRED_MOVE_SET_TIMEOUT = 5 + + def __init__(self, prefix: str, name: str = ""): + with self.add_children_as_readables(): + self.chi = Motor(prefix + "CHI") + self.phi = Motor(prefix + "PHI") + self.real_x1 = Motor(prefix + "MOTOR_3") + self.real_x2 = Motor(prefix + "MOTOR_4") + self.real_y = Motor(prefix + "MOTOR_1") + self.real_z = Motor(prefix + "MOTOR_2") + self.real_phi = Motor(prefix + "MOTOR_5") + self.real_chi = Motor(prefix + "MOTOR_6") + self.stub_offsets = StubOffsets(prefix=prefix) + self.disabled = epics_signal_r(int, prefix + "DISABLED") + + self.defer_move = epics_signal_rw(DeferMoves, prefix + "CS1:DeferMoves") + + super().__init__(prefix, name) + + @AsyncStatus.wrap + async def set(self, value: CombinedMove): + """This will move all motion together in a deferred move. + + Once defer_move is on, sets to any axis do not immediately move the axis. Instead + the setpoint will go to that value. Then, when defer_move is switched off all + axes will move at the same time. The put callbacks on the axes themselves will + only come back after the motion on that axis finished. + """ + await self.defer_move.set(DeferMoves.ON) + try: + finished_moving = [] + for motor_name, new_setpoint in value.items(): + if new_setpoint is not None and isinstance(new_setpoint, int | float): + axis: Motor = getattr(self, motor_name) + await axis.check_motor_limit( + await axis.user_setpoint.get_value(), new_setpoint + ) + put_completion = await set_and_wait_for_value( + axis.user_setpoint, + new_setpoint, + timeout=self.DEFERRED_MOVE_SET_TIMEOUT, + wait_for_set_completion=False, + ) + finished_moving.append(put_completion) + finally: + await self.defer_move.set(DeferMoves.OFF) + await asyncio.gather(*finished_moving) diff --git a/tests/devices/beamlines/i03/test_beamsize.py b/tests/devices/beamlines/i03/test_beamsize.py index d76db1c5c7..2bb8aca4d8 100644 --- a/tests/devices/beamlines/i03/test_beamsize.py +++ b/tests/devices/beamlines/i03/test_beamsize.py @@ -4,10 +4,10 @@ import pytest from ophyd_async.core import set_mock_value -from dodal.devices.aperturescatterguard import ( +from dodal.devices.beamlines.i03.beamsize import Beamsize +from dodal.devices.mx_phase1.aperturescatterguard import ( ApertureScatterguard, ) -from dodal.devices.beamlines.i03.beamsize import Beamsize @pytest.mark.parametrize( diff --git a/tests/devices/beamlines/i04/test_beamsize.py b/tests/devices/beamlines/i04/test_beamsize.py index 8f2916003d..12aeefb5e1 100644 --- a/tests/devices/beamlines/i04/test_beamsize.py +++ b/tests/devices/beamlines/i04/test_beamsize.py @@ -4,11 +4,11 @@ import pytest from ophyd_async.core import set_mock_value -from dodal.devices.aperturescatterguard import ( - ApertureScatterguard, -) from dodal.devices.beamlines.i04.beamsize import Beamsize from dodal.devices.beamlines.i04.transfocator import Transfocator +from dodal.devices.mx_phase1.aperturescatterguard import ( + ApertureScatterguard, +) @pytest.mark.parametrize( diff --git a/tests/devices/conftest.py b/tests/devices/conftest.py index 9e7b0e631d..1a8eb0c4ef 100644 --- a/tests/devices/conftest.py +++ b/tests/devices/conftest.py @@ -1,10 +1,10 @@ import asyncio import pytest -from ophyd_async.core import init_devices +from ophyd_async.core import get_mock_put, init_devices from dodal.common.beamlines.beamline_parameters import GDABeamlineParameters -from dodal.devices.aperturescatterguard import ( +from dodal.devices.mx_phase1.aperturescatterguard import ( AperturePosition, ApertureScatterguard, ApertureScatterguardConfiguration, @@ -85,10 +85,15 @@ async def set_to_position( ): aperture_x, aperture_y, aperture_z, scatterguard_x, scatterguard_y = position.values + motors = [ + aperture_scatterguard.aperture.x, + aperture_scatterguard.aperture.y, + aperture_scatterguard.aperture.z, + aperture_scatterguard.scatterguard.x, + aperture_scatterguard.scatterguard.y, + ] await asyncio.gather( - aperture_scatterguard.aperture.x.set(aperture_x), - aperture_scatterguard.aperture.y.set(aperture_y), - aperture_scatterguard.aperture.z.set(aperture_z), - aperture_scatterguard.scatterguard.x.set(scatterguard_x), - aperture_scatterguard.scatterguard.y.set(scatterguard_y), + *[m.set(value) for m, value in zip(motors, position.values, strict=True)] ) + for signal in [m.user_setpoint for m in motors]: + get_mock_put(signal).reset_mock() diff --git a/tests/devices/test_aperture_scatterguard.py b/tests/devices/test_aperture_scatterguard.py index f60b8a7e7b..dfa59a1adf 100644 --- a/tests/devices/test_aperture_scatterguard.py +++ b/tests/devices/test_aperture_scatterguard.py @@ -6,6 +6,8 @@ import bluesky.plan_stubs as bps import pytest from bluesky.run_engine import RunEngine +from bluesky.simulators import RunEngineSimulator, assert_message_and_return_remaining +from bluesky.utils import MsgGenerator from ophyd_async.core import ( callback_on_mock_put, get_mock, @@ -13,12 +15,13 @@ set_mock_value, ) -from dodal.devices.aperturescatterguard import ( +from dodal.devices.mx_phase1.aperturescatterguard import ( AperturePosition, ApertureScatterguard, ApertureScatterguardConfiguration, ApertureValue, InvalidApertureMoveError, + do_with_aperture_scatterguard_in_scin_move_position, ) from tests.devices.conftest import set_to_position @@ -597,3 +600,65 @@ async def test_get_scin_move_position_returns_expected( positions[ap_sg.scatterguard.x] == ap_sg_configuration.scintillator_move_scatterguard_x ) + + +def test_do_with_aperture_scatterguard_in_scin_move_position_moves_and_restores_ap_sg( + sim_run_engine: RunEngineSimulator, + aperture_in_medium_pos: ApertureScatterguard, +): + ap_sg = aperture_in_medium_pos + + def my_plan() -> MsgGenerator: + yield from bps.sleep(1.0) + + sim_run_engine.add_read_handler_for(ap_sg.aperture.x.user_readback, 2.384) + sim_run_engine.add_read_handler_for(ap_sg.scatterguard.x.user_readback, 5.285) + msgs = sim_run_engine.simulate_plan( + do_with_aperture_scatterguard_in_scin_move_position(ap_sg, my_plan) + ) + + msgs = assert_message_and_return_remaining( + msgs, + lambda msg: msg.command == "set" + and msg.obj is ap_sg.aperture.x + and msg.args[0] == -4.91, + ) + msgs = assert_message_and_return_remaining( + msgs, + lambda msg: msg.command == "set" + and msg.obj is ap_sg.scatterguard.x + and msg.args[0] == -4.75, + ) + msgs = assert_message_and_return_remaining(msgs, lambda msg: msg.command == "sleep") + msgs = assert_message_and_return_remaining( + msgs, + lambda msg: msg.command == "set" + and msg.obj is ap_sg.aperture.x + and msg.args[0] == 2.384, + ) + assert_message_and_return_remaining( + msgs, + lambda msg: msg.command == "set" + and msg.obj is ap_sg.scatterguard.x + and msg.args[0] == 5.285, + ) + + +def test_do_with_aperture_scatterguard_in_scin_move_position_does_not_restore_on_inner_failure( + run_engine: RunEngine, + aperture_in_medium_pos: ApertureScatterguard, +): + ap_sg = aperture_in_medium_pos + + def my_plan() -> MsgGenerator: + raise AssertionError("simulated exception") + + with pytest.raises(AssertionError): + run_engine(do_with_aperture_scatterguard_in_scin_move_position(ap_sg, my_plan)) + + get_mock_put(ap_sg.aperture.x.user_setpoint).assert_called_once_with( + -4.91, wait=True + ) + get_mock_put(ap_sg.scatterguard.x.user_setpoint).assert_called_once_with( + -4.75, wait=True + ) diff --git a/tests/devices/test_scintillator.py b/tests/devices/test_scintillator.py index 33949610e7..cc245db725 100644 --- a/tests/devices/test_scintillator.py +++ b/tests/devices/test_scintillator.py @@ -2,16 +2,17 @@ from unittest.mock import AsyncMock, MagicMock, call import pytest -from ophyd_async.core import Reference, get_mock_put, init_devices, set_mock_value +from bluesky import RunEngine +from ophyd_async.core import get_mock_put, init_devices, set_mock_value from ophyd_async.testing import assert_value from dodal.common.beamlines.beamline_parameters import GDABeamlineParameters -from dodal.devices.aperturescatterguard import ( +from dodal.devices.mx_phase1.aperturescatterguard import ( ApertureScatterguard, ApertureValue, ) -from dodal.devices.mx_phase1.beamstop import Beamstop, BeamstopPositions -from dodal.devices.scintillator import InOut, Scintillator +from dodal.devices.mx_phase1.beamstop import Beamstop +from dodal.devices.mx_phase1.scintillator import InOut, Scintillator @pytest.fixture @@ -35,10 +36,19 @@ def mock_beamline_parameters() -> GDABeamlineParameters: @pytest.fixture -def beamstop(mock_beamline_parameters) -> Beamstop: +async def beamstop(mock_beamline_parameters) -> Beamstop: beamstop = Beamstop("-MO-BS-01:", mock_beamline_parameters, name="beamstop") - beamstop.selected_pos.get_value = AsyncMock( - return_value=BeamstopPositions.DATA_COLLECTION + async with init_devices(mock=True): + beamstop = Beamstop("", mock_beamline_parameters) + + set_mock_value( + beamstop.x_mm.user_readback, mock_beamline_parameters["in_beam_x_STANDARD"] + ) + set_mock_value( + beamstop.y_mm.user_readback, mock_beamline_parameters["in_beam_y_STANDARD"] + ) + set_mock_value( + beamstop.z_mm.user_readback, mock_beamline_parameters["in_beam_z_STANDARD"] ) return beamstop @@ -46,7 +56,6 @@ def beamstop(mock_beamline_parameters) -> Beamstop: @pytest.fixture async def scintillator_and_ap_sg( mock_beamline_parameters: GDABeamlineParameters, - beamstop: Beamstop, ap_sg: ApertureScatterguard, ) -> tuple[Scintillator, ApertureScatterguard]: async with init_devices(mock=True): @@ -56,8 +65,6 @@ async def scintillator_and_ap_sg( scintillator = Scintillator( prefix="", name="test_scin", - aperture_scatterguard=Reference(ap_sg), - beamstop=Reference(beamstop), beamline_parameters=mock_beamline_parameters, ) set_mock_value(ap_sg.aperture.x.user_readback, 1.0) @@ -90,23 +97,29 @@ async def test_given_at_positions_when_position_read_then_returns_expected( assert in_out == expected_position -async def test_when_set_to_unknown_position_then_error_raised( +def test_when_set_to_unknown_position_then_error_raised( scintillator_and_ap_sg: tuple[Scintillator, ApertureScatterguard], + beamstop: Beamstop, + run_engine: RunEngine, ): - scintillator, _ = scintillator_and_ap_sg - await scintillator.y_mm.set(100.855) - await scintillator.z_mm.set(101.5115) + scintillator, ap_sg = scintillator_and_ap_sg + set_mock_value(scintillator.y_mm.user_readback, 100.855) + set_mock_value(scintillator.z_mm.user_readback, 101.5115) with pytest.raises(ValueError): - await scintillator.selected_pos.set(InOut.UNKNOWN) + run_engine( + scintillator.move_scintillator_safely(ap_sg, beamstop, InOut.UNKNOWN) + ) async def test_given_aperture_scatterguard_parked_when_set_to_out_position_then_returns_expected( + run_engine: RunEngine, scintillator_and_ap_sg: tuple[Scintillator, ApertureScatterguard], + beamstop: Beamstop, ): scintillator, ap_sg = scintillator_and_ap_sg ap_sg.selected_aperture.get_value.return_value = ApertureValue.PARKED # type: ignore - await scintillator.selected_pos.set(InOut.OUT) + run_engine(scintillator.move_scintillator_safely(ap_sg, beamstop, InOut.OUT)) await assert_value(scintillator.y_mm.user_setpoint, -0.02) await assert_value(scintillator.z_mm.user_setpoint, 0.1) @@ -114,11 +127,13 @@ async def test_given_aperture_scatterguard_parked_when_set_to_out_position_then_ async def test_given_aperture_scatterguard_parked_when_set_to_in_position_then_returns_expected( scintillator_and_ap_sg: tuple[Scintillator, ApertureScatterguard], + run_engine: RunEngine, + beamstop: Beamstop, ): scintillator, ap_sg = scintillator_and_ap_sg ap_sg.selected_aperture.get_value.return_value = ApertureValue.PARKED # type: ignore - await scintillator.selected_pos.set(InOut.IN) + run_engine(scintillator.move_scintillator_safely(ap_sg, beamstop, InOut.IN)) await assert_value(scintillator.y_mm.user_setpoint, 100.855) await assert_value(scintillator.z_mm.user_setpoint, 101.5115) @@ -137,13 +152,14 @@ async def test_given_scintillator_already_out_when_moved_in_or_out_then_does_not expected_position, y, z, + run_engine: RunEngine, ): scintillator, ap_sg = scintillator_and_ap_sg ap_sg.get_scin_move_position.return_value = { # type: ignore ap_sg.aperture.x: -1.0, ap_sg.scatterguard.x: -1.5, } - beamstop.selected_pos.get_value.return_value = BeamstopPositions.UNKNOWN # type: ignore + set_mock_value(beamstop.x_mm.user_readback, 25) # unknown position await scintillator.y_mm.set(y) await scintillator.z_mm.set(z) @@ -151,7 +167,9 @@ async def test_given_scintillator_already_out_when_moved_in_or_out_then_does_not get_mock_put(scintillator.z_mm.user_setpoint).reset_mock() ap_sg.selected_aperture.get_value.return_value = ApertureValue.LARGE # type: ignore - await scintillator.selected_pos.set(expected_position) + run_engine( + scintillator.move_scintillator_safely(ap_sg, beamstop, expected_position) + ) get_mock_put(scintillator.y_mm.user_setpoint).assert_not_called() get_mock_put(scintillator.z_mm.user_setpoint).assert_not_called() @@ -160,21 +178,24 @@ async def test_given_scintillator_already_out_when_moved_in_or_out_then_does_not @pytest.mark.parametrize( - "beamstop_position, expected_good", + "beamstop_y_position, expected_good", [ - [BeamstopPositions.DATA_COLLECTION, True], - [BeamstopPositions.OUT_OF_BEAM, True], - [BeamstopPositions.UNKNOWN, False], + [45.4, True], # in beam + [43.4, True], # Out of beam + [25, False], # Unknown position ], ) async def test_beamstop_check_in_known_good_position( scintillator_and_ap_sg: tuple[Scintillator, ApertureScatterguard], beamstop: Beamstop, - beamstop_position: BeamstopPositions, + beamstop_y_position: float, expected_good: bool, + run_engine: RunEngine, ): - beamstop.selected_pos.get_value.return_value = beamstop_position # type: ignore - scintillator, _ = scintillator_and_ap_sg + set_mock_value( + beamstop.y_mm.user_readback, beamstop_y_position + ) # Beamstop out of beam + scintillator, ap_sg = scintillator_and_ap_sg with ( pytest.raises( @@ -183,7 +204,7 @@ async def test_beamstop_check_in_known_good_position( if not expected_good else nullcontext() ): - await scintillator.selected_pos.set(InOut.OUT) + run_engine(scintillator.move_scintillator_safely(ap_sg, beamstop, InOut.OUT)) @pytest.mark.parametrize( @@ -201,6 +222,8 @@ async def test_move_scintillator_moves_ap_sg_to_scin_move_and_back( final_z: float, swap_order: bool, final_position: InOut, + beamstop: Beamstop, + run_engine: RunEngine, ): scintillator, ap_sg = scintillator_and_ap_sg ap_sg.get_scin_move_position.return_value = { # type: ignore @@ -226,7 +249,7 @@ async def test_move_scintillator_moves_ap_sg_to_scin_move_and_back( parent.scintillator.attach_mock( get_mock_put(scintillator.z_mm.user_setpoint), "z_mm" ) - await scintillator.selected_pos.set(final_position) + run_engine(scintillator.move_scintillator_safely(ap_sg, beamstop, final_position)) expected_scintillator_move = [ call.scintillator.y_mm(final_y, wait=True), From 71941a4808aade591892fdb57301e7aff506d7ee Mon Sep 17 00:00:00 2001 From: Robert Tuck Date: Thu, 5 Feb 2026 15:13:24 +0000 Subject: [PATCH 15/16] Refactor scintillator, aperture-scatterguard again for selected_pos sets to still operate conditional on valid device positioning --- docs/how-to/external-io-devices.md | 2 +- .../devices/mx_phase1/aperturescatterguard.py | 71 +++++++++++++++++-- src/dodal/devices/mx_phase1/scintillator.py | 55 ++++++++++---- tests/devices/test_aperture_scatterguard.py | 63 ++++++++++++++++ tests/devices/test_scintillator.py | 52 ++++++++++---- 5 files changed, 209 insertions(+), 34 deletions(-) diff --git a/docs/how-to/external-io-devices.md b/docs/how-to/external-io-devices.md index eb10d4131f..4570f152c6 100644 --- a/docs/how-to/external-io-devices.md +++ b/docs/how-to/external-io-devices.md @@ -14,7 +14,7 @@ It's not recommended to read from the filesystem going forward and instead devel ## Extant examples -- [aperturescatterguard](../../src/dodal/devices/aperturescatterguard.py) - reads a set of valid positions from a file. +- [aperturescatterguard](../../src/dodal/devices/mx_phase1/aperturescatterguard.py) - reads a set of valid positions from a file. - [oav_to_redis_forwarder](../../src/dodal/devices/oav/oav_to_redis_forwarder.py) - pushes data into redis - [OAV_detector](../../src/dodal/devices/oav/oav_detector.py) - detector configuration is based on a file on disk diff --git a/src/dodal/devices/mx_phase1/aperturescatterguard.py b/src/dodal/devices/mx_phase1/aperturescatterguard.py index 7fa6957dcb..f09260e3e9 100644 --- a/src/dodal/devices/mx_phase1/aperturescatterguard.py +++ b/src/dodal/devices/mx_phase1/aperturescatterguard.py @@ -3,7 +3,7 @@ import asyncio import dataclasses from collections.abc import Callable -from math import inf +from math import inf, isclose import bluesky.plan_stubs as bps from bluesky.preprocessors import contingency_wrapper @@ -110,6 +110,7 @@ class ApertureValue(StrictEnum): LARGE = "LARGE_APERTURE" OUT_OF_BEAM = "Out of beam" PARKED = "Parked" # Parked under the collimation table for manual load + SCIN_MOVE = "Scintillator Move" # Transient position that is not defined by a single location def __str__(self): return self.name.capitalize() @@ -208,6 +209,8 @@ def __init__( large=self.aperture.large, medium=self.aperture.medium, small=self.aperture.small, + current_sg_x=self.scatterguard.x.user_readback, + current_ap_x=self.aperture.x.user_readback, current_ap_y=self.aperture.y.user_readback, current_ap_z=self.aperture.z.user_readback, ) @@ -243,12 +246,32 @@ async def _set_current_aperture_position(self, value: ApertureValue) -> None: raise NotImplementedError( "Currently not able to park aperture/scatterguard, see https://github.com/DiamondLightSource/mx-bluesky/issues/1197" ) + if value == ApertureValue.SCIN_MOVE: + raise NotImplementedError( + "The SCIN_MOVE position cannot be set directly - use move_scintillator_safely plan instead." + ) position = self._config.aperture_positions[value] + current_sg_x = await self.scatterguard.x.user_readback.get_value() + current_ap_x = await self.aperture.x.user_readback.get_value() current_ap_y = await self.aperture.y.user_readback.get_value() current_ap_z = await self.aperture.z.user_readback.get_value() - if self._is_in_position(ApertureValue.PARKED, current_ap_y, current_ap_z): + if self._is_in_position( + ApertureValue.SCIN_MOVE, + current_sg_x, + current_ap_x, + current_ap_y, + current_ap_z, + ): + raise NotImplementedError( + "Cannot move aperture-scatterguard while in SCIN_MOVE position, please ensure scintillator " + "move is complete and restore the aperture-scatterguard position." + ) + + if self._is_in_position( + ApertureValue.PARKED, current_sg_x, current_ap_x, current_ap_y, current_ap_z + ): await self._unpark(value) await self._check_safe_to_move(position.aperture_z) @@ -293,8 +316,24 @@ def _get_current_diameter(self, current_aperture: ApertureValue) -> float: return self._config.aperture_positions[current_aperture].diameter def _is_in_position( - self, position: ApertureValue, current_ap_y: float, current_ap_z: float + self, + position: ApertureValue, + current_sg_x: float, + current_ap_x: float, + current_ap_y: float, + current_ap_z: float, ) -> bool: + if position == ApertureValue.SCIN_MOVE: + return isclose( + self._config.scintillator_move_aperture_x, + current_ap_x, + abs_tol=self._tolerances.aperture_x, + ) and isclose( + self._config.scintillator_move_scatterguard_x, + current_sg_x, + abs_tol=self._tolerances.scatterguard_x, + ) + position_y = self._config.aperture_positions[position].aperture_y position_z = self._config.aperture_positions[position].aperture_z y_matches = abs(current_ap_y - position_y) <= self._tolerances.aperture_y @@ -306,19 +345,35 @@ def _get_current_aperture_position( large: float, medium: float, small: float, + current_sg_x: float, + current_ap_x: float, current_ap_y: float, current_ap_z: float, ) -> ApertureValue: + if self._is_in_position( + ApertureValue.SCIN_MOVE, + current_sg_x, + current_ap_x, + current_ap_y, + current_ap_z, + ): + return ApertureValue.SCIN_MOVE if large == 1: return ApertureValue.LARGE elif medium == 1: return ApertureValue.MEDIUM elif small == 1: return ApertureValue.SMALL - elif self._is_in_position(ApertureValue.PARKED, current_ap_y, current_ap_z): + elif self._is_in_position( + ApertureValue.PARKED, current_sg_x, current_ap_x, current_ap_y, current_ap_z + ): return ApertureValue.PARKED elif self._is_in_position( - ApertureValue.OUT_OF_BEAM, current_ap_y, current_ap_z + ApertureValue.OUT_OF_BEAM, + current_sg_x, + current_ap_x, + current_ap_y, + current_ap_z, ): return ApertureValue.OUT_OF_BEAM @@ -384,9 +439,13 @@ async def prepare(self, value: ApertureValue): Moving the assembly whilst out of the beam has no collision risk so we can just move all the motors together. """ + current_x = await self.aperture.x.user_readback.get_value() + current_sg_x = await self.scatterguard.x.user_readback.get_value() current_y = await self.aperture.y.user_readback.get_value() current_z = await self.aperture.z.user_readback.get_value() - if self._is_in_position(ApertureValue.OUT_OF_BEAM, current_y, current_z): + if self._is_in_position( + ApertureValue.OUT_OF_BEAM, current_sg_x, current_x, current_y, current_z + ): aperture_x, _, aperture_z, scatterguard_x, scatterguard_y = ( self._config.aperture_positions[value].values ) diff --git a/src/dodal/devices/mx_phase1/scintillator.py b/src/dodal/devices/mx_phase1/scintillator.py index 5797ad0408..0b30edb9a0 100644 --- a/src/dodal/devices/mx_phase1/scintillator.py +++ b/src/dodal/devices/mx_phase1/scintillator.py @@ -2,12 +2,13 @@ import bluesky.plan_stubs as bps from bluesky.utils import MsgGenerator -from ophyd_async.core import StandardReadable, StrictEnum, derived_signal_r +from ophyd_async.core import Reference, StandardReadable, StrictEnum, derived_signal_rw from ophyd_async.epics.motor import Motor from dodal.common.beamlines.beamline_parameters import GDABeamlineParameters from dodal.devices.mx_phase1.aperturescatterguard import ( ApertureScatterguard, + ApertureValue, do_with_aperture_scatterguard_in_scin_move_position, ) from dodal.devices.mx_phase1.beamstop import Beamstop, BeamstopPositions @@ -39,16 +40,21 @@ def __init__( self, prefix: str, beamline_parameters: GDABeamlineParameters, + aperture_scatterguard: Reference[ApertureScatterguard], + beamstop: Reference[Beamstop], name: str = "", ): with self.add_children_as_readables(): self.y_mm = Motor(f"{prefix}Y") self.z_mm = Motor(f"{prefix}Z") - self.selected_pos = derived_signal_r( + self.selected_pos = derived_signal_rw( self._get_selected_position, + self._set_selected_position, y=self.y_mm, z=self.z_mm, ) + self._beamstop = beamstop + self._aperture_scatterguard = aperture_scatterguard self._scintillator_out_yz_mm = [ float(beamline_parameters[f"scin_{axis}_SCIN_OUT"]) for axis in ("y", "z") @@ -64,8 +70,6 @@ def __init__( def move_scintillator_safely( self, - aperture_scatterguard: ApertureScatterguard, - beamstop: Beamstop, position: InOut, ) -> MsgGenerator: """Bluesky plan to move the scintillator which moves the aperture-scatterguard out of the way @@ -84,18 +88,12 @@ def move_scintillator_safely( return def _move_to_new_position(): - if position == InOut.OUT: - yield from bps.mv(self.y_mm, self._scintillator_out_yz_mm[0]) - yield from bps.mv(self.z_mm, self._scintillator_out_yz_mm[1]) - elif position == InOut.IN: - yield from bps.mv(self.z_mm, self._scintillator_in_yz_mm[1]) - yield from bps.mv(self.y_mm, self._scintillator_in_yz_mm[0]) + yield from bps.abs_set(self.selected_pos, position, wait=True) match position: case InOut.OUT | InOut.IN: - yield from self._check_beamstop_position(beamstop) yield from do_with_aperture_scatterguard_in_scin_move_position( - aperture_scatterguard, _move_to_new_position + self._aperture_scatterguard(), _move_to_new_position ) case _: raise ValueError(f"Cannot set scintillator to position {position}") @@ -122,8 +120,37 @@ def _get_selected_position(self, y: float, z: float) -> InOut: else: return InOut.UNKNOWN - def _check_beamstop_position(self, beamstop: Beamstop) -> MsgGenerator: - position = yield from bps.rd(beamstop.selected_pos) + async def _set_selected_position(self, position: InOut) -> None: + current_y = await self.y_mm.user_readback.get_value() + current_z = await self.z_mm.user_readback.get_value() + if self._get_selected_position(current_y, current_z) == position: + return + + await self._check_beamstop_position() + aperture_scatterguard_pos = ( + await self._aperture_scatterguard().selected_aperture.get_value() + ) + if aperture_scatterguard_pos != ApertureValue.SCIN_MOVE: + raise ValueError( + "Scintillator cannot be moved while aperture-scatterguard not in SCIN_MOVE position" + ) + + match position: + case InOut.OUT | InOut.IN: + await self._move_to_new_position(position) + case _: + raise ValueError(f"Cannot set scintillator to position {position}") + + async def _move_to_new_position(self, position): + if position == InOut.OUT: + await self.y_mm.set(self._scintillator_out_yz_mm[0]) + await self.z_mm.set(self._scintillator_out_yz_mm[1]) + elif position == InOut.IN: + await self.z_mm.set(self._scintillator_in_yz_mm[1]) + await self.y_mm.set(self._scintillator_in_yz_mm[0]) + + async def _check_beamstop_position(self): + position = await self._beamstop().selected_pos.get_value() match position: case BeamstopPositions.OUT_OF_BEAM | BeamstopPositions.DATA_COLLECTION: return diff --git a/tests/devices/test_aperture_scatterguard.py b/tests/devices/test_aperture_scatterguard.py index dfa59a1adf..6e7b8b5ae3 100644 --- a/tests/devices/test_aperture_scatterguard.py +++ b/tests/devices/test_aperture_scatterguard.py @@ -662,3 +662,66 @@ def my_plan() -> MsgGenerator: get_mock_put(ap_sg.scatterguard.x.user_setpoint).assert_called_once_with( -4.75, wait=True ) + + +async def test_set_scin_move_position_raises_not_implemented( + aperture_in_medium_pos: ApertureScatterguard, +): + aperture_scatterguard = aperture_in_medium_pos + with pytest.raises( + NotImplementedError, match="The SCIN_MOVE position cannot be set directly" + ): + await aperture_scatterguard.selected_aperture.set(ApertureValue.SCIN_MOVE) + + +@pytest.mark.parametrize( + "ap_x_offset, sg_x_offset, expected_value", + [ + [-0.0041, 0, ApertureValue.MEDIUM], + [-0.0039, 0, ApertureValue.SCIN_MOVE], + [0.0041, 0, ApertureValue.MEDIUM], + [0, -0.09, ApertureValue.SCIN_MOVE], + [0, 0.09, ApertureValue.SCIN_MOVE], + [0, 0.11, ApertureValue.MEDIUM], + [0, -0.11, ApertureValue.MEDIUM], + ], +) +async def test_get_selected_position_returns_scin_move( + ap_x_offset: float, + sg_x_offset: float, + expected_value: ApertureValue | None, + aperture_in_medium_pos: ApertureScatterguard, + ap_sg_configuration: ApertureScatterguardConfiguration, +): + aperture_scatterguard = aperture_in_medium_pos + set_mock_value( + aperture_scatterguard.aperture.x.user_readback, + ap_sg_configuration.scintillator_move_aperture_x + ap_x_offset, + ) + + set_mock_value( + aperture_scatterguard.scatterguard.x.user_readback, + ap_sg_configuration.scintillator_move_scatterguard_x + sg_x_offset, + ) + + assert await aperture_scatterguard.selected_aperture.get_value() == expected_value + + +async def test_set_selected_position_raises_not_implemented_if_currently_in_scin_move_position( + aperture_in_medium_pos: ApertureScatterguard, + ap_sg_configuration: ApertureScatterguardConfiguration, +): + aperture_scatterguard = aperture_in_medium_pos + set_mock_value( + aperture_scatterguard.aperture.x.user_readback, + ap_sg_configuration.scintillator_move_aperture_x, + ) + set_mock_value( + aperture_scatterguard.scatterguard.x.user_readback, + ap_sg_configuration.scintillator_move_scatterguard_x, + ) + with pytest.raises( + NotImplementedError, + match="Cannot move aperture-scatterguard while in SCIN_MOVE position", + ): + await aperture_scatterguard.selected_aperture.set(ApertureValue.SMALL) diff --git a/tests/devices/test_scintillator.py b/tests/devices/test_scintillator.py index cc245db725..9844d4d31d 100644 --- a/tests/devices/test_scintillator.py +++ b/tests/devices/test_scintillator.py @@ -2,8 +2,8 @@ from unittest.mock import AsyncMock, MagicMock, call import pytest -from bluesky import RunEngine -from ophyd_async.core import get_mock_put, init_devices, set_mock_value +from bluesky import FailedStatus, RunEngine +from ophyd_async.core import Reference, get_mock_put, init_devices, set_mock_value from ophyd_async.testing import assert_value from dodal.common.beamlines.beamline_parameters import GDABeamlineParameters @@ -57,6 +57,7 @@ async def beamstop(mock_beamline_parameters) -> Beamstop: async def scintillator_and_ap_sg( mock_beamline_parameters: GDABeamlineParameters, ap_sg: ApertureScatterguard, + beamstop: Beamstop, ) -> tuple[Scintillator, ApertureScatterguard]: async with init_devices(mock=True): ap_sg.selected_aperture.set = AsyncMock() @@ -66,6 +67,8 @@ async def scintillator_and_ap_sg( prefix="", name="test_scin", beamline_parameters=mock_beamline_parameters, + aperture_scatterguard=Reference(ap_sg), + beamstop=Reference(beamstop), ) set_mock_value(ap_sg.aperture.x.user_readback, 1.0) set_mock_value(ap_sg.scatterguard.x.user_readback, 2.0) @@ -106,9 +109,7 @@ def test_when_set_to_unknown_position_then_error_raised( set_mock_value(scintillator.y_mm.user_readback, 100.855) set_mock_value(scintillator.z_mm.user_readback, 101.5115) with pytest.raises(ValueError): - run_engine( - scintillator.move_scintillator_safely(ap_sg, beamstop, InOut.UNKNOWN) - ) + run_engine(scintillator.move_scintillator_safely(InOut.UNKNOWN)) async def test_given_aperture_scatterguard_parked_when_set_to_out_position_then_returns_expected( @@ -119,7 +120,7 @@ async def test_given_aperture_scatterguard_parked_when_set_to_out_position_then_ scintillator, ap_sg = scintillator_and_ap_sg ap_sg.selected_aperture.get_value.return_value = ApertureValue.PARKED # type: ignore - run_engine(scintillator.move_scintillator_safely(ap_sg, beamstop, InOut.OUT)) + run_engine(scintillator.move_scintillator_safely(InOut.OUT)) await assert_value(scintillator.y_mm.user_setpoint, -0.02) await assert_value(scintillator.z_mm.user_setpoint, 0.1) @@ -133,7 +134,7 @@ async def test_given_aperture_scatterguard_parked_when_set_to_in_position_then_r scintillator, ap_sg = scintillator_and_ap_sg ap_sg.selected_aperture.get_value.return_value = ApertureValue.PARKED # type: ignore - run_engine(scintillator.move_scintillator_safely(ap_sg, beamstop, InOut.IN)) + run_engine(scintillator.move_scintillator_safely(InOut.IN)) await assert_value(scintillator.y_mm.user_setpoint, 100.855) await assert_value(scintillator.z_mm.user_setpoint, 101.5115) @@ -167,9 +168,7 @@ async def test_given_scintillator_already_out_when_moved_in_or_out_then_does_not get_mock_put(scintillator.z_mm.user_setpoint).reset_mock() ap_sg.selected_aperture.get_value.return_value = ApertureValue.LARGE # type: ignore - run_engine( - scintillator.move_scintillator_safely(ap_sg, beamstop, expected_position) - ) + run_engine(scintillator.move_scintillator_safely(expected_position)) get_mock_put(scintillator.y_mm.user_setpoint).assert_not_called() get_mock_put(scintillator.z_mm.user_setpoint).assert_not_called() @@ -199,12 +198,12 @@ async def test_beamstop_check_in_known_good_position( with ( pytest.raises( - ValueError, match="Scintillator cannot be moved due to beamstop position" + FailedStatus, match="Scintillator cannot be moved due to beamstop position" ) if not expected_good else nullcontext() ): - run_engine(scintillator.move_scintillator_safely(ap_sg, beamstop, InOut.OUT)) + run_engine(scintillator.move_scintillator_safely(InOut.OUT)) @pytest.mark.parametrize( @@ -249,7 +248,7 @@ async def test_move_scintillator_moves_ap_sg_to_scin_move_and_back( parent.scintillator.attach_mock( get_mock_put(scintillator.z_mm.user_setpoint), "z_mm" ) - run_engine(scintillator.move_scintillator_safely(ap_sg, beamstop, final_position)) + run_engine(scintillator.move_scintillator_safely(final_position)) expected_scintillator_move = [ call.scintillator.y_mm(final_y, wait=True), @@ -266,3 +265,30 @@ async def test_move_scintillator_moves_ap_sg_to_scin_move_and_back( ] ) parent.assert_has_calls(expected_calls) + + +async def test_scintillator_set_raises_if_aperture_scatterguard_not_in_scin_move_position( + scintillator_and_ap_sg: tuple[Scintillator, ApertureScatterguard], +): + scintillator, ap_sg = scintillator_and_ap_sg + with pytest.raises( + ValueError, + match="Scintillator cannot be moved while aperture-scatterguard not in SCIN_MOVE position", + ): + await scintillator.selected_pos.set(InOut.OUT) + + +async def test_scintillator_set_raises_if_beamstop_not_in_good_position( + scintillator_and_ap_sg: tuple[Scintillator, ApertureScatterguard], + beamstop: Beamstop, +): + scintillator, ap_sg = scintillator_and_ap_sg + set_mock_value( + beamstop.y_mm.user_readback, + 25, # Unknown position + ) + scintillator, ap_sg = scintillator_and_ap_sg + with pytest.raises( + ValueError, match="Scintillator cannot be moved due to beamstop position" + ): + await scintillator.selected_pos.set(InOut.OUT) From a2e9c3f7da2f27adba6a9d12ef96efc483580cdc Mon Sep 17 00:00:00 2001 From: Robert Tuck Date: Thu, 5 Feb 2026 16:45:05 +0000 Subject: [PATCH 16/16] Fixup unit tests --- src/dodal/beamlines/i03.py | 8 +- src/dodal/beamlines/i04.py | 8 +- src/dodal/devices/mx_phase1/scintillator.py | 60 +++++++-------- tests/devices/test_scintillator.py | 83 +++++++++++++++------ 4 files changed, 102 insertions(+), 57 deletions(-) diff --git a/src/dodal/beamlines/i03.py b/src/dodal/beamlines/i03.py index 3897e2ffbd..553c651b16 100644 --- a/src/dodal/beamlines/i03.py +++ b/src/dodal/beamlines/i03.py @@ -1,6 +1,6 @@ from functools import cache -from ophyd_async.core import PathProvider +from ophyd_async.core import PathProvider, Reference from ophyd_async.fastcs.eiger import EigerDetector as FastEiger from ophyd_async.fastcs.panda import HDFPanda from yarl import URL @@ -342,10 +342,14 @@ def fluorescence_det_motion() -> FluorescenceDetector: @devices.factory() -def scintillator() -> Scintillator: +def scintillator( + aperture_scatterguard: ApertureScatterguard, beamstop: Beamstop +) -> Scintillator: return Scintillator( f"{PREFIX.beamline_prefix}-MO-SCIN-01:", get_beamline_parameters(), + Reference(aperture_scatterguard), + Reference(beamstop), ) diff --git a/src/dodal/beamlines/i04.py b/src/dodal/beamlines/i04.py index 6eaf32b024..12ef6e2a7d 100644 --- a/src/dodal/beamlines/i04.py +++ b/src/dodal/beamlines/i04.py @@ -1,3 +1,5 @@ +from ophyd_async.core import Reference + from dodal.common.beamlines.beamline_parameters import get_beamline_parameters from dodal.common.beamlines.beamline_utils import set_beamline as set_utils_beamline from dodal.device_manager import DeviceManager @@ -275,10 +277,14 @@ def pin_tip_detection() -> PinTipDetection: @devices.factory() -def scintillator() -> Scintillator: +def scintillator( + aperture_scatterguard: ApertureScatterguard, beamstop: Beamstop +) -> Scintillator: return Scintillator( f"{PREFIX.beamline_prefix}-MO-SCIN-01:", get_beamline_parameters(), + Reference(aperture_scatterguard), + Reference(beamstop), ) diff --git a/src/dodal/devices/mx_phase1/scintillator.py b/src/dodal/devices/mx_phase1/scintillator.py index 0b30edb9a0..6c46d9678d 100644 --- a/src/dodal/devices/mx_phase1/scintillator.py +++ b/src/dodal/devices/mx_phase1/scintillator.py @@ -68,36 +68,6 @@ def __init__( super().__init__(name) - def move_scintillator_safely( - self, - position: InOut, - ) -> MsgGenerator: - """Bluesky plan to move the scintillator which moves the aperture-scatterguard out of the way - for the duration of the move and then restores it. - - Args: - aperture_scatterguard (ApertureScatterguard): - The aperture-scatterguard device which will be moved - beamstop (Beamstop): - The beamstop device which will be checked for potential collisions - position (InOut): - The scintillator position to move to - """ - current_pos = yield from bps.rd(self.selected_pos) - if current_pos == position: - return - - def _move_to_new_position(): - yield from bps.abs_set(self.selected_pos, position, wait=True) - - match position: - case InOut.OUT | InOut.IN: - yield from do_with_aperture_scatterguard_in_scin_move_position( - self._aperture_scatterguard(), _move_to_new_position - ) - case _: - raise ValueError(f"Cannot set scintillator to position {position}") - def _check_position(self, current_pos: list[float], pos_to_check: list[float]): return all( isclose(axis_pos, axis_in_beam, abs_tol=axis_tolerance) @@ -158,3 +128,33 @@ async def _check_beamstop_position(self): raise ValueError( f"Scintillator cannot be moved due to beamstop position {position}, must be in either in DATA_COLLECTION or OUT_OF_BEAM position." ) + + +def move_scintillator_safely( + scintillator: Scintillator, + position: InOut, +) -> MsgGenerator: + """Bluesky plan to move the scintillator which moves the aperture-scatterguard out of the way + for the duration of the move and then restores it. + + Args: + scintillator (Scintillator): + The scintillator to move + position (InOut): + The scintillator position to move to + """ + current_pos = yield from bps.rd(scintillator.selected_pos) + if current_pos == position: + return + + def _move_to_new_position(): + yield from bps.abs_set(scintillator.selected_pos, position, wait=True) + + match position: + case InOut.OUT | InOut.IN: + yield from do_with_aperture_scatterguard_in_scin_move_position( + scintillator._aperture_scatterguard(), # noqa: SLF001 + _move_to_new_position, + ) + case _: + raise ValueError(f"Cannot set scintillator to position {position}") diff --git a/tests/devices/test_scintillator.py b/tests/devices/test_scintillator.py index 9844d4d31d..00a693656c 100644 --- a/tests/devices/test_scintillator.py +++ b/tests/devices/test_scintillator.py @@ -1,5 +1,5 @@ from contextlib import nullcontext -from unittest.mock import AsyncMock, MagicMock, call +from unittest.mock import MagicMock, call import pytest from bluesky import FailedStatus, RunEngine @@ -12,7 +12,11 @@ ApertureValue, ) from dodal.devices.mx_phase1.beamstop import Beamstop -from dodal.devices.mx_phase1.scintillator import InOut, Scintillator +from dodal.devices.mx_phase1.scintillator import ( + InOut, + Scintillator, + move_scintillator_safely, +) @pytest.fixture @@ -35,6 +39,41 @@ def mock_beamline_parameters() -> GDABeamlineParameters: ) +async def move_aperture_scatterguard_and_reset_mocks( + aperture_scatterguard: ApertureScatterguard, pos: ApertureValue +): + motor_positions = aperture_scatterguard._config.aperture_positions[pos] + for motor, value in zip( + [ + aperture_scatterguard.aperture.x, + aperture_scatterguard.aperture.y, + aperture_scatterguard.aperture.z, + aperture_scatterguard.scatterguard.x, + aperture_scatterguard.scatterguard.y, + ], + [ + motor_positions.aperture_x, + motor_positions.aperture_y, + motor_positions.aperture_z, + motor_positions.scatterguard_x, + motor_positions.scatterguard_y, + ], + strict=True, + ): + set_mock_value(motor.user_readback, value) + get_mock_put(motor.user_readback).reset_mock() + + set_mock_value( + aperture_scatterguard.aperture.large, 1 if pos == ApertureValue.LARGE else 0 + ) + set_mock_value( + aperture_scatterguard.aperture.medium, 1 if pos == ApertureValue.MEDIUM else 0 + ) + set_mock_value( + aperture_scatterguard.aperture.small, 1 if pos == ApertureValue.SMALL else 0 + ) + + @pytest.fixture async def beamstop(mock_beamline_parameters) -> Beamstop: beamstop = Beamstop("-MO-BS-01:", mock_beamline_parameters, name="beamstop") @@ -59,10 +98,8 @@ async def scintillator_and_ap_sg( ap_sg: ApertureScatterguard, beamstop: Beamstop, ) -> tuple[Scintillator, ApertureScatterguard]: + # ap_sg.get_scin_move_position = MagicMock() async with init_devices(mock=True): - ap_sg.selected_aperture.set = AsyncMock() - ap_sg.selected_aperture.get_value = AsyncMock() - ap_sg.get_scin_move_position = MagicMock() scintillator = Scintillator( prefix="", name="test_scin", @@ -109,7 +146,7 @@ def test_when_set_to_unknown_position_then_error_raised( set_mock_value(scintillator.y_mm.user_readback, 100.855) set_mock_value(scintillator.z_mm.user_readback, 101.5115) with pytest.raises(ValueError): - run_engine(scintillator.move_scintillator_safely(InOut.UNKNOWN)) + run_engine(move_scintillator_safely(scintillator, InOut.UNKNOWN)) async def test_given_aperture_scatterguard_parked_when_set_to_out_position_then_returns_expected( @@ -118,9 +155,9 @@ async def test_given_aperture_scatterguard_parked_when_set_to_out_position_then_ beamstop: Beamstop, ): scintillator, ap_sg = scintillator_and_ap_sg - ap_sg.selected_aperture.get_value.return_value = ApertureValue.PARKED # type: ignore + await move_aperture_scatterguard_and_reset_mocks(ap_sg, ApertureValue.PARKED) - run_engine(scintillator.move_scintillator_safely(InOut.OUT)) + run_engine(move_scintillator_safely(scintillator, InOut.OUT)) await assert_value(scintillator.y_mm.user_setpoint, -0.02) await assert_value(scintillator.z_mm.user_setpoint, 0.1) @@ -132,9 +169,9 @@ async def test_given_aperture_scatterguard_parked_when_set_to_in_position_then_r beamstop: Beamstop, ): scintillator, ap_sg = scintillator_and_ap_sg - ap_sg.selected_aperture.get_value.return_value = ApertureValue.PARKED # type: ignore + await move_aperture_scatterguard_and_reset_mocks(ap_sg, ApertureValue.PARKED) - run_engine(scintillator.move_scintillator_safely(InOut.IN)) + run_engine(move_scintillator_safely(scintillator, InOut.IN)) await assert_value(scintillator.y_mm.user_setpoint, 100.855) await assert_value(scintillator.z_mm.user_setpoint, 101.5115) @@ -156,19 +193,15 @@ async def test_given_scintillator_already_out_when_moved_in_or_out_then_does_not run_engine: RunEngine, ): scintillator, ap_sg = scintillator_and_ap_sg - ap_sg.get_scin_move_position.return_value = { # type: ignore - ap_sg.aperture.x: -1.0, - ap_sg.scatterguard.x: -1.5, - } set_mock_value(beamstop.x_mm.user_readback, 25) # unknown position + await move_aperture_scatterguard_and_reset_mocks(ap_sg, ApertureValue.LARGE) await scintillator.y_mm.set(y) await scintillator.z_mm.set(z) get_mock_put(scintillator.y_mm.user_setpoint).reset_mock() get_mock_put(scintillator.z_mm.user_setpoint).reset_mock() - ap_sg.selected_aperture.get_value.return_value = ApertureValue.LARGE # type: ignore - run_engine(scintillator.move_scintillator_safely(expected_position)) + run_engine(move_scintillator_safely(scintillator, expected_position)) get_mock_put(scintillator.y_mm.user_setpoint).assert_not_called() get_mock_put(scintillator.z_mm.user_setpoint).assert_not_called() @@ -195,7 +228,7 @@ async def test_beamstop_check_in_known_good_position( beamstop.y_mm.user_readback, beamstop_y_position ) # Beamstop out of beam scintillator, ap_sg = scintillator_and_ap_sg - + await move_aperture_scatterguard_and_reset_mocks(ap_sg, ApertureValue.MEDIUM) with ( pytest.raises( FailedStatus, match="Scintillator cannot be moved due to beamstop position" @@ -203,7 +236,7 @@ async def test_beamstop_check_in_known_good_position( if not expected_good else nullcontext() ): - run_engine(scintillator.move_scintillator_safely(InOut.OUT)) + run_engine(move_scintillator_safely(scintillator, InOut.OUT)) @pytest.mark.parametrize( @@ -225,10 +258,6 @@ async def test_move_scintillator_moves_ap_sg_to_scin_move_and_back( run_engine: RunEngine, ): scintillator, ap_sg = scintillator_and_ap_sg - ap_sg.get_scin_move_position.return_value = { # type: ignore - ap_sg.aperture.x: -1.0, - ap_sg.scatterguard.x: -1.5, - } set_mock_value(scintillator.y_mm.user_readback, initial_y) set_mock_value(scintillator.z_mm.user_readback, initial_z) @@ -248,7 +277,7 @@ async def test_move_scintillator_moves_ap_sg_to_scin_move_and_back( parent.scintillator.attach_mock( get_mock_put(scintillator.z_mm.user_setpoint), "z_mm" ) - run_engine(scintillator.move_scintillator_safely(final_position)) + run_engine(move_scintillator_safely(scintillator, final_position)) expected_scintillator_move = [ call.scintillator.y_mm(final_y, wait=True), @@ -257,7 +286,12 @@ async def test_move_scintillator_moves_ap_sg_to_scin_move_and_back( if swap_order: expected_scintillator_move = expected_scintillator_move[::-1] expected_calls = ( - [call.aperture.x(-1.0, wait=True), call.scatterguard.x(-1.5, wait=True)] + [ + call.aperture.x(ap_sg._config.scintillator_move_aperture_x, wait=True), + call.scatterguard.x( + ap_sg._config.scintillator_move_scatterguard_x, wait=True + ), + ] + expected_scintillator_move + [ call.aperture.x(1.0, wait=True), @@ -271,6 +305,7 @@ async def test_scintillator_set_raises_if_aperture_scatterguard_not_in_scin_move scintillator_and_ap_sg: tuple[Scintillator, ApertureScatterguard], ): scintillator, ap_sg = scintillator_and_ap_sg + await move_aperture_scatterguard_and_reset_mocks(ap_sg, ApertureValue.MEDIUM) with pytest.raises( ValueError, match="Scintillator cannot be moved while aperture-scatterguard not in SCIN_MOVE position",