Skip to content

Fix for smargon combined moves with current slow-smargon workaround #1999

New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Merged
rtuck99 merged 5 commits into from
May 14, 2026
Merged

Fix for smargon combined moves with current slow-smargon workaround #1999

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
8c8f10d
Fix for smargon combined moves with current slow-smargon workaround
rtuck99 Mar 30, 2026
5c6d7ba
Remove omega from CombinedMoves
rtuck99 Apr 8, 2026
eb7d0e6
Make lint happy
rtuck99 Apr 8, 2026
b4f8891
Additional comment to describe the omission of omega from CombinedMove
rtuck99 May 14, 2026
f377ba6
Merge branch 'main' into 1998_serialize_omega_combined_move
rtuck99 May 14, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 15 additions & 17 deletions src/dodal/devices/smargon.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,9 @@
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


class StubPosition(Enum):
Expand Down Expand Up @@ -69,12 +69,23 @@ 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
z: float | None
omega: float | None
phi: float | None
chi: float | None

Expand Down Expand Up @@ -105,15 +116,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):
Copy link
Copy Markdown
Collaborator

@noemifrisina noemifrisina May 14, 2026

Choose a reason for hiding this comment

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

Nit: may be useful for future reference/future people looking at this mentioning either in this docstring or in the CombinedMove why it does not include the omega axis

"""This will move all motion together in a deferred move.
Expand All @@ -123,16 +125,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.
"""
LOGGER.info("Doing smargon move...")
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
Expand Down
72 changes: 17 additions & 55 deletions tests/devices/test_smargon.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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,
)
Expand All @@ -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)

Expand All @@ -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):
Expand Down
Loading