Add cbor-raw compression support (ros2) #574
Conversation
The CBOR compression is already a huge win over JSON or PNG encoding, but it’s still suboptimal in some situations. This PR adds support for getting messages in their raw binary (ROS-serialized) format. This has benefits in the following cases: - Your application already knows how to parse messages in bag files (e.g. using [rosbag.js](https://github.com/cruise-automation/rosbag.js), which means that now you can use consistent code paths for both bags and live messages. - You want to parse messages as late as possible, or in parallel, e.g. only in the thread or WebWorker that cares about the message. Delaying the parsing of the message means that moving or copying the message to the thread is cheaper when its in binary form, since no serialization between threads is necessary. - You only care about part of the message, and don't need to parse the rest of it. - You really care about performance; no conversion between the ROS binary format and CBOR is done in the rosbridge_sever.
|
Sorry, I am not involved in the project anymore. |
|
If you happen to know who is currently involved, I’d appreciate a pointer! Thanks |
Major changes were made to ros1 develop after ros2 was branched, including fixes for backpressure and deadlock conditions. To summarize, in current ros2 websocket server outgoing messages are infinitely queued by Tornado after the implemented rosbridge_library queue/throttle features. This means a client can cause OOM on the server by subscribing to topics and not handling the messages fast enough. The fix:
|
|
Do you see an easy path to backport (forward-port?) these changes to the ros2 branch? Was there any particular reason the ros2 branch wasn't published in a release since Dashing? I'd love to get this feature in and get it published in some or all of Foxy/Galactic/Humble if possible. |
Potentially just re-apply all the PRs, as long as there's E2E tests on the ros2 branch (like there are in ros1 develop) this should be low risk.
Not that I know of, just need somebody to push the button. I don't have any reason to take ownership of this atm. |
|
Yep, re-applying the PRs makes sense. Unfortunately it looks like the e2e tests were disabled in one of the initial commits porting to ROS 2. If someone (@dirk-thomas?) could point me at a suitable replacement for rostest, I could work on re-enabling the tests. Part of my question was also: do you think it's possible to make ros1 and ros2 releases from the same branch of the same repo? That way we wouldn't continue having feature disparities between the two branches. Alternatively, would it make sense to make ros2 the default branch, and treat the ros1 develop branch as bugfix-only? |
I think launch_testing is current practice.
Don't think it's practical to use the same branch. Would make sense for ros2 to be default, hopefully everybody understands ROS1 branch is maintenance only since we're now in the final LTS distro. |
|
(or maybe rename develop to ros1, and ros2 to develop, or whatever is needed to bring this repo in line with standard branch naming for ROS projects?) |
|
I have been in charge of the release process for ros1. I don't think anyone is in charge of ros2 at the moment. If anyone with proper background steps up for ros2 release responsibility, I am happy to help you. |
|
@jtbandes I don't think I am joining the next web tools group meeting. Would you send me an email with one or two github ids on it? I will invite them as maintainers for ros2 branch.
I agree that we should set ros2 branch as default at some point. But, I am unsure whether it is the right time to do it. |
…uiltin_interfaces in combined definitions (#597) In #574, I implemented get_topics_and_raw_types by reconstructing message definitions from generated Python code. It recently came to my attention [via ROS Answers](https://answers.ros.org/question/321062/python-rosmsgrospack-in-ros2/?answer=354166#post-id-354166) that `ros2 interface show` and similar tooling actually access the **original** `.msg` files using a function called `rosidl_runtime_py.get_interface_path`. This seems to work reliably enough, and produces better fidelity message definitions (comments, constants, and default values are preserved), so it can be used instead of reconstructing definitions from the generated code. This PR also removes `/msg/` again from the "raw types" response. In #584 and #591 I made sure `/msg/` was included in datatype names and definitions because it seemed more correct in ROS 2. But while writing this PR, I realized that `.msg` definition files actually do not and **cannot** include `/msg/` in their references to other message types — it is rejected by the [rosidl adapter](https://github.com/ros2/rosidl/blob/36ed120f43daeaab31fd9ba2bf8dfb58db05091d/rosidl_adapter/rosidl_adapter/parser.py#L188-L190) at build time — so I think it should be excluded from the combined definition which is meant to stay as close as possible to the original .msg sources. Still, the **datatype names** do seem to include `/msg/`, so I'm keeping them there. Unfortunately this means that the top-level datatype names don't match the contents of the definitions, but this isn't a big deal since the definitions are all self-contained. Finally, this PR removes logic that excluded `builtin_interfaces/Time` and `builtin_interfaces/Duration` from the combined message definitions. While these are "builtin" interfaces, they are technically still messages, so it seems more correct to include their contents in the combined definitions: https://groups.google.com/g/ros-sig-ng-ros/c/f7OcEJBj9tU > Time and Duration are now messages and not built-in types. This is a good thing, because it makes our builtin types more portable. For example, you don’t need to depend on rostime (a C++ library) to use our generated message code. See also: ros2/ros2#1159
This PR is an adaptation of #452 for the
ros2branch.After the initial cherry-pick, a new implementation of the
/rosapi/get_topics_and_raw_typesservice was required for ROS 2, because the generated Python msg classes don't contain the original/combined message definition (see related rosbag2 discussion at ros2/rosbag2#782). To accomplish this, I added astringify_field_types()which reconstitutes a plausible .msg definition (although without constants or default values) based on the_fields_and_field_types, in the====-separated format seen in ROS 1 bags, which can be parsed by ROS 2 .msg parsers such as @foxglove/rosmsg.I hope that
stringify_field_typescould be removed in the future if message generation is modified to output the original message definition text (ros2/ros2#1159) — I see it as a stopgap solution to get basic compatibility working.I also modified
get_publications_and_typesto remove the extra/msg/from the ROS 2 IDL names, sostd_msgs/msg/Stringbecomesstd_msgs/Stringwhen the list of topics and datatypes are retrieved. This is more in line with what clients migrating from ROS 1 expect.I had to make a few launch file / parameter changes to get the
rosbridge_websocket_launch.xmlto work. The test included with #452 is not enabled, because there seems to be norostestreplacement for ROS 2 and I wasn't sure how best to port the test. I noticed that some other tests are also disabled in this branch.@mvollrath or @dirk-thomas, would you be able to help review this? I'd also be interested in pursuing including this package into Foxy/Galactic/Humble. It looks like it was released in Dashing but has been dropped recently (#536) and I'm not clear if that was intentional, or if it might just take a little shepherding to get it back into a release again.
(I'm also curious if you think there is any future where the ROS 1 and 2 versions of this package might be able to coexist, rather than living as two separate branches forever. It seems like there have already been several changes on develop that aren't in the ros2 branch...)