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

fix(protocol-engine): Fix hanging after cancellation #14215

Merged
merged 1 commit into from
Dec 15, 2023

Conversation

SyntaxColoring
Copy link
Contributor

@SyntaxColoring SyntaxColoring commented Dec 15, 2023

Overview

Fixes RQA-2091

Test Plan

  • Flex and OT-2 protocols can still succeed
  • Flex protocols, when cancelled, home the X/Y/Z axes but leave the plungers and tip ejectors alone
  • OT-2 protocols, when cancelled, eject the tips in the fixed trash if there are any tips attached

Changelog

  • After a protocol ends, always call hardware_api.stop before doing anything further with the hardware. This is the bug fix.
  • Formerly, if a protocol ended without any tips attached, we were taking the opportunity to home the plungers. That's dropped in this PR; for simplicity, we will never ever home the plungers as part of run cleanup. Homing the plungers is taken care of at run start. @sfoster1 and I agree that there was no benefit to homing the plungers during run cleanup, and that it could only open up opportunities for something to go wrong.

Review requests

There are opportunities for further cleanup here. For example, HardwareStopper._drop_tip() is also stopping and homing, which makes the overall sequence hard to follow and seems outside of its responsibility.

Since this PR has already been tested without that cleanup, let's not do it in this PR—but if you have any ideas, please do leave comments.

Risk assessment

Medium. I haven't looked at how well this code is covered by unit tests, but this PR is passing CI without having had to change anything, which suggests there are gaps.


Co-authored with @jbleon95 and @sfoster1

Copy link

codecov bot commented Dec 15, 2023

Codecov Report

Merging #14215 (96d2e0f) into chore_release-7.1.0 (a7d1187) will decrease coverage by 0.01%.
Report is 1 commits behind head on chore_release-7.1.0.
The diff coverage is n/a.

Additional details and impacted files

Impacted file tree graph

@@                   Coverage Diff                   @@
##           chore_release-7.1.0   #14215      +/-   ##
=======================================================
- Coverage                70.42%   70.42%   -0.01%     
=======================================================
  Files                     2512     2512              
  Lines                    71296    71292       -4     
  Branches                  9006     9006              
=======================================================
- Hits                     50212    50208       -4     
  Misses                   18879    18879              
  Partials                  2205     2205              
Flag Coverage Δ
g-code-testing 96.44% <ø> (ø)
notify-server 89.13% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
...rons/protocol_engine/execution/hardware_stopper.py 100.00% <ø> (ø)

Copy link
Member

@sfoster1 sfoster1 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 to me if it works, I think this is the right approach for homing

@SyntaxColoring SyntaxColoring marked this pull request as ready for review December 15, 2023 17:57
@SyntaxColoring SyntaxColoring requested a review from a team as a code owner December 15, 2023 17:57
Copy link
Contributor

@jbleon95 jbleon95 left a comment

Choose a reason for hiding this comment

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

Fixes the cancellation bug and looks correct to me

@SyntaxColoring SyntaxColoring merged commit 9842207 into chore_release-7.1.0 Dec 15, 2023
31 checks passed
@SyntaxColoring SyntaxColoring deleted the fix_hanging_after_cancellation branch December 15, 2023 18:10
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.

3 participants