refactor(api,shared-data): Clarify gripper offsets (#16711)

Author: SyntaxColoring

api/src/opentrons/protocol_engine/commands/absorbance_reader/close_lid.py

@@ -107,7 +107,9 @@ async def execute(self, params: CloseLidParams) -> SuccessData[CloseLidResult]:
                 )
             )
 
-            lid_gripper_offsets = self._state_view.labware.get_labware_gripper_offsets(
+            # The lid's labware definition stores gripper offsets for itself in the
+            # space normally meant for offsets for labware stacked atop it.
+            lid_gripper_offsets = self._state_view.labware.get_child_gripper_offsets(
                 loaded_lid.id, None
             )
             if lid_gripper_offsets is None:

api/src/opentrons/protocol_engine/commands/absorbance_reader/open_lid.py

@@ -106,7 +106,9 @@ async def execute(self, params: OpenLidParams) -> SuccessData[OpenLidResult]:
                 mod_substate.module_id
             )
 
-            lid_gripper_offsets = self._state_view.labware.get_labware_gripper_offsets(
+            # The lid's labware definition stores gripper offsets for itself in the
+            # space normally meant for offsets for labware stacked atop it.
+            lid_gripper_offsets = self._state_view.labware.get_child_gripper_offsets(
                 loaded_lid.id, None
             )
             if lid_gripper_offsets is None:

api/src/opentrons/protocol_engine/commands/unsafe/unsafe_place_labware.py

@@ -2,7 +2,7 @@
 
 from __future__ import annotations
 from pydantic import BaseModel, Field
-from typing import TYPE_CHECKING, Optional, Type, cast
+from typing import TYPE_CHECKING, Optional, Type
 from typing_extensions import Literal
 
 from opentrons.hardware_control.types import Axis, OT3Mount
@@ -84,13 +84,25 @@ async def execute(
                 "Cannot place labware when gripper is not gripping."
             )
 
-        # Allow propagation of LabwareNotLoadedError.
         labware_id = params.labwareId
+        # Allow propagation of LabwareNotLoadedError.
         definition_uri = self._state_view.labware.get(labware_id).definitionUri
-        final_offsets = self._state_view.labware.get_labware_gripper_offsets(
+
+        # todo(mm, 2024-11-06): This is only correct in the special case of an
+        # absorbance reader lid. Its definition currently puts the offsets for *itself*
+        # in the property that's normally meant for offsets for its *children.*
+        final_offsets = self._state_view.labware.get_child_gripper_offsets(
             labware_id, None
         )
-        drop_offset = cast(Point, final_offsets.dropOffset) if final_offsets else None
+        drop_offset = (
+            Point(
+                final_offsets.dropOffset.x,
+                final_offsets.dropOffset.y,
+                final_offsets.dropOffset.z,
+            )
+            if final_offsets
+            else None
+        )
 
         if isinstance(params.location, DeckSlotLocation):
             self._state_view.addressable_areas.raise_if_area_not_in_deck_configuration(

api/src/opentrons/protocol_engine/state/geometry.py

@@ -1203,6 +1203,7 @@ def get_total_nominal_gripper_offset_for_move_type(
                 extra_offset = LabwareOffsetVector(x=0, y=0, z=0)
                 if (
                     isinstance(ancestor, ModuleLocation)
+                    # todo(mm, 2024-11-06): Do not access private module state; only use public ModuleView methods.
                     and self._modules._state.requested_model_by_id[ancestor.moduleId]
                     == ModuleModel.THERMOCYCLER_MODULE_V2
                     and labware_validation.validate_definition_is_lid(current_labware)
@@ -1241,6 +1242,19 @@ def get_total_nominal_gripper_offset_for_move_type(
                     + extra_offset
                 )
 
+    # todo(mm, 2024-11-05): This may be incorrect because it does not take the following
+    # offsets into account:
+    #
+    # * The pickup offset in the definition of the parent of the gripped labware.
+    # * The "additional offset" or "user offset", e.g. the `pickUpOffset` and `dropOffset`
+    #   params in the `moveLabware` command.
+    #
+    # For robustness, we should combine this with `get_gripper_labware_movement_waypoints()`.
+    #
+    # We should also be more explicit about which offsets act to move the gripper paddles
+    # relative to the gripped labware, and which offsets act to change how the gripped
+    # labware sits atop its parent. Those have different effects on how far the gripped
+    # labware juts beyond the paddles while it's in transit.
     def check_gripper_labware_tip_collision(
         self,
         gripper_homed_position_z: float,
@@ -1321,11 +1335,11 @@ def _labware_gripper_offsets(
             module_loc = self._modules.get_location(parent_location.moduleId)
             slot_name = module_loc.slotName
 
-        slot_based_offset = self._labware.get_labware_gripper_offsets(
+        slot_based_offset = self._labware.get_child_gripper_offsets(
             labware_id=labware_id, slot_name=slot_name.to_ot3_equivalent()
         )
 
-        return slot_based_offset or self._labware.get_labware_gripper_offsets(
+        return slot_based_offset or self._labware.get_child_gripper_offsets(
             labware_id=labware_id, slot_name=None
         )
 

api/src/opentrons/protocol_engine/state/labware.py

@@ -928,19 +928,24 @@ def get_deck_default_gripper_offsets(self) -> Optional[LabwareMovementOffsetData
             else None
         )
 
-    def get_labware_gripper_offsets(
+    def get_child_gripper_offsets(
         self,
         labware_id: str,
         slot_name: Optional[DeckSlotName],
     ) -> Optional[LabwareMovementOffsetData]:
-        """Get the labware's gripper offsets of the specified type.
+        """Get the offsets that a labware says should be applied to children stacked atop it.
+
+        Params:
+            labware_id: The ID of a parent labware (atop which another labware, the child, will be stacked).
+            slot_name: The ancestor slot that the parent labware is ultimately loaded into,
+                       perhaps after going through a module in the middle.
 
         Returns:
-            If `slot_name` is provided, returns the gripper offsets that the labware definition
+            If `slot_name` is provided, returns the gripper offsets that the parent labware definition
             specifies just for that slot, or `None` if the labware definition doesn't have an
             exact match.
 
-            If `slot_name` is `None`, returns the gripper offsets that the labware
+            If `slot_name` is `None`, returns the gripper offsets that the parent labware
             definition designates as "default," or `None` if it doesn't designate any as such.
         """
         parsed_offsets = self.get_definition(labware_id).gripperOffsets

api/tests/opentrons/protocol_engine/state/test_geometry_view.py

@@ -2750,7 +2750,7 @@ def test_get_stacked_labware_total_nominal_offset_slot_specific(
         DeckSlotLocation(slotName=DeckSlotName.SLOT_C1)
     )
     decoy.when(
-        mock_labware_view.get_labware_gripper_offsets(
+        mock_labware_view.get_child_gripper_offsets(
             labware_id="adapter-id", slot_name=DeckSlotName.SLOT_C1
         )
     ).then_return(
@@ -2802,12 +2802,12 @@ def test_get_stacked_labware_total_nominal_offset_default(
         DeckSlotLocation(slotName=DeckSlotName.SLOT_4)
     )
     decoy.when(
-        mock_labware_view.get_labware_gripper_offsets(
+        mock_labware_view.get_child_gripper_offsets(
             labware_id="adapter-id", slot_name=DeckSlotName.SLOT_C1
         )
     ).then_return(None)
     decoy.when(
-        mock_labware_view.get_labware_gripper_offsets(
+        mock_labware_view.get_child_gripper_offsets(
             labware_id="adapter-id", slot_name=None
         )
     ).then_return(

api/tests/opentrons/protocol_engine/state/test_labware_view.py

@@ -1530,10 +1530,9 @@ def test_get_labware_gripper_offsets(
     )
 
     assert (
-        subject.get_labware_gripper_offsets(labware_id="plate-id", slot_name=None)
-        is None
+        subject.get_child_gripper_offsets(labware_id="plate-id", slot_name=None) is None
     )
-    assert subject.get_labware_gripper_offsets(
+    assert subject.get_child_gripper_offsets(
         labware_id="adapter-plate-id", slot_name=DeckSlotName.SLOT_D1
     ) == LabwareMovementOffsetData(
         pickUpOffset=LabwareOffsetVector(x=0, y=0, z=0),
@@ -1570,13 +1569,13 @@ def test_get_labware_gripper_offsets_default_no_slots(
     )
 
     assert (
-        subject.get_labware_gripper_offsets(
+        subject.get_child_gripper_offsets(
             labware_id="labware-id", slot_name=DeckSlotName.SLOT_D1
         )
         is None
     )
 
-    assert subject.get_labware_gripper_offsets(
+    assert subject.get_child_gripper_offsets(
         labware_id="labware-id", slot_name=None
     ) == LabwareMovementOffsetData(
         pickUpOffset=LabwareOffsetVector(x=1, y=2, z=3),

shared-data/labware/schemas/2.json

@@ -65,6 +65,20 @@
           "type": "number"
         }
       }
+    },
+    "pickUpAndDropOffsets": {
+      "type": "object",
+      "required": ["pickUpOffset", "dropOffset"],
+      "properties": {
+        "pickUpOffset": {
+          "$ref": "#/definitions/coordinates",
+          "description": "Offset added to calculate pick-up coordinates."
+        },
+        "dropOffset": {
+          "$ref": "#/definitions/coordinates",
+          "description": "Offset added to calculate drop coordinates."
+        }
+      }
     }
   },
   "type": "object",
@@ -343,21 +357,23 @@
     },
     "gripperOffsets": {
       "type": "object",
-      "description": "Offsets to be added when calculating the coordinates a gripper should go to when picking up or dropping a labware on this labware.",
+      "description": "Offsets to add when picking up or dropping another labware stacked atop this one. Do not use this to adjust the position of the gripper paddles relative to this labware or the child labware; use `gripHeightFromLabwareBottom` on this definition or the child's definition for that.",
+      "additionalProperties": {
+        "$ref": "#/definitions/pickUpAndDropOffsets",
+        "description": "Properties here are named for, and matched based on, the deck slot that this labware is atop--or, if this labware is atop a module, the deck slot that that module is atop."
+      },
       "properties": {
         "default": {
-          "type": "object",
-          "properties": {
-            "pickUpOffset": {
-              "$ref": "#/definitions/coordinates",
-              "description": "Offset added to calculate pick-up coordinates of a labware placed on this labware."
-            },
-            "dropOffset": {
-              "$ref": "#/definitions/coordinates",
-              "description": "Offset added to calculate drop coordinates of a labware placed on this labware."
-            }
-          },
-          "required": ["pickUpOffset", "dropOffset"]
+          "$ref": "#/definitions/pickUpAndDropOffsets",
+          "description": "The offsets to use if there's no slot-specific match in `additionalProperties`."
+        },
+        "lidOffsets": {
+          "$ref": "#/definitions/pickUpAndDropOffsets",
+          "description": "Additional offsets for gripping this labware, if this labware is a lid. Beware this property's placement: instead of affecting the labware stacked atop this labware, like the rest of the `gripperOffsets` properties, it affects this labware."
+        },
+        "lidDisposalOffsets": {
+          "$ref": "#/definitions/pickUpAndDropOffsets",
+          "description": "Additional offsets for gripping this labware, if this labware is a lid and it's being moved to a trash bin. Beware this property's placement: instead of affecting the labware stacked atop this labware, like the rest of the `gripperOffsets` properties, it affects this labware."
         }
       }
     },

shared-data/labware/schemas/3.json

@@ -65,6 +65,20 @@
         }
       }
     },
+    "pickUpAndDropOffsets": {
+      "type": "object",
+      "required": ["pickUpOffset", "dropOffset"],
+      "properties": {
+        "pickUpOffset": {
+          "$ref": "#/definitions/coordinates",
+          "description": "Offset added to calculate pick-up coordinates."
+        },
+        "dropOffset": {
+          "$ref": "#/definitions/coordinates",
+          "description": "Offset added to calculate drop coordinates."
+        }
+      }
+    },
     "SphericalSegment": {
       "type": "object",
       "description": "A partial sphere shaped section at the bottom of the well.",
@@ -538,21 +552,23 @@
     },
     "gripperOffsets": {
       "type": "object",
-      "description": "Offsets to be added when calculating the coordinates a gripper should go to when picking up or dropping a labware on this labware.",
+      "description": "Offsets to add when picking up or dropping another labware stacked atop this one. Do not use this to adjust the position of the gripper paddles relative to this labware or the child labware; use `gripHeightFromLabwareBottom` on this definition or the child's definition for that.",
+      "additionalProperties": {
+        "$ref": "#/definitions/pickUpAndDropOffsets",
+        "description": "Properties here are named for, and matched based on, the deck slot that this labware is atop--or, if this labware is atop a module, the deck slot that that module is atop."
+      },
       "properties": {
         "default": {
-          "type": "object",
-          "properties": {
-            "pickUpOffset": {
-              "$ref": "#/definitions/coordinates",
-              "description": "Offset added to calculate pick-up coordinates of a labware placed on this labware."
-            },
-            "dropOffset": {
-              "$ref": "#/definitions/coordinates",
-              "description": "Offset added to calculate drop coordinates of a labware placed on this labware."
-            }
-          },
-          "required": ["pickUpOffset", "dropOffset"]
+          "$ref": "#/definitions/pickUpAndDropOffsets",
+          "description": "The offsets to use if there's no slot-specific match in `additionalProperties`."
+        },
+        "lidOffsets": {
+          "$ref": "#/definitions/pickUpAndDropOffsets",
+          "description": "Additional offsets for gripping this labware, if this labware is a lid. Beware this property's placement: instead of affecting the labware stacked atop this labware, like the rest of the `gripperOffsets` properties, it affects this labware."
+        },
+        "lidDisposalOffsets": {
+          "$ref": "#/definitions/pickUpAndDropOffsets",
+          "description": "Additional offsets for gripping this labware, if this labware is a lid and it's being moved to a trash bin. Beware this property's placement: instead of affecting the labware stacked atop this labware, like the rest of the `gripperOffsets` properties, it affects this labware."
         }
       }
     },

shared-data/module/schemas/3.json

@@ -141,7 +141,7 @@
     },
     "gripperOffsets": {
       "type": "object",
-      "description": "Offsets to be added when calculating the coordinates a gripper should go to when picking up or dropping a labware on a module.",
+      "description": "Offsets to be added when calculating the coordinates a gripper should go to when picking up or dropping a labware on this module.",
       "properties": {
         "default": {
           "type": "object",