-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Document NaN support in interfaces and NaN usage in CLI #4210
base: rolling
Are you sure you want to change the base?
Conversation
Signed-off-by: Ryan Friedman <[email protected]>
@@ -183,6 +183,7 @@ Field default value | |||
|
|||
Default values can be set to any field in the message type. | |||
Currently default values are not supported for string arrays and complex types (i.e. types not present in the built-in-types table above; that applies to all nested messages). | |||
Special floating point values such as ``NaN``, ``+infinity``, and ``-infinity`` are not yet supported as default values. |
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.
just a comment, this does not block the PR.
instead of adding the limitation here temporary, can we update the doc after ros2/rosidl#789?
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.
Yep, I'm not in a rush to fix the docs, and am happy to revise if we can get a quick review and merge on the ROSIDL PR.
source/Tutorials/Beginner-CLI-Tools/Understanding-ROS2-Topics/Understanding-ROS2-Topics.rst
Outdated
Show resolved
Hide resolved
Co-authored-by: Tomoya Fujita <[email protected]> Signed-off-by: Ryan <[email protected]>
Certain messages use ``NaN`` as a significant value. | ||
For example, with a ``sensor_msgs/msg/BatteryState``, the temperature field may not be measured. | ||
If so, it should be set to ``NaN``. | ||
|
||
.. code-block:: console | ||
|
||
ros2 topic pub /battery sensor_msgs/msg/BatteryState '{voltage: 12.4, temperature: .nan}' | ||
|
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.
I don't think that this is appropriate for the Beginner documentation. The point of the beginner docs is to show one way to do it, not to document all of the nuances and alternate ways to do it. Instead, I'll suggest putting this in another tutorial or how-to guide that talks about ros2 topic pub
.
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.
Sounds like I could write a new how-to guide on for Advanced usage of ros2 topic
. Seems like a good spot to also talk about hidden
topics since the docs don't cover those
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.
Sounds like I could write a new how-to guide on for
Advanced usage of ros2 topic
. Seems like a good spot to also talk abouthidden
topics since the docs don't cover those
That sounds very reasonable to me. That could also possibly go into Tutorials/Advanced .
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.
@Ryanf55 Would you like to remove this portion from this PR, so we can put the changes to About-Interfaces
in? Then when you have time you could write the advanced tutorial.
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.
yea, I'm swamped now. in the mean time, can we get some capable maintainers to review the two proposed PR's and provide input to the blocks with IDL and JSON not supporting NaN?
Purpose
Document what is currently supported by ROS when using NaN/infinity values for floating points in messaging.
Why
sensor_msgs/msg/BatteryState
tells users to useNaN
, but there's not info in the docs on working with NaN's. This documents what currently exists. I plan to improve support for NaN's across the ROS ecosystem because they are heavily used in the aerial messages of REP-147 and in MAVLink.Ticket
Closes #4135