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

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

Open
wants to merge 3 commits into
base: rolling
Choose a base branch
from

Conversation

Ryanf55
Copy link

@Ryanf55 Ryanf55 commented Mar 5, 2024

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.

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

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

@Ryanf55 Ryanf55 changed the title Add support for NaN constants in ROS IDL Add support for NaN constants and NaN defaults in ROS IDL Mar 5, 2024
Copy link

@fujitatomoya fujitatomoya left a comment

Choose a reason for hiding this comment

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

lgtm with green CI

rosidl_generator_c/resource/idl__struct.h.em Outdated Show resolved Hide resolved
rosidl_generator_c/rosidl_generator_c/__init__.py Outdated Show resolved Hide resolved
@Ryanf55 Ryanf55 force-pushed the nan-constants branch 2 times, most recently from e7223a0 to 5280456 Compare March 6, 2024 03:09
Copy link

@fujitatomoya fujitatomoya left a comment

Choose a reason for hiding this comment

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

lgtm

@Ryanf55
Copy link
Author

Ryanf55 commented Mar 6, 2024

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
Copy link

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

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
Copy link

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
Copy link
Author

Ryanf55 commented Mar 14, 2024

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 Ryanf55 marked this pull request as draft March 15, 2024 16:36
@Ryanf55
Copy link
Author

Ryanf55 commented Mar 15, 2024

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
Copy link
Author

Ryanf55 commented Sep 6, 2024

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
Copy link
Author

Ryanf55 commented Sep 6, 2024

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 Ryanf55 marked this pull request as ready for review September 6, 2024 23:43
Copy link

@henrygerardmoore henrygerardmoore left a comment

Choose a reason for hiding this comment

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

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.

rosidl_adapter/test/test_constant.py Outdated Show resolved Hide resolved
* Add nan default example and tests
* Add NaN constant example and tests
* Test NaNs on float64 and float32 in C++

Signed-off-by: Ryan Friedman <ryanfriedman5410+github@gmail.com>
@Ryanf55 Ryanf55 force-pushed the nan-constants branch 2 times, most recently from 24aeed3 to 65140bd Compare September 7, 2024 15:44
@Ryanf55
Copy link
Author

Ryanf55 commented Sep 7, 2024

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.

Co-authored-by: Henry Moore <henry.moore@picknik.ai>
Signed-off-by: Ryan <ryanfriedman5410+github@gmail.com>
@Ryanf55
Copy link
Author

Ryanf55 commented Sep 19, 2024

@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
Copy link

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
Copy link

@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

Signed-off-by: Marco A. Gutierrez <marcogg@marcogg.com>
@fujitatomoya
Copy link

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 Build Status
  • Linux-aarch64 Build Status
  • Linux-rhel Build Status
  • Windows Build Status

@fujitatomoya
Copy link

@Ryanf55 can you check the windows failure?

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.

4 participants