From fb7d65b53386e333698d9758ac7d76e0d0af4278 Mon Sep 17 00:00:00 2001 From: Mike Cousins Date: Wed, 7 Apr 2021 18:47:24 -0400 Subject: [PATCH 1/2] wip: noodle around with location cache --- .../protocols/api_support/definitions.py | 2 +- .../context/engine/protocol_context.py | 11 ++++++++-- .../opentrons/protocols/context/protocol.py | 11 ++++++++-- .../protocol_api/instrument_context.py | 17 ++++++++++++-- .../context/protocol_api/protocol_context.py | 22 ++++++++++++++----- 5 files changed, 51 insertions(+), 12 deletions(-) diff --git a/api/src/opentrons/protocols/api_support/definitions.py b/api/src/opentrons/protocols/api_support/definitions.py index 08230959672..067200e7864 100644 --- a/api/src/opentrons/protocols/api_support/definitions.py +++ b/api/src/opentrons/protocols/api_support/definitions.py @@ -1,6 +1,6 @@ from .types import APIVersion -MAX_SUPPORTED_VERSION = APIVersion(2, 9) +MAX_SUPPORTED_VERSION = APIVersion(2, 10) #: The maximum supported protocol API version in this release V2_MODULE_DEF_VERSION = APIVersion(2, 3) diff --git a/api/src/opentrons/protocols/context/engine/protocol_context.py b/api/src/opentrons/protocols/context/engine/protocol_context.py index ad66feab76a..3f48540b930 100644 --- a/api/src/opentrons/protocols/context/engine/protocol_context.py +++ b/api/src/opentrons/protocols/context/engine/protocol_context.py @@ -132,8 +132,15 @@ def get_rail_lights_on(self) -> bool: def door_closed(self) -> bool: raise NotImplementedError() - def get_last_location(self) -> Optional[types.Location]: + def get_last_location( + self, + mount: Optional[types.Mount] = None, + ) -> Optional[types.Location]: raise NotImplementedError() - def set_last_location(self, location: Optional[types.Location]) -> None: + def set_last_location( + self, + location: Optional[types.Location], + mount: Optional[types.Mount] = None, + ) -> None: raise NotImplementedError() diff --git a/api/src/opentrons/protocols/context/protocol.py b/api/src/opentrons/protocols/context/protocol.py index 1b0e6746caa..2de36dee39b 100644 --- a/api/src/opentrons/protocols/context/protocol.py +++ b/api/src/opentrons/protocols/context/protocol.py @@ -160,9 +160,16 @@ def door_closed(self) -> bool: ... @abstractmethod - def get_last_location(self) -> Optional[types.Location]: + def get_last_location( + self, + mount: Optional[types.Mount] = None, + ) -> Optional[types.Location]: ... @abstractmethod - def set_last_location(self, location: Optional[types.Location]) -> None: + def set_last_location( + self, + location: Optional[types.Location], + mount: Optional[types.Mount] = None, + ) -> None: ... diff --git a/api/src/opentrons/protocols/context/protocol_api/instrument_context.py b/api/src/opentrons/protocols/context/protocol_api/instrument_context.py index 07cadfe589a..157a25f535a 100644 --- a/api/src/opentrons/protocols/context/protocol_api/instrument_context.py +++ b/api/src/opentrons/protocols/context/protocol_api/instrument_context.py @@ -154,7 +154,17 @@ def move_to(self, minimum_z_height: Optional[float], speed: Optional[float]) -> None: """Move the instrument.""" - last_location = self._protocol_interface.get_last_location() + # prevent direct movement bugs in PAPI version >= 2.10 + location_cache_mount = ( + self._mount + if self._api_version >= APIVersion(2, 10) else + None + ) + + last_location = self._protocol_interface.get_last_location( + mount=location_cache_mount + ) + if last_location: from_lw = last_location.labware else: @@ -190,7 +200,10 @@ def move_to(self, self._protocol_interface.set_last_location(None) raise else: - self._protocol_interface.set_last_location(location) + self._protocol_interface.set_last_location( + location=location, + mount=location_cache_mount + ) def get_mount(self) -> types.Mount: """Get the mount this pipette is attached to.""" diff --git a/api/src/opentrons/protocols/context/protocol_api/protocol_context.py b/api/src/opentrons/protocols/context/protocol_api/protocol_context.py index 31de2108a35..a1fb1e5a29e 100644 --- a/api/src/opentrons/protocols/context/protocol_api/protocol_context.py +++ b/api/src/opentrons/protocols/context/protocol_api/protocol_context.py @@ -86,6 +86,7 @@ def __init__(self, self._bundled_data: Dict[str, bytes] = bundled_data or {} self._default_max_speeds = AxisMaxSpeeds() self._last_location: Optional[types.Location] = None + self._last_mount: Optional[types.Mount] = None self._loaded_modules: Set['AbstractModule'] = set() @classmethod @@ -195,8 +196,8 @@ def load_module( hc_mod_instance = None for mod in available_modules: compatible = module_geometry.models_compatible( - module_geometry.module_model_from_string(mod.model()), - resolved_model) + module_geometry.module_model_from_string(mod.model()), + resolved_model) if compatible and mod not in self._loaded_modules: self._loaded_modules.add(mod) hc_mod_instance = SynchronousAdapter(mod) @@ -295,10 +296,21 @@ def door_closed(self) -> bool: """Check if door is closed.""" return DoorState.CLOSED == self._hw_manager.hardware.door_state - def get_last_location(self) -> Optional[types.Location]: + def get_last_location( + self, + mount: Optional[types.Mount] = None, + ) -> Optional[types.Location]: """Get the most recent moved to location.""" - return self._last_location + if mount is None or mount == self._last_mount: + return self._last_location - def set_last_location(self, location: Optional[types.Location]) -> None: + return None + + def set_last_location( + self, + location: Optional[types.Location], + mount: Optional[types.Mount] = None, + ) -> None: """Set the most recent moved to location.""" self._last_location = location + self._last_mount = mount From 06d8835d1438d5838f40b3ec7bffe33a5438f3b9 Mon Sep 17 00:00:00 2001 From: amit lissack Date: Thu, 8 Apr 2021 10:35:09 -0400 Subject: [PATCH 2/2] add unit tests --- .../opentrons/protocol_api/test_context.py | 47 +++++++++++++++++++ 1 file changed, 47 insertions(+) diff --git a/api/tests/opentrons/protocol_api/test_context.py b/api/tests/opentrons/protocol_api/test_context.py index 06fefdee1dd..4893eb37904 100644 --- a/api/tests/opentrons/protocol_api/test_context.py +++ b/api/tests/opentrons/protocol_api/test_context.py @@ -147,6 +147,53 @@ def fake_plan_move(from_loc, to_loc, deck, assert test_args[0].labware.as_well() == lw.wells()[0] +async def test_location_cache_two_pipettes(ctx, get_labware_def, hardware): + """It should be invalidated when next movement is a different pipette + than the cached location.""" + ctx.home() + left = ctx.load_instrument('p10_single', Mount.LEFT) + right = ctx.load_instrument('p10_single', Mount.RIGHT) + + left_loc = Location(point=Point(1, 2, 3), labware="1") + right_loc = Location(point=Point(3, 4, 5), labware="2") + + with mock.patch.object(papi_geometry.planning, "plan_moves") as m: + # The first moves. The location cache is empty. + left.move_to(left_loc) + assert m.call_args[0][0].labware.is_empty + assert m.call_args[0][1] == left_loc + # The second move the location cache is not used because we're moving + # a different pipette. + right.move_to(right_loc) + assert m.call_args[0][0].labware.is_empty + assert m.call_args[0][1] == right_loc + + +async def test_location_cache_two_pipettes_fails_pre_2_10( + ctx, + get_labware_def, hardware +): + """It should reuse location cache even if cached location was set by + move of a different pipette.""" + ctx.home() + left = ctx.load_instrument('p10_single', Mount.LEFT) + right = ctx.load_instrument('p10_single', Mount.RIGHT) + left._implementation._api_version = APIVersion(2, 9) + right._implementation._api_version = APIVersion(2, 9) + + left_loc = Location(point=Point(1, 2, 3), labware="1") + right_loc = Location(point=Point(3, 4, 5), labware="2") + + with mock.patch.object(papi_geometry.planning, "plan_moves") as m: + # The first moves. The location cache is empty. + left.move_to(left_loc) + assert m.call_args[0][0].labware.is_empty + assert m.call_args[0][1] == left_loc + right.move_to(right_loc) + assert m.call_args[0][0].labware == left_loc.labware + assert m.call_args[0][1] == right_loc + + async def test_move_uses_arc(ctx, monkeypatch, get_labware_def, hardware): ctx.connect(hardware) ctx.home()