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(engine): add mmToEdge parameter to touchTip #17107

Open
wants to merge 3 commits into
base: edge
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 18 additions & 1 deletion api/src/opentrons/protocol_engine/commands/touch_tip.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,11 @@

from opentrons.types import Point

from ..errors import TouchTipDisabledError, LabwareIsTipRackError
from ..errors import (
TouchTipDisabledError,
TouchTipIncompatibleArgumentsError,
LabwareIsTipRackError,
)
from ..types import DeckPoint
from .command import (
AbstractCommandImpl,
Expand Down Expand Up @@ -45,6 +49,12 @@ class TouchTipParams(PipetteIdMixin, WellLocationMixin):
),
)

mmToEdge: Optional[float] = Field(
None,
description="Offset away from the the well edge, in millimeters."
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you describe this value as the offset "from the the well edge," do you want to call it mmFromEdge?

"Incompatible when a radius is included as a non 1.0 value.",
)

speed: Optional[float] = Field(
None,
description=(
Expand Down Expand Up @@ -89,6 +99,11 @@ async def execute(
labware_id = params.labwareId
well_name = params.wellName

if params.radius != 1.0 and params.mmToEdge is not None:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the radius of 1.0 a ratio, or is it a value in millimeters?

raise TouchTipIncompatibleArgumentsError(
"Cannot use mmToEdge with a radius that is not 1.0"
)

if self._state_view.labware.get_has_quirk(labware_id, "touchTipDisabled"):
raise TouchTipDisabledError(
f"Touch tip not allowed on labware {labware_id}"
Expand All @@ -112,11 +127,13 @@ async def execute(
pipette_id, params.speed
)

mm_to_edge = params.mmToEdge if params.mmToEdge is not None else 0
touch_waypoints = self._state_view.motion.get_touch_tip_waypoints(
pipette_id=pipette_id,
labware_id=labware_id,
well_name=well_name,
radius=params.radius,
mm_to_edge=mm_to_edge,
center_point=Point(
center_result.public.position.x,
center_result.public.position.y,
Expand Down
2 changes: 2 additions & 0 deletions api/src/opentrons/protocol_engine/errors/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
LabwareIsTipRackError,
LabwareIsAdapterError,
TouchTipDisabledError,
TouchTipIncompatibleArgumentsError,
WellDoesNotExistError,
PipetteNotLoadedError,
ModuleNotLoadedError,
Expand Down Expand Up @@ -110,6 +111,7 @@
"LabwareIsTipRackError",
"LabwareIsAdapterError",
"TouchTipDisabledError",
"TouchTipIncompatibleArgumentsError",
"WellDoesNotExistError",
"PipetteNotLoadedError",
"ModuleNotLoadedError",
Expand Down
13 changes: 13 additions & 0 deletions api/src/opentrons/protocol_engine/errors/exceptions.py
Original file line number Diff line number Diff line change
Expand Up @@ -361,6 +361,19 @@ def __init__(
super().__init__(ErrorCodes.GENERAL_ERROR, message, details, wrapping)


class TouchTipIncompatibleArgumentsError(ProtocolEngineError):
"""Raised when touch tip is used with both a custom radius and a mmToEdge argument."""

def __init__(
self,
message: Optional[str] = None,
details: Optional[Dict[str, Any]] = None,
wrapping: Optional[Sequence[EnumeratedError]] = None,
) -> None:
"""Build a TouchTipIncompatibleArgumentsError."""
super().__init__(ErrorCodes.GENERAL_ERROR, message, details, wrapping)


class WellDoesNotExistError(ProtocolEngineError):
"""Raised when referencing a well that does not exist."""

Expand Down
14 changes: 9 additions & 5 deletions api/src/opentrons/protocol_engine/state/_move_types.py
Original file line number Diff line number Diff line change
Expand Up @@ -53,15 +53,19 @@ def get_move_type_to_well(


def get_edge_point_list(
center: Point, x_radius: float, y_radius: float, edge_path_type: EdgePathType
center: Point,
x_radius: float,
y_radius: float,
mm_to_edge: float,
edge_path_type: EdgePathType,
) -> List[Point]:
"""Get list of edge points dependent on edge path type."""
edges = EdgeList(
right=center + Point(x=x_radius, y=0, z=0),
left=center + Point(x=-x_radius, y=0, z=0),
right=center + Point(x=x_radius - mm_to_edge, y=0, z=0),
left=center + Point(x=-x_radius + mm_to_edge, y=0, z=0),
Copy link
Contributor

@ddcc4 ddcc4 Dec 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Points are supposed to be an arithmetic type, right? Is it intended to be used like:

right = center + Point(x=x_radius) - Point(x=mm_to_edge)
left = center + Point(x=x_radius) + Point(x=mm_to_edge)

?

center=center,
forward=center + Point(x=0, y=y_radius, z=0),
back=center + Point(x=0, y=-y_radius, z=0),
forward=center + Point(x=0, y=y_radius - mm_to_edge, z=0),
back=center + Point(x=0, y=-y_radius + mm_to_edge, z=0),
)

if edge_path_type == EdgePathType.LEFT:
Expand Down
7 changes: 6 additions & 1 deletion api/src/opentrons/protocol_engine/state/motion.py
Original file line number Diff line number Diff line change
Expand Up @@ -327,6 +327,7 @@ def get_touch_tip_waypoints(
labware_id: str,
well_name: str,
center_point: Point,
mm_to_edge: float,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why isn't this defaulted to 0.0 here?

radius: float = 1.0,
) -> List[motion_planning.Waypoint]:
"""Get a list of touch points for a touch tip operation."""
Expand All @@ -346,7 +347,11 @@ def get_touch_tip_waypoints(
)

positions = _move_types.get_edge_point_list(
center_point, x_offset, y_offset, edge_path_type
center=center_point,
x_radius=x_offset,
y_radius=y_offset,
mm_to_edge=mm_to_edge,
edge_path_type=edge_path_type,
)
critical_point: Optional[CriticalPoint] = None

Expand Down
110 changes: 109 additions & 1 deletion api/tests/opentrons/protocol_engine/commands/test_touch_tip.py
Original file line number Diff line number Diff line change
Expand Up @@ -102,8 +102,99 @@ async def test_touch_tip_implementation(
pipette_id="abc",
labware_id="123",
well_name="A3",
center_point=Point(x=1, y=2, z=3),
radius=0.456,
mm_to_edge=0,
center_point=Point(x=1, y=2, z=3),
)
).then_return(
[
Waypoint(
position=Point(x=11, y=22, z=33),
critical_point=CriticalPoint.XY_CENTER,
),
Waypoint(
position=Point(x=44, y=55, z=66),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these dummy return values supposed to be consistent with the size and center of the well at all?

critical_point=CriticalPoint.XY_CENTER,
),
]
)

decoy.when(
await mock_gantry_mover.move_to(
pipette_id="abc",
waypoints=[
Waypoint(
position=Point(x=11, y=22, z=33),
critical_point=CriticalPoint.XY_CENTER,
),
Waypoint(
position=Point(x=44, y=55, z=66),
critical_point=CriticalPoint.XY_CENTER,
),
],
speed=9001,
)
).then_return(Point(x=4, y=5, z=6))

result = await subject.execute(params)

assert result == SuccessData(
public=TouchTipResult(position=DeckPoint(x=4, y=5, z=6)),
state_update=update_types.StateUpdate(
pipette_location=update_types.PipetteLocationUpdate(
pipette_id="abc",
new_location=update_types.Well(labware_id="123", well_name="A3"),
new_deck_point=DeckPoint(x=4, y=5, z=6),
)
),
)


async def test_touch_tip_implementation_with_mm_to_edge(
decoy: Decoy,
mock_state_view: StateView,
mock_movement_handler: MovementHandler,
mock_gantry_mover: GantryMover,
subject: TouchTipImplementation,
) -> None:
"""A TouchTip command should use mmToEdge if provided."""
params = TouchTipParams(
pipetteId="abc",
labwareId="123",
wellName="A3",
wellLocation=WellLocation(offset=WellOffset(x=1, y=2, z=3)),
mmToEdge=0.789,
speed=42.0,
)

decoy.when(
await mock_movement_handler.move_to_well(
pipette_id="abc",
labware_id="123",
well_name="A3",
well_location=WellLocation(offset=WellOffset(x=1, y=2, z=3)),
current_well=None,
force_direct=False,
minimum_z_height=None,
speed=None,
operation_volume=None,
)
).then_return(Point(x=1, y=2, z=3))

decoy.when(
mock_state_view.pipettes.get_movement_speed(
pipette_id="abc", requested_speed=42.0
)
).then_return(9001)

decoy.when(
mock_state_view.motion.get_touch_tip_waypoints(
pipette_id="abc",
labware_id="123",
well_name="A3",
radius=1.0,
mm_to_edge=0.789,
Copy link
Contributor

@ddcc4 ddcc4 Dec 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm confused what this is testing. Where in these tests do they demonstrate the right=center+Point(x=x_radius-mm_to_edge) etc. calculations that you implemented?

I was going to call you out for using non-fraction-of-2 values in the test :) expecting that your test would fail because 0.789 is not representable exactly as a float and you would hit some inexact rounding behavior when your test ran, but I'm not sure where in this test you trigger the arithmetic.

center_point=Point(x=1, y=2, z=3),
)
).then_return(
[
Expand Down Expand Up @@ -183,3 +274,20 @@ async def test_touch_tip_no_tip_racks(

with pytest.raises(errors.LabwareIsTipRackError):
await subject.execute(params)


async def test_touch_tip_incompatible_arguments(
decoy: Decoy, mock_state_view: StateView, subject: TouchTipImplementation
) -> None:
"""It should disallow touch tip if radius and mmToEdge is provided."""
params = TouchTipParams(
pipetteId="abc",
labwareId="123",
wellName="A3",
wellLocation=WellLocation(),
radius=1.23,
mmToEdge=4.56,
)

with pytest.raises(errors.TouchTipIncompatibleArgumentsError):
await subject.execute(params)
Original file line number Diff line number Diff line change
Expand Up @@ -928,6 +928,7 @@ def test_get_touch_tip_waypoints(
x_radius=1.2,
y_radius=3.4,
edge_path_type=_move_types.EdgePathType.RIGHT,
mm_to_edge=0.456,
)
).then_return([Point(x=11, y=22, z=33), Point(x=44, y=55, z=66)])

Expand All @@ -937,6 +938,7 @@ def test_get_touch_tip_waypoints(
well_name="B2",
center_point=center_point,
radius=0.123,
mm_to_edge=0.456,
)

assert result == [
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,11 @@ def get_edge_point_list(
) -> None:
"""It should get a list of well edge points."""
result = subject.get_edge_point_list(
Point(x=10, y=20, z=30), x_radius=5, y_radius=10, edge_path_type=edge_path_type
Point(x=13, y=23, z=33),
x_radius=5,
y_radius=10,
mm_to_edge=3,
edge_path_type=edge_path_type,
)

assert result == expected_result
5 changes: 5 additions & 0 deletions shared-data/command/schemas/11.json
Original file line number Diff line number Diff line change
Expand Up @@ -3924,6 +3924,11 @@
"default": 1.0,
"type": "number"
},
"mmToEdge": {
"title": "Mmtoedge",
"description": "Offset away from the the well edge, in millimeters.Incompatible when a radius is included as a non 1.0 value.",
"type": "number"
},
"speed": {
"title": "Speed",
"description": "Override the travel speed in mm/s. This controls the straight linear speed of motion.",
Expand Down
Loading