From e2a9b9f523a87e4d3aff1feb007a3b2efa253224 Mon Sep 17 00:00:00 2001 From: huw-dls <53818372+huw-dls@users.noreply.github.com> Date: Mon, 13 Jan 2025 13:38:34 +0000 Subject: [PATCH 1/6] i13-1 merlin test initial commit (#965) * i13-1 merlin test initial commit * Removed commented out stuff thats not needed with ADCore 3-12 version * merlin_controller frame_timeout now init to 0 and print replaced with logging * merlindetector replaced with merlin * merlindetector now replaced with merlin * i13-1 merlin test initial commit * Removed commented out stuff thats not needed with ADCore 3-12 version * merlindetector replaced with merlin * Using LocalDirectoryServiceClient for visit directory (as the hdf path was being set to 2022) * Unit tests added. * Use absolute imports * Use correct imports * Apply suggestions from code review * Formatting and type checking for merlin tests * Drop validation of merlin trigger mode for now * Fix mock file path in merlin tests --------- Co-authored-by: Joseph Ware <53935796+DiamondJoseph@users.noreply.github.com> Co-authored-by: Callum Forrester --- src/dodal/beamlines/i13_1.py | 22 +++- src/dodal/devices/i13_1/__init__.py | 0 src/dodal/devices/i13_1/merlin.py | 33 ++++++ src/dodal/devices/i13_1/merlin_controller.py | 52 +++++++++ src/dodal/devices/i13_1/merlin_io.py | 17 +++ tests/devices/i13_1/test_merlin.py | 106 +++++++++++++++++++ 6 files changed, 228 insertions(+), 2 deletions(-) create mode 100644 src/dodal/devices/i13_1/__init__.py create mode 100644 src/dodal/devices/i13_1/merlin.py create mode 100644 src/dodal/devices/i13_1/merlin_controller.py create mode 100644 src/dodal/devices/i13_1/merlin_io.py create mode 100644 tests/devices/i13_1/test_merlin.py diff --git a/src/dodal/beamlines/i13_1.py b/src/dodal/beamlines/i13_1.py index fcd354a721..c32be50d1b 100644 --- a/src/dodal/beamlines/i13_1.py +++ b/src/dodal/beamlines/i13_1.py @@ -8,7 +8,8 @@ set_path_provider, ) from dodal.common.beamlines.beamline_utils import set_beamline as set_utils_beamline -from dodal.common.visit import StaticVisitPathProvider +from dodal.common.visit import LocalDirectoryServiceClient, StaticVisitPathProvider +from dodal.devices.i13_1.merlin import Merlin from dodal.devices.motors import XYZPositioner from dodal.log import set_beamline as set_log_beamline from dodal.utils import get_beamline_name @@ -19,7 +20,8 @@ set_path_provider( StaticVisitPathProvider( BL, - Path("/data/2024/cm37257-4/"), # latest commissioning visit + Path("/dls/i13-1/data/2024/cm37257-5/tmp/"), # latest commissioning visit + client=LocalDirectoryServiceClient(), ) ) @@ -64,3 +66,19 @@ def side_camera( wait=wait_for_connection, fake=fake_with_ophyd_sim, ) + + +def merlin( + wait_for_connection: bool = True, fake_with_ophyd_sim: bool = False +) -> Merlin: + return device_instantiation( + Merlin, + prefix="BL13J-EA-DET-04:", + name="merlin", + bl_prefix=False, + drv_suffix="CAM:", + hdf_suffix="HDF5:", + path_provider=get_path_provider(), + wait=wait_for_connection, + fake=fake_with_ophyd_sim, + ) diff --git a/src/dodal/devices/i13_1/__init__.py b/src/dodal/devices/i13_1/__init__.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/src/dodal/devices/i13_1/merlin.py b/src/dodal/devices/i13_1/merlin.py new file mode 100644 index 0000000000..940175f127 --- /dev/null +++ b/src/dodal/devices/i13_1/merlin.py @@ -0,0 +1,33 @@ +from ophyd_async.core import PathProvider, StandardDetector +from ophyd_async.epics import adcore + +from dodal.devices.i13_1.merlin_controller import MerlinController +from dodal.devices.i13_1.merlin_io import MerlinDriverIO + + +class Merlin(StandardDetector): + _controller: MerlinController + _writer: adcore.ADHDFWriter + + def __init__( + self, + prefix: str, + path_provider: PathProvider, + drv_suffix="CAM:", + hdf_suffix="HDF:", + name: str = "", + ): + self.drv = MerlinDriverIO(prefix + drv_suffix) + self.hdf = adcore.NDFileHDFIO(prefix + hdf_suffix) + + super().__init__( + MerlinController(self.drv), + adcore.ADHDFWriter( + self.hdf, + path_provider, + lambda: self.name, + adcore.ADBaseDatasetDescriber(self.drv), + ), + config_sigs=(self.drv.acquire_period, self.drv.acquire_time), + name=name, + ) diff --git a/src/dodal/devices/i13_1/merlin_controller.py b/src/dodal/devices/i13_1/merlin_controller.py new file mode 100644 index 0000000000..66f9e84ec9 --- /dev/null +++ b/src/dodal/devices/i13_1/merlin_controller.py @@ -0,0 +1,52 @@ +import asyncio +import logging + +from ophyd_async.core import ( + DEFAULT_TIMEOUT, + AsyncStatus, + DetectorController, + TriggerInfo, +) +from ophyd_async.epics import adcore + +from dodal.devices.i13_1.merlin_io import MerlinDriverIO, MerlinImageMode + + +class MerlinController(DetectorController): + def __init__( + self, + driver: MerlinDriverIO, + good_states: frozenset[adcore.DetectorState] = adcore.DEFAULT_GOOD_STATES, + ) -> None: + self.driver = driver + self.good_states = good_states + self.frame_timeout: float = 0 + self._arm_status: AsyncStatus | None = None + for drv_child in self.driver.children(): + logging.debug(drv_child) + + def get_deadtime(self, exposure: float | None) -> float: + return 0.002 + + async def prepare(self, trigger_info: TriggerInfo): + self.frame_timeout = ( + DEFAULT_TIMEOUT + await self.driver.acquire_time.get_value() + ) + await asyncio.gather( + self.driver.num_images.set(trigger_info.total_number_of_triggers), + self.driver.image_mode.set(MerlinImageMode.MULTIPLE), + ) + + async def arm(self): + self._arm_status = await adcore.start_acquiring_driver_and_ensure_status( + self.driver, good_states=self.good_states, timeout=self.frame_timeout + ) + + async def wait_for_idle(self): + if self._arm_status: + await self._arm_status + + async def disarm(self): + # We can't use caput callback as we already used it in arm() and we can't have + # 2 or they will deadlock + await adcore.stop_busy_record(self.driver.acquire, False, timeout=1) diff --git a/src/dodal/devices/i13_1/merlin_io.py b/src/dodal/devices/i13_1/merlin_io.py new file mode 100644 index 0000000000..b686a4c075 --- /dev/null +++ b/src/dodal/devices/i13_1/merlin_io.py @@ -0,0 +1,17 @@ +from ophyd_async.core import StrictEnum +from ophyd_async.epics import adcore +from ophyd_async.epics.core import epics_signal_rw_rbv + + +class MerlinImageMode(StrictEnum): + SINGLE = "Single" + MULTIPLE = "Multiple" + CONTINUOUS = "Continuous" + THRESHOLD = "Threshold" + BACKGROUND = "Background" + + +class MerlinDriverIO(adcore.ADBaseIO): + def __init__(self, prefix: str, name: str = "") -> None: + super().__init__(prefix, name) + self.image_mode = epics_signal_rw_rbv(MerlinImageMode, prefix + "ImageMode") diff --git a/tests/devices/i13_1/test_merlin.py b/tests/devices/i13_1/test_merlin.py new file mode 100644 index 0000000000..2332f9f6c5 --- /dev/null +++ b/tests/devices/i13_1/test_merlin.py @@ -0,0 +1,106 @@ +from typing import cast + +import pytest +from event_model import StreamDatum, StreamResource +from ophyd_async.core import ( + DetectorTrigger, + DeviceCollector, + PathProvider, + TriggerInfo, +) +from ophyd_async.testing import set_mock_value + +from dodal.devices.i13_1.merlin import Merlin + + +@pytest.fixture +def one_shot_trigger_info() -> TriggerInfo: + return TriggerInfo( + frame_timeout=None, + number_of_triggers=1, + trigger=DetectorTrigger.INTERNAL, + deadtime=None, + livetime=None, + ) + + +@pytest.fixture +async def merlin(static_path_provider: PathProvider) -> Merlin: + async with DeviceCollector(mock=True): + merlin = Merlin( + prefix="BL13J-EA-DET-04", + # name="merlin", + # drv_suffix="CAM:", + # hdf_suffix="HDF5:", + path_provider=static_path_provider, + ) + + return merlin + + +async def test_trigger( + merlin: Merlin, + one_shot_trigger_info: TriggerInfo, +): + set_mock_value(merlin.hdf.file_path_exists, True) + + await merlin.stage() + await merlin.prepare(one_shot_trigger_info) + await merlin.controller.arm() + + assert await merlin.drv.acquire.get_value() + + await merlin.controller.wait_for_idle() + + +async def test_can_collect( + merlin: Merlin, + static_path_provider: PathProvider, + one_shot_trigger_info: TriggerInfo, +): + set_mock_value(merlin.hdf.file_path_exists, True) + set_mock_value(merlin.drv.array_size_x, 10) + set_mock_value(merlin.drv.array_size_y, 20) + set_mock_value(merlin.hdf.num_frames_chunks, 1) + set_mock_value(merlin.hdf.full_file_name, "/foo/bar.hdf") + + await merlin.stage() + await merlin.prepare(one_shot_trigger_info) + docs = [(name, doc) async for name, doc in merlin.collect_asset_docs(1)] + assert len(docs) == 2 + assert docs[0][0] == "stream_resource" + stream_resource = cast(StreamResource, docs[0][1]) + + sr_uid = stream_resource["uid"] + assert stream_resource["data_key"] == "merlin" + assert stream_resource["uri"] == "file://localhost/foo/bar.hdf" + assert stream_resource["parameters"] == { + "dataset": "/entry/data/data", + "swmr": False, + "multiplier": 1, + "chunk_shape": (1, 20, 10), + } + assert docs[1][0] == "stream_datum" + stream_datum = cast(StreamDatum, docs[1][1]) + assert stream_datum["stream_resource"] == sr_uid + assert stream_datum["seq_nums"] == {"start": 0, "stop": 0} + assert stream_datum["indices"] == {"start": 0, "stop": 1} + + +async def test_can_decribe_collect(merlin: Merlin, one_shot_trigger_info: TriggerInfo): + set_mock_value(merlin.hdf.file_path_exists, True) + set_mock_value(merlin.drv.array_size_x, 10) + set_mock_value(merlin.drv.array_size_y, 20) + + assert (await merlin.describe_collect()) == {} + await merlin.stage() + await merlin.prepare(one_shot_trigger_info) + assert (await merlin.describe_collect()) == { + "merlin": { + "source": "mock+ca://BL13J-EA-DET-04HDF:FullFileName_RBV", + "shape": [20, 10], + "dtype": "array", + "dtype_numpy": "|i1", + "external": "STREAM:", + } + } From 4263907b933e667333bd6ef936a3ec6ba0d3488c Mon Sep 17 00:00:00 2001 From: Zoheb Shaikh Date: Tue, 14 Jan 2025 09:53:35 +0000 Subject: [PATCH 2/6] Add Panda to Training Rigs (#980) * add panda to training rig * change file path --- src/dodal/beamlines/training_rig.py | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/src/dodal/beamlines/training_rig.py b/src/dodal/beamlines/training_rig.py index 8c6782f8c5..7a75d14903 100644 --- a/src/dodal/beamlines/training_rig.py +++ b/src/dodal/beamlines/training_rig.py @@ -1,6 +1,7 @@ from pathlib import Path from ophyd_async.epics.adaravis import AravisDetector +from ophyd_async.fastcs.panda import HDFPanda from dodal.common.beamlines.beamline_utils import ( device_factory, @@ -33,7 +34,7 @@ set_path_provider( StaticVisitPathProvider( BL, - Path("/exports/mybeamline/data"), + Path("/data"), client=LocalDirectoryServiceClient(), ) ) @@ -52,3 +53,11 @@ def det() -> AravisDetector: drv_suffix="DET:", hdf_suffix=HDF5_PREFIX, ) + + +@device_factory() +def panda() -> HDFPanda: + return HDFPanda( + prefix=f"{PREFIX.beamline_prefix}-MO-PANDA-01:", + path_provider=get_path_provider(), + ) From a44bce1c1c3fbf22795eff5e7ab7bfc90941df24 Mon Sep 17 00:00:00 2001 From: Shihab S <162436767+shihab-dls@users.noreply.github.com> Date: Wed, 15 Jan 2025 12:03:39 +0000 Subject: [PATCH 3/6] Add import linting (#988) * Change p45 to phyd async * Use standard readable instead of device * Forbid imports from beamlines except for tests * Test importing from beamlines to tests * Fix ci linting job * Fix patch path for save panda plan * Fix patch path for save panda plan * Revert unwanted changes from branch * Remove duplicate work from actions workflow --------- Co-authored-by: Shihab Suliman --- .github/workflows/_tox.yml | 3 +++ .pre-commit-config.yaml | 8 ++++++++ pyproject.toml | 15 +++++++++++++++ .../{devices/util => plans}/save_panda.py | 0 .../devices/unit_tests/util/test_save_panda.py | 18 +++++++++--------- 5 files changed, 35 insertions(+), 9 deletions(-) rename src/dodal/{devices/util => plans}/save_panda.py (100%) diff --git a/.github/workflows/_tox.yml b/.github/workflows/_tox.yml index ec3bb5b513..62825096b0 100644 --- a/.github/workflows/_tox.yml +++ b/.github/workflows/_tox.yml @@ -20,5 +20,8 @@ jobs: - name: Install python packages uses: ./.github/actions/install_requirements + - name: Run import linter + run: lint-imports + - name: Run tox run: tox -e ${{ inputs.tox }} diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 60fc23f9a7..de130334ed 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -22,3 +22,11 @@ repos: entry: ruff format --force-exclude types: [python] require_serial: true + + - id: import-contracts + name: Ensure import directionality + pass_filenames: false + language: system + entry: lint-imports + types: [python] + require_serial: false diff --git a/pyproject.toml b/pyproject.toml index 32727d97da..28093ed942 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -44,6 +44,7 @@ requires-python = ">=3.10" dev = [ "black", "diff-cover", + "import-linter", "mypy", # Commented out due to dependency version conflict with pydantic 1.x # "copier", @@ -181,3 +182,17 @@ lint.select = [ # Remove this line to forbid private member access in tests "tests/**/*" = ["SLF001"] "system_tests/**/*" = ["SLF001"] + +[tool.importlinter] +root_package = "dodal" + +[[tool.importlinter.contracts]] +name = "Common cannot import from beamlines" +type = "forbidden" +source_modules = ["dodal.common"] +forbidden_modules = ["dodal.beamlines"] + +[[tool.importlinter.contracts]] +name = "Enforce import order" +type = "layers" +layers = ["dodal.plans", "dodal.beamlines", "dodal.devices"] diff --git a/src/dodal/devices/util/save_panda.py b/src/dodal/plans/save_panda.py similarity index 100% rename from src/dodal/devices/util/save_panda.py rename to src/dodal/plans/save_panda.py diff --git a/tests/devices/unit_tests/util/test_save_panda.py b/tests/devices/unit_tests/util/test_save_panda.py index 27d94d5196..867effa50c 100644 --- a/tests/devices/unit_tests/util/test_save_panda.py +++ b/tests/devices/unit_tests/util/test_save_panda.py @@ -5,7 +5,7 @@ from bluesky.simulators import RunEngineSimulator from ophyd_async.fastcs.panda import phase_sorter -from dodal.devices.util.save_panda import _save_panda, main +from dodal.plans.save_panda import _save_panda, main def test_save_panda(): @@ -13,13 +13,13 @@ def test_save_panda(): panda = MagicMock() with ( patch( - "dodal.devices.util.save_panda.make_device", return_value={"panda": panda} + "dodal.plans.save_panda.make_device", return_value={"panda": panda} ) as mock_make_device, patch( - "dodal.devices.util.save_panda.RunEngine", + "dodal.plans.save_panda.RunEngine", return_value=MagicMock(side_effect=sim_run_engine.simulate_plan), ), - patch("dodal.devices.util.save_panda.save_device") as mock_save_device, + patch("dodal.plans.save_panda.save_device") as mock_save_device, ): _save_panda("i03", "panda", "test/file.yml") @@ -28,12 +28,12 @@ def test_save_panda(): @patch( - "dodal.devices.util.save_panda.sys.exit", + "dodal.plans.save_panda.sys.exit", side_effect=AssertionError("This exception expected"), ) def test_save_panda_failure_to_create_device_exits_with_failure_code(mock_exit): with patch( - "dodal.devices.util.save_panda.make_device", + "dodal.plans.save_panda.make_device", side_effect=ValueError("device does not exist"), ): with pytest.raises(AssertionError): @@ -42,7 +42,7 @@ def test_save_panda_failure_to_create_device_exits_with_failure_code(mock_exit): mock_exit.assert_called_once_with(1) -@patch("dodal.devices.util.save_panda._save_panda") +@patch("dodal.plans.save_panda._save_panda") @pytest.mark.parametrize( "beamline, args, expected_beamline, expected_device_name, expected_output_file, " "expected_return_value", @@ -115,8 +115,8 @@ def test_main( (True, True, True, 0), ], ) -@patch("dodal.devices.util.save_panda._save_panda") -@patch("dodal.devices.util.save_panda.Path", autospec=True) +@patch("dodal.plans.save_panda._save_panda") +@patch("dodal.plans.save_panda.Path", autospec=True) def test_file_exists_check( mock_path: MagicMock, mock_save_panda: MagicMock, From 317698f586cad64fe3c38a4c5995f0733d22451f Mon Sep 17 00:00:00 2001 From: Daniel Fernandes <65790536+dan-fernandes@users.noreply.github.com> Date: Wed, 15 Jan 2025 14:01:58 +0000 Subject: [PATCH 4/6] Fix typo in reviews.md (#989) Humiliary is not a word. --- docs/explanations/reviews.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/explanations/reviews.md b/docs/explanations/reviews.md index 78a9ab48b2..ad56fa048d 100644 --- a/docs/explanations/reviews.md +++ b/docs/explanations/reviews.md @@ -51,6 +51,6 @@ New members should be regular contributors to dodal and should have been "shadow - Adherence to the review standards above as well as the [repository standards](../reference/standards.rst) and [device standards](../reference/device-standards.rst). - Independent (i.e. not just to satisfy a reviewer) motivation to make sure all code in dodal is well-tested. Use of unit and system tests as appropriate. Clear effort made to keep test coverage high (>90%). - Advanced understanding of bluesky and ophyd-async including concepts and best practices, such as how to appropriately split logic between devices/plans/callbacks. -- Humility in the use of reviewing as a tool in a way that balances the need to preserve quality with the need for progress. Appropriate use of the Must/Should/Could/Nit system documented above is helpful in showing this, as it gives the reviewer the opportunity to weight their comments in terms of project impact. They should also demonstrate similar humiliary as a reviewee. +- Humility in the use of reviewing as a tool in a way that balances the need to preserve quality with the need for progress. Appropriate use of the Must/Should/Could/Nit system documented above is helpful in showing this, as it gives the reviewer the opportunity to weight their comments in terms of project impact. They should also demonstrate similar humility as a reviewee. Additionally, they should be regularly raising issues in the repository and demonstrating the ability to write well formed issues, with well defined acceptance criteria, that are understandable without large amounts of context. From 157afd6c1b72810ee0a95e8706459b5d5de13279 Mon Sep 17 00:00:00 2001 From: Shihab S <162436767+shihab-dls@users.noreply.github.com> Date: Wed, 15 Jan 2025 15:37:27 +0000 Subject: [PATCH 5/6] Remove ophyd references from motion devices (#985) * Change p45 to phyd async * Use standard readable instead of device * Instantiate p45 devices correctly * Add prefix to device pvs * Convert i03 and i04 devices to ophyd_async --------- Co-authored-by: Shihab Suliman --- src/dodal/devices/flux.py | 13 ++++++-- src/dodal/devices/p45.py | 51 +++++++++++++++++++------------ src/dodal/devices/s4_slit_gaps.py | 12 +++++--- 3 files changed, 49 insertions(+), 27 deletions(-) diff --git a/src/dodal/devices/flux.py b/src/dodal/devices/flux.py index c8fcd781ba..bd425f6940 100644 --- a/src/dodal/devices/flux.py +++ b/src/dodal/devices/flux.py @@ -1,7 +1,14 @@ -from ophyd import Component, Device, EpicsSignalRO, Kind +from ophyd_async.core import ( + StandardReadable, + StandardReadableFormat, +) +from ophyd_async.epics.core import epics_signal_r -class Flux(Device): +class Flux(StandardReadable): """Simple device to get the flux reading""" - flux_reading = Component(EpicsSignalRO, "SAMP", kind=Kind.hinted) + def __init__(self, prefix: str, name="") -> None: + with self.add_children_as_readables(StandardReadableFormat.HINTED_SIGNAL): + self.flux_reading = epics_signal_r(float, prefix + "SAMP") + super().__init__(name=name) diff --git a/src/dodal/devices/p45.py b/src/dodal/devices/p45.py index f2939d142d..2a2de45160 100644 --- a/src/dodal/devices/p45.py +++ b/src/dodal/devices/p45.py @@ -1,44 +1,55 @@ -from ophyd import Component as Cpt -from ophyd import EpicsMotor, MotorBundle -from ophyd.areadetector.base import ADComponent as Cpt +from ophyd_async.core import StandardReadable +from ophyd_async.epics.motor import Motor -class SampleY(MotorBundle): +class SampleY(StandardReadable): """ Motors for controlling the sample's y position and stretch in the y axis. """ - base = Cpt(EpicsMotor, "CS:Y") - stretch = Cpt(EpicsMotor, "CS:Y:STRETCH") - top = Cpt(EpicsMotor, "Y:TOP") - bottom = Cpt(EpicsMotor, "Y:BOT") + def __init__(self, prefix: str, name="") -> None: + with self.add_children_as_readables(): + self.base = Motor(prefix + "CS:Y") + self.stretch = Motor(prefix + "CS:Y:STRETCH") + self.top = Motor(prefix + "Y:TOP") + self.bottom = Motor(prefix + "Y:BOT") + super().__init__(name=name) -class SampleTheta(MotorBundle): +class SampleTheta(StandardReadable): """ Motors for controlling the sample's theta position and skew """ - base = Cpt(EpicsMotor, "THETA:POS") - skew = Cpt(EpicsMotor, "THETA:SKEW") - top = Cpt(EpicsMotor, "THETA:TOP") - bottom = Cpt(EpicsMotor, "THETA:BOT") + def __init__(self, prefix: str, name="") -> None: + with self.add_children_as_readables(): + self.base = Motor(prefix + "THETA:POS") + self.skew = Motor(prefix + "THETA:SKEW") + self.top = Motor(prefix + "THETA:TOP") + self.bottom = Motor(prefix + "THETA:BOT") + super().__init__(name=name) -class TomoStageWithStretchAndSkew(MotorBundle): +class TomoStageWithStretchAndSkew(StandardReadable): """ Grouping of motors for the P45 tomography stage """ - x = Cpt(EpicsMotor, "X") - y = Cpt(SampleY, "") - theta = Cpt(SampleTheta, "") + def __init__(self, prefix: str, name="") -> None: + with self.add_children_as_readables(): + self.x = Motor(prefix + "X") + self.y = SampleY(prefix) + self.theta = SampleTheta(prefix) + super().__init__(name=name) -class Choppers(MotorBundle): +class Choppers(StandardReadable): """ Grouping for the P45 chopper motors """ - x = Cpt(EpicsMotor, "ENDAT") - y = Cpt(EpicsMotor, "BISS") + def __init__(self, prefix: str, name="") -> None: + with self.add_children_as_readables(): + self.x = Motor(prefix + "ENDAT") + self.y = Motor(prefix + "BISS") + super().__init__(name=name) diff --git a/src/dodal/devices/s4_slit_gaps.py b/src/dodal/devices/s4_slit_gaps.py index 522bb551ac..45e04c9ca8 100644 --- a/src/dodal/devices/s4_slit_gaps.py +++ b/src/dodal/devices/s4_slit_gaps.py @@ -1,8 +1,12 @@ -from ophyd import Component, Device, EpicsMotor +from ophyd_async.core import StandardReadable +from ophyd_async.epics.motor import Motor -class S4SlitGaps(Device): +class S4SlitGaps(StandardReadable): """Note that the S4 slits have a different PV fromat to other beamline slits""" - xgap = Component(EpicsMotor, "XGAP") - ygap = Component(EpicsMotor, "YGAP") + def __init__(self, prefix: str, name="") -> None: + with self.add_children_as_readables(): + self.xgap = Motor(prefix + "XGAP") + self.ygap = Motor(prefix + "YGAP") + super().__init__(name=name) From 413aa34578c29b85f52944c29a76e248ab0eb87c Mon Sep 17 00:00:00 2001 From: olliesilvester <122091460+olliesilvester@users.noreply.github.com> Date: Thu, 16 Jan 2025 10:44:28 +0000 Subject: [PATCH 6/6] Add beamline-specific zebra mappings to zebra device (#976) * Add beamline-specific zebra mappings to zebra device * Add more structure to the zebra mappings * Use -1 as sentinel value --- src/dodal/beamlines/i03.py | 16 +++- src/dodal/beamlines/i04.py | 15 ++- src/dodal/beamlines/i24.py | 13 ++- src/dodal/devices/zebra/__init__.py | 0 src/dodal/devices/{ => zebra}/zebra.py | 35 +------ .../devices/zebra/zebra_constants_mapping.py | 96 +++++++++++++++++++ .../{ => zebra}/zebra_controlled_shutter.py | 0 system_tests/test_zebra_system.py | 5 +- tests/common/beamlines/test_beamline_utils.py | 13 ++- tests/devices/unit_tests/test_shutter.py | 2 +- tests/devices/unit_tests/test_zebra.py | 2 +- .../test_zebra_constants_mapping.py | 42 ++++++++ 12 files changed, 191 insertions(+), 48 deletions(-) create mode 100644 src/dodal/devices/zebra/__init__.py rename src/dodal/devices/{ => zebra}/zebra.py (93%) create mode 100644 src/dodal/devices/zebra/zebra_constants_mapping.py rename src/dodal/devices/{ => zebra}/zebra_controlled_shutter.py (100%) create mode 100644 tests/devices/unit_tests/test_zebra_constants_mapping.py diff --git a/src/dodal/beamlines/i03.py b/src/dodal/beamlines/i03.py index 07e116604a..612f83c7ce 100644 --- a/src/dodal/beamlines/i03.py +++ b/src/dodal/beamlines/i03.py @@ -40,8 +40,13 @@ from dodal.devices.webcam import Webcam from dodal.devices.xbpm_feedback import XBPMFeedback from dodal.devices.xspress3.xspress3 import Xspress3 -from dodal.devices.zebra import Zebra -from dodal.devices.zebra_controlled_shutter import ZebraShutter +from dodal.devices.zebra.zebra import Zebra +from dodal.devices.zebra.zebra_constants_mapping import ( + ZebraMapping, + ZebraSources, + ZebraTTLOutputs, +) +from dodal.devices.zebra.zebra_controlled_shutter import ZebraShutter from dodal.devices.zocalo import ZocaloResults from dodal.log import set_beamline as set_log_beamline from dodal.utils import BeamlinePrefix, get_beamline_name, skip_device @@ -58,6 +63,12 @@ set_path_provider(PandASubpathProvider()) +I03_ZEBRA_MAPPING = ZebraMapping( + outputs=ZebraTTLOutputs(TTL_DETECTOR=1, TTL_SHUTTER=2, TTL_XSPRESS3=3, TTL_PANDA=4), + sources=ZebraSources(), + AND_GATE_FOR_AUTO_SHUTTER=2, +) + def aperture_scatterguard( wait_for_connection: bool = True, @@ -368,6 +379,7 @@ def zebra(wait_for_connection: bool = True, fake_with_ophyd_sim: bool = False) - "-EA-ZEBRA-01:", wait_for_connection, fake_with_ophyd_sim, + mapping=I03_ZEBRA_MAPPING, ) diff --git a/src/dodal/beamlines/i04.py b/src/dodal/beamlines/i04.py index 1494c9c300..952f425e31 100644 --- a/src/dodal/beamlines/i04.py +++ b/src/dodal/beamlines/i04.py @@ -28,8 +28,13 @@ from dodal.devices.thawer import Thawer from dodal.devices.undulator import Undulator from dodal.devices.xbpm_feedback import XBPMFeedback -from dodal.devices.zebra import Zebra -from dodal.devices.zebra_controlled_shutter import ZebraShutter +from dodal.devices.zebra.zebra import Zebra +from dodal.devices.zebra.zebra_constants_mapping import ( + ZebraMapping, + ZebraSources, + ZebraTTLOutputs, +) +from dodal.devices.zebra.zebra_controlled_shutter import ZebraShutter from dodal.log import set_beamline as set_log_beamline from dodal.utils import BeamlinePrefix, get_beamline_name, skip_device @@ -47,6 +52,11 @@ set_log_beamline(BL) set_utils_beamline(BL) +I04_ZEBRA_MAPPING = ZebraMapping( + outputs=(ZebraTTLOutputs(TTL_DETECTOR=1, TTL_FAST_SHUTTER=2, TTL_XSPRESS3=3)), + sources=ZebraSources(), +) + def smargon( wait_for_connection: bool = True, fake_with_ophyd_sim: bool = False @@ -341,6 +351,7 @@ def zebra(wait_for_connection: bool = True, fake_with_ophyd_sim: bool = False) - "-EA-ZEBRA-01:", wait_for_connection, fake_with_ophyd_sim, + mapping=I04_ZEBRA_MAPPING, ) diff --git a/src/dodal/beamlines/i24.py b/src/dodal/beamlines/i24.py index 4dc35dfd86..9345796f46 100644 --- a/src/dodal/beamlines/i24.py +++ b/src/dodal/beamlines/i24.py @@ -16,7 +16,12 @@ from dodal.devices.i24.vgonio import VerticalGoniometer from dodal.devices.oav.oav_detector import OAV from dodal.devices.oav.oav_parameters import OAVConfig -from dodal.devices.zebra import Zebra +from dodal.devices.zebra.zebra import Zebra +from dodal.devices.zebra.zebra_constants_mapping import ( + ZebraMapping, + ZebraSources, + ZebraTTLOutputs, +) from dodal.log import set_beamline as set_log_beamline from dodal.utils import get_beamline_name, skip_device @@ -29,6 +34,11 @@ set_log_beamline(BL) set_utils_beamline(BL) +I24_ZEBRA_MAPPING = ZebraMapping( + outputs=ZebraTTLOutputs(TTL_EIGER=1, TTL_PILATUS=2, TTL_FAST_SHUTTER=4), + sources=ZebraSources(), +) + def attenuator( wait_for_connection: bool = True, fake_with_ophyd_sim: bool = False @@ -191,6 +201,7 @@ def zebra(wait_for_connection: bool = True, fake_with_ophyd_sim: bool = False) - "-EA-ZEBRA-01:", wait_for_connection, fake_with_ophyd_sim, + mapping=I24_ZEBRA_MAPPING, ) diff --git a/src/dodal/devices/zebra/__init__.py b/src/dodal/devices/zebra/__init__.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/src/dodal/devices/zebra.py b/src/dodal/devices/zebra/zebra.py similarity index 93% rename from src/dodal/devices/zebra.py rename to src/dodal/devices/zebra/zebra.py index 477a3284a8..f7becfef4a 100644 --- a/src/dodal/devices/zebra.py +++ b/src/dodal/devices/zebra/zebra.py @@ -14,37 +14,7 @@ ) from ophyd_async.epics.core import epics_signal_r, epics_signal_rw -# These constants refer to I03's Zebra. See https://github.com/DiamondLightSource/dodal/issues/772 -# Sources -DISCONNECT = 0 -IN1_TTL = 1 -IN2_TTL = 4 -IN3_TTL = 7 -IN4_TTL = 10 -PC_ARM = 29 -PC_GATE = 30 -PC_PULSE = 31 -AND3 = 34 -AND4 = 35 -OR1 = 36 -PULSE1 = 52 -PULSE2 = 53 -SOFT_IN1 = 60 -SOFT_IN2 = 61 -SOFT_IN3 = 62 - -# Instrument specific -TTL_DETECTOR = 1 -TTL_SHUTTER = 2 -TTL_XSPRESS3 = 3 -TTL_PANDA = 4 - -# The AND gate that controls the automatic shutter -AUTO_SHUTTER_GATE = 2 - -# The first two inputs of the auto shutter gate. -AUTO_SHUTTER_INPUT_1 = 1 -AUTO_SHUTTER_INPUT_2 = 2 +from dodal.devices.zebra.zebra_constants_mapping import ZebraMapping class ArmSource(StrictEnum): @@ -303,7 +273,8 @@ def __init__(self, prefix: str, name: str = "") -> None: class Zebra(StandardReadable): """The Zebra device.""" - def __init__(self, name: str, prefix: str) -> None: + def __init__(self, mapping: ZebraMapping, name: str, prefix: str) -> None: + self.mapping = mapping self.pc = PositionCompare(prefix, name) self.output = ZebraOutputPanel(prefix, name) self.inputs = SoftInputs(prefix, name) diff --git a/src/dodal/devices/zebra/zebra_constants_mapping.py b/src/dodal/devices/zebra/zebra_constants_mapping.py new file mode 100644 index 0000000000..e9eaf1f695 --- /dev/null +++ b/src/dodal/devices/zebra/zebra_constants_mapping.py @@ -0,0 +1,96 @@ +from collections import Counter + +from pydantic import BaseModel, Field, model_validator + + +class ZebraMappingValidations(BaseModel): + """Raises an exception if field set to -1 is accessed, and validate against + multiple fields mapping to the same integer""" + + def __getattribute__(self, name: str): + """To protect against mismatch between the Zebra configuration that a plan expects and the Zebra which has + been instantiated, raise exception if a field which has been set to -1 is accessed.""" + value = object.__getattribute__(self, name) + if not name.startswith("__"): + if value == -1: + raise UnmappedZebraException( + f"'{type(self).__name__}.{name}' was accessed but is set to -1. Please check the zebra mappings against the zebra's physical configuration" + ) + return value + + @model_validator(mode="after") + def ensure_no_duplicate_connections(self): + """Check that TTL outputs and sources are mapped to unique integers""" + + integer_fields = { + key: value + for key, value in self.model_dump().items() + if isinstance(value, int) and value != -1 + } + counted_vals = Counter(integer_fields.values()) + integer_fields_with_duplicates = { + k: v for k, v in integer_fields.items() if counted_vals[v] > 1 + } + if len(integer_fields_with_duplicates): + raise ValueError( + f"Each field in {type(self)} must be mapped to a unique integer. Duplicate fields: {integer_fields_with_duplicates}" + ) + return self + + +class ZebraTTLOutputs(ZebraMappingValidations): + """Maps hardware to the Zebra TTL output (1-4) that they're physically wired to, or + None if that hardware is not connected. A value of -1 means this hardware is not connected.""" + + TTL_EIGER: int = Field(default=-1, ge=-1, le=4) + TTL_PILATUS: int = Field(default=-1, ge=-1, le=4) + TTL_FAST_SHUTTER: int = Field(default=-1, ge=-1, le=4) + TTL_DETECTOR: int = Field(default=-1, ge=-1, le=4) + TTL_SHUTTER: int = Field(default=-1, ge=-1, le=4) + TTL_XSPRESS3: int = Field(default=-1, ge=-1, le=4) + TTL_PANDA: int = Field(default=-1, ge=-1, le=4) + + +class ZebraSources(ZebraMappingValidations): + """Maps internal Zebra signal source to their integer PV value""" + + DISCONNECT: int = Field(default=0, ge=0, le=63) + IN1_TTL: int = Field(default=1, ge=0, le=63) + IN2_TTL: int = Field(default=63, ge=0, le=63) + IN3_TTL: int = Field(default=7, ge=0, le=63) + IN4_TTL: int = Field(default=10, ge=0, le=63) + PC_ARM: int = Field(default=29, ge=0, le=63) + PC_GATE: int = Field(default=30, ge=0, le=63) + PC_PULSE: int = Field(default=31, ge=0, le=63) + AND3: int = Field(default=34, ge=0, le=63) + AND4: int = Field(default=35, ge=0, le=63) + OR1: int = Field(default=36, ge=0, le=63) + PULSE1: int = Field(default=52, ge=0, le=63) + PULSE2: int = Field(default=53, ge=0, le=63) + SOFT_IN1: int = Field(default=60, ge=0, le=63) + SOFT_IN2: int = Field(default=61, ge=0, le=63) + SOFT_IN3: int = Field(default=62, ge=0, le=63) + + +class ZebraMapping(ZebraMappingValidations): + """Mappings to locate a Zebra device's Ophyd signals based on a specific + Zebra's hardware configuration and wiring. + """ + + # Zebra ophyd signal for connection can be accessed + # with, eg, zebra.output.out_pvs[zebra.mapping.outputs.TTL_DETECTOR] + outputs: ZebraTTLOutputs = ZebraTTLOutputs() + + # Zebra ophyd signal sources can be mapped to a zebra output by doing, eg, + # bps.abs_set(zebra.output.out_pvs[zebra.mapping.outputs.TTL_DETECTOR], + # zebra.mapping.sources.AND3) + sources: ZebraSources = ZebraSources() + + # Which of the Zebra's four AND gates is used to control the automatic shutter, if it's being used. + # After defining, the correct GateControl device can be accessed with, eg, + # zebra.logic_gates.and_gates[zebra.mapping.AND_GATE_FOR_AUTO_SHUTTER]. Set to -1 if not being used. + AND_GATE_FOR_AUTO_SHUTTER: int = Field(default=-1, ge=-1, le=4) + + +class UnmappedZebraException(Exception): + pass diff --git a/src/dodal/devices/zebra_controlled_shutter.py b/src/dodal/devices/zebra/zebra_controlled_shutter.py similarity index 100% rename from src/dodal/devices/zebra_controlled_shutter.py rename to src/dodal/devices/zebra/zebra_controlled_shutter.py diff --git a/system_tests/test_zebra_system.py b/system_tests/test_zebra_system.py index 74517c36c7..28f30e589c 100644 --- a/system_tests/test_zebra_system.py +++ b/system_tests/test_zebra_system.py @@ -1,11 +1,12 @@ import pytest -from dodal.devices.zebra import ArmDemand, Zebra +from dodal.beamlines.i03 import I03_ZEBRA_MAPPING +from dodal.devices.zebra.zebra import ArmDemand, Zebra @pytest.fixture() async def zebra(): - zebra = Zebra(name="zebra", prefix="BL03S-EA-ZEBRA-01:") + zebra = Zebra(name="zebra", prefix="BL03S-EA-ZEBRA-01:", mapping=I03_ZEBRA_MAPPING) yield zebra await zebra.pc.arm.set(ArmDemand.DISARM) diff --git a/tests/common/beamlines/test_beamline_utils.py b/tests/common/beamlines/test_beamline_utils.py index 5478a2aa72..a2c8dfaf59 100644 --- a/tests/common/beamlines/test_beamline_utils.py +++ b/tests/common/beamlines/test_beamline_utils.py @@ -16,7 +16,6 @@ from dodal.devices.focusing_mirror import FocusingMirror from dodal.devices.motors import XYZPositioner from dodal.devices.smargon import Smargon -from dodal.devices.zebra import Zebra from dodal.log import LOGGER from dodal.utils import DeviceInitializationController, make_all_devices @@ -40,7 +39,7 @@ def setup(): def test_instantiate_function_makes_supplied_device(): - device_types = [Zebra, XYZPositioner, Smargon] + device_types = [XYZPositioner, Smargon] for device in device_types: dev = beamline_utils.device_instantiation( device, device.__name__, "", False, True, None @@ -50,7 +49,7 @@ def test_instantiate_function_makes_supplied_device(): def test_instantiating_different_device_with_same_name(): dev1 = beamline_utils.device_instantiation( # noqa - Zebra, "device", "", False, True, None + XYZPositioner, "device", "", False, True, None ) with pytest.raises(TypeError): dev2 = beamline_utils.device_instantiation( @@ -76,11 +75,11 @@ def test_instantiate_v1_function_fake_makes_fake(): def test_instantiate_v2_function_fake_makes_fake(): RE() - fake_zeb: Zebra = beamline_utils.device_instantiation( - i03.Zebra, "zebra", "", True, True, None + fake_smargon: Smargon = beamline_utils.device_instantiation( + i03.Smargon, "smargon", "", True, True, None ) - assert isinstance(fake_zeb, StandardReadable) - assert fake_zeb.pc.arm.armed.source.startswith("mock+ca") + assert isinstance(fake_smargon, StandardReadable) + assert fake_smargon.omega.user_setpoint.source.startswith("mock+ca") def test_clear_devices(RE): diff --git a/tests/devices/unit_tests/test_shutter.py b/tests/devices/unit_tests/test_shutter.py index 1d6a0e0772..d8eafee18b 100644 --- a/tests/devices/unit_tests/test_shutter.py +++ b/tests/devices/unit_tests/test_shutter.py @@ -2,7 +2,7 @@ from ophyd_async.core import DeviceCollector from ophyd_async.testing import callback_on_mock_put, set_mock_value -from dodal.devices.zebra_controlled_shutter import ( +from dodal.devices.zebra.zebra_controlled_shutter import ( ZebraShutter, ZebraShutterControl, ZebraShutterState, diff --git a/tests/devices/unit_tests/test_zebra.py b/tests/devices/unit_tests/test_zebra.py index e5cf0e2629..95c5dfc0b2 100644 --- a/tests/devices/unit_tests/test_zebra.py +++ b/tests/devices/unit_tests/test_zebra.py @@ -4,7 +4,7 @@ from bluesky.run_engine import RunEngine from ophyd_async.testing import set_mock_value -from dodal.devices.zebra import ( +from dodal.devices.zebra.zebra import ( ArmDemand, ArmingDevice, ArmSource, diff --git a/tests/devices/unit_tests/test_zebra_constants_mapping.py b/tests/devices/unit_tests/test_zebra_constants_mapping.py new file mode 100644 index 0000000000..4020bbf651 --- /dev/null +++ b/tests/devices/unit_tests/test_zebra_constants_mapping.py @@ -0,0 +1,42 @@ +import pytest +from ophyd_async.core import DeviceCollector + +from dodal.beamlines.i03 import I03_ZEBRA_MAPPING +from dodal.devices.zebra.zebra import Zebra +from dodal.devices.zebra.zebra_constants_mapping import ( + UnmappedZebraException, + ZebraMapping, + ZebraTTLOutputs, +) + + +async def fake_zebra(zebra_mapping: ZebraMapping): + async with DeviceCollector(mock=True): + zebra = Zebra(mapping=zebra_mapping, name="", prefix="") + return zebra + + +async def test_exception_when_accessing_mapping_set_to_minus_1(): + mapping_no_output = ZebraMapping(outputs=ZebraTTLOutputs()) + with pytest.raises( + UnmappedZebraException, + match="'ZebraTTLOutputs.TTL_EIGER' was accessed but is set to -1. Please check the zebra mappings against the zebra's physical configuration", + ): + zebra = await fake_zebra(mapping_no_output) + zebra.mapping.outputs.TTL_EIGER # noqa: B018 + + +def test_exception_when_multiple_fields_set_to_same_integer(): + expected_error_dict = {"TTL_DETECTOR": 1, "TTL_PANDA": 1} + with pytest.raises( + ValueError, + match=f"must be mapped to a unique integer. Duplicate fields: {expected_error_dict}", + ): + ZebraMapping(outputs=ZebraTTLOutputs(TTL_DETECTOR=1, TTL_PANDA=1)) + + +async def test_validly_mapped_zebra_is_happy(): + zebra = await fake_zebra(zebra_mapping=I03_ZEBRA_MAPPING) + assert zebra.mapping.outputs.TTL_DETECTOR == 1 + assert zebra.mapping.sources.DISCONNECT == 0 + assert zebra.mapping.AND_GATE_FOR_AUTO_SHUTTER == 2