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

ENH: Monitor the DISP field of records connected to by PyDMWritableWidget #1011

Merged
merged 7 commits into from
Jul 19, 2023

Conversation

jbellister-slac
Copy link
Collaborator

This fixes #932.

I still need to add docstrings and a new test case, but just want to get a quick check that this solutions looks reasonable before proceeding.

This sets up an additional monitor on channel_name.DISP for any writable widget so that the widget can be disabled with an appropriate tooltip, similar to the way access security is currently handled.

If DISP is not set, it will timeout in a thread without performance impact, I checked with several displays with write widgets and did not see adverse performance issues.

if value is not None and self._disp_channel != f'{value}.DISP':
if self._disp_channel is not None:
self._disp_channel.disconnect()
self._disp_channel = f'{value}.DISP'
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few thoughts:

  1. An EPICS record-specific thing like this seems like it should only apply to certain protocols (CA, PVA)
  2. PV names may already include a field name: this could end up being PV:NAME.VAL.DISP, couldn't it?
  3. Similarly, PV names may include a channel filter as in e.g. PV:NAME.[3:5] - so stripping off the field/filter will be necessary. Fortunately both are guaranteed to be preceded by a ., so channel.split(".", 1)[0] if "." in channel else channel can be sufficient
  4. Python IOCs may not have fields at all depending on how they are configured. I assume if this doesn't connect it's just a no-op, right? So then we just waste a search for each PV of that type.
  5. I didn't realize disable put didn't manifest itself as showing "read-only" in cainfo - that's new and confusing to me. I assume that's where you started and this solution came about because of that?

@jbellister-slac
Copy link
Collaborator Author

Thanks for taking a look!

For 1, 2, and 3, agreed with all of them, I can update with all of that.

Python IOCs may not have fields at all depending on how they are configured. I assume if this doesn't connect it's just a no-op, right? So then we just waste a search for each PV of that type.

Yes it is just a wasted search, nothing else happens. I considered putting this behind something like a monitorDISP checkbox, but wasn't sure if there was enough benefit to that.

I didn't realize disable put didn't manifest itself as showing "read-only" in cainfo - that's new and confusing to me. I assume that's where you started and this solution came about because of that?

Yes, this was the very weird one to me too and why I was unsure of this solution. That is basically where I started. cainfo showed write access even with DISP set, and camonitor showed no changes when DISP was updated either way.

Then I tried with pyepics, caproto, and pyca and none of the 3 showed any indication of having write access disabled when DISP was set. And DISP also wasn't a field that could be returned with any of the various metadata requests you can ask for with a monitor. I asked around a bit at SLAC and nikea to check if I wasn't missing something obvious, and didn't seem to be.
So it seemed a separate monitor like this was the only way I could find of getting updates to the field, despite trying to find something better.

@jbellister-slac
Copy link
Collaborator Author

PV names may already include a field name: this could end up being PV:NAME.VAL.DISP, couldn't it?

Similarly, PV names may include a channel filter as in e.g. PV:NAME.[3:5] - so stripping off the field/filter will be necessary. Fortunately both are guaranteed to be preceded by a ., so channel.split(".", 1)[0] if "." in channel else channel can be sufficient

These cases are handled and tested for now.

An EPICS record-specific thing like this seems like it should only apply to certain protocols (CA, PVA)

Python IOCs may not have fields at all depending on how they are configured. I assume if this doesn't connect it's just a no-op, right? So then we just waste a search for each PV of that type.

After thinking on this further I figured it might be a good idea to add a checkbox for this, since it is actually many wasted name searches as it is a monitor and will keep checking to see if the PV is responding yet. It does have exponential backoff on how often it checks, but having pointless name searches continually go out that the user didn't necessarily ask for doesn't seem great.

So 83060e7 adds an option for the user to ask for the DISP field or not, which ensures this new channel will only be created when it is wanted, which covers the protocol case too. If anyone has another idea they like better I could change this back, but I think all the basic functionality should work now.

@jbellister-slac jbellister-slac changed the title WIP: Monitor the DISP field of records connected to by PyDMWritableWidget ENH: Monitor the DISP field of records connected to by PyDMWritableWidget Jul 19, 2023
Copy link
Collaborator

@YektaY YektaY left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This all looks good to me

@YektaY YektaY merged commit 29d6b5c into slaclab:master Jul 19, 2023
16 of 18 checks passed
@jbellister-slac jbellister-slac deleted the disp branch July 19, 2023 21:06
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.

Widgets not correctly indicating failed CA writes when DISP is active
3 participants