Make a concrete superset of types that Signal should allow

[callumforrester]

As a developer I would like to solidify the requirement for enum signals to also inherit from str so that I can have more confidence my enum signals will connect.

Possible Solution

We don't want to pollute all the backends in a duplicating way so the check should go in Signal (@coretl?)
For static type checking, only support certain data types for signals (via a Union)

Acceptance Criteria

• When a signal is created with an enum type, validate that it inherits from str as well
• When instantiating a device with an enum signal that doesn't inherit from str, an error is raised
• Tests to validate behavoir

[coretl]

One idea is:

• Make Signal take an optional datatype on __init__
• Have each SignalBackend take a union datatype classvar supported_datatypes
• When the backend is set on the Signal then check that the Signal datatype is contained in the union of the SignalBackend.supported_datatypes and fail with a suitable error message
• This should happen in mock mode against the real backend so we can validate types without connecting to the control system

[coretl]

@burkeds this would make the following changes to Signal and SignalBackend:

• Signal would take an optional datatype in __init__ and do some validation on it
• EpicsSignalBackend would no longer take datatype in __init__
• SignalBackend.connect now takes datatype instead

Would that be fine for the Tango backend?

[burkeds]

@coretl This shouldn't cause any major issues. I am in the process of doing some major refactoring of the tango backend so now would be a good time to implement a change like this.

[coretl]

@evalott100 when the PCOMP change is done, this is higher priority than the layout ticket

[coretl]

The array of types we should accept:

1. str, int, float, bool
2. str, Enum and SubsetEnum, which should mean we can drop the write_value conversion of the *EnumConverters
3. np.ndarray of np.bool_, np.[u]int[8,16,32,64], np.float[32,64], str
4. np.ndarray of record array of the above to support tables
5. pydantic.BaseModel

For the transport mechanisms:

• CA should accept 1, 2, half of 3
• PVA should accept 1, 2, 3, 4
• Soft should accept all

Depends on #442

[coretl]

Query on the usecase for BaseModels

[evalott100]

It occurs to me that we should also allow pathlib.Path as a backend datatype:

signal = Signal(WhateverSignalBackend(Path))
assert isinstance(await signal.get_value(), Path)
await signal.set("some/path") # fails explaining the correct datatype.

There's quite a lot of places across the codebase that we convert to string before setting the signal.

[callumforrester]

@evalott100 see #122

[evalott100]

Closing as PathProvider makes the above usecase less compelling. We can revisit after 1.0 if this is still needed

Hmmm I'll probably still look into it, but I can see this logic.

[evalott100]

One thought I had today after realising that it would be lovely to be able to represent SeqTable as a pydantic RootModel:

A new abstract class PvaTableAbstraction with methods

• convert_to_pva_table
• convert_from_pva_table

This would allow you to have pydantic models, dataclasses, etc. which subclass the above that could then be used as backend datatypes. The new backend converter for p4p will then check for this on read write.

[callumforrester]

Does the conversation literally go to/from a dict or does it also transpose the data?

[evalott100]

Does the conversation literally go to/from a dict or does it also transpose the data?

The user could choose whatever formatting for the object - it will have to end up as a dictionary though

SeqTable is a column-wise typed dict:

ophyd-async/src/ophyd_async/fastcs/panda/_block.py

Lines 52 to 53 in e4fb849

	class SeqBlock(Device):
	table: SignalRW[SeqTable]

ophyd-async/src/ophyd_async/fastcs/panda/_table.py

Line 58 in e4fb849

class SeqTable(TypedDict):

We could then do something like:

class SeqTable(RootModel, PvaTableAbstraction):
    root: NpNDArray

    def convert_to_pva_table(self) -> Dict[str, NpNDArray]:
        """Convert root to the column-wise dict representation for backend put"""
    
    @classmethod
    def convert_from_pva_table(cls, pva_table: Dict[str, NpNDArray]):
        """For use in the backend."""
        ... # formatting
        return cls(formatted_rows)

    @model_validator(mode="before")
    @classmethod
    def check_valid_rows(cls, rows: Iterable):
        if not (0 <= len(rows) < 4096):
            raise ValueError(f"Length {len(rows)} not in range.")

        if not all(isinstance(row, np.ndarray) for row in rows):
            for row in rows:
                if not isinstance(row, np.void):
                    raise ValueError(
                        f"Cannot construct a SeqTable, some rows {row} are not arrays {type(row)}."
                    )
            raise ValueError("Cannot construct a SeqTable, some rows are not arrays.")
        if not all(row.shape == _SEQ_TABLE_ROW_SHAPE for row in rows):
            raise ValueError(
                "Cannot construct a SeqTable, some rows have incorrect shapes."
            )
        if not all(row.dtype is SeqTableRowType for row in rows):
            raise ValueError(
                "Cannot construct a SeqTable, some rows have incorrect types."
            )

        return np.array(rows)

Very rough draft. The general concept I'm going for is that we can represent the single signal pva dictionary as a much higher level class, and equip it with a way of converting to and from the dictionary only on write read.

You could also include a public interface and keep the dictionary private within the class, then convert_to_pva_table would just return a copy of the private dictionary.

[callumforrester]

I agree, although I'm not sure whether it's the class' responsibility to covert itself to a valid PVA table or the signal backend's. I would lean towards the latter...

[evalott100]

I agree, although I'm not sure whether it's the class' responsibility to covert itself to a valid PVA table or the signal backend's. I would lean towards the latter...

This is so the backend can check:

# Before connection
if issubclass(datatype, PvaTableAbstraction):
    initial_value = datatype.convert_from_pva_table({})

# In PvaConverter, called on read
if issubclass(datatype, PvaTableAbstraction):
    return datatype.convert_from_pva_table(value)

Though actually it shouldn't be a class function. In the case of this SeqTable it should update and recalling the validator.

[evalott100]

* `Signal` would take an optional `datatype` in `__init__` and do some validation on it

* `EpicsSignalBackend` would no longer take `datatype` in `__init__`

* `SignalBackend.connect` now takes `datatype` instead

This will require make_converter to exist in the signal backend, since it will be called from the connect of a signal backend (where we have the datatype) but MockSignalBackend won't have access to it unless we make a mapping of backend type to converter. I think we could just make make_converter a class method withing SignalBackend?

[coretl]

Query on the usecase for BaseModels

Downstream support (e.g. Tiled) for arbitrary BaseModels requires more use cases before we do this, so decided to support:

• str, int, float, bool
• str, Enum and SubsetEnum, which should mean we can drop the write_value conversion of the *EnumConverters
• np.ndarray of np.bool_, np.[u]int[8,16,32,64], np.float[32,64], str
• Sequence[str] and Sequence[Enum]
• class Table(BaseModel) which only accepts np.ndarray or Sequence of the above types, has validator to check lengths match, and a classmethod to return the numpy dtype of a row of the table

Support for arbitrary BaseModels is deferred until we have more use cases