Simplify ElectronAnalyserDetector driver reference (#1592)

* Simplify ElectronAnalyserDetector driver reference

* Addec comment on why driver is a child device

* Updated doc string

* Made controller private

* Correct spelling

---------

Co-authored-by: olliesilvester <[email protected]>

Author: oliwenmandiamond

src/dodal/devices/electron_analyser/abstract/__init__.py

@@ -1,5 +1,5 @@
 from .base_detector import (
-    AbstractElectronAnalyserDetector,
+    BaseElectronAnalyserDetector,
 )
 from .base_driver_io import AbstractAnalyserDriverIO, TAbstractAnalyserDriverIO
 from .base_region import (
@@ -19,7 +19,7 @@
     "TAcquisitionMode",
     "TLensMode",
     "AbstractAnalyserDriverIO",
-    "AbstractElectronAnalyserDetector",
+    "BaseElectronAnalyserDetector",
     "AbstractAnalyserDriverIO",
     "TAbstractAnalyserDriverIO",
 ]

src/dodal/devices/electron_analyser/abstract/base_detector.py

@@ -1,4 +1,3 @@
-from abc import abstractmethod
 from typing import Generic
 
 from bluesky.protocols import Reading, Triggerable
@@ -9,14 +8,14 @@
     AsyncStatus,
     Device,
 )
+from ophyd_async.epics.adcore import ADBaseController
 
-from dodal.devices.controllers import ConstantDeadTimeController
 from dodal.devices.electron_analyser.abstract.base_driver_io import (
     TAbstractAnalyserDriverIO,
 )
 
 
-class AbstractElectronAnalyserDetector(
+class BaseElectronAnalyserDetector(
     Device,
     Triggerable,
     AsyncReadable,
@@ -34,43 +33,31 @@ class AbstractElectronAnalyserDetector(
 
     def __init__(
         self,
-        driver: TAbstractAnalyserDriverIO,
+        controller: ADBaseController[TAbstractAnalyserDriverIO],
         name: str = "",
     ):
-        self.controller = ConstantDeadTimeController(driver, 0)
+        self._controller = controller
         super().__init__(name)
 
     @AsyncStatus.wrap
     async def trigger(self) -> None:
-        await self.controller.arm()
-        await self.controller.wait_for_idle()
+        await self._controller.arm()
+        await self._controller.wait_for_idle()
 
     async def read(self) -> dict[str, Reading]:
-        return await self.driver.read()
+        return await self._controller.driver.read()
 
     async def describe(self) -> dict[str, DataKey]:
-        data = await self.driver.describe()
+        data = await self._controller.driver.describe()
         # Correct the shape for image
-        prefix = self.driver.name + "-"
-        energy_size = len(await self.driver.energy_axis.get_value())
-        angle_size = len(await self.driver.angle_axis.get_value())
+        prefix = self._controller.driver.name + "-"
+        energy_size = len(await self._controller.driver.energy_axis.get_value())
+        angle_size = len(await self._controller.driver.angle_axis.get_value())
         data[prefix + "image"]["shape"] = [angle_size, energy_size]
         return data
 
     async def read_configuration(self) -> dict[str, Reading]:
-        return await self.driver.read_configuration()
+        return await self._controller.driver.read_configuration()
 
     async def describe_configuration(self) -> dict[str, DataKey]:
-        return await self.driver.describe_configuration()
-
-    @property
-    @abstractmethod
-    def driver(self) -> TAbstractAnalyserDriverIO:
-        """
-        Define common property for all implementations to access the driver. Some
-        implementations will store this as a reference so it doesn't have conflicting
-        parents.
-
-        Returns:
-            instance of the driver.
-        """
+        return await self._controller.driver.describe_configuration()

src/dodal/devices/electron_analyser/detector.py

@@ -1,14 +1,13 @@
 from typing import Generic, TypeVar
 
 from bluesky.protocols import Stageable
-from ophyd_async.core import (
-    AsyncStatus,
-    Reference,
-)
+from ophyd_async.core import AsyncStatus
+from ophyd_async.epics.adcore import ADBaseController
 
 from dodal.common.data_util import load_json_file_to_class
+from dodal.devices.controllers import ConstantDeadTimeController
 from dodal.devices.electron_analyser.abstract.base_detector import (
-    AbstractElectronAnalyserDetector,
+    BaseElectronAnalyserDetector,
 )
 from dodal.devices.electron_analyser.abstract.base_driver_io import (
     TAbstractAnalyserDriverIO,
@@ -20,35 +19,27 @@
 
 
 class ElectronAnalyserRegionDetector(
-    AbstractElectronAnalyserDetector[TAbstractAnalyserDriverIO],
+    BaseElectronAnalyserDetector[TAbstractAnalyserDriverIO],
     Generic[TAbstractAnalyserDriverIO, TAbstractBaseRegion],
 ):
     """
     Extends electron analyser detector to configure specific region settings before data
-    acqusition. This object must be passed in a driver and store it as a reference. It
-    is designed to only exist inside a plan.
+    acquisition. It is designed to only exist inside a plan.
     """
 
     def __init__(
         self,
-        driver: TAbstractAnalyserDriverIO,
+        controller: ADBaseController[TAbstractAnalyserDriverIO],
         region: TAbstractBaseRegion,
         name: str = "",
     ):
-        self._driver_ref = Reference(driver)
         self.region = region
-        super().__init__(driver, name)
-
-    @property
-    def driver(self) -> TAbstractAnalyserDriverIO:
-        # Store as a reference, this implementation will be given a driver so needs to
-        # make sure we don't get conflicting parents.
-        return self._driver_ref()
+        super().__init__(controller, name)
 
     @AsyncStatus.wrap
     async def trigger(self) -> None:
         # Configure region parameters on the driver first before data collection.
-        await self.driver.set(self.region)
+        await self._controller.driver.set(self.region)
         await super().trigger()
 
 
@@ -59,7 +50,7 @@ async def trigger(self) -> None:
 
 
 class ElectronAnalyserDetector(
-    AbstractElectronAnalyserDetector[TAbstractAnalyserDriverIO],
+    BaseElectronAnalyserDetector[TAbstractAnalyserDriverIO],
     Stageable,
     Generic[
         TAbstractAnalyserDriverIO,
@@ -79,16 +70,11 @@ def __init__(
         driver: TAbstractAnalyserDriverIO,
         name: str = "",
     ):
-        # Pass in driver
-        self._driver = driver
+        # Save driver as direct child so particpates with connect()
+        self.driver = driver
         self._sequence_class = sequence_class
-        super().__init__(self.driver, name)
-
-    @property
-    def driver(self) -> TAbstractAnalyserDriverIO:
-        # This implementation creates the driver and wants this to be the parent so it
-        # can be used with connect() method.
-        return self._driver
+        controller = ConstantDeadTimeController[TAbstractAnalyserDriverIO](driver, 0)
+        super().__init__(controller, name)
 
     @AsyncStatus.wrap
     async def stage(self) -> None:
@@ -103,13 +89,13 @@ async def stage(self) -> None:
         Raises:
             Any exceptions raised by the driver's stage or controller's disarm methods.
         """
-        await self.controller.disarm()
+        await self._controller.disarm()
         await self.driver.stage()
 
     @AsyncStatus.wrap
     async def unstage(self) -> None:
         """Disarm the detector."""
-        await self.controller.disarm()
+        await self._controller.disarm()
         await self.driver.unstage()
 
     def load_sequence(self, filename: str) -> TAbstractBaseSequence:
@@ -144,7 +130,9 @@ def create_region_detector_list(
         seq = self.load_sequence(filename)
         regions = seq.get_enabled_regions() if enabled_only else seq.regions
         return [
-            ElectronAnalyserRegionDetector(self.driver, r, self.name + "_" + r.name)
+            ElectronAnalyserRegionDetector(
+                self._controller, r, self.name + "_" + r.name
+            )
             for r in regions
         ]
 

tests/devices/electron_analyser/abstract/test_base_detector.py

@@ -14,7 +14,7 @@
 import dodal.devices.i09 as i09
 from dodal.devices.electron_analyser.abstract import (
     AbstractAnalyserDriverIO,
-    AbstractElectronAnalyserDetector,
+    BaseElectronAnalyserDetector,
 )
 from dodal.devices.electron_analyser.energy_sources import EnergySource
 from dodal.devices.electron_analyser.specs import SpecsDetector
@@ -32,7 +32,7 @@ async def sim_detector(
     request: pytest.FixtureRequest,
     single_energy_source: EnergySource,
     RE: RunEngine,
-) -> AbstractElectronAnalyserDetector[AbstractAnalyserDriverIO]:
+) -> BaseElectronAnalyserDetector[AbstractAnalyserDriverIO]:
     async with init_devices(mock=True):
         sim_detector = await create_detector(
             request.param,
@@ -45,50 +45,49 @@ async def sim_detector(
 
 
 def test_analyser_detector_trigger(
-    sim_detector: AbstractElectronAnalyserDetector[AbstractAnalyserDriverIO],
+    sim_detector: BaseElectronAnalyserDetector[AbstractAnalyserDriverIO],
     RE: RunEngine,
 ) -> None:
-    sim_detector.controller.arm = AsyncMock()
-    sim_detector.controller.wait_for_idle = AsyncMock()
+    sim_detector._controller.arm = AsyncMock()
+    sim_detector._controller.wait_for_idle = AsyncMock()
 
     RE(bps.trigger(sim_detector), wait=True)
 
-    sim_detector.controller.arm.assert_awaited_once()
-    sim_detector.controller.wait_for_idle.assert_awaited_once()
+    sim_detector._controller.arm.assert_awaited_once()
+    sim_detector._controller.wait_for_idle.assert_awaited_once()
 
 
 async def test_analyser_detector_read(
-    sim_detector: AbstractElectronAnalyserDetector[AbstractAnalyserDriverIO],
+    sim_detector: BaseElectronAnalyserDetector[AbstractAnalyserDriverIO],
 ) -> None:
-    driver = sim_detector.driver
-    driver_read = await driver.read()
+    driver_read = await sim_detector._controller.driver.read()
     await assert_reading(sim_detector, driver_read)
 
 
 async def test_analyser_describe(
-    sim_detector: AbstractElectronAnalyserDetector[AbstractAnalyserDriverIO],
+    sim_detector: BaseElectronAnalyserDetector[AbstractAnalyserDriverIO],
 ) -> None:
-    energy_array = await sim_detector.driver.energy_axis.get_value()
-    angle_array = await sim_detector.driver.angle_axis.get_value()
+    energy_array = await sim_detector._controller.driver.energy_axis.get_value()
+    angle_array = await sim_detector._controller.driver.angle_axis.get_value()
     data = await sim_detector.describe()
-    assert data[f"{sim_detector.name}-_driver-image"]["shape"] == [
+    assert data[f"{sim_detector._controller.driver.image.name}"]["shape"] == [
         len(angle_array),
         len(energy_array),
     ]
 
 
 async def test_analyser_detector_configuration(
-    sim_detector: AbstractElectronAnalyserDetector[AbstractAnalyserDriverIO],
+    sim_detector: BaseElectronAnalyserDetector[AbstractAnalyserDriverIO],
 ) -> None:
-    driver = sim_detector.driver
-    driver_config = await driver.read_configuration()
+    driver_config = await sim_detector._controller.driver.read_configuration()
     await assert_configuration(sim_detector, driver_config)
 
 
 async def test_analyser_detector_describe_configuration(
-    sim_detector: AbstractElectronAnalyserDetector[AbstractAnalyserDriverIO],
+    sim_detector: BaseElectronAnalyserDetector[AbstractAnalyserDriverIO],
 ) -> None:
-    driver = sim_detector.driver
-    driver_describe_config = await driver.describe_configuration()
+    driver_describe_config = (
+        await sim_detector._controller.driver.describe_configuration()
+    )
 
     assert await sim_detector.describe_configuration() == driver_describe_config

tests/devices/electron_analyser/test_detector.py

@@ -55,24 +55,24 @@ def test_analyser_detector_loads_sequence_correctly(
 async def test_analyser_detector_stage(
     sim_detector: GenericElectronAnalyserDetector,
 ) -> None:
-    sim_detector.controller.disarm = AsyncMock()
+    sim_detector._controller.disarm = AsyncMock()
     sim_detector.driver.stage = AsyncMock()
 
     await sim_detector.stage()
 
-    sim_detector.controller.disarm.assert_awaited_once()
+    sim_detector._controller.disarm.assert_awaited_once()
     sim_detector.driver.stage.assert_awaited_once()
 
 
 async def test_analyser_detector_unstage(
     sim_detector: GenericElectronAnalyserDetector,
 ) -> None:
-    sim_detector.controller.disarm = AsyncMock()
+    sim_detector._controller.disarm = AsyncMock()
     sim_detector.driver.unstage = AsyncMock()
 
     await sim_detector.unstage()
 
-    sim_detector.controller.disarm.assert_awaited_once()
+    sim_detector._controller.disarm.assert_awaited_once()
     sim_detector.driver.unstage.assert_awaited_once()
 
 
@@ -104,7 +104,7 @@ def test_analyser_detector_has_driver_as_child_and_region_detector_does_not(
 
     for det in region_detectors:
         assert det._child_devices.get(driver_name) is None
-        assert det.driver.parent == sim_detector
+        assert det._controller.driver.parent == sim_detector
 
 
 async def test_analyser_region_detector_trigger_sets_driver_with_region(
@@ -117,13 +117,13 @@ async def test_analyser_region_detector_trigger_sets_driver_with_region(
     )
 
     for reg_det in region_detectors:
-        reg_det.driver.set = AsyncMock()
+        reg_det._controller.driver.set = AsyncMock()
 
-        reg_det.controller.arm = AsyncMock()
-        reg_det.controller.wait_for_idle = AsyncMock()
+        reg_det._controller.arm = AsyncMock()
+        reg_det._controller.wait_for_idle = AsyncMock()
 
         RE(bps.trigger(reg_det), wait=True)
 
-        reg_det.controller.arm.assert_awaited_once()
-        reg_det.controller.wait_for_idle.assert_awaited_once()
-        reg_det.driver.set.assert_awaited_once_with(reg_det.region)
+        reg_det._controller.arm.assert_awaited_once()
+        reg_det._controller.wait_for_idle.assert_awaited_once()
+        reg_det._controller.driver.set.assert_awaited_once_with(reg_det.region)