-
Notifications
You must be signed in to change notification settings - Fork 180
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(api): Add ready-to-aspirate into engine state so analysis correctly fails #17636
base: edge
Are you sure you want to change the base?
Conversation
44d1571
to
8345292
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## edge #17636 +/- ##
=======================================
Coverage 21.84% 21.84%
=======================================
Files 2822 2822
Lines 217431 217431
Branches 8214 8214
=======================================
Hits 47491 47491
Misses 169940 169940
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
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.
Some inline comments, but also a couple larger things:
- The state updates that go along with stall or overpressure defined errors in the commands that move the plunger should also mark not-ready-to-aspirate
home
should mark a pipette as ready to aspirate if that pipette's axis is homed
Thanks for doing this!
api/src/opentrons/protocol_engine/commands/dispense_in_place.py
Outdated
Show resolved
Hide resolved
5ce719c
to
ff63f28
Compare
78c8217
to
b71f9c8
Compare
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.
Very nice, looks good to me!
Overview
The "ready to aspirate" state was previously only known to the hardware control layer, this adds the state updates to the protocol engine so that commands will fail analysis when steps would produce an error at runtime without removing any of the hardware layer checks.
This stems from a scenario where the following steps would produce a "not ready to aspirate" error at runtime but pass analysis.
Since we automatically do a prepare-to-aspirate as part of the aspirate command and the pick up tip, it successfully passes step 2 but we don't automatically do it during dispense or air gap so when the hardware attempts to start step 5, and step 4 leaves it in an "not-ready" state the hardware controller fails at runtime.
This PR updates all of the various protocol engine command that effect the "ready" state to have a new StateUpdate element that stores the current state in the state view. this way analysis will fail when a particular order of calls would result in an illegal move.
Test Plan and Hands on Testing
Changelog
Review requests
Risk assessment