Skip to content

Add dropdowns for EPICS mbb records #40

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Sep 24, 2024
Merged

Add dropdowns for EPICS mbb records #40

merged 4 commits into from
Sep 24, 2024

Conversation

jsouter
Copy link
Contributor

@jsouter jsouter commented Jun 6, 2024

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.

@jsouter jsouter requested review from GDYendell and MJGaughran June 6, 2024 09:52
Copy link

codecov bot commented Jun 6, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

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

Additional details and impacted files
@@            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     

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

@jsouter jsouter marked this pull request as draft June 12, 2024 12:39
@jsouter jsouter force-pushed the dropdowns branch 2 times, most recently from 48ee34d to e324d66 Compare June 12, 2024 14:18
@GDYendell
Copy link
Contributor

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
Copy link
Contributor Author

jsouter commented Jun 19, 2024

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
Copy link
Contributor Author

jsouter commented Jun 19, 2024

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 jsouter force-pushed the dropdowns branch 2 times, most recently from 3fc0a70 to 0a67b87 Compare June 19, 2024 15:05
@GDYendell GDYendell force-pushed the dropdowns branch 3 times, most recently from 0cfbf5b to 9ceba1e Compare September 23, 2024 13:49
@jsouter
Copy link
Contributor Author

jsouter commented Sep 23, 2024

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

@GDYendell GDYendell marked this pull request as ready for review September 23, 2024 14:50
@jsouter
Copy link
Contributor Author

jsouter commented Sep 24, 2024

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
Copy link
Contributor

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.

jsouter and others added 3 commits September 24, 2024 10:37
Add check for enum attributes and perform conversions in pythonSoftIOC
wrapper functions.

Co-authored-by: Gary Yendell <[email protected]>
Add some type ignores for bugs in mypy. These will be removed when moved
to pyright.
@GDYendell
Copy link
Contributor

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.

@GDYendell GDYendell merged commit 751233b into main Sep 24, 2024
17 checks passed
@GDYendell GDYendell deleted the dropdowns branch September 24, 2024 12:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants