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

feat(hardware, api): check motor engaged status #14479

Merged
merged 10 commits into from
Mar 19, 2024

Conversation

ahiuchingau
Copy link
Contributor

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.

@ahiuchingau ahiuchingau requested a review from a team as a code owner February 12, 2024 21:36
Copy link

codecov bot commented Feb 12, 2024

Codecov Report

Attention: Patch coverage is 29.78723% with 33 lines in your changes are missing coverage. Please review.

Project coverage is 67.47%. Comparing base (b533692) to head (8f47695).

Additional details and impacted files

Impacted file tree graph

@@            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              
Flag Coverage Δ
g-code-testing 92.43% <ø> (ø)
hardware 57.05% <29.78%> (-0.17%) ⬇️

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

Files Coverage Δ
...entrons/hardware_control/backends/ot3controller.py 68.48% <ø> (ø)
...pentrons/hardware_control/backends/ot3simulator.py 86.23% <ø> (ø)
api/src/opentrons/hardware_control/ot3api.py 79.62% <ø> (ø)
.../opentrons_hardware/firmware_bindings/constants.py 99.25% <100.00%> (+<0.01%) ⬆️
.../firmware_bindings/messages/message_definitions.py 97.52% <100.00%> (+0.02%) ⬆️
...ns_hardware/firmware_bindings/messages/messages.py 91.66% <ø> (ø)
...ns_hardware/firmware_bindings/messages/payloads.py 95.87% <ø> (-0.02%) ⬇️
..._hardware/hardware_control/motor_enable_disable.py 48.43% <19.51%> (-51.57%) ⬇️

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.

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),
Copy link
Member

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?

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.

LGTM!

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.

LGTM if it works!

@ahiuchingau ahiuchingau merged commit e6cd823 into edge Mar 19, 2024
23 of 24 checks passed
@ahiuchingau ahiuchingau deleted the use_motor-status-request branch March 19, 2024 17:41
ahiuchingau added a commit that referenced this pull request Apr 18, 2024
<!--
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.
Carlos-fernandez pushed a commit that referenced this pull request May 20, 2024
<!--
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.
Carlos-fernandez pushed a commit that referenced this pull request May 20, 2024
<!--
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.
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