DiamondLightSource / FastCS

Control system agnostic framework for building device support in Python for both EPICS and Tango
Apache License 2.0
1 stars 2 forks source link

Add dropdowns for EPICS mbb records #40

Closed jsouter closed 1 week ago

jsouter commented 4 months ago

Adds enum_mapping attribute for Attribute classes, which expect a mapping of str to int e.g. {"string value 0": 0, "string value 1": 1}, and uses these to populate the ZRST, ONST, TWST... fields of an MBBO if enum_mapping is not None.

This implementation may be too specific for the EPICS case however, not sure if Tango etc use enums in other ways.

codecov[bot] commented 4 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 82.78%. Comparing base (881a7cd) to head (dda9e45). Report is 5 commits behind head on main.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #40 +/- ## ========================================== + Coverage 81.24% 82.78% +1.54% ========================================== Files 22 23 +1 Lines 869 912 +43 ========================================== + Hits 706 755 +49 + Misses 163 157 -6 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

GDYendell commented 3 months ago

This works for APIs that expect to be sent integers for enum options, but we also want to display any parameter that has allowed_values (a limited set of valid values) as an mbb and a combo box. In this case it shouldn't need the mapping. Perhaps Attributes should (optionally) have allowed_values, and then for the case that those allowed values are just integers, it could also have an additional list of string descriptions to override the STA fields and combo box text.

jsouter commented 3 months ago

This works for APIs that expect to be sent integers for enum options, but we also want to display any parameter that has allowed_values (a limited set of valid values) as an mbb and a combo box. In this case it shouldn't need the mapping. Perhaps Attributes should (optionally) have allowed_values, and then for the case that those allowed values are just integers, it could also have an additional list of string descriptions to override the STA fields and combo box text.

Trying to think how best to do this. I guess it would be more generic to add a kwargs: Dict[str, Any] member to Attribute that could contain EPICS fields like ZRST, ONST, TWST that get passed into the Widget by _get_write_widget/_get_read_widget and in _get_input_record/_get_output_record as long as they match a list of allowed EPICS field names, and if ZRST, ONST.../ZRVL, ONVL... etc is found it populates a ComboBox/mbb, otherwise a long is used. Could then have the optional allowed_values attribute to handle the string case where we have more than 16 allowed values (as with the element parameter provided by Eiger/SIMPLON), or, and this is possibly uglier, it could be passed in as kwargs["allowed_values"] or kwargs["string_choices"].

jsouter commented 3 months ago

Also realised in my testing that some logic is needed to ensure that ZRST, ONST etc are not longer than 25 characters, and if shortened should be made unique, though this may be something that is better handled in the adapters/controllers themselves e.g. eiger_fastcs

jsouter commented 1 week ago

Changes look good to me, appears to work as expected against fastcs-eiger with the tickit sim

jsouter commented 1 week ago

Just had another look and found a bug, trying to pin it down. When running fastcs-eiger there's an error with the element attribute (which has more than 16 allowed string values), when attr.set is called with the response value (a string), the following gets raised

Update loop in EigerConfigHandler(name='detector/api/1.8.0/config/element', update_period=0.2) stopped:
AttributeError: 'int' object has no attribute 'encode

The error goes away if change the line in convert_if_enum to

if allowed_values is not None and len(allowed_values) <= MBB_MAX_CHOICES:

but maybe there's a better way to designate an Attribute as being an enum type.

GDYendell commented 1 week ago

Yeah I'm not happy with that either. The trouble is, it having 16 or fewer fields is an EPICS-specific limitation. So, I am not sure how to encode that as a first class property of Attributes in a generic way.

GDYendell commented 1 week ago

OK I have refactored the enum logic into more helper functions to make it less intrusive to the core logic and added some more tests to get the coverage up.