Skip to content
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

Add allowed values for creating combo box widgets #46

Merged
merged 2 commits into from
Jul 26, 2024
Merged

Conversation

GDYendell
Copy link
Contributor

No description provided.

@GDYendell GDYendell requested a review from jsouter July 24, 2024 14:40
Copy link

codecov bot commented Jul 24, 2024

Codecov Report

Attention: Patch coverage is 90.47619% with 2 lines in your changes missing coverage. Please review.

Project coverage is 62.14%. Comparing base (151f217) to head (68c1930).

Files Patch % Lines
src/fastcs/backends/epics/gui.py 87.50% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #46      +/-   ##
==========================================
+ Coverage   61.73%   62.14%   +0.41%     
==========================================
  Files          22       22              
  Lines         831      840       +9     
==========================================
+ Hits          513      522       +9     
  Misses        318      318              

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

@GDYendell
Copy link
Contributor Author

GDYendell commented Jul 24, 2024

@jsouter I think this is all that is required to display combo box widgets for enums. It doesn't use mbbi/o, but the effect in the UI is the same and we don't have to worry about converting ints. What do you think?

#40 seems to do more than this by supporting specific EPICS fields. This may require a bit more thought because I don't think we should use the EPICS record fields in the core code. Maybe we should some up with some more descriptive names that map on to EPICS fields and then we can use them for Tango too.

@jsouter
Copy link
Contributor

jsouter commented Jul 24, 2024

@jsouter I think this is all that is required to display combo box widgets for enums. It doesn't use mbbi/o, but the effect in the UI is the same and we don't have to worry about converting ints. What do you think?

#40 seems to do more that this by supporting specific EPICS fields. This may require a bit more thought because I don't think we should use the EPICS record fields in the core code. Maybe we should some up with some more descriptive names that map on to EPICS fields and then we can use them for Tango too.

Yeah I think we should figure out how to handle that, though this seems to work okay for string enums in my testing. As far as I know there's no way to have the dropdowns have a label that is different from the value that it sends to the record, (e.g. a string label on a dropdown that actually sends a integer value to the device), so finding some nice interface to assign ZRVL, ONVL, TWVL etc seems like a natural way to implement EPICS integer enums.

@GDYendell
Copy link
Contributor Author

As far as I know there's no way to have the dropdowns have a label that is different from the value that it sends to the record, (e.g. a string label on a dropdown that actually sends a integer value to the device)

Ah... Yeah I think this is a limitation in pvi currently. I am not sure we have added support for any device that requires this yet. Did you have a specific case that you started #40 for?

@jsouter
Copy link
Contributor

jsouter commented Jul 25, 2024

As far as I know there's no way to have the dropdowns have a label that is different from the value that it sends to the record, (e.g. a string label on a dropdown that actually sends a integer value to the device)

Ah... Yeah I think this is a limitation in pvi currently. I am not sure we have added support for any device that requires this yet. Did you have a specific case that you started #40 for?

It came up when trying to make odin-fastcs screens for the odin detectors from their ParameterTrees (tested with eiger and merlin), which generally expect integer values for the enums (e.g. trigger mode for eiger has the allowed_values [0, 1, 2, 3] which get mapped to ['ints', 'inte', 'exts', 'exte'] when passed to/from the detector by the odin adapter. Obviously there is another eiger detector interface for fastcs, but this pops up in a few places in all the odin detectors. Also in the current implementation I need to pass extra metadata from odin to provide a mapping between the allowed values and their string names otherwise all the dropdowns entries will just be non descriptive integers, but that's more of an odin discussion).

@GDYendell
Copy link
Contributor Author

OK that makes sense. This works pretty nicely for devices with enums that accept the descriptive name (eiger), so I am going to merge this for now and in future we can look at how to add descriptive names on top for devices that only accept the integer values.

For odin we should be able to accept the descriptive names everywhere, but currently it has been made to work with EPICS mbb records when we won't need to any more.

@GDYendell GDYendell merged commit db33d03 into main Jul 26, 2024
17 checks passed
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