refactor(api): configure instrs only if needed (#13085)

We've had this consistent problem with race conditions around
cache_instruments, which gets called when something hits /instruments.
It can reconfigure a bunch of internal data caches and also do things
like change the active currents on the axes. We used to do this pretty
often because it was the only time we actually checked what instruments
were connected. We configured everything unconditionally.

However, the app is basically always polling /instruments. That means
that if a process is running - a protocol, a calibration - some app is
going to hit /instruments and cause a current reconfiguration and
anything that requires a specific current will break, for instance. This
is bad!

The fix is to add in a concept from the OT2, of only running a system
reconfiguration if something actually changed since the last
cache_pipette. It's really easy to make this comparison, since we cache
stuff anyway - that's the point of cache pipette - and a lot of the
worker functions we call during the process actually return a bool to
let us know we can skip config anyway.

api/src/opentrons/hardware_control/instruments/ot3/gripper.py

@@ -3,7 +3,7 @@
 """ Classes and functions for gripper state tracking
 """
 import logging
-from typing import Any, Optional, Set
+from typing import Any, Optional, Set, Tuple
 
 from opentrons.types import Point
 from opentrons.config import gripper_config
@@ -237,7 +237,7 @@ def _reload_gripper(
     new_config: GripperDefinition,
     attached_instr: Gripper,
     cal_offset: GripperCalibrationOffset,
-) -> Gripper:
+) -> Tuple[Gripper, bool]:
     # Once we have determined that the new and attached grippers
     # are similar enough that we might skip, see if the configs
     # match closely enough.
@@ -247,7 +247,7 @@ def _reload_gripper(
         and cal_offset == attached_instr._calibration_offset
     ):
         # Same config, good enough
-        return attached_instr
+        return attached_instr, True
     else:
         newdict = new_config.dict()
         olddict = attached_instr.config.dict()
@@ -257,22 +257,25 @@ def _reload_gripper(
                 changed.add(k)
         if changed.intersection(RECONFIG_KEYS):
             # Something has changed that requires reconfig
-            return Gripper(
-                new_config,
-                cal_offset,
-                attached_instr._gripper_id,
+            return (
+                Gripper(
+                    new_config,
+                    cal_offset,
+                    attached_instr._gripper_id,
+                ),
+                False,
             )
         else:
             # update just the cal offset and update info
             attached_instr._calibration_offset = cal_offset
-            return attached_instr
+            return attached_instr, True
 
 
 def compare_gripper_config_and_check_skip(
     freshly_detected: AttachedGripper,
     attached: Optional[Gripper],
     cal_offset: GripperCalibrationOffset,
-) -> Optional[Gripper]:
+) -> Tuple[Optional[Gripper], bool]:
     """
     Given the gripper config for an attached gripper (if any) freshly read
     from disk, and any attached instruments,
@@ -288,7 +291,7 @@ def compare_gripper_config_and_check_skip(
     if not config and not attached:
         # nothing attached now, nothing used to be attached, nothing
         # to reconfigure
-        return attached
+        return attached, True
 
     if config and attached:
         # something was attached and something is attached. are they
@@ -298,6 +301,6 @@ def compare_gripper_config_and_check_skip(
             return _reload_gripper(config, attached, cal_offset)
 
     if config:
-        return Gripper(config, cal_offset, serial)
+        return Gripper(config, cal_offset, serial), False
     else:
-        return None
+        return None, False

api/src/opentrons/hardware_control/ot3_calibration.py

@@ -726,19 +726,18 @@ async def calibrate_gripper_jaw(
     the average of the pin offsets, which can be obtained by passing the
     two offsets into the `gripper_pin_offsets_mean` func.
     """
-    async with hcapi.instrument_cache_lock():
-        try:
-            await hcapi.reset_instrument_offset(OT3Mount.GRIPPER)
-            hcapi.add_gripper_probe(probe)
-            await hcapi.grip(GRIPPER_GRIP_FORCE)
-            offset = await _calibrate_mount(
-                hcapi, OT3Mount.GRIPPER, slot, method, raise_verify_error
-            )
-            LOG.info(f"Gripper {probe.name} probe offset: {offset}")
-            return offset
-        finally:
-            hcapi.remove_gripper_probe()
-            await hcapi.ungrip()
+    try:
+        await hcapi.reset_instrument_offset(OT3Mount.GRIPPER)
+        hcapi.add_gripper_probe(probe)
+        await hcapi.grip(GRIPPER_GRIP_FORCE)
+        offset = await _calibrate_mount(
+            hcapi, OT3Mount.GRIPPER, slot, method, raise_verify_error
+        )
+        LOG.info(f"Gripper {probe.name} probe offset: {offset}")
+        return offset
+    finally:
+        hcapi.remove_gripper_probe()
+        await hcapi.ungrip()
 
 
 async def calibrate_gripper(
@@ -766,17 +765,14 @@ async def calibrate_pipette(
     tip has been attached, or the conductive probe has been attached,
     or the probe has been lowered).
     """
-    async with hcapi.instrument_cache_lock():
-        try:
-            await hcapi.reset_instrument_offset(mount)
-            await hcapi.add_tip(mount, hcapi.config.calibration.probe_length)
-            offset = await _calibrate_mount(
-                hcapi, mount, slot, method, raise_verify_error
-            )
-            await hcapi.save_instrument_offset(mount, offset)
-            return offset
-        finally:
-            await hcapi.remove_tip(mount)
+    try:
+        await hcapi.reset_instrument_offset(mount)
+        await hcapi.add_tip(mount, hcapi.config.calibration.probe_length)
+        offset = await _calibrate_mount(hcapi, mount, slot, method, raise_verify_error)
+        await hcapi.save_instrument_offset(mount, offset)
+        return offset
+    finally:
+        await hcapi.remove_tip(mount)
 
 
 async def calibrate_module(
@@ -800,39 +796,38 @@ async def calibrate_module(
     The robot should be homed before calling this function.
     """
 
-    async with hcapi.instrument_cache_lock():
-        try:
-            # add the probe depending on the mount
-            if mount == OT3Mount.GRIPPER:
-                hcapi.add_gripper_probe(GripperProbe.FRONT)
-            else:
-                await hcapi.add_tip(mount, hcapi.config.calibration.probe_length)
+    try:
+        # add the probe depending on the mount
+        if mount == OT3Mount.GRIPPER:
+            hcapi.add_gripper_probe(GripperProbe.FRONT)
+        else:
+            await hcapi.add_tip(mount, hcapi.config.calibration.probe_length)
 
-            LOG.info(
-                f"Starting module calibration for {module_id} at {nominal_position} using {mount}"
-            )
-            # FIXME (ba, 2023-04-04): Well B1 of the module adapter definition includes the z prep offset
-            # of 13x13mm in the nominial position, but we are still using PREP_OFFSET_DEPTH in
-            # find_calibration_structure_height which effectively doubles the offset. We plan
-            # on removing PREP_OFFSET_DEPTH in the near future, but for now just subtract PREP_OFFSET_DEPTH
-            # from the nominal position so we dont have to alter any other part of the system.
-            nominal_position = nominal_position - PREP_OFFSET_DEPTH
-            offset = await find_calibration_structure_position(
-                hcapi,
-                mount,
-                nominal_position,
-                method=CalibrationMethod.BINARY_SEARCH,
-                target=CalibrationTarget.DECK_OBJECT,
-            )
-            await hcapi.save_module_offset(module_id, mount, slot, offset)
-            return offset
-        finally:
-            # remove probe
-            if mount == OT3Mount.GRIPPER:
-                hcapi.remove_gripper_probe()
-                await hcapi.ungrip()
-            else:
-                await hcapi.remove_tip(mount)
+        LOG.info(
+            f"Starting module calibration for {module_id} at {nominal_position} using {mount}"
+        )
+        # FIXME (ba, 2023-04-04): Well B1 of the module adapter definition includes the z prep offset
+        # of 13x13mm in the nominial position, but we are still using PREP_OFFSET_DEPTH in
+        # find_calibration_structure_height which effectively doubles the offset. We plan
+        # on removing PREP_OFFSET_DEPTH in the near future, but for now just subtract PREP_OFFSET_DEPTH
+        # from the nominal position so we dont have to alter any other part of the system.
+        nominal_position = nominal_position - PREP_OFFSET_DEPTH
+        offset = await find_calibration_structure_position(
+            hcapi,
+            mount,
+            nominal_position,
+            method=CalibrationMethod.BINARY_SEARCH,
+            target=CalibrationTarget.DECK_OBJECT,
+        )
+        await hcapi.save_module_offset(module_id, mount, slot, offset)
+        return offset
+    finally:
+        # remove probe
+        if mount == OT3Mount.GRIPPER:
+            hcapi.remove_gripper_probe()
+            await hcapi.ungrip()
+        else:
+            await hcapi.remove_tip(mount)
 
 
 async def calibrate_belts(
@@ -852,18 +847,17 @@ async def calibrate_belts(
     -------
     A listed matrix of the linear transform in the x and y dimensions that accounts for the stretch of the gantry x and y belts.
     """
-    async with hcapi.instrument_cache_lock():
-        if mount == OT3Mount.GRIPPER:
-            raise RuntimeError("Must use pipette mount, not gripper")
-        try:
-            hcapi.reset_deck_calibration()
-            await hcapi.add_tip(mount, hcapi.config.calibration.probe_length)
-            belt_attitude = await _determine_transform_matrix(hcapi, mount)
-            save_robot_belt_attitude(belt_attitude, pipette_id)
-            return belt_attitude
-        finally:
-            hcapi.load_deck_calibration()
-            await hcapi.remove_tip(mount)
+    if mount == OT3Mount.GRIPPER:
+        raise RuntimeError("Must use pipette mount, not gripper")
+    try:
+        hcapi.reset_deck_calibration()
+        await hcapi.add_tip(mount, hcapi.config.calibration.probe_length)
+        belt_attitude = await _determine_transform_matrix(hcapi, mount)
+        save_robot_belt_attitude(belt_attitude, pipette_id)
+        return belt_attitude
+    finally:
+        hcapi.load_deck_calibration()
+        await hcapi.remove_tip(mount)
 
 
 def apply_machine_transform(

api/src/opentrons/hardware_control/ot3api.py

@@ -7,7 +7,6 @@
 from collections import OrderedDict
 from typing import (
     AsyncIterator,
-    AsyncGenerator,
     cast,
     Callable,
     Dict,
@@ -194,7 +193,6 @@ def __init__(
         self._config = config
         self._backend = backend
         self._loop = loop
-        self._instrument_cache_lock = asyncio.Lock()
 
         self._callbacks: Set[HardwareEventHandler] = set()
         # {'X': 0.0, 'Y': 0.0, 'Z': 0.0, 'A': 0.0, 'B': 0.0, 'C': 0.0}
@@ -324,6 +322,7 @@ async def build_hardware_controller(
         backend.add_door_state_listener(api_instance._update_door_state)
         checked_loop.create_task(backend.watch(loop=checked_loop))
         backend.initialized = True
+        await api_instance.refresh_positions()
         return api_instance
 
     @classmethod
@@ -368,6 +367,7 @@ async def build_hardware_simulator(
         )
         backend.module_controls = module_controls
         await backend.watch(api_instance.loop)
+        await api_instance.refresh_positions()
         return api_instance
 
     def __repr__(self) -> str:
@@ -504,13 +504,13 @@ async def cache_pipette(
         mount: OT3Mount,
         instrument_data: OT3AttachedPipette,
         req_instr: Optional[PipetteName],
-    ) -> None:
+    ) -> bool:
         """Set up pipette based on scanned information."""
         config = instrument_data.get("config")
         pip_id = instrument_data.get("id")
         pip_offset_cal = load_pipette_offset(pip_id, mount)
 
-        p, _ = load_from_config_and_check_skip(
+        p, skipped = load_from_config_and_check_skip(
             config,
             self._pipette_handler.hardware_instruments[mount],
             req_instr,
@@ -520,16 +520,18 @@ async def cache_pipette(
         self._pipette_handler.hardware_instruments[mount] = p
         # TODO (lc 12-5-2022) Properly support backwards compatibility
         # when applicable
+        return skipped
 
-    async def cache_gripper(self, instrument_data: AttachedGripper) -> None:
+    async def cache_gripper(self, instrument_data: AttachedGripper) -> bool:
         """Set up gripper based on scanned information."""
         grip_cal = load_gripper_calibration_offset(instrument_data.get("id"))
-        g = compare_gripper_config_and_check_skip(
+        g, skipped = compare_gripper_config_and_check_skip(
             instrument_data,
             self._gripper_handler._gripper,
             grip_cal,
         )
         self._gripper_handler.gripper = g
+        return skipped
 
     def get_all_attached_instr(self) -> Dict[OT3Mount, Optional[InstrumentDict]]:
         # NOTE (spp, 2023-03-07): The return type of this method indicates that
@@ -551,21 +553,25 @@ async def cache_instruments(
         Scan the attached instruments, take necessary configuration actions,
         and set up hardware controller internal state if necessary.
         """
-        if self._instrument_cache_lock.locked():
-            self._log.info("Instrument cache is locked, not refreshing")
-            return
-        async with self.instrument_cache_lock():
-            await self._cache_instruments(require)
+        skip_configure = await self._cache_instruments(require)
+        self._log.info(
+            f"Instrument model cache updated, skip configure: {skip_configure}"
+        )
+        if not skip_configure:
             await self._configure_instruments()
 
-    async def _cache_instruments(
+    async def _cache_instruments(  # noqa: C901
         self, require: Optional[Dict[top_types.Mount, PipetteName]] = None
-    ) -> None:
-        """Actually cache instruments and scan network."""
-        self._log.info("Updating instrument model cache")
+    ) -> bool:
+        """Actually cache instruments and scan network.
+
+        Returns True if nothing changed since the last call and can skip any follow-up
+        configuration; False if we need to reconfigure.
+        """
         checked_require = {
             OT3Mount.from_mount(m): v for m, v in (require or {}).items()
         }
+        skip_configure = True
         for mount, name in checked_require.items():
             # TODO (lc 12-5-2022) cache instruments should be receiving
             # a pipette type / channels rather than the named config.
@@ -579,27 +585,50 @@ async def _cache_instruments(
             found = await self._backend.get_attached_instruments(checked_require)
 
         if OT3Mount.GRIPPER in found.keys():
-            await self.cache_gripper(cast(AttachedGripper, found.get(OT3Mount.GRIPPER)))
+            # Is now a gripper, ask if it's ok to skip
+            gripper_skip = await self.cache_gripper(
+                cast(AttachedGripper, found.get(OT3Mount.GRIPPER))
+            )
+            skip_configure &= gripper_skip
+            if not gripper_skip:
+                self._log.info(
+                    "cache_instruments: must configure because gripper now attached or changed config"
+                )
         elif self._gripper_handler.gripper:
+            # Is no gripper, have a cached gripper, definitely need to reconfig
             await self._gripper_handler.reset()
+            skip_configure = False
+            self._log.info("cache_instruments: must configure because gripper now gone")
 
         for pipette_mount in [OT3Mount.LEFT, OT3Mount.RIGHT]:
             if pipette_mount in found.keys():
+                # is now a pipette, ask if we need to reconfig
                 req_instr_name = checked_require.get(pipette_mount, None)
-                await self.cache_pipette(
+                pipette_skip = await self.cache_pipette(
                     pipette_mount,
                     cast(OT3AttachedPipette, found.get(pipette_mount)),
                     req_instr_name,
                 )
-            else:
+                skip_configure &= pipette_skip
+                if not pipette_skip:
+                    self._log.info(
+                        f"cache_instruments: must configure because {pipette_mount.name} now attached or changed"
+                    )
+
+            elif self._pipette_handler.hardware_instruments[pipette_mount]:
+                # Is no pipette, have a cached pipette, need to reconfig
+                skip_configure = False
                 self._pipette_handler.hardware_instruments[pipette_mount] = None
+                self._log.info(
+                    f"cache_instruments: must configure because {pipette_mount.name} now empty"
+                )
 
-        await self.refresh_positions()
+        return skip_configure
 
     async def _configure_instruments(self) -> None:
         """Configure instruments"""
-        await self._backend.update_motor_status()
         await self.set_gantry_load(self._gantry_load_from_instruments())
+        await self.refresh_positions()
 
     @ExecutionManagerProvider.wait_for_running
     async def _update_position_estimation(
@@ -2164,11 +2193,6 @@ async def capacitive_probe(
             await self.move_to(mount, pass_start_pos)
         return moving_axis.of_point(end_pos)
 
-    @contextlib.asynccontextmanager
-    async def instrument_cache_lock(self) -> AsyncGenerator[None, None]:
-        async with self._instrument_cache_lock:
-            yield
-
     async def capacitive_sweep(
         self,
         mount: OT3Mount,

api/tests/opentrons/hardware_control/test_gripper.py

@@ -80,9 +80,13 @@ def test_reload_instrument_cal_ot3(fake_offset: "GripperCalibrationOffset") -> N
         source=cal_types.SourceType.user,
         status=cal_types.CalibrationStatus(),
     )
-    new_gripper = gripper._reload_gripper(old_gripper.config, old_gripper, new_cal)
+    new_gripper, skip = gripper._reload_gripper(
+        old_gripper.config, old_gripper, new_cal
+    )
 
-    # it's the same pipette
+    # it's the same gripper
     assert new_gripper == old_gripper
+    # we said upstream could skip
+    assert skip
     # only pipette offset has been updated
     assert new_gripper._calibration_offset == new_cal

api/tests/opentrons/hardware_control/test_ot3_api.py

@@ -447,6 +447,7 @@ async def prepare_for_mock_blowout(
     )
     instr_data = OT3AttachedPipette(config=pipette_config, id="fakepip")
     await ot3_hardware.cache_pipette(mount, instr_data, None)
+    await ot3_hardware.refresh_positions()
     with patch.object(
         ot3_hardware, "pick_up_tip", AsyncMock(spec=ot3_hardware.liquid_probe)
     ) as mock_tip_pickup: