From 8c8f10d44ca3880a40cde039c4f5e5d24fa6653c Mon Sep 17 00:00:00 2001 From: Robert Tuck Date: Mon, 30 Mar 2026 12:56:53 +0100 Subject: [PATCH 1/4] Fix for smargon combined moves with current slow-smargon workaround --- src/dodal/devices/smargon.py | 25 ++++++++++++++++++++++++- 1 file changed, 24 insertions(+), 1 deletion(-) diff --git a/src/dodal/devices/smargon.py b/src/dodal/devices/smargon.py index b1a2897e385..3a72b3bfce6 100644 --- a/src/dodal/devices/smargon.py +++ b/src/dodal/devices/smargon.py @@ -17,7 +17,7 @@ from dodal.common.maths import AngleWithPhase from dodal.devices.motors import XYZWrappedOmegaStage from dodal.devices.util.epics_util import SetWhenEnabled - +from dodal.log import LOGGER class StubPosition(Enum): CURRENT_AS_CENTER = 0 @@ -123,6 +123,29 @@ async def set(self, value: CombinedMove): 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. """ + + #Sticky Y motor on i03 - try to move chi before the other motors to see if this helps + # issues. + # Nb smargon motors also appear to be ignoring max velocity during combined moves + LOGGER.info("Unwrapping target values...") + for motor_name, new_setpoint in value.items(): + if new_setpoint is not None and isinstance(new_setpoint, int | float): + mapped_value = await self._get_target_value( + motor_name, new_setpoint + ) + LOGGER.info(f"Unwrapped {motor_name} {new_setpoint} -> {mapped_value}") + value[motor_name] = mapped_value + + LOGGER.info("Doing smargon move...") + if "chi" in value.keys() and value["chi"] is not None: + LOGGER.info("Doing chi.set") + await self.chi.set(value["chi"]) + LOGGER.info("Chi move done") + else: + LOGGER.info("Chi not moving in this smargon move") + if "omega" in value.keys() and value["omega"] is not None: + LOGGER.info("Doing omega set") + await self.omega.set(value["omega"]) await self.defer_move.set(DeferMoves.ON) # TODO Hotfix required here until https://github.com/DiamondLightSource/dodal/issues/1998 # is implemented in separate PR From 5c6d7ba37ad1a2c318a82a7c3977df0d5383addc Mon Sep 17 00:00:00 2001 From: Robert Tuck Date: Wed, 8 Apr 2026 14:10:13 +0100 Subject: [PATCH 2/4] Remove omega from CombinedMoves --- src/dodal/devices/smargon.py | 38 ------------------ tests/devices/test_smargon.py | 72 +++++++++-------------------------- 2 files changed, 17 insertions(+), 93 deletions(-) diff --git a/src/dodal/devices/smargon.py b/src/dodal/devices/smargon.py index 3a72b3bfce6..67b6fbea5b1 100644 --- a/src/dodal/devices/smargon.py +++ b/src/dodal/devices/smargon.py @@ -14,7 +14,6 @@ from ophyd_async.epics.core import epics_signal_r, epics_signal_rw from ophyd_async.epics.motor import Motor -from dodal.common.maths import AngleWithPhase from dodal.devices.motors import XYZWrappedOmegaStage from dodal.devices.util.epics_util import SetWhenEnabled from dodal.log import LOGGER @@ -74,7 +73,6 @@ class CombinedMove(TypedDict, total=False): x: float | None y: float | None z: float | None - omega: float | None phi: float | None chi: float | None @@ -105,15 +103,6 @@ def __init__(self, prefix: str, name: str = ""): super().__init__(prefix, name) - async def _get_target_value(self, motor_name: str, value: float) -> float: - if motor_name == "omega": - current_angle = AngleWithPhase.from_iterable( - await self.wrapped_omega.offset_and_phase.get_value() - ) - return current_angle.nearest_with_phase(value).unwrap() - else: - return value - @AsyncStatus.wrap async def set(self, value: CombinedMove): """This will move all motion together in a deferred move. @@ -123,39 +112,12 @@ async def set(self, value: CombinedMove): 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. """ - - #Sticky Y motor on i03 - try to move chi before the other motors to see if this helps - # issues. - # Nb smargon motors also appear to be ignoring max velocity during combined moves - LOGGER.info("Unwrapping target values...") - for motor_name, new_setpoint in value.items(): - if new_setpoint is not None and isinstance(new_setpoint, int | float): - mapped_value = await self._get_target_value( - motor_name, new_setpoint - ) - LOGGER.info(f"Unwrapped {motor_name} {new_setpoint} -> {mapped_value}") - value[motor_name] = mapped_value - LOGGER.info("Doing smargon move...") - if "chi" in value.keys() and value["chi"] is not None: - LOGGER.info("Doing chi.set") - await self.chi.set(value["chi"]) - LOGGER.info("Chi move done") - else: - LOGGER.info("Chi not moving in this smargon move") - if "omega" in value.keys() and value["omega"] is not None: - LOGGER.info("Doing omega set") - await self.omega.set(value["omega"]) await self.defer_move.set(DeferMoves.ON) - # TODO Hotfix required here until https://github.com/DiamondLightSource/dodal/issues/1998 - # is implemented in separate PR try: finished_moving = [] for motor_name, new_setpoint in value.items(): if new_setpoint is not None and isinstance(new_setpoint, int | float): - new_setpoint = await self._get_target_value( - motor_name, new_setpoint - ) axis = getattr(self, motor_name) await axis.check_motor_limit( await axis.user_setpoint.get_value(), new_setpoint diff --git a/tests/devices/test_smargon.py b/tests/devices/test_smargon.py index 73118ff2b7c..ff25713c6ac 100644 --- a/tests/devices/test_smargon.py +++ b/tests/devices/test_smargon.py @@ -85,56 +85,23 @@ async def test_given_center_disp_low_when_stub_offsets_set_to_center_and_moved_t assert await smargon.stub_offsets.to_robot_load.proc.get_value() == 0 -async def test_set_with_omega_outside_smargon_limit( - smargon: Smargon, -): - set_mock_value(smargon.omega.low_limit_travel, -1999) - set_mock_value(smargon.omega.high_limit_travel, 1999) - set_mock_value(smargon.omega.dial_low_limit_travel, -1999) - set_mock_value(smargon.omega.dial_high_limit_travel, 1999) - await smargon.omega.set(1999) - with pytest.raises(MotorLimitsError): - await smargon.set( - CombinedMove( - x=10, - y=20, - z=30, - omega=200, - chi=15, - phi=25, - ) - ) - await smargon.omega.set(-1999) - with pytest.raises(MotorLimitsError): - await smargon.set( - CombinedMove( - x=10, - y=20, - z=30, - omega=160, - chi=15, - phi=25, - ) - ) - - @pytest.mark.parametrize( - "test_x, test_y, test_z, test_omega, test_chi, test_phi", + "test_x, test_y, test_z, test_chi, test_phi", [ - (2000, 20, 30, 5, 15, 25), # x goes beyond upper limit - (-2000, 20, 30, 5, 15, 25), # x goes beyond lower limit - (10, 2000, 30, 5, 15, 25), # y goes beyond upper limit - (10, -2000, 30, 5, 15, 25), # y goes beyond lower limit - (10, 20, 2000, 5, 15, 25), # z goes beyond upper limit - (10, 20, -2000, 5, 15, 25), # z goes beyond lower limit - (10, 20, 30, 5, 2000, 25), # chi goes beyond upper limit - (10, 20, 30, 5, -2000, 25), # chi goes beyond lower limit - (10, 20, 30, 5, 15, 2000), # phi goes beyond upper limit - (10, 20, 30, 5, 15, -2000), # phi goes beyond lower limit + (2000, 20, 30, 15, 25), # x goes beyond upper limit + (-2000, 20, 30, 15, 25), # x goes beyond lower limit + (10, 2000, 30, 15, 25), # y goes beyond upper limit + (10, -2000, 30, 15, 25), # y goes beyond lower limit + (10, 20, 2000, 15, 25), # z goes beyond upper limit + (10, 20, -2000, 15, 25), # z goes beyond lower limit + (10, 20, 30, 2000, 25), # chi goes beyond upper limit + (10, 20, 30, -2000, 25), # chi goes beyond lower limit + (10, 20, 30, 15, 2000), # phi goes beyond upper limit + (10, 20, 30, 15, -2000), # phi goes beyond lower limit ], ) async def test_given_set_with_value_outside_motor_limit( - smargon: Smargon, test_x, test_y, test_z, test_omega, test_chi, test_phi + smargon: Smargon, test_x, test_y, test_z, test_chi, test_phi ): for motor in [ smargon.x, @@ -154,7 +121,6 @@ async def test_given_set_with_value_outside_motor_limit( x=test_x, y=test_y, z=test_z, - omega=test_omega, chi=test_chi, phi=test_phi, ) @@ -181,12 +147,11 @@ async def test_given_set_with_none_then_that_motor_does_not_move(smargon: Smargo async def test_given_set_with_all_values_then_motors_move(smargon: Smargon): - await smargon.set(CombinedMove(x=10, y=20, z=30, omega=5, chi=15, phi=25)) + await smargon.set(CombinedMove(x=10, y=20, z=30, chi=15, phi=25)) get_mock_put(smargon.x.user_setpoint).assert_called_once_with(10) get_mock_put(smargon.y.user_setpoint).assert_called_once_with(20) get_mock_put(smargon.z.user_setpoint).assert_called_once_with(30) - get_mock_put(smargon.omega.user_setpoint).assert_called_once_with(5) get_mock_put(smargon.chi.user_setpoint).assert_called_once_with(15) get_mock_put(smargon.phi.user_setpoint).assert_called_once_with(25) @@ -201,26 +166,23 @@ async def test_given_set_with_all_values_then_motors_set_in_order(smargon: Smarg parent.attach_mock(get_mock_put(smargon.x.user_setpoint), "x") parent.attach_mock(get_mock_put(smargon.y.user_setpoint), "y") parent.attach_mock(get_mock_put(smargon.z.user_setpoint), "z") - parent.attach_mock(get_mock_put(smargon.omega.user_setpoint), "omega") parent.attach_mock(get_mock_put(smargon.chi.user_setpoint), "chi") parent.attach_mock(get_mock_put(smargon.phi.user_setpoint), "phi") - await smargon.set(CombinedMove(x=10, y=20, z=30, omega=5, chi=15, phi=25)) + await smargon.set(CombinedMove(x=10, y=20, z=30, chi=15, phi=25)) - assert len(parent.mock_calls) == 8 - assert parent.mock_calls[0] == call.defer_move(DeferMoves.ON) + assert len(parent.mock_calls) == 7 parent.assert_has_calls( [ + call.defer_move(DeferMoves.ON), call.x(10), call.y(20), call.z(30), - call.omega(5), call.chi(15), call.phi(25), + call.defer_move(DeferMoves.OFF), ], - any_order=True, ) - assert parent.mock_calls[-1] == call.defer_move(DeferMoves.OFF) async def test_given_set_fails_then_defer_moves_turned_back_off(smargon: Smargon): From eb7d0e631a1a5a78ffe9fdad8c8729452baf0279 Mon Sep 17 00:00:00 2001 From: Robert Tuck Date: Wed, 8 Apr 2026 15:49:22 +0100 Subject: [PATCH 3/4] Make lint happy --- src/dodal/devices/smargon.py | 1 + 1 file changed, 1 insertion(+) diff --git a/src/dodal/devices/smargon.py b/src/dodal/devices/smargon.py index 67b6fbea5b1..f10cd7f75a6 100644 --- a/src/dodal/devices/smargon.py +++ b/src/dodal/devices/smargon.py @@ -18,6 +18,7 @@ from dodal.devices.util.epics_util import SetWhenEnabled from dodal.log import LOGGER + class StubPosition(Enum): CURRENT_AS_CENTER = 0 RESET_TO_ROBOT_LOAD = 1 From b4f8891df3132f036753fb0fea6f29c3948dc7f7 Mon Sep 17 00:00:00 2001 From: Robert Tuck Date: Thu, 14 May 2026 13:24:55 +0100 Subject: [PATCH 4/4] Additional comment to describe the omission of omega from CombinedMove --- src/dodal/devices/smargon.py | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/src/dodal/devices/smargon.py b/src/dodal/devices/smargon.py index f10cd7f75a6..ffc08d20483 100644 --- a/src/dodal/devices/smargon.py +++ b/src/dodal/devices/smargon.py @@ -69,7 +69,19 @@ class DeferMoves(StrictEnum): class CombinedMove(TypedDict, total=False): - """A move on multiple axes at once using a deferred move.""" + """A move on multiple axes at once using a deferred move. + Note that omega is excluded from the list of axes here since large omega moves + interfere with the precision of the other axes. In addition, the omega move is more + naturally specified as a wrapped angle which would not be in keeping with the other + axes which are specified as absolute angles. + + Attributes: + x (float): target x-coordinate in mm + y (float): target y-coordinate in mm + z (float): target z-coordinate in mm + phi (float): target phi angle in degrees + chi (float): target chi angle in degrees + """ x: float | None y: float | None