-
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
fix(api): Center instrument over moveable trash regardless of configuration #14413
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## edge #14413 +/- ##
==========================================
+ Coverage 68.14% 68.15% +0.01%
==========================================
Files 2522 2522
Lines 72023 72228 +205
Branches 9218 9314 +96
==========================================
+ Hits 49077 49226 +149
- Misses 20757 20787 +30
- Partials 2189 2215 +26
Flags with carried forward coverage won't be shown. Click here to find out more.
|
This reverts commit 8effe0b.
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.
Thank you! I have one fundamental question, and a number of smaller things that follow from that.
destination_cp = CriticalPoint.XY_CENTER | ||
if "movableTrash" in addressable_area_name: | ||
destination_cp = None | ||
else: | ||
destination_cp = CriticalPoint.XY_CENTER |
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.
Fundamental question. From your PR description:
Rather than cover many addressable area types, we have chose to special case the moveable trash given the likelihood of an adjacent item obstruction causing an overlap in various partial tip configurations.
Can you elaborate on this?
The moveToAddressableArea
command is deliberately specified to center all of its nozzles over the given addressable area—independent of whether the addressable area is a trash, or something else. We should aim for the implementation to match that specification. Are there any challenges with doing that?
In other words, I don't think there should be anything here that's conditional on the addressable area.
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.
The current behavior is that the center of the chosen pipettes is the "critical point" that moves to the center of the addressable area. This means that when a pipette is in partial configuration, the critical point becomes the center of that selection. For example, the center of a partially configured 96 channel in COLUMN configuration for column A1 would be the middle of that column (so somewhere between D1 and E1?). That is my current understanding anyway. Whether or not we want to use the instrument center for ANY AND ALL move to addressable area commands is a difficult question to answer.
In the case of movableTrash, we specifically want to use the center of the instrument, because we're in the trash! We don't want to overlap with anything outside of the bins walls.
In the case of the wasteChute with a cover attached, we would instead want the partially configured 96 channel with single column pickup to use only its active channel as the critical point, so that it can still utilize the cover slot (unless we want to remove that support).
There may be some other examples as well, but these are the two examples related to 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.
Whether or not we want to use the instrument center for ANY AND ALL move to addressable area commands is a difficult question to answer.
Okay, I think this is the crux of the confusion. For the purposes of what this code needs to do, we've already answered that question. The answer is "yes, use the instrument center always."
(For the broader purposes of future extensions to the deck configuration system, and generically supporting lots of fixtures and lots of pipettes, yes, it gets more difficult. But even there, I don't think the solution will be to special-case different addressable areas; it will be to make it configurable per-area, or add a parameter to moveToAddressableArea
.)
In the case of the wasteChute with a cover attached, we would instead want the partially configured 96 channel with single column pickup to use only its active channel as the critical point, so that it can still utilize the cover slot (unless we want to remove that support).
As far as I know, this was never supported. We can double-check this, but for example, if a Python protocol does p96.drop_tip(waste_chute)
, I think it will emit a moveToAddressableArea
command with "addressableAreaName": "96ChannelWasteChute"
, not "addressableAreaName": "8ChannelWastechute"
. That would be the proper use of the Protocol Engine API, as documented.
"Whether to utilize the critical point of the tip configuraiton when moving to an addressable area." | ||
" If True, this command will ignore the tip configuration and use the center of the entire instrument" | ||
" of the entire instrument as the critical point for movement." |
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.
"Whether to utilize the critical point of the tip configuraiton when moving to an addressable area." | |
" If True, this command will ignore the tip configuration and use the center of the entire instrument" | |
" of the entire instrument as the critical point for movement." | |
"Whether to utilize the critical point of the tip configuraiton when moving to an addressable area." | |
" If True, this command will ignore the tip configuration and use the center of the entire instrument" | |
" as the critical point for movement." |
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.
duplicate
api/src/opentrons/protocol_engine/commands/move_to_addressable_area_for_drop_tip.py
Show resolved
Hide resolved
Utilizing the new instrument critical point approach we achieved the desired result of the protocol outlined in the test plan above. When dropping tips in a trash bin, regardless of configuration, the pipette correctly distributed them within the bounds of the trashbin in the expected safe tip drop locations. |
Resolution to noted issue by defaulting ignoreTipConfiguraiton behavior to true. Behavior change ensures that trash bins and waste chutes always operate with center of instrument. Tested on Flex, performed as expected. Of note, covered waste chutes are already incompatible with the 96 channel, so we are safe to proceed. |
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.
LGTM! Thanks for making the changes!
Overview
This seeks to solve problems identified in: RSS-419, RSS-434
Rather than cover many addressable area types, we have chose to special case the moveable trash given the likelihood of an adjacent item obstruction causing an overlap in various partial tip configurations.
The solution to this is to ensure that the waypoint destination does not utilize the active channel of the pipette (such as a single column on a 96 channel) but rather the center of the destination instead.
We also override the adjacent slot checks in cases such as this, as we know that we will not overlap their slots in instances involving the trashbins because of this reconfiguraiton.
Test Plan
Ensure the a protocol switching between partial and full tip pickup drops tips within the bounds of a moveable trash the same way regardless of configuration, and does not at any point overlap with adjacent slots as identified in various collision cases.
Test Case:
This protocol should also be tested for an 8 channel pipette with two full pickups (no partial necessary at this time) to ensure that we have not caused the pipette to incorrectly center over A1 when moving to the moveable Trash.
Risk assessment
Waste chutes were omitted from special casing on this version, so as not to interrupt the current addressable areas provided by the waste chute with cover fixture. This fixture supports single channel tip drop from a 96 channel, but if we centered the partially configured 96 channel on the waste chute we would not be able to make use of the slot. Right now, the 96ch adapter is taller than the waste chute. This means a 96ch adapter in D2 could potentially obstruct the 96ch if partially offset on the Waste Chute, however we would need to verify if such a collision was even possible.