Aggregate Foxy Message API Review comments. #86
Conversation
Signed-off-by: Tully Foote <tfoote@osrfoundation.org>
|
This pull request has been mentioned on ROS Discourse. There might be relevant details there: https://discourse.ros.org/t/foxy-message-api-review/13105/14 |
APIReviewFoxy.md
Outdated
| * [https://github.com/ros2/common_interfaces/blob/master/geometry_msgs/msg/PointStamped.msg](https://github.com/ros2/common_interfaces/blob/master/geometry_msgs/msg/PointStamped.msg) | ||
| * [https://github.com/ros2/common_interfaces/blob/master/geometry_msgs/msg/Polygon.msg](https://github.com/ros2/common_interfaces/blob/master/geometry_msgs/msg/Polygon.msg) | ||
| * Comment: Polygon uses Point32 instead of Point. Would expect two message types for consistency: Polygon and Polygon32 | ||
| * **Suggested Action**: None. For “completeness” we could fill out the data types. But in general we don’t want to have a “complete” set of messages we would rather have everyone using the same datatype. There are very few use cases where a polygon is large enough require 64 bit precision and this saves 50% on bandwidth. It’s the same compromise as is made for point clouds. |
There was a problem hiding this comment.
Just a note, a polygon does not need to be large to justify 64 bit. It can also have a reference frame far away (UTM).
Of course this can be mitigated using in-between TF frames, but that defeats the purpose a little.
There was a problem hiding this comment.
If you're going to be working on those scales I'd recommend making another datatype with semantic meaning to support those use cases. A float is able to maintain millimeter precision to over 8 km, at which point you're experiencing a couple meters of curvature of the earth at the same time. This datatype is not complex to reproduce in one line of msg.
APIReviewFoxy.md
Outdated
| * **Suggested Action:** Review removal of header. | ||
|
|
||
|
|
||
| #### trajectory_msgs |
There was a problem hiding this comment.
This package contains only messages related to joints. So would joint_trajectory_msgs be a better name? A typical trajectory is also a path (see nav_msgs), so this might avoid some naming confusion.
Co-Authored-By: William Woodall <william+github@osrfoundation.org>
There was a problem hiding this comment.
We reviewed many of the proposed actions, but we ran out of time to get through all proposed actions. There will be a second followup.
Present @wjwwood @chapulina @dirk-thomas @jacobperron @Karsten1987 @claireyywang @cottsay @scpeters
APIReviewFoxy.md
Outdated
|
|
||
| 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? |
There was a problem hiding this comment.
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. Related to generator review.
APIReviewFoxy.md
Outdated
|
|
||
| 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. |
There was a problem hiding this comment.
There might already be a ticket for this. Double check
APIReviewFoxy.md
Outdated
|
|
||
| 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. |
There was a problem hiding this comment.
Ticket for long term planning.
APIReviewFoxy.md
Outdated
| * [https://github.com/ros2/common_interfaces/blob/master/geometry_msgs/msg/InertiaStamped.msg](https://github.com/ros2/common_interfaces/blob/master/geometry_msgs/msg/InertiaStamped.msg) | ||
| * [https://github.com/ros2/common_interfaces/blob/master/geometry_msgs/msg/Point.msg](https://github.com/ros2/common_interfaces/blob/master/geometry_msgs/msg/Point.msg) | ||
| * Comment: Point and Vector3 are the same message | ||
| * **Suggested Action**: None, these are the same data structure but semantically different. |
There was a problem hiding this comment.
Ticket to add documentation. Cross reference.
APIReviewFoxy.md
Outdated
| * [https://github.com/ros2/common_interfaces/blob/master/geometry_msgs/msg/PolygonStamped.msg](https://github.com/ros2/common_interfaces/blob/master/geometry_msgs/msg/PolygonStamped.msg) | ||
| * [https://github.com/ros2/common_interfaces/blob/master/geometry_msgs/msg/Pose.msg](https://github.com/ros2/common_interfaces/blob/master/geometry_msgs/msg/Pose.msg) | ||
| * Comment: Pose and Transform seem to be the same message, except one uses `Point` and the other uses `Vector3`. | ||
| * **Suggested Action**: None, these are the same data structure but semantically different. |
There was a problem hiding this comment.
Ticket adding integrated doc references to semantics.
APIReviewFoxy.md
Outdated
| * Fill image: [https://github.com/ros2/common_interfaces/blob/master/sensor_msgs/include/sensor_msgs/fill_image.hpp](https://github.com/ros2/common_interfaces/blob/master/sensor_msgs/include/sensor_msgs/fill_image.hpp) | ||
| * Image encodings: [https://github.com/ros2/common_interfaces/blob/master/sensor_msgs/include/sensor_msgs/image_encodings.hpp](https://github.com/ros2/common_interfaces/blob/master/sensor_msgs/include/sensor_msgs/image_encodings.hpp) | ||
| * Comment: There's a whole slew of image encodings that are not represented in image_encodings.hpp. http://fourcc.org/rgb.php and http://fourcc.org/yuv.php are pretty exhaustive. We don't necessarily need to expose all of those, but certainly more of the common YUV ones would be nice to have. | ||
| * **Suggested Action:** Review image_encodings available in opencv and other platforms and potentially expand listed enumerations. |
APIReviewFoxy.md
Outdated
|
|
||
| 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. |
There was a problem hiding this comment.
Sounds good to plan for moving them out to something like example_msgs or demo_msgs.
Could build in a warning in the generator.
APIReviewFoxy.md
Outdated
| * Comment: Wasn’t TwistStamped added for cases where this mattered? I’m not entirely sure whether there are semantics attached to it that would prevent it from being used here. I’ve not seen it used with many mobile base drivers, but it would seem to be able to address this shortcoming of Twist. | ||
| * Comment: There is indeed now a twist stamped, but not much of the ecosystem uses it. I agree wider use would be valuable, especially in absence of a sequence. I’m going to file a ticket for that in Navigation/related. It won’t be in for foxy, but I’ll add a deprecation warning and move into TwistStamped in G turtle. The TRV command seems limited, I think we should stick to Twist. It can be used for omni robots with non-forward velocities and converted to ackermann trivially. Its also used on some drones. Thanks for that comment! Edit: ticket [https://github.com/ros-planning/navigation2/issues/1594](https://github.com/ros-planning/navigation2/issues/1594) | ||
| * Comment: [Ticket to remove seq from Header](https://github.com/ros2/common_interfaces/pull/2) | ||
| * **Suggested Action**: None. This has been deprecated for years, we can finally remove it. There are other more appropriate mechanisms to get information about dropped packets etc from the middleware than embedding that data into the users data. |
There was a problem hiding this comment.
see discourse thread. Stick with removal.
APIReviewFoxy.md
Outdated
|
|
||
| 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. |
There was a problem hiding this comment.
Colorization of point clouds is another example of only one subscription. Stick with not bundling them.
APIReviewFoxy.md
Outdated
|
|
||
| * [https://github.com/ros2/common_interfaces/blob/master/stereo_msgs/msg/DisparityImage.msg](https://github.com/ros2/common_interfaces/blob/master/stereo_msgs/msg/DisparityImage.msg) | ||
| * Comment: Header that might be removed in a future release, maybe this one? | ||
| * **Suggested Action:** Review removal of header. |
There was a problem hiding this comment.
Review round 2
@wjwwood @jacobperron @mabelzhang @chapulina @sloretz @tfoote @cottsay
APIReviewFoxy.md
Outdated
|
|
||
| * [https://github.com/ros2/common_interfaces/blob/master/visualization_msgs/msg/ImageMarker.msg](https://github.com/ros2/common_interfaces/blob/master/visualization_msgs/msg/ImageMarker.msg) | ||
| * Comment: In ImageMarker.msg, action and type are defined as int32, but their constants are defined as uint8. Probably action and type can be changed to uint8. | ||
| * **Suggested Action:** Review updating const type |
There was a problem hiding this comment.
Open enhancement issue
APIReviewFoxy.md
Outdated
| * [https://github.com/ros2/common_interfaces/blob/master/visualization_msgs/msg/InteractiveMarkerUpdate.msg](https://github.com/ros2/common_interfaces/blob/master/visualization_msgs/msg/InteractiveMarkerUpdate.msg) | ||
| * [https://github.com/ros2/common_interfaces/blob/master/visualization_msgs/msg/Marker.msg](https://github.com/ros2/common_interfaces/blob/master/visualization_msgs/msg/Marker.msg) | ||
| * Comment: In Marker.msg, action and type are defined as int32, but their constants are defined as uint8. Probably action and type can be changed to uint8. | ||
| * **Suggested Action:** Review updating const type |
There was a problem hiding this comment.
Use same issue as Image Marker
APIReviewFoxy.md
Outdated
|
|
||
| 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 |
There was a problem hiding this comment.
This is possible to do as it's a new package in ROS 2. Open a ticket to review possible migrations.
APIReviewFoxy.md
Outdated
| * [https://github.com/ros2/rcl_interfaces/blob/master/builtin_interfaces/msg/Time.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. |
There was a problem hiding this comment.
We definitely need to document the requirement of ros time better. Time since epoc across the synchronized system times, or simulation.
A use case is to be able to communicate hardware
We could make a steady time separate type to support hardware timestamps that are only relevant for relative comparisons. Or consider setting up a standard approach using duration deltas.
Larger issue of non-monotonic system time due to leap seconds. Solution is TAI (atomic clock based) Not something we're potentially doing right now. But might be something to consider in the future. It would be best to consider a different datatype.
APIReviewFoxy.md
Outdated
| * [https://github.com/ros2/rcl_interfaces/blob/master/rcl_interfaces/msg/ListParametersResult.msg](https://github.com/ros2/rcl_interfaces/blob/master/rcl_interfaces/msg/ListParametersResult.msg) | ||
| * [https://github.com/ros2/rcl_interfaces/blob/master/rcl_interfaces/msg/Log.msg](https://github.com/ros2/rcl_interfaces/blob/master/rcl_interfaces/msg/Log.msg) | ||
| * Constants in Log.msg do not match the type of level | ||
| * **Suggested Action**: Improve documentation of log levels for clarity. |
There was a problem hiding this comment.
Improve documentation. Different implementations have different level enumerations so we are using this abstraction to bridge between the different implementations that we can't change.
APIReviewFoxy.md
Outdated
| * [https://github.com/ros-planning/navigation_msgs/blob/jade-devel/map_msgs/msg/ProjectedMap.msg](https://github.com/ros-planning/navigation_msgs/blob/jade-devel/map_msgs/msg/ProjectedMap.msg) | ||
| * [https://github.com/ros-planning/navigation_msgs/blob/jade-devel/map_msgs/msg/ProjectedMapInfo.msg](https://github.com/ros-planning/navigation_msgs/blob/jade-devel/map_msgs/msg/ProjectedMapInfo.msg) | ||
| * Comment: Seems to be a problem with ProjectedMapsInfo.srv Because it’s a service that receives a map and returns nothing. | ||
| * **Suggested Action:** Review usage and documentation for a potential change in name and extension of return type |
There was a problem hiding this comment.
Proposed to rename this and the below with semantically correct services names. Deprecate the current names.
APIReviewFoxy.md
Outdated
| ### navigation_msgs | ||
|
|
||
|
|
||
| #### map_msgs |
There was a problem hiding this comment.
Ticket: Higher level issue that this and nav_msgs should be homogenized.
APIReviewFoxy.md
Outdated
|
|
||
| Comment: Remove- move base msgs are unused in ROS2 Navigation | ||
|
|
||
| **Suggested Action**: Check for usage and if none, do not release into Foxy |
|
It would be good to review the comments in existing message / service / action files to ensure they are being extracted into proper docblock parts when being converted to |
Signed-off-by: Tully Foote <tfoote@osrfoundation.org>
Signed-off-by: Tully Foote <tfoote@osrfoundation.org>
|
This pull request has been mentioned on ROS Discourse. There might be relevant details there: https://discourse.ros.org/t/soss-a-whole-new-approach-to-your-ros-1-ros-2-bridge/17040/18 |
This PR contains the aggregated results of the API review comments solicited in this thread. Please use this PR to make comments suggestions or give any other feedback on the existing comments.
I will be going over these actions to triage them for scale and prioritization on Friday the 13th. If you would like to be involved in the meeting please ping me directly.