fix(api): OT-2 trash bins in 2.16 get added to engine at the start of…

… protocol (#14475)

api/src/opentrons/execute.py

@@ -41,7 +41,7 @@
 from opentrons.protocols.api_support.deck_type import (
     guess_from_global_config as guess_deck_type_from_global_config,
     should_load_fixed_trash,
-    should_load_fixed_trash_for_python_protocol,
+    should_load_fixed_trash_labware_for_python_protocol,
 )
 from opentrons.protocols.api_support.types import APIVersion
 from opentrons.protocols.execution import execute as execute_apiv2
@@ -540,7 +540,9 @@ def _create_live_context_pe(
             config=_get_protocol_engine_config(),
             drop_tips_after_run=False,
             post_run_hardware_state=PostRunHardwareState.STAY_ENGAGED_IN_PLACE,
-            load_fixed_trash=should_load_fixed_trash_for_python_protocol(api_version),
+            load_fixed_trash=should_load_fixed_trash_labware_for_python_protocol(
+                api_version
+            ),
         )
     )
 

api/src/opentrons/protocol_api/core/engine/protocol.py

@@ -134,9 +134,14 @@ def _load_fixed_trash(self) -> None:
     def append_disposal_location(
         self,
         disposal_location: Union[Labware, TrashBin, WasteChute],
-        skip_add_to_engine: bool = False,
     ) -> None:
-        """Append a disposal location object to the core"""
+        """Append a disposal location object to the core."""
+        self._disposal_locations.append(disposal_location)
+
+    def add_disposal_location_to_engine(
+        self, disposal_location: Union[TrashBin, WasteChute]
+    ) -> None:
+        """Verify and add disposal location to engine store and append it to the core."""
         if isinstance(disposal_location, TrashBin):
             self._engine_client.state.addressable_areas.raise_if_area_not_in_deck_configuration(
                 disposal_location.area_name
@@ -152,8 +157,7 @@ def append_disposal_location(
                 existing_labware_ids=list(self._labware_cores_by_id.keys()),
                 existing_module_ids=list(self._module_cores_by_id.keys()),
             )
-            if not skip_add_to_engine:
-                self._engine_client.add_addressable_area(disposal_location.area_name)
+            self._engine_client.add_addressable_area(disposal_location.area_name)
         elif isinstance(disposal_location, WasteChute):
             # TODO(jbl 2024-01-25) hardcoding this specific addressable area should be refactored
             #   when analysis is fixed up
@@ -166,9 +170,8 @@ def append_disposal_location(
             self._engine_client.state.addressable_areas.raise_if_area_not_in_deck_configuration(
                 "1ChannelWasteChute"
             )
-            if not skip_add_to_engine:
-                self._engine_client.add_addressable_area("1ChannelWasteChute")
-        self._disposal_locations.append(disposal_location)
+            self._engine_client.add_addressable_area("1ChannelWasteChute")
+        self.append_disposal_location(disposal_location)
 
     def get_disposal_locations(self) -> List[Union[Labware, TrashBin, WasteChute]]:
         """Get disposal locations."""

api/src/opentrons/protocol_api/core/legacy/legacy_protocol_core.py

@@ -136,14 +136,18 @@ def is_simulating(self) -> bool:
     def append_disposal_location(
         self,
         disposal_location: Union[Labware, TrashBin, WasteChute],
-        skip_add_to_engine: bool = False,
     ) -> None:
         if isinstance(disposal_location, (TrashBin, WasteChute)):
             raise APIVersionError(
                 "Trash Bin and Waste Chute Disposal locations are not supported in this API Version."
             )
         self._disposal_locations.append(disposal_location)
 
+    def add_disposal_location_to_engine(
+        self, disposal_location: Union[TrashBin, WasteChute]
+    ) -> None:
+        assert False, "add_disposal_location_to_engine only supported on engine core"
+
     def add_labware_definition(
         self,
         definition: LabwareDefinition,

api/src/opentrons/protocol_api/core/protocol.py

@@ -65,11 +65,17 @@ def add_labware_definition(
     def append_disposal_location(
         self,
         disposal_location: Union[Labware, TrashBin, WasteChute],
-        skip_add_to_engine: bool = False,
     ) -> None:
         """Append a disposal location object to the core"""
         ...
 
+    @abstractmethod
+    def add_disposal_location_to_engine(
+        self, disposal_location: Union[TrashBin, WasteChute]
+    ) -> None:
+        """Verify and add disposal location to engine store and append it to the core."""
+        ...
+
     @abstractmethod
     def load_labware(
         self,

api/src/opentrons/protocol_api/create_protocol_context.py

@@ -15,10 +15,14 @@
 from opentrons.protocol_engine import ProtocolEngine
 from opentrons.protocol_engine.clients import SyncClient, ChildThreadTransport
 from opentrons.protocols.api_support.types import APIVersion
+from opentrons.protocols.api_support.deck_type import (
+    should_load_fixed_trash_area_for_python_protocol,
+)
 from opentrons.protocols.api_support.definitions import MAX_SUPPORTED_VERSION
 
 from .protocol_context import ProtocolContext
 from .deck import Deck
+from ._trash_bin import TrashBin
 
 from .core.common import ProtocolCore as AbstractProtocolCore
 from .core.legacy.deck import Deck as LegacyDeck
@@ -148,7 +152,7 @@ def create_protocol_context(
     # this swap may happen once `ctx.move_labware` off-deck is implemented
     deck = None if isinstance(core, ProtocolCore) else cast(Deck, core.get_deck())
 
-    return ProtocolContext(
+    context = ProtocolContext(
         api_version=api_version,
         # TODO(mm, 2023-05-11): This cast shouldn't be necessary.
         # Fix this by making the appropriate TypeVars covariant?
@@ -158,3 +162,18 @@ def create_protocol_context(
         deck=deck,
         bundled_data=bundled_data,
     )
+    # If we're loading an engine based core into the context, and we're on api level 2.16 or above, on an OT-2 we need
+    # to insert a fixed trash addressable area into the protocol engine, for correctness in anything that relies on
+    # knowing what addressable areas have been loaded (and any checks involving trash geometry). Because the method
+    # that uses this in the core relies on the sync client and this code will run in the main thread (which if called
+    # will cause a deadlock), we're directly calling the protocol engine method here where we have access to it.
+    if (
+        protocol_engine is not None
+        and should_load_fixed_trash_area_for_python_protocol(
+            api_version=api_version,
+            robot_type=protocol_engine.state_view.config.robot_type,
+        )
+    ):
+        assert isinstance(context.fixed_trash, TrashBin)
+        protocol_engine.add_addressable_area(context.fixed_trash.area_name)
+    return context

api/src/opentrons/protocol_api/protocol_context.py

@@ -26,7 +26,8 @@
 from opentrons.protocols.api_support import instrument as instrument_support
 from opentrons.protocols.api_support.deck_type import (
     NoTrashDefinedError,
-    should_load_fixed_trash_for_python_protocol,
+    should_load_fixed_trash_labware_for_python_protocol,
+    should_load_fixed_trash_area_for_python_protocol,
 )
 from opentrons.protocols.api_support.types import APIVersion
 from opentrons.protocols.api_support.util import (
@@ -159,22 +160,19 @@ def __init__(
         # protocols after 2.16 expect trash to exist as either a TrashBin or WasteChute object.
 
         self._load_fixed_trash()
-        if should_load_fixed_trash_for_python_protocol(self._api_version):
+        if should_load_fixed_trash_labware_for_python_protocol(self._api_version):
             self._core.append_disposal_location(self.fixed_trash)
-        elif (
-            self._api_version >= APIVersion(2, 16)
-            and self._core.robot_type == "OT-2 Standard"
+        elif should_load_fixed_trash_area_for_python_protocol(
+            self._api_version, self._core.robot_type
         ):
             _fixed_trash_trashbin = TrashBin(
                 location=DeckSlotName.FIXED_TRASH, addressable_area_name="fixedTrash"
             )
-            # We have to skip adding this fixed trash bin to engine because this __init__ is called in the main thread
-            # and any calls to sync client will cause a deadlock. This means that OT-2 fixed trashes are not added to
-            # the engine store until one is first referenced. This should have minimal consequences for OT-2 given that
-            # we do not need to worry about the 96 channel pipette and partial tip configuration with that pipette.
-            self._core.append_disposal_location(
-                _fixed_trash_trashbin, skip_add_to_engine=True
-            )
+            # We are just appending the fixed trash to the core's internal list here, not adding it to the engine via
+            # the core, since that method works through the SyncClient and if called from here, will cause protocols
+            # to deadlock. Instead, that method is called in protocol engine directly in create_protocol_context after
+            # ProtocolContext is initialized.
+            self._core.append_disposal_location(_fixed_trash_trashbin)
 
         self._commands: List[str] = []
         self._unsubscribe_commands: Optional[Callable[[], None]] = None
@@ -517,7 +515,7 @@ def load_trash_bin(self, location: DeckLocation) -> TrashBin:
         trash_bin = TrashBin(
             location=slot_name, addressable_area_name=addressable_area_name
         )
-        self._core.append_disposal_location(trash_bin)
+        self._core.add_disposal_location_to_engine(trash_bin)
         return trash_bin
 
     @requires_version(2, 16)
@@ -534,7 +532,7 @@ def load_waste_chute(
         API will raise an error.
         """
         waste_chute = WasteChute()
-        self._core.append_disposal_location(waste_chute)
+        self._core.add_disposal_location_to_engine(waste_chute)
         return waste_chute
 
     @requires_version(2, 15)

api/src/opentrons/protocols/api_support/deck_type.py

@@ -45,15 +45,30 @@ def __init__(
         )
 
 
-def should_load_fixed_trash_for_python_protocol(api_version: APIVersion) -> bool:
+def should_load_fixed_trash_labware_for_python_protocol(
+    api_version: APIVersion,
+) -> bool:
+    """Whether to automatically load the fixed trash as a labware for a Python protocol at protocol start."""
     return api_version <= LOAD_FIXED_TRASH_GATE_VERSION_PYTHON
 
 
+def should_load_fixed_trash_area_for_python_protocol(
+    api_version: APIVersion, robot_type: RobotType
+) -> bool:
+    """Whether to automatically load the fixed trash addressable area for OT-2 protocols on 2.16 and above."""
+    return (
+        api_version > LOAD_FIXED_TRASH_GATE_VERSION_PYTHON
+        and robot_type == "OT-2 Standard"
+    )
+
+
 def should_load_fixed_trash(protocol_config: ProtocolConfig) -> bool:
-    """Decide whether to automatically load fixed trash on the deck based on version."""
+    """Decide whether to automatically load fixed trash labware on the deck based on version."""
     load_fixed_trash = False
     if isinstance(protocol_config, PythonProtocolConfig):
-        return should_load_fixed_trash_for_python_protocol(protocol_config.api_version)
+        return should_load_fixed_trash_labware_for_python_protocol(
+            protocol_config.api_version
+        )
     # TODO(jbl 2023-10-27), when schema v8 is out, use a new deck version field to support fixed trash protocols
     elif isinstance(protocol_config, JsonProtocolConfig):
         load_fixed_trash = (

api/src/opentrons/simulate.py

@@ -66,7 +66,7 @@
 from opentrons.protocols.api_support.deck_type import (
     for_simulation as deck_type_for_simulation,
     should_load_fixed_trash,
-    should_load_fixed_trash_for_python_protocol,
+    should_load_fixed_trash_labware_for_python_protocol,
 )
 from opentrons.protocols.api_support.types import APIVersion
 from opentrons_shared_data.labware.labware_definition import LabwareDefinition
@@ -801,7 +801,9 @@ def _create_live_context_pe(
             config=_get_protocol_engine_config(robot_type),
             drop_tips_after_run=False,
             post_run_hardware_state=PostRunHardwareState.STAY_ENGAGED_IN_PLACE,
-            load_fixed_trash=should_load_fixed_trash_for_python_protocol(api_version),
+            load_fixed_trash=should_load_fixed_trash_labware_for_python_protocol(
+                api_version
+            ),
         )
     )
 

api/tests/opentrons/protocol_api_integration/test_trashes.py

@@ -123,16 +123,6 @@ def test_trash_search() -> None:
             "2.16",
             "OT-2",
             False,
-            # This should ideally raise, matching OT-2 behavior on prior Protocol API versions.
-            # It currently does not because Protocol API v2.15's trashes are implemented as
-            # addressable areas, not labware--and they're only brought into existence
-            # *on first use,* not at the beginning of a protocol.
-            #
-            # The good news is that even though the conflicting load will not raise like we want,
-            # something in the protocol will eventually raise, e.g. when a pipette goes to drop a
-            # tip in the fixed trash and finds that a fixed trash can't exist there because there's
-            # a labware.
-            marks=pytest.mark.xfail(strict=True, raises=pytest.fail.Exception),
         ),
         pytest.param(
             "2.16",