From ff3dc71721b8aaf64d2138c90041725b13c380c3 Mon Sep 17 00:00:00 2001 From: Seth Foster Date: Fri, 8 Jul 2022 11:12:48 -0400 Subject: [PATCH] refactor(api): ot3: add gripper critical points (#11024) For now, add some gripper-specific critical points to the enum that the gripper handles, and use them to calculate position offsets for motion. JAW_CENTER is the default, and will be what gets used in most calls when a gripper is present; the calibration points will be used for calibration. These new critical points will cause an error if used with pipettes, and the pipette critical points will cause an error if used with the gripper. It's no longer valid to move the mount critical point for the gripper, since that criticalpoint is attached directly to the carriage. These values are tested on a proto build. --- api/src/opentrons/config/defaults_ot3.py | 3 +- .../hardware_control/instruments/gripper.py | 44 +++++++- .../instruments/gripper_handler.py | 20 ++-- .../hardware_control/instruments/pipette.py | 16 ++- api/src/opentrons/hardware_control/ot3api.py | 3 +- api/src/opentrons/hardware_control/types.py | 29 +++++ api/tests/opentrons/config/ot3_settings.py | 10 +- .../hardware_control/test_gripper.py | 27 ++++- .../opentrons/hardware_control/test_moves.py | 67 +++++------ .../hardware_control/test_ot3_api.py | 105 +++++++++++++++++- .../gripper/definitions/1/gripperV1.json | 14 +-- shared-data/python/Makefile | 2 +- 12 files changed, 258 insertions(+), 82 deletions(-) diff --git a/api/src/opentrons/config/defaults_ot3.py b/api/src/opentrons/config/defaults_ot3.py index 68866c12e70..7b267850e6e 100644 --- a/api/src/opentrons/config/defaults_ot3.py +++ b/api/src/opentrons/config/defaults_ot3.py @@ -56,7 +56,7 @@ DEFAULT_CARRIAGE_OFFSET: Final[Offset] = (436.605, 484.975, 233.475) DEFAULT_LEFT_MOUNT_OFFSET: Final[Offset] = (-21.0, -63.05, 256.175) DEFAULT_RIGHT_MOUNT_OFFSET: Final[Offset] = (33, -63.05, 256.175) -DEFAULT_GRIPPER_MOUNT_OFFSET: Final[Offset] = (82.15, 16, 92.55) +DEFAULT_GRIPPER_MOUNT_OFFSET: Final[Offset] = (82.15, -16, 92.55) DEFAULT_Z_RETRACT_DISTANCE: Final = 2 DEFAULT_MAX_SPEEDS: Final[ByGantryLoad[Dict[OT3AxisKind, float]]] = ByGantryLoad( @@ -225,6 +225,7 @@ two_low_throughput={ OT3AxisKind.X: 1.0, OT3AxisKind.Y: 1.0, + OT3AxisKind.Z: 1.4, }, gripper={ OT3AxisKind.Z: 1.4, diff --git a/api/src/opentrons/hardware_control/instruments/gripper.py b/api/src/opentrons/hardware_control/instruments/gripper.py index 19bf6f25d75..d09c8dc3be8 100644 --- a/api/src/opentrons/hardware_control/instruments/gripper.py +++ b/api/src/opentrons/hardware_control/instruments/gripper.py @@ -9,7 +9,11 @@ from opentrons.types import Point from opentrons.calibration_storage.types import GripperCalibrationOffset from opentrons.config import gripper_config -from opentrons.hardware_control.types import CriticalPoint, GripperJawState +from opentrons.hardware_control.types import ( + CriticalPoint, + GripperJawState, + InvalidMoveError, +) from .instrument_abc import AbstractInstrument from opentrons.hardware_control.dev_types import AttachedGripper, GripperDict @@ -37,7 +41,24 @@ def __init__( self._config = config self._name = self._config.name self._model = self._config.model + base_offset = Point(*self._config.base_offset_from_mount) + self._jaw_center_offset = ( + Point(*self._config.jaw_center_offset_from_base) + base_offset + ) + #: the distance between the gripper mount and the jaw center at home + self._front_calibration_pin_offset = ( + Point(*self._config.pin_one_offset_from_base) + base_offset + ) + #: the distance between the gripper mount and the front calibration pin + #: at home + self._back_calibration_pin_offset = ( + Point(*self._config.pin_two_offset_from_base) + base_offset + ) + #: the distance between the gripper mount and the back calibration pin + #: at home self._calibration_offset = gripper_cal_offset + #: The output value of calibration - the additional vector added into + #: the critical point geometry based on gripper mount calibration self._gripper_id = gripper_id self._state = GripperJawState.UNHOMED self._log = mod_log.getChild(self._gripper_id) @@ -80,12 +101,23 @@ def update_calibration_offset(self, cal_offset: GripperCalibrationOffset) -> Non def critical_point(self, cp_override: Optional[CriticalPoint] = None) -> Point: """ - The vector from the gripper's origin to its critical point. The - critical point for a pipette is the end of the nozzle if no tip is - attached, or the end of the tip if a tip is attached. + The vector from the gripper mount to the critical point, which is selectable + between the center of the gripper engagement volume and the calibration pins. """ - # TODO: add critical point implementation - return Point(0, 0, 0) + if cp_override == CriticalPoint.GRIPPER_FRONT_CALIBRATION_PIN: + return self._front_calibration_pin_offset + Point( + *self._calibration_offset.offset + ) + elif cp_override == CriticalPoint.GRIPPER_BACK_CALIBRATION_PIN: + return self._back_calibration_pin_offset + Point( + *self._calibration_offset.offset + ) + elif cp_override == CriticalPoint.GRIPPER_JAW_CENTER or not cp_override: + return self._jaw_center_offset + Point(*self._calibration_offset.offset) + else: + raise InvalidMoveError( + f"Critical point {cp_override.name} is not valid for a gripper" + ) def duty_cycle_by_force(self, newton: float) -> float: return gripper_config.piecewise_force_conversion( diff --git a/api/src/opentrons/hardware_control/instruments/gripper_handler.py b/api/src/opentrons/hardware_control/instruments/gripper_handler.py index b6724d24445..f33f48a4a9b 100644 --- a/api/src/opentrons/hardware_control/instruments/gripper_handler.py +++ b/api/src/opentrons/hardware_control/instruments/gripper_handler.py @@ -3,16 +3,15 @@ from opentrons.types import Point from opentrons.hardware_control.robot_calibration import load_gripper_calibration_offset from opentrons.hardware_control.dev_types import GripperDict -from opentrons.hardware_control.types import CriticalPoint, GripperJawState +from opentrons.hardware_control.types import ( + CriticalPoint, + GripperJawState, + InvalidMoveError, + GripperNotAttachedError, +) from .gripper import Gripper -class GripperNotAttachedError(Exception): - """An error raised if a gripper is accessed that is not attached""" - - pass - - class GripError(Exception): """An error raised if a gripper action is blocked""" @@ -63,9 +62,10 @@ def gripper(self, gripper: Optional[Gripper] = None) -> None: def get_critical_point(self, cp_override: Optional[CriticalPoint] = None) -> Point: if not self._gripper: - return Point(0, 0, 0) - else: - return self._gripper.critical_point(cp_override) + raise GripperNotAttachedError() + if cp_override == CriticalPoint.MOUNT: + raise InvalidMoveError("The gripper mount may not be moved directly.") + return self._gripper.critical_point(cp_override) def get_gripper_dict(self) -> Optional[GripperDict]: if not self._gripper: diff --git a/api/src/opentrons/hardware_control/instruments/pipette.py b/api/src/opentrons/hardware_control/instruments/pipette.py index 3ee84ab538f..0052771193e 100644 --- a/api/src/opentrons/hardware_control/instruments/pipette.py +++ b/api/src/opentrons/hardware_control/instruments/pipette.py @@ -16,7 +16,12 @@ from opentrons.config.types import RobotConfig, OT3Config from opentrons.drivers.types import MoveSplit from .instrument_abc import AbstractInstrument -from opentrons.hardware_control.types import CriticalPoint, BoardRevision, OT3AxisKind +from opentrons.hardware_control.types import ( + CriticalPoint, + BoardRevision, + OT3AxisKind, + InvalidMoveError, +) if TYPE_CHECKING: @@ -146,6 +151,15 @@ def critical_point(self, cp_override: Optional[CriticalPoint] = None) -> Point: instr = Point(*self._pipette_offset.offset) offsets = self.nozzle_offset + if cp_override in [ + CriticalPoint.GRIPPER_JAW_CENTER, + CriticalPoint.GRIPPER_FRONT_CALIBRATION_PIN, + CriticalPoint.GRIPPER_BACK_CALIBRATION_PIN, + ]: + raise InvalidMoveError( + f"Critical point {cp_override.name} is not valid for a pipette" + ) + if not self.has_tip or cp_override == CriticalPoint.NOZZLE: cp_type = CriticalPoint.NOZZLE tip_length = 0.0 diff --git a/api/src/opentrons/hardware_control/ot3api.py b/api/src/opentrons/hardware_control/ot3api.py index a8cd163e7fe..ef5f3d42b88 100644 --- a/api/src/opentrons/hardware_control/ot3api.py +++ b/api/src/opentrons/hardware_control/ot3api.py @@ -64,6 +64,7 @@ OT3AxisMap, OT3SubSystem, GripperJawState, + GripperNotAttachedError, ) from . import modules from .robot_calibration import ( @@ -76,7 +77,7 @@ from .protocols import HardwareControlAPI from .instruments.pipette_handler import OT3PipetteHandler, InstrumentsByMount -from .instruments.gripper_handler import GripperHandler, GripperNotAttachedError +from .instruments.gripper_handler import GripperHandler from .motion_utilities import ( target_position_from_absolute, target_position_from_relative, diff --git a/api/src/opentrons/hardware_control/types.py b/api/src/opentrons/hardware_control/types.py index f300e3778ff..c07809c2ff9 100644 --- a/api/src/opentrons/hardware_control/types.py +++ b/api/src/opentrons/hardware_control/types.py @@ -387,6 +387,25 @@ class CriticalPoint(enum.Enum): Only relevant when a multichannel pipette is present. """ + GRIPPER_JAW_CENTER = enum.auto() + """ + The center of the gripper jaw engagement zone, such that if this critical + point is moved to the center of a labware the gripper will be ready to + grip it. + """ + + GRIPPER_FRONT_CALIBRATION_PIN = enum.auto() + """ + The center of the bottom face of a calibration pin inserted in the gripper's + front calibration pin slot. + """ + + GRIPPER_BACK_CALIBRATION_PIN = enum.auto() + """ + The center of the bottom face of a calibration pin inserted in the gripper's + back calibration pin slot. + """ + class ExecutionState(enum.Enum): RUNNING = enum.auto() @@ -462,3 +481,13 @@ class GripperJawState(enum.Enum): @property def ready_for_grip(self) -> bool: return self in [GripperJawState.HOMED_READY, GripperJawState.HOLDING_OPENED] + + +class InvalidMoveError(ValueError): + pass + + +class GripperNotAttachedError(Exception): + """An error raised if a gripper is accessed that is not attached""" + + pass diff --git a/api/tests/opentrons/config/ot3_settings.py b/api/tests/opentrons/config/ot3_settings.py index 657c25ade5f..990a0f12ddc 100644 --- a/api/tests/opentrons/config/ot3_settings.py +++ b/api/tests/opentrons/config/ot3_settings.py @@ -129,10 +129,7 @@ "Z": 0.7, "P": 0.8, }, - "two_low_throughput": { - "X": 0.7, - "Y": 0.7, - }, + "two_low_throughput": {"X": 0.7, "Y": 0.7, "Z": 0.6}, "gripper": { "Z": 0.7, }, @@ -156,10 +153,7 @@ "Z": 0.4, "P": 2.0, }, - "two_low_throughput": { - "X": 9, - "Y": 0.1, - }, + "two_low_throughput": {"X": 9, "Y": 0.1, "Z": 0.6}, "gripper": { "Z": 10, }, diff --git a/api/tests/opentrons/hardware_control/test_gripper.py b/api/tests/opentrons/hardware_control/test_gripper.py index 1d41aeab9d4..2fe06a663c5 100644 --- a/api/tests/opentrons/hardware_control/test_gripper.py +++ b/api/tests/opentrons/hardware_control/test_gripper.py @@ -1,6 +1,10 @@ +from typing import Optional, Callable +import pytest + from opentrons.types import Point from opentrons.hardware_control.robot_calibration import load_gripper_calibration_offset from opentrons.hardware_control.instruments import gripper +from opentrons.hardware_control.types import CriticalPoint from opentrons.calibration_storage.delete import clear_gripper_calibration_offsets from opentrons.config import gripper_config from opentrons_shared_data.gripper.dev_types import GripperModel @@ -26,10 +30,27 @@ def test_id_get_added_to_dict(): assert gripr.as_dict()["gripper_id"] == "fakeid123" -def test_critical_point(): +@pytest.mark.parametrize( + "override,result_accessor", + [ + (None, lambda g: g._jaw_center_offset), + (CriticalPoint.GRIPPER_JAW_CENTER, lambda g: g._jaw_center_offset), + ( + CriticalPoint.GRIPPER_FRONT_CALIBRATION_PIN, + lambda g: g._front_calibration_pin_offset, + ), + ( + CriticalPoint.GRIPPER_BACK_CALIBRATION_PIN, + lambda g: g._back_calibration_pin_offset, + ), + ], +) +def test_critical_point( + override: Optional[CriticalPoint], + result_accessor: Callable[[gripper.Gripper], Point], +): gripr = gripper.Gripper(fake_gripper_conf, FAKE_OFFSET, "fakeid123") - # TODO: update test when critical_point() is fully implemented - assert gripr.critical_point() == Point(0, 0, 0) + assert gripr.critical_point(override) == result_accessor(gripr) def test_load_gripper_cal_offset(): diff --git a/api/tests/opentrons/hardware_control/test_moves.py b/api/tests/opentrons/hardware_control/test_moves.py index 698bc3fa2da..8ba2c130b6c 100644 --- a/api/tests/opentrons/hardware_control/test_moves.py +++ b/api/tests/opentrons/hardware_control/test_moves.py @@ -14,9 +14,10 @@ OutOfBoundsMove, MotionChecks, MustHomeError, + InvalidMoveError, ) from opentrons.hardware_control.robot_calibration import RobotCalibration -from opentrons.hardware_control.types import OT3Axis, OT3Mount +from opentrons.hardware_control.types import OT3Axis async def test_controller_must_home(hardware_api): @@ -59,16 +60,6 @@ async def test_retract(hardware_api): } -@pytest.fixture -async def mock_backend_move(ot3_hardware): - with mock.patch.object( - ot3_hardware.managed_obj._backend, - "move", - mock.AsyncMock(spec=ot3_hardware.managed_obj._backend.move), - ) as mock_move: - yield mock_move - - @pytest.fixture def mock_home(ot3_hardware): with mock.patch.object(ot3_hardware._backend, "home") as mock_home: @@ -179,6 +170,29 @@ async def test_mount_offset_applied(hardware_api, is_robot): assert hardware_api._current_position == target_position +@pytest.mark.parametrize( + "critical_point", + [ + CriticalPoint.GRIPPER_JAW_CENTER, + CriticalPoint.GRIPPER_FRONT_CALIBRATION_PIN, + CriticalPoint.GRIPPER_BACK_CALIBRATION_PIN, + ], +) +async def test_gripper_critical_points_fail_on_pipettes( + hardware_api, is_robot, critical_point +): + await hardware_api.home() + hardware_api._backend._attached_instruments = { + types.Mount.LEFT: {"model": None, "id": None}, + types.Mount.RIGHT: {"model": "p10_single_v1", "id": "testyness"}, + } + await hardware_api.cache_instruments() + with pytest.raises(InvalidMoveError): + await hardware_api.move_to( + types.Mount.RIGHT, types.Point(0, 0, 0), critical_point=critical_point + ) + + async def test_critical_point_applied(hardware_api, monkeypatch, is_robot): await hardware_api.home() hardware_api._backend._attached_instruments = { @@ -516,34 +530,3 @@ async def test_current_position_homing_failures(hardware_api): mount=types.Mount.RIGHT, fail_on_not_homed=True, ) - - -async def test_gripper_move_to(ot3_hardware, mock_backend_move): - # Moving the gripper should, well, work - await ot3_hardware.move_to(OT3Mount.GRIPPER, types.Point(0, 0, 0)) - origin, moves = mock_backend_move.call_args_list[0][0] - # The moves that it emits should move only x, y, and the gripper z - assert origin == { - OT3Axis.X: 0, - OT3Axis.Y: 0, - OT3Axis.Z_L: 0, - OT3Axis.Z_R: 0, - OT3Axis.P_L: 0, - OT3Axis.P_R: 0, - OT3Axis.Z_G: 0, - OT3Axis.G: 0, - } - for move in moves: - assert list(sorted(move.unit_vector.keys(), key=lambda elem: elem.value)) == [ - OT3Axis.X, - OT3Axis.Y, - OT3Axis.Z_G, - ] - - -async def test_gripper_position(ot3_hardware): - await ot3_hardware.home() - position = await ot3_hardware.gantry_position(OT3Mount.GRIPPER) - assert position == types.Point(*ot3_hardware.config.carriage_offset) + types.Point( - *ot3_hardware.config.gripper_mount_offset - ) diff --git a/api/tests/opentrons/hardware_control/test_ot3_api.py b/api/tests/opentrons/hardware_control/test_ot3_api.py index ff4f972711b..91dfd7e126e 100644 --- a/api/tests/opentrons/hardware_control/test_ot3_api.py +++ b/api/tests/opentrons/hardware_control/test_ot3_api.py @@ -11,10 +11,15 @@ AttachedGripper, ) from opentrons.hardware_control.instruments.gripper_handler import ( - GripperNotAttachedError, GripError, ) -from opentrons.hardware_control.types import OT3Mount, OT3Axis +from opentrons.hardware_control.types import ( + OT3Mount, + OT3Axis, + GripperNotAttachedError, + InvalidMoveError, + CriticalPoint, +) from opentrons.hardware_control.ot3api import OT3API from opentrons.hardware_control import ThreadManager from opentrons.hardware_control.backends.ot3utils import axis_to_node @@ -86,6 +91,16 @@ def mock_ungrip(ot3_hardware: ThreadManager[OT3API]) -> Iterator[AsyncMock]: yield mock_move +@pytest.fixture +async def mock_backend_move(ot3_hardware: ThreadManager[OT3API]) -> Iterator[AsyncMock]: + with patch.object( + ot3_hardware.managed_obj._backend, + "move", + AsyncMock(spec=ot3_hardware.managed_obj._backend.move), + ) as mock_move: + yield mock_move + + @pytest.mark.parametrize( "attached,load", ( @@ -381,3 +396,89 @@ async def test_gripper_action( await ot3_hardware.ungrip() mock_ungrip.assert_called_once() + + +async def test_gripper_move_fails_with_no_gripper( + ot3_hardware: ThreadManager[OT3API], +) -> None: + assert not ot3_hardware._gripper_handler.gripper + with pytest.raises(GripperNotAttachedError): + await ot3_hardware.move_to(OT3Mount.GRIPPER, Point(0, 0, 0)) + + +async def test_gripper_mount_not_movable( + ot3_hardware: ThreadManager[OT3API], +) -> None: + gripper_config = gc.load(GripperModel.V1, "g12345") + instr_data = AttachedGripper(config=gripper_config, id="g12345") + await ot3_hardware.cache_gripper(instr_data) + assert ot3_hardware._gripper_handler.gripper + with pytest.raises(InvalidMoveError): + await ot3_hardware.move_to( + OT3Mount.GRIPPER, Point(0, 0, 0), critical_point=CriticalPoint.MOUNT + ) + + +@pytest.mark.parametrize( + "critical_point", + [ + CriticalPoint.NOZZLE, + CriticalPoint.TIP, + CriticalPoint.XY_CENTER, + CriticalPoint.FRONT_NOZZLE, + ], +) +async def test_gripper_fails_for_pipette_cps( + ot3_hardware: ThreadManager[OT3API], critical_point: CriticalPoint +) -> None: + gripper_config = gc.load(GripperModel.V1, "g12345") + instr_data = AttachedGripper(config=gripper_config, id="g12345") + await ot3_hardware.cache_gripper(instr_data) + assert ot3_hardware._gripper_handler.gripper + with pytest.raises(InvalidMoveError): + await ot3_hardware.move_to( + OT3Mount.GRIPPER, Point(0, 0, 0), critical_point=critical_point + ) + + +async def test_gripper_position(ot3_hardware: ThreadManager[OT3API]): + gripper_config = gc.load(GripperModel.V1, "g12345") + instr_data = AttachedGripper(config=gripper_config, id="g12345") + await ot3_hardware.cache_gripper(instr_data) + await ot3_hardware.home() + position = await ot3_hardware.gantry_position(OT3Mount.GRIPPER) + assert ( + position + == Point(*ot3_hardware.config.carriage_offset) + + Point(*ot3_hardware.config.gripper_mount_offset) + + ot3_hardware._gripper_handler.gripper._jaw_center_offset + ) + + +async def test_gripper_move_to( + ot3_hardware: ThreadManager[OT3API], mock_backend_move: AsyncMock +): + # Moving the gripper should, well, work + gripper_config = gc.load(GripperModel.V1, "g12345") + instr_data = AttachedGripper(config=gripper_config, id="g12345") + await ot3_hardware.cache_gripper(instr_data) + + await ot3_hardware.move_to(OT3Mount.GRIPPER, Point(0, 0, 0)) + origin, moves = mock_backend_move.call_args_list[0][0] + # The moves that it emits should move only x, y, and the gripper z + assert origin == { + OT3Axis.X: 0, + OT3Axis.Y: 0, + OT3Axis.Z_L: 0, + OT3Axis.Z_R: 0, + OT3Axis.P_L: 0, + OT3Axis.P_R: 0, + OT3Axis.Z_G: 0, + OT3Axis.G: 0, + } + for move in moves: + assert list(sorted(move.unit_vector.keys(), key=lambda elem: elem.value)) == [ + OT3Axis.X, + OT3Axis.Y, + OT3Axis.Z_G, + ] diff --git a/shared-data/gripper/definitions/1/gripperV1.json b/shared-data/gripper/definitions/1/gripperV1.json index 838cd58c014..7880fb51eb9 100644 --- a/shared-data/gripper/definitions/1/gripperV1.json +++ b/shared-data/gripper/definitions/1/gripperV1.json @@ -39,23 +39,23 @@ ], "baseOffsetFromMount": { "x": 6.775, - "y": 87.325, - "z": 32.05 + "y": -87.325, + "z": -32.05 }, "jawCenterOffsetFromBase": { - "x": 8.5, + "x": -8.5, "y": 2.5, - "z": 86 + "z": -86 }, "pinOneOffsetFromBase": { "x": 23, - "y": 73.37920159, - "z": 95 + "y": -73.37920159, + "z": -95 }, "pinTwoOffsetFromBase": { "x": 23, "y": 78.37920159, - "z": 95 + "z": -95 }, "quirks": [] } diff --git a/shared-data/python/Makefile b/shared-data/python/Makefile index fd49c55f858..734564400c6 100644 --- a/shared-data/python/Makefile +++ b/shared-data/python/Makefile @@ -86,7 +86,7 @@ $(wheel_file): setup.py $(py_sources) $(json_sources) $(SHX) ls $(BUILD_DIR) -$(sdist_file): setup.py $(py_sources) +$(sdist_file): setup.py $(py_sources) $(json_sources) $(SHX) mkdir -p build $(python) setup.py sdist $(SHX) ls $(BUILD_DIR)