From 237f8896f125af24ce3f4ef2e7bc0709f0cf7a8e Mon Sep 17 00:00:00 2001 From: Seth Foster Date: Fri, 2 Nov 2018 13:14:36 -0400 Subject: [PATCH] refactor(api): Check moves for oob in hardware control Hardware control now checks that incoming gantry moves are in acceptable bounds. That means less than the home position and > 0, for XYZA. If the move is outside bounds, hardware_control raises a RuntimeError. This bounds checking is applied in deck coordinates after the mount offset and critical points are applied, which means that top level move calls may succeed or fail based on the state of the rest of the system. For instance, with no pipette attached to the right mount, move_to(RIGHT, Point(50, 50, 0)) will fail (because the mount on its own cannot descend to 0); but with a P300 single with tip attached in the right mount, that move will move the end of the tip to the deck. --- .../opentrons/hardware_control/__init__.py | 20 ++++- .../opentrons/hardware_control/controller.py | 7 ++ .../opentrons/hardware_control/simulator.py | 6 ++ .../opentrons/hardware_control/test_moves.py | 73 +++++++++++++++++-- 4 files changed, 99 insertions(+), 7 deletions(-) diff --git a/api/src/opentrons/hardware_control/__init__.py b/api/src/opentrons/hardware_control/__init__.py index fc720fb6c9e..9492757ddb4 100644 --- a/api/src/opentrons/hardware_control/__init__.py +++ b/api/src/opentrons/hardware_control/__init__.py @@ -460,10 +460,28 @@ async def _move(self, target_position: 'OrderedDict[Axis, float]', # Since target_position is an OrderedDict with the axes ordered by # (x, y, z, a, b, c), and we’ll only have one of a or z (as checked # by the len(to_transform) check above) we can use an enumerate to - # fuse the specified axes and the transformed values back together + # 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()}) + raise RuntimeError( + "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])) try: self._backend.move(smoothie_pos, speed=speed) except Exception: diff --git a/api/src/opentrons/hardware_control/controller.py b/api/src/opentrons/hardware_control/controller.py index 82ff75f98d3..88d85becb5c 100644 --- a/api/src/opentrons/hardware_control/controller.py +++ b/api/src/opentrons/hardware_control/controller.py @@ -171,3 +171,10 @@ def _set_temp_speed(self, speed): yield finally: self._smoothie_driver.pop_speed() + + @property + def axis_bounds(self) -> Dict[str, Tuple[float, float]]: + """ The (minimum, maximum) bounds for each axis. """ + return {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 f0ddd2b9eee..fb92aa566dd 100644 --- a/api/src/opentrons/hardware_control/simulator.py +++ b/api/src/opentrons/hardware_control/simulator.py @@ -125,3 +125,9 @@ async def update_module( loop: Optional[asyncio.AbstractEventLoop])\ -> modules.AbstractModule: return module + + @property + def axis_bounds(self) -> Dict[str, Tuple[float, float]]: + """ The (minimum, maximum) bounds for each axis. """ + return {ax: (0, pos) for ax, pos in _HOME_POSITION.items() + if ax not in 'BC'} diff --git a/api/tests/opentrons/hardware_control/test_moves.py b/api/tests/opentrons/hardware_control/test_moves.py index 021b5cd412a..5eb5d8257aa 100644 --- a/api/tests/opentrons/hardware_control/test_moves.py +++ b/api/tests/opentrons/hardware_control/test_moves.py @@ -56,12 +56,12 @@ async def test_controller_musthome(hardware_api): async def test_home_specific_sim(hardware_api, monkeypatch): await hardware_api.home() - await hardware_api.move_to(types.Mount.RIGHT, types.Point(-10, 10, 20)) + await hardware_api.move_to(types.Mount.RIGHT, types.Point(0, 10, 20)) # Avoid the autoretract when moving two difference instruments hardware_api._last_moved_mount = None await hardware_api.move_rel(types.Mount.LEFT, types.Point(0, 0, -20)) await hardware_api.home([Axis.Z, Axis.C]) - assert hardware_api._current_position == {Axis.X: -10, + assert hardware_api._current_position == {Axis.X: 0, Axis.Y: 10, Axis.Z: 218, Axis.A: 20, @@ -71,9 +71,9 @@ async def test_home_specific_sim(hardware_api, monkeypatch): async def test_retract(hardware_api): await hardware_api.home() - await hardware_api.move_to(types.Mount.RIGHT, types.Point(-10, 10, 20)) + await hardware_api.move_to(types.Mount.RIGHT, types.Point(0, 10, 20)) await hardware_api.retract(types.Mount.RIGHT, 10) - assert hardware_api._current_position == {Axis.X: -10, + assert hardware_api._current_position == {Axis.X: 0, Axis.Y: 10, Axis.Z: 218, Axis.A: 218, @@ -97,11 +97,11 @@ async def test_move(hardware_api): # This assert implicitly checks that the mount offset is not applied to # relative moves; if you change this to move_to, the offset will be # applied again - rel_position = types.Point(30, 20, 10) + rel_position = types.Point(30, 20, -10) mount2 = types.Mount.LEFT target_position2 = {Axis.X: 60, Axis.Y: 40, - Axis.Z: 228, + Axis.Z: 208, Axis.A: 218, # The other instrument is retracted Axis.B: 19, Axis.C: 19} @@ -157,10 +157,15 @@ async def test_critical_point_applied(hardware_api, monkeypatch): target[Axis.A] = 0 assert hardware_api.current_position(types.Mount.RIGHT) == target # And removing the tip should move us back to the original + await hardware_api.move_rel(types.Mount.RIGHT, types.Point(2.5, 0, 0)) await hardware_api.drop_tip(types.Mount.RIGHT) target[Axis.A] = 33 + hc.DROP_TIP_RELEASE_DISTANCE + target_no_offset[Axis.X] = 2.5 + target[Axis.X] = 2.5 assert hardware_api.current_position(types.Mount.RIGHT) == target await hardware_api.move_to(types.Mount.RIGHT, types.Point(0, 0, 0)) + target[Axis.X] = 0 + target_no_offset[Axis.X] = 0 target_no_offset[Axis.A] = 13 target[Axis.A] = 0 assert hardware_api._current_position == target_no_offset @@ -203,3 +208,59 @@ async def test_other_mount_retracted(hardware_api): await hardware_api.move_to(types.Mount.LEFT, types.Point(20, 20, 0)) assert hardware_api.gantry_position(types.Mount.RIGHT) \ == types.Point(54, 20, 218) + + +async def catch_oob_moves(hardware_api): + 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 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 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 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 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))