-
Notifications
You must be signed in to change notification settings - Fork 59
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: for LCLS-I att, give proper error for uninterruptable move #1183
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
------- | ||
status : MoveStatus | ||
""" | ||
if self.moving: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Double-checking the moving
definition: https://github.com/bluesky/ophyd/blob/9a0bf3f5fd8c98ee23e5d51c35db695c86a6a921/ophyd/pv_positioner.py#L154-L169
This seems fine
"previous move has not completed. This is not an " | ||
"interruptable positioner. Try waiting after the previous " | ||
f"move or for the move's status to complete. {progress}" | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the addition of "progress" here
I understand what the error message means - and it's likely clear enough for the end user as well. 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like it, nice and simple. I was thinking we could also guard against puts to the PVs themselves, but maybe that's too far? (if a user is reaching to the setpoint directly, can they truly be stopped?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Additional docstring 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this addresses the original issue, while also making it transparent to the end user. We can test this implementation at XPP pretty much anytime until Wed 11/15.
This has the desired behavior in practice, with one caveat: it's possible to get into a state where moving=False and there is still a lingering MoveStatus that isn't complete. There's some race conditions for the case where you start a new move immediately after the previous move is complete. The IOC will lock up in this case as well, but here's my testing log that sort of shows what the python side does:
|
I think I want to merge this as-is and consider what follow-up options exist for mitigating this behavior. |
I've pulled some follow-up tasks into jira tickets, which despite being less transparent on the github end are more convenient internally for assignment/tracking: |
Description
PVPositionerNoInterrupt
: a utility class for PV positioners that cannot start a new move until the previous move completes.PVPositionerNoInterrupt
toAttBase
to gain this behavior for LCLS-I style attenuators.Motivation and Context
For the LCLSI attenuators, if you try to start a new move during the previous move, you get a confusing and useless error. This new error makes it very clear that cannot do this.
As implemented, the IOC will not consider new move requests during the previous move, so nothing of value is lost here.
https://jira.slac.stanford.edu/browse/ECS-4099
How Has This Been Tested?
Where Has This Been Documented?
pre-release notes and the aforementioned Jira ticket only
Pre-merge checklist