Skip to content

Conversation

bpeterson92
Copy link

Related to issue #32
requested_axis_state service now returns CLOSED_LOOP_CONTROL requests "instantly"

built in optional watchdog feeding

improved CAN frame error checking and logging

@CLAassistant
Copy link

CLAassistant commented Jun 24, 2025

CLA assistant check
All committers have signed the CLA.

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.

Sorry took me some time to get to this. There are some high level issues with the proposed changes:

  • Returning immediately from request_axis_state: Since request_axis_state is a "service call" (a remote procedure call) with a result, as opposed to a continuous one-directional topic, the natural semantics are "execute action, wait for its completion and return result". Therefore, when the user requests CLOSED_LOOP_CONTROL, the service call should report the correct result of trying to enter CLOSED_LOOP_CONTROL. If the request cannot be completed due to common errors such as DC voltage too low, then the service call needs to report that. Therefore returning immediately without awaiting the result is not what I would consider the way to address #32. Instead I would point to the async approach I laid out in #32 (comment). If there are other considerations that I'm forgetting please let me know.

  • Watchdog feeder: The current design intent is that the watchdog safety chain goes from the ODrive all the way up to the user application. So if the user application stops sending continuous setpoints, the ODrive goes to IDLE. Adding automatic feeding to the ROS node breaks that chain. Is there a strong use case where continuous sending from the user application is not possible? (if so, there would still be things about the implementation that need to be changed)

  • Added error logs: This is a change we can take in.

Comment on lines +320 to +321
// Small delay to ensure error clear is processed
std::this_thread::sleep_for(std::chrono::milliseconds(10));
Copy link
Member

Choose a reason for hiding this comment

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

This should not be required, at least not for the reason given. The ODrive handles all CAN frames in the same thread context so "clear errors" finishes before it handles the new state request.

I could imagine that with certain CAN interfaces, if you enqueue both messages in short succession, that the CAN interface looks at the message IDs of both at the same time and prioritizes the lower ID, even though it was sent later.

What setup were you using, and what behavior did you observe that made you add this delay?

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.

3 participants