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): dont move gen1s all the way up #16316

Merged
merged 2 commits into from
Sep 20, 2024
Merged

fix(api): dont move gen1s all the way up #16316

merged 2 commits into from
Sep 20, 2024

Conversation

sfoster1
Copy link
Member

Gen1 pipettes need more z margin than usual when moving around at maximum z for some reason.

testing

  • run through dtwiz with a gen1 of any kind

Closes RQA-3229

Gen1 pipettes need more z margin than usual when moving around at
maximum z for some reason.

Closes RQA-3229
@sfoster1 sfoster1 requested a review from a team as a code owner September 20, 2024 14:53
Copy link

codecov bot commented Sep 20, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.43%. Comparing base (a0f42de) to head (ba419b0).
Report is 50 commits behind head on edge.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##             edge   #16316       +/-   ##
===========================================
+ Coverage   73.62%   92.43%   +18.80%     
===========================================
  Files          41       77       +36     
  Lines        2992     1283     -1709     
===========================================
- Hits         2203     1186     -1017     
+ Misses        789       97      -692     
Flag Coverage Δ
g-code-testing 92.43% <ø> (?)
shared-data ?

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

see 118 files with indirect coverage changes

@nusrat813 nusrat813 self-requested a review September 20, 2024 15:19
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 with gen1 p300s

Screenshot 2024-09-20 at 11 18 00 AM

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.

Odd. Do we know why this is only a problem for moveToAddressableArea? That can't be the only place we're traveling at max height, can it?

Is this also going to reduce the effective maximum labware height by 5 mm? Is there a chance that this will break existing protocols?

Looks good to merge as a quick fix if testing says it works and you're comfortable with the caveats above.

@sfoster1
Copy link
Member Author

sfoster1 commented Sep 20, 2024

Odd. Do we know why this is only a problem for moveToAddressableArea? That can't be the only place we're traveling at max height, can it?

I think it is - everywhere else we rely on the baseline arc style move that will hold whatever z it's at or some margin over the tallest loaded labware, whichever is higher. I think that the z position for a gen1 after home has a larger backoff distance than 1mm.

Is this also going to reduce the effective maximum labware height by 5 mm? Is there a chance that this will break existing protocols?

It won't, because as far as I can tell the only place that this kind of motion is used is when you create a moveToAddressableArea with stayAtHighestPossibleZ=True, which isn't done anywhere but by the app during drop tip wiz - based on a grep, anyway.

Looks good to merge as a quick fix if testing says it works and you're comfortable with the caveats above.

Testing does say it works and as far as I can see the caveats don't apply. The change will only affect moveToAddressibleArea commands with stayAtHighestPossibleZ=True, nothing else, and that's only used by dtwiz.

@sfoster1 sfoster1 merged commit c742a9f into edge Sep 20, 2024
21 of 23 checks passed
@sfoster1 sfoster1 deleted the fix-gen1-drop-tip branch September 20, 2024 17:05
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.

3 participants