Add support for NaN constants and NaN defaults in ROS IDL

[Ryanf55]

Purpose

Add support for NaN floating-point constants and defaults for float and double c/c++ types which translate to a quiet NaN in C++ per IEEE 754 .

Ticket

See ros2/ros2_documentation#4135.

Currently, you can set a NaN value for a message field when using the CLI, but there is no way to define a constant as NaN, or to set a default as a NaN.

Details

As implemented, the NAN literal is case-insensitive because it uses the python3 float() function. Because the DDS IDL 4.2 standard does not specify this special value as part of the standard, I propose following the python convention for now.
If this needs to be a certain case for other tools, they can convert it.

• See https://docs.python.org/3/library/functions.html#float for details on the float() method.

Note that in the tests, you can't use TEST_BASIC_TYPE_FIELD_ASSIGNMENT. EXPECT_EQ will return false with two NaN values. Instead, you have to use std::isnan to check a value is set to NaN.

I created an OMG IDL ticket to explain the problem that it's not supported in IDL.

• https://issues.omg.org/issues/lists/unclassified#issue-53557

Because of the way that ROSIDL works and generates the C/C++ directly, I do not believe the lack of support in the OMG IDL 4.2 standard should block this PR.

References

• https://en.cppreference.com/w/cpp/types/numeric_limits/quiet_NaN
• https://en.cppreference.com/w/c/numeric/math/nan

Ticket

Relates to #351

Future work

• Backport this to humble on approval
• Add similar support for infinity with +inf and -inf

[fujitatomoya]

lgtm with green CI

[fujitatomoya]

lgtm

[Ryanf55]

lgtm

Thanks. Do you want to approve the workflow that would kick off a build on the build farm to see how it fares?

I tried the backport to humble; it got messy - seems like ROSIDL changed a ton. Do you have any recommendations on how I can be successful in getting this feature into humble ?

[fujitatomoya]

NaN is not a valid JSON symbol http://json.org/, but there are several files use json format.
e.g

rosidl/rosidl_generator_type_description/rosidl_generator_type_description/__init__.py

Lines 476 to 486 in 0775e10

	hashable_repr = json.dumps(
	hashable_dict,
	skipkeys=False,
	ensure_ascii=True,
	check_circular=True,
	allow_nan=False,
	indent=None,
	# note: libyaml in C doesn't allow for tweaking these separators, this is its builtin
	separators=(', ', ': '),
	sort_keys=False
	)

it would be nice to check if this does not break those json operation.

maybe we can add documentation that says we manages NaN as IEEE 754 standard always, probably in ros2/ros2_documentation#4135.

as described in #351, inf would be also useful to support accordingly.

[fujitatomoya]

I tried the backport to humble; it got messy - seems like ROSIDL changed a ton. Do you have any recommendations on how I can be successful in getting this feature into humble ?

i am not sure if we should do backport. if this does not break user space, it seems okay. on the other hand, this can be new enhancement, so it can be available from on next distribution to avoid possible risk? any thoughts?

[Ryanf55]

I tried the backport to humble; it got messy - seems like ROSIDL changed a ton. Do you have any recommendations on how I can be successful in getting this feature into humble ?

i am not sure if we should do backport. if this does not break user space, it seems okay. on the other hand, this can be new enhancement, so it can be available from on next distribution to avoid possible risk? any thoughts?

I forked ROSIDL internally for the time being for humble; we're using both NaN constants and defaults in our messages and it's passing CI with the changes (C++ and python tests). I haven't tried other tooling yet.

If we want to let this simmer with the community for a bit while I can work on figuring out JSON and IDL support for NaN, that sounds good.

It would be great to get this in for Jazzy; existing ROS messages like BatteryState are supposed to be using NaN, but instead default to 0.0. I totally understand if we have to close this if there is no way all the tools will support it, but then I would recommend the ROS community stop trying to use NaN's in messages. Coming from the aerial side where NaN is used a significant value in MAVLink and fully supported in that toolchain, it's hard to do seamless inter-op with ROS without NaN support.

[Ryanf55]

Don't merge this yet. I found some issues with the generated python code that were caught only at runtime on humble.

From rosidl_generator_py/resource/_idl.py.em.

Looks like a small change, but I think I also need to look into the comparison operations and how they treat NaN. Because NaN doesn't equal itself in IEEE-754, comparisons get tricky.
https://docs.python.org/3/library/math.html#math.nan

    def __eq__(self, other):
        if not isinstance(other, self.__class__):
            return False
        if self.header != other.header:
            return False
        if self.my_maybe_nan_value != other.my_maybe_nan_value :
            return False
        return True

I have a fix, but need to get time for approval to push it upstream (open source).

[Ryanf55]

The branch we have been using internally is now open source here:
https://github.com/Ryanf55/rosidl/tree/nan-constants
This is branched off humble. It looks like it should be a clean cherry-pick.
It has no known issues.

[Ryanf55]

Here's a sample message aggregating all the changes from both proposed PR's. It works with ros2 interface show and ros2 topic pub and ros2 topic echo.

You can publish the default

ros2 topic pub /nans my_msgs/msg/NanExample

Or set a value to NaN in the CLI.

ros2 topic pub /nans public_interfaces/msg/NanExample {"float32_nan: NaN"}

Here's the message:

$ ros2 interface show my_msgs/msg/NanExample 
float64 FLOAT64_NAN_UC=NAN
float64 FLOAT64_NAN_LC=nan
float32 FLOAT32_NAN_UC=NAN
float32 FLOAT32_NAN_LC=nan
float32 float32_nan NaN
float64 float64_nan nan
float32 FLOAT32_NAN=NaN
float64 FLOAT64_NAN=nan

And you can echo it:

$ ros2 topic echo /nans 
float32_nan: .nan
float64_nan: .nan
---
float32_nan: .nan
float64_nan: .nan
---

Whatever original issues I had are fixed, so I'm marking this ready for review. Let's make sure someone else can repeat these tests.

[henrygerardmoore]

Only one minor suggestion, otherwise everything looks good to me! I think this is a really useful feature.

I merged in rolling locally (no conflicts, as you said) and built and verified all the generated files. I think the CI failure is just from not being up-to-date with rolling, the ones I looked at seem unrelated to these changes.

[Ryanf55]

Only one minor suggestion, otherwise everything looks good to me! I think this is a really useful feature.

I merged in rolling locally (no conflicts, as you said) and built and verified all the generated files. I think the CI failure is just from not being up-to-date with rolling, the ones I looked at seem unrelated to these changes.

It's rebased. Let's see how CI does.

[Ryanf55]

@fujitatomoya What are you thoughts on this? If you like it, I'll rebase.
Otherwise, I will delete these PR's and remove this feature from our message definitions.
We don't want to carry this capability into our release in november if ROS won't allow it.

[henrygerardmoore]

To chime in, I think this would be a really useful capability. A lot of ros2_control stuff in particular utilizes NaN values for sentinel or default values, and being able to have messages use these by default without needing to make factory functions would be really great.

[fujitatomoya]

@Ryanf55 i think this is useful feature. but i do not have maintainer permission on this repo, so someone else with maintainer permission needs to review and give approval for us.

@ros2/team

[fujitatomoya]

Pulls: #789
Gist: https://gist.githubusercontent.com/fujitatomoya/4ca28f34619ed962ec4e448811e20803/raw/a6e1239a2a0c443546616d5da6ae2983fb9be489/ros2.repos
BUILD args: --packages-above-and-dependencies rosidl_adapter rosidl_generator_c rosidl_generator_cpp rosidl_generator_tests
TEST args: --packages-above rosidl_adapter rosidl_generator_c rosidl_generator_cpp rosidl_generator_tests
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/14584

• Linux
• Linux-aarch64
• Linux-rhel
• Windows

[fujitatomoya]

@Ryanf55 can you check the windows failure?

[Ryanf55]

@Ryanf55 can you check the windows failure?

Gah, I missed your message. Can you re-run windows and I'll take a look at the build failure?

We now have two customers we had to go give them our rosidl patches so they could compile our interfaces because I dragged my feet on upstream 🙈

[fujitatomoya]

Pulls: #789
Gist: https://gist.githubusercontent.com/fujitatomoya/3a2c452459828c8d720edcc6fbf0a8df/raw/8c1196d84489d9514a9fa42cc7ba72ee455b8e57/ros2.repos
BUILD args: --packages-above-and-dependencies rosidl_adapter rosidl_generator_c rosidl_generator_cpp rosidl_generator_tests
TEST args: --packages-above rosidl_adapter rosidl_generator_c rosidl_generator_cpp rosidl_generator_tests
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/14981

• Linux
• Linux-aarch64
• Linux-rhel
• Windows