Adding driver and controller classes for Kinetix and Vimba cameras

[jwlodek]

Addresses #210

[coretl]

Please could we have some tests too?

A subset of https://github.com/bluesky/ophyd-async/blob/main/tests/epics/areadetector/test_aravis.py would be good

I note that the vimba driver should probably use the deadtime calculations from aravis, and the aravis driver should use the gated modes from vimba, but I'll make a separate issue for that

[coretl]

For areaDetector I've tended to name the attribute after the PV suffix, is there a reason to name this mode rather than readout_port_idx or readout_mode?

[jwlodek]

I had ReadoutMode originally, but changed it because I couldn't make the linter happy actually...

I didn't want to use ReadoutModeIdx because I think treating it as a ReadoutMode selection rather than an index in the list is better.

[coretl]

Could you point me to the corresponding record in the template please?

[jwlodek]

https://github.com/NSLS-II/ADKinetix/blob/ee1359dbb02d99e74bd442e228a760f4f9e069b7/kinetixApp/Db/ADKinetix.template#L116

[coretl]

Could these enum strings be pushed down to the template? Then having a ReadoutMode PV would make more sense...

I've tried not to add any translation in the "bucket of PVs" layer, and put the logic in the Controller, but I guess that is difficult here as the attribute isn't used in the controller.

Either way, I think readout_mode for the attribute and KinetixReadoutMode are reasonable names, is the linter happy with those?

[DiamondJoseph]

I don't know enough about AD DetectorStates, so I ignored this param for the Pilatus and Aravis- do we think it's important to expose? I can go back and adjust those detectors before there's any pickup.

[coretl]

I see no reason to have to override the good states when instantiating the Controller, I think we know up front what are the good states for a given detector.

@jwlodek any reason you added it as an init arg? If not then I recommend we:

Suggested change

good_states: Set[DetectorState] = set(DEFAULT_GOOD_STATES),

[jwlodek]

The only reason was that it was done this way for the Pilatus when I was writing these classes.

[DiamondJoseph]

For the Pilatus and the Aravis we've also created fooDetector types- partly because we didn't want to split the implementation of the device between two repositories- should we stick with that format (and have facility specific handling limited to calling the constructor fo the StandardDetector), or should the StandardDetectorImplementation be in facility specific code?

[coretl]

For the Pilatus and the Aravis we've also created fooDetector types- partly because we didn't want to split the implementation of the device between two repositories- should we stick with that format (and have facility specific handling limited to calling the constructor fo the StandardDetector), or should the StandardDetectorImplementation be in facility specific code?

Having thought about this a bit more, I think I would prefer this pattern:

class FooDetector(StandardDetector):
    def __init__(
        self,
        drv_prefix: str,
        hdf_prefix: str,
        ...,
        name: str = "",
    ):
        self.drv = ADFoo(drv_prefix)
        self.hdf = NDFileHDF(hdf_prefix)
        super().__init__(
            FooController(self.drv),
            HDFWriter(self.hdf, ...),
            ...,
            name=name
        )     

Then we instantiate as FooDetector(f"{prefix}DRV:", f"{prefix}HDF:") which makes no assumptions about PV naming convention, but does tie the detector to use the correct class of template class and controller class

@jwlodek @DiamondJoseph what do you think?

[jwlodek]

I would go a step further:

class FooDetector(StandardDetector):
    def __init__(
        self,
        prefix: str,                  # Use base prefix as only positional arg
        drv_prefix: str = 'cam1:',    # Add defaults for drv/hdf component after base prefix
        hdf_prefix: str = 'HDF1:',
        ...,
        name: str = "",
    ):
        self.drv = ADFoo(f'{prefix}{drv_prefix}')
        self.hdf = NDFileHDF(f'{prefix}{drv_prefix}')
        super().__init__(
            FooController(self.drv),
            HDFWriter(self.hdf, ...),
            ...,
            name=name
        )     

In 99% of cases at NSLS2 we leave the cam1: and HDF1: names as defaults, and so I'd like the default behavior for instantiation to be just

foo = FooDetector('XF:31ID1-ES{My-Det:1}')

for example.

If DLS use different defaults rather than cam1 or HDF1 we could even use some kind of global that can be pre-configured to use whichever defaults.

[jwlodek]

@coretl could we merge this PR in before we make further changes to the format of the various classes? I'd like to be able to close out this branch and re-sync with main.

[DiamondJoseph]

Pilatus is now using the default GOOD_STATES from the ADBaseDriver, I'd like to see that consistent until we find a detector that isn't just taking GOOD_STATES. The exact form of FooDetector we can continue to quibble about.

[DiamondJoseph]

eek pushed the big rebase button prematurely

[DiamondJoseph]

I've made a new issue from the "What form should StandardDetector.init take"

#263

[jwlodek]

Removed the states set kwarg.

[coretl]

I'd prefer to get the mode -> readout_mode change (or similar) in soon, but happy to merge this and do a separate PR later if that is better