Add service to clear errors. #14
Conversation
liborw
changed the title
There was a problem hiding this comment.
In addition to (or instead of?) an explicit service call to clear errors, I think it would be nice to autoclear errors when entering CLOSED_LOOP_CONTROL. (Especially since on ODrive S1 this is required to re-enable the brake resistor after undervoltage.)
To do that, you could add this snippet to ODriveCanNode::request_state_callback(), before the state request is sent:
struct can_frame frame;
frame.can_id = node_id_ << 5 | CmdId::kClearErrors;
frame.can_dlc = 0;
can_intf_.send_can_frame(frame);(if you're already set up for testing it would be cool if you could try this)
In fact, do you feel it's strong requirement for ClearErrors to be an explicit call? If yes, see my other comments in the review, but if not, it might be better to stick to autoclearing until explicit clearing is requirement for someone.
src/odrive_can_node.cpp
Outdated
| frame.can_id = node_id_ << 5 | CmdId::kClearErrors; | ||
| { | ||
| std::unique_lock<std::mutex> guard(axis_state_mutex_); | ||
| write_le<uint8_t>(identify_, frame.data); |
There was a problem hiding this comment.
I think we can hardcode the identify field to 0 instead of exposing it to ROS. The reason is that this field is intended for interactive commissioning, whereas the scope of the ROS node is intended to be runtime (rather than setup-time).
For interactive commissioning, there are ROS-independent example scripts linked here.
(This also allows to remove clear_errors_mutex_)
There was a problem hiding this comment.
I'm not sure about auto clearing, I'm just starting with odrive by the way. My idea is that when I start the some controller, I would clear errors as I know what I'm doing, but I do not want them cleared during the run as they might be sign of something bad happening. However, autoclearing could be optional. I will implement your sugestions in this PR...
There was a problem hiding this comment.
Ah I don't mean autoclear during the run (while the motor is active). What I mean is, your ROS application explicitly requests a state change from IDLE to CLOSED_LOOP_CONTROL to enable the motor. Since this request is explicit, it means that by this time your application had an opportunity to read potential errors and to decide that it's ok to re-enable the motor. Therefore I think it makes sense for the ROS node to clear errors by itself on this transition.
Once the ODrive is in CLOSED_LOOP_CONTROL, it will go back to IDLE by itself as soon as there's a new error.
In addition, on the ODrive S1, clearing errors makes sure the brake resistor is active (in case it previously got disarmed due to undervoltage). Requesting CLOSED_LOOP_CONTROL otherwise causes a BRAKE_RESISTOR_DISARMED error. To avoid this error, I think in this case it's especially nice if the ROS node sends ClearErrors by itself.
There was a problem hiding this comment.
Neither did I. Actually the break resistor is good example, when the state change command implicitly clears errors, you would newer know about the undervoltage, with explicit call to clear errors you can chose to ignore it or somehow deal with it...
There was a problem hiding this comment.
In both cases the user can check errors before entering CLOSED_LOOP_CONTROL, and handle them accordingly. The usage would be like this:
- Without auto-clearing in ROS node:
- Check errors
- Clear errors
- Request CLOSED_LOOP_CONTROL
- With auto-clearing in ROS node:
- Check errors
- Request CLOSED_LOOP_CONTROL
I think what you might be referring to is more about educating the user, rather than client code capability: An uninformed user might indeed miss errors in the second variant, but an informed user can read them in both cases.
However regarding that, I should clarify the semantics of errors and ClearErrors: The error variable disarm_reason does not prevent the ODrive itself from re-entering CLOSED_LOOP_CONTROL. The only reasons to call ClearErrors are:
- re-enable brake resistor (S1 only)
- be sure that when
disarm_reasonindicates an error at a later time, it was newer than the last call to ClearErrors - make the LED not be red
This change in semantics was a conscious choice that dates back to the early days of odrivetool: Everyone just got used to calling odrv0.clear_errors() before odrv0.axis0.requested_state = AxisState.CLOSED_LOOP_CONTROL, without even looking at the errors. So we decided to do it automatically on the ODrive, but just missed the three points above.
|
@samuelsadok , @liborw |
|
Thanks for taking over. This PR seems to be abandoned, so I would recommend to just open a new PR from your own fork and we can close this one when done. I see you already did that but then closed it again for some reason? (Btw quick comment from a preliminary look: I saw quite a bit of formatting noise (whitespace changes). It's better to keep that out of the PR, or if your IDE insists, put it in a separate commit at least.) |
|
@samuelsadok |
|
Superseded by #27 |
Service that sends Clear_Errors command, fixes #13 and #9.
Usage: