Fix errors from testing mx_bluesky (#1036)

* Wait for setting ROI to finish before we call disarm finished

* Use correct PV for checking robot light curtain and do it in the device

* Add Stageable protocol for eiger

* Fix unit tests for robot

* Update names of tests

Author: DominicOram

src/dodal/devices/eiger.py

@@ -2,6 +2,7 @@
 from dataclasses import dataclass
 from enum import Enum
 
+from bluesky.protocols import Stageable
 from ophyd import Component, Device, EpicsSignalRO, Signal
 from ophyd.areadetector.cam import EigerDetectorCam
 from ophyd.status import AndStatus, Status, StatusBase
@@ -42,7 +43,7 @@ class InternalEigerTriggerMode(Enum):
 }
 
 
-class EigerDetector(Device):
+class EigerDetector(Device, Stageable):
     class ArmingSignal(Signal):
         def set(self, value, *, timeout=None, settle_time=None, **kwargs):
             assert isinstance(self.parent, EigerDetector)
@@ -161,7 +162,7 @@ def unstage(self) -> bool:
             status_ok = self.odin.check_and_wait_for_odin_state(
                 self.timeouts.general_status_timeout
             )
-            self.disable_roi_mode()
+            self.disable_roi_mode().wait(self.timeouts.general_status_timeout)
         self.disarming_status.set_finished()
         return status_ok
 

src/dodal/devices/robot.py

@@ -5,6 +5,7 @@
 from bluesky.protocols import Movable
 from ophyd_async.core import (
     AsyncStatus,
+    Device,
     StandardReadable,
     StrictEnum,
     set_and_wait_for_value,
@@ -38,23 +39,36 @@ class PinMounted(StrictEnum):
     PIN_MOUNTED = "Pin Mounted"
 
 
+class ErrorStatus(Device):
+    def __init__(self, prefix: str) -> None:
+        self.str = epics_signal_r(str, prefix + "_ERR_MSG")
+        self.code = epics_signal_r(int, prefix + "_ERR_CODE")
+        super().__init__()
+
+    async def raise_if_error(self, raise_from: Exception):
+        error_code = await self.code.get_value()
+        if error_code:
+            error_string = await self.str.get_value()
+            raise RobotLoadFailed(int(error_code), error_string) from raise_from
+
+
 class BartRobot(StandardReadable, Movable):
     """The sample changing robot."""
 
     # How long to wait for the robot if it is busy soaking/drying
     NOT_BUSY_TIMEOUT = 5 * 60
+
     # How long to wait for the actual load to happen
     LOAD_TIMEOUT = 60
+
+    # Error codes that we do special things on
     NO_PIN_ERROR_CODE = 25
+    LIGHT_CURTAIN_TRIPPED = 40
 
     # How far the gonio position can be out before loading will fail
     LOAD_TOLERANCE_MM = 0.02
 
-    def __init__(
-        self,
-        name: str,
-        prefix: str,
-    ) -> None:
+    def __init__(self, name: str, prefix: str) -> None:
         self.barcode = epics_signal_r(str, prefix + "BARCODE")
         self.gonio_pin_sensor = epics_signal_r(PinMounted, prefix + "PIN_MOUNTED")
 
@@ -69,8 +83,10 @@ def __init__(
         self.load = epics_signal_x(prefix + "LOAD.PROC")
         self.program_running = epics_signal_r(bool, prefix + "PROGRAM_RUNNING")
         self.program_name = epics_signal_r(str, prefix + "PROGRAM_NAME")
-        self.error_str = epics_signal_r(str, prefix + "PRG_ERR_MSG")
-        self.error_code = epics_signal_r(int, prefix + "PRG_ERR_CODE")
+
+        self.prog_error = ErrorStatus(prefix + "PRG")
+        self.controller_error = ErrorStatus(prefix + "CNTL")
+
         self.reset = epics_signal_x(prefix + "RESET.PROC")
         super().__init__(name=name)
 
@@ -81,7 +97,7 @@ async def pin_mounted_or_no_pin_found(self):
         """
 
         async def raise_if_no_pin():
-            await wait_for_value(self.error_code, self.NO_PIN_ERROR_CODE, None)
+            await wait_for_value(self.prog_error.code, self.NO_PIN_ERROR_CODE, None)
             raise RobotLoadFailed(self.NO_PIN_ERROR_CODE, "Pin was not detected")
 
         async def wfv():
@@ -108,6 +124,9 @@ async def wfv():
             raise
 
     async def _load_pin_and_puck(self, sample_location: SampleLocation):
+        if await self.controller_error.code.get_value() == self.LIGHT_CURTAIN_TRIPPED:
+            LOGGER.info("Light curtain tripped, trying again")
+            await self.reset.trigger()
         LOGGER.info(f"Loading pin {sample_location}")
         if await self.program_running.get_value():
             LOGGER.info(
@@ -137,6 +156,6 @@ async def set(self, value: SampleLocation):
             )
         except (asyncio.TimeoutError, TimeoutError) as e:
             # Will only need to catch asyncio.TimeoutError after https://github.com/bluesky/ophyd-async/issues/572
-            error_code = await self.error_code.get_value()
-            error_string = await self.error_str.get_value()
-            raise RobotLoadFailed(int(error_code), error_string) from e
+            await self.prog_error.raise_if_error(e)
+            await self.controller_error.raise_if_error(e)
+            raise RobotLoadFailed(0, "Robot timed out") from e

tests/devices/unit_tests/test_bart_robot.py

@@ -1,8 +1,14 @@
 from asyncio import create_task, sleep
-from unittest.mock import ANY, AsyncMock, MagicMock, patch
+from unittest.mock import ANY, AsyncMock, MagicMock, call, patch
 
 import pytest
-from ophyd_async.testing import set_mock_value
+from ophyd_async.core import AsyncStatus
+from ophyd_async.testing import (
+    callback_on_mock_put,
+    get_mock,
+    get_mock_put,
+    set_mock_value,
+)
 
 from dodal.devices.robot import BartRobot, PinMounted, RobotLoadFailed, SampleLocation
 
@@ -28,8 +34,8 @@ async def _sleep(*args):
 
     device._load_pin_and_puck = AsyncMock(side_effect=_sleep)
 
-    set_mock_value(device.error_code, (expected_error_code := 10))
-    set_mock_value(device.error_str, (expected_error_string := "BAD"))
+    set_mock_value(device.prog_error.code, (expected_error_code := 10))
+    set_mock_value(device.prog_error.str, (expected_error_string := "BAD"))
 
     with pytest.raises(RobotLoadFailed) as e:
         await device.set(SampleLocation(0, 0))
@@ -82,32 +88,36 @@ async def test_given_program_not_running_and_pin_unmounting_but_new_pin_not_moun
     assert "Waiting on new pin loaded" in last_log
 
 
-async def test_given_program_not_running_and_pin_unmounts_then_mounts_when_load_pin_then_pin_loaded():
-    device = await _get_bart_robot()
+async def set_with_happy_path(device: BartRobot) -> AsyncStatus:
+    """Mocks the logic that the robot would do on a successful load"""
     device.LOAD_TIMEOUT = 0.05  # type: ignore
     set_mock_value(device.program_running, False)
     set_mock_value(device.gonio_pin_sensor, PinMounted.PIN_MOUNTED)
-    device.load = AsyncMock(side_effect=device.load)
     status = device.set(SampleLocation(15, 10))
     await sleep(0.025)
-    device.load.trigger.assert_called_once()  # type:ignore
 
     set_mock_value(device.gonio_pin_sensor, PinMounted.NO_PIN_MOUNTED)
     await sleep(0.025)
 
     set_mock_value(device.gonio_pin_sensor, PinMounted.PIN_MOUNTED)
+    return status
+
+
+async def test_given_program_not_running_and_pin_unmounts_then_mounts_when_load_pin_then_pin_loaded():
+    device = await _get_bart_robot()
+    status = await set_with_happy_path(device)
     await status
     assert status.success
     assert (await device.next_puck.get_value()) == 15
     assert (await device.next_pin.get_value()) == 10
-    device.load.trigger.assert_called_once()  # type:ignore
+    get_mock_put(device.load).assert_called_once()
 
 
 async def test_given_waiting_for_pin_to_mount_when_no_pin_mounted_then_error_raised():
     device = await _get_bart_robot()
     status = create_task(device.pin_mounted_or_no_pin_found())
     await sleep(0.2)
-    set_mock_value(device.error_code, 25)
+    set_mock_value(device.prog_error.code, 25)
     await sleep(0.01)
     with pytest.raises(RobotLoadFailed):
         await status
@@ -128,3 +138,42 @@ async def test_set_waits_for_both_timeouts(mock_wait_for: AsyncMock):
     device._load_pin_and_puck = MagicMock()
     await device.set(SampleLocation(1, 2))
     mock_wait_for.assert_awaited_once_with(ANY, timeout=0.02)
+
+
+async def test_moving_the_robot_will_reset_error_if_light_curtain_is_tripped_and_still_throw_if_error_not_cleared():
+    device = await _get_bart_robot()
+    set_mock_value(device.controller_error.code, BartRobot.LIGHT_CURTAIN_TRIPPED)
+
+    with pytest.raises(RobotLoadFailed) as e:
+        await device.set(SampleLocation(1, 2))
+        assert e.value.error_code == 40
+
+    get_mock(device).assert_has_calls(
+        [
+            call.reset.put(None, wait=True),
+            call.next_puck.put(ANY, wait=True),
+            call.next_pin.put(ANY, wait=True),
+            call.load.put(None, wait=True),
+        ]
+    )
+
+
+async def test_moving_the_robot_will_reset_error_if_light_curtain_is_tripped_and_continue_if_error_cleared():
+    device = await _get_bart_robot()
+    set_mock_value(device.controller_error.code, BartRobot.LIGHT_CURTAIN_TRIPPED)
+
+    callback_on_mock_put(
+        device.reset,
+        lambda *_, **__: set_mock_value(device.controller_error.code, 0),
+    )
+
+    await (await set_with_happy_path(device))
+
+    get_mock(device).assert_has_calls(
+        [
+            call.reset.put(None, wait=True),
+            call.next_puck.put(ANY, wait=True),
+            call.next_pin.put(ANY, wait=True),
+            call.load.put(None, wait=True),
+        ]
+    )