feat(engine): allow calibrateGripper command to save calibration data

[sanni-t]

Closes RLAB-198

Overview

Until now the calibrateGripper command only performed calibration of the specified gripper jaw and returned its offset. This PR adds the capability to have this command also save the final gripper offset once the offset for both the jaws is known.

Test Plan

Testing is blocked by #11807

Changelog

• updated calibrateGripper command to accept a new optional param otherProbeOffset and return a new optional field gripperOffset
• This gripperOffset is provided by the engine only when it has access to the current probe's offset and the other probe's offset is specified in otherProbeOffset.
• added tests and updated command schema

Review requests

• Does this behavior of the command make sense? I was originally working on adding an http endpoint that can be used to send a PATCH/ POST request to save calibration to disk when the two probes' offsets are provided but realized it was not a great idea for a few reasons-
  • it creates a one-off endpoint that doesn't match how we save any other calibration data
  • it requires building a lot of code around a very simple task that can be achieved in just a few lines of code in the existing command. I was also confused whether to make it a part of /calibration or /instrument (here's the first pass of endpoint)
  • requires the client to send one more request than necessary
  • if we were to match the behavior of the other 'calibration' commands, they handle saving the calibration too. So it felt odd to have just the gripper calibration command delegate the saving part to robot server.

Risk assessment

Very low. Doesn't change the existing command behavior

[codecov]

Codecov Report

Merging #12046 (b135ad8) into edge (3fa56d2) will increase coverage by 0.05%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             edge   #12046      +/-   ##
==========================================
+ Coverage   74.04%   74.10%   +0.05%     
==========================================
  Files        2191     2183       -8     
  Lines       60609    60654      +45     
  Branches     6416     6465      +49     
==========================================
+ Hits        44879    44946      +67     
+ Misses      14194    14142      -52     
- Partials     1536     1566      +30     

Flag	Coverage Δ
app	72.75% <ø> (+0.22%)	⬆️
shared-data	91.53% <ø> (+6.04%)	⬆️

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	80.66% <ø> (ø)
...a/python/opentrons_shared_data/gripper/__init__.py	75.00% <0.00%> (-18.75%)	⬇️
...ared-data/python/opentrons_shared_data/_version.py	61.53% <0.00%> (-7.70%)	⬇️
...ngsCalibration/CalibrationDetails/OverflowMenu.tsx	73.41% <0.00%> (-2.98%)	⬇️
app/src/organisms/CalibrationTaskList/index.tsx	78.57% <0.00%> (-1.43%)	⬇️
...a/python/opentrons_shared_data/pipette/__init__.py	93.93% <0.00%> (-0.51%)	⬇️
...pentrons_shared_data/pipette/pipette_definition.py	91.22% <0.00%> (-0.16%)	⬇️
app/src/atoms/buttons/BackButton.tsx	100.00% <0.00%> (ø)
app/src/organisms/SetupNetwork/SearchNetwork.tsx	100.00% <0.00%> (ø)
... and 48 more

[b-cooper]

Code looks good to me 👍

[SyntaxColoring]

Groovy! 🕺 Responses to your review requests...

[SyntaxColoring]

• Does this behavior of the command make sense? I was originally working on adding an http endpoint that can be used to send a PATCH/ POST request to save calibration to disk when the two probes' offsets are provided but realized it was not a great idea for a few reasons-

I would have preferred a separate PATCH/POST/PUT endpoint. But, like you said:

• if we were to match the behavior of the other 'calibration' commands, they handle saving the calibration too. So it felt odd to have just the gripper calibration command delegate the saving part to robot server.

So I agree with making it a Protocol Engine command, for consistency.

However, I do think that the saving of the calibration should happen in a separate third command, rather than building it into the second command. So, it would be like this:

1. calibrateGripper with front.
2. calibrateGripper with rear.
3. saveGripperCalibration with measurements from front and rear.

(And perhaps we could rename calibrateGripper to probeGripper or measureGripperCalibration.)

Separating out the save step is nice because:

• It lets a client show the user confirmation of the measurements before we apply those measurements. Like how Labware Position Check has a confirmation step at the end, where you can choose to abandon the offsets if they look wrong.
• Use manually-supplied measurements. Or manual adjustments to the automatic measurements. This is helpful for debugging and testing, and for implementing custom calibration methods.

[sanni-t]

The difference between LPC and instrument calibration is that the latter is handled fully by the backend. So deciding whether the offsets are wrong is something that the backend should decide, not the client or user. Until now it's been the robot server's calibration flow doing that work and I think now it will be hardware controller or some other backend entity that holds the information necessary to make that call.
As for manually supplying measurements for testing and debugging, we can always edit the calibration file. I don't think that calls for expanding the capability of client-facing engine commands.

But it is a good point that the command should let the client know whether the calibration process fully succeeded (with the offset status). Although technically the client can check the calibration status via instruments endpoint but that's yet another request and who knows how long it takes until the file change is propagated; opens up avenues for race condition.
What do you think about returning the entire calibration data in the response instead of just the total offset and keeping the calibration contained to the single command?

[ahiuchingau]

Makes sense to me

[SyntaxColoring]

Discussed in-person with @sanni-t:

I still think that saving should be a separate command. But doing it this way doesn't preclude us from changing it in the future, so I'm cool with this being merged if we need to move forward.