-
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 pipette movement restrictions for 96ch col A1 and 8ch single A1 #14230
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## edge #14230 +/- ##
=======================================
Coverage 68.53% 68.53%
=======================================
Files 2421 2421
Lines 67108 67108
Branches 8823 8823
=======================================
Hits 45993 45993
Misses 19040 19040
Partials 2075 2075
Flags with carried forward coverage won't be shown. Click here to find out more. |
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 pending comments
).to_equivalent_for_robot_type(engine_state.config.robot_type) | ||
destination_slot_num = _deck_slot_to_int(DeckSlotLocation(slotName=labware_slot)) | ||
adjacent_slot_num = None | ||
if primary_nozzle == "A12": |
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 probably want to change this eventually to "column 1"/"column 12" via the column mappings in the pipette geometry def.
).to_equivalent_for_robot_type(engine_state.config.robot_type) | ||
destination_slot = _deck_slot_to_int(DeckSlotLocation(slotName=labware_slot)) | ||
adjacent_slot_num = None | ||
if primary_nozzle == "H1": |
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.
probably want to change this to column mapping via the geometry def at some point
) | ||
# TODO (spp, 2023-11-07): check for 8-channel nozzle H1 extents on Flex & OT2 | ||
if pipette_channels == 96 and nozzle_config == NozzleConfigurationType.COLUMN: | ||
if primary_nozzle == "A12": |
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.
probably want to change this to column mapping in the geometry def at some point
When attempting to drop tips in a trash bin in A3 with the leftmost column (A1) filled with tips, the 96 channel collides with the right side of the robot. Also revealed through this, when the 96 channel has a single column of tips in A1 and is at its rightmost position, the further it can reach is A10. The single column will never reach A11 or A12 within the tiprack/wellplate/labware it is interacting with. |
Hmm, the robot extents/ pipette bounds checking currently doesn't happen when moving to addressable areas so this isn't unexpected.
I'm assuming you're talking about column A10 of a labware in the third deck column. This is an important thing to fix in this PR. The behavior indicates that the numbers I've used for out-of-bounds check are incorrect. I'll find out what the correct numbers are and update this PR |
I think I told @CaseyBatten yesterday that this would be a problem for v7.1.0, but I was under the mistaken impression at the time that we were trying to support actually using column 1. After discussing with @sanni-t and Linda, I agree that it's okay for that gap in motion planning and validation to not be fixed in this PR. |
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'm still catching up on the 96-channel code so I haven't worked all the way through this, but this looks good to me so far. Just some minor suggestions.
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.
Also LGTM pending comments.
Co-authored-by: Max Marrone <[email protected]>
bedf52b
to
ced6577
Compare
Tested the new robot bounds on a Flex and all accessible areas were reachable by the pipette without collision while the locations that raise an error are actually not accessible by the pipette. |
@ecormany @jwwojak tagging you here to let you know about this robot bounds check change. Turns out we can access the first column of thermocycler labware using A12 nozzle column of the 96 channel (although it's very close to the left bound, so big offsets to this location might still result in out of bounds error). Once this change is released the API will no longer raise an error for this condition and we will have to update the docs that mention this restriction. |
from opentrons.protocol_api.core.engine.deck_conflict import ( | ||
PartialTipMovementNotAllowedError, | ||
) | ||
from opentrons.types import Point |
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.
from opentrons.types import Point |
There's a lint error from this unused import, but this otherwise looks good to me to merge.
…single A1 (#14230) --------- Co-authored-by: Max Marrone <[email protected]>
Closes RSS-423
Overview
Implements deck conflict checking for when:
Even though 7.1.0 release doesn't officially support any partial configurations other than 96-channel rightmost column, one can write protocols that use other configurations. The conflict checking added in this PR makes these not-yet-supported configurations at least safe for use.
Test Plan
Test protocol:
Changelog
Review requests
Risk assessment
Medium. Adds new restrictions for yet unsupported pipette configuration but also touches the logic for supported configurations.