-
Notifications
You must be signed in to change notification settings - Fork 28
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Move ADAravis detectors to ophyd-async #190
Conversation
adcf3bf
to
d5801ec
Compare
@DiamondJoseph @coretl I'm not overjoyed at the separation of detector components across libraries. The fact that I have to have the writer/controller over in ophyd-async but assemble them in dodal is a bit cumbersome, especially if I want to update my code, I may have to do two PRs and an alpha release etc. One thing that I think would help is to have an example assembly of these components in ophyd-async. In other words a more generic version of To be clear, that is as well as rather than instead of. What do you think? |
That sounds like a good idea, the only reason I don't want to put it in ophyd-async in the first place is because we don't want to put anything that relies on our naming conventions there. Actually we could extend that idea even further, and make the class HDFAravis(StandardDetector):
def __init__(
self,
prefix: str,
driver_suffix: str,
hdf_suffix: str,
...
):
self.drv = ADAravis(prefix + driver_suffix)
self.hdf = NDFileHDF(prefix + hdf_suffix)
super().__init__(
controller=ADAravisController(self.drv, ...),
writer=HDFWriter(self.hdf, ...),
...
) Then in dodal: def oav():
return HDFAravis("BL22I-EA-DET-01:", driver_suffix="DRV:", hdf_suffix="HDF:") More typing, but clearer how the PV names are constructed... What do you reckon? |
At that point why not just class HDFAravis(StandardDetector):
def __init__(
self,
driver: ADAravisDriver,
hdf: NDFileHDF,
...
):
self.drv = driver
self.hdf = hdf
super().__init__(
controller=ADAravisController(driver),
writer=HDFWriter(hdf, ...),
...
) I'm sure someone is creating a Driver or Writer that isn't on the same prefix as the rest of the device |
Hmm, so then dodal would have: def oav():
prefix = "BL22I-EA-DET-01:"
return HDFAravis(ADAravis(prefix + "DRV:"), NDFileHDF(prefix + "HDF:")) I guess that extends to extra plugins reasonably nicely too... |
device_instantiation still requires prefix, name as arguments, but with those passed too |
34c60a9
to
1f5ed93
Compare
tests/epics/demo/test_demo.py::test_mover_moving_well seems to be reliably slower than expected on Python 3.10... |
3a9ea0c
to
bc2846d
Compare
76076a6
to
84aa40a
Compare
|
bd3da38
to
d1c4487
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Having it merged allows us make a pre-release and test before commuting to a full release so I'm favouring ship and iterate here.
As discussed with @coretl, standard detectors should exist in ophyd-async instead of dodal.
Implementation of ADAravis OAV for i22, p38 at Diamond.
Fixes DiamondLightSource/dodal#409 (after a release of ophyd-async)