-
Notifications
You must be signed in to change notification settings - Fork 178
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
feat(api): add deck conflict checks for pipetting with partial tip configuration #14066
feat(api): add deck conflict checks for pipetting with partial tip configuration #14066
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## chore_release-7.1.0 #14066 +/- ##
=======================================================
- Coverage 70.46% 70.46% -0.01%
=======================================================
Files 2512 2512
Lines 71196 71205 +9
Branches 8958 8964 +6
=======================================================
+ Hits 50168 50172 +4
- Misses 18837 18841 +4
- Partials 2191 2192 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
|
010b294
to
b28f5fa
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.
Looks good to me. I think the warnings about ot2 vs flex are because we don't do extent checking for ot2 yet, is that right?
from opentrons.types import DeckSlotName, Point | ||
|
||
|
||
class PartialTipMovementNotAllowedError(ValueError): |
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.
We should make this an enumerated error
Z_SAFETY_MARGIN = 10 | ||
|
||
# Bounding box measurements | ||
A12_column_front_left_bound = Point(x=-1.8, y=2) |
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.
Let's add a todo to push this somewhere else because I would like to derive this from geometry definitions
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.
yep yep
engine_state=engine_state, pipette_id=pipette_id, location=well_location_point | ||
): | ||
# WARNING: (spp, 2023-11-30: this needs to be wired up to check for | ||
# 8-channel pipette extents on both OT2 & Flex!!) |
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.
is it wired up for flex only right now? what's the failure case?
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.
It's wired up to check the extents of only the 96-channel in rightmost column configuration. I don't have the 8-channel movement bounds for both OT2 & Flex.
So failure case is if you configure an 8-channel to use the frontmost nozzle and you made it move to well A1 of a labware in slot 11/ A1, then the pipette might crash against the back of the robot instead of raising an error that it's an unreachable location for the 8-channel in this configuration.
@@ -8,7 +8,7 @@ | |||
"z": 108.96 | |||
}, | |||
"dimensions": { | |||
"bareOverallHeight": 98.0, | |||
"bareOverallHeight": 108.96, |
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.
where'd this come from?
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.
So this PR updated the labwareOffset
z to be the topmost point of the labware-holding metal cylinders. Previously this z was 98.26, and I believe the bare overall height was the bottom plane of those cylinders. When I updated the z to be the topmost point, I forgot to update the overall height. Also it makes more sense to have the topmost point be the height, especially now that some pipette movements will be allowed or not depending on the heights of deck items.
labware_id=labware_id, | ||
well_name=well_name, | ||
well_location=WellLocation(), | ||
) |
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 think I missed adding the check to move_to
. Do we want to allow move_to
to bypass this check?
Nevermind. move_to
is for moving to non-well locations, which makes it checking for collisions with surrounding deck items non-trivial.
Yes, but also we don't do all kinds of extents checking for Flex either. It is usually not an issue because the pipettes are designed to reach all deck locations when used normally, but because a partially configured pipette might not be able to reach locations that it can reach under normal config, it becomes more important to do the checks. |
Successfully tested on a Flex to make sure a protocol that passes deck conflict checks is able to run correctly without issues. |
Closes RSS-378
Overview
Adds conflicts that could arise by moving a partial configuration pipette to the specified location.
This PR adds full conflict check for 96-channel when configured with a rightmost column nozzle layout which includes:
This PR also adds partial conflict check for an 8-channel when configured with single nozzle layout (
H1
nozzle) which includes:If he pipette is configured with any other configuration, there will not be any deck conflict checking and all checks will simply no-op so use other configurations with caution.
NOTE: Conflict checking with addressable areas of trash bin and waste chute will be in a follow-up ticket.
Test Plan
Changelog
Review requests
Risk assessment
Low for existing API