search

ros2 / rosidl

Packages which provide the ROS IDL (.msg) definition and code generation.
Apache License 2.0
75 stars 125 forks source link

Add support for NaN constants and NaN defaults in ROS IDL #789

Open Ryanf55 opened 6 months ago

Ryanf55 commented 6 months ago

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 https://github.com/ros2/ros2_documentation/issues/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.

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.

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

Ticket

Relates to #351

Future work

Ryanf55 commented 6 months ago

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 commented 6 months ago

NaN is not a valid JSON symbol http://json.org/, but there are several files use json format. e.g https://github.com/ros2/rosidl/blob/0775e1059638dc24e7d87c0b6b51bd2c8809d678/rosidl_generator_type_description/rosidl_generator_type_description/__init__.py#L476-L486 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 https://github.com/ros2/ros2_documentation/issues/4135.

as described in https://github.com/ros2/rosidl/issues/351, inf would be also useful to support accordingly.

fujitatomoya commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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 commented 1 week ago

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 commented 1 week ago

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.

Ryanf55 commented 1 week ago

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.