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, app): Clear instrument offset before performing deck calibration #6208

Merged
merged 2 commits into from
Jul 28, 2020

Conversation

Laura-Danielle
Copy link
Contributor

@Laura-Danielle Laura-Danielle commented Jul 23, 2020

Overview

Closes #5022 by resetting instrument offset for all pipettes any time a new deck calibration begins. Some design changes were also added in the app by following instructions from this design.

Changelog

  • Add messaging in deck cal CLI about re-doing pipette calibration (instrument offset does not get taken into account here because robot settings/deck cal are reset before beginning the program)
  • Modify the app according to the design above
  • The robot no longer restarts after a deck calibration
  • Reset instrument offset in endpoints.py and reload the current pip instrument offset to be (0, 0, 0)
  • Remove feature flags for v1 in endpoints.py as it's no longer relevant

Review requests

Please check that the code place for resetting instrument offset etc are OK.

Test Plan

  1. Verify the issue with the robot and app on 3.19 or edge
    a. Modify your instrument offset (the value in /data/robot_settings.json under instrument_offset and the mount and type of pipette you'll use for deck calibration - the output of tip probe) to be bad, for instance having a value of 20 in its z component.
    b. Run deck calibration, trying to do a good job.
    c. verify that after the robot restarts, the deck calibration is obviously incorrect - if you modified the z component of instrument offset, the deck calibration z offset should be very wrong
  2. Verify that this PR fixes the issue, after putting this PR on the robot with make push and building the app on this pr with make -C app dev
    a. Edit /data/robot_settings.json and verify that the value under instrument_offset for the mount and type of pipette used for deck calibration is still bad and large
    b. Run deck calibration, trying to do a good job
    c. When ending deck calibration, the button should say "save and exit" rather than "save and restart robot". The robot should not restart when you click the button
    d. Edit /data/robot_settings.json and verify that the instrument_offset for the mount and type of pipette you did deck calibration with is now (0, 0, 0)
    e. Edit /data/gantry_calibration.json and verify that the deck calibration is not bad in the same way it was in 1c
    f. Run the calibrate-to-crosses protocol used by the labware creator. Run tip probe and check that the pipettes move to the crosses accurately (or as accurately as they normally do).

Let me know if you have any other questions!

Risk assessment

Medium. There was concern that customers would be confused by the new messaging at the end of the deck calibration program in the app. Please make sure we are OK with this language before merging.

@Laura-Danielle Laura-Danielle added app Affects the `app` project api Affects the `api` project ready for review labels Jul 23, 2020
@Laura-Danielle Laura-Danielle requested review from pantslakz and a team July 23, 2020 21:17
@Laura-Danielle Laura-Danielle requested review from a team as code owners July 23, 2020 21:17
@Laura-Danielle Laura-Danielle requested review from ahiuchingau and mcous and removed request for a team July 23, 2020 21:18
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.

Python side looks good (haven't tested though) but we should change the FE copy a bit

You must restart your robot to finish the initial robot calibration
process and have the new settings take effect. It may take several
minutes for your robot to restart.
Please note that as a result of the new deck calibration data, your
Copy link
Member

Choose a reason for hiding this comment

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

Maybe cut this para as discussed yesterday? I think just the final para ("In order to effectively use..." is enough of a warning.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah ok, I misunderstood. Will remove it.

@Laura-Danielle Laura-Danielle requested a review from sfoster1 July 23, 2020 22:14
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.

LGTM pending QA and UX review

@sfoster1 sfoster1 force-pushed the api_ignore_instrument_offset_deck_cal branch from 0275770 to 4631945 Compare July 27, 2020 14:35
@codecov
Copy link

codecov bot commented Jul 27, 2020

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff           @@
##             edge    #6208   +/-   ##
=======================================
  Coverage        ?   79.58%           
=======================================
  Files           ?      199           
  Lines           ?    18420           
  Branches        ?        0           
=======================================
  Hits            ?    14660           
  Misses          ?     3760           
  Partials        ?        0           
Impacted Files Coverage Δ
opentrons/protocol_api/geometry.py 93.60% <0.00%> (ø)
opentrons/calibration_storage/types.py 100.00% <0.00%> (ø)
opentrons/config/feature_flags.py 94.11% <0.00%> (ø)
..._server/service/legacy/routers/deck_calibration.py 95.45% <0.00%> (ø)
...ver/service/session/session_types/check_session.py 95.34% <0.00%> (ø)
...-server/robot_server/service/legacy/models/logs.py 100.00% <0.00%> (ø)
...r/robot_server/service/legacy/models/networking.py 100.00% <0.00%> (ø)
opentrons/legacy_api/robot/mover.py 88.63% <0.00%> (ø)
opentrons/commands/types.py 100.00% <0.00%> (ø)
update-server/otupdate/buildroot/config.py 92.50% <0.00%> (ø)
... and 189 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 0e1b15b...4631945. Read the comment docs.

Copy link

@nusrat813 nusrat813 left a comment

Choose a reason for hiding this comment

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

Tested - working as intended

@sfoster1 sfoster1 merged commit cefa633 into edge Jul 28, 2020
@sfoster1 sfoster1 deleted the api_ignore_instrument_offset_deck_cal branch July 28, 2020 13:19
@SyntaxColoring
Copy link
Contributor

@Laura-Danielle

by resetting instrument offset for all pipettes any time a new deck calibration begins

If you start and then abort a deck calibration, is the pipette calibration still cleared? (If so, I don't think it would be a big problem—just looking out for surprises.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api Affects the `api` project app Affects the `app` project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pipette calibration poisons deck calibration
5 participants