`StandardReadable`: support declarative subdevices

[evvaaaa]

The declarative approach does not currently supports sub devices:

The below gives the following error:

class SomeSubDevice(StandardReadable, EpicsDevice):
    signal_r_1: A[SignalR[int], Fmt.HINTED_SIGNAL]
    signal_r_2: A[SignalR[Array1D[np.float64]], Fmt.CHILD]
    config_signal: A[SignalR[Array1D[np.float64]], Fmt.CONFIG_SIGNAL]
    enum_signal: A[SignalR[Sequence[SomeEnum]], Fmt.CHILD]


class SomeDevice(StandardReadable, EpicsDevice):
    signal_r_1: A[SignalR[int], Fmt.CHILD]
    signal_r_2: A[SignalR[Array1D[np.float64]], Fmt.UNCACHED_SIGNAL]
    signal_rw_1: A[SignalRW[bool], Fmt.CHILD]
    signal_rw_1: A[SignalRW[bool], Fmt.CHILD]
    sub_device_1: A[SomeSubDevice, Fmt.CHILD]
    sub_device_2: SomeSubDevice

  File "/home/a5497/Code/bluesky_nexus/venv/lib/python3.12/site-packages/ophyd_async/epics/core/_epics_connector.py", line 70, in create_children_from_annotations
    list(self.filler.create_devices_from_annotations())
  File "/home/a5497/Code/bluesky_nexus/venv/lib/python3.12/site-packages/ophyd_async/core/_device_filler.py", line 231, in create_devices_from_annotations
    device = child_type(connector=connector)
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
TypeError: EpicsDevice.__init__() got an unexpected keyword argument 'connector'

because all ophyd-async device annotations (whether explicitly given a StandardReadableFormat or not) are assumed to be signals:

A Device is added here:

ophyd-async/src/ophyd_async/core/_device_filler.py

Lines 166 to 167 in f96a9fa

	else:
	self._uncreated_devices[name] = origin

and then attempted to be connected to as if it were a signal here:

ophyd-async/src/ophyd_async/core/_device_filler.py

Line 231 in f96a9fa

device = child_type(connector=connector)

Suggestion

1 (Preferred)

We allow these devices to be connected to as if they were signals. The subdevice can only be specified declaritavely if it takes default values for this kind of device, in this case the PvSuffix for prefix and name from the annotation key.

2

We add logic here so that only signals, or DeviceVector of signals are connected to in this way.

[coretl]

I think the fix here is to support both prefix and connector in EpicsDevice.__init__ so that we can pass either and it will work. I will roll this into a similar issue making FastCSDevice at the same time.

[evvaaaa]

I think the fix here is to support both prefix and connector in EpicsDevice.init

That's an elegant solution! Hope everyone in the office is doing well 🙂

[coretl]

@oliwenmandiamond suggested it would be useful if we could replace assemblies like:

class XYStage(Stage):
    """Two-axis stage with an x and a y motor."""

    def __init__(
        self, prefix: str, name: str = "", x_infix: str = _X, y_infix: str = _Y
    ):
        with self.add_children_as_readables():
            self.x = Motor(prefix + x_infix)
            self.y = Motor(prefix + y_infix)
        super().__init__(name=name)

with

class XYStage(EpicsDevice):
    """Two-axis stage with an x and a y motor."""

    x: Annotated[Motor, PvSuffix("X")]
    y: Annotated[Motor, PvSuffix("Y")]

Then to allow the overrides we add a new parameter to EpicsDevice: pv_suffix_overrides: dict[str, str] (name up for grabs), which will use these overrides instead of the given PvSuffix for the attribute

[evvaaaa]

pv_suffix_overrides: dict[str, str] (name up for grabs), which will use these overrides instead of the given PvSuffix for the attribute

Hmmmm I'm not sure I like this...

Doesn't it kind of go against the declarative philosophy to define one suffix on the class level and then overwrite it at instantiation time?

I suppose we could have PvSuffix(at_instantation=True) (or a shorter variable name) with

class PvSuffix:
    pvs: tuple[str, ...]
    """Either a single pv, where default `_RBV` will be appended in the case of SignalR
    or a tuple with read suffix, write suffix.
    """
    at_instantiation = False
    """If true then the pv suffix will be passed in at as an argument to the __init__.

    If true the no `pvs` can be provided in the decorator.
    """

    def __init__(self, *pvs: str, at_instantiation: bool = False):
        self.pvs = pvs
        self.at_instantiation = at_instantiation

        if at_instantiation and pvs:
            raise ValueError(
                "Cannot define a declarative signal with a static prefix and also allow "
                "it to be passed in at at instantiation time."
            )


class Foo(StandardReadable, EpicsDevice):
    a: Annotated[SignalRW[int], PvSuffix("Foo")]  # Same as `b`
    b: Annotated[SignalRW[int], PvSuffix("Foo", "Foo_RBV")]
    c: Annotated[SignalRW[int], PvSuffix("SetBar", "RbkBar")]
    d: Annotated[SignalRW[int], PvSuffix(at_instantiation=True)]

What would the use case for this be? ... but also it's your package 🙇‍♀️

[coretl]

The use case is for when 2 beamlines can't agree on what to call their motors...

For the XYStage above, it means you can strongly encourage them to call their motors PREFIX:X and PREFIX:Y because then they can instantiate XYStage("PREFIX:"), but if they insist on PREFIX:XStage and PREFIX:YStage then they can instantiate the messier XYStage("PREFIX:", pv_suffix_overrides={"x": "XStage", "y": "YStage"}) without making a new class.

We could specify this just for assemblies of motors as that's the main use case, but I thought putting it into EpicsDevice would open up other possibilities...

Also, you made me remember that PvSuffix specifies the readback value too, so maybe it should be dict[str, str | PvSuffix].

Maybe this needs a bit more thought...

[evvaaaa]

without making a new class.

Ah I see. The solution I'm proposing at HZB would be an ABC device with all the device logic and the annotations without suffixes, and then forcing beamlines to make their own class defining PVs, but for us differing motor standards are here to stay. I can see the override making sense in your case 👍

[jwlodek]

In the case you're describing, wouldn't it be easier/more readable if the beamline that doesn't want to follow convention to make their own XYStage with the PvSuffix settings matching their naming convention? Or even an ABC with all the logic and beamlines make implementations w/ the suffixes that match their beamline (and maybe have one implementation that includes the facility naming standard suffixes centrally)?

Edit: It seems @evvaaaa had the same idea

[oliwenmandiamond]

The motivation behind my suggestion is our current approach at DLS

class XYZPolarAzimuthStage(XYZPolarStage):
    """Five-axis stage with a standard xyz stage and two axis of rotation: polar and
    azimuth.
    """

    def __init__(
        self,
        prefix: str,
        name: str = "",
        x_infix: str = _X,
        y_infix: str = _Y,
        z_infix: str = _Z,
        polar_infix: str = _POLAR,
        azimuth_infix: str = _AZIMUTH,
    ):
        with self.add_children_as_readables():
            self.azimuth = Motor(prefix + azimuth_infix)
        super().__init__(prefix, name, x_infix, y_infix, z_infix, polar_infix)

vs my prototype https://github.com/DiamondLightSource/dodal/pull/1920/changes#diff-74fbe22bab8aa883c7fef330d15167b9f622f350aa612847d7a67b9c3df05541

class XYZPolarAzimuthStage(XYZPolarStage):
    """Five-axis stage with a standard xyz stage and two axis of rotation: polar and
    azimuth.
    """
    azimuth: Annotated[Motor, DefaultInfix("AZIMUTH")]

The current approach has way too much boilerplate for such simple motor groups, especially when we have to do this many, many times for different amounts of combinations of motors for stages, mirrors, etc. We also ideally would want the motor PV's to be flexible because there is always an exception to the rule for a beamline and the PV won't match for some motors.

My thinking was to use kwargs if the user wants to override:

stage = XYZPolarAzimuthStage("BL-STAGE:", azimuth="CUSTOM_AZIMUTH")

The benefits are:

• Much less boilerplate and simpler
• More flexibility, classes can be much easier shared across facilities.

Cons:

• Lose static type checking for overriding device PVs.

I personally think the benefits outweigh the cons. The cons can be mitigated with strong runtime checking and strong test coverage which would instantly pick up any mistakes e.g

XYStage("TEST:", z="MYZ")
...
root ➜ /workspaces/dodal (Reduce_motor_stage_boilerplate ✗) 
$ /cache/venv-for/scratch/bluesky_development/dodal/bin/python /workspaces/dodal/src/dodal/devices/motors.py
Traceback (most recent call last):
  File "/workspaces/dodal/src/dodal/devices/motors.py", line 248, in <module>
    XYStage("TEST:", z="MYZ")
  File "/workspaces/dodal/src/dodal/devices/motors.py", line 59, in __init__
    raise TypeError(
TypeError: XYStage got unexpected motor infix override(s): z. Valid motor names are: x, y

If this is to be implemented in EpicsDevice, then yes the args would be `str | PvSuffix'.

Yes the alternative is to to define more custom classes per beamline with the correct defaults, but I think it is overkill and forces more duplicate code for just overriding a pv

[evvaaaa]

had the same idea

My WIP example:

# Still on ophyd v1 but the concept is the same since suffixes are defined on the class
class AxisMotorA(PvPositioner): 
    # Has it's own implementation of set move, etc, matching standard `A`,
    # with suffixes defined on the class.

class AxisMotorB(PvPositioner):
    # Has it's own implementation of set move, etc, matching standard `B`,
    # with suffixes defined on the class.

# Defined centrally
T = TypeVar("T", bound=PvPositioner)

class SoftMono(ABC, Device, Generic[T]):
    def __init__(self, prefix: str, name: str = ""):
        bases = get_original_bases(type(self))
        if not bases or not issubclass(bases[0], PVPositioner):
            raise TypeError("Didn't pass pv positioner class to SoftPgm")
        positioner_cls = cast(type[PVPositioner], bases[0])
        self.positioner = positioner_cls(prefix=prefix, name=name)
        # Could also do `positioner_cls.__init__(self, ...)

    # Device Logic using move, set, etc

# Defined on the beamline
# If there is one troublesome IOC which has bizarre records not really matching any standard
# then a beamline can do their own PvPositioner.
class EmilMono(SoftMono[AxisMotorA]):
   ...

Yes the alternative is to to define more custom classes per beamline with the correct defaults, but I think it is overkill and forces more duplicate code for just overriding a pv

In our case the devices themselves can have different logic/signals too, so this is another case where our needs differ.