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

Add clear errors #27

Merged
merged 1 commit into from
Sep 26, 2024
Merged

Conversation

kenconnor
Copy link

@kenconnor kenconnor commented Sep 12, 2024

Taking over #14
The former PR broke file structure and I made a brand new PR.

I tested it on S1. I called /request_axis_state with wrong state, called /clear_errors and then called /request_axis_state with correct state. I confirmed that procedure_result is changed to 0.

Copy link
Member

@samuelsadok samuelsadok left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested it on S1. I called /request_axis_state with wrong state, called /clear_errors and then called /request_axis_state with correct state. I confirmed that procedure_result is changed to 0.

To be clear, an explicit clear_errors call should not be considered a requirement for entering CLOSED_LOOP_CONTROL. On a high level, an explicit call to clear_errors should only be used to make the LED not be red. See explanation here. If entering CLOSED_LOOP_CONTROL is the goal, clearing errors should be handled transparently by (future) ODrive firmware, or, for compatibiliy with older firmware, transparently in request_state_callback().

If clearing the LED color is the goal of this PR, then the approach looks mostly good. The git history is still a bit convoluted and the PR contains unrelated formatting changes. Please use git rebase / cherry-pick / amend so that the PR consists of a single commit and doesn't contain unrelated formatting changes. Please also add a note in README about the new service call.

odrive_node/src/odrive_can_node.cpp Show resolved Hide resolved
@kenconnor
Copy link
Author

I think S1 needs to call clear_errors() to re-enable the brake resistor, but under what circumstances is the brake resistor disarmed? Since I use S1, I would like to enable and use the brake resistor if it is disabled.

I made the following edits:

  • Removed whitespace unrelated to the PR from the commit
  • Added to the README

I was concerned that the contribution of liborw in #14 might disappear, so I decided not to consolidate the commits to preserve the commit history. Should we consolidate all changes, including the additions to the README, into a single commit (add service to clear errors)?

Copy link
Member

@samuelsadok samuelsadok left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes that's true, if you need the brake resistor in your system to be running even if the motor is off (e.g. to sink power from another ODrive), then that's another reason to call clear_errors(). The brake resistor disarms on conditions that specifically restrict the brake resistor's ability to run safely: overvoltage, undervoltage, configuration error and low level system error. We're considering to change this in future firmware versions to re-enable it automatically when the conditions allow it.

Formatting looks good now.

If you wish to preserve authorship, there are multiple ways to do it. E.g. you can still squash the original commits into one, and put additional changes in a separate one, or mention co-authorship in the commit message.

odrive_node/README.md Outdated Show resolved Hide resolved
Co-authored-by: Libor Wagner (wagnelib) <[email protected]>
@kenconnor
Copy link
Author

@samuelsadok
I have made the following changes:

  • Squashed original commits
  • Left Libor as a co-author
  • Changed the sentence in README

@samuelsadok
Copy link
Member

looks good, thanks!

@samuelsadok samuelsadok merged commit d2f0f54 into odriverobotics:main Sep 26, 2024
2 checks passed
@kenconnor kenconnor deleted the add-clear-errors branch September 27, 2024 00:21
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