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(robot-server,app): add download deck calibration button #6453

Merged
merged 6 commits into from
Sep 3, 2020

Conversation

ahiuchingau
Copy link
Contributor

@ahiuchingau ahiuchingau commented Aug 27, 2020

closes #6055

Overview

This PR adds a download deck calibration button per this design.

Changelog

Robot server:

  • add DeckCalibrationData model with fields: type, matrix, lastModified, pipetteCalibratedWith, tiprack
  • update get calibration status endpoint based on the deck calibration type
    • old deck calibration data found in /data/ will only contain the type MatrixType.affine and the matrix data
    • new deck calibration data in /data/robot/ will have the type MatrixType.attitude, matrix data, information on the pipette and tiprack used in calibration

App:

  • new DeckCalibrationDownload component that renders whenever the deck calibration status is not null
    • renders last modified date time only if the deck calibration type is attitude (affine deck cal doesn't contain info on when it was last modified)

Component:

  • add a download icon

Review requests

Put this on your robot, you should be able to download your deck calibration file in robot settings on the app:

  1. enable the calibration overhaul feature flag on your robot
  2. complete new deck calibration if you haven't already
  3. you should be able to see when the deck calibration was last calibrated as follows
    Screen Shot 2020-08-27 at 4 37 34 PM
  4. download calibration file, the default file name should be {your-robot-name}-deck-calibration.json
  5. the downloaded file's format should look like:
{
  type: 'attitude',
  matrix: [[]],
  lastModified: 'somedatetime',
  pipetteLastCalibratedWith: 'pipetteID',
  tiprack: 'tiprack hash'
}
  1. make sure the info is the same as the data/robot/deck_calibration.json
  2. switch off the calibration overhaul feature flag
  3. see that the last modified field disappears
  4. download calibration
  5. the downloaded format contain the same info as the old deck calibration data in data/deck_calibration.json
{
  type: 'affine',
  matrix: [[]]
}

Risk assessment

Medium since this PR modifies an existing endpoint and alters the look of the robot settings tab on the app.

@ahiuchingau ahiuchingau added app Affects the `app` project ready for review robot server Affects the `robot-server` project labels Aug 27, 2020
@ahiuchingau ahiuchingau requested review from mcous, b-cooper, Laura-Danielle and a team August 27, 2020 20:48
@ahiuchingau ahiuchingau requested review from a team as code owners August 27, 2020 20:48
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 in general but there's some little stuff to change.

Field(...,
description="The type of deck calibration matrix:"
"affine or attitude")
matrix: typing.List[typing.List[float]] = \
Copy link
Member

Choose a reason for hiding this comment

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

This could I think be done with tuples to properly set the length of each row and the number of columns (at least, I think - I assume that pydantic properly translates that into a json array with a fixed number of elements)

[number, number, number, number],
[number, number, number, number]
],
data: DeckCalibrationData,
Copy link
Member

Choose a reason for hiding this comment

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

If we're going to keep the app compatible with the last version of the robot as well as the current one, we should probably handle it here. I think the general use of null coalescence makes it work, but maybe add a test based on the robot returning the old style of the affine matrix directly in data?

@@ -83,8 +85,21 @@ async def post_calibration_deck(operation: DeckCalibrationDispatch) \
async def get_calibration_status(
hardware: ThreadManager = Depends(get_hardware)) -> CalibrationStatus:
robot_conf = robot_configs.load()
if ff.enable_calibration_overhaul():
deck_cal = robot_cal.load().deck_calibration
Copy link
Member

Choose a reason for hiding this comment

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

We should get the loaded version from the hardware controller if possible. I'd like to start avoiding reloading stuff from disk every time.

type: affine
matrix:
- &matrix_row
- !anyfloat
Copy link
Member

Choose a reason for hiding this comment

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

probably needs one fewer !anyfloat

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought it should be 4 in this case because an affine matrix would still have 4 columns

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I think you're right

@ahiuchingau ahiuchingau requested a review from sfoster1 September 2, 2020 03:43
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.

Code LGTM once you fix the type checks

@ahiuchingau ahiuchingau merged commit b3b365d into edge Sep 3, 2020
@ahiuchingau ahiuchingau deleted the app-robot-server_download-deck-cal branch September 3, 2020 16:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
app Affects the `app` project robot server Affects the `robot-server` project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat: make it easier for support to see a user's deck calibration
2 participants