Skip to content

Commit

Permalink
refactor(api): configure instrs only if needed (#13085)
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@sfoster1
sfoster1 authored Jul 12, 2023
1 parent 6a92280 commit 2376f87
Show file tree
Hide file tree
Showing 5 changed files with 133 additions and 107 deletions.
27 changes: 15 additions & 12 deletions api/src/opentrons/hardware_control/instruments/ot3/gripper.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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.
Expand All @@ -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()
Expand All @@ -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,
Expand All @@ -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
Expand All @@ -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
130 changes: 62 additions & 68 deletions api/src/opentrons/hardware_control/ot3_calibration.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down Expand Up @@ -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(
Expand All @@ -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(
Expand All @@ -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(
Expand Down
Loading

0 comments on commit 2376f87

Please sign in to comment.