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

Aggregate Foxy Message API Review comments. #86

Merged
merged 5 commits into from
May 18, 2020
Merged

Conversation

tfoote
Copy link
Contributor

@tfoote tfoote commented Mar 12, 2020

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.

@ros-discourse
Copy link

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

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@tfoote tfoote Apr 7, 2020

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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.

@tfoote
Copy link
Contributor Author

tfoote commented Mar 13, 2020

For easier review of any differences to ROS 1 I ran a diff of msg and srv folders installed between melodic and eloquent. It's not entirely up to date but has most existing changes.

https://gist.github.com/tfoote/9a9525ed69c3dd79914d0f7e255a872e

If you'd like to view it visually I've been using kompare which can view diffs side by side.

I flagged the following as changes of note:

Higlighed Diffs to ROS 1

BatteryState added temperatures
https://gist.github.com/tfoote/9a9525ed69c3dd79914d0f7e255a872e#file-messages-diff-L11
https://gist.github.com/tfoote/9a9525ed69c3dd79914d0f7e255a872e#file-messages-diff-L19

CameraInfo lowercase terms d, k, r, and p to follow naming conventions
https://gist.github.com/tfoote/9a9525ed69c3dd79914d0f7e255a872e#file-messages-diff-L56
https://gist.github.com/tfoote/9a9525ed69c3dd79914d0f7e255a872e#file-messages-diff-L65
https://gist.github.com/tfoote/9a9525ed69c3dd79914d0f7e255a872e#file-messages-diff-L72
https://gist.github.com/tfoote/9a9525ed69c3dd79914d0f7e255a872e#file-messages-diff-L81

Aside: ros-users mention: https://gist.github.com/tfoote/9a9525ed69c3dd79914d0f7e255a872e#file-messages-diff-L206

Quaternion default values:
https://gist.github.com/tfoote/9a9525ed69c3dd79914d0f7e255a872e#file-messages-diff-L642-L645

SolidPrimative
Limit array size https://gist.github.com/tfoote/9a9525ed69c3dd79914d0f7e255a872e#file-messages-diff-L982

Header remove seq https://gist.github.com/tfoote/9a9525ed69c3dd79914d0f7e255a872e#file-messages-diff-L1051

DisparityImage lowercase parameter t https://gist.github.com/tfoote/9a9525ed69c3dd79914d0f7e255a872e#file-messages-diff-L1166

APIReviewFoxy.md Outdated Show resolved Hide resolved
APIReviewFoxy.md Outdated Show resolved Hide resolved
Co-Authored-By: William Woodall <[email protected]>
Copy link
Contributor Author

@tfoote tfoote left a comment

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

See image comment.

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

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

Open ticket

Copy link
Contributor Author

@tfoote tfoote left a comment

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

Proposed to rename this and the below with semantically correct services names. Deprecate the current names.

APIReviewFoxy.md Outdated
### navigation_msgs


#### map_msgs
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

Remove it not used

@dirk-thomas
Copy link
Member

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 .idl and from there into source code in various languages.

tfoote added 2 commits April 7, 2020 17:59
Signed-off-by: Tully Foote <[email protected]>
@tfoote
Copy link
Contributor Author

tfoote commented May 6, 2020

Immediate followups:

Enhancement Requests which need more research:

@ros-discourse
Copy link

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

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.

6 participants