FooDetector should have a consistent initialiser pattern.

[DiamondJoseph]

FooDetector is an implementation of StandardDetector, with a FooController (which takes a FooDriver) and a HDFWriter.

class FooDetector(StandardDetector):
    """A Foo StandardDetector writing HDF files"""

    _controller: FooController
    _writer: HDFWriter

    def __init__(<SOME ARGS>):
        self.drv = driver  # Must be child of device to be staged
        self.hdf = hdf  # Must be child of device to be staged

        super().__init__(
            FooController(self.drv),
            HDFWriter(
                self.hdf,
                directory_provider,
                lambda: self.name,
                ADBaseShapeProvider(self.drv),
                **scalar_sigs,
            ),
            config_sigs=config_sigs or (self.drv.acquire_time,),
            name=name,
        )

Each facility has a particular standard (which may be kept to varying degrees of rigour) for how the child component (.drv, .hdf) epics addresses extend the prefix of their parent device.

Diamond also has the particular limitation that our device factory functions (which primarily are init methods, but do not necessarily need to be) must have the args prefix and name.

[DiamondJoseph]

The current Diamond wrapper function looks like this, with the complexity that some StandardDetector implementations (The Aravis that are used as OAVs, but not those used on the test rigs) have CAM instead of DRV.

def DLSFoo(prefix: str, name: str) -> FooDetector:
    driver = FooDriver(prefix + "DRV:")
    hdf = HDFWriter(prefix + "HDF5:")
    return FooDetector(driver=driver, hdf=hdf)

[jwlodek]

I'll mention what I had in the other PR, but at NSLS2 we use cam1: and HDF1: 99% of the time. If diamond uses a different standard, my suggestion would be to have some configurable global that holds this info, that we can then use as a default with a kwarg.

Basically:

from ophyd_async.epics.areadetector import DRV_SUFFIX, HDF_SUFFIX

class FooDetector:
    def __init__(self, prefix: str, name: str, drv_suffix=DRV_SUFFIX, hdf_suffix=HDF_SUFFIX):
         ....

With the globals in NSLS2's case being cam1: and HDF1:. Then, ideally, instantiating a detector can be done with just a base prefix and some kind of name.

[coretl]

DLS has no such standards unfortunately...

@DiamondJoseph what do you think about?

def DLSFoo(prefix: str, name: str) -> FooDetector:
    return FooDetector(prefix=prefix, drv_suffix="DRV:", hdf_suffix="HDF:")

In which case we could default to the NSLS-II versions:

class FooDetector:
    def __init__(self, prefix: str, drv_suffix=":cam1", hdf_suffix=":HDF1", name=""):
         ....

Also I would like to make all names optional, defaulting to "" so they can be used with DeviceCollector...

[DiamondJoseph]

That patttern works for me, happy with those defaults (good argument to push for standardisation later)

[callumforrester]

I'm happy with @coretl's solution too.

In general when porting DLS stuff we have been trying to adopt PV naming standards and persuade the controls team to change PV names. @DominicOram has had some success in this and I think it's a good strategy going forward, so I think we are allowed to assume we follow a standard. That said the use of globals makes it more difficult to cater for the 1% that do not.

I also think that detectors should look as much like other ophyd devices as possible, which the proposed solution satisfies because name and prefix and the only things you must pass.

[DiamondJoseph]

I'll put together a change to standardise the detectors we have so far, something for the docs too

[coretl]

In general when porting DLS stuff we have been trying to adopt PV naming standards and persuade the controls team to change PV names. @DominicOram has had some success in this and I think it's a good strategy going forward, so I think we are allowed to assume we follow a standard.

Although we aren't going to make this PV name change as a pre-requisite of adopting ophyd, we must be able to run alongside Malcolm so we will need to do FooDetector(prefix=prefix, drv_suffix="DRV:", hdf_suffix="HDF:") for some considerable amount of time.

Also the NSLS-II conventions are technically against our PV naming conventions, which are all caps, but then all areaDetector PVs are anyway so I'm not sure I care...

That said the use of globals makes it more difficult to cater for the 1% that do not.

My suggestion ditched the globals, you get NSLS-II naming by default, and you have to specify if different. No DLS defaults here...

I also think that detectors should look as much like other ophyd devices as possible, which the proposed solution satisfies because name and prefix and the only things you must pass.

I see no reason to make name required. We have DeviceCollector and soon https://github.com/DiamondLightSource/dodal/issues/483, both of which call set_name after __init__, so I really don't want name to become required anywhere...

[coretl]

Also, just to clarify, I wasn't suggesting we write DLSFoo anywhere, but actually:

@device_factory
def saxs() -> PilatusDetector:
    return PilatusDetector("BL22I-EA-PILAT-01:", drv="DRV:", hdf="HDF:")

@device_factory
def waxs() -> PilatusDetector:
    return PilatusDetector("BL22I-EA-PILAT-02:", drv="DRV:", hdf="HDF:")

Not as DRY as it could be, but at least it's explicit

[callumforrester]

Why is this required for compatibility with Malcolm?

[coretl]

Why is this required for compatibility with Malcolm?

So we can gradually migrate scans over. Only change one thing at a time.