-
Notifications
You must be signed in to change notification settings - Fork 179
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): enable stall detection #12377
Conversation
Codecov Report
@@ Coverage Diff @@
## edge #12377 +/- ##
==========================================
+ Coverage 73.13% 73.42% +0.28%
==========================================
Files 1510 2291 +781
Lines 49516 62901 +13385
Branches 3025 6792 +3767
==========================================
+ Hits 36215 46184 +9969
- Misses 12838 15104 +2266
- Partials 463 1613 +1150
Flags with carried forward coverage won't be shown. Click here to find out more.
|
I think we shouldn't target this on the internal release if it needs that much testing, unfortunately. Let's put it on edge, though! And I think in terms of testing, you're right on about how to do it. I think a feature flag is a pretty good idea, although it's sort of getting to the point where we should add mechanical revisions or something to handle this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got some minor feedback things but overall looks good as long as the hardware works now
@@ -76,16 +76,16 @@ | |||
DEFAULT_MAX_SPEEDS: Final[ByGantryLoad[Dict[OT3AxisKind, float]]] = ByGantryLoad( | |||
high_throughput={ | |||
OT3AxisKind.X: 500, | |||
OT3AxisKind.Y: 500, | |||
OT3AxisKind.Y: 375, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
imo this should be a separate PR
@@ -35,3 +35,7 @@ def rear_panel_integration() -> bool: | |||
"""Whether to enable usb connected rear_panel for the OT-3.""" | |||
|
|||
return advs.get_setting_with_env_overload("rearPanelIntegration") | |||
|
|||
|
|||
def disable_stall_detection() -> bool: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
kinda sounds like a verb-phrase, can we call this like stall_detection_disabled()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or even stall_detection_enabled()
and return not get_setting...("disableStallDetection")
e187d0a
to
ef52fac
Compare
cf818be
to
838de1e
Compare
Co-authored-by: Seth Foster <[email protected]>
dc3a78e
to
412e531
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested, LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tested 5/9. LGTM
Overview
We previously disable stall detection because the Y axes were unreliable and used to skip steps often on our EVT models. Now that we have the DVT units, we should verify that the motors are accurate enough for us to safely re-enable stall detection.
On top of that, how do we feel about adding a robot feature flag to disable & enable this feature for testing purposes?
Changes:
Test Plan
In order to merge this PR, we need to put the changes in this PR on DVT several units (ideally, as many as we can) and make sure we can run a protocol or two without triggering any stall detection error.