From 67e7c37fd9b07e7208cbec7d1e9c00dc635e07cd Mon Sep 17 00:00:00 2001 From: Oli Wenman Date: Thu, 12 Feb 2026 10:25:03 +0000 Subject: [PATCH 01/27] Initial attempt --- src/dodal/beamlines/__init__.py | 1 + src/dodal/beamlines/i09_2.py | 9 +- src/dodal/devices/beamlines/i09_2/__init__.py | 0 .../devices/beamlines/i09_2/i09_2_motors.py | 101 ++++++++++++++++++ 4 files changed, 110 insertions(+), 1 deletion(-) create mode 100644 src/dodal/devices/beamlines/i09_2/__init__.py create mode 100644 src/dodal/devices/beamlines/i09_2/i09_2_motors.py diff --git a/src/dodal/beamlines/__init__.py b/src/dodal/beamlines/__init__.py index e8a1a03be9b..e9d315dcb95 100644 --- a/src/dodal/beamlines/__init__.py +++ b/src/dodal/beamlines/__init__.py @@ -12,6 +12,7 @@ "i05-1": "i05_1", "b07-1": "b07_1", "i09-1": "i09_1", + "i09-2": "i09_2", "i13-1": "i13_1", "i15-1": "i15_1", "i10-1": "i10_1", diff --git a/src/dodal/beamlines/i09_2.py b/src/dodal/beamlines/i09_2.py index 99b29b7880b..0b2d4272c54 100644 --- a/src/dodal/beamlines/i09_2.py +++ b/src/dodal/beamlines/i09_2.py @@ -1,12 +1,14 @@ from dodal.beamlines.i09_2_shared import devices as i09_2_shared_devices from dodal.common.beamlines.beamline_utils import set_beamline as set_utils_beamline from dodal.device_manager import DeviceManager +from dodal.devices.beamlines.i09_2.i09_2_motors import I092SampleManipulator from dodal.devices.synchrotron import Synchrotron from dodal.log import set_beamline as set_log_beamline from dodal.utils import BeamlinePrefix, get_beamline_name BL = get_beamline_name("i09-2") -PREFIX = BeamlinePrefix(BL, suffix="J") +J_PREFIX = BeamlinePrefix(BL, suffix="J") +K_PREFIX = BeamlinePrefix(BL, suffix="K") set_log_beamline(BL) set_utils_beamline(BL) @@ -17,3 +19,8 @@ @devices.factory() def synchrotron() -> Synchrotron: return Synchrotron() + + +@devices.factory() +def sm() -> I092SampleManipulator: + return I092SampleManipulator(f"{K_PREFIX.beamline_prefix}-MO-SM-01:PI:") diff --git a/src/dodal/devices/beamlines/i09_2/__init__.py b/src/dodal/devices/beamlines/i09_2/__init__.py new file mode 100644 index 00000000000..e69de29bb2d diff --git a/src/dodal/devices/beamlines/i09_2/i09_2_motors.py b/src/dodal/devices/beamlines/i09_2/i09_2_motors.py new file mode 100644 index 00000000000..c79a7ad086e --- /dev/null +++ b/src/dodal/devices/beamlines/i09_2/i09_2_motors.py @@ -0,0 +1,101 @@ +import asyncio + +from bluesky.protocols import ( + Locatable, + Location, + Movable, + Reading, + Stoppable, + Subscribable, +) +from ophyd_async.core import ( + Callback, + StandardReadable, + StandardReadableFormat, + WatchableAsyncStatus, + WatcherUpdate, + observe_value, +) +from ophyd_async.epics.core import epics_signal_r, epics_signal_rw, epics_signal_w +from ophyd_async.epics.motor import Motor + + +class PiezoElectricMotor( + StandardReadable, Stoppable, Locatable[float], Subscribable[float], Movable[float] +): + def __init__(self, prefix: str, name: str = ""): + with self.add_children_as_readables(StandardReadableFormat.HINTED_SIGNAL): + self.user_readback = epics_signal_r(float, prefix + ":POS:RD") + + self.user_setpoint = epics_signal_rw(float, prefix + ":MOV:RD") + self.motor_stop = epics_signal_w(int, prefix + ":HLT:WR.PROC") + + # Whether set() should complete successfully or not + self._set_success = True + + super().__init__(name) + + def set_name(self, name: str, *, child_name_separator: str | None = None) -> None: + """Set name of the motor and its children.""" + super().set_name(name, child_name_separator=child_name_separator) + # Readback should be named the same as its parent in read() + self.user_readback.set_name(name) + + async def stop(self, success=False): + """Request to stop moving and return immediately.""" + self._set_success = success + # Put with completion will never complete as we are waiting for completion on + # the move above, so need to pass wait=False + await self.motor_stop.set(1, wait=False) + + async def locate(self) -> Location[float]: + """Return the current setpoint and readback of the motor.""" + setpoint, readback = await asyncio.gather( + self.user_setpoint.get_value(), self.user_readback.get_value() + ) + return Location(setpoint=setpoint, readback=readback) + + def subscribe_reading(self, function: Callback[dict[str, Reading[float]]]) -> None: + """Subscribe to reading.""" + self.user_readback.subscribe_reading(function) + + subscribe = subscribe_reading + + def clear_sub(self, function: Callback[dict[str, Reading[float]]]) -> None: + """Unsubscribe.""" + self.user_readback.clear_sub(function) + + @WatchableAsyncStatus.wrap + async def set(self, new_position: float): + """Move motor to the given value.""" + self._set_success = True + + old_position = await self.user_setpoint.get_value() + move_status = self.user_setpoint.set(new_position, wait=True) + async for current_position in observe_value( + self.user_readback, done_status=move_status + ): + yield WatcherUpdate( + current=current_position, + initial=old_position, + target=new_position, + name=self.name, + ) + if not self._set_success: + raise RuntimeError("Motor was stopped") + + +class I092SampleManipulator(StandardReadable): + def __init__(self, prefix: str, name: str = ""): + with self.add_children_as_readables(): + self.x1 = PiezoElectricMotor(prefix + "X1") + self.x2 = PiezoElectricMotor(prefix + "X2") + self.x3 = PiezoElectricMotor(prefix + "X3") + self.y = PiezoElectricMotor(prefix + "Y") + self.z1 = PiezoElectricMotor(prefix + "Z1") + self.z2 = PiezoElectricMotor(prefix + "Z2") + + self.xc = Motor(prefix + "X") + self.zc = Motor(prefix + "Z") + + super().__init__(name) From 44468b2ff7dfc058602d7daa462476ef7f6cb61c Mon Sep 17 00:00:00 2001 From: Oli Wenman Date: Mon, 15 Jun 2026 12:13:36 +0000 Subject: [PATCH 02/27] Update to use StandardMovable --- .../devices/beamlines/i09_2/i09_2_motors.py | 110 +++++++----------- 1 file changed, 42 insertions(+), 68 deletions(-) diff --git a/src/dodal/devices/beamlines/i09_2/i09_2_motors.py b/src/dodal/devices/beamlines/i09_2/i09_2_motors.py index c79a7ad086e..9466b6ff17f 100644 --- a/src/dodal/devices/beamlines/i09_2/i09_2_motors.py +++ b/src/dodal/devices/beamlines/i09_2/i09_2_motors.py @@ -1,88 +1,62 @@ -import asyncio +from dataclasses import dataclass +from functools import cached_property -from bluesky.protocols import ( - Locatable, - Location, - Movable, - Reading, - Stoppable, - Subscribable, -) from ophyd_async.core import ( - Callback, + MovableLogic, + SignalR, + SignalW, + StandardMovable, StandardReadable, StandardReadableFormat, - WatchableAsyncStatus, - WatcherUpdate, - observe_value, + derived_signal_r, + soft_signal_rw, ) from ophyd_async.epics.core import epics_signal_r, epics_signal_rw, epics_signal_w from ophyd_async.epics.motor import Motor -class PiezoElectricMotor( - StandardReadable, Stoppable, Locatable[float], Subscribable[float], Movable[float] -): - def __init__(self, prefix: str, name: str = ""): - with self.add_children_as_readables(StandardReadableFormat.HINTED_SIGNAL): - self.user_readback = epics_signal_r(float, prefix + ":POS:RD") +@dataclass +class PiezoElectricMovableLogic(MovableLogic): + deadband: SignalR[float] + motor_stop: SignalW[int] - self.user_setpoint = epics_signal_rw(float, prefix + ":MOV:RD") - self.motor_stop = epics_signal_w(int, prefix + ":HLT:WR.PROC") + async def stop(self) -> None: + await self.motor_stop.set(1) - # Whether set() should complete successfully or not - self._set_success = True + # How do provide calculate_timeout without velocity and acceleration? - super().__init__(name) - def set_name(self, name: str, *, child_name_separator: str | None = None) -> None: - """Set name of the motor and its children.""" - super().set_name(name, child_name_separator=child_name_separator) - # Readback should be named the same as its parent in read() - self.user_readback.set_name(name) +class PiezoElectricMotor(StandardMovable[float], StandardReadable): + """Motor like device with user_readback and user_setpoint.""" - async def stop(self, success=False): - """Request to stop moving and return immediately.""" - self._set_success = success - # Put with completion will never complete as we are waiting for completion on - # the move above, so need to pass wait=False - await self.motor_stop.set(1, wait=False) + def __init__(self, prefix: str, deadband: float = 0.01, name: str = ""): + with self.add_children_as_readables(StandardReadableFormat.HINTED_SIGNAL): + self.user_readback = epics_signal_r(float, prefix + ":POS:RD") + self.user_setpoint = epics_signal_rw(float, prefix + ":MOV:RD") + self.deadband = soft_signal_rw(float, initial_value=deadband) - async def locate(self) -> Location[float]: - """Return the current setpoint and readback of the motor.""" - setpoint, readback = await asyncio.gather( - self.user_setpoint.get_value(), self.user_readback.get_value() + self.motor_stop = epics_signal_w(int, prefix + ":HLT:WR.PROC") + self.within_threshold = derived_signal_r( + self._within_threshold_read, + setpoint=self.user_setpoint, + readback=self.user_readback, + deadband=self.deadband, ) - return Location(setpoint=setpoint, readback=readback) - - def subscribe_reading(self, function: Callback[dict[str, Reading[float]]]) -> None: - """Subscribe to reading.""" - self.user_readback.subscribe_reading(function) - - subscribe = subscribe_reading - - def clear_sub(self, function: Callback[dict[str, Reading[float]]]) -> None: - """Unsubscribe.""" - self.user_readback.clear_sub(function) - - @WatchableAsyncStatus.wrap - async def set(self, new_position: float): - """Move motor to the given value.""" - self._set_success = True + super().__init__(name) - old_position = await self.user_setpoint.get_value() - move_status = self.user_setpoint.set(new_position, wait=True) - async for current_position in observe_value( - self.user_readback, done_status=move_status - ): - yield WatcherUpdate( - current=current_position, - initial=old_position, - target=new_position, - name=self.name, - ) - if not self._set_success: - raise RuntimeError("Motor was stopped") + def _within_threshold_read( + self, setpoint: float, readback: float, deadband: float + ) -> bool: + return abs(setpoint - readback) < deadband + + @cached_property + def movable_logic(self) -> PiezoElectricMovableLogic: + return PiezoElectricMovableLogic( + readback=self.user_readback, + setpoint=self.user_setpoint, + deadband=self.deadband, + motor_stop=self.motor_stop, + ) class I092SampleManipulator(StandardReadable): From ec3b250fee3c39fb13c61ec4fde643d1fabfd6d6 Mon Sep 17 00:00:00 2001 From: Oli Wenman Date: Mon, 15 Jun 2026 15:44:56 +0000 Subject: [PATCH 03/27] Add tests for threshold --- src/dodal/devices/beamlines/i09_2/__init__.py | 7 +++ .../devices/beamlines/i09_2/i09_2_motors.py | 38 +++++++---- tests/devices/beamlines/i09_2/__init__.py | 0 .../beamlines/i09_2/test_i09_2_motors.py | 63 +++++++++++++++++++ 4 files changed, 96 insertions(+), 12 deletions(-) create mode 100644 tests/devices/beamlines/i09_2/__init__.py create mode 100644 tests/devices/beamlines/i09_2/test_i09_2_motors.py diff --git a/src/dodal/devices/beamlines/i09_2/__init__.py b/src/dodal/devices/beamlines/i09_2/__init__.py index e69de29bb2d..5bf0dafaa9e 100644 --- a/src/dodal/devices/beamlines/i09_2/__init__.py +++ b/src/dodal/devices/beamlines/i09_2/__init__.py @@ -0,0 +1,7 @@ +from .i09_2_motors import ( + I092SampleManipulator, + PiezoElectricMotor, + PiezoElectricMovableLogic, +) + +__all__ = ["I092SampleManipulator", "PiezoElectricMotor", "PiezoElectricMovableLogic"] diff --git a/src/dodal/devices/beamlines/i09_2/i09_2_motors.py b/src/dodal/devices/beamlines/i09_2/i09_2_motors.py index 9466b6ff17f..20ee531e536 100644 --- a/src/dodal/devices/beamlines/i09_2/i09_2_motors.py +++ b/src/dodal/devices/beamlines/i09_2/i09_2_motors.py @@ -9,6 +9,7 @@ StandardReadable, StandardReadableFormat, derived_signal_r, + set_and_wait_for_other_value, soft_signal_rw, ) from ophyd_async.epics.core import epics_signal_r, epics_signal_rw, epics_signal_w @@ -17,44 +18,57 @@ @dataclass class PiezoElectricMovableLogic(MovableLogic): - deadband: SignalR[float] + within_tolerance: SignalR[bool] motor_stop: SignalW[int] + async def move(self, new_position: float, timeout: float | None) -> None: + await set_and_wait_for_other_value( + set_signal=self.setpoint, + set_value=new_position, + match_signal=self.within_tolerance, + match_value=True, + timeout=timeout, + ) + async def stop(self) -> None: await self.motor_stop.set(1) # How do provide calculate_timeout without velocity and acceleration? +def _within_threshold_read(setpoint: float, readback: float, deadband: float) -> bool: + return abs(setpoint - readback) < deadband + + class PiezoElectricMotor(StandardMovable[float], StandardReadable): - """Motor like device with user_readback and user_setpoint.""" + """Motor like device with user_readback, user_setpoint, and a stop signals. Has a + configurable deadband soft signal to configure the tolerance of when a motor is done + moving. For example, if deadband is configured to be 0.5, and the setpoint is 10 and + the readback is 9.8, the motor will be done moving and stop blocking for the + AsyncStatus. + """ def __init__(self, prefix: str, deadband: float = 0.01, name: str = ""): with self.add_children_as_readables(StandardReadableFormat.HINTED_SIGNAL): self.user_readback = epics_signal_r(float, prefix + ":POS:RD") - self.user_setpoint = epics_signal_rw(float, prefix + ":MOV:RD") - self.deadband = soft_signal_rw(float, initial_value=deadband) + self.user_setpoint = epics_signal_rw(float, prefix + ":MOV:RD") + self.deadband = soft_signal_rw(float, initial_value=deadband) self.motor_stop = epics_signal_w(int, prefix + ":HLT:WR.PROC") self.within_threshold = derived_signal_r( - self._within_threshold_read, + _within_threshold_read, + deadband=self.deadband, setpoint=self.user_setpoint, readback=self.user_readback, - deadband=self.deadband, ) super().__init__(name) - def _within_threshold_read( - self, setpoint: float, readback: float, deadband: float - ) -> bool: - return abs(setpoint - readback) < deadband - @cached_property def movable_logic(self) -> PiezoElectricMovableLogic: return PiezoElectricMovableLogic( readback=self.user_readback, setpoint=self.user_setpoint, - deadband=self.deadband, + within_tolerance=self.within_threshold, motor_stop=self.motor_stop, ) diff --git a/tests/devices/beamlines/i09_2/__init__.py b/tests/devices/beamlines/i09_2/__init__.py new file mode 100644 index 00000000000..e69de29bb2d diff --git a/tests/devices/beamlines/i09_2/test_i09_2_motors.py b/tests/devices/beamlines/i09_2/test_i09_2_motors.py new file mode 100644 index 00000000000..6fdec5ec31b --- /dev/null +++ b/tests/devices/beamlines/i09_2/test_i09_2_motors.py @@ -0,0 +1,63 @@ +import pytest +from ophyd_async.core import DeviceMock, init_devices, set_mock_value +from ophyd_async.testing import assert_reading, partial_reading + +from dodal.devices.beamlines.i09_2 import ( + I092SampleManipulator, + PiezoElectricMotor, +) + + +@pytest.fixture +def sm() -> I092SampleManipulator: + with init_devices(mock=True): + sm = I092SampleManipulator("TEST:") + return sm + + +async def test_sm_read(sm: I092SampleManipulator) -> None: + await assert_reading( + sm, + { + "sm-x1": partial_reading(0), + "sm-x2": partial_reading(0), + "sm-x3": partial_reading(0), + "sm-y": partial_reading(0), + "sm-z1": partial_reading(0), + "sm-z2": partial_reading(0), + "sm-xc": partial_reading(0), + "sm-zc": partial_reading(0), + }, + ) + + +@pytest.fixture +async def piezo_motor() -> PiezoElectricMotor: + piezo_motor = PiezoElectricMotor("TEST:", name="piezo_motor") + # Setup mock to not use default as do not want the setpoint and readback to be the + # same for tests so we can correctly test threshold signals. + await piezo_motor.connect(mock=DeviceMock()) + return piezo_motor + + +@pytest.mark.parametrize( + "deadband, setpoint, readback, expected_within_threshold", + ( + (1, 10, 11.1, False), + (2, 10, 7, False), + (0.5, 5, 4.9, True), + (0.1, 100, 100.01, True), + ), +) +async def test_piezo_motor_within_threshold( + piezo_motor: PiezoElectricMotor, + deadband: float, + setpoint: float, + readback: float, + expected_within_threshold: bool, +) -> None: + set_mock_value(piezo_motor.deadband, deadband) + set_mock_value(piezo_motor.user_setpoint, setpoint) + set_mock_value(piezo_motor.user_readback, readback) + + assert await piezo_motor.within_threshold.get_value() == expected_within_threshold From 4f7227521cf5ccb7ffd5ba27d3b26e475d206138 Mon Sep 17 00:00:00 2001 From: Oli Wenman Date: Tue, 16 Jun 2026 10:36:01 +0000 Subject: [PATCH 04/27] Add motor stop test --- tests/devices/beamlines/i09_2/test_i09_2_motors.py | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/tests/devices/beamlines/i09_2/test_i09_2_motors.py b/tests/devices/beamlines/i09_2/test_i09_2_motors.py index 6fdec5ec31b..e268e528ab3 100644 --- a/tests/devices/beamlines/i09_2/test_i09_2_motors.py +++ b/tests/devices/beamlines/i09_2/test_i09_2_motors.py @@ -1,3 +1,5 @@ +from unittest.mock import AsyncMock + import pytest from ophyd_async.core import DeviceMock, init_devices, set_mock_value from ophyd_async.testing import assert_reading, partial_reading @@ -61,3 +63,9 @@ async def test_piezo_motor_within_threshold( set_mock_value(piezo_motor.user_readback, readback) assert await piezo_motor.within_threshold.get_value() == expected_within_threshold + + +async def test_piezo_motor_stop(piezo_motor: PiezoElectricMotor) -> None: + piezo_motor.motor_stop.set = AsyncMock() + await piezo_motor.stop() + piezo_motor.motor_stop.set.assert_awaited_once_with(1) From 983b14b8e97145f2b37e11cf45de86ffc7daef71 Mon Sep 17 00:00:00 2001 From: Oli Wenman Date: Tue, 16 Jun 2026 10:51:54 +0000 Subject: [PATCH 05/27] Add missing tests --- .../devices/beamlines/i09_2/i09_2_motors.py | 8 +++--- .../beamlines/i09_2/test_i09_2_motors.py | 28 +++++++++++++++---- 2 files changed, 27 insertions(+), 9 deletions(-) diff --git a/src/dodal/devices/beamlines/i09_2/i09_2_motors.py b/src/dodal/devices/beamlines/i09_2/i09_2_motors.py index 20ee531e536..f6c1943ae59 100644 --- a/src/dodal/devices/beamlines/i09_2/i09_2_motors.py +++ b/src/dodal/devices/beamlines/i09_2/i09_2_motors.py @@ -36,7 +36,7 @@ async def stop(self) -> None: # How do provide calculate_timeout without velocity and acceleration? -def _within_threshold_read(setpoint: float, readback: float, deadband: float) -> bool: +def _within_tolerance_read(setpoint: float, readback: float, deadband: float) -> bool: return abs(setpoint - readback) < deadband @@ -55,8 +55,8 @@ def __init__(self, prefix: str, deadband: float = 0.01, name: str = ""): self.user_setpoint = epics_signal_rw(float, prefix + ":MOV:RD") self.deadband = soft_signal_rw(float, initial_value=deadband) self.motor_stop = epics_signal_w(int, prefix + ":HLT:WR.PROC") - self.within_threshold = derived_signal_r( - _within_threshold_read, + self.within_tolerance = derived_signal_r( + _within_tolerance_read, deadband=self.deadband, setpoint=self.user_setpoint, readback=self.user_readback, @@ -68,7 +68,7 @@ def movable_logic(self) -> PiezoElectricMovableLogic: return PiezoElectricMovableLogic( readback=self.user_readback, setpoint=self.user_setpoint, - within_tolerance=self.within_threshold, + within_tolerance=self.within_tolerance, motor_stop=self.motor_stop, ) diff --git a/tests/devices/beamlines/i09_2/test_i09_2_motors.py b/tests/devices/beamlines/i09_2/test_i09_2_motors.py index e268e528ab3..0ce1386b643 100644 --- a/tests/devices/beamlines/i09_2/test_i09_2_motors.py +++ b/tests/devices/beamlines/i09_2/test_i09_2_motors.py @@ -35,10 +35,8 @@ async def test_sm_read(sm: I092SampleManipulator) -> None: @pytest.fixture async def piezo_motor() -> PiezoElectricMotor: - piezo_motor = PiezoElectricMotor("TEST:", name="piezo_motor") - # Setup mock to not use default as do not want the setpoint and readback to be the - # same for tests so we can correctly test threshold signals. - await piezo_motor.connect(mock=DeviceMock()) + with init_devices(mock=True): + piezo_motor = PiezoElectricMotor("TEST:") return piezo_motor @@ -58,14 +56,34 @@ async def test_piezo_motor_within_threshold( readback: float, expected_within_threshold: bool, ) -> None: + # Setup mock to not use default as do not want the setpoint and readback to be the + # same for tests so we can correctly test threshold signals. + await piezo_motor.connect(mock=DeviceMock()) set_mock_value(piezo_motor.deadband, deadband) set_mock_value(piezo_motor.user_setpoint, setpoint) set_mock_value(piezo_motor.user_readback, readback) - assert await piezo_motor.within_threshold.get_value() == expected_within_threshold + assert await piezo_motor.within_tolerance.get_value() == expected_within_threshold async def test_piezo_motor_stop(piezo_motor: PiezoElectricMotor) -> None: piezo_motor.motor_stop.set = AsyncMock() await piezo_motor.stop() piezo_motor.motor_stop.set.assert_awaited_once_with(1) + + +async def test_piezo_motor_set(piezo_motor: PiezoElectricMotor) -> None: + await piezo_motor.set(4) + assert await piezo_motor.user_readback.get_value() == 4 + assert await piezo_motor.user_setpoint.get_value() == 4 + + +async def test_tolerance_logic_move(piezo_motor: PiezoElectricMotor): + set_mock_value(piezo_motor.movable_logic.readback, 0.0) + move_task = piezo_motor.movable_logic.move(new_position=10.0, timeout=5.0) + for value in [2.0, 5.0, 8.0, 9.5, 13.0]: + set_mock_value(piezo_motor.movable_logic.readback, value) + assert await piezo_motor.within_tolerance.get_value() is False + set_mock_value(piezo_motor.movable_logic.readback, 9.91) + await move_task + assert await piezo_motor.within_tolerance.get_value() is True From c98dd31d2ea0d2a22df0e781449954aa1f85d134 Mon Sep 17 00:00:00 2001 From: Oli Wenman Date: Tue, 16 Jun 2026 14:06:23 +0000 Subject: [PATCH 06/27] Update classes to use MovableWithTolerance --- .../devices/beamlines/i09_2/i09_2_motors.py | 36 ++----- .../high_field_magnet/high_field_magnet.py | 42 +++----- src/dodal/devices/movable.py | 60 +++++++++++ .../beamlines/i09_2/test_i09_2_motors.py | 39 +------ tests/devices/test_movable.py | 101 ++++++++++++++++++ 5 files changed, 183 insertions(+), 95 deletions(-) create mode 100644 src/dodal/devices/movable.py create mode 100644 tests/devices/test_movable.py diff --git a/src/dodal/devices/beamlines/i09_2/i09_2_motors.py b/src/dodal/devices/beamlines/i09_2/i09_2_motors.py index f6c1943ae59..3f723014545 100644 --- a/src/dodal/devices/beamlines/i09_2/i09_2_motors.py +++ b/src/dodal/devices/beamlines/i09_2/i09_2_motors.py @@ -2,45 +2,28 @@ from functools import cached_property from ophyd_async.core import ( - MovableLogic, - SignalR, SignalW, - StandardMovable, StandardReadable, StandardReadableFormat, - derived_signal_r, - set_and_wait_for_other_value, soft_signal_rw, ) from ophyd_async.epics.core import epics_signal_r, epics_signal_rw, epics_signal_w from ophyd_async.epics.motor import Motor +from dodal.devices.movable import MovableWithTolerance, MovableWithToleranceLogic + @dataclass -class PiezoElectricMovableLogic(MovableLogic): - within_tolerance: SignalR[bool] +class PiezoElectricMovableLogic(MovableWithToleranceLogic): motor_stop: SignalW[int] - async def move(self, new_position: float, timeout: float | None) -> None: - await set_and_wait_for_other_value( - set_signal=self.setpoint, - set_value=new_position, - match_signal=self.within_tolerance, - match_value=True, - timeout=timeout, - ) - async def stop(self) -> None: await self.motor_stop.set(1) # How do provide calculate_timeout without velocity and acceleration? -def _within_tolerance_read(setpoint: float, readback: float, deadband: float) -> bool: - return abs(setpoint - readback) < deadband - - -class PiezoElectricMotor(StandardMovable[float], StandardReadable): +class PiezoElectricMotor(MovableWithTolerance): """Motor like device with user_readback, user_setpoint, and a stop signals. Has a configurable deadband soft signal to configure the tolerance of when a motor is done moving. For example, if deadband is configured to be 0.5, and the setpoint is 10 and @@ -48,20 +31,19 @@ class PiezoElectricMotor(StandardMovable[float], StandardReadable): AsyncStatus. """ - def __init__(self, prefix: str, deadband: float = 0.01, name: str = ""): + def __init__(self, prefix: str, tolerance: float = 0.01, name: str = ""): with self.add_children_as_readables(StandardReadableFormat.HINTED_SIGNAL): self.user_readback = epics_signal_r(float, prefix + ":POS:RD") self.user_setpoint = epics_signal_rw(float, prefix + ":MOV:RD") - self.deadband = soft_signal_rw(float, initial_value=deadband) + self.tolerance = soft_signal_rw(float, initial_value=tolerance) self.motor_stop = epics_signal_w(int, prefix + ":HLT:WR.PROC") - self.within_tolerance = derived_signal_r( - _within_tolerance_read, - deadband=self.deadband, + super().__init__( + tolerance=self.tolerance, setpoint=self.user_setpoint, readback=self.user_readback, + name=name, ) - super().__init__(name) @cached_property def movable_logic(self) -> PiezoElectricMovableLogic: diff --git a/src/dodal/devices/beamlines/i10_1/high_field_magnet/high_field_magnet.py b/src/dodal/devices/beamlines/i10_1/high_field_magnet/high_field_magnet.py index ff39102e93b..e2791f05ee9 100644 --- a/src/dodal/devices/beamlines/i10_1/high_field_magnet/high_field_magnet.py +++ b/src/dodal/devices/beamlines/i10_1/high_field_magnet/high_field_magnet.py @@ -10,23 +10,19 @@ from ophyd_async.core import ( DEFAULT_TIMEOUT, AsyncStatus, - MovableLogic, - SignalR, SignalRW, - StandardMovable, - StandardReadable, StandardReadableFormat, StrictEnum, SubsetEnum, WatchableAsyncStatus, - derived_signal_r, error_if_none, - set_and_wait_for_other_value, soft_signal_rw, ) from ophyd_async.epics.core import epics_signal_r, epics_signal_rw, epics_signal_w from pydantic import BaseModel, Field +from dodal.devices.movable import MovableWithTolerance, MovableWithToleranceLogic + class HighFieldMangetSweepTypes(StrictEnum): FAST = "Fast" @@ -56,11 +52,9 @@ class FlyMagInfo(BaseModel): @dataclass -class ToleranceMovableLogic(MovableLogic[float]): - tolerance: SignalRW[float] +class HighFieldMagnetMovableLogic(MovableWithToleranceLogic): speed: SignalRW[float] acc_time: SignalRW[float] - within_tolerance: SignalR[bool] async def stop(self): current_val = await self.readback.get_value() @@ -81,19 +75,9 @@ async def calculate_timeout( raise ValueError(msg) from error return timeout - async def move(self, new_position: float, timeout: float | None) -> None: - await set_and_wait_for_other_value( - set_signal=self.setpoint, - set_value=new_position, - match_signal=self.within_tolerance, - match_value=True, - timeout=timeout, - ) - class HighFieldMagnet( - StandardMovable, - StandardReadable, + MovableWithTolerance, Flyable, Preparable, ): @@ -130,17 +114,16 @@ def __init__( write_pv=prefix + "SET:SETPOINTFIELD", ) - self.within_tolerance = derived_signal_r( - raw_to_derived=self._within_tolerance, - setpoint=self.user_setpoint, - readback=self.user_readback, - tolerance=self.field_tolerance, - ) self._set_success = True self._fly_info: FlyMagInfo | None = None self._fly_status: WatchableAsyncStatus | None = None - super().__init__(name=name) + super().__init__( + tolerance=self.field_tolerance, + setpoint=self.user_setpoint, + readback=self.user_readback, + name=name, + ) def _within_tolerance( self, setpoint: float, readback: float, tolerance: float @@ -149,11 +132,10 @@ def _within_tolerance( return abs(setpoint - readback) < abs(tolerance) @cached_property - def movable_logic(self) -> MovableLogic: - return ToleranceMovableLogic( + def movable_logic(self) -> HighFieldMagnetMovableLogic: + return HighFieldMagnetMovableLogic( setpoint=self.user_setpoint, readback=self.user_readback, - tolerance=self.field_tolerance, within_tolerance=self.within_tolerance, speed=self.sweep_rate, acc_time=self.ramp_up_time, diff --git a/src/dodal/devices/movable.py b/src/dodal/devices/movable.py new file mode 100644 index 00000000000..042b4a00d79 --- /dev/null +++ b/src/dodal/devices/movable.py @@ -0,0 +1,60 @@ +from dataclasses import dataclass +from functools import cached_property + +from ophyd_async.core import ( + MovableLogic, + Reference, + SignalR, + SignalRW, + StandardMovable, + StandardReadable, + derived_signal_r, + set_and_wait_for_other_value, +) + + +@dataclass +class MovableWithToleranceLogic(MovableLogic[float]): + within_tolerance: SignalR[bool] + + async def move(self, new_position: float, timeout: float | None) -> None: + await set_and_wait_for_other_value( + set_signal=self.setpoint, + set_value=new_position, + match_signal=self.within_tolerance, + match_value=True, + timeout=timeout, + ) + + +def _within_tolerance_read(setpoint: float, readback: float, tolerance: float) -> bool: + return abs(setpoint - readback) < tolerance + + +class MovableWithTolerance(StandardMovable[float], StandardReadable): + def __init__( + self, + tolerance: SignalR[float], + setpoint: SignalRW[float], + readback: SignalR[float], + name: str = "", + ): + # Use reference so sub classes still have flexibility to name signals to what + # they want. + self._setpoint_ref = Reference(setpoint) + self._readback_ref = Reference(readback) + self.within_tolerance = derived_signal_r( + _within_tolerance_read, + tolerance=tolerance, + setpoint=setpoint, + readback=readback, + ) + super().__init__(name) + + @cached_property + def movable_logic(self) -> MovableWithToleranceLogic: + return MovableWithToleranceLogic( + readback=self._readback_ref(), + setpoint=self._setpoint_ref(), + within_tolerance=self.within_tolerance, + ) diff --git a/tests/devices/beamlines/i09_2/test_i09_2_motors.py b/tests/devices/beamlines/i09_2/test_i09_2_motors.py index 0ce1386b643..eee0f063521 100644 --- a/tests/devices/beamlines/i09_2/test_i09_2_motors.py +++ b/tests/devices/beamlines/i09_2/test_i09_2_motors.py @@ -1,7 +1,7 @@ from unittest.mock import AsyncMock import pytest -from ophyd_async.core import DeviceMock, init_devices, set_mock_value +from ophyd_async.core import init_devices from ophyd_async.testing import assert_reading, partial_reading from dodal.devices.beamlines.i09_2 import ( @@ -40,32 +40,6 @@ async def piezo_motor() -> PiezoElectricMotor: return piezo_motor -@pytest.mark.parametrize( - "deadband, setpoint, readback, expected_within_threshold", - ( - (1, 10, 11.1, False), - (2, 10, 7, False), - (0.5, 5, 4.9, True), - (0.1, 100, 100.01, True), - ), -) -async def test_piezo_motor_within_threshold( - piezo_motor: PiezoElectricMotor, - deadband: float, - setpoint: float, - readback: float, - expected_within_threshold: bool, -) -> None: - # Setup mock to not use default as do not want the setpoint and readback to be the - # same for tests so we can correctly test threshold signals. - await piezo_motor.connect(mock=DeviceMock()) - set_mock_value(piezo_motor.deadband, deadband) - set_mock_value(piezo_motor.user_setpoint, setpoint) - set_mock_value(piezo_motor.user_readback, readback) - - assert await piezo_motor.within_tolerance.get_value() == expected_within_threshold - - async def test_piezo_motor_stop(piezo_motor: PiezoElectricMotor) -> None: piezo_motor.motor_stop.set = AsyncMock() await piezo_motor.stop() @@ -76,14 +50,3 @@ async def test_piezo_motor_set(piezo_motor: PiezoElectricMotor) -> None: await piezo_motor.set(4) assert await piezo_motor.user_readback.get_value() == 4 assert await piezo_motor.user_setpoint.get_value() == 4 - - -async def test_tolerance_logic_move(piezo_motor: PiezoElectricMotor): - set_mock_value(piezo_motor.movable_logic.readback, 0.0) - move_task = piezo_motor.movable_logic.move(new_position=10.0, timeout=5.0) - for value in [2.0, 5.0, 8.0, 9.5, 13.0]: - set_mock_value(piezo_motor.movable_logic.readback, value) - assert await piezo_motor.within_tolerance.get_value() is False - set_mock_value(piezo_motor.movable_logic.readback, 9.91) - await move_task - assert await piezo_motor.within_tolerance.get_value() is True diff --git a/tests/devices/test_movable.py b/tests/devices/test_movable.py new file mode 100644 index 00000000000..d4e645e1919 --- /dev/null +++ b/tests/devices/test_movable.py @@ -0,0 +1,101 @@ +import pytest +from ophyd_async.core import ( + DeviceMock, + init_devices, + set_mock_value, + soft_signal_r_and_setter, + soft_signal_rw, +) + +from dodal.devices.movable import MovableWithTolerance + + +class MovableWithToleranceImpl(MovableWithTolerance): + def __init__(self, name: str = ""): + self.custom_tolerance = soft_signal_rw(float) + self.custom_setpoint = soft_signal_rw(float) + self.custom_readback, _ = soft_signal_r_and_setter(float) + super().__init__( + tolerance=self.custom_tolerance, + setpoint=self.custom_setpoint, + readback=self.custom_readback, + name=name, + ) + + +@pytest.fixture +async def movable_with_tolerance() -> MovableWithToleranceImpl: + with init_devices(mock=True): + movable_with_tolerance = MovableWithToleranceImpl() + return movable_with_tolerance + + +# @pytest.mark.parametrize( +# "tolerance, setpoint, readback, expected_within_threshold", +# ( +# (1, 10, 11.1, False), +# (2, 10, 7, False), +# (0.5, 5, 4.9, True), +# (0.1, 100, 100.01, True), +# ), +# ) +@pytest.mark.parametrize( + "setpoint, readback, tolerance, expected_within_threshold", + [ + (10.0, 10.005, 0.01, True), # test positive tolerance + (10.0, 9.995, 0.01, True), + (10.0, 10.02, 0.01, False), + (10.0, 0.9, 0.01, False), + (10.0, 9.995, -0.01, True), # test negative tolerance + (10.0, 10.0005, -0.01, True), + (10.0, 9.98, -0.01, False), + (10.0, 10.1, -0.01, False), + ], +) +async def test_movable_with_tolerance_within_threshold( + movable_with_tolerance: MovableWithToleranceImpl, + tolerance: float, + setpoint: float, + readback: float, + expected_within_threshold: bool, +) -> None: + # Setup mock to not use default as do not want the setpoint and readback to be the + # same for tests so we can correctly test threshold signal. + await movable_with_tolerance.connect(mock=DeviceMock()) + set_mock_value(movable_with_tolerance.custom_tolerance, tolerance) + set_mock_value(movable_with_tolerance.custom_setpoint, setpoint) + set_mock_value(movable_with_tolerance.custom_readback, readback) + assert ( + await movable_with_tolerance.within_tolerance.get_value() + == expected_within_threshold + ) + + +async def test_movable_with_tolerance_logic_move( + movable_with_tolerance: MovableWithToleranceImpl, +) -> None: + set_mock_value(movable_with_tolerance.movable_logic.readback, 0.0) + move_task = movable_with_tolerance.movable_logic.move( + new_position=10.0, timeout=5.0 + ) + for value in [2.0, 5.0, 8.0, 9.5, 13.0]: + set_mock_value(movable_with_tolerance.movable_logic.readback, value) + assert await movable_with_tolerance.within_tolerance.get_value() is False + set_mock_value(movable_with_tolerance.movable_logic.readback, 9.91) + await move_task + assert await movable_with_tolerance.within_tolerance.get_value() is True + + +async def test_movable_with_tolerance_sub_class_signal_names_are_not_renamed( + movable_with_tolerance: MovableWithToleranceImpl, +) -> None: + assert ( + movable_with_tolerance.custom_setpoint.name + == "movable_with_tolerance-custom_setpoint" + ) + assert ( + movable_with_tolerance.custom_tolerance.name + == "movable_with_tolerance-custom_tolerance" + ) + # Readback is the exception as renamed by StandardMovable to be device name + assert movable_with_tolerance.custom_readback.name == "movable_with_tolerance" From e223a439e10993935f59a597a020bff90129669b Mon Sep 17 00:00:00 2001 From: Oli Wenman Date: Tue, 16 Jun 2026 14:08:33 +0000 Subject: [PATCH 07/27] Fix test --- src/dodal/devices/movable.py | 2 +- tests/devices/test_movable.py | 9 --------- 2 files changed, 1 insertion(+), 10 deletions(-) diff --git a/src/dodal/devices/movable.py b/src/dodal/devices/movable.py index 042b4a00d79..1b5ddb0a605 100644 --- a/src/dodal/devices/movable.py +++ b/src/dodal/devices/movable.py @@ -28,7 +28,7 @@ async def move(self, new_position: float, timeout: float | None) -> None: def _within_tolerance_read(setpoint: float, readback: float, tolerance: float) -> bool: - return abs(setpoint - readback) < tolerance + return abs(setpoint - readback) < abs(tolerance) class MovableWithTolerance(StandardMovable[float], StandardReadable): diff --git a/tests/devices/test_movable.py b/tests/devices/test_movable.py index d4e645e1919..ca7f1a816c5 100644 --- a/tests/devices/test_movable.py +++ b/tests/devices/test_movable.py @@ -30,15 +30,6 @@ async def movable_with_tolerance() -> MovableWithToleranceImpl: return movable_with_tolerance -# @pytest.mark.parametrize( -# "tolerance, setpoint, readback, expected_within_threshold", -# ( -# (1, 10, 11.1, False), -# (2, 10, 7, False), -# (0.5, 5, 4.9, True), -# (0.1, 100, 100.01, True), -# ), -# ) @pytest.mark.parametrize( "setpoint, readback, tolerance, expected_within_threshold", [ From 4c81aa59528ca29be9e42ee22a7b5eab3b25b403 Mon Sep 17 00:00:00 2001 From: Oli Wenman Date: Tue, 16 Jun 2026 14:10:41 +0000 Subject: [PATCH 08/27] Remove duplicate tests --- .../test_high_field_magnet.py | 35 ------------------- 1 file changed, 35 deletions(-) diff --git a/tests/devices/beamlines/i10_1/high_field_magnet/test_high_field_magnet.py b/tests/devices/beamlines/i10_1/high_field_magnet/test_high_field_magnet.py index e39f153e9e3..11c6cae3ac9 100644 --- a/tests/devices/beamlines/i10_1/high_field_magnet/test_high_field_magnet.py +++ b/tests/devices/beamlines/i10_1/high_field_magnet/test_high_field_magnet.py @@ -147,32 +147,9 @@ async def test_read(high_field_magnet: HighFieldMagnet): ) -@pytest.mark.parametrize( - "setpoint,readback,tolerance,expected", - [ - (10.0, 10.005, 0.01, True), # test positive tolerance - (10.0, 9.995, 0.01, True), - (10.0, 10.02, 0.01, False), - (10.0, 0.9, 0.01, False), - (10.0, 9.995, -0.01, True), # test negative tolerance - (10.0, 10.0005, -0.01, True), - (10.0, 9.98, -0.01, False), - (10.0, 10.1, -0.01, False), - ], -) -async def test_tolerance_logic_within_tolerance( - high_field_magnet: HighFieldMagnet, setpoint, readback, tolerance, expected -): - result = high_field_magnet._within_tolerance( - setpoint=setpoint, readback=readback, tolerance=tolerance - ) - assert result is expected - - async def test_tolerance_logic_stop_clears_set_success_and_restores_setpoint( high_field_magnet: HighFieldMagnet, ): - set_mock_value(high_field_magnet.movable_logic.readback, 7.5) set_mock_value(high_field_magnet.movable_logic.readback, 1.5) await high_field_magnet.stop() assert high_field_magnet._set_success is False @@ -189,18 +166,6 @@ async def test_tolerance_logic_calculate_timeout_with_zero_speed( ) -async def test_tolerance_logic_move(high_field_magnet: HighFieldMagnet): - set_mock_value(high_field_magnet.movable_logic.readback, 0.0) - move_task = high_field_magnet.movable_logic.move(new_position=10.0, timeout=5.0) - for value in [2.0, 5.0, 8.0, 9.5, 13.0]: - set_mock_value(high_field_magnet.movable_logic.readback, value) - await asyncio.sleep(0.0) - assert await high_field_magnet.within_tolerance.get_value() is False - set_mock_value(high_field_magnet.movable_logic.readback, 9.91) - await move_task - assert await high_field_magnet.within_tolerance.get_value() is True - - def test_run_engine_scan( run_engine: RunEngine, high_field_magnet: HighFieldMagnet, From 3f51519f6a42e3772a86d565f86ede9e32764539 Mon Sep 17 00:00:00 2001 From: Oli Wenman Date: Tue, 16 Jun 2026 14:31:36 +0000 Subject: [PATCH 09/27] Update doc strings and remove unused methods --- src/dodal/devices/beamlines/i09_2/i09_2_motors.py | 4 ++-- .../i10_1/high_field_magnet/high_field_magnet.py | 14 ++------------ src/dodal/devices/movable.py | 4 ++++ 3 files changed, 8 insertions(+), 14 deletions(-) diff --git a/src/dodal/devices/beamlines/i09_2/i09_2_motors.py b/src/dodal/devices/beamlines/i09_2/i09_2_motors.py index 3f723014545..a73e4307a4b 100644 --- a/src/dodal/devices/beamlines/i09_2/i09_2_motors.py +++ b/src/dodal/devices/beamlines/i09_2/i09_2_motors.py @@ -25,8 +25,8 @@ async def stop(self) -> None: class PiezoElectricMotor(MovableWithTolerance): """Motor like device with user_readback, user_setpoint, and a stop signals. Has a - configurable deadband soft signal to configure the tolerance of when a motor is done - moving. For example, if deadband is configured to be 0.5, and the setpoint is 10 and + configurable tolerance soft signal to configure the tolerance of when a motor is done + moving. For example, if tolerance is configured to be 0.5, and the setpoint is 10 and the readback is 9.8, the motor will be done moving and stop blocking for the AsyncStatus. """ diff --git a/src/dodal/devices/beamlines/i10_1/high_field_magnet/high_field_magnet.py b/src/dodal/devices/beamlines/i10_1/high_field_magnet/high_field_magnet.py index e2791f05ee9..b7df0e67d83 100644 --- a/src/dodal/devices/beamlines/i10_1/high_field_magnet/high_field_magnet.py +++ b/src/dodal/devices/beamlines/i10_1/high_field_magnet/high_field_magnet.py @@ -76,11 +76,7 @@ async def calculate_timeout( return timeout -class HighFieldMagnet( - MovableWithTolerance, - Flyable, - Preparable, -): +class HighFieldMagnet(MovableWithTolerance, Flyable, Preparable): def __init__( self, prefix: str, field_tolerance: float = 0.01, name: str = "" ) -> None: @@ -101,6 +97,7 @@ def __init__( ) self.ramp_up_time = soft_signal_rw(datatype=float, initial_value=1.0) self.field_tolerance = soft_signal_rw(float, initial_value=field_tolerance) + with self.add_children_as_readables(StandardReadableFormat.HINTED_SIGNAL): self.user_readback = epics_signal_r(float, prefix + "RBV:DEMANDFIELD") @@ -113,7 +110,6 @@ def __init__( read_pv=prefix + "RBV:SETPOINTFIELD", write_pv=prefix + "SET:SETPOINTFIELD", ) - self._set_success = True self._fly_info: FlyMagInfo | None = None self._fly_status: WatchableAsyncStatus | None = None @@ -125,12 +121,6 @@ def __init__( name=name, ) - def _within_tolerance( - self, setpoint: float, readback: float, tolerance: float - ) -> bool: - """Check if the readback is within the tolerance of the setpoint.""" - return abs(setpoint - readback) < abs(tolerance) - @cached_property def movable_logic(self) -> HighFieldMagnetMovableLogic: return HighFieldMagnetMovableLogic( diff --git a/src/dodal/devices/movable.py b/src/dodal/devices/movable.py index 1b5ddb0a605..15f6842bce9 100644 --- a/src/dodal/devices/movable.py +++ b/src/dodal/devices/movable.py @@ -32,6 +32,10 @@ def _within_tolerance_read(setpoint: float, readback: float, tolerance: float) - class MovableWithTolerance(StandardMovable[float], StandardReadable): + """Movable with a signal to configure the tolerance of when the device is done + moving if it the readback and setpoint difference is within the tolerance. + """ + def __init__( self, tolerance: SignalR[float], From 50181463fdc94d7665c016ee9b0908ef6d91c5ee Mon Sep 17 00:00:00 2001 From: Oli Wenman Date: Tue, 16 Jun 2026 15:10:10 +0000 Subject: [PATCH 10/27] Improve failing threshold test --- tests/devices/test_movable.py | 37 ++++++++++++++++++++--------------- 1 file changed, 21 insertions(+), 16 deletions(-) diff --git a/tests/devices/test_movable.py b/tests/devices/test_movable.py index ca7f1a816c5..ade22e1e907 100644 --- a/tests/devices/test_movable.py +++ b/tests/devices/test_movable.py @@ -1,7 +1,7 @@ import pytest from ophyd_async.core import ( + AsyncStatus, DeviceMock, - init_devices, set_mock_value, soft_signal_r_and_setter, soft_signal_rw, @@ -25,8 +25,10 @@ def __init__(self, name: str = ""): @pytest.fixture async def movable_with_tolerance() -> MovableWithToleranceImpl: - with init_devices(mock=True): - movable_with_tolerance = MovableWithToleranceImpl() + movable_with_tolerance = MovableWithToleranceImpl("movable_with_tolerance") + # Setup mock to not use default as do not want the setpoint and readback to be the + # same for tests so we can correctly test threshold signal. + await movable_with_tolerance.connect(mock=DeviceMock()) return movable_with_tolerance @@ -50,9 +52,6 @@ async def test_movable_with_tolerance_within_threshold( readback: float, expected_within_threshold: bool, ) -> None: - # Setup mock to not use default as do not want the setpoint and readback to be the - # same for tests so we can correctly test threshold signal. - await movable_with_tolerance.connect(mock=DeviceMock()) set_mock_value(movable_with_tolerance.custom_tolerance, tolerance) set_mock_value(movable_with_tolerance.custom_setpoint, setpoint) set_mock_value(movable_with_tolerance.custom_readback, readback) @@ -62,19 +61,25 @@ async def test_movable_with_tolerance_within_threshold( ) -async def test_movable_with_tolerance_logic_move( +async def test_movable_with_tolerance_logic_moves_to_setpoint_and_is_done_when_wiithin_tolerance( movable_with_tolerance: MovableWithToleranceImpl, ) -> None: + set_mock_value(movable_with_tolerance.custom_tolerance, 0.1) set_mock_value(movable_with_tolerance.movable_logic.readback, 0.0) - move_task = movable_with_tolerance.movable_logic.move( - new_position=10.0, timeout=5.0 - ) - for value in [2.0, 5.0, 8.0, 9.5, 13.0]: - set_mock_value(movable_with_tolerance.movable_logic.readback, value) - assert await movable_with_tolerance.within_tolerance.get_value() is False - set_mock_value(movable_with_tolerance.movable_logic.readback, 9.91) - await move_task - assert await movable_with_tolerance.within_tolerance.get_value() is True + async with AsyncStatus( + movable_with_tolerance.movable_logic.move(new_position=10, timeout=1) + ) as move_status: + # Start the move by setting the setpoint. + # Set some values between initial readback and final setpoint that are outside + # the threshold to test signal is correct. + for value in [2.0, 5.0, 9.5, 13.0]: + set_mock_value(movable_with_tolerance.movable_logic.readback, value) + assert await movable_with_tolerance.within_tolerance.get_value() is False + # Now move to a value within threshold. Check within_threshold is True and the + # status is also done as threshold reached. + set_mock_value(movable_with_tolerance.movable_logic.readback, 9.91) + assert await movable_with_tolerance.within_tolerance.get_value() is True + assert move_status.done is True async def test_movable_with_tolerance_sub_class_signal_names_are_not_renamed( From dea0e8ab43d7a3eee1f3ee72918feb5b6458f634 Mon Sep 17 00:00:00 2001 From: Oli Wenman Date: Tue, 16 Jun 2026 15:13:35 +0000 Subject: [PATCH 11/27] Optimise import space --- .../beamlines/i10_1/high_field_magnet/high_field_magnet.py | 7 +------ tests/devices/beamlines/i09_2/test_i09_2_motors.py | 5 +---- 2 files changed, 2 insertions(+), 10 deletions(-) diff --git a/src/dodal/devices/beamlines/i10_1/high_field_magnet/high_field_magnet.py b/src/dodal/devices/beamlines/i10_1/high_field_magnet/high_field_magnet.py index b7df0e67d83..2720323532d 100644 --- a/src/dodal/devices/beamlines/i10_1/high_field_magnet/high_field_magnet.py +++ b/src/dodal/devices/beamlines/i10_1/high_field_magnet/high_field_magnet.py @@ -1,12 +1,7 @@ -from __future__ import annotations - from dataclasses import dataclass from functools import cached_property -from bluesky.protocols import ( - Flyable, - Preparable, -) +from bluesky.protocols import Flyable, Preparable from ophyd_async.core import ( DEFAULT_TIMEOUT, AsyncStatus, diff --git a/tests/devices/beamlines/i09_2/test_i09_2_motors.py b/tests/devices/beamlines/i09_2/test_i09_2_motors.py index eee0f063521..e90e566f812 100644 --- a/tests/devices/beamlines/i09_2/test_i09_2_motors.py +++ b/tests/devices/beamlines/i09_2/test_i09_2_motors.py @@ -4,10 +4,7 @@ from ophyd_async.core import init_devices from ophyd_async.testing import assert_reading, partial_reading -from dodal.devices.beamlines.i09_2 import ( - I092SampleManipulator, - PiezoElectricMotor, -) +from dodal.devices.beamlines.i09_2 import I092SampleManipulator, PiezoElectricMotor @pytest.fixture From 238529d7311b4bf1cba0be7c19440b1fde9c3199 Mon Sep 17 00:00:00 2001 From: Oli Wenman Date: Tue, 16 Jun 2026 16:21:21 +0000 Subject: [PATCH 12/27] Fix test for all python versions --- tests/devices/test_movable.py | 34 ++++++++++++++++++++++++++-------- 1 file changed, 26 insertions(+), 8 deletions(-) diff --git a/tests/devices/test_movable.py b/tests/devices/test_movable.py index ade22e1e907..c126598c112 100644 --- a/tests/devices/test_movable.py +++ b/tests/devices/test_movable.py @@ -1,3 +1,5 @@ +import asyncio + import pytest from ophyd_async.core import ( AsyncStatus, @@ -5,6 +7,7 @@ set_mock_value, soft_signal_r_and_setter, soft_signal_rw, + wait_for_value, ) from dodal.devices.movable import MovableWithTolerance @@ -26,8 +29,8 @@ def __init__(self, name: str = ""): @pytest.fixture async def movable_with_tolerance() -> MovableWithToleranceImpl: movable_with_tolerance = MovableWithToleranceImpl("movable_with_tolerance") - # Setup mock to not use default as do not want the setpoint and readback to be the - # same for tests so we can correctly test threshold signal. + # Setup mock to not use default as do not want the setpoint and readback to be in + # sync for tests so we can correctly test threshold signal. await movable_with_tolerance.connect(mock=DeviceMock()) return movable_with_tolerance @@ -65,20 +68,35 @@ async def test_movable_with_tolerance_logic_moves_to_setpoint_and_is_done_when_w movable_with_tolerance: MovableWithToleranceImpl, ) -> None: set_mock_value(movable_with_tolerance.custom_tolerance, 0.1) - set_mock_value(movable_with_tolerance.movable_logic.readback, 0.0) + setpoint = 10 + + # Needed for python 3.11, tolerance can't be already True + set_mock_value(movable_with_tolerance.movable_logic.readback, -10) + assert await movable_with_tolerance.within_tolerance.get_value() is False + async with AsyncStatus( - movable_with_tolerance.movable_logic.move(new_position=10, timeout=1) + movable_with_tolerance.movable_logic.move(new_position=setpoint, timeout=1) ) as move_status: - # Start the move by setting the setpoint. + # This prevents a race where the test proceeds before the move coroutine has + # had a chance to execute its first steps. + await wait_for_value( + movable_with_tolerance.movable_logic.setpoint, setpoint, timeout=1 + ) # Set some values between initial readback and final setpoint that are outside - # the threshold to test signal is correct. + # the threshold to test signal is correct and status hasn't completed. for value in [2.0, 5.0, 9.5, 13.0]: set_mock_value(movable_with_tolerance.movable_logic.readback, value) assert await movable_with_tolerance.within_tolerance.get_value() is False - # Now move to a value within threshold. Check within_threshold is True and the - # status is also done as threshold reached. + assert move_status.done is False + + # Now move to a value within threshold. set_mock_value(movable_with_tolerance.movable_logic.readback, 9.91) assert await movable_with_tolerance.within_tolerance.get_value() is True + + # Wait for the underlying move task to complete so no race condition. + # This ensures the status has fully processed the tolerance condition + # and transitioned to a finished state before asserting `done`. + await asyncio.wait_for(move_status.task, timeout=1) assert move_status.done is True From 0033645039a952e982fef21ee72a52a69943dc1a Mon Sep 17 00:00:00 2001 From: Oli Wenman Date: Wed, 17 Jun 2026 09:43:05 +0000 Subject: [PATCH 13/27] Updated to not use set_and_wait_for_value as timing on complete is different between python versions --- src/dodal/devices/movable.py | 20 ++++++++++++-------- tests/devices/test_movable.py | 22 ++++++++++++++++++---- 2 files changed, 30 insertions(+), 12 deletions(-) diff --git a/src/dodal/devices/movable.py b/src/dodal/devices/movable.py index 15f6842bce9..c85923cb7e6 100644 --- a/src/dodal/devices/movable.py +++ b/src/dodal/devices/movable.py @@ -9,7 +9,7 @@ StandardMovable, StandardReadable, derived_signal_r, - set_and_wait_for_other_value, + wait_for_value, ) @@ -18,13 +18,17 @@ class MovableWithToleranceLogic(MovableLogic[float]): within_tolerance: SignalR[bool] async def move(self, new_position: float, timeout: float | None) -> None: - await set_and_wait_for_other_value( - set_signal=self.setpoint, - set_value=new_position, - match_signal=self.within_tolerance, - match_value=True, - timeout=timeout, - ) + await self.setpoint.set(new_position) + # Don't use set_and_wait_for_other_value until we drop support for Python 3.11. + # In Python 3.11 there appears to be a timing issue where, if within_tolerance + # is already True (for example, because the current readback is close to the + # current setpoint), the wait condition can be satisfied before the new setpoint + # is applied. This can cause the move task to complete immediately instead of + # waiting for the new target position to be reached. + # + # For now, set the setpoint first and then wait for within_tolerance to become + # True for the newly requested position. + await wait_for_value(signal=self.within_tolerance, match=True, timeout=timeout) def _within_tolerance_read(setpoint: float, readback: float, tolerance: float) -> bool: diff --git a/tests/devices/test_movable.py b/tests/devices/test_movable.py index c126598c112..61dca2a9a64 100644 --- a/tests/devices/test_movable.py +++ b/tests/devices/test_movable.py @@ -64,15 +64,26 @@ async def test_movable_with_tolerance_within_threshold( ) +@pytest.mark.parametrize( + "initial_readback, initial_setpoint, initial_within_tolerance", + ((0, 0, True), (0, -10, False)), + ids=("initial_within_tolerance[True]", "initial_within_tolerance[False]"), +) async def test_movable_with_tolerance_logic_moves_to_setpoint_and_is_done_when_wiithin_tolerance( + initial_readback: float, + initial_setpoint: float, + initial_within_tolerance: bool, movable_with_tolerance: MovableWithToleranceImpl, ) -> None: set_mock_value(movable_with_tolerance.custom_tolerance, 0.1) - setpoint = 10 - # Needed for python 3.11, tolerance can't be already True - set_mock_value(movable_with_tolerance.movable_logic.readback, -10) - assert await movable_with_tolerance.within_tolerance.get_value() is False + set_mock_value(movable_with_tolerance.movable_logic.setpoint, initial_setpoint) + set_mock_value(movable_with_tolerance.movable_logic.readback, initial_readback) + assert ( + await movable_with_tolerance.movable_logic.within_tolerance.get_value() + is initial_within_tolerance + ) + setpoint = 10 async with AsyncStatus( movable_with_tolerance.movable_logic.move(new_position=setpoint, timeout=1) @@ -82,6 +93,9 @@ async def test_movable_with_tolerance_logic_moves_to_setpoint_and_is_done_when_w await wait_for_value( movable_with_tolerance.movable_logic.setpoint, setpoint, timeout=1 ) + assert ( + await movable_with_tolerance.movable_logic.setpoint.get_value() == setpoint + ) # Set some values between initial readback and final setpoint that are outside # the threshold to test signal is correct and status hasn't completed. for value in [2.0, 5.0, 9.5, 13.0]: From 26c8056358969ab291458b1577c7f2fe68ea031a Mon Sep 17 00:00:00 2001 From: Oli Wenman Date: Wed, 17 Jun 2026 11:19:57 +0000 Subject: [PATCH 14/27] Correct test name --- tests/devices/test_movable.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/devices/test_movable.py b/tests/devices/test_movable.py index 61dca2a9a64..04ca5daf454 100644 --- a/tests/devices/test_movable.py +++ b/tests/devices/test_movable.py @@ -69,7 +69,7 @@ async def test_movable_with_tolerance_within_threshold( ((0, 0, True), (0, -10, False)), ids=("initial_within_tolerance[True]", "initial_within_tolerance[False]"), ) -async def test_movable_with_tolerance_logic_moves_to_setpoint_and_is_done_when_wiithin_tolerance( +async def test_movable_with_tolerance_logic_moves_to_setpoint_and_is_done_when_within_tolerance( initial_readback: float, initial_setpoint: float, initial_within_tolerance: bool, From 87c063b6a840505880ce266e88c00a6cb274e582 Mon Sep 17 00:00:00 2001 From: Oli Wenman Date: Wed, 17 Jun 2026 11:20:49 +0000 Subject: [PATCH 15/27] Demostrate failing tests --- src/dodal/devices/movable.py | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/dodal/devices/movable.py b/src/dodal/devices/movable.py index c85923cb7e6..45a5686c2c6 100644 --- a/src/dodal/devices/movable.py +++ b/src/dodal/devices/movable.py @@ -9,7 +9,7 @@ StandardMovable, StandardReadable, derived_signal_r, - wait_for_value, + set_and_wait_for_other_value, ) @@ -28,7 +28,12 @@ async def move(self, new_position: float, timeout: float | None) -> None: # # For now, set the setpoint first and then wait for within_tolerance to become # True for the newly requested position. - await wait_for_value(signal=self.within_tolerance, match=True, timeout=timeout) + await set_and_wait_for_other_value( + set_signal=self.setpoint, + set_value=new_position, + match_signal=self.within_tolerance, + match_value=True, + ) def _within_tolerance_read(setpoint: float, readback: float, tolerance: float) -> bool: From 5e405a6b403b4b70407d3f46b016bd1000d24242 Mon Sep 17 00:00:00 2001 From: Oli Wenman Date: Wed, 17 Jun 2026 11:27:30 +0000 Subject: [PATCH 16/27] Second attempt --- src/dodal/devices/movable.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/dodal/devices/movable.py b/src/dodal/devices/movable.py index 45a5686c2c6..4b7616ae780 100644 --- a/src/dodal/devices/movable.py +++ b/src/dodal/devices/movable.py @@ -18,7 +18,7 @@ class MovableWithToleranceLogic(MovableLogic[float]): within_tolerance: SignalR[bool] async def move(self, new_position: float, timeout: float | None) -> None: - await self.setpoint.set(new_position) + # await self.setpoint.set(new_position) # Don't use set_and_wait_for_other_value until we drop support for Python 3.11. # In Python 3.11 there appears to be a timing issue where, if within_tolerance # is already True (for example, because the current readback is close to the @@ -28,11 +28,13 @@ async def move(self, new_position: float, timeout: float | None) -> None: # # For now, set the setpoint first and then wait for within_tolerance to become # True for the newly requested position. + # wait_for_value(self.within_tolerance, True, timeout) await set_and_wait_for_other_value( set_signal=self.setpoint, set_value=new_position, match_signal=self.within_tolerance, match_value=True, + timeout=timeout, ) From ab853b8dfd8a78b1417eef7eeb8d1e4682c4263f Mon Sep 17 00:00:00 2001 From: Oli Wenman Date: Wed, 17 Jun 2026 13:06:28 +0000 Subject: [PATCH 17/27] Revert demo of breaking tests to fix tests again --- src/dodal/devices/movable.py | 13 +++---------- 1 file changed, 3 insertions(+), 10 deletions(-) diff --git a/src/dodal/devices/movable.py b/src/dodal/devices/movable.py index 4b7616ae780..82f9c1a65ed 100644 --- a/src/dodal/devices/movable.py +++ b/src/dodal/devices/movable.py @@ -9,7 +9,7 @@ StandardMovable, StandardReadable, derived_signal_r, - set_and_wait_for_other_value, + wait_for_value, ) @@ -18,7 +18,6 @@ class MovableWithToleranceLogic(MovableLogic[float]): within_tolerance: SignalR[bool] async def move(self, new_position: float, timeout: float | None) -> None: - # await self.setpoint.set(new_position) # Don't use set_and_wait_for_other_value until we drop support for Python 3.11. # In Python 3.11 there appears to be a timing issue where, if within_tolerance # is already True (for example, because the current readback is close to the @@ -28,14 +27,8 @@ async def move(self, new_position: float, timeout: float | None) -> None: # # For now, set the setpoint first and then wait for within_tolerance to become # True for the newly requested position. - # wait_for_value(self.within_tolerance, True, timeout) - await set_and_wait_for_other_value( - set_signal=self.setpoint, - set_value=new_position, - match_signal=self.within_tolerance, - match_value=True, - timeout=timeout, - ) + await self.setpoint.set(new_position) + await wait_for_value(self.within_tolerance, True, timeout) def _within_tolerance_read(setpoint: float, readback: float, tolerance: float) -> bool: From 75dbc8fe82031ee63ad2213af325bd4004cf61ac Mon Sep 17 00:00:00 2001 From: Oli Wenman Date: Wed, 17 Jun 2026 13:26:34 +0000 Subject: [PATCH 18/27] Return None for calcaulte_timeout --- src/dodal/devices/beamlines/i09_2/i09_2_motors.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/dodal/devices/beamlines/i09_2/i09_2_motors.py b/src/dodal/devices/beamlines/i09_2/i09_2_motors.py index a73e4307a4b..453cfe55c62 100644 --- a/src/dodal/devices/beamlines/i09_2/i09_2_motors.py +++ b/src/dodal/devices/beamlines/i09_2/i09_2_motors.py @@ -20,12 +20,15 @@ class PiezoElectricMovableLogic(MovableWithToleranceLogic): async def stop(self) -> None: await self.motor_stop.set(1) - # How do provide calculate_timeout without velocity and acceleration? + def calculate_timeout(self, old_position: float, new_position: float) -> float: # type: ignore + # Bug: should allow return None. + # See https://github.com/bluesky/ophyd-async/pull/1300 + return None # type: ignore class PiezoElectricMotor(MovableWithTolerance): """Motor like device with user_readback, user_setpoint, and a stop signals. Has a - configurable tolerance soft signal to configure the tolerance of when a motor is done + configurable tolerance soft signal to configure the tolerance of when a devuce is done moving. For example, if tolerance is configured to be 0.5, and the setpoint is 10 and the readback is 9.8, the motor will be done moving and stop blocking for the AsyncStatus. From 346f0261dbdbbf69f8ebe40cfb29791a85c107f4 Mon Sep 17 00:00:00 2001 From: Oli Wenman Date: Wed, 17 Jun 2026 13:53:21 +0000 Subject: [PATCH 19/27] Add missing async --- src/dodal/devices/beamlines/i09_2/i09_2_motors.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/dodal/devices/beamlines/i09_2/i09_2_motors.py b/src/dodal/devices/beamlines/i09_2/i09_2_motors.py index 453cfe55c62..d30db51f789 100644 --- a/src/dodal/devices/beamlines/i09_2/i09_2_motors.py +++ b/src/dodal/devices/beamlines/i09_2/i09_2_motors.py @@ -20,7 +20,9 @@ class PiezoElectricMovableLogic(MovableWithToleranceLogic): async def stop(self) -> None: await self.motor_stop.set(1) - def calculate_timeout(self, old_position: float, new_position: float) -> float: # type: ignore + async def calculate_timeout( + self, old_position: float, new_position: float + ) -> float: # Bug: should allow return None. # See https://github.com/bluesky/ophyd-async/pull/1300 return None # type: ignore From bc265ae98b761386acc4bed6c8c9d661a22a1d1b Mon Sep 17 00:00:00 2001 From: Oli Wenman Date: Fri, 19 Jun 2026 13:37:54 +0000 Subject: [PATCH 20/27] Update doc --- src/dodal/devices/movable.py | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-) diff --git a/src/dodal/devices/movable.py b/src/dodal/devices/movable.py index 82f9c1a65ed..96c9c7e83b0 100644 --- a/src/dodal/devices/movable.py +++ b/src/dodal/devices/movable.py @@ -18,17 +18,12 @@ class MovableWithToleranceLogic(MovableLogic[float]): within_tolerance: SignalR[bool] async def move(self, new_position: float, timeout: float | None) -> None: - # Don't use set_and_wait_for_other_value until we drop support for Python 3.11. - # In Python 3.11 there appears to be a timing issue where, if within_tolerance - # is already True (for example, because the current readback is close to the - # current setpoint), the wait condition can be satisfied before the new setpoint - # is applied. This can cause the move task to complete immediately instead of - # waiting for the new target position to be reached. - # - # For now, set the setpoint first and then wait for within_tolerance to become - # True for the newly requested position. - await self.setpoint.set(new_position) - await wait_for_value(self.within_tolerance, True, timeout) + await self.setpoint.set(new_position, timeout=timeout) + await wait_for_value(self.setpoint, new_position, timeout=timeout) + # Once setpoint is at new position, we can check for tolerance signal to see if + # true now as the within_tolerance window has updated to the new setpoint + # position. Now it doesn't matter if motor steps are small or large. + await wait_for_value(self.within_tolerance, True, timeout=timeout) def _within_tolerance_read(setpoint: float, readback: float, tolerance: float) -> bool: From bbb9ab2c97291fbca94e41ee4de82b0e7657827a Mon Sep 17 00:00:00 2001 From: Oli Wenman Date: Thu, 25 Jun 2026 08:14:00 +0000 Subject: [PATCH 21/27] Update doc string --- .../devices/beamlines/i09_2/i09_2_motors.py | 17 +++++------------ 1 file changed, 5 insertions(+), 12 deletions(-) diff --git a/src/dodal/devices/beamlines/i09_2/i09_2_motors.py b/src/dodal/devices/beamlines/i09_2/i09_2_motors.py index d30db51f789..10c2cdf1fff 100644 --- a/src/dodal/devices/beamlines/i09_2/i09_2_motors.py +++ b/src/dodal/devices/beamlines/i09_2/i09_2_motors.py @@ -20,20 +20,13 @@ class PiezoElectricMovableLogic(MovableWithToleranceLogic): async def stop(self) -> None: await self.motor_stop.set(1) - async def calculate_timeout( - self, old_position: float, new_position: float - ) -> float: - # Bug: should allow return None. - # See https://github.com/bluesky/ophyd-async/pull/1300 - return None # type: ignore - class PiezoElectricMotor(MovableWithTolerance): - """Motor like device with user_readback, user_setpoint, and a stop signals. Has a - configurable tolerance soft signal to configure the tolerance of when a devuce is done - moving. For example, if tolerance is configured to be 0.5, and the setpoint is 10 and - the readback is 9.8, the motor will be done moving and stop blocking for the - AsyncStatus. + """A piezoelectric positioning stage with configurable move tolerance. + + This device exposes EPICS signals for readback, setpoint, and motion stop commands. + Motion completion is determined by comparing the readback and setpoint positions + using a configurable tolerance. """ def __init__(self, prefix: str, tolerance: float = 0.01, name: str = ""): From b6e946c93ce64c92c09d3b547416e1615e518a02 Mon Sep 17 00:00:00 2001 From: Oli Wenman Date: Mon, 6 Jul 2026 09:18:46 +0000 Subject: [PATCH 22/27] Implement feedback --- .../devices/beamlines/i09_2/i09_2_motors.py | 7 +--- .../high_field_magnet/high_field_magnet.py | 9 ++--- src/dodal/devices/movable.py | 22 +++++------- tests/devices/test_movable.py | 36 +++++-------------- 4 files changed, 20 insertions(+), 54 deletions(-) diff --git a/src/dodal/devices/beamlines/i09_2/i09_2_motors.py b/src/dodal/devices/beamlines/i09_2/i09_2_motors.py index 10c2cdf1fff..0259e98db76 100644 --- a/src/dodal/devices/beamlines/i09_2/i09_2_motors.py +++ b/src/dodal/devices/beamlines/i09_2/i09_2_motors.py @@ -36,12 +36,7 @@ def __init__(self, prefix: str, tolerance: float = 0.01, name: str = ""): self.user_setpoint = epics_signal_rw(float, prefix + ":MOV:RD") self.tolerance = soft_signal_rw(float, initial_value=tolerance) self.motor_stop = epics_signal_w(int, prefix + ":HLT:WR.PROC") - super().__init__( - tolerance=self.tolerance, - setpoint=self.user_setpoint, - readback=self.user_readback, - name=name, - ) + super().__init__(name=name) @cached_property def movable_logic(self) -> PiezoElectricMovableLogic: diff --git a/src/dodal/devices/beamlines/i10_1/high_field_magnet/high_field_magnet.py b/src/dodal/devices/beamlines/i10_1/high_field_magnet/high_field_magnet.py index 2720323532d..7c59b2efc2e 100644 --- a/src/dodal/devices/beamlines/i10_1/high_field_magnet/high_field_magnet.py +++ b/src/dodal/devices/beamlines/i10_1/high_field_magnet/high_field_magnet.py @@ -91,7 +91,7 @@ def __init__( read_pv=prefix + "STS:ACTIVITY", ) self.ramp_up_time = soft_signal_rw(datatype=float, initial_value=1.0) - self.field_tolerance = soft_signal_rw(float, initial_value=field_tolerance) + self.tolerance = soft_signal_rw(float, initial_value=field_tolerance) with self.add_children_as_readables(StandardReadableFormat.HINTED_SIGNAL): self.user_readback = epics_signal_r(float, prefix + "RBV:DEMANDFIELD") @@ -109,12 +109,7 @@ def __init__( self._fly_info: FlyMagInfo | None = None self._fly_status: WatchableAsyncStatus | None = None - super().__init__( - tolerance=self.field_tolerance, - setpoint=self.user_setpoint, - readback=self.user_readback, - name=name, - ) + super().__init__(name=name) @cached_property def movable_logic(self) -> HighFieldMagnetMovableLogic: diff --git a/src/dodal/devices/movable.py b/src/dodal/devices/movable.py index f917b7701d0..53e9276c805 100644 --- a/src/dodal/devices/movable.py +++ b/src/dodal/devices/movable.py @@ -3,7 +3,6 @@ from ophyd_async.core import ( MovableLogic, - Reference, SignalR, SignalRW, StandardMovable, @@ -36,29 +35,26 @@ class MovableWithTolerance(StandardMovable[float], StandardReadable): moving if it the readback and setpoint difference is within the tolerance. """ + tolerance: SignalR[float] + user_setpoint: SignalRW[float] + user_readback: SignalR[float] + def __init__( self, - tolerance: SignalR[float], - setpoint: SignalRW[float], - readback: SignalR[float], name: str = "", ): - # Use reference so sub classes still have flexibility to name signals to what - # they want. - self._setpoint_ref = Reference(setpoint) - self._readback_ref = Reference(readback) self.within_tolerance = derived_signal_r( _within_tolerance_read, - tolerance=tolerance, - setpoint=setpoint, - readback=readback, + tolerance=self.tolerance, + setpoint=self.user_setpoint, + readback=self.user_readback, ) super().__init__(name) @cached_property def movable_logic(self) -> MovableWithToleranceLogic: return MovableWithToleranceLogic( - readback=self._readback_ref(), - setpoint=self._setpoint_ref(), + readback=self.user_readback, + setpoint=self.user_setpoint, within_tolerance=self.within_tolerance, ) diff --git a/tests/devices/test_movable.py b/tests/devices/test_movable.py index c7c7071721a..e0ac18f0718 100644 --- a/tests/devices/test_movable.py +++ b/tests/devices/test_movable.py @@ -16,15 +16,10 @@ class MovableWithToleranceImpl(MovableWithTolerance): def __init__(self, name: str = ""): - self.custom_tolerance = soft_signal_rw(float) - self.custom_setpoint = soft_signal_rw(float) - self.custom_readback, _ = soft_signal_r_and_setter(float) - super().__init__( - tolerance=self.custom_tolerance, - setpoint=self.custom_setpoint, - readback=self.custom_readback, - name=name, - ) + self.tolerance = soft_signal_rw(float) + self.user_setpoint = soft_signal_rw(float) + self.user_readback, _ = soft_signal_r_and_setter(float) + super().__init__(name=name) @pytest.fixture @@ -56,9 +51,9 @@ async def test_movable_with_tolerance_within_threshold( readback: float, expected_within_threshold: bool, ) -> None: - set_mock_value(movable_with_tolerance.custom_tolerance, tolerance) - set_mock_value(movable_with_tolerance.custom_setpoint, setpoint) - set_mock_value(movable_with_tolerance.custom_readback, readback) + set_mock_value(movable_with_tolerance.tolerance, tolerance) + set_mock_value(movable_with_tolerance.user_setpoint, setpoint) + set_mock_value(movable_with_tolerance.user_readback, readback) assert ( await movable_with_tolerance.within_tolerance.get_value() == expected_within_threshold @@ -76,7 +71,7 @@ async def test_movable_with_tolerance_logic_moves_to_setpoint_and_is_done_when_w initial_within_tolerance: bool, movable_with_tolerance: MovableWithToleranceImpl, ) -> None: - set_mock_value(movable_with_tolerance.custom_tolerance, 0.1) + set_mock_value(movable_with_tolerance.tolerance, 0.1) set_mock_value(movable_with_tolerance.movable_logic.setpoint, initial_setpoint) set_mock_value(movable_with_tolerance.movable_logic.readback, initial_readback) @@ -115,18 +110,3 @@ async def test_movable_with_tolerance_logic_moves_to_setpoint_and_is_done_when_w # and transitioned to a finished state before asserting `done`. await asyncio.wait_for(move_status.task, timeout=1) assert move_status.done is True - - -async def test_movable_with_tolerance_sub_class_signal_names_are_not_renamed( - movable_with_tolerance: MovableWithToleranceImpl, -) -> None: - assert ( - movable_with_tolerance.custom_setpoint.name - == "movable_with_tolerance-custom_setpoint" - ) - assert ( - movable_with_tolerance.custom_tolerance.name - == "movable_with_tolerance-custom_tolerance" - ) - # Readback is the exception as renamed by StandardMovable to be device name - assert movable_with_tolerance.custom_readback.name == "movable_with_tolerance" From b81ce78990bb43ed670f7fc1ee7761a863f13a58 Mon Sep 17 00:00:00 2001 From: Oli Wenman Date: Mon, 6 Jul 2026 14:39:35 +0000 Subject: [PATCH 23/27] Implement feedback --- src/dodal/devices/movable.py | 4 ++-- tests/devices/test_movable.py | 3 --- 2 files changed, 2 insertions(+), 5 deletions(-) diff --git a/src/dodal/devices/movable.py b/src/dodal/devices/movable.py index 53e9276c805..2897cf62a43 100644 --- a/src/dodal/devices/movable.py +++ b/src/dodal/devices/movable.py @@ -26,7 +26,7 @@ async def move(self, new_position: float, timeout: TimeoutCalculator) -> None: await wait_for_value(self.within_tolerance, True, timeout=timeout()) -def _within_tolerance_read(setpoint: float, readback: float, tolerance: float) -> bool: +def _is_within_tolerance(setpoint: float, readback: float, tolerance: float) -> bool: return abs(setpoint - readback) < abs(tolerance) @@ -44,7 +44,7 @@ def __init__( name: str = "", ): self.within_tolerance = derived_signal_r( - _within_tolerance_read, + _is_within_tolerance, tolerance=self.tolerance, setpoint=self.user_setpoint, readback=self.user_readback, diff --git a/tests/devices/test_movable.py b/tests/devices/test_movable.py index e0ac18f0718..6a5708a5e99 100644 --- a/tests/devices/test_movable.py +++ b/tests/devices/test_movable.py @@ -91,9 +91,6 @@ async def test_movable_with_tolerance_logic_moves_to_setpoint_and_is_done_when_w await wait_for_value( movable_with_tolerance.movable_logic.setpoint, setpoint, timeout=1 ) - assert ( - await movable_with_tolerance.movable_logic.setpoint.get_value() == setpoint - ) # Set some values between initial readback and final setpoint that are outside # the threshold to test signal is correct and status hasn't completed. for value in [2.0, 5.0, 9.5, 13.0]: From 12064c7dbb05d89b1b79d2d490d4751727b11f14 Mon Sep 17 00:00:00 2001 From: Oli Wenman Date: Wed, 15 Jul 2026 14:13:44 +0000 Subject: [PATCH 24/27] Simplify, remove within_tolerance signal and movable. Only logic now --- .../devices/beamlines/i09_2/i09_2_motors.py | 7 ++- .../high_field_magnet/high_field_magnet.py | 8 ++- src/dodal/devices/movable.py | 57 +++++-------------- tests/devices/test_movable.py | 48 +++++++++++----- 4 files changed, 55 insertions(+), 65 deletions(-) diff --git a/src/dodal/devices/beamlines/i09_2/i09_2_motors.py b/src/dodal/devices/beamlines/i09_2/i09_2_motors.py index 0259e98db76..ff6a277cbc2 100644 --- a/src/dodal/devices/beamlines/i09_2/i09_2_motors.py +++ b/src/dodal/devices/beamlines/i09_2/i09_2_motors.py @@ -3,6 +3,7 @@ from ophyd_async.core import ( SignalW, + StandardMovable, StandardReadable, StandardReadableFormat, soft_signal_rw, @@ -10,7 +11,7 @@ from ophyd_async.epics.core import epics_signal_r, epics_signal_rw, epics_signal_w from ophyd_async.epics.motor import Motor -from dodal.devices.movable import MovableWithTolerance, MovableWithToleranceLogic +from dodal.devices.movable import MovableWithToleranceLogic @dataclass @@ -21,7 +22,7 @@ async def stop(self) -> None: await self.motor_stop.set(1) -class PiezoElectricMotor(MovableWithTolerance): +class PiezoElectricMotor(StandardMovable[float], StandardReadable): """A piezoelectric positioning stage with configurable move tolerance. This device exposes EPICS signals for readback, setpoint, and motion stop commands. @@ -43,7 +44,7 @@ def movable_logic(self) -> PiezoElectricMovableLogic: return PiezoElectricMovableLogic( readback=self.user_readback, setpoint=self.user_setpoint, - within_tolerance=self.within_tolerance, + tolerance=self.tolerance, motor_stop=self.motor_stop, ) diff --git a/src/dodal/devices/beamlines/i10_1/high_field_magnet/high_field_magnet.py b/src/dodal/devices/beamlines/i10_1/high_field_magnet/high_field_magnet.py index 7c59b2efc2e..a7dbd422bbd 100644 --- a/src/dodal/devices/beamlines/i10_1/high_field_magnet/high_field_magnet.py +++ b/src/dodal/devices/beamlines/i10_1/high_field_magnet/high_field_magnet.py @@ -6,6 +6,8 @@ DEFAULT_TIMEOUT, AsyncStatus, SignalRW, + StandardMovable, + StandardReadable, StandardReadableFormat, StrictEnum, SubsetEnum, @@ -16,7 +18,7 @@ from ophyd_async.epics.core import epics_signal_r, epics_signal_rw, epics_signal_w from pydantic import BaseModel, Field -from dodal.devices.movable import MovableWithTolerance, MovableWithToleranceLogic +from dodal.devices.movable import MovableWithToleranceLogic class HighFieldMangetSweepTypes(StrictEnum): @@ -71,7 +73,7 @@ async def calculate_timeout( return timeout -class HighFieldMagnet(MovableWithTolerance, Flyable, Preparable): +class HighFieldMagnet(StandardMovable[float], StandardReadable, Flyable, Preparable): def __init__( self, prefix: str, field_tolerance: float = 0.01, name: str = "" ) -> None: @@ -116,9 +118,9 @@ def movable_logic(self) -> HighFieldMagnetMovableLogic: return HighFieldMagnetMovableLogic( setpoint=self.user_setpoint, readback=self.user_readback, - within_tolerance=self.within_tolerance, speed=self.sweep_rate, acc_time=self.ramp_up_time, + tolerance=self.tolerance, ) @AsyncStatus.wrap diff --git a/src/dodal/devices/movable.py b/src/dodal/devices/movable.py index 2897cf62a43..b32cfda510e 100644 --- a/src/dodal/devices/movable.py +++ b/src/dodal/devices/movable.py @@ -1,60 +1,29 @@ from dataclasses import dataclass -from functools import cached_property from ophyd_async.core import ( MovableLogic, SignalR, - SignalRW, - StandardMovable, - StandardReadable, TimeoutCalculator, - derived_signal_r, - wait_for_value, + set_and_wait_for_other_value, ) @dataclass class MovableWithToleranceLogic(MovableLogic[float]): - within_tolerance: SignalR[bool] + tolerance: SignalR[float] async def move(self, new_position: float, timeout: TimeoutCalculator) -> None: - await self.setpoint.set(new_position, timeout=timeout()) - await wait_for_value(self.setpoint, new_position, timeout=timeout()) - # Once setpoint is at new position, we can check for tolerance signal to see if - # true now as the within_tolerance window has updated to the new setpoint - # position. Now it doesn't matter if motor steps are small or large. - await wait_for_value(self.within_tolerance, True, timeout=timeout()) + tolerance = await self.tolerance.get_value() + await set_and_wait_for_other_value( + self.setpoint, + new_position, + self.readback, + lambda current_position: is_within_tolerance( + new_position, current_position, tolerance + ), + timeout=timeout(), + ) -def _is_within_tolerance(setpoint: float, readback: float, tolerance: float) -> bool: +def is_within_tolerance(setpoint: float, readback: float, tolerance: float) -> bool: return abs(setpoint - readback) < abs(tolerance) - - -class MovableWithTolerance(StandardMovable[float], StandardReadable): - """Movable with a signal to configure the tolerance of when the device is done - moving if it the readback and setpoint difference is within the tolerance. - """ - - tolerance: SignalR[float] - user_setpoint: SignalRW[float] - user_readback: SignalR[float] - - def __init__( - self, - name: str = "", - ): - self.within_tolerance = derived_signal_r( - _is_within_tolerance, - tolerance=self.tolerance, - setpoint=self.user_setpoint, - readback=self.user_readback, - ) - super().__init__(name) - - @cached_property - def movable_logic(self) -> MovableWithToleranceLogic: - return MovableWithToleranceLogic( - readback=self.user_readback, - setpoint=self.user_setpoint, - within_tolerance=self.within_tolerance, - ) diff --git a/tests/devices/test_movable.py b/tests/devices/test_movable.py index 6a5708a5e99..de427825d4e 100644 --- a/tests/devices/test_movable.py +++ b/tests/devices/test_movable.py @@ -1,9 +1,11 @@ import asyncio +from functools import cached_property import pytest from ophyd_async.core import ( AsyncStatus, DeviceMock, + StandardMovable, set_mock_value, soft_signal_r_and_setter, soft_signal_rw, @@ -11,16 +13,24 @@ ) from ophyd_async.core._movable import MoveTimeout -from dodal.devices.movable import MovableWithTolerance +from dodal.devices.movable import MovableWithToleranceLogic, is_within_tolerance -class MovableWithToleranceImpl(MovableWithTolerance): +class MovableWithToleranceImpl(StandardMovable[float]): def __init__(self, name: str = ""): self.tolerance = soft_signal_rw(float) self.user_setpoint = soft_signal_rw(float) self.user_readback, _ = soft_signal_r_and_setter(float) super().__init__(name=name) + @cached_property + def movable_logic(self) -> MovableWithToleranceLogic: + return MovableWithToleranceLogic( + readback=self.user_readback, + setpoint=self.user_setpoint, + tolerance=self.tolerance, + ) + @pytest.fixture async def movable_with_tolerance() -> MovableWithToleranceImpl: @@ -44,18 +54,14 @@ async def movable_with_tolerance() -> MovableWithToleranceImpl: (10.0, 10.1, -0.01, False), ], ) -async def test_movable_with_tolerance_within_threshold( - movable_with_tolerance: MovableWithToleranceImpl, +async def test_is_within_tolerance( tolerance: float, setpoint: float, readback: float, expected_within_threshold: bool, ) -> None: - set_mock_value(movable_with_tolerance.tolerance, tolerance) - set_mock_value(movable_with_tolerance.user_setpoint, setpoint) - set_mock_value(movable_with_tolerance.user_readback, readback) assert ( - await movable_with_tolerance.within_tolerance.get_value() + is_within_tolerance(setpoint=setpoint, readback=readback, tolerance=tolerance) == expected_within_threshold ) @@ -71,13 +77,14 @@ async def test_movable_with_tolerance_logic_moves_to_setpoint_and_is_done_when_w initial_within_tolerance: bool, movable_with_tolerance: MovableWithToleranceImpl, ) -> None: - set_mock_value(movable_with_tolerance.tolerance, 0.1) + tolerance = 0.1 + set_mock_value(movable_with_tolerance.tolerance, tolerance) set_mock_value(movable_with_tolerance.movable_logic.setpoint, initial_setpoint) set_mock_value(movable_with_tolerance.movable_logic.readback, initial_readback) assert ( - await movable_with_tolerance.movable_logic.within_tolerance.get_value() - is initial_within_tolerance + is_within_tolerance(initial_setpoint, initial_readback, tolerance) + == initial_within_tolerance ) setpoint = 10 @@ -93,14 +100,25 @@ async def test_movable_with_tolerance_logic_moves_to_setpoint_and_is_done_when_w ) # Set some values between initial readback and final setpoint that are outside # the threshold to test signal is correct and status hasn't completed. - for value in [2.0, 5.0, 9.5, 13.0]: + for value in [2.0, 5.0, 9.5, 13.0, setpoint - tolerance * 1.01]: set_mock_value(movable_with_tolerance.movable_logic.readback, value) - assert await movable_with_tolerance.within_tolerance.get_value() is False + current_readback = await movable_with_tolerance.user_readback.get_value() + assert ( + is_within_tolerance( + setpoint=setpoint, readback=current_readback, tolerance=tolerance + ) + is False + ) assert move_status.done is False # Now move to a value within threshold. - set_mock_value(movable_with_tolerance.movable_logic.readback, 9.91) - assert await movable_with_tolerance.within_tolerance.get_value() is True + set_mock_value( + movable_with_tolerance.movable_logic.readback, setpoint - tolerance + ) + current_readback = await movable_with_tolerance.user_readback.get_value() + assert is_within_tolerance( + setpoint=setpoint, readback=current_readback, tolerance=tolerance + ) # Wait for the underlying move task to complete so no race condition. # This ensures the status has fully processed the tolerance condition From 0cd305cee1d51e377d3d9b56fe318438dd41bf9e Mon Sep 17 00:00:00 2001 From: Oli Wenman Date: Wed, 15 Jul 2026 14:34:30 +0000 Subject: [PATCH 25/27] Rename back to field_tolerance --- .../beamlines/i10_1/high_field_magnet/high_field_magnet.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/dodal/devices/beamlines/i10_1/high_field_magnet/high_field_magnet.py b/src/dodal/devices/beamlines/i10_1/high_field_magnet/high_field_magnet.py index a7dbd422bbd..7cd9b78fd75 100644 --- a/src/dodal/devices/beamlines/i10_1/high_field_magnet/high_field_magnet.py +++ b/src/dodal/devices/beamlines/i10_1/high_field_magnet/high_field_magnet.py @@ -93,7 +93,7 @@ def __init__( read_pv=prefix + "STS:ACTIVITY", ) self.ramp_up_time = soft_signal_rw(datatype=float, initial_value=1.0) - self.tolerance = soft_signal_rw(float, initial_value=field_tolerance) + self.field_tolerance = soft_signal_rw(float, initial_value=field_tolerance) with self.add_children_as_readables(StandardReadableFormat.HINTED_SIGNAL): self.user_readback = epics_signal_r(float, prefix + "RBV:DEMANDFIELD") @@ -120,7 +120,7 @@ def movable_logic(self) -> HighFieldMagnetMovableLogic: readback=self.user_readback, speed=self.sweep_rate, acc_time=self.ramp_up_time, - tolerance=self.tolerance, + tolerance=self.field_tolerance, ) @AsyncStatus.wrap From 95981eeeb272cfa87e73b9059ab57ea77a736b10 Mon Sep 17 00:00:00 2001 From: Oli Wenman Date: Wed, 15 Jul 2026 14:41:08 +0000 Subject: [PATCH 26/27] Add doc strings to the logic --- src/dodal/devices/movable.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/dodal/devices/movable.py b/src/dodal/devices/movable.py index b32cfda510e..de9a5ea62e5 100644 --- a/src/dodal/devices/movable.py +++ b/src/dodal/devices/movable.py @@ -10,6 +10,8 @@ @dataclass class MovableWithToleranceLogic(MovableLogic[float]): + """Movable logic that completes once the readback is within a configured tolerance.""" + tolerance: SignalR[float] async def move(self, new_position: float, timeout: TimeoutCalculator) -> None: @@ -26,4 +28,5 @@ async def move(self, new_position: float, timeout: TimeoutCalculator) -> None: def is_within_tolerance(setpoint: float, readback: float, tolerance: float) -> bool: + """Return whether the readback is within the specified tolerance of the setpoint.""" return abs(setpoint - readback) < abs(tolerance) From bd374a7c2c72bd61fd3af0f35bd7c182caa4ad4b Mon Sep 17 00:00:00 2001 From: Oli Wenman Date: Thu, 16 Jul 2026 08:41:17 +0000 Subject: [PATCH 27/27] Apply suggestion --- tests/devices/test_movable.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/tests/devices/test_movable.py b/tests/devices/test_movable.py index de427825d4e..1d731d19063 100644 --- a/tests/devices/test_movable.py +++ b/tests/devices/test_movable.py @@ -1,4 +1,3 @@ -import asyncio from functools import cached_property import pytest @@ -123,5 +122,5 @@ async def test_movable_with_tolerance_logic_moves_to_setpoint_and_is_done_when_w # Wait for the underlying move task to complete so no race condition. # This ensures the status has fully processed the tolerance condition # and transitioned to a finished state before asserting `done`. - await asyncio.wait_for(move_status.task, timeout=1) + await move_status assert move_status.done is True