fix(api): preserve last_moved in retract #14592
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
8398c83 made a refactor to the way the hardware controller retracts the last-moved mount to get it out of the way when you change a move. This change allowed mounts that have electronic brakes to have those brakes engaged more often, lowring current consumption and heat generation. Unfortunately, it also introduced an issue in a specific code path. If you have a 96 channel attached and you've been moving a mount that is not the left mount, so last_moved_mount is RIGHT or GRIPPER; and then you call `_cache_and_maybe_retract_mount(LEFT)`; then - the left mount is idle, so we home it with home_z - home_z clears _last_moved_mount - there is now no mount to retract, and so it is not retracted The fix for this is to copy last_moved_mount to a local so altering the cached value won't effect the rest of the method. This fixes an issue where we use exactly this codepath: moving to maintenance position immediately after calibrating the gripper. _last_moved_mount is GRIPPER, but we always move the left mount to the maintenance position. This hits the problem. Any other codepath of this kind would also hit it. This change should be strictly safer than the previous behavior. Closes RABR-45, RQA-2380
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## chore_release-7.2.0 #14592 +/- ##
=======================================================
- Coverage 67.62% 67.62% -0.01%
=======================================================
Files 2521 2521
Lines 72192 72191 -1
Branches 9290 9290
=======================================================
- Hits 48817 48816 -1
Misses 21185 21185
Partials 2190 2190
Flags with carried forward coverage won't be shown. Click here to find out more.
|
ahiuchingau
approved these changes
Mar 4, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
8398c83 made a refactor to the way the hardware controller retracts the last-moved mount to get it out of the way when you change a move. This change allowed mounts that have electronic brakes to have those brakes engaged more often, lowring current consumption and heat generation. Unfortunately, it also introduced an issue in a specific code path.
If you have a 96 channel attached and you've been moving a mount that is not the left mount, so last_moved_mount is RIGHT or GRIPPER; and then you call
_cache_and_maybe_retract_mount(LEFT); thenThe fix for this is to copy last_moved_mount to a local so altering the cached value won't effect the rest of the method.
This fixes an issue where we use exactly this codepath: moving to maintenance position immediately after calibrating the gripper. _last_moved_mount is GRIPPER, but we always move the left mount to the maintenance position. This hits the problem. Any other codepath of this kind would also hit it.
This change should be strictly safer than the previous behavior.
Closes RABR-45, RQA-2380, RABR-23, RABR-24, RABR-25
Overview
Test Plan
Changelog
Review requests
Risk assessment