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(hardware): minimum movement displacement of 0.05mm #13052

Merged
merged 2 commits into from
Jul 6, 2023

Conversation

fsinapi
Copy link
Contributor

@fsinapi fsinapi commented Jul 6, 2023

Overview

Protocols kept failing with collision errors on release 0.13.0, and logs would show a noticeable drift in the encoder position over time. Release 0.9.0 seemed completely fine with the same exact protocols.

One difference between those releases has been changing the units for acceleration to use µm instead of mm. When we move the pipette down to aspirate/dispense/pick up a tip/etc, there are usually small displacements in the X and Y axes that are due to precision loss on the stepper motor accumulator. With the new, more precise acceleration, we seem to actually commanding some gantry movements that are affecting the error buildup over time.

By clamping the minimum displacement to 0.05mm, this error seems gone. A protocol that transfers liquid from a reservoir into an entire 96-well plate used to fail after the first column, and can now finish successfully.

Test Plan

Ran a failing protocol and now it works. There used to be noticeable encoder drift if you disabled stall detection, now the pipettes look fine.

Changelog

  • Added a minimum displacement of 0.05mm in each axis of movement

Review requests

Any reason to make the minimum displacement smaller? Do we ever actually need to move 0.05mm?

Risk assessment

@fsinapi fsinapi requested a review from a team July 6, 2023 16:03
@fsinapi fsinapi requested a review from a team as a code owner July 6, 2023 16:03
@fsinapi fsinapi self-assigned this Jul 6, 2023
@codecov
Copy link

codecov bot commented Jul 6, 2023

Codecov Report

Merging #13052 (12cabd0) into internal-release_0.13.0 (37609fd) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@                     Coverage Diff                     @@
##           internal-release_0.13.0   #13052      +/-   ##
===========================================================
+ Coverage                    72.81%   72.82%   +0.01%     
===========================================================
  Files                         2364     2364              
  Lines                        64474    64526      +52     
  Branches                      7191     7191              
===========================================================
+ Hits                         46948    46994      +46     
- Misses                       15847    15853       +6     
  Partials                      1679     1679              
Flag Coverage Δ
g-code-testing 96.44% <ø> (ø)
hardware 59.21% <100.00%> (+0.21%) ⬆️
notify-server 89.13% <ø> (ø)

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

Impacted Files Coverage Δ
...are/hardware_control/motion_planning/move_utils.py 91.70% <100.00%> (+0.16%) ⬆️

... and 5 files with indirect coverage changes

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 because we update our position cache back based on the actual outcomes of moves, but we should be generally very careful that nothing caches the positions it commands before this call.

@sfoster1 sfoster1 merged commit bbffb4a into internal-release_0.13.0 Jul 6, 2023
@fsinapi fsinapi deleted the RQA_1034_fix_gantry_stalls branch July 6, 2023 17:18
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.

2 participants