Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(api): lock instrument cache during calibration #12577

Merged
merged 1 commit into from
Apr 27, 2023

Conversation

fsinapi
Copy link
Contributor

@fsinapi fsinapi commented Apr 27, 2023

Overview

If a desktop app is on the same network as a robot, it will poll the GET /instruments endpoint to get the current attached instruments. This process involves loading any saved calibration offsets in the hardware controller. If the robot is currently calibrating an axis, this is a very bad thing! The calibration offsets are cleared at the beginning of calibration and must remain cleared for the entirety of calibration.

This PR adds a simple asyncio lock to disable the cache_instruments function. The calibration functions acquire the lock for their entire duration, thus preventing the robot from accidentally applying unwanted offsets.

Test Plan

Push this to a robot and ping the GET /instruments endpoint both during calibration & before/after calibration, while monitoring the logs with journalctl --follow | grep "opentrons-api\["

  • During calibration, an entry should pop up saying "Instrument cache is locked, not refreshing" and calibration should complete fine
  • Outside of calibration, the logs should say "Updating instrument model cache"

The endpoint should always return valid data as well.

Changelog

  • Added a context manager to lock the ot3api cache_instruments function
  • Acquire aforementioned context manager during any calibration functions

Review requests

  • This worked fine on my testing through the ODD, do other workflows for calibration exist that may present an issue?

Risk assessment

- This lock is acquired as an asynccontextmanager from any of the ot3 calibration functions, so that they can clear out the calibration data without worrying about it coming back
- If the GET /instruments endpoint tries to call the cache function, it will refuse to update instead of wiping out good data
@fsinapi fsinapi requested a review from a team April 27, 2023 20:01
@fsinapi fsinapi requested a review from a team as a code owner April 27, 2023 20:01
@fsinapi fsinapi self-assigned this Apr 27, 2023
Copy link
Contributor

@Laura-Danielle Laura-Danielle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice solution :)

@codecov
Copy link

codecov bot commented Apr 27, 2023

Codecov Report

Merging #12577 (64ec956) into internal-release_0.5.0 (3c3003a) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@                   Coverage Diff                   @@
##           internal-release_0.5.0   #12577   +/-   ##
=======================================================
  Coverage                   73.59%   73.59%           
=======================================================
  Files                        2267     2267           
  Lines                       62352    62352           
  Branches                     6596     6596           
=======================================================
  Hits                        45886    45886           
  Misses                      14890    14890           
  Partials                     1576     1576           
Flag Coverage Δ
notify-server 89.13% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
api/src/opentrons/hardware_control/ot3api.py 79.37% <ø> (ø)

Copy link
Member

@sfoster1 sfoster1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, though I'm a little surprised this works without thread manager bridging

@fsinapi fsinapi merged commit 4f952ab into internal-release_0.5.0 Apr 27, 2023
@fsinapi fsinapi deleted the RQA-661_calibration_failure branch April 27, 2023 20:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants