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: