Support for mod-360 omega by rtuck99 · Pull Request #1929 · DiamondLightSource/dodal

rtuck99 · 2026-02-19T13:18:24Z

Required for

Robot unload at 360 intervals instead of reset to 0 omega spin on unload mx-bluesky#1598

Introduces a mod_360_motor parameter to the init method for XYZOmegaStage; when set to True this replaces the standard omega motor with an instance of ModMotor which delegates motion to the underlying omega motor with the difference that the omega values are remapped to 0<=omega<360.

When making an absolute move on the ModMotor:

The motor will never do a real movement more than 180 degrees
For moves to absolute positions which appear (from the ModMotor readback) to be less than 180 degrees from the current position, the real motor will always move in the same direction as implied by the apparent move.

Instructions to reviewer on how to test:

Do thing x
Confirm thing y happens

Checks for reviewer

Would the PR title make sense to a scientist on a set of release notes
If a new device has been added does it follow the standards
If changing the API for a pre-existing device, ensure that any beamlines using this device have updated their Bluesky plans accordingly
Have the connection tests for the relevant beamline(s) been run via dodal connect ${BEAMLINE}

codecov · 2026-02-19T13:22:59Z

Codecov Report

❌ Patch coverage is 91.93548% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 99.08%. Comparing base (7aebba2) to head (a95aa88).

Files with missing lines	Patch %	Lines
src/dodal/common/maths.py	90.00%	5 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1929      +/-   ##
==========================================
- Coverage   99.12%   99.08%   -0.04%     
==========================================
  Files         319      319              
  Lines       12201    12260      +59     
==========================================
+ Hits        12094    12148      +54     
- Misses        107      112       +5

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

oliwenmandiamond · 2026-02-19T13:54:37Z

Sorry to jump in here, but I am working on a StandardMovable in ophyd-async which looks like a perfect use case for your ModMotor and caught my eye. Allows us to create Motor like things in a standard way. bluesky/ophyd-async#1200

Secondly, I don't think you should be altering the existing XYZOmegaStage. It will make the type ModMotor | Motor which isn't a good idea when this isn't standard behaviour for beamline stages. This is MX specific behaviour, therefore you should create a specific one like ModMotorXYZOmegaStage for example.

jacob720

Looks good! Logic is clean and easy to follow

jacob720 · 2026-02-27T11:06:28Z

src/dodal/devices/motors.py

-            self.omega = Motor(prefix + omega_infix)
+            real_motor = Motor(prefix + omega_infix)
+            self.omega = real_motor
+            self.omega_axis = WrappedAxis(self.omega)


Should something similar be added to XYZThetaStage or anything else with a rotational axis?

jacob720 · 2026-02-27T11:17:15Z

tests/devices/test_motors.py

+    set_mock_value(stage.omega.user_readback, real_value)
+    offset, phase = await stage.omega_axis.offset_and_phase.get_value()
+    assert 0 <= expected_phase < 360
+    assert math.isclose(phase, expected_phase, abs_tol=1e-6)


Could: add expected offset to this test

DominicOram

Thanks, some comments

DominicOram · 2026-03-02T09:08:09Z

src/dodal/devices/smargon.py

        only come back after the motion on that axis finished.
        """
        await self.defer_move.set(DeferMoves.ON)
+        # TODO something something i03 broken smargon serialise moves workaround


Should: Can this be an issue please?

DominicOram · 2026-03-02T09:13:33Z

src/dodal/devices/smargon.py

                        await axis.user_setpoint.get_value(), new_setpoint
                    )
                    put_completion = await set_and_wait_for_value(
                        axis.user_setpoint,


Should: I don't really like the check against the name "omega" above, it would be much cleaner if we can make the omega object more like a thing that just handles the logic itself. I think if we change the phase signal to be user_setpoint in WrappedAxis then we can just remove the logic in here for getting the target value and we can set it like any other motor?

DominicOram · 2026-03-02T09:15:47Z

src/dodal/devices/motors.py

-            self.omega = Motor(prefix + omega_infix)
+            real_motor = Motor(prefix + omega_infix)
+            self.omega = real_motor
+            self.omega_axis = WrappedAxis(self.omega)


Should: I agree with @oliwenmandiamond that we shouldn't be polluting the common XYZOmegaStage with this logic. We should do that overriding when we instantiate the smargon. Maybe the cleanest way to do this is to change these collection classes to protocols?

I think having a protocol is overkill as were effectively defining the device twice, more boilerplate.

It should be done like this:

class MyCustomStage(XYZOmegaStage): def __init__( self, prefix: str, name: str = "", x_infix: str = _X, y_infix: str = _Y, z_infix: str = _Z, omega_infix: str = _OMEGA, ) -> None: super().__init__(prefix, name, x_infix, y_infix, z_infix, omega_infix) with self.add_children_as_readables(): self.omega_axis = WrappedAxis(self.omega)

This is how every beamline has defined stage. If it is common and generic, it goes in dodal/devices/motors.py.

If it is beamline specific, it should be defined as dodal/devices/beamlines/iXX/my_device.py. For example i05, https://github.com/DiamondLightSource/dodal/blob/main/src/dodal/devices/beamlines/i05/i05_motors.py and i05_1 https://github.com/DiamondLightSource/dodal/blob/main/src/dodal/devices/beamlines/i05_1/i05_1_motors.py

If this is used mx wide, it should go in dodal/devices/my_mx_specific_device.py

I think it should be a protocol 'cos I think we should be overriding omega completely, rather than creating a second wrapped axis. I think this helps downstream plans not worry so much about this logic and just use omega, making them more generic across MX

Okay, I see. I was referring to the current implementation.

Yes a protocol could work. If this could wait for [StandardMovable](https://github.com/bluesky/ophyd-async/pull/1200), you could make the protocol

class XYZOmega(Protocol): x: StandardMovable y: StandardMovable z: StandardMovable omega: StandardMovable

Then you could implement your own MovableLogic rather than use the MotorMovableLogic, then it doesn't matter what the implementation is for the plan.

DominicOram · 2026-03-02T09:48:57Z

src/dodal/devices/smargon.py

+            return value
+
    @AsyncStatus.wrap
    async def set(self, value: CombinedMove):


Should: I'm not sure I like that the co-ordinate system of this set is different to that of omega.set. I think it is possible to get the underlying motor object to always be moving based on phase.

oliwenmandiamond · 2026-03-02T15:19:29Z

src/dodal/common/maths.py

+class WrappedAxis(StandardReadable):
+    def __init__(self, real_motor: Motor, name=""):
+        self._real_motor = Reference(real_motor)
+        with self.add_children_as_readables(StandardReadableFormat.HINTED_SIGNAL):
+            self.offset_and_phase = derived_signal_rw(
+                self._get_motor_offset_and_phase,
+                self._set_motor_offset_and_phase,
+                motor_pos=real_motor,
+            )
+        with self.add_children_as_readables():
+            self.phase = derived_signal_rw(
+                self._get_phase, self._set_phase, offset_and_phase=self.offset_and_phase
+            )
+        super().__init__(name=name)
+
+    def _get_motor_offset_and_phase(self, motor_pos: float) -> MotorOffsetAndPhase:
+        angle = AngleWithPhase.wrap(motor_pos)
+        return np.array([angle.offset, angle.phase])
+
+    async def _set_motor_offset_and_phase(self, value: MotorOffsetAndPhase):
+        await self._real_motor().set(
+            AngleWithPhase.from_offset_and_phase(value).unwrap()
+        )
+
+    def _get_phase(self, offset_and_phase: MotorOffsetAndPhase) -> float:
+        return offset_and_phase[1].item()
+
+    async def _set_phase(self, value: float):
+        """Set the motor phase to the specified phase value in degrees.
+        The motor will travel via the shortest distance path.
+        """
+        offset_and_phase = await self.offset_and_phase.get_value()
+        current_position = AngleWithPhase.from_offset_and_phase(offset_and_phase)
+        target_value = current_position.nearest_to_phase(value).unwrap()
+        await self._real_motor().set(target_value)
+
+    def distance(self, theta1_deg: float, theta2_deg: float) -> float:
+        """Obtain the shortest distance between theta2 and theta1 in degrees.
+        This will be the shortest distance in mod360 space (i.e. always <= 180 degrees).
+        """
+        return AngleWithPhase.wrap(theta1_deg).phase_distance(
+            AngleWithPhase.wrap(theta2_deg)
+        )


This isn't common maths. This is a device and should live with where you define your custom stage.

Change calculate_motion_profile to start at the neareset omega

rtuck99 changed the title ~~Mx bluesky 1598 mod 360~~ Support for mod-360 omega Feb 24, 2026

jacob720 reviewed Feb 27, 2026

View reviewed changes

DominicOram requested changes Mar 2, 2026

View reviewed changes

oliwenmandiamond reviewed Mar 2, 2026

View reviewed changes

rtuck99 added 6 commits March 18, 2026 10:38

Rollup of initial experimental changes

1c9b363

Fix smargon combined moves

db9a17f

Update smargon limit tests

c6f84ad

Remove some unused methods in WrappedAxis

8b2387c

Change calculate_motion_profile to start at the neareset omega

Remove unused project_to_unwrapped

e8067ac

Reinstate distance method

a95aa88

rtuck99 force-pushed the mx-bluesky_1598_mod_360 branch from 585d794 to a95aa88 Compare March 18, 2026 10:40

Conversation

rtuck99 commented Feb 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Instructions to reviewer on how to test:

Checks for reviewer

Uh oh!

codecov bot commented Feb 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

oliwenmandiamond commented Feb 19, 2026

Uh oh!

jacob720 left a comment

Choose a reason for hiding this comment

Uh oh!

jacob720 Feb 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DominicOram left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oliwenmandiamond Mar 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

rtuck99 commented Feb 19, 2026 •

edited

Loading

codecov bot commented Feb 19, 2026 •

edited

Loading

jacob720 Feb 27, 2026 •

edited

Loading

oliwenmandiamond Mar 2, 2026 •

edited

Loading