-
Notifications
You must be signed in to change notification settings - Fork 178
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
feat(hardware, api): check motor engaged status #14479
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## edge #14479 +/- ##
==========================================
- Coverage 67.50% 67.47% -0.03%
==========================================
Files 2514 2514
Lines 72375 72418 +43
Branches 9317 9317
==========================================
+ Hits 48856 48866 +10
- Misses 21314 21347 +33
Partials 2205 2205
Flags with carried forward coverage won't be shown. Click here to find out more.
|
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.
Let's get some tests at least for the opentrons_hardware
side. I also wonder if get_motor_status
is maybe better implemented with a broadcast message so we don't need so many canbus back and forths
Axis.Y: phony_bounds, | ||
Axis.Z_G: phony_bounds, | ||
Axis.Q: phony_bounds, | ||
Axis.Z_L: (0, 300), |
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.
how close to reality are these bounds?
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.
LGTM!
4abf7e0
to
8f47695
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.
LGTM if it works!
<!-- Thanks for taking the time to open a pull request! Please make sure you've read the "Opening Pull Requests" section of our Contributing Guide: https://github.com/Opentrons/opentrons/blob/edge/CONTRIBUTING.md#opening-pull-requests To ensure your code is reviewed quickly and thoroughly, please fill out the sections below to the best of your ability! --> # Overview When #14479 was added, the motor enable message listener was never cleaned up properly and thus causing the robot to slow down significantly as it runs more and more move commands.
<!-- Thanks for taking the time to open a pull request! Please make sure you've read the "Opening Pull Requests" section of our Contributing Guide: https://github.com/Opentrons/opentrons/blob/edge/CONTRIBUTING.md#opening-pull-requests To ensure your code is reviewed quickly and thoroughly, please fill out the sections below to the best of your ability! --> # Overview Up until now, the hardware controller doesn't actually know if a motor has been engaged. We've been assuming a motor has been disengaged by checking the stepper position status, which doesn't provide the full story and causes us to engage the motor when it's already enabled (while homing the 96-channel pipette mount). This PR enables the hardware controller to prompt for the motor enabled status and allows us to finally implement the `OT3controller.engaged_axes` method. This PR also updated the phony axis bound to values that are closer to reality; this prevents us from getting stuck in a extremely long homing move (like > 33 minutes) that never times out if a limit switch malfunctions.
<!-- Thanks for taking the time to open a pull request! Please make sure you've read the "Opening Pull Requests" section of our Contributing Guide: https://github.com/Opentrons/opentrons/blob/edge/CONTRIBUTING.md#opening-pull-requests To ensure your code is reviewed quickly and thoroughly, please fill out the sections below to the best of your ability! --> # Overview When #14479 was added, the motor enable message listener was never cleaned up properly and thus causing the robot to slow down significantly as it runs more and more move commands.
Overview
Up until now, the hardware controller doesn't actually know if a motor has been engaged. We've been assuming a motor has been disengaged by checking the stepper position status, which doesn't provide the full story and causes us to engage the motor when it's already enabled (while homing the 96-channel pipette mount).
This PR enables the hardware controller to prompt for the motor enabled status and allows us to finally implement the
OT3controller.engaged_axes
method.This PR also updated the phony axis bound to values that are closer to reality; this prevents us from getting stuck in a extremely long homing move (like > 33 minutes) that never times out if a limit switch malfunctions.