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

feat(app): update pipette attach flow to include calibration #6760

Merged
merged 16 commits into from
Oct 14, 2020

Conversation

b-cooper
Copy link
Contributor

Overview

When attaching a pipette, if you do not currently have a pipette offset calibration stored for that
combination of pipette serial number and mount, attach pipette flow will directly feed into the
pipette offset calibration flow (which may also include tip length cal for the tip used in pipette
offset cal)

Closes #2130

Changelog

  • create new hook called useCalibratePipetteOffset that manages the complexities of pipette offset calibration setup and flow. It returns a callback to start the flow and the component that contains the wizard if the flow has started. This flexibility will allow us to anchor the pipette offset calibration flow in the many embedded locations that it needs to exist, without having to duplicate the pretty gnarly session request management concerns.
  • use this new hook in the final confirm screen of the change pipette flow, in the case that a pipette has been attached to a mount that doesn't have existing pipette offset calibration.
  • use the new hook in place of the old component in the stand alone "Calibrate Offset" button on the Pipettes Page

Review requests

  • Attach a pipette for which you don't have pipette offset calibration data, you should be prompted to proceed through the pipette offset calibration flow after successfully attaching the pipette
  • If you already have pipette offset calibration data for a given pipette on a mount, you should not see the "Proceed to Calibrate Pipette Offset" in the final screen of the attach pipette flow.
    -The standalone "CalibrateOffset" button on the Pipettes tab should function exactly as it has in the past, despite the fact that it is using the new hooks based "useCalibratePipetteOffset" implementation

Risk assessment

medium as this functionality is not being put behind a feature flag, considering it will be release along with everything else soon to have the enableCalibrationOverhaul gate removed.

When attaching a pipette, if you do not currently have a pipette offset calibration stored for that
combination of pipette serial number and mount, attach pipette flow will directly feed into the
pipette offset calibration flow (which may also include tip length cal for the tip used in pipette
offset cal)

Closes #2130
@b-cooper b-cooper requested a review from a team as a code owner October 13, 2020 22:39
@b-cooper b-cooper requested review from IanLondon and removed request for a team October 13, 2020 22:39
@b-cooper b-cooper added app Affects the `app` project hmg hardware, motion, and geometry ready for review labels Oct 13, 2020
@b-cooper b-cooper requested review from a team and Laura-Danielle and removed request for a team October 13, 2020 22:39
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.

Problems:

  • Styling nit: the continue button from leveling probably should be the same fixed-width as the confirm button, and there should be some space between the two.
    Screenshot from 2020-10-14 08-47-50

We may also want to have the second button be a SecondaryBtn and say something like "Exit without calibrating"

  • If you exit the offset calibration session early, it sends the command to move to "exited" but doesn't actually delete the session
  • Probably want the spinner to render with "Homing {mount} pipette" when you enter offset calibration rather than "Moving {mount} pipette up"

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.

We also really should put the new buttons behind a feature flag.

@codecov
Copy link

codecov bot commented Oct 14, 2020

Codecov Report

Merging #6760 into edge will decrease coverage by 8.46%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             edge    #6760      +/-   ##
==========================================
- Coverage   89.11%   80.65%   -8.47%     
==========================================
  Files         101      240     +139     
  Lines        4519    20641   +16122     
==========================================
+ Hits         4027    16647   +12620     
- Misses        492     3994    +3502     
Impacted Files Coverage Δ
opentrons/config/feature_flags.py 94.73% <0.00%> (ø)
opentrons/legacy_api/instruments/pipette.py 94.75% <0.00%> (ø)
opentrons/hardware_control/simulator_setup.py 97.95% <0.00%> (ø)
opentrons/api/routers.py 100.00% <0.00%> (ø)
opentrons/drivers/temp_deck/__init__.py 100.00% <0.00%> (ø)
opentrons/calibration_storage/file_operators.py 83.33% <0.00%> (ø)
opentrons/hardware_control/socket_server.py 87.57% <0.00%> (ø)
opentrons/commands/tree.py 100.00% <0.00%> (ø)
opentrons/protocols/implementations/well_grid.py 100.00% <0.00%> (ø)
opentrons/calibration_storage/modify.py 79.04% <0.00%> (ø)
... and 129 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 139e91b...f0d2975. Read the comment docs.

@b-cooper b-cooper requested a review from sfoster1 October 14, 2020 17:02
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.

Tested the flows in attach pipette and in attach pipette with leveling, and the pipette info button, LGTM

@b-cooper b-cooper merged commit c873113 into edge Oct 14, 2020
@b-cooper b-cooper deleted the app_pipette-attach-calibrate branch October 14, 2020 18:24
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.

Calibrate Pipette Mount: Prompt User After Pipette Change
2 participants