Start an ophyd-async OAV #815
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #815 +/- ##
==========================================
+ Coverage 95.03% 95.12% +0.09%
==========================================
Files 119 120 +1
Lines 4810 4901 +91
==========================================
+ Hits 4571 4662 +91
Misses 239 239 ☔ View full report in Codecov by Sentry. |
noemifrisina
requested review from
rtuck99,
DominicOram,
d-perl and
olliesilvester
There was a problem hiding this comment.
Great start, thanks. Some comments in code. I also wondered if it was a bit cleaner to treat everything as tuples, see https://github.com/DiamondLightSource/dodal/tree/716_ophyd_async_oav_suggestion. I think it makes the functions in OAV a bit cleaner but I'm not 100% sure I like the implicit assumption that 0 == x and 1 == y. What do you think?
Additionally, it would be good to have some extra tests more specific to oav_parameters that directly test OAVConfig rather than test it through the device.
src/dodal/beamlines/i24.py
Outdated
| StaticVisitPathProvider( | ||
| BL, | ||
| Path("/dls/i24/data/2024/cm37275-4/bluesky"), | ||
| # client=RemoteDirectoryServiceClient("http://i24-control:8088/api"), |
There was a problem hiding this comment.
Should: I think we can remove this as it defaults to it.
src/dodal/beamlines/i24.py
Outdated
| drv_suffix="CAM:", | ||
| hdf_suffix="HDF5:", |
There was a problem hiding this comment.
Should: I would have these just default in the device and not needed to be passed in, I don't see when we will change them
src/dodal/devices/oav/oav_async.py
Outdated
| "thst": epics_signal_r(str, f"{prefix}MP:SELECT.THST"), | ||
| "frst": epics_signal_r(str, f"{prefix}MP:SELECT.FRST"), | ||
| "fvst": epics_signal_r(str, f"{prefix}MP:SELECT.FVST"), | ||
| "sxst": epics_signal_r(str, f"{prefix}MP:SELECT.SXST"), |
There was a problem hiding this comment.
Should: We can get these dynamically with self.level.describe() e.g.
from dodal.devices.oav.oav_async import ZoomController
from bluesky.run_engine import RunEngine
from ophyd_async.core import DeviceCollector
import asyncio
RE = RunEngine()
with DeviceCollector():
zoom_controller = ZoomController("BL03I-EA-OAV-01:FZOOM:")
async def get_values():
description = await zoom_controller.level.describe()
print(description["zoom_controller-level"]["choices"])
asyncio.run(get_values())
src/dodal/devices/oav/oav_async.py
Outdated
|
|
||
| _bl_prefix = prefix.split("-")[0] | ||
| self.zoom_controller = ZoomController(f"{_bl_prefix}-EA-OAV-01:FZOOM:", name) | ||
| # TODO This will actually come from the MJPG but for now... |
There was a problem hiding this comment.
Should: Can we put this an issue please?
src/dodal/devices/oav/oav_async.py
Outdated
| prefix: str, | ||
| path_provider: PathProvider, | ||
| drv_suffix: str, | ||
| hdf_suffix: str, |
There was a problem hiding this comment.
hdf_suffix doesn't really make much sense for us, we never write these to hdf. Maybe this means we should be making it optional in the AravisDetector. I guess we should discuss.
| _zoom = str(_get_element_as_float(node, "level")) | ||
| _um_pix_x = _get_element_as_float(node, "micronsPerXPixel") | ||
| _um_pix_y = _get_element_as_float(node, "micronsPerYPixel") |
There was a problem hiding this comment.
Nit: _ is usually used to indicate this is a private variable but the fact it's scoped to the function means it has to be private so this feels redundant
src/dodal/devices/oav/oav_async.py
Outdated
| self.micronsPerXPixel = create_hardware_backed_soft_signal( | ||
| float, | ||
| lambda: self._get_microns_per_pixel("x"), | ||
| ) | ||
| self.micronsPerYPixel = create_hardware_backed_soft_signal( |
There was a problem hiding this comment.
Should: Can we make these camel case please?
src/dodal/devices/oav/oav_async.py
Outdated
| beam_y - vertical_pixels, | ||
| ) | ||
|
|
||
| async def connect( |
There was a problem hiding this comment.
Should: I think it might be ok to do all this on init rather than connect
It does look tidier, but I don't really like the assumption either. I guess we could use an IntEnum just to make it a little less implicit? |
Starts the work for #716
Creates a first version of an ophyd-async OAV, for the moment including only the zoom controller and the code to get the microns per pixels and beam position out of the parameter files.
Instructions to reviewer on how to test:
Checks for reviewer
dodal connect ${BEAMLINE}