Merge branch 'fix-movements' into 'v3'

Fix movements axes

Closes #498

See merge request openflexure/openflexure-microscope-server!338

src/openflexure_microscope_server/things/stage/__init__.py

@@ -11,33 +11,71 @@ As the object will be used as a context manager create the hardware connection i
 
 from __future__ import annotations
 from collections.abc import Sequence, Mapping
+from typing import Literal
 
 import labthings_fastapi as lt
 
 
+class RedefinedBaseMovementError(RuntimeError):
+    """The subclass of BaseStage has overridden ``move_relative`` or ``move_absolute``.
+
+    Overriding ``move_relative`` or ``move_absolute`` can be problematic as these use the
+    external position not the hardware position. It is recommended to override
+    ``_hardware_move_relative`` and ``_hardware_move_absolute`` instead.
+
+    The BaseStage will raise this on ``__init__``, it is the last thing ``__init__``
+    does. As such, this exception can be captured by ``try`` if a stage needs to
+    override these for a specific reason.
+    """
+
+
 class BaseStage(lt.Thing):
     """A base stage class for OpenFlexure translation stages.
 
     This can't be used directly but should reduce boilerplate code when
-    implementing new stages. A minimal working stage must implement
-    ``move_relative`` and ``move_absolute`` actions, which update the
-    ``position`` property on completion, and provide ``set_zero_position``.
+    implementing new stages.
+
+    Note that the coordinate system used for the microscope may need to have different
+    axis direction as those used by the underlying stage controller.
+
+    A minimal working stage must implement ``_hardware_move_relative``
+    and ``_hardware_move_absolute`` actions, which update the ``_hardware_position``
+    attribute on completion, and also should implement ``set_zero_position``.
     """
 
     _axis_names = ("x", "y", "z")
 
+    def __init__(self):
+        """Initialise the stage.
+
+        :raises RedefinedBaseMovementError: if ``move_relative`` and/or
+            ``move_absolute`` are overridden. It is recommended to override
+            ``_hardware_move_relative`` and/or ``_hardware_move_absolute`` instead so
+            that all code in the child class uses the hardware reference frame.
+        """
+        self._hardware_position = dict.fromkeys(self._axis_names, 0)
+
+        # This must be the last thing the function does in case it is caught in a try.
+        if (
+            self.__class__.move_relative.func is not BaseStage.move_relative.func
+            or self.__class__.move_absolute.func is not BaseStage.move_absolute.func
+        ):
+            raise RedefinedBaseMovementError(
+                "move_relative and/or move_absolute has been overridden. This may "
+                "cause issues as the base methods implement converting from program "
+                "coordinates to hardware coordinates. Consider overriding "
+                "_hardware_move_relative and/or _hardware_move_absolute instead."
+            )
+
     @lt.thing_property
     def axis_names(self) -> Sequence[str]:
         """The names of the stage's axes, in order."""
         return self._axis_names
 
-    position = lt.ThingProperty(
-        Mapping[str, int],
-        dict.fromkeys(_axis_names, 0),
-        readonly=True,
-        observable=True,
-    )
-    """Current position of the stage."""
+    @lt.thing_property
+    def position(self) -> Mapping[str, int]:
+        """Current position of the stage."""
+        return self._apply_axis_direction(self._hardware_position)
 
     moving = lt.ThingProperty(
         bool,
@@ -47,11 +85,56 @@ class BaseStage(lt.Thing):
     )
     """Whether the stage is in motion."""
 
+    axis_inverted = lt.ThingSetting(
+        initial_value={"x": False, "y": False, "z": False},
+        model=Mapping[str, bool],
+        readonly=True,
+    )
+    """Used to convert coordinates between the program frame and the hardware frame."""
+
+    def _apply_axis_direction(
+        self, position: list[int] | tuple[int] | Mapping[str, int]
+    ) -> list[int] | Mapping[str, int]:
+        if isinstance(position, (list, tuple)):
+            return [
+                -int(pos) if inverted else int(pos)
+                for pos, inverted in zip(position, self.axis_inverted.values())
+            ]
+        if isinstance(position, Mapping):
+            try:
+                return {
+                    ax: -int(position[ax])
+                    if self.axis_inverted[ax]
+                    else int(position[ax])
+                    for ax in position
+                }
+            except KeyError as e:
+                raise KeyError(
+                    f"One or more axis in {position.keys()} is not defined."
+                ) from e
+        raise TypeError(
+            "Position must be a sequence of positions or a mapping from axis to position."
+        )
+
     @property
     def thing_state(self):
         """Summary metadata describing the current state of the stage."""
         return {"position": self.position}
 
+    @lt.thing_action
+    def invert_axis_direction(self, axis: Literal["x", "y", "z"]):
+        """Invert the direction setting of the given axis.
+
+        :param axis: The axis name (x, y or z) to invert.
+        """
+        # Not mutating in place so that setting is saved on change.
+        direction = self.axis_inverted
+        try:
+            direction[axis] = not direction[axis]
+        except KeyError as e:
+            raise KeyError(f"The axis {axis} is not defined.") from e
+        self.axis_inverted = direction
+
     @lt.thing_action
     def move_relative(
         self,
@@ -60,8 +143,24 @@ class BaseStage(lt.Thing):
         **kwargs: Mapping[str, int],
     ):
         """Make a relative move. Keyword arguments should be axis names."""
+        self._hardware_move_relative(
+            cancel=cancel,
+            block_cancellation=block_cancellation,
+            **self._apply_axis_direction(kwargs),
+        )
+
+    def _hardware_move_relative(
+        self,
+        cancel: lt.deps.CancelHook,
+        block_cancellation: bool = False,
+        **kwargs: Mapping[str, int],
+    ):
+        """Make a relative move in the coordinate system used by the physical hardware.
+
+        Make sure to use and update ``self._hardware_position`` not ``self.position``.
+        """
         raise NotImplementedError(
-            "StageThings must define their own move_relative method"
+            "StageThings must define their own _hardware_move_relative method"
         )
 
     @lt.thing_action
@@ -72,6 +171,22 @@ class BaseStage(lt.Thing):
         **kwargs: Mapping[str, int],
     ):
         """Make an absolute move. Keyword arguments should be axis names."""
+        self._hardware_move_absolute(
+            cancel=cancel,
+            block_cancellation=block_cancellation,
+            **self._apply_axis_direction(kwargs),
+        )
+
+    def _hardware_move_absolute(
+        self,
+        cancel: lt.deps.CancelHook,
+        block_cancellation: bool = False,
+        **kwargs: Mapping[str, int],
+    ):
+        """Make a absolute move in the coordinate system used by the physical hardware.
+
+        Make sure to use and update ``self._hardware_position`` not ``self.position``.
+        """
         raise NotImplementedError(
             "StageThings must define their own move_absolute method"
         )

src/openflexure_microscope_server/things/stage/dummy.py

@@ -27,16 +27,23 @@ class DummyStage(BaseStage):
         """
         super().__init__(**kwargs)
         self.step_time = step_time
+        self.instantaneous_position = self._hardware_position
 
     def __enter__(self):
         """Register the stage position when the Thing context manager is opened."""
-        self.instantaneous_position = self.position
+        self.instantaneous_position = self._hardware_position
 
     def __exit__(self, _exc_type, _exc_value, _traceback):
         """Nothing to do when the Thing context manager is closed."""
 
-    @lt.thing_action
-    def move_relative(
+    axis_inverted = lt.ThingSetting(
+        initial_value={"x": True, "y": False, "z": False},
+        model=Mapping[str, bool],
+        readonly=True,
+    )
+    """Used to convert coordinates between the program frame and the hardware frame."""
+
+    def _hardware_move_relative(
         self,
         cancel: lt.deps.CancelHook,
         block_cancellation: bool = False,
@@ -57,8 +64,8 @@ class DummyStage(BaseStage):
                     cancel.sleep(dt)
                 fraction_complete = (time.time() - start_time) / (dt * max_displacement)
                 self.instantaneous_position = {
-                    k: self.position[k] + int(fraction_complete * v)
-                    for k, v in zip(self.axis_names, displacement)
+                    ax: self._hardware_position[ax] + int(fraction_complete * disp)
+                    for ax, disp in zip(self.axis_names, displacement)
                 }
             fraction_complete = 1.0
         except lt.exceptions.InvocationCancelledError as e:
@@ -68,14 +75,13 @@ class DummyStage(BaseStage):
             raise e
         finally:
             self.moving = False
-            self.position = {
-                k: self.position[k] + int(fraction_complete * v)
-                for k, v in zip(self.axis_names, displacement)
+            self._hardware_position = {
+                ax: self._hardware_position[ax] + int(fraction_complete * disp)
+                for ax, disp in zip(self.axis_names, displacement)
             }
-            self.instantaneous_position = self.position
+            self.instantaneous_position = self._hardware_position
 
-    @lt.thing_action
-    def move_absolute(
+    def _hardware_move_absolute(
         self,
         cancel: lt.deps.CancelHook,
         block_cancellation: bool = False,
@@ -83,11 +89,11 @@ class DummyStage(BaseStage):
     ):
         """Make an absolute move. Keyword arguments should be axis names."""
         displacement = {
-            axis: int(pos) - self.position[axis]
+            axis: int(pos) - self._hardware_position[axis]
             for axis, pos in kwargs.items()
             if axis in self.axis_names
         }
-        self.move_relative(
+        self._hardware_move_relative(
             cancel, block_cancellation=block_cancellation, **displacement
         )
 
@@ -99,5 +105,5 @@ class DummyStage(BaseStage):
         It is intended for use after manually or automatically recentring the
         stage.
         """
-        self.position = dict.fromkeys(self.axis_names, 0)
-        self.instantaneous_position = self.position
+        self._hardware_position = dict.fromkeys(self.axis_names, 0)
+        self.instantaneous_position = self._hardware_position

src/openflexure_microscope_server/things/stage/sangaboard.py

@@ -4,6 +4,7 @@ from __future__ import annotations
 import logging
 import threading
 import time
+from copy import copy
 from typing import Iterator, Literal
 from contextlib import contextmanager
 from collections.abc import Mapping
@@ -36,8 +37,9 @@ class SangaboardThing(BaseStage):
             Sangaboard class
 
         """
-        self.sangaboard_kwargs = kwargs
+        self.sangaboard_kwargs = copy(kwargs)
         self.sangaboard_kwargs["port"] = port
+        super().__init__(**kwargs)
 
     def __enter__(self):
         """Connect to the sangaboard when the Thing context manager is opened."""
@@ -65,19 +67,25 @@ class SangaboardThing(BaseStage):
         with self._sangaboard_lock:
             yield self._sangaboard
 
+    axis_inverted = lt.ThingSetting(
+        initial_value={"x": True, "y": False, "z": True},
+        model=Mapping[str, bool],
+        readonly=True,
+    )
+    """Used to convert coordinates between the program frame and the hardware frame."""
+
     def update_position(self) -> None:
         """Read position from the stage and set the corresponding property."""
         with self.sangaboard() as sb:
-            self.position = dict(zip(self.axis_names, sb.position))
+            self._hardware_position = dict(zip(self.axis_names, sb.position))
 
-    @lt.thing_action
-    def move_relative(
+    def _hardware_move_relative(
         self,
         cancel: lt.deps.CancelHook,
         block_cancellation: bool = False,
         **kwargs: Mapping[str, int],
     ) -> None:
-        """Make a relative move. Keyword arguments should be axis names."""
+        """Make a relative move in the coordinate system used by the sangaboard."""
         displacement = [kwargs.get(axis, 0) for axis in self.axis_names]
         with self.sangaboard() as sb:
             self.moving = True
@@ -98,22 +106,21 @@ class SangaboardThing(BaseStage):
                 self.moving = False
                 self.update_position()
 
-    @lt.thing_action
-    def move_absolute(
+    def _hardware_move_absolute(
         self,
         cancel: lt.deps.CancelHook,
         block_cancellation: bool = False,
         **kwargs: Mapping[str, int],
     ) -> None:
-        """Make an absolute move. Keyword arguments should be axis names."""
+        """Make a absolute move in the coordinate system used by the sangaboard."""
         with self.sangaboard():
             self.update_position()
             displacement = {
-                axis: int(pos) - self.position[axis]
+                axis: int(pos) - self._hardware_position[axis]
                 for axis, pos in kwargs.items()
                 if axis in self.axis_names
             }
-            self.move_relative(
+            self._hardware_move_relative(
                 cancel, block_cancellation=block_cancellation, **displacement
             )