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

feat(api): add deck conflict checks for pipetting with partial tip configuration #14066

Merged

Conversation

sanni-t
Copy link
Member

@sanni-t sanni-t commented Nov 30, 2023

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:

  • conflict check between destination location and labware in the west slot based on final pipette position and west labware height
  • conflict check based on the physical extents of the 96 channel within the Flex deck space

This PR also adds partial conflict check for an 8-channel when configured with single nozzle layout (H1 nozzle) which includes:

  • conflict check between destination location and labware in the north slot based on final pipette position and north labware height.
  • ❌ conflict checks based on physical extents of 8-channel pipettes within OT2 or Flex deck space

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

  • Here's a sample protocol to test the deck conflicts of the 96-channel. The protocol should fail analysis if there's deck conflicts. If there aren't, then you should be able to run the full protocol on a robot.
from opentrons.protocol_api import COLUMN, ALL


requirements = {
	"robotType": "Flex",
	"apiLevel": "2.16"
}

def run(protocol_context):

	#############  TALL LABWARE  #################
	# These labware should be to the east of short labware to avoid partial tip deck conflicts
	trash_labware = protocol_context.load_labware("opentrons_1_trash_3200ml_fixed", "A3")

	# Will result in errors in this placement-  <---------------------
	tip_rack1 = protocol_context.load_labware("opentrons_flex_96_tiprack_50ul", "C1")
	tip_rack2 = protocol_context.load_labware("opentrons_flex_96_tiprack_50ul", "B1")
	
	# Will NOT raise errors in this placement-
	# tip_rack1 = protocol_context.load_labware("opentrons_flex_96_tiprack_50ul", "C3")
	# tip_rack2 = protocol_context.load_labware("opentrons_flex_96_tiprack_50ul", "B3")


	instrument = protocol_context.load_instrument('flex_96channel_1000', mount="left", tip_racks=[tip_rack1, tip_rack2])
	instrument.trash_container = trash_labware
	
	#############  SHORT LABWARE  ################
	# These labware should be to the west of tall labware to avoid any partial tip deck conflicts
	my_pcr_plate = protocol_context.load_labware('nest_96_wellplate_200ul_flat', "C2")
	my_other_pcr_plate = protocol_context.load_labware('nest_96_wellplate_200ul_flat', "D2")


	############# MEDIUM LABWARE ###############
	# These should be in the middle, but might be fine to the west of some labware 
	# if the pipette doesn't lower enough to collide with it
	my_tuberack = protocol_context.load_labware('opentrons_10_tuberack_falcon_4x50ml_6x15ml_conical', "D1")

	instrument.configure_nozzle_layout(style=COLUMN, start="A12")

	# No error cuz short labware to left of tiprack
	instrument.pick_up_tip(tip_rack1.wells()[0])

	# No error cuz no labware on left of plate
	instrument.aspirate(50, my_pcr_plate.wells_by_name()["A4"])

	# No error cuz dispensing from high above plate, so it clears reservoir on the left
	instrument.dispense(20, my_other_pcr_plate.wells_by_name()["A1"].top(150))

	# Will raise error   <---------------------
	instrument.dispense(20, my_other_pcr_plate.wells_by_name()["A1"].bottom())

	# No error cuz A1 is within 96 channel right column bounds
	instrument.dispense(10, my_tuberack.wells_by_name()["A1"])

	instrument.drop_tip()

	######### CHANGE CONFIG TO ALL #########
	instrument.configure_nozzle_layout(style=ALL)

	# No error cuz short labware to left of tiprack
	instrument.pick_up_tip(tip_rack2.wells()[0])

	# No error cuz no labware on left of plate
	instrument.aspirate(50, my_pcr_plate.wells_by_name()["A4"])

	# No error NOW because of full config
	instrument.dispense(20, my_other_pcr_plate.wells_by_name()["A1"].bottom())
  • Also test a regular 8- and single channel protocol to make sure its behavior is unchanged.

Changelog

  • added deck conflict check logic to all pipette movement methods, based on pipette configuration
  • added deck item geometry getters to calculate heights of items in slots

Review requests

  • Test and make sure the checks work correctly

Risk assessment

Low for existing API

Copy link

codecov bot commented Nov 30, 2023

Codecov Report

Merging #14066 (de1d5a2) into chore_release-7.1.0 (5a33ab4) will decrease coverage by 0.01%.
Report is 15 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   #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     
Flag Coverage Δ
app 67.66% <ø> (-0.02%) ⬇️
g-code-testing 96.44% <ø> (ø)
notify-server 89.13% <ø> (ø)
protocol-designer 44.99% <ø> (ø)
shared-data 75.02% <ø> (ø)
step-generation 86.84% <ø> (ø)

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

Files Coverage Δ
...pi/src/opentrons/protocol_engine/state/geometry.py 100.00% <ø> (ø)
api/src/opentrons/protocol_engine/state/modules.py 91.35% <ø> (ø)
...pi/src/opentrons/protocol_engine/state/pipettes.py 100.00% <ø> (ø)

... and 5 files with indirect coverage changes

@sanni-t sanni-t marked this pull request as ready for review December 5, 2023 21:02
@sanni-t sanni-t requested review from a team as code owners December 5, 2023 21:02
@sanni-t sanni-t force-pushed the RSS-378-deck-conflict-checks-for-partial-tip-3 branch from 010b294 to b28f5fa Compare December 5, 2023 21:30
@sanni-t sanni-t requested a review from a team as a code owner December 5, 2023 21:30
@sanni-t sanni-t requested review from brenthagen and removed request for a team December 5, 2023 21:30
@sanni-t sanni-t changed the base branch from edge to chore_release-7.1.0 December 5, 2023 21:30
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. 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):
Copy link
Member

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)
Copy link
Member

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

Copy link
Member Author

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!!)
Copy link
Member

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?

Copy link
Member Author

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,
Copy link
Member

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?

Copy link
Member Author

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(),
)
Copy link
Member Author

@sanni-t sanni-t Dec 6, 2023

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.

@sanni-t
Copy link
Member Author

sanni-t commented Dec 6, 2023

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?

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.

@sanni-t
Copy link
Member Author

sanni-t commented Dec 6, 2023

Successfully tested on a Flex to make sure a protocol that passes deck conflict checks is able to run correctly without issues.

@sanni-t sanni-t merged commit 5b4842b into chore_release-7.1.0 Dec 6, 2023
46 checks passed
@sanni-t sanni-t deleted the RSS-378-deck-conflict-checks-for-partial-tip-3 branch July 15, 2024 21:37
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.

2 participants