CA enums with more than 16 values backed by string record

[jsouter]

Closes #150

May need to add test? Note that I have tested this change against a WIP version of fastcs-eiger running against slightly reworked branch of FastCS #134.

Records with the Enum datatype whose enum_cls have more than 16 members now use longStringIn/longStringOut records instead of longIn/longOut, this allows us to still have labelled dropdowns and string readbacks

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.83%. Comparing base (2c546da) to head (b32d867).
Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #151      +/-   ##
==========================================
+ Coverage   90.63%   90.83%   +0.20%     
==========================================
  Files          41       41              
  Lines        1943     1954      +11     
==========================================
+ Hits         1761     1775      +14     
+ Misses        182      179       -3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[GDYendell]

Thanks for spotting this. I guess this was broken in #102

[GDYendell]

Looks good. Happy to merge when the coverage is bumped up.

Could you add three parameterized tests for cast_from_epics_type, cast_to_epics_type, builder_callable_from_attribute to cover all the branches?

[jsouter]

Aha, so this of course only works when we are using a string enum, which is what I had in my fastcs-eiger branch. I guess we need to decide if we handle several different types of enum based on the underlying type or if we restrict ourselves.
Currently FastCS expects simple Enums with ints as the underlying type. We could require, or support alongside, string type enums, this would allow us to have labels with spaces and other characters as per #149 - we would use the Enum member's value when constructing guis. For int enums, the labels are obtained from the Enum member's name.

We may also want to check that the enum_cls is valid when it passed to fastcs.datatypes.Enum. I don't know if it's best to require that the enum_cls inherits from one or either of enum.StrEnum or enum.IntEnum, check that it inherits just from enum.Enum and all the members are of the same type, or we could provide a subclass of enum.Enum inside FastCS that implements what we need and gets imported downstream, and we check that enum_cls is an instance of that.

edit: Seeing from example_p4p_ioc.py that Eva intended the enums to be able to have arbitrary values at least for p4p, so maybe this restriction needs more thought.

[jsouter]

Proposal: something like

class Enum(Generic[T_Enum], DataType[T_Enum]):
    enum_cls: type[T_Enum]

    def __post_init__(self):
        if not issubclass(self.enum_cls, enum.Enum):
            raise ValueError("Enum class has to take an Enum.")

    def index_of(self, value: T_Enum) -> int:
        return self.members.index(value)

    def label_of(self, value: T_Enum):
        if self._is_string:
            return value.value
        else:
            return value.name

    @cached_property
    def members(self) -> list[T_Enum]:
        return list(self.enum_cls)

    @cached_property
    def _is_string(self):
        return all(isinstance(member.value, str) for member in self.members)

    @cached_property
    def labels(self) -> list[str]:
        return [self.label_of(member) for member in self.members]

    @property
    def dtype(self) -> type[T_Enum]:
        return self.enum_cls

    @property
    def initial_value(self) -> T_Enum:
        return self.members[0]

Then when we populate the metadata/give choices in the dropdowns we use the labels, which are the string values if all members of the enum have str values, otherwise the names of the members.

[evvaaaa]

Seeing from example_p4p_ioc.py that Eva intended the enums to be able to have arbitrary values at least for p4p, so maybe this restriction needs more thought.

For both p4p and softioc, even if the .value in the enum_cls is an string/integer, we still use the string .name / the index of the .name in the list(enum_cls) when writing from the client.

My suggestion would be that the string values you would put in the case of either an mbbOut, or stringOut (for options n>16), are the names of the elements.

I would suggest in the case of n>16, you do a logging.debug and state that only the string names can be used in the record, not integer indices.

[jsouter]

For both p4p and softioc, even if the .value in the enum_cls is an string/integer, we still use the string .name / the index of the .name in the list(enum_cls) when writing from the client.

Right okay, so the .value only has meaning downstream. I still think we need some mechanism to specify more flexible string labels for the dropdowns, whether or not it affects the underlying epics value.

edit: I suppose Enum names can be arbitrary strings if we construct the Enum from a dict, though I don't love it.

[GDYendell]

I think it makes sense to have the field name as the descriptive name and the value as the value to send to the device, so that it is consistent between StrEnum and IntEnum. It is annoying that wouldn't allow strings with spaces and via a dict is a bit horrible. Maybe we could add a helper for constructing these?

[evvaaaa]

Maybe we could add a helper for constructing these?

I think the clearest thing would be to just use a normal python enum and access it with standard []:

Foo = Enum("Foo", {"One option": 1, "Second option": "2"})

print(Foo["One option"], Foo["One option"].value)
print(Foo["Second option"], Foo["Second option"].value)

Foo.One option 1
Foo.Second option 2

Okay, ich bin aus 🫡

[jsouter]

Maybe we could add a helper for constructing these?

I think the clearest thing would be to just use a normal python enum and access it with standard []:

Foo = Enum("Foo", {"One option": 1, "Second option": "2"})

print(Foo["One option"], Foo["One option"].value)
print(Foo["Second option"], Foo["Second option"].value)

Foo.One option 1
Foo.Second option 2

Okay, ich bin aus 🫡

Tschüss. I think this is okay too, thinking about it, it comes quite naturally to create an Enum class this way especially if we're doing it dynamically, that's how I was intending to do it with fastcs-eiger anyway. It would actually be extra work to exclude this as an option.

We can always add a helper function later if we need it for explicit/statically defined Attributes, though I can't think of an interface immediately that would be much better.

[evvaaaa]

Also, as part of this you might want to add an on_update, before=True which rejects the put on the longStringOut if it isn't a string name in the enum.

Without this, you would put "Some value not in the record enum.Enum" to the record, it would be accepted by softioc, rejected by the Attribute.

[jsouter]

Also, as part of this you might want to add an on_update, before=True which rejects the put on the longStringOut if it isn't a string name in the enum.

Without this, you would put "Some value not in the record enum.Enum" to the record, it would be accepted by softioc, rejected by the Attribute.

Yep, that's a good idea.

edit: will add test + fix pyright but works with the 'validate' metadata arg.

[GDYendell]

This is OK for now, but I am slightly concerned that this mechanism of passing callables and kwargs and boolean flags around to make record instantiation generic and DRY is becoming unreadable.

I think this is probably ready to merge once the CI is working?