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): handle gantry backdrive during instrument attach #13555

Conversation

fsinapi
Copy link
Contributor

@fsinapi fsinapi commented Sep 14, 2023

Overview

Closes RQA-1578

When attaching an instrument (gripper or pipette), it's possible to push a little too hard and backdrive the gantry axes a little bit. If this happens, the motor controllers detect that their position is invalid and the subsequent command to start calibrating the instrument will fail as a result.

We could fix this by unconditionally homing all of the axes before starting calibration (we used to do this on at least some of the workflows), but that is Really Annoying and most of the time it is just a waste of the user's time.

In order to handle situations where the gantry position becomes invalid, we add a new optional parameter to the Home command, named skipIfMountPositionOk. If a mount is provided to this parameter, the protocol engine will check if that mount has a valid position (e.g. every axis has a known motor position & the encoder position has not drifted). If the position is ok, no action is performed; otherwise, the gantry homes.

Test Plan

Push the API and ODD packages to a robot and test the following workflows:

  • Gripper attach
  • Pipette attach (left & right)
  • Gripper calibrate
  • Pipette calibrate

On each one, try it once normally and ensure the gantry doesn't home to the origin before calibrating. Then, run the workflow again but push the gantry enough that it loses position; this time, make sure the gantry homes.

Changelog

  • Added an optional parameter to the home command that will skip homing if the position of a mount is valid
  • Before calibrating gripper or pipette, call home with the proper mount provided just in case the user backdrove the gantry

Review requests

  • Is there anyplace else we would want to add this call? I didn't add it to module calibration because my understanding is that there needs to be a pipette already attached through the normal pipette-attach workflow

Risk assessment

@fsinapi fsinapi requested a review from a team September 14, 2023 18:40
@fsinapi fsinapi requested review from a team as code owners September 14, 2023 18:40
@fsinapi fsinapi requested review from mjhuff and removed request for a team September 14, 2023 18:40
@codecov
Copy link

codecov bot commented Sep 14, 2023

Codecov Report

Merging #13555 (c292c4d) into chore_release-7.0.0 (552cd65) will decrease coverage by 0.01%.
Report is 1 commits behind head on chore_release-7.0.0.
The diff coverage is 37.50%.

Additional details and impacted files

Impacted file tree graph

@@                   Coverage Diff                   @@
##           chore_release-7.0.0   #13555      +/-   ##
=======================================================
- Coverage                71.31%   71.31%   -0.01%     
=======================================================
  Files                     2419     2419              
  Lines                    68080    68080              
  Branches                  7908     7908              
=======================================================
- Hits                     48550    48549       -1     
- Misses                   17679    17680       +1     
  Partials                  1851     1851              
Flag Coverage Δ
app 68.92% <37.50%> (-0.01%) ⬇️
g-code-testing 96.44% <ø> (ø)
notify-server 89.13% <ø> (ø)
protocol-designer 46.09% <ø> (ø)
shared-data 73.91% <ø> (ø)
step-generation 87.18% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
api/src/opentrons/protocol_engine/commands/home.py 100.00% <ø> (ø)
...rc/opentrons/protocol_engine/execution/movement.py 100.00% <ø> (ø)
...p/src/organisms/PipetteWizardFlows/AttachProbe.tsx 81.81% <ø> (ø)
app/src/organisms/GripperWizardFlows/MovePin.tsx 57.57% <37.50%> (-0.49%) ⬇️

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.

This looks good to me as long as it's a serialization-safe change

Copy link
Contributor

@SyntaxColoring SyntaxColoring left a comment

Choose a reason for hiding this comment

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

Yep, this is serialization-safe. Thanks!

@@ -25,6 +26,14 @@ class HomeParams(BaseModel):
" to ensure accurate homing of the explicitly specified axes."
),
)
skipIfMountPositionOk: Optional[MountType] = Field(
Copy link
Contributor

@SyntaxColoring SyntaxColoring Sep 14, 2023

Choose a reason for hiding this comment

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

It feels kind of weird to me that this is skipIfMountPositionOk: Optional[MountType] instead of skipIfAllPositionsOk: Optional[bool].

The PR description mentions pushing too hard when attaching pipettes. Is it possible for that to affect one mount, but not the others?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mostly a product of the problem mostly occurring when you're working with attach/detach flows - my thinking here was that the client is basically saying "skip if all of the axes pertinent to this mount that I'm in the process of using are good." We also don't really have a concept of the gantry's position sans a mount in the hardware controller or in any existing commands.

When attaching a 96ch there's also the chance for the weight of the pipette to cause the left mount to drop down - however, we're homing that axis anyways in the preceding command of any relevant workflow.

Copy link
Contributor

@mjhuff mjhuff left a comment

Choose a reason for hiding this comment

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

Sorry to keep you waiting - the JS side looks good to me!

@fsinapi fsinapi merged commit a0e7785 into chore_release-7.0.0 Sep 15, 2023
@fsinapi fsinapi deleted the RQA-1578-bug-Error-while-calibrating-during-gripper-attach branch September 15, 2023 14:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants