From 2376f87add7bb8f56c3dbcafb7672076880834bf Mon Sep 17 00:00:00 2001 From: Seth Foster Date: Wed, 12 Jul 2023 14:48:42 -0400 Subject: [PATCH] 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. --- .../instruments/ot3/gripper.py | 27 ++-- .../hardware_control/ot3_calibration.py | 130 +++++++++--------- api/src/opentrons/hardware_control/ot3api.py | 74 ++++++---- .../hardware_control/test_gripper.py | 8 +- .../hardware_control/test_ot3_api.py | 1 + 5 files changed, 133 insertions(+), 107 deletions(-) diff --git a/api/src/opentrons/hardware_control/instruments/ot3/gripper.py b/api/src/opentrons/hardware_control/instruments/ot3/gripper.py index 6da1f8b428e..e2d44bfce0c 100644 --- a/api/src/opentrons/hardware_control/instruments/ot3/gripper.py +++ b/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 diff --git a/api/src/opentrons/hardware_control/ot3_calibration.py b/api/src/opentrons/hardware_control/ot3_calibration.py index 56d9b82a77c..c092c1e0056 100644 --- a/api/src/opentrons/hardware_control/ot3_calibration.py +++ b/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( diff --git a/api/src/opentrons/hardware_control/ot3api.py b/api/src/opentrons/hardware_control/ot3api.py index 9f619417bad..66b9ee0bacd 100644 --- a/api/src/opentrons/hardware_control/ot3api.py +++ b/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, diff --git a/api/tests/opentrons/hardware_control/test_gripper.py b/api/tests/opentrons/hardware_control/test_gripper.py index 20ec386e155..1578538b777 100644 --- a/api/tests/opentrons/hardware_control/test_gripper.py +++ b/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 diff --git a/api/tests/opentrons/hardware_control/test_ot3_api.py b/api/tests/opentrons/hardware_control/test_ot3_api.py index 7a56b988b86..9e1aa404a7e 100644 --- a/api/tests/opentrons/hardware_control/test_ot3_api.py +++ b/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: