-
Notifications
You must be signed in to change notification settings - Fork 2
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
Use BlueAPI abort #124
Use BlueAPI abort #124
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.
Looks like it should work. I'm not a fan of the ABORTED
global variable. I think that you should just call dcid.collection_complete(end_time, aborted=False)
in an else_plan
and dcid.collection_complete(end_time, aborted=True)
in the except plan. Ultimately we should be using a callback for ispyb and it will be handled in there, could you make an issue for this if there isn't one already?
if detector_name == "pilatus": | ||
caput(pv.pilat_acquire, 0) | ||
elif detector_name == "eiger": | ||
caput(pv.eiger_acquire, 0) |
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.
Should: The finally
plan will do the detector disarm, right?
caput(pv.pilat_acquire, 0) | ||
elif detector_name == "eiger": | ||
caput(pv.eiger_acquire, 0) | ||
sleep(1.0) |
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.
Should: Why are we sleeping?
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 believe it's to allow the PVs to actually be set - since there is no wait option on this caput
. 1s is probably quite long though, in other parts of the code when setting these PVs it's 0.5
...
global ABORTED | ||
ABORTED = True | ||
logger.warning("Data Collection Aborted") | ||
yield from disarm_zebra(zebra) # If aborted/timed out zebra still armed |
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.
Should: Doesn't reset_zebra_when_collection_done_plan
in the finally
plan deal with this?
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.
Yes, although after going through these changes I'm not entirely sure we should be disarming the zebra in that plan. Or to be more precise, it's fine as it is for the fixed target, but in the extruder the zebra is disarmed at collection end so we shouldn't need to do it unless the collection failed/was aborted
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 don't think it really costs anything to do it in both places
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 good, if we see any issues we'll fix them separately
Closes #117
Needs Dodal661