Skip to content

APIReviewFoxy.md

Latest commit

 

History

History
746 lines (505 loc) · 58.9 KB

APIReviewFoxy.md

File metadata and controls

746 lines (505 loc) · 58.9 KB

API Review for Foxy Release

This document is both the working copy and will be the result of the ROS 2 Messages API review in preparation for Foxy. It is contained in the common_interfaces repository as that's where most of the messages under discussion are, however the scope includes other packages for a general review.

What’s the goal of this review?

In preparing for the Foxy release we are bringing up new QA standards. TODO REP LINK

Outputs we'd like to generate are:

  • A list of known issues.
  • Triage the list into things we would like to do before 1.0.0 (foxy) and things that could or must be done later.
  • Create or improve issues so that we have properly captured these issues for future improvements.

What is not the goal of this review?

  • Achieving consensus on solutions to API problems. Instead we’d prefer to create well written issues that collect everything that’s known, so that conversations can be carried out in parallel on that issues.
  • Fix problems with API. We hopefully will be able to fix some things before foxy, but that’s out-of-scope for the review.

TODO review for changes to ROS 1 and history of changes/last change

ROS 2 Message Packages API Review

Process

Please read through the listed APIs and if you have any comments fill them out here:

https://docs.google.com/forms/d/e/1FAIpQLSeNogqYD0tPe7CKBP0dpSqFmf54NzWpj8ksN8vuAq55JupyIg/viewform?usp=sf_link

These comments will be used as an starting point for the review.

Repositories:

Comment: Use FQN for interface types that are members of other interfaces. For example, inside std_msgs/msg/Header, the stamp field should have type "builtin_interfaces/msg/Time" instead of "builtin_interfaces/Time".

Suggested Action: Consider updating all message definitions to use this syntax for clarity. Caveat, do we support members of non-msg types?

Followup This isn't necessary for now but the more complete approach might be useful for future idl usage. This would require an update to msg format. We'll create an issue for future consderation.

ros2/design#277

common_interfaces

actionlib_msgs

Package officially deprecated and to be removed when ros1_bridge has support for actions. It should not be used by users.

Suggested Action: review deprecation and make sure it’s clear.

Followup: #90

Msg

Diagonstic_msgs

No comments on these messages.

They are unchanged in ROS 1 since 2014.

Msg

Srv

geometry_msgs

Last change in ROS 1 in 2015

Comment: Covariance expressed as a full matrix is heavy, where upper triangular is adaquate due to symmetry. And 64bit floats is also very heavy for the level of accuracy most applications need. This large of a message is prohibitive for low bandwidth links such as to drones and UUVs

Suggested Action: Create new Covariance representation (3x3 upper triangle and 6x6 upper triangle) as well as potential helper functions. Identify areas to leverage it and then determine a migration path. New messages can leverage it, but existing ones will have a long migration.

Followup #91

Msg

nav_msgs

Msg

  • https://github.com/ros2/common_interfaces/blob/master/nav_msgs/msg/GridCells.msg
    • Comment: Are the points in cells the center point? It could use some documentation
    • Suggested Action: Improve documentation.
    • **Followup: #95
  • https://github.com/ros2/common_interfaces/blob/master/nav_msgs/msg/MapMetaData.msg
    • Comment: Is origin the center point of the (0,0) cell? Which direction do cell coordinates increase? Is (0,0) x,y in a right handed coordinate system such that (0,0) is the bottom right cell, or is it row,col such that (0,0) is the top left cell?
    • **Suggested Action: **Improve documentation.
    • **Followup: #95
  • https://github.com/ros2/common_interfaces/blob/master/nav_msgs/msg/OccupancyGrid.msg
    • Comment: probability being 0-100 doesn't take advantage of all the bits the int8 type offers. 0-255 where 255 is 100% probability would have slightly better resolution.
    • Suggested Action: None. The message was designed to support both probabilities as well as special values like the -1 unknown value. Switching to a slightly higher precision would cause a lot of disruption. If there is a use case for needing higher precision then it would make sense to define another message with significantly higher precision, potentially something like 32bit floating point. But until there’s a need for that we should stick with the status quo.
    • Followup #96
  • https://github.com/ros2/common_interfaces/blob/master/nav_msgs/msg/Odometry.msg
  • https://github.com/ros2/common_interfaces/blob/master/nav_msgs/msg/Path.msg
    • Comment: Path message is stamped, but each individual pose is also stamped. It's unclear what each stamp means. Is the stamp in each individual pose for the time the robot needs to be at that location? If so, the frame_id that's in each individual header is still redundant with the frame_id on the Path message itself. However, stamping each individual pose seems inflexible to the robot being delayed. If it's slowed down mid path, should it target the pose at the current time, or the next closest to it's current location? This seems hard to answer if the path makes a loop such that two points on the path are physically close but far apart in time. For a little while nav2 had it's own version of Path which had non-stamped poses, though they removed it: https://github.com/ros-planning/navigation2/pull/1107/files#diff-2d8dabb75c11aa980f6c2629eba1e75f
    • Suggested Action: Iterate with navigation to improve documentation of semantics of timestamps. Note that there are multiple valid behaviors for any given path representation.
    • Followup #97

Srv

sensor_msgs

Msg

Srv

Implementations

Proposed

  • sensor_msgs/Bumper
    • Comment: Should we add a message to describe the state of the bumper? Certainly the iRobot Create (Turtlebot1) and the kobuki (Turtlebot 2) have them, I'd assume other robots have them as well. Complicating factors here include bumpers with multiple sensors (iRobot Create can report at least 5 different bump "zones"), and bumpers that are based on hall-effect sensors (the iRobot Braava Jet has one of these, for instance).
    • Suggested Action: Look at potential additions but I would suggest continuing to protoype in other packages before trying to promote it to sensor_msgs in an untested form. There is a kobuki_msgs/BumperEvent but that won’t work for Roomba/Create.
    • Followup As this is a potential new message we'll defer from reviewing and it can be proposed.

shape_msgs

Msg

Std_msgs

Comment:

std_msgs: Often in ROS 1 we see recommendations to “not use std_msgs directly but semantically meaningful messages instead”.


It would be good to treat differently the ones that are expected to be used (e.g. Header) from the ones that “should not be used”


Suggestions:


take out the non-semantically meaningful messages out of std_msgs to e.g example_interfaces, and put them in another package (or remove them completely)


Move the widely used ones (e.g. Header) to their own package to avoid building and installing the non-semantically meaningful ones


It would both allow to save build time + install size as well as encouraging users to not use those in their custom messages


Some relevant past discussion [Suggestions for std_srvs](https://discourse.ros.org/t/suggestions-for-std-srvs/1079/10)

Suggested Action: Review moving non-semantically meaninful messages into a different package. Followup Issue #89

Msg

std_srvs

Srv

stereo_msgs

Comment: In addition to DisparityImage.msg , I always wished stereo_msgs would have a message type for stereo image pairs. Instead of a stereo camera driver having to publish two separate image topics, then having every subscriber needing to use an message synchronizer to capture the matching image pairs via matching timestamp, the driver could publish both images over a stereo image pair message. There could be common fixed length size message for an common array of two images, or perhaps in a separate message package, a message type for collecting a dynamic number of Image.msg for multi camera rigs, like motion capture or trinary cameras for multi view reconstruction.

Suggested Action: None. The design was chosen to send the parallel streams of data to avoid publishing data multiple times or requiring extra subscriptions. The simplest use case is if you want to process one of the two stereo images. If they’re published as a pair, you will have to receive both and then discard the entire second image which wastes a lot of resources. On the other hand we can simply have tools that collect the parallel streams of messages and give you callbacks when you get the groupings you want without causing extra bandwidth. Followup No Action. Colorization of point clouds is another example of only one subscription. Stick with not bundling them.

Msg

trajectory_msgs

Msg

visualization_msgs

Msg

Srv

example_interfaces

Services

Actions

rcl_interfaces

action_msgs

Comment: Refactor the package name to be consistent with other ROS 2 interface packages: "action_interfaces"

Comment: As for the internal action servers, in the rcl interfaces it is defined GoalInfo.msg and the goal related msgs and the cancelGoal.srv, however, there is no “standard” action msg (because this depends on the type of goal for the application). Maybe not API related, but it could be good to have a link to the ros2 actions tutorial in the rcl_interfaces/action_msgs repo.

Suggested Action: Consider cost/benefits of renaming Followup Issue ros2/rcl_interfaces#95

Msg

Srv

builtin_interfaces

Msg

  • https://github.com/ros2/rcl_interfaces/blob/master/builtin_interfaces/msg/Duration.msg
  • https://github.com/ros2/rcl_interfaces/blob/master/builtin_interfaces/msg/Time.msg
    • Comment: What clock is the time in reference to? System clock? ROS Clock?
    • Comment: There is no field for the rcl_clock_type_t. If any clock is used, other than the default RCL_SYSTEM_TIME, then accurate comparisons cannot be made when a message is received. Likewise, a message sender cannot specify if the time is monotonic or not, leaving the receiver to guess. Suggestion: add a field to denote the clock type
    • Suggested Action: Review communication of time primatives. The design so far has been that the only time that’s consistent across the system is system and ROS time. Steady time and monotonic clocks are not usually expected to be synchronized between systems and as such passing timestamps doesn’t make a lot of sense. But maybe there are use cases to do so. The time message could be extended, or a new datatypes could be added that’s differently typed to avoid accidental conversions.
    • Followup Issue ros2/rcl_interfaces#94

composition_interfaces

Srv

lifecycle_msgs

Msg

Srv

rcl_interfaces

Msg

Srv

rosgraph_msgs

Msg

test_msgs

N/A

unique_identifier_msgs

Messages

navigation_msgs

map_msgs

Action Ticket: Higher level issue that this and nav_msgs should be homogenized. Followup Issue ros-planning/navigation_msgs#18

Messages

Services

move_base_actions

Comment: Remove- move base msgs are unused in ROS2 Navigation

Suggested Action: Check for usage and if none, do not release into Foxy

Followup Issue ros-planning/navigation_msgs#15