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

Use BlueAPI abort #124

Merged
merged 39 commits into from
Jul 29, 2024
Merged

Use BlueAPI abort #124

merged 39 commits into from
Jul 29, 2024

Conversation

noemifrisina
Copy link
Contributor

Closes #117

Needs Dodal661

@noemifrisina noemifrisina marked this pull request as draft July 15, 2024 08:44
@noemifrisina noemifrisina added the I24 serial Issues relating to ssx on I24 label Jul 17, 2024
Copy link
Contributor

@DominicOram DominicOram left a 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?

Comment on lines 405 to 408
if detector_name == "pilatus":
caput(pv.pilat_acquire, 0)
elif detector_name == "eiger":
caput(pv.eiger_acquire, 0)
Copy link
Contributor

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)
Copy link
Contributor

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?

Copy link
Contributor Author

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
Copy link
Contributor

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?

Copy link
Contributor Author

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

Copy link
Contributor

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

@noemifrisina noemifrisina marked this pull request as ready for review July 26, 2024 15:20
Copy link
Contributor

@dperl-dls dperl-dls left a 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

@dperl-dls dperl-dls dismissed DominicOram’s stale review July 29, 2024 12:09

changes were adressed

@dperl-dls dperl-dls merged commit e37b3f6 into main Jul 29, 2024
13 checks passed
@dperl-dls dperl-dls deleted the 117_use-blueapi-abort branch July 29, 2024 12:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I24 serial Issues relating to ssx on I24
Projects
None yet
Development

Successfully merging this pull request may close these issues.

I24 SSX: Use BlueAPI abort rather than general PV
3 participants