diff --git a/api/src/opentrons/motion_planning/waypoints.py b/api/src/opentrons/motion_planning/waypoints.py index c9112295d70..4fd44d983ce 100644 --- a/api/src/opentrons/motion_planning/waypoints.py +++ b/api/src/opentrons/motion_planning/waypoints.py @@ -18,7 +18,7 @@ def get_waypoints( dest: Point, *, max_travel_z: float, - min_travel_z: float = 0.0, + min_travel_z: float, move_type: MoveType = MoveType.GENERAL_ARC, xy_waypoints: Sequence[Tuple[float, float]] = (), origin_cp: Optional[CriticalPoint] = None, @@ -41,17 +41,22 @@ def get_waypoints( :returns: A list of :py:class:`.Waypoint` locations to move through. """ - # NOTE(mc, 2020-10-28): This function is currently experimental. Flipping - # `use_experimental_waypoint_planning` to True in - # `opentrons.protocols.geometry.plan_moves` causes three test failures at - # the time of this writing. + # NOTE(mm, 2022-06-22): + # This function is used by v6+ JSON protocols and v3+ + # Python API protocols, but not v2 Python API protocols. # - # Eventually, it may take over for opentrons.hardware_control.util.plan_arc + # Flipping `use_experimental_waypoint_planning` to True to make PAPIv2 use this too + # causes three test failures at the time of this writing. + # Eventually, those may be resolved and this may take over for + # opentrons.hardware_control.util.plan_arc, which PAPIv2 currently uses. dest_waypoint = Waypoint(dest, dest_cp) waypoints: List[Waypoint] = [] # a direct move can ignore all arc and waypoint planning if move_type == MoveType.DIRECT: + # TODO(mm, 2022-06-17): This will not raise an out-of-bounds error + # even if the destination is far out of bounds. A protocol can run into this by + # doing a direct move to bad coordinates. Should we raise in that case? return [dest_waypoint] # ensure destination is not out of bounds diff --git a/api/src/opentrons/protocol_engine/__init__.py b/api/src/opentrons/protocol_engine/__init__.py index 54354e583ca..3a0c6544768 100644 --- a/api/src/opentrons/protocol_engine/__init__.py +++ b/api/src/opentrons/protocol_engine/__init__.py @@ -24,6 +24,7 @@ LabwareOffsetCreate, LabwareOffsetVector, LabwareOffsetLocation, + DeckPoint, DeckSlotLocation, ModuleLocation, Dimensions, @@ -68,6 +69,7 @@ "LabwareOffsetVector", "LabwareOffsetLocation", "DeckSlotLocation", + "DeckPoint", "ModuleLocation", "Dimensions", "EngineStatus", diff --git a/api/src/opentrons/protocol_engine/commands/move_to_coordinates.py b/api/src/opentrons/protocol_engine/commands/move_to_coordinates.py index 8cc46c9d48b..e478c9f86f9 100644 --- a/api/src/opentrons/protocol_engine/commands/move_to_coordinates.py +++ b/api/src/opentrons/protocol_engine/commands/move_to_coordinates.py @@ -1,13 +1,21 @@ """Move to coordinates command request, result, and implementation models.""" from __future__ import annotations + from pydantic import BaseModel, Field -from typing import Optional, Type +from typing import Optional, Type, TYPE_CHECKING from typing_extensions import Literal +from opentrons.hardware_control import HardwareControlAPI +from opentrons.types import Point + from ..types import DeckPoint from .pipetting_common import PipetteIdMixin from .command import AbstractCommandImpl, BaseCommand, BaseCommandCreate +if TYPE_CHECKING: + from ..execution import MovementHandler + from ..state import StateView + MoveToCoordinatesCommandType = Literal["moveToCoordinates"] @@ -55,12 +63,70 @@ class MoveToCoordinatesImplementation( ): """Move to coordinates command implementation.""" - def __init__(self, **kwargs: object) -> None: - pass + def __init__( + self, + state_view: StateView, + hardware_api: HardwareControlAPI, + movement: MovementHandler, + **kwargs: object, + ) -> None: + self._state_view = state_view + self._hardware_api = hardware_api + self._movement = movement async def execute(self, params: MoveToCoordinatesParams) -> MoveToCoordinatesResult: """Move the requested pipette to the requested coordinates.""" - raise NotImplementedError() + await self._move_to_coordinates( + pipette_id=params.pipetteId, + deck_coordinates=params.coordinates, + direct=params.forceDirect, + additional_min_travel_z=params.minimumZHeight, + ) + return MoveToCoordinatesResult() + + async def _move_to_coordinates( + self, + pipette_id: str, + deck_coordinates: DeckPoint, + direct: bool, + additional_min_travel_z: Optional[float], + ) -> None: + hw_mount = self._state_view.pipettes.get( + pipette_id=pipette_id + ).mount.to_hw_mount() + + origin = await self._hardware_api.gantry_position( + mount=hw_mount, + # critical_point=None to get the current position of whatever tip is + # currently attached (if any). + critical_point=None, + ) + + max_travel_z = self._hardware_api.get_instrument_max_height( + mount=hw_mount, + # critical_point=None to get the maximum z-coordinate + # given whatever tip is currently attached (if any). + critical_point=None, + ) + + # calculate the movement's waypoints + waypoints = self._state_view.motion.get_movement_waypoints_to_coords( + origin=origin, + dest=Point( + x=deck_coordinates.x, y=deck_coordinates.y, z=deck_coordinates.z + ), + max_travel_z=max_travel_z, + direct=direct, + additional_min_travel_z=additional_min_travel_z, + ) + + # move through the waypoints + for waypoint in waypoints: + await self._hardware_api.move_to( + mount=hw_mount, + abs_position=waypoint.position, + critical_point=waypoint.critical_point, + ) class MoveToCoordinates(BaseCommand[MoveToCoordinatesParams, MoveToCoordinatesResult]): diff --git a/api/src/opentrons/protocol_engine/execution/movement.py b/api/src/opentrons/protocol_engine/execution/movement.py index 3e96b2538d4..bb785a40983 100644 --- a/api/src/opentrons/protocol_engine/execution/movement.py +++ b/api/src/opentrons/protocol_engine/execution/movement.py @@ -98,7 +98,7 @@ async def move_to_well( max_travel_z = self._hardware_api.get_instrument_max_height(mount=hw_mount) # calculate the movement's waypoints - waypoints = self._state_store.motion.get_movement_waypoints( + waypoints = self._state_store.motion.get_movement_waypoints_to_well( pipette_id=pipette_id, labware_id=labware_id, well_name=well_name, @@ -110,11 +110,11 @@ async def move_to_well( ) # move through the waypoints - for wp in waypoints: + for waypoint in waypoints: await self._hardware_api.move_to( mount=hw_mount, - abs_position=wp.position, - critical_point=wp.critical_point, + abs_position=waypoint.position, + critical_point=waypoint.critical_point, ) async def move_relative( diff --git a/api/src/opentrons/protocol_engine/state/motion.py b/api/src/opentrons/protocol_engine/state/motion.py index 03253725dcc..d11d942a985 100644 --- a/api/src/opentrons/protocol_engine/state/motion.py +++ b/api/src/opentrons/protocol_engine/state/motion.py @@ -4,12 +4,7 @@ from opentrons.types import MountType, Point, DeckSlotName from opentrons.hardware_control.types import CriticalPoint -from opentrons.motion_planning import ( - MoveType, - Waypoint, - MotionPlanningError, - get_waypoints, -) +from opentrons import motion_planning from .. import errors from ..types import WellLocation @@ -68,7 +63,7 @@ def get_pipette_location( critical_point = CriticalPoint.XY_CENTER return PipetteLocationData(mount=mount, critical_point=critical_point) - def get_movement_waypoints( + def get_movement_waypoints_to_well( self, pipette_id: str, labware_id: str, @@ -78,8 +73,8 @@ def get_movement_waypoints( origin_cp: Optional[CriticalPoint], max_travel_z: float, current_well: Optional[CurrentWell] = None, - ) -> List[Waypoint]: - """Get the movement waypoints from an origin to a given location.""" + ) -> List[motion_planning.Waypoint]: + """Calculate waypoints to a destination that's specified as a well.""" location = current_well or self._pipettes.get_current_well() center_dest = self._labware.get_has_quirk( labware_id, @@ -100,13 +95,13 @@ def get_movement_waypoints( and labware_id == location.labware_id ): move_type = ( - MoveType.IN_LABWARE_ARC + motion_planning.MoveType.IN_LABWARE_ARC if well_name != location.well_name - else MoveType.DIRECT + else motion_planning.MoveType.DIRECT ) min_travel_z = self._geometry.get_labware_highest_z(labware_id) else: - move_type = MoveType.GENERAL_ARC + move_type = motion_planning.MoveType.GENERAL_ARC min_travel_z = self._geometry.get_all_labware_highest_z() if location is not None: if self._module.should_dodge_thermocycler( @@ -123,8 +118,7 @@ def get_movement_waypoints( # could crash onto the thermocycler if current well is not known. try: - # TODO(mc, 2021-01-08): inject `get_waypoints` via constructor - return get_waypoints( + return motion_planning.get_waypoints( move_type=move_type, origin=origin, origin_cp=origin_cp, @@ -134,5 +128,50 @@ def get_movement_waypoints( max_travel_z=max_travel_z, xy_waypoints=extra_waypoints, ) - except MotionPlanningError as error: + except motion_planning.MotionPlanningError as error: + raise errors.FailedToPlanMoveError(str(error)) + + def get_movement_waypoints_to_coords( + self, + origin: Point, + dest: Point, + max_travel_z: float, + direct: bool, + additional_min_travel_z: Optional[float], + ) -> List[motion_planning.Waypoint]: + """Calculate waypoints to a destination that's specified as deck coordinates. + + Args: + origin: The start point of the movement. + dest: The end point of the movement. + max_travel_z: How high, in deck coordinates, the pipette can go. + This should be measured at the bottom of whatever tip is currently + attached (if any). + direct: If True, move directly. If False, move in an arc. + additional_min_travel_z: The minimum height to clear, if moving in an arc. + Ignored if `direct` is True. If lower than the default height, + the default is used; this can only increase the height, not decrease it. + """ + all_labware_highest_z = self._geometry.get_all_labware_highest_z() + if additional_min_travel_z is None: + additional_min_travel_z = float("-inf") + min_travel_z = max(all_labware_highest_z, additional_min_travel_z) + + move_type = ( + motion_planning.MoveType.DIRECT + if direct + else motion_planning.MoveType.GENERAL_ARC + ) + + try: + return motion_planning.get_waypoints( + origin=origin, + dest=dest, + min_travel_z=min_travel_z, + max_travel_z=max_travel_z, + move_type=move_type, + origin_cp=None, + dest_cp=None, + ) + except motion_planning.MotionPlanningError as error: raise errors.FailedToPlanMoveError(str(error)) diff --git a/api/src/opentrons/protocol_engine/state/pipettes.py b/api/src/opentrons/protocol_engine/state/pipettes.py index 75f87a3f6fb..71201f6b56d 100644 --- a/api/src/opentrons/protocol_engine/state/pipettes.py +++ b/api/src/opentrons/protocol_engine/state/pipettes.py @@ -14,6 +14,7 @@ LoadPipetteResult, AspirateResult, DispenseResult, + MoveToCoordinatesResult, MoveToWellResult, PickUpTipResult, DropTipResult, @@ -87,8 +88,12 @@ def _handle_command(self, command: Command) -> None: labware_id=command.params.labwareId, well_name=command.params.wellName, ) + # TODO(mc, 2021-11-12): wipe out current_well on movement failures, too - elif isinstance(command.result, HomeResult): + elif isinstance(command.result, (HomeResult, MoveToCoordinatesResult)): + # A command left the pipette in a place that we can't associate + # with a logical well location. Set the current well to None + # to reflect the fact that it's now unknown. self._state.current_well = None if isinstance(command.result, LoadPipetteResult): diff --git a/api/src/opentrons/protocol_runner/json_command_translator.py b/api/src/opentrons/protocol_runner/json_command_translator.py index 5cedd54708f..f18107ad30b 100644 --- a/api/src/opentrons/protocol_runner/json_command_translator.py +++ b/api/src/opentrons/protocol_runner/json_command_translator.py @@ -115,7 +115,6 @@ def translate( exclude_commands = [ "loadLiquid", "moveToSlot", - "moveToCoordinates", ] commands_to_parse = [ command diff --git a/api/tests/opentrons/motion_planning/test_waypoints.py b/api/tests/opentrons/motion_planning/test_waypoints.py index 64a90200d62..9f0e99166d6 100644 --- a/api/tests/opentrons/motion_planning/test_waypoints.py +++ b/api/tests/opentrons/motion_planning/test_waypoints.py @@ -18,6 +18,7 @@ def test_get_waypoints_direct() -> None: origin=Point(1, 2, 3), dest=Point(1, 2, 4), move_type=MoveType.DIRECT, + min_travel_z=0, max_travel_z=100, ) diff --git a/api/tests/opentrons/protocol_engine/commands/test_move_to_coordinates.py b/api/tests/opentrons/protocol_engine/commands/test_move_to_coordinates.py index 1154021d0b9..9e12f06035d 100644 --- a/api/tests/opentrons/protocol_engine/commands/test_move_to_coordinates.py +++ b/api/tests/opentrons/protocol_engine/commands/test_move_to_coordinates.py @@ -1,8 +1,12 @@ """Test move-to-coordinates commands.""" -import pytest from decoy import Decoy -from opentrons.protocol_engine.types import DeckPoint +from opentrons.hardware_control import CriticalPoint, HardwareControlAPI +from opentrons.types import Mount, MountType, Point +from opentrons.motion_planning import Waypoint +from opentrons.protocol_engine.execution import MovementHandler +from opentrons.protocol_engine.state import StateView +from opentrons.protocol_engine.types import DeckPoint, LoadedPipette, PipetteName from opentrons.protocol_engine.commands.move_to_coordinates import ( MoveToCoordinatesParams, @@ -11,15 +15,86 @@ ) -async def test_move_to_coordinates_implementation(decoy: Decoy) -> None: - """A MoveRelative command should have an execution implementation.""" - subject = MoveToCoordinatesImplementation() - data = MoveToCoordinatesParams( +async def test_move_to_coordinates_implementation( + decoy: Decoy, + state_view: StateView, + hardware_api: HardwareControlAPI, + movement: MovementHandler, +) -> None: + """Test the `moveToCoordinates` implementation. + + It should: + + 1. Query the hardware controller for the given pipette's current position + and how high it can go with its current tip. + 2. Plan the movement, taking the above into account, plus the input parameters. + 3. Iterate through the waypoints of the movement. + """ + subject = MoveToCoordinatesImplementation( + state_view=state_view, + hardware_api=hardware_api, + movement=movement, + ) + + params = MoveToCoordinatesParams( pipetteId="pipette-id", coordinates=DeckPoint(x=1.11, y=2.22, z=3.33), - minimumZHeight=1000, + minimumZHeight=1234, forceDirect=True, ) - with pytest.raises(NotImplementedError): - assert await subject.execute(data) == MoveToCoordinatesResult() + mount_type = MountType.RIGHT + mount = Mount.RIGHT + + current_position = Point(4.44, 5.55, 6.66) + max_height = 5678 + + planned_waypoint_1 = Waypoint(position=Point(3, 1, 4), critical_point=None) + planned_waypoint_2 = Waypoint( + position=Point(1, 5, 9), critical_point=CriticalPoint.XY_CENTER + ) + + decoy.when(state_view.pipettes.get(params.pipetteId)).then_return( + LoadedPipette( + mount=mount_type, + id="loaded-pipette-id-should-not-matter", + pipetteName=PipetteName.P10_SINGLE, # Shouldn't matter. + ) + ) + + decoy.when( + await hardware_api.gantry_position(mount=mount, critical_point=None) + ).then_return(current_position) + + decoy.when( + hardware_api.get_instrument_max_height(mount=mount, critical_point=None) + ).then_return(max_height) + + decoy.when( + state_view.motion.get_movement_waypoints_to_coords( + origin=current_position, + dest=Point( + params.coordinates.x, params.coordinates.y, params.coordinates.z + ), + max_travel_z=max_height, + direct=params.forceDirect, + additional_min_travel_z=params.minimumZHeight, + ) + ).then_return([planned_waypoint_1, planned_waypoint_2]) + + result = await subject.execute(params=params) + + decoy.verify( + await hardware_api.move_to( + mount=mount, + abs_position=planned_waypoint_1.position, + critical_point=planned_waypoint_1.critical_point, + ), + await hardware_api.move_to( + mount=mount, + abs_position=planned_waypoint_2.position, + critical_point=planned_waypoint_2.critical_point, + ), + ) + + assert result == MoveToCoordinatesResult() diff --git a/api/tests/opentrons/protocol_engine/execution/test_movement_handler.py b/api/tests/opentrons/protocol_engine/execution/test_movement_handler.py index a9ebac72668..e2d93423fd8 100644 --- a/api/tests/opentrons/protocol_engine/execution/test_movement_handler.py +++ b/api/tests/opentrons/protocol_engine/execution/test_movement_handler.py @@ -116,7 +116,7 @@ async def test_move_to_well( ) decoy.when( - state_store.motion.get_movement_waypoints( + state_store.motion.get_movement_waypoints_to_well( origin=Point(1, 1, 1), origin_cp=CriticalPoint.FRONT_NOZZLE, max_travel_z=42.0, @@ -197,7 +197,7 @@ async def test_move_to_well_from_starting_location( ) decoy.when( - state_store.motion.get_movement_waypoints( + state_store.motion.get_movement_waypoints_to_well( current_well=current_well, origin=Point(1, 2, 5), origin_cp=CriticalPoint.XY_CENTER, diff --git a/api/tests/opentrons/protocol_engine/state/test_motion_view.py b/api/tests/opentrons/protocol_engine/state/test_motion_view.py index 8d0859da3e9..5ea1156cb5c 100644 --- a/api/tests/opentrons/protocol_engine/state/test_motion_view.py +++ b/api/tests/opentrons/protocol_engine/state/test_motion_view.py @@ -6,7 +6,7 @@ from opentrons.types import Point, MountType, DeckSlotName from opentrons.hardware_control.types import CriticalPoint -from opentrons.motion_planning import MoveType, get_waypoints +from opentrons import motion_planning from opentrons.protocol_engine import errors from opentrons.protocol_engine.types import ( @@ -25,24 +25,31 @@ @pytest.fixture -def module_view(decoy: Decoy) -> ModuleView: +def mock_module_view(decoy: Decoy) -> ModuleView: """Get a mock in the shape of a ModuleView.""" return decoy.mock(cls=ModuleView) +@pytest.fixture(autouse=True) +def patch_mock_get_waypoints(decoy: Decoy, monkeypatch: pytest.MonkeyPatch) -> None: + """Replace motion_planning.get_waypoints() with a mock.""" + mock_get_waypoints = decoy.mock(func=motion_planning.get_waypoints) + monkeypatch.setattr(motion_planning, "get_waypoints", mock_get_waypoints) + + @pytest.fixture def subject( labware_view: LabwareView, pipette_view: PipetteView, geometry_view: GeometryView, - module_view: ModuleView, + mock_module_view: ModuleView, ) -> MotionView: """Get a MotionView with its dependencies mocked out.""" return MotionView( labware_view=labware_view, pipette_view=pipette_view, geometry_view=geometry_view, - module_view=module_view, + module_view=mock_module_view, ) @@ -180,10 +187,10 @@ def test_get_pipette_location_override_current_location( @dataclass(frozen=True) class WaypointSpec: - """Spec data for testing the get_movement_waypoints selector.""" + """Spec data for testing the get_movement_waypoints_to_well selector.""" name: str - expected_move_type: MoveType + expected_move_type: motion_planning.MoveType pipette_id: str = "pipette-id" labware_id: str = "labware-id" well_name: str = "A1" @@ -198,19 +205,18 @@ class WaypointSpec: all_labware_z: Optional[float] = None max_travel_z: float = 50 should_dodge_thermocycler: bool = False - extra_waypoints: Sequence[Tuple[float, float]] = () + extra_waypoints: Sequence[Tuple[float, float]] = field(default_factory=list) -# TODO(mc, 2021-01-14): these tests probably need to be rethought; this fixture -# is impossible to reason with. The test is really just trying to be a collaborator -# test for the `get_waypoints` function, so we should rewrite it as such. +# TODO(mm, 2022-06-22): Break this test apart into the parts that are orthogonal +# (see this test's docstring) or refactor the subject to do less at once. @pytest.mark.parametrize( "spec", [ WaypointSpec( name="General arc if moving from unknown location", all_labware_z=20, - expected_move_type=MoveType.GENERAL_ARC, + expected_move_type=motion_planning.MoveType.GENERAL_ARC, ), WaypointSpec( name="General arc if moving from other labware", @@ -220,7 +226,7 @@ class WaypointSpec: well_name="A1", ), all_labware_z=20, - expected_move_type=MoveType.GENERAL_ARC, + expected_move_type=motion_planning.MoveType.GENERAL_ARC, ), WaypointSpec( name="In-labware arc if moving to same labware", @@ -230,7 +236,7 @@ class WaypointSpec: well_name="B2", ), labware_z=10, - expected_move_type=MoveType.IN_LABWARE_ARC, + expected_move_type=motion_planning.MoveType.IN_LABWARE_ARC, ), WaypointSpec( name="General arc if moving to same labware with different pipette", @@ -240,7 +246,7 @@ class WaypointSpec: well_name="A1", ), all_labware_z=20, - expected_move_type=MoveType.GENERAL_ARC, + expected_move_type=motion_planning.MoveType.GENERAL_ARC, ), WaypointSpec( name="Direct movement from well to same well", @@ -250,13 +256,13 @@ class WaypointSpec: well_name="A1", ), labware_z=10, - expected_move_type=MoveType.DIRECT, + expected_move_type=motion_planning.MoveType.DIRECT, ), WaypointSpec( name="General arc with XY_CENTER destination CP", has_center_multichannel_quirk=True, all_labware_z=20, - expected_move_type=MoveType.GENERAL_ARC, + expected_move_type=motion_planning.MoveType.GENERAL_ARC, expected_dest_cp=CriticalPoint.XY_CENTER, ), WaypointSpec( @@ -265,7 +271,7 @@ class WaypointSpec: well_location=WellLocation( origin=WellOrigin.TOP, offset=WellOffset(x=0, y=0, z=1) ), - expected_move_type=MoveType.GENERAL_ARC, + expected_move_type=motion_planning.MoveType.GENERAL_ARC, ), WaypointSpec( name="General arc with extra waypoints to dodge thermocycler", @@ -276,22 +282,50 @@ class WaypointSpec: ), all_labware_z=20, should_dodge_thermocycler=True, - expected_move_type=MoveType.GENERAL_ARC, + expected_move_type=motion_planning.MoveType.GENERAL_ARC, extra_waypoints=[(123, 456)], ) - # TODO(mc, 2021-01-08): add test for override current location + # TODO(mc, 2021-01-08): add test for override current location (current_well) ], ) -def test_get_movement_waypoints( +def test_get_movement_waypoints_to_well( decoy: Decoy, labware_view: LabwareView, pipette_view: PipetteView, geometry_view: GeometryView, - module_view: ModuleView, + mock_module_view: ModuleView, subject: MotionView, spec: WaypointSpec, ) -> None: - """It should calculate the correct set of waypoints for a move.""" + """It should call get_waypoints() with the correct args to move to a well. + + The arguments to get_waypoints() should be as follows: + + * move_type: + * DIRECT if moving within a single well. + * IN_LABWARE_ARC if going well-to-well in a single labware. + * GENERAL_ARC otherwise. + * origin: + Always passed through as-is from subject's arguments. + * origin_cp: + Always passed through as-is from subject's arguments. + * max_travel_z: + Always passed through as-is from subject's arguments. + * min_travel_z: + * Labware's highest Z if going well-to-well in a single labware. + * Doesn't matter if moving within a single well (because it'll be DIRECT). + * get_all_labware_highest_z() deck-wide safe height otherwise. + * dest: + Point calculated from subject's labware_id, well_name, and well_location + arguments. + * dest_cp: + None or XY_CENTER depending on the labware's centerMultichannelOnWells quirk. + * max_travel_z: + Always passed through as-is from subject's arguments. + * xy_waypoints: + * Center of slot 5 if source is known and path warrants a Thermocycler dodge. + * [] otherwise. + """ decoy.when( labware_view.get_has_quirk( spec.labware_id, @@ -334,7 +368,7 @@ def test_get_movement_waypoints( geometry_view.get_ancestor_slot_name(spec.location.labware_id) ).then_return(DeckSlotName.SLOT_1) decoy.when( - module_view.should_dodge_thermocycler( + mock_module_view.should_dodge_thermocycler( from_slot=DeckSlotName.SLOT_1, to_slot=DeckSlotName.SLOT_1 ) ).then_return(spec.should_dodge_thermocycler) @@ -346,7 +380,29 @@ def test_get_movement_waypoints( Point(x=spec.extra_waypoints[0][0], y=spec.extra_waypoints[0][1], z=123) ) - result = subject.get_movement_waypoints( + waypoints = [ + motion_planning.Waypoint( + position=Point(1, 2, 3), critical_point=CriticalPoint.XY_CENTER + ), + motion_planning.Waypoint( + position=Point(4, 5, 6), critical_point=CriticalPoint.MOUNT + ), + ] + + decoy.when( + motion_planning.get_waypoints( + move_type=spec.expected_move_type, + origin=spec.origin, + origin_cp=spec.origin_cp, + max_travel_z=spec.max_travel_z, + min_travel_z=min_travel_z, + dest=spec.dest, + dest_cp=spec.expected_dest_cp, + xy_waypoints=spec.extra_waypoints, + ) + ).then_return(waypoints) + + result = subject.get_movement_waypoints_to_well( pipette_id=spec.pipette_id, labware_id=spec.labware_id, well_name=spec.well_name, @@ -356,40 +412,162 @@ def test_get_movement_waypoints( max_travel_z=spec.max_travel_z, ) - expected = get_waypoints( - move_type=spec.expected_move_type, - origin=spec.origin, - origin_cp=spec.origin_cp, - max_travel_z=spec.max_travel_z, - min_travel_z=min_travel_z, - dest=spec.dest, - dest_cp=spec.expected_dest_cp, - xy_waypoints=spec.extra_waypoints, - ) + assert result == waypoints - assert result == expected - -def test_get_movement_waypoints_raises( +def test_get_movement_waypoints_to_well_raises( decoy: Decoy, pipette_view: PipetteView, geometry_view: GeometryView, subject: MotionView, ) -> None: - """It should raise FailedToPlanMoveError if get_waypoints raises.""" + """It should raise FailedToPlanMoveError if motion_planning.get_waypoints raises.""" + decoy.when( + geometry_view.get_well_position( + labware_id="labware-id", + well_name="A1", + well_location=None, + ) + ).then_return(Point(x=4, y=5, z=6)) decoy.when(pipette_view.get_current_well()).then_return(None) - decoy.when(geometry_view.get_well_position("labware-id", "A1", None)).then_return( - Point(4, 5, 6) + decoy.when(geometry_view.get_all_labware_highest_z()).then_return(456) + decoy.when( + # TODO(mm, 2022-06-22): We should use decoy.matchers.Anything() for all + # arguments. For some reason, Decoy does not match the call unless we + # specify concrete values for all (?) arguments? + motion_planning.get_waypoints( + Point(x=1, y=2, z=3), + Point(x=4, y=5, z=6), + max_travel_z=123, + min_travel_z=456, + ), + ignore_extra_args=True, + ).then_raise( + motion_planning.MotionPlanningError( + origin=Point(1, 2, 3), + dest=Point(1, 2, 3), + clearance=123, + min_travel_z=123, + max_travel_z=123, + message="oh the humanity", + ) ) - with pytest.raises(errors.FailedToPlanMoveError, match="out of bounds"): - subject.get_movement_waypoints( + with pytest.raises(errors.FailedToPlanMoveError, match="oh the humanity"): + subject.get_movement_waypoints_to_well( pipette_id="pipette-id", labware_id="labware-id", well_name="A1", well_location=None, origin=Point(1, 2, 3), origin_cp=None, - # this max_travel_z is too low and will induce failure - max_travel_z=1, + max_travel_z=123, + ) + + +@pytest.mark.parametrize( + ("direct", "expected_move_type"), + [ + (False, motion_planning.MoveType.GENERAL_ARC), + (True, motion_planning.MoveType.DIRECT), + ], +) +@pytest.mark.parametrize( + ("additional_min_travel_z", "all_labware_highest_z", "expected_min_travel_z"), + [ + (None, 100, 100), + (200, 100, 200), + (100, 200, 200), + (None, -100, -100), + (-200, -100, -100), + (-100, -200, -100), + ], +) +def test_get_movement_waypoints_to_coords( + decoy: Decoy, + geometry_view: GeometryView, + subject: MotionView, + direct: bool, + expected_move_type: motion_planning.MoveType, + additional_min_travel_z: float, + all_labware_highest_z: float, + expected_min_travel_z: float, +) -> None: + """It should call get_waypoints() with the correct args to move to coordinates.""" + origin = Point(1, 2, 3) + dest = Point(4, 5, 6) + max_travel_z = 789 + + decoy.when(geometry_view.get_all_labware_highest_z()).then_return( + all_labware_highest_z + ) + + waypoints = [ + motion_planning.Waypoint( + position=Point(1, 2, 3), critical_point=CriticalPoint.XY_CENTER + ), + motion_planning.Waypoint( + position=Point(4, 5, 6), critical_point=CriticalPoint.MOUNT + ), + ] + + decoy.when( + motion_planning.get_waypoints( + origin=origin, + origin_cp=None, + dest=dest, + dest_cp=None, + min_travel_z=expected_min_travel_z, + max_travel_z=max_travel_z, + move_type=expected_move_type, + ) + ).then_return(waypoints) + + result = subject.get_movement_waypoints_to_coords( + origin=origin, + dest=dest, + max_travel_z=max_travel_z, + direct=direct, + additional_min_travel_z=additional_min_travel_z, + ) + + assert result == waypoints + + +def test_get_movement_waypoints_to_coords_raises( + decoy: Decoy, + geometry_view: GeometryView, + subject: MotionView, +) -> None: + """It should raise FailedToPlanMoveError if motion_planning.get_waypoints raises.""" + decoy.when(geometry_view.get_all_labware_highest_z()).then_return(123) + decoy.when( + # TODO(mm, 2022-06-22): We should use decoy.matchers.Anything() for all + # arguments. For some reason, Decoy does not match the call unless we + # specify concrete values for all (?) arguments? + motion_planning.get_waypoints( + Point(x=1, y=2, z=3), + Point(x=1, y=2, z=3), + max_travel_z=123, + min_travel_z=123, + ), + ignore_extra_args=True, + ).then_raise( + motion_planning.MotionPlanningError( + origin=Point(1, 2, 3), + dest=Point(1, 2, 3), + clearance=123, + min_travel_z=123, + max_travel_z=123, + message="oh the humanity", + ) + ) + + with pytest.raises(errors.FailedToPlanMoveError, match="oh the humanity"): + subject.get_movement_waypoints_to_coords( + origin=Point(1, 2, 3), + dest=Point(1, 2, 3), + max_travel_z=123, + direct=False, + additional_min_travel_z=None, ) diff --git a/api/tests/opentrons/protocol_engine/state/test_pipette_store.py b/api/tests/opentrons/protocol_engine/state/test_pipette_store.py index 92336588d09..b15d1c91a07 100644 --- a/api/tests/opentrons/protocol_engine/state/test_pipette_store.py +++ b/api/tests/opentrons/protocol_engine/state/test_pipette_store.py @@ -4,7 +4,7 @@ from opentrons.types import MountType from opentrons.protocol_engine import commands as cmd -from opentrons.protocol_engine.types import PipetteName, LoadedPipette +from opentrons.protocol_engine.types import DeckPoint, LoadedPipette, PipetteName from opentrons.protocol_engine.actions import UpdateCommandAction from opentrons.protocol_engine.state.pipettes import ( PipetteStore, @@ -239,8 +239,34 @@ def test_movement_commands_update_current_well( assert subject.state.current_well == expected_location -def test_home_clears_current_well(subject: PipetteStore) -> None: - """It clear the last accessed well with a home command.""" +@pytest.mark.parametrize( + "command", + [ + cmd.Home( + id="command-id-2", + key="command-key-2", + status=cmd.CommandStatus.SUCCEEDED, + createdAt=datetime(year=2021, month=1, day=1), + params=cmd.HomeParams(), + result=cmd.HomeResult(), + ), + cmd.MoveToCoordinates( + id="command-id-2", + key="command-key-2", + status=cmd.CommandStatus.SUCCEEDED, + createdAt=datetime(year=2021, month=1, day=1), + params=cmd.MoveToCoordinatesParams( + pipetteId="pipette-id", + coordinates=DeckPoint(x=1.1, y=2.2, z=3.3), + ), + result=cmd.MoveToCoordinatesResult(), + ), + ], +) +def test_movement_commands_without_well_clear_current_well( + subject: PipetteStore, command: cmd.Command +) -> None: + """Commands that make the current well unknown should clear the current well.""" load_pipette_command = create_load_pipette_command( pipette_id="pipette-id", pipette_name=PipetteName.P300_SINGLE, @@ -251,18 +277,10 @@ def test_home_clears_current_well(subject: PipetteStore) -> None: labware_id="labware-id", well_name="well-name", ) - home_command = cmd.Home( - id="command-id-2", - key="command-key-2", - status=cmd.CommandStatus.SUCCEEDED, - createdAt=datetime(year=2021, month=1, day=1), - params=cmd.HomeParams(), - result=cmd.HomeResult(), - ) subject.handle_action(UpdateCommandAction(command=load_pipette_command)) subject.handle_action(UpdateCommandAction(command=move_command)) - subject.handle_action(UpdateCommandAction(command=home_command)) + subject.handle_action(UpdateCommandAction(command=command)) assert subject.state.current_well is None diff --git a/api/tests/opentrons/protocol_runner/test_json_command_translator.py b/api/tests/opentrons/protocol_runner/test_json_command_translator.py index 43ab3a6ca7a..4ecb5c5225f 100644 --- a/api/tests/opentrons/protocol_runner/test_json_command_translator.py +++ b/api/tests/opentrons/protocol_runner/test_json_command_translator.py @@ -19,6 +19,7 @@ from opentrons.protocol_runner.json_command_translator import JsonCommandTranslator from opentrons.protocol_engine import ( commands as pe_commands, + DeckPoint, DeckSlotLocation, PipetteName, WellLocation, @@ -268,6 +269,25 @@ ) ), ), + ( + protocol_schema_v6.Command( + commandType="moveToCoordinates", + params=protocol_schema_v6.Params( + pipetteId="pipette-id-abc123", + coordinates=protocol_schema_v6.OffsetVector(x=1.1, y=2.2, z=3.3), + minimumZHeight=123.4, + forceDirect=True, + ), + ), + pe_commands.MoveToCoordinatesCreate( + params=pe_commands.MoveToCoordinatesParams( + pipetteId="pipette-id-abc123", + coordinates=DeckPoint(x=1.1, y=2.2, z=3.3), + minimumZHeight=123.4, + forceDirect=True, + ) + ), + ), ] diff --git a/robot-server/tests/integration/http_api/protocols/test_v6_json_upload.tavern.yaml b/robot-server/tests/integration/http_api/protocols/test_v6_json_upload.tavern.yaml index bc8d663f575..f8994793f7c 100644 --- a/robot-server/tests/integration/http_api/protocols/test_v6_json_upload.tavern.yaml +++ b/robot-server/tests/integration/http_api/protocols/test_v6_json_upload.tavern.yaml @@ -405,6 +405,22 @@ stages: result: {} startedAt: !anystr completedAt: !anystr + - id: !anystr + createdAt: !anystr + commandType: moveToCoordinates + key: !anystr + status: succeeded + params: + pipetteId: pipetteId + coordinates: + x: 0 + y: 0 + z: 0 + minimumZHeight: 35 + forceDirect: true + result: {} + startedAt: !anystr + completedAt: !anystr - id: !anystr createdAt: !anystr commandType: moveRelative