949 make ophyd devices for the diagonstics for i10 #960
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #960 +/- ##
==========================================
- Coverage 97.69% 97.68% -0.01%
==========================================
Files 160 161 +1
Lines 6635 6747 +112
==========================================
+ Hits 6482 6591 +109
- Misses 153 156 +3 ☔ View full report in Codecov by Sentry. |
| """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""" |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
The issue for datatype is without changing it I with get the follow error as datatype is strict enum drv: NotConnected: data_type: TypeError: BL10I-DI-PHDGN-01:DCAM:CAM:DataType_RBV has choices ('UInt8', 'UInt16'), but <enum 'ADBaseDataType'> requested ['Int8', 'UInt8', 'Int16', 'UInt16', 'Int32', 'UInt32', 'Int64', 'UInt64', 'Float32', 'Float64'] to be strictly equal to them.
There was a problem hiding this comment.
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/devices/i10/slits.py
Outdated
| @@ -20,7 +26,38 @@ def __init__(self, prefix: str, name: str = "") -> None: | |||
| ) | |||
|
|
|||
|
|
|||
| class BladeDrainCurrents(Device): | |||
| """ "The drain current measurements on each blade.""" | |||
There was a problem hiding this comment.
@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?
Fixes #949
Instructions to reviewer on how to test:
Checks for reviewer
dodal connect ${BEAMLINE}