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

Expose driver enumerations in messages #2

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

AdamPettinger
Copy link

I was doing some testing commanding ODrive's via ROS 2 with this package, and noticed that it was hard to figure out what to put in some of the uint fields in the control messages or service.

So I pulled the enum values from the docs and put them directly in the message files. This way everybody who compiles the messages will have the enum values come along with the messages themselves (but note that msg constants are not serialized, so this adds no overhead when passing messages).

Obviously, this changes the structure of the original messages, which will break the code of anybody currently using this package. I thought of a few other ways to make these changes without changing the messages, but did not like them as much:

  1. Add the enums as constants in every message that needs them. But it is a lot of repetition and doesn't make it clear which constants belong with which types
  2. Add a header file declaring all these. Again, doesn't make it clear which enums go with which message fields, and to gain access in user code they would have to link/include this package

The changes only really effect the format of the input to the node, but I did test them on hardware anyway and everything looks to be working.

Note: this change would also need to be reflected in the tutorial page. Specifically, the service call near the bottom changes to:

ros2 service call /odrive_axis0/request_axis_state /odrive_can/srv/RequestAxisState '{axis_requested_state: {state: 1}}'

@madcowswe madcowswe requested a review from samuelsadok November 1, 2023 01:29
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 for letting this sit for so long.

I am quite reluctant to introduce a breaking change but maybe there's a viable middle ground: having a single .msg file for all enum constants and leaving all other .msg files unchanged.

This ties into your concerns from the alternative considerations:

  1. But it [...] doesn't make it clear which constants belong with which types
  2. [...] Again, doesn't make it clear which enums go with which message fields

However this seems to be more of a documentation problem, because this PR also doesn't enforce strong typing. You can still assign values from mismatching enum (in the example below it seems like a rather unlikely mistake, but what about C, C++, bash? I don't know enough ROS to tell immediately).

As far as expressiveness goes, these two are equally expressive:

This PR:

from odrive_node.srv import RequestAxisState
from odrive_node.msg import AxisState

request = RequestAxisState.Request()
request.axis_requested_state = AxisState(state=AxisState.CLOSED_LOOP_CONTROL)

Alternative solution:

from odrive_node.srv import RequestAxisState
from odrive_node.msg import Enums

request = RequestAxisState.Request()
request.axis_requested_state = Enums.AXIS_STATE_CLOSED_LOOP_CONTROL

Separately, the enum values aren't actually used in the package or examples, so it will be easy to forget or break them when if we pull them in. What would be the simplest snippet we can show users to try them?

For instance can we use "IDLE" instead of "1" in this command?

ros2 service call /odrive_axis0/request_axis_state /odrive_can/srv/RequestAxisState '{axis_requested_state: {state: 1}}'

Copy link
Member

Choose a reason for hiding this comment

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

Should be called ODriveErrors to match other places (like the Python odrive library).

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

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