From 5a82384d1f15b3e9b828586f5489e62e5a87d610 Mon Sep 17 00:00:00 2001 From: Seth Foster Date: Fri, 18 Sep 2020 11:34:59 -0400 Subject: [PATCH] fix(api): limit calibration jogs when going upward (#6565) Changelog: - better optional bounds checking for _move Refactor the bounds checking done in _move to a new function in utils to cut down on the size of the function a bit and add independent testability. - allow bounds checking in move_rel Use the new functionality to optionally check bounds in move_rel (by passing along the checker spec to _move). - check high bounds in jog, swallow errors When we jog during labware calibration, it's possible to trigger hard limits, most commonly in +z. We should catch and prevent these, since hard limits mess up the positioning state. But we have no way to actually display errors during jog actions, so we just swallow the error at the jog level. So, if you jog too far, the jog just doesn't do anything. Closes #6562 --- api/src/opentrons/api/calibration.py | 9 ++- api/src/opentrons/hardware_control/api.py | 43 +++++------- .../opentrons/hardware_control/controller.py | 4 +- .../opentrons/hardware_control/simulator.py | 4 +- api/src/opentrons/hardware_control/types.py | 19 ++++++ api/src/opentrons/hardware_control/util.py | 47 ++++++++++++- api/tests/opentrons/api/test_calibration.py | 3 +- .../opentrons/hardware_control/test_moves.py | 67 +++---------------- .../opentrons/hardware_control/test_util.py | 33 ++++++++- 9 files changed, 136 insertions(+), 93 deletions(-) diff --git a/api/src/opentrons/api/calibration.py b/api/src/opentrons/api/calibration.py index 5394018c091..77b94b8b227 100755 --- a/api/src/opentrons/api/calibration.py +++ b/api/src/opentrons/api/calibration.py @@ -9,6 +9,7 @@ from opentrons.types import Point, Mount, Location from opentrons.protocol_api import labware from opentrons.hardware_control import CriticalPoint, ThreadedAsyncLock +from opentrons.hardware_control.types import OutOfBoundsMove, MotionChecks from .models import Container from .util import robot_is_busy, RobotBusy @@ -236,8 +237,12 @@ def jog(self, instrument, distance, axis): instrument.name, distance, axis)) self._set_state('moving') if instrument._context: - self._hardware.move_rel( - Mount[inst.mount.upper()], Point(**{axis: distance})) + try: + self._hardware.move_rel( + Mount[inst.mount.upper()], Point(**{axis: distance}), + check_bounds=MotionChecks.HIGH) + except OutOfBoundsMove: + log.exception('Out of bounds jog') else: calibration_functions.jog_instrument( instrument=inst, diff --git a/api/src/opentrons/hardware_control/api.py b/api/src/opentrons/hardware_control/api.py index 2a8d7465ea5..21b6e590505 100644 --- a/api/src/opentrons/hardware_control/api.py +++ b/api/src/opentrons/hardware_control/api.py @@ -13,7 +13,8 @@ from opentrons.config import ( robot_configs, feature_flags as ff) -from .util import use_or_initialize_loop, DeckTransformState +from .util import ( + use_or_initialize_loop, DeckTransformState, check_motion_bounds) from .pipette import ( Pipette, generate_hardware_configs, load_from_config_and_check_skip) from .controller import Controller @@ -25,7 +26,8 @@ from .types import (Axis, HardwareAPILike, CriticalPoint, MustHomeError, NoTipAttachedError, DoorState, DoorStateNotification, PipettePair, TipAttachedError, - HardwareAction, PairedPipetteConfigValueError) + HardwareAction, PairedPipetteConfigValueError, + MotionChecks) from . import modules, robot_calibration as rb_cal if TYPE_CHECKING: @@ -913,7 +915,8 @@ async def move_to( async def move_rel(self, mount: Union[top_types.Mount, PipettePair], delta: top_types.Point, speed: float = None, - max_speeds: Dict[Axis, float] = None): + max_speeds: Dict[Axis, float] = None, + check_bounds: MotionChecks = MotionChecks.NONE): """ Move the critical point of the specified mount by a specified displacement in a specified direction, at the specified speed. 'speed' sets the speed of all axes to the given value. So, if multiple @@ -952,7 +955,8 @@ async def move_rel(self, mount: Union[top_types.Mount, PipettePair], await self._cache_and_maybe_retract_mount(primary_mount) await self._move( target_position, speed=speed, - max_speeds=max_speeds, secondary_z=secondary_z) + max_speeds=max_speeds, secondary_z=secondary_z, + check_bounds=check_bounds) async def _cache_and_maybe_retract_mount(self, mount: top_types.Mount): """ Retract the 'other' mount if necessary @@ -1036,7 +1040,8 @@ async def _move(self, target_position: 'OrderedDict[Axis, float]', speed: float = None, home_flagged_axes: bool = True, max_speeds: Dict[Axis, float] = None, acquire_lock: bool = True, - secondary_z: Axis = None): + secondary_z: Axis = None, + check_bounds: MotionChecks = MotionChecks.NONE): """ Worker function to apply robot motion. Robot motion means the kind of motions that are relevant to the robot, @@ -1070,7 +1075,6 @@ async def _move(self, target_position: 'OrderedDict[Axis, float]', target_position)) raise ValueError("Moves must specify either exactly an " "x, y, and (z or a) or none of them") - primary_transformed, secondary_transformed =\ self._get_transformed(to_transform_primary, to_transform_secondary) transformed = (*primary_transformed, secondary_transformed[2]) @@ -1080,25 +1084,14 @@ async def _move(self, target_position: 'OrderedDict[Axis, float]', # fuse the specified axes and the transformed values back together. # While we do this iteration, we’ll also check axis bounds. bounds = self._backend.axis_bounds - for idx, ax in enumerate(target_position.keys()): - if ax in Axis.gantry_axes(): - smoothie_pos[ax.name] = transformed[idx] - if smoothie_pos[ax.name] < bounds[ax.name][0]\ - or smoothie_pos[ax.name] > bounds[ax.name][1]: - deck_mins = self._deck_from_smoothie({ax: bound[0] - for ax, bound - in bounds.items()}) - deck_max = self._deck_from_smoothie({ax: bound[1] - for ax, bound - in bounds.items()}) - self._log.warning( - "Out of bounds move: {}={} (transformed: {}) not in" - "limits ({}, {}) (transformed: ({}, {})" - .format(ax.name, - target_position[ax], - smoothie_pos[ax.name], - deck_mins[ax], deck_max[ax], - bounds[ax.name][0], bounds[ax.name][1])) + to_check = {ax: transformed[idx] + for idx, ax in enumerate(target_position.keys()) + if ax in Axis.gantry_axes()} + check_motion_bounds( + to_check, + target_position, + bounds, check_bounds) + smoothie_pos.update({ax.name: pos for ax, pos in to_check.items()}) checked_maxes = max_speeds or {} str_maxes = {ax.name: val for ax, val in checked_maxes.items()} async with contextlib.AsyncExitStack() as stack: diff --git a/api/src/opentrons/hardware_control/controller.py b/api/src/opentrons/hardware_control/controller.py index 599a5277856..e413f536203 100644 --- a/api/src/opentrons/hardware_control/controller.py +++ b/api/src/opentrons/hardware_control/controller.py @@ -249,9 +249,9 @@ async def connect(self, port: str = None): await self.update_fw_version() @property - def axis_bounds(self) -> Dict[str, Tuple[float, float]]: + def axis_bounds(self) -> Dict[Axis, Tuple[float, float]]: """ The (minimum, maximum) bounds for each axis. """ - return {ax: (0, pos + .05) for ax, pos + return {Axis[ax]: (0, pos) for ax, pos in self._smoothie_driver.homed_position.items() if ax not in 'BC'} diff --git a/api/src/opentrons/hardware_control/simulator.py b/api/src/opentrons/hardware_control/simulator.py index 5a3f3b93d33..f2a4239bdc2 100644 --- a/api/src/opentrons/hardware_control/simulator.py +++ b/api/src/opentrons/hardware_control/simulator.py @@ -259,9 +259,9 @@ async def build_module( sim_model=sim_model) @property - def axis_bounds(self) -> Dict[str, Tuple[float, float]]: + def axis_bounds(self) -> Dict[Axis, Tuple[float, float]]: """ The (minimum, maximum) bounds for each axis. """ - return {ax: (0, pos + 0.5) for ax, pos in _HOME_POSITION.items() + return {Axis[ax]: (0, pos) for ax, pos in _HOME_POSITION.items() if ax not in 'BC'} @property diff --git a/api/src/opentrons/hardware_control/types.py b/api/src/opentrons/hardware_control/types.py index 48418ddd069..a7524adb964 100644 --- a/api/src/opentrons/hardware_control/types.py +++ b/api/src/opentrons/hardware_control/types.py @@ -12,6 +12,25 @@ MODULE_LOG = logging.getLogger(__name__) +class OutOfBoundsMove(RuntimeError): + def __init__(self, message: str): + self.message = message + super().__init__() + + def __str__(self) -> str: + return f'OutOfBoundsMove: {self.message}' + + def __repr__(self) -> str: + return f'<{str(self.__class__)}: {self.message}>' + + +class MotionChecks(enum.Enum): + NONE = 0 + LOW = 1 + HIGH = 2 + BOTH = 3 + + class Axis(enum.Enum): X = 0 Y = 1 diff --git a/api/src/opentrons/hardware_control/util.py b/api/src/opentrons/hardware_control/util.py index 9555c345692..2480b068f97 100644 --- a/api/src/opentrons/hardware_control/util.py +++ b/api/src/opentrons/hardware_control/util.py @@ -2,9 +2,9 @@ import asyncio import logging from enum import Enum -from typing import Dict, Any, Optional, List, Tuple +from typing import Dict, Any, Optional, List, Mapping, Tuple -from .types import CriticalPoint +from .types import CriticalPoint, MotionChecks, OutOfBoundsMove, Axis from opentrons.types import Point mod_log = logging.getLogger(__name__) @@ -48,3 +48,46 @@ class DeckTransformState(Enum): def __str__(self): return self.name + + +def check_motion_bounds( + target_smoothie: Mapping[Axis, float], + target_deck: Mapping[Axis, float], + bounds: Mapping[Axis, Tuple[float, float]], + checks: MotionChecks): + """ + Log or raise an error (depending on checks) if a specified + target position is outside the acceptable bounds of motion + + Which axes are checked is defined by the elements of + target_smoothie. + + The target_deck and mapping does not need to be + entirely filled; it is used only for logging + """ + bounds_message_format = ( + 'Out of bounds move: {axis}=({tsp} motor controller, {tdp} deck) too ' + '{dir} for limit {limsp}') + for ax in target_smoothie.keys(): + if target_smoothie[ax] < bounds[ax][0]: + bounds_message = bounds_message_format.format( + axis=ax, + tsp=target_smoothie[ax], + tdp=target_deck.get(ax, 'unknown'), + dir='low', + limsp=bounds.get(ax, ('unknown',))[0], + ) + mod_log.warning(bounds_message) + if checks.value & MotionChecks.LOW.value: + raise OutOfBoundsMove(bounds_message) + elif target_smoothie[ax] > bounds[ax][1]: + bounds_message = bounds_message_format.format( + axis=ax, + tsp=target_smoothie[ax], + tdp=target_deck.get(ax, 'unknown'), + dir='high', + limsp=bounds.get(ax, (None, 'unknown'))[1], + ) + mod_log.warning(bounds_message) + if checks.value & MotionChecks.HIGH.value: + raise OutOfBoundsMove(bounds_message) diff --git a/api/tests/opentrons/api/test_calibration.py b/api/tests/opentrons/api/test_calibration.py index c0bd455412e..fe0559cbd25 100755 --- a/api/tests/opentrons/api/test_calibration.py +++ b/api/tests/opentrons/api/test_calibration.py @@ -7,6 +7,7 @@ from opentrons.api import models from opentrons.types import Point, Location, Mount from opentrons.hardware_control import CriticalPoint, API +from opentrons.hardware_control.types import MotionChecks state = partial(state, 'calibration') @@ -282,7 +283,7 @@ async def test_jog_api2(main_router, model): ) expected = [ - mock.call(Mount.RIGHT, point) + mock.call(Mount.RIGHT, point, check_bounds=MotionChecks.HIGH) for point in [Point(x=1), Point(y=2), Point(z=3)]] assert jog.mock_calls == expected diff --git a/api/tests/opentrons/hardware_control/test_moves.py b/api/tests/opentrons/hardware_control/test_moves.py index 60622815cd5..c17cf616bf4 100644 --- a/api/tests/opentrons/hardware_control/test_moves.py +++ b/api/tests/opentrons/hardware_control/test_moves.py @@ -3,7 +3,8 @@ from opentrons import types from opentrons import hardware_control as hc from opentrons.config import robot_configs -from opentrons.hardware_control.types import Axis, CriticalPoint +from opentrons.hardware_control.types import ( + Axis, CriticalPoint, OutOfBoundsMove, MotionChecks) from opentrons.hardware_control.robot_calibration import ( RobotCalibration, DeckCalibration) @@ -288,62 +289,6 @@ async def test_other_mount_retracted( == types.Point(54, 20, 218) -async def catch_oob_moves(hardware_api, is_robot, toggle_new_calibration): - await hardware_api.home() - # Check axis max checking for move and move rel - with pytest.raises(RuntimeError): - await hardware_api.move_rel(types.Mount.RIGHT, types.Point(1, 0, 0)) - assert await hardware_api.gantry_position(types.Mount.RIGHT)\ - == types.Point(418, 353, 218) - with pytest.raises(RuntimeError): - await hardware_api.move_rel(types.Mount.RIGHT, types.Point(0, 1, 0)) - with pytest.raises(RuntimeError): - await hardware_api.move_rel(types.Mount.RIGHT, types.Point(0, 0, 1)) - with pytest.raises(RuntimeError): - await hardware_api.move_to(types.Mount.RIGHT, - types.Point(419, 353, 218)) - with pytest.raises(RuntimeError): - await hardware_api.move_to(types.Mount.RIGHT, - types.Point(418, 354, 218)) - with pytest.raises(RuntimeError): - await hardware_api.move_to(types.Mount.RIGHT, - types.Point(418, 353, 219)) - assert await hardware_api.gantry_position(types.Mount.RIGHT)\ - == types.Point(418, 353, 218) - # Axis min checking for move and move rel - with pytest.raises(RuntimeError): - await hardware_api.move_to(types.Mount.RIGHT, - types.Point(-1, 353, 218)) - assert await hardware_api.gantry_position(types.Mount.RIGHT)\ - == types.Point(418, 353, 218) - with pytest.raises(RuntimeError): - await hardware_api.move_to(types.Mount.RIGHT, - types.Point(418, -1, 218)) - with pytest.raises(RuntimeError): - await hardware_api.move_to(types.Mount.RIGHT, - types.Point(418, 353, -1)) - with pytest.raises(RuntimeError): - await hardware_api.move_rel(types.Mount.RIGHT, types.Point(-419, 0, 0)) - with pytest.raises(RuntimeError): - await hardware_api.move_rel(types.Mount.RIGHT, types.Point(0, -354, 0)) - with pytest.raises(RuntimeError): - await hardware_api.move_rel(types.Mount.RIGHT, types.Point(0, 0, -219)) - assert await hardware_api.gantry_position(types.Mount.RIGHT)\ - == types.Point(418, 353, 218) - # Make sure we are checking after mount offset and critical points - # are applied - with pytest.raises(RuntimeError): - await hardware_api.move_to(types.Mount.LEFT, types.Point(33, 0, 0)) - with pytest.raises(RuntimeError): - await hardware_api.move_to(types.Mount.LEFT, types.Point(385, 0, 0)) - await hardware_api.move_to(types.Mount.RIGHT, types.Point(50, 50, 100)) - await hardware_api.cache_instruments({types.Mount.LEFT: 'p10_single'}) - with pytest.raises(RuntimeError): - await hardware_api.move_rel(types.Mount.LEFT, types.Point(0, 0, 12)) - await hardware_api.pick_up_tip(types.Mount.LEFT) - await hardware_api.move_rel(types.Mount.LEFT, types.Point(0, 0, 0)) - - async def test_shake_during_pick_up( hardware_api, monkeypatch, toggle_new_calibration): await hardware_api.home() @@ -436,3 +381,11 @@ async def test_shake_during_drop( mock.call(types.Mount.RIGHT, types.Point(-1, 0, 0), speed=50), mock.call(types.Mount.RIGHT, types.Point(0, 0, 20))] move_rel.assert_has_calls(move_rel_calls) + + +async def test_move_rel_bounds( + hardware_api, toggle_new_calibration): + with pytest.raises(OutOfBoundsMove): + await hardware_api.move_rel( + types.Mount.RIGHT, types.Point(0, 0, 2000), + check_bounds=MotionChecks.HIGH) diff --git a/api/tests/opentrons/hardware_control/test_util.py b/api/tests/opentrons/hardware_control/test_util.py index d56b317dd40..fbf30d6366e 100644 --- a/api/tests/opentrons/hardware_control/test_util.py +++ b/api/tests/opentrons/hardware_control/test_util.py @@ -1,9 +1,11 @@ from typing import List -from opentrons.hardware_control.util import plan_arc -from opentrons.hardware_control.types import CriticalPoint +from opentrons.hardware_control.util import plan_arc, check_motion_bounds +from opentrons.hardware_control.types import ( + CriticalPoint, MotionChecks, OutOfBoundsMove, Axis) from opentrons.types import Point + import pytest @@ -68,3 +70,30 @@ def test_cp_blending(): assert arc2[0][1] == CriticalPoint.TIP assert arc2[1][1] is None assert arc2[2][1] is None + + +@pytest.mark.parametrize( + 'xformed,deck,bounds,check,catch,phrase', + [ + # acceptable moves, iterating through checks + ({Axis.X: 2}, {Axis.X: -15}, {Axis.X: (1, 4)}, MotionChecks.NONE, False, ''), # noqa(E501) + ({Axis.X: 2}, {Axis.X: -10}, {Axis.X: (1, 4)}, MotionChecks.LOW, False, ''), # noqa(E501) + ({Axis.X: 2}, {Axis.X: 0}, {Axis.X: (1, 4)}, MotionChecks.HIGH, False, ''), # noqa(E501) + ({Axis.X: 2}, {Axis.X: 10}, {Axis.X: (1, 4)}, MotionChecks.BOTH, False, ''), # noqa(E501) + # unacceptable low, with and without checking + ({Axis.Z: 1}, {Axis.Z: 3}, {Axis.Z: (2, 4)}, MotionChecks.NONE, False, ''), # noqa(E501) + ({Axis.Z: 1}, {Axis.Z: 3}, {Axis.Z: (2, 4)}, MotionChecks.LOW, True, 'low'), # noqa(E501) + ({Axis.Z: 1}, {Axis.Z: 3}, {Axis.Z: (2, 4)}, MotionChecks.HIGH, False, ''), # noqa(E501) + ({Axis.Z: 1}, {Axis.Z: 3}, {Axis.Z: (2, 4)}, MotionChecks.BOTH, True, 'low'), # noqa(E501) + # unacceptable high, with and without checking + ({Axis.Z: 5}, {Axis.Z: 3}, {Axis.Z: (2, 4)}, MotionChecks.NONE, False, ''), # noqa(E501) + ({Axis.Z: 5}, {Axis.Z: 3}, {Axis.Z: (2, 4)}, MotionChecks.LOW, False, ''), # noqa(E501) + ({Axis.Z: 5}, {Axis.Z: 3}, {Axis.Z: (2, 4)}, MotionChecks.HIGH, True, 'high'), # noqa(E501) + ({Axis.Z: 5}, {Axis.Z: 3}, {Axis.Z: (2, 4)}, MotionChecks.BOTH, True, 'high') # noqa(E501) + ]) +def test_check_motion_bounds(xformed, deck, bounds, check, catch, phrase): + if catch: + with pytest.raises(OutOfBoundsMove, match=phrase): + check_motion_bounds(xformed, deck, bounds, check) + else: + check_motion_bounds(xformed, deck, bounds, check)