-
Notifications
You must be signed in to change notification settings - Fork 125
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 hex support for msg constants #559
Conversation
@dawonn I think the idea makes sense, but I would like to:
|
Sounds good, I'll work on the tests in the next day or two. FWIW: This feature would really help with hardware driver interfaces that I'm developing so that the values match across the device API and User Manuals. |
http://design.ros2.org/articles/legacy_interface_definition.html |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems okay to me.
Technically, this change also adds support for base-2 and base-8 constants too, e.g. |
Great, thanks @sloretz . @dawonn So I think the first step here is to update https://github.com/ros2/design/blob/gh-pages/articles/116_legacy_interface_definition.md to add hex, binary, and octal into the "constants" section. Once we have that, then we can go ahead and get the actual code change in. |
b7da6df
to
969412b
Compare
Signed-off-by: Dereck Wonnacott <dereck@gmail.com>
|
@jacobperron wrote:
Wouldn't the grammar need to be updated as well? rosidl/rosidl_parser/rosidl_parser/grammar.lark Lines 37 to 40 in 5b74acd
The IDL spec also doesn't appear to allow for any other bases than 8, 10 and 16 (version |
IIUC this is adding a feature to the legacy ROS 1 style msg/srv/action format, not the IDL parser. The grammar for IDL parsing should be unchanged by this PR. |
That sounds right to me. To be sure, it might be good to extend our test interfaces files to exercise this new feature (e.g. add new fields to |
Wouldn't that make it possible to create |
I think using |
This is what I thought too. As I suggested, modifying our current test files would help confirm. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like it is backwards compatible, and faithfully implements ros2/design#308 . I'm going to run full CI on it just to make sure nothing breaks.
The failures on macOS and Windows are well-known, so not caused by this PR. Going ahead and merging, thanks @dawonn |
Signed-off-by: Dereck Wonnacott <dereck@gmail.com>
fixes #558