949 make ophyd devices for the diagonstics for i10 #960

Relm-Arrowny · 2024-12-13T14:34:09Z

Fixes #949

Instructions to reviewer on how to test:

Pass test.
Check connection.

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}

…or-i10

codecov · 2024-12-13T14:42:59Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.53%. Comparing base (75f1019) to head (b2846f6).
Report is 7 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #960      +/-   ##
==========================================
+ Coverage   95.88%   97.53%   +1.65%     
==========================================
  Files         145      152       +7     
  Lines        6240     6417     +177     
==========================================
+ Hits         5983     6259     +276     
+ Misses        257      158      -99

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

…or-i10

callumforrester · 2025-01-13T15:14:51Z

src/dodal/devices/i10/diagnostics.py

+    """This is the standard webcam that can be found in ophyd_async
+    with four extra centroid signal. There is also a change in data_type due to it being
+    an older/different model"""


I would like @coretl's opinion on this approach, whether we should make a configurable "super detector" in ophyd-async or accept variations like these

This should be rolled into ophyd-async AravisDetector. Since bluesky/ophyd-async#606 this should now be possible. We pass stats as a plugin to the init. I'm unsure why the datatype change was needed.

DominicOram

Great, thanks! I've added a bunch of suggestions based on how I would do a similar device in MX but obviously I don't know your usecase so feel free to reject suggestions if they don't work for you. Do you have some example plans that use the devices? I was expecting to see some in https://github.com/DiamondLightSource/i10-bluesky but couldn't, maybe I'm just missing them.

src/dodal/beamlines/i10.py

src/dodal/devices/i10/diagnostics.py

DominicOram · 2025-01-24T10:55:39Z

src/dodal/devices/i10/slits.py

@@ -20,7 +26,38 @@ def __init__(self, prefix: str, name: str = "") -> None:
        )


+class BladeDrainCurrents(Device):


Should: I wasn't aware of the technique of using drain current to measure flux (in MX we just use direct diode measurements). Can you expand on the docstring here (and in I10SlitsDrainCurrent below) to explain why drain currents are useful? Ideally add a link to some further reading on the technique?

Just a very quick one, I will add the dostring later, The way we use drain current on mirror the absolute flux does not matter we use it to normalise the detector so it is always relative and it has the added advantage of you do not put anything in the beam path that is not needed. As for the slits/blade drain, we use it to correct correct the beam position(feedback). E.g. depending of the drain current values on the left and right blades we will move the mirror pitch to make sure it always in the right place.
I am not sure I would call it a technique, it is mostly photoelectric effect. In theory you can convert them into flux but there are a lot assumptions/corrections you have to make, for example the beam distribution, as we are not intercepting all of the beam, furthermore drain current also suffer form charging up and has a strong energy dependence, it will be a very big job to do all the corrections.

DominicOram · 2025-01-24T11:07:02Z

src/dodal/devices/i10/slits.py

@@ -20,7 +26,38 @@ def __init__(self, prefix: str, name: str = "") -> None:
        )


+class BladeDrainCurrents(Device):
+    """ "The drain current measurements on each blade."""


@DiamondLightSource/bluesky-reviewers could someone from core weigh in on this? If they agree I will add the statement to https://diamondlightsource.github.io/dodal/main/reference/device-standards.html

Could: Presumably there is a lookup table or equation that takes the drain current and converts it to flux? In MX we have tried to have the ophyd device interface be the things that are scientifically interesting, rather than hardware implementation details. So in this case I would put the conversion in the device itself and have soft signals that are flux. Even better, I would try and get this pushed into the EPICS layer, particularly if the look up table rarely changes.

The reason for having scientific terms in ophyd devices is that when I read() the device these values are what come out into documents and ultimately get put in the nexus file/LIMS. If we need to do the conversion to flux we now need to do this downstream in multiple places rather than here in one.

This is likely a big-ish job so for now (assuming a core person agrees) can you just make the issue to track it?

DominicOram · 2025-01-24T11:19:05Z

src/dodal/devices/i10/slits.py



-class I10Slits(Slits):
+class I10SlitsBlades(Slits):


Should: In MX we also have slits where we can address individual blades but so far we have never needed to. Do you definitely need to be able to? If yes then I think we should:

Move this to the general slits.py and rename to be generic

Standardise on a naming scheme (at least at the ophyd layer) - in MX we call the ring blade inboard and the hall blade outbound

This is certainly a new issue though, could you make one pleas, with an explanation of why you need individual blades?

DominicOram · 2025-01-24T11:47:18Z

src/dodal/devices/i10/diagnostics.py

+        self.positioner = DropDownStage(
+            prefix=prefix,
+            positioner_enum=positioner_enum,
+            positioner_suffix=positioner_suffix,
+            dropdown_pv_suffix=dropdown_pv_suffix,
+        )


Could:

Suggested change

self.positioner = DropDownStage(

prefix=prefix,

positioner_enum=positioner_enum,

positioner_suffix=positioner_suffix,

dropdown_pv_suffix=dropdown_pv_suffix,

)

self.position = epics_signal_rw(positioner_enum, prefix + positioner_suffix + dropdown_pv_suffix)

I think this might be a case where the extra layer makes things more complicated for the end user and a less readable plan i.e (and including the renames from above)

yield from bps.set(diagnostics.positioner.stage_drop_down, D3DropDown.NOTHING)

vs.

yield from bps.set(diagnostics.position, D3Position.NOTHING)

DominicOram · 2025-01-24T12:40:29Z

src/dodal/devices/i10/diagnostics.py

+class I10CentroidDetector(StandardReadable, Triggerable):
+    """Detector to read out the centroid position,
+    this is base off the SingleTriggerDetector in ophyd_async with added
+    readable default"""
+
+    def __init__(
+        self,
+        prefix: str,
+        name: str = "",
+    ) -> None:
+        self.drv = I10AravisDriverIO(prefix=prefix)
+        self.add_readables(
+            [self.drv.array_counter, self.drv.centroid_x, self.drv.centroid_y],
+            Format.HINTED_UNCACHED_SIGNAL,
+        )
+
+        self.add_readables([self.drv.acquire_time], Format.CONFIG_SIGNAL)
+
+        super().__init__(name=name)
+
+    @AsyncStatus.wrap
+    async def stage(self) -> None:
+        await asyncio.gather(
+            self.drv.image_mode.set(ImageMode.SINGLE),
+            self.drv.wait_for_plugins.set(True),
+        )
+        await super().stage()
+
+    @AsyncStatus.wrap
+    async def trigger(self) -> None:
+        await self.drv.acquire.set(True)


Should: I think you could just inherit from SingleTriggerDetector then? Something like:

Suggested change

class I10CentroidDetector(StandardReadable, Triggerable):

"""Detector to read out the centroid position,

this is base off the SingleTriggerDetector in ophyd_async with added

readable default"""

def __init__(

self,

prefix: str,

name: str = "",

) -> None:

self.drv = I10AravisDriverIO(prefix=prefix)

self.add_readables(

[self.drv.array_counter, self.drv.centroid_x, self.drv.centroid_y],

Format.HINTED_UNCACHED_SIGNAL,

)

self.add_readables([self.drv.acquire_time], Format.CONFIG_SIGNAL)

super().__init__(name=name)

@AsyncStatus.wrap

async def stage(self) -> None:

await asyncio.gather(

self.drv.image_mode.set(ImageMode.SINGLE),

self.drv.wait_for_plugins.set(True),

)

await super().stage()

@AsyncStatus.wrap

async def trigger(self) -> None:

await self.drv.acquire.set(True)

class I10CentroidDetector(SingleTriggerDetector):

"""Detector to read out the centroid position,

this is based off the SingleTriggerDetector in ophyd_async with added

readable defaults"""

def __init__(

self,

prefix: str,

name: str = "",

) -> None:

driver = I10AravisDriverIO(prefix=prefix)

super().__init__(driver, [driver.centroid_x, driver.centroid_y], name)

src/dodal/devices/i10/diagnostics.py

DominicOram · 2025-01-24T12:48:24Z

src/dodal/devices/i10/diagnostics.py

+        stage_write_enum: type[StrictEnum] = InOutTable,
+        stage_read_enum: type[StrictEnum] = InOutReadBackTable,
+        stage_read_suffix: str = "STA",
+        stage_write_suffix: str = "CON",


Should: I can't see anywhere in the code currently where we use different values for stage_write_enum, stage_read_enum, stage_read_suffix or stage_write_suffix than the defaults. I would just hardcode them in below until you have a case where they are different as it leads to lots of boilerplate

DominicOram · 2025-01-24T12:51:32Z

tests/devices/i10/test_diagnostics.py

+    return mock_centroid_detector
+
+
+async def test_I10CentroidDetector_with_blueSky(


Nit: My preference is to name tests very verbosely and explicitly so it's clear what you're testing. For example here I would name it something like test_when_I10CentroidDetector_is_counted_in_plan_then_emitted_document_contains_expected_values. If you have vague names I think it leads to the temptation to make one big test that tests loads of different stuff, that test then becomes long to run and hard to debug

Relm-Arrowny added 3 commits December 9, 2024 09:59

test

f01a357

Merge branch 'main' into 949-make-ophyd-devices-for-the-diagonstics-f…

d18fdcc

…or-i10

Added Rasor diagnostics

bdc36ff

Relm-Arrowny linked an issue Dec 13, 2024 that may be closed by this pull request

Make Ophyd devices for the Diagonstics for i10 #949

Open

2 tasks

Relm-Arrowny added 6 commits December 13, 2024 15:56

Correct pvs

dd9628f

added slits drain current

cf20221

Added drain current and diode detection for d5a

e3fb382

added test for the single shot webcam

f371e08

Merge branch 'main' into 949-make-ophyd-devices-for-the-diagonstics-f…

2c284f0

…or-i10

added docsting and clean up

94d141b

Relm-Arrowny marked this pull request as ready for review December 17, 2024 10:55

Relm-Arrowny requested a review from a team as a code owner December 17, 2024 10:55

Relm-Arrowny added 5 commits December 17, 2024 11:01

added missing typing

8bc02b6

change test name

cbe51b3

Merge branch 'main' into 949-make-ophyd-devices-for-the-diagonstics-f…

d56d674

…or-i10

fix typo

244fbec

Merge branch 'main' into 949-make-ophyd-devices-for-the-diagonstics-f…

3ed4a5e

…or-i10

callumforrester reviewed Jan 13, 2025

View reviewed changes

correct pv suffix

b2846f6

DominicOram requested changes Jan 24, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

949 make ophyd devices for the diagonstics for i10 #960

949 make ophyd devices for the diagonstics for i10 #960

Relm-Arrowny commented Dec 13, 2024 •

edited

Loading

codecov bot commented Dec 13, 2024 •

edited

Loading

callumforrester Jan 13, 2025

coretl Jan 13, 2025

DominicOram left a comment

DominicOram Jan 24, 2025

Relm-Arrowny Jan 24, 2025

DominicOram Jan 24, 2025

DominicOram Jan 24, 2025

DominicOram Jan 24, 2025

DominicOram Jan 24, 2025

DominicOram Jan 24, 2025

DominicOram Jan 24, 2025

		@@ -20,7 +26,38 @@ def __init__(self, prefix: str, name: str = "") -> None:
		)


		class BladeDrainCurrents(Device):

		return mock_centroid_detector


		async def test_I10CentroidDetector_with_blueSky(

949 make ophyd devices for the diagonstics for i10 #960

Are you sure you want to change the base?

949 make ophyd devices for the diagonstics for i10 #960

Conversation

Relm-Arrowny commented Dec 13, 2024 • edited Loading

Instructions to reviewer on how to test:

Checks for reviewer

codecov bot commented Dec 13, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DominicOram left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Relm-Arrowny commented Dec 13, 2024 •

edited

Loading

codecov bot commented Dec 13, 2024 •

edited

Loading