ENH: make PVStateSignal report enum_strs and write_access metadata #1046
Conversation
| that everything is ready. Doing it earlier can run into some | ||
| race conditions. | ||
| """ | ||
| # Do some one-time setup here |
There was a problem hiding this comment.
I'm not 100% sure if this was the right place to do this
There was a problem hiding this comment.
The one-time setup in the first callback seems appropriate and efficient to me.
The only thing worth noting is that _calc_readback happens inside of an acquired RLock . I see nothing in the block that points to a potential deadlock condition though: running subscriptions happens in a separate thread and shouldn't block.
There was a problem hiding this comment.
Since it is in an RLock though I might take a moment to see if I can set it up earlier in the chain, both to avoid potential mishaps and to not set a bad example for future people
There was a problem hiding this comment.
Can't quickly find a better way, I'm going to move on
| ref_str = self.parent._state_logic_set_ref | ||
| if isinstance(ref_str, str): | ||
| setpoint_sig = getattr(self.parent, ref_str) | ||
| setpoint_sig.subscribe( |
There was a problem hiding this comment.
Should stash the subscription identifier and unsubscribe on destroy()
There was a problem hiding this comment.
I'll make sure to do this
There was a problem hiding this comment.
Apparently unsubscribe_all() is called on all signals during destroy() whether or not this is handled here.
https://github.com/bluesky/ophyd/blob/0e8acb3df3d17e0ab7aa2cc924831f4a0c580449/ophyd/device.py#L1090-L1093
https://github.com/bluesky/ophyd/blob/0e8acb3df3d17e0ab7aa2cc924831f4a0c580449/ophyd/ophydobj.py#L345-L347
|
Marking as draft again so I don't accidentally hit the green button before resolving the cleanup |
|
|
||
| _state_logic_set_ref : str or None | ||
| An optional reference to the component that will be used to set some | ||
| metadata on the state signal if provided. |
There was a problem hiding this comment.
So this is something that will have to be set on any future PVStatePositioner devices if we want the metadata to carry forward correctly. Is there somewhere we can broadcast that information? I don't see natural place in the docs for it yet.
I also can't imagine there's a good way to dynamically guess at the component name without the creator's help. Throwing an error in its absence might be a bit heavy handed, since it's only a gui-related issue.
There was a problem hiding this comment.
I've actually intentionally omitted it in the MovableStand class since the state there isn't settable
pcdsdevices/pcdsdevices/movablestand.py
Line 32 in 613893f
One thing I was considering was switching this to a required attribute, defaults to None, needs to become a list. So then we know that you didn't include it if it's still at None, we know that you meant "this is never settable" if it's an empty list, and if there are multiple set PVs we can summarize their write state together. Maybe it's worth doing.
There was a problem hiding this comment.
And of course, then we could raise an exception during init if it's left at None
|
This actually isn't strictly needed for lightpath any more after the full sum of changes leading up to v1.0.0, but I'll still try to get this done for completeness's sake as it could be useful for something else in the future. |
|
It's been a while so I want to at least force the CI to re-run, so I'm going close and re-open the PR |
|
old pr, new tests, still passing, gonna merge |
Description
PVStateSignalhas been givenenum_strsandwrite_accessmetadata that behaves similarly to how theEpicsSignalmetadata does.On the first value calculation, which occurs only after the device is fully ready to go, we check the parent states device for its static state enum to use to collect the enum strings and for a setpoint reference that we can subscribe to for write access metadata updates, forwarding these in the
PVStateSignalas appropriate.Motivation and Context
We noticed in the lightpath that any devices with
PVStateSignalbehaved inappropriately at the pcdsdevices level, creating what naively looks like ui bugs inlightpath, while actually being sourced to here.This signal type is used in the legacy stoppers, gate valves, pulse pickers, and movable stands.
How Has This Been Tested?
Interactively by loading happi devices and via checking the lightpath.
Unit test added.
Where Has This Been Documented?
I need to make the pre-release docs.
Pre-merge checklist