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): use consistent CriticalPoints when moving to maintenance position #12878

Merged
merged 4 commits into from
Jun 9, 2023

Conversation

fsinapi
Copy link
Contributor

@fsinapi fsinapi commented Jun 8, 2023

Overview

When calling the MoveToMaintenancePosition command with a 96-channel pipette attached, the gantry was moving the pipette directly into the trash and stalling. It appears that the issue was that the waypoint positions were being calculated with respect to the Mount instead of the default critical point of the pipette, but the position values were being pulled from the default critical point.

To fix this discrepancy, we make sure to specify the CriticalPoint when calculating the waypoints to move the gantry to the maintenance position. To make sure the Z position is correct at the end, we calculate the final Z position based on the default critical point of whatever pipette is attached.

Test Plan

Tested by @smb2268 with 96-channel attach & detach flows. Before this change, the detach flows would fail (as described above). After this PR, the detach flow goes to the correct position.

Changelog

  • Specify CriticalPoint.MOUNT when calculating the waypoints to the maintenance position

Review requests

The alternative solution here is to specify CriticalPoint.MOUNT when grabbing the gantry position & the max height, but this seems to present some issues when the Z axes are moved into the attach position. Can someone more familiar with the context here confirm whether that approach would make more sense?

Risk assessment

Low, OT-3 only.

@fsinapi fsinapi requested review from TamarZanzouri, smb2268 and a team June 8, 2023 18:54
@fsinapi fsinapi requested a review from a team as a code owner June 8, 2023 18:54
@fsinapi fsinapi self-assigned this Jun 8, 2023
Copy link
Contributor

@smb2268 smb2268 left a comment

Choose a reason for hiding this comment

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

I tested attach, detach, and calibrate for a 96-channel with this change (on top of this commit since edge has other firmware issues) and it worked as expected. I was able to get through each flow and the positions seemed appropriate to me

@codecov
Copy link

codecov bot commented Jun 8, 2023

Codecov Report

Merging #12878 (9d0c535) into edge (f074b53) will increase coverage by 0.26%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             edge   #12878      +/-   ##
==========================================
+ Coverage   73.01%   73.27%   +0.26%     
==========================================
  Files        1516     2316     +800     
  Lines       49689    63459   +13770     
  Branches     3037     6963    +3926     
==========================================
+ Hits        36280    46501   +10221     
- Misses      12943    15306    +2363     
- Partials      466     1652    +1186     
Flag Coverage Δ
app 71.50% <ø> (+27.39%) ⬆️
g-code-testing 96.44% <ø> (ø)
notify-server 89.13% <ø> (ø)

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

see 800 files with indirect coverage changes

Copy link
Contributor

@ahiuchingau ahiuchingau left a comment

Choose a reason for hiding this comment

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

Do we know if this affects the location for attach/detach & add/remove probe for both single channel pipettes and gripper?

@fsinapi
Copy link
Contributor Author

fsinapi commented Jun 8, 2023

Do we know if this affects the location for attach/detach & add/remove probe for both single channel pipettes and gripper?

Good point. I think this does affect the X and Y positions, but not the Z.

In order to maintain the same X and Y positions I think we need to do a couple things

  • For the first two waypoint moves, get the positions with the Mount critical point and move with the same parameter as before
  • Re-get the max Z travel respective to the default critical point before moving the Z axis down, because the move_axes function is hard coded to work with that critical point

I can set it up that way and test it tomorrow.

@fsinapi fsinapi changed the title fix(api): don't move relative to Mount when moving to maintenance position fix(api): use consistent CriticalPoints when moving to maintenance position Jun 9, 2023
@fsinapi
Copy link
Contributor Author

fsinapi commented Jun 9, 2023

Tested attach/detach flows again after the latest changes and the maintenance position still seems correct. I got a whitescreen on the ODD but I think that was unrelated, I could go through the entire workflow with the OT3 app at v0.9.0

Copy link
Contributor

@TamarZanzouri TamarZanzouri left a comment

Choose a reason for hiding this comment

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

Looks great! Thank you so much for looking into this :)

@fsinapi fsinapi merged commit 9f28e7f into edge Jun 9, 2023
@fsinapi fsinapi deleted the fix_maintenance_pos_critical_point branch June 9, 2023 14:28
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