Skip to content

Commit

Permalink
fix(robot-server): dont let /instruments block (#14608)
Browse files Browse the repository at this point in the history
It calls cache_instruments and that can block because it takes the
motion lock, but we really don't want that. We don't mind if
cache_instruments doesn't get called on the flex because it's sort of a
secondary functionality, so just bail early in this case.

## Testing
- Run on a robot and do some `GET /instruments` while things are homign
and note that it instantly returns
- Do an attach flow and note that it still returns the new instruments.

Closes EXEC-298
  • Loading branch information
sfoster1 authored Mar 8, 2024
1 parent f50e261 commit a8d78a3
Show file tree
Hide file tree
Showing 5 changed files with 14 additions and 7 deletions.
4 changes: 3 additions & 1 deletion api/src/opentrons/hardware_control/api.py
Original file line number Diff line number Diff line change
Expand Up @@ -431,7 +431,9 @@ def has_gripper(self) -> bool:
return False

async def cache_instruments(
self, require: Optional[Dict[top_types.Mount, PipetteName]] = None
self,
require: Optional[Dict[top_types.Mount, PipetteName]] = None,
skip_if_would_block: bool = False,
) -> None:
"""
Scan the attached instruments, take necessary configuration actions,
Expand Down
6 changes: 5 additions & 1 deletion api/src/opentrons/hardware_control/ot3api.py
Original file line number Diff line number Diff line change
Expand Up @@ -653,12 +653,16 @@ def get_all_attached_instr(self) -> Dict[OT3Mount, Optional[InstrumentDict]]:

# TODO (spp, 2023-01-31): add unit tests
async def cache_instruments(
self, require: Optional[Dict[top_types.Mount, PipetteName]] = None
self,
require: Optional[Dict[top_types.Mount, PipetteName]] = None,
skip_if_would_block: bool = False,
) -> None:
"""
Scan the attached instruments, take necessary configuration actions,
and set up hardware controller internal state if necessary.
"""
if skip_if_would_block and self._motion_lock.locked():
return
async with self._motion_lock:
skip_configure = await self._cache_instruments(require)
if not skip_configure or not self._configured_since_update:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ def reset_instrument(self, mount: Optional[MountArgType] = None) -> None:
async def cache_instruments(
self,
require: Optional[Dict[Mount, PipetteName]] = None,
skip_if_would_block: bool = False,
) -> None:
"""
Scan the attached instruments, take necessary configuration actions,
Expand Down
2 changes: 1 addition & 1 deletion robot-server/robot_server/instruments/router.py
Original file line number Diff line number Diff line change
Expand Up @@ -216,7 +216,7 @@ async def _get_attached_instruments_ot3(
hardware: OT3HardwareControlAPI,
) -> PydanticResponse[SimpleMultiBody[AttachedItem]]:
# OT3
await hardware.cache_instruments()
await hardware.cache_instruments(skip_if_would_block=True)
response_data = await _get_instrument_data(hardware)
return await PydanticResponse.create(
content=SimpleMultiBody.construct(
Expand Down
8 changes: 4 additions & 4 deletions robot-server/tests/instruments/test_router.py
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,7 @@ async def test_get_all_attached_instruments(
subsystem=SubSystem.pipette_right,
)

async def rehearse_instrument_retrievals() -> None:
async def rehearse_instrument_retrievals(skip_if_would_block: bool = False) -> None:
decoy.when(ot3_hardware_api.attached_gripper).then_return(
cast(
GripperDict,
Expand Down Expand Up @@ -188,9 +188,9 @@ async def rehearse_instrument_retrievals() -> None:

# We use this convoluted way of testing to verify the important point that
# cache_instruments is called before fetching attached pipette and gripper data.
decoy.when(await ot3_hardware_api.cache_instruments()).then_do(
rehearse_instrument_retrievals
)
decoy.when(
await ot3_hardware_api.cache_instruments(skip_if_would_block=True)
).then_do(rehearse_instrument_retrievals)
decoy.when(ot3_hardware_api.get_instrument_offset(mount=OT3Mount.LEFT)).then_return(
PipetteOffsetSummary(
offset=Point(1, 2, 3),
Expand Down

0 comments on commit a8d78a3

Please sign in to comment.