Add hex support for msg constants

[dawonn]

fixes #558

[clalancette]

@dawonn I think the idea makes sense, but I would like to:

1. Get input from the rest of the @ros2/team
2. Figure out whether this was allowed in ROS 1 (and if not, why not)
3. Figure out whether there is a specification for the ROS message types, and if so, update it
4. Have tests for this functionality

[dawonn]

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.

[sloretz]

3. Figure out whether there is a specification for the ROS message types, and if so, update it

http://design.ros2.org/articles/legacy_interface_definition.html

[jacobperron]

Seems okay to me.

[jacobperron]

Technically, this change also adds support for base-2 and base-8 constants too, e.g. 0b01 and 0o01.

[clalancette]

1. Figure out whether there is a specification for the ROS message types, and if so, update it

http://design.ros2.org/articles/legacy_interface_definition.html

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.

[dawonn]

1. Update 116_legacy_interface_definition.md design#308
2. Tests added for bin, oct, & hex.

[gavanderhoorn]

@jacobperron wrote:

Technically, this change also adds support for base-2 and base-8 constants too, e.g. 0b01 and 0o01.

Wouldn't the grammar need to be updated as well?

rosidl/rosidl_parser/rosidl_parser/grammar.lark

Lines 37 to 40 in 5b74acd

	decimal_literal: DIGIT
	| "1".."9" DIGIT+
	octal_literal: "0" "0".."7"+
	hexadecimal_literal: "0x"i HEXDIGIT+

The IDL spec also doesn't appear to allow for any other bases than 8, 10 and 16 (version 4.2 anyway, section 7.2.6.1).

[sloretz]

Wouldn't the grammar need to be updated as well?

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.

[jacobperron]

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 Constants.msg). Depending which message definitions are changed, it might also involve updating the test fixtures here and here.

[gavanderhoorn]

Wouldn't that make it possible to create .msg (etc) files which would be problematic when considering their "IDL use"? Or is this something where ROS does not need to follow the IDL spec?

[sloretz]

Wouldn't that make it possible to create .msg (etc) files which would be problematic when considering their "IDL use"? Or is this something where ROS does not need to follow the IDL spec?

I think using int here in rosidl_adapter means the constants written to the OMG IDL file would always be base 10 regardless of the base used in the .msg file.

[jacobperron]

I think using int here in rosidl_adapter means the constants written to the OMG IDL file would always be base 10 regardless of the base used in the .msg file.

This is what I thought too. As I suggested, modifying our current test files would help confirm.

[clalancette]

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.

[clalancette]

CI:

• Linux
• Linux-aarch64
• macOS
• Windows

[clalancette]

The failures on macOS and Windows are well-known, so not caused by this PR. Going ahead and merging, thanks @dawonn