-
Notifications
You must be signed in to change notification settings - Fork 146
Description
We recently merged a PR to add geometry_msgs/PoseStampedArray #262 however I'd like to propose renaming that because I believe that it doesn't carry the necessary semantic information appropriate reuse in the ecosystem. And the PoseStampedArray was already discussed in ros/common_msgs#144
Why should we have Strong Typing
As background early in the ROS ecosystem we created a bunch of basic datatypes of what seemed like they would be useful datatypes. However over the years we learned that these generic datatypes that have a structure but not semantic meaning can actually be counter productive in effective reuse. Counterintuitively by reusing the same datatype for different semantic information we break the systems ability to help the user by validating datatypes match. An example of this is that we originally thought that we could use the https://github.com/ros/std_msgs/blob/kinetic-devel/msg/Float32MultiArray.msg for capturing point clouds. There's a really neat way to define the layout and it's completely generic: https://github.com/ros/std_msgs/blob/kinetic-devel/msg/MultiArrayLayout.msg However with this you now have the problem that you potentially have incompatible versions of MultiArray datatypes running around on the system and it's unclear if any given node will know how to process a MultiArray message because it could be an Image or it could be a PointCloud. They both pack into a MultiArray datatype, and just have some different layout settings. And this means that you can accidentally connect the point cloud source to your image processing pipeline. And vice versa for the camera and the point cloud processing pipeline. But with the distinct datatypes we can be confident that image sources and image processing pipelines will connect and be able to process the data without a runtime error letting you know that the data is incompatible because of the detected layout fields. In that case the layout is at least enough to detect at runtime that the datatypes are incompatible, however an even more pernicious error is if the datatypes are indistinguishable at runtime. For example if we were to have a range sensor and a force sensor on the front bumper both producing float32 values, one in meters, one in newtons. If the configuration accidentially connects these two sensors to the wrong topic on an emergency stop, you can get a robot that will think that it's hitting free space, and that when it has hit something that there is free space ahead. With dedicated types this miss-configuration is impossible. And furthermore with dedicated types, rviz can have semantically meaningful displays for each datatype, showing a ranging cone for the Range measurement automatically, and a symbol for the Force feedback which is proportional to the force applied. Without the datatypes it would be necessary to configure this for each topic with ambiguous datatypes and that's just another place for potential miss-configuration.
Looking at geometry_msgs/PoseStampedArray
Looking at geometry_msgs/PoseStampedArray it is field for field identical to the already existing nav_msgs/Path and as argued in #262 nav_msgs/Path does not have the appropriate semantics to capture the information needed for the nav2 stack. That's completely valid and I think we should support this, however we should be making another semantically specific datatype so that we can again know if we're veering into new territory and the new scope will break existing assumptions.
To this end I would like to propose that we rename the message and provide clear documentation in the message of what it represents and what it doesn't so that it can be unambiguously interpreted by any subscriber.
I've spent some time reviewing the PR to merge this into nav2 ros-navigation/navigation2#4791
It's clear that this datatype is being used to represent goals: https://github.com/ros-navigation/navigation2/pull/4791/files#diff-81bfc646583fcbc8bc1e8478251bb41be1fcff7499e5fccfef817c7d25b7d23cL34
This the name should probably reflect that it is indtended to represent goals.
To that end I would like to propose that we rename it to nav_msgs/Goals which will sit as a peer to nav_msgs/Plan
And we should be filling in comments and documentation to reflechttps://github.com/ros2/common_interfaces/blob/rolling/nav_msgs/srv/GetPlan.srvt this usage.
I have the following outstanding questions about the intent of this message that I'd like to add documentation to the message to cover:
- Is there meaning to the header.frame_id or should it be always blank?
- What is the meaning of the ordering of the array entries?
- I see a reference to via points being removed which suggests there is an ordering.
- This is getting into how this is semantically different than the nav_msgs/Path
- What is the meaning of the timestamp on the individual Poses?
- Is that the time at which it should be traversed? Or is that the time at which it was computed?
- What validation should be applied to the timestamps?
With respect to the structure:
- If this is capturing goals should we also be capturing tolerances? In space, in orientation, in time? We have this sort of thing in https://github.com/ros2/common_interfaces/blob/rolling/nav_msgs/srv/GetPlan.srv
- As a minor additional I think it would be best to replace the subfield
poseswithgoalsso that when you iterate it's more likefor goal in mymsg.goals:but that's less important than the top level naming.
I would love to get some feedback from @SteveMacenski and @tonynajjar on how it's being used in practice so we can make sure to capture their full expectations of the semantic datatypes and provide that for future implementers too.