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(app): make cal commands chain instead of race #6561

Merged
merged 13 commits into from
Sep 18, 2020

Conversation

b-cooper
Copy link
Contributor

@b-cooper b-cooper commented Sep 16, 2020

Overview

In order to avoid various race conditions created by sequential commands being requested of the
robot during/before/after calibration sessions, create mechanism to ensure that they are dispatched
sequentially. Also ensure that wizard only closes on successful deletion, to avoid orphaned session.

Closes #6535

NOTE: these changes have been made within the generic CalibrationPanels component directory, which is only being used in the pipette offset calibration flow. The deck calibration and tip length calibration flows will be refactored to use the panels from this shared directory in a following PR.

TODO: unit tests for new useDispatchApiRequests hook, ensure unit tests for PipetteOffsetCalibrationControl cover new additions

Changelog

  • add a new utility hook called useDispatchApiRequests that is very similar to the existing useDispatchApiRequest, but accepts any number of api request actions and will only dispatch each sequential action once the request from the previous action has resolved (status !== PENDING).
  • plug this new hook into the pipette offset calibration flow for all sequential calls
  • remove temporary setTimeout fix for sequential calls
  • while hoisting dispatch machinery up to the PipetteOffsetCalibrationControl component, let it manage closing the wizard so as to avoid orphaned (undeleted) sessions on the robot that can't be viewed or "redeleted" on the app.

Review requests

  • run through the existing pipette offset calibration flow and ensure that all transitions occur as expected.

Risk assessment

low, this change is contained within the unreleased pipette offset calibration flow, though this functionality will be added to the other cal overhaul front-ends immediately following this PR merging.

In order to avoid various race conditions created by sequential commands being requested of the
robot during/before/after calibration sessions, create mechanism to ensure that they are dispatched
sequentially. Also ensure that wizard only closes on successful deletion, to avoid orphaned session.

Closes #6535
@b-cooper b-cooper added the WIP label Sep 16, 2020
@codecov
Copy link

codecov bot commented Sep 16, 2020

Codecov Report

❗ No coverage uploaded for pull request base (edge@1b6f3a6). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             edge    #6561   +/-   ##
=======================================
  Coverage        ?   80.23%           
=======================================
  Files           ?      236           
  Lines           ?    20492           
  Branches        ?        0           
=======================================
  Hits            ?    16442           
  Misses          ?     4050           
  Partials        ?        0           
Impacted Files Coverage Δ
opentrons/config/reset.py 86.95% <0.00%> (ø)
...t_server/service/legacy/models/deck_calibration.py 100.00% <0.00%> (ø)
opentrons/hardware_control/modules/thermocycler.py 94.15% <0.00%> (ø)
opentrons/calibration_storage/get.py 92.75% <0.00%> (ø)
robot-server/robot_server/service/access/router.py 73.33% <0.00%> (ø)
...ot-server/robot_server/robot/calibration/errors.py 100.00% <0.00%> (ø)
...rver/service/session/session_types/base_session.py 91.11% <0.00%> (ø)
...rver/service/session/session_types/null_session.py 95.23% <0.00%> (ø)
opentrons/protocols/execution/json_dispatchers.py 85.71% <0.00%> (ø)
opentrons/hardware_control/modules/mod_abc.py 77.52% <0.00%> (ø)
... and 226 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1b6f3a6...e80c5dc. Read the comment docs.

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 like some typos in the example for the new selector

app/src/robot-api/hooks.js Outdated Show resolved Hide resolved
app/src/robot-api/hooks.js Outdated Show resolved Hide resolved
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.

A quick test on my robot does indicate that these requests are chaining!

@b-cooper b-cooper added app Affects the `app` project hmg hardware, motion, and geometry and removed WIP labels Sep 17, 2020
@b-cooper b-cooper marked this pull request as ready for review September 17, 2020 20:31
@b-cooper b-cooper requested a review from a team as a code owner September 17, 2020 20:31
@b-cooper b-cooper requested review from IanLondon, a team, ahiuchingau and mcous and removed request for a team September 17, 2020 20:31
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.

lgtm, also cool

@b-cooper b-cooper merged commit 20f01bc into edge Sep 18, 2020
@b-cooper b-cooper deleted the app_chain-commands-responsibly branch September 18, 2020 16:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
app Affects the `app` project hmg hardware, motion, and geometry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pipette Offset Calibration: Refactor chained commands into epics
3 participants