Remove rmw_fastrtps dependency from rosbag2 cdr converter plugin

[rotu]

Description

When running rosbag2, it does not use the expected RMW implementation, instead always using FastRTPS.

Instead of depending on rmw_fastrtps_cpp, depend on Fast-CDR directly to get the CDR implementation for rosbag2_converters_default_plugins

Original ticket context

Expected Behavior

If RMW_IMPLEMENTATION=rmw_cyclonedds_cpp, cdr_converter should use rmw_cyclonedds_cpp.

Actual Behavior

If RMW_IMPLEMENTATION=rmw_cyclonedds_cpp, rosbag2_converter_default_plugins tests fail.

/opt/ros/master/build/rosbag2_converter_default_plugins/Testing/20200312-1905/Test.xml: 7 tests, 0 errors, 1 failure, 0 skipped
- test_cdr_converter
  <<< failure message
    -- run_test.py: invoking following command in '/opt/ros/master/build/rosbag2_converter_default_plugins':
     - /opt/ros/master/build/rosbag2_converter_default_plugins/test_cdr_converter --gtest_output=xml:/opt/ros/master/build/rosbag2_converter_default_plugins/test_cdr_converter.gtest.xml
    [ERROR] [1584039940.511109472] [rcl]: Expected RMW implementation identifier of 'rmw_cyclonedds_cpp' but instead found 'rmw_fastrtps_cpp', exiting with 102.
    -- run_test.py: return code 102
    -- run_test.py: generate result file '/opt/ros/master/build/rosbag2_converter_default_plugins/test_cdr_converter.gtest.xml' with failed test
    -- run_test.py: verify result file '/opt/ros/master/build/rosbag2_converter_default_plugins/test_cdr_converter.gtest.xml'
  >>>
/opt/ros/master/build/rosbag2_converter_default_plugins/test_cdr_converter.gtest.xml: 1 test, 0 errors, 1 failure, 0 skipped
- rosbag2_converter_default_plugins test_cdr_converter.gtest.missing_result
  <<< failure message
    The test did not generate a result file:

    [ERROR] [1584039940.511109472] [rcl]: Expected RMW implementation identifier of 'rmw_cyclonedds_cpp' but instead found 'rmw_fastrtps_cpp', exiting with 102.
  >>>
/opt/ros/master/build/spdlog_vendor/Testing/20200312-1833/Test.xml: 2 tests, 0 errors, 1 failure, 0 skipped

To Reproduce

1. (not sure if necessary) touch src/ros2/rmw_fastrtps/rmw_fastrtps_dynamic_cpp/COLCON_IGNORE
2. colcon build
3. RMW_IMPLEMENTATION=rmw_cyclonedds_cpp colcon test --packages-select rosbag2_converter_default_plugins
4. colcon test-result

System (please complete the following information)

• OS: Ubuntu Focal
• ROS 2 Distro: Foxy
• Version: master

[Karsten1987]

I agree this is suboptimal, but on the other hand it's to be expected.

For context, the converter plugin has the following task: Given a bag file which was recorded with a non-DDS RMW implementation, this bag file might not be generally usable on other machines where this specific RMW implementation is not available. One could think of a setup where a highly optimized RMW implementation is deployed on an autonomous vehicle, but the setup on the developer machines is using a default ROS2 installation (DDS). In order to convert the messages from their custom serialization format to something generic, we introduced the converter plugins. By default, we provide a CDR converter. That is, the custom serialization format is getting converted to a ROS2 C++ instance and then re-serialized with the converter plugin to CDR. Back then, when we originally developed rosbag2, we didn't want to re-implement our own CDR serialization library. We've been thus relying on the ROS2 default implementation provided by Fast-RTPS. Hence the hardcoded dependency to it.

Now, as I mentioned, this is maybe not ideal, but at the same we don't have a real strong dependency to Fast-RTPS other than the default package for the CDR converter. As you might have noticed, your workspace should still compile just fine and solely the integration tests are failing. The converter logic is all plugin based so that it shouldn't really be that hard to come up with a second CDR converter plugin which is relying on other implementations such as Cyclone. I believe in this case, a simple search&replace in the existing default CDR plugin should do the trick.

Does this help?

---

Moving forward, I don't really see an easy way to provide a generic converter plugin which doesn't rely on any of the default RMW implementations. The only option I see to ease this hardcoded dependency is by providing our own CDR implementation. I currently don't have capacity for this and honestly don't see it as a high priority as for now, but I am happy to review any PR which tries to address this issue.

[emersonknapp]

@Karsten1987 I was just thinking about this, given the choice to use CycloneDDS as the default RMW implementation in G-turtle. Theoretically the CDR format should be the same between all DDS implementations given that it's part of the DDS Spec? But, a ros_base installation would now be pulling rmw_fastrtps only due to rosbag2_default_converter_plugins depending on it.

It doesn't make sense to me that we would create a new CDR implementation, perhaps we can pull a dependency directly on e.g. https://github.com/eProsima/Fast-CDR and avoid the larger rmw_fastrtps dependency chain (Cyclone doesn't seem to be broken up into separate libraries in a way such that we could take the CDR implementation from it)

[Karsten1987]

we had this situation earlier even without the official change where people wanted to use rosbag2 exclusively with CycloneDDS. I've been simply ignoring this for now - you can actually see this in the converter tests which bluntly doesn't run the tests if FastCDR isn't present. I guess we can't easily do this now.

While I generally agree with you that creating a new CDR implementation makes little sense, I am wondering if it makes sense to copy/fetch an existing implementation - talking about cmake's ExternalProject_Add - and don't rely on it as a dependency. While I think it's not the most elegant situation, we kind of guarantee that the converter is always present and doesn't have to be compiled separately.

[clalancette]

While I generally agree with you that creating a new CDR implementation makes little sense, I am wondering if it makes sense to copy/fetch an existing implementation - talking about cmake's ExternalProject_Add - and don't rely on it as a dependency. While I think it's not the most elegant situation, we kind of guarantee that the converter is always present and doesn't have to be compiled separately.

Is there a reason you can't just rely on Fast-CDR as a dependency? That is, I don't really see a need to compile Fast-CDR twice if you can just use the version that is already available in the distribution.

[Karsten1987]

That's fair enough. I was just thinking that we should keep further dependencies to a minimum. But I guess that's not a big issue for now. So let's go for this then!

[clalancette]

Just as an FYI: this issue is what is causing the current Rpr failures, like: https://build.ros2.org/view/Rpr/job/Rpr__rosbag2__ubuntu_focal_amd64/537/consoleFull

Essentially what is happening is that the PR job looks at all of the package.xml in the repository, and computes the package dependencies to satisfy that. Since rosbag2_converter_default_plugins has an explicit dependency on rmw_fastrtps_cpp, that is one of the packages that is installed.

But the further piece of the puzzle is that rmw_cyclonedds_cpp is now the default. Usually the rmw_implementation debian file pulls the default in, but looking at the debian control file, there is an important subtlety there. In particular, ros-rolling-rmw-cyclonedds-cpp | ros-rolling-rmw-connext-cpp | ros-rolling-rmw-fastrtps-cpp means that any of those packages will satisfy the dependency. For most packages, there is no explicit dependency on a particular rmw, so it will just choose the first (rmw_cyclonedds_cpp). But in this case, there is an explicit dependency so it only needs to install rmw_fastrtps_cpp to satisfy it. Then when the tests go to run, they try to open up librmw_cyclonedds_cpp.so and fail.

I think the best way to fix this problem is to remove the dependency on rmw_fastrtps_cpp and instead switch to Fast-CDR. That should fix the issue.

[emersonknapp]

I think the best way to fix this problem is to remove the dependency on rmw_fastrtps_cpp and instead switch to Fast-CDR. That should fix the issue.

Yes - that's the plan. I've updated the title and ticket description to reflect this.

As a note, this is on my AWS roadmap in time for Galactic - but I won't assign the ticket to myself until I start working on it. If somebody else has it as a higher priority and picks it up before me just let us know here.

[clalancette]

Yes - that's the plan. I've updated the title and ticket description to reflect this.

As a note, this is on my AWS roadmap in time for Galactic - but I won't assign the ticket to myself until I start working on it. If somebody else has it as a higher priority and picks it up before me just let us know here.

Sounds good, thanks.

[emersonknapp]

Shuffled around some priorities - I've started working on this now

[emersonknapp]

After spending a bit of time with this - I'm realizing that "just depend on Fast-CDR" may not make sense as a solution (unless I'm missing something).

Reasoning:

• the conversion method needs to convert between the serialized format (e.g. CDR) and the ROS in-memory format (e.g. the output of rosidl_generator_cpp like class std_msgs::msg::String)
• this conversion happens, in ROS 2, exclusively through the rosidl_typesupport_{RMW_IMPL} stuff, which is generated on a per-rmw-implementation basis "Just use Fast-CDR" doesn't get us any of the generated typesupport code to actually understand the mapping between the ROS structs and the CDR format.

@Karsten1987 let me know if you've already thought about this, I feel like I'm missing something. Maybe something about how typesupport works that I don't know.

If the above train of thought is correct, then I think we might have to depend on RMW implementations for all type conversions, and have to have the RMW implementations installed for both the input and output types. To more generically support the conversion without explicitly depending on a given implementation, maybe we just search "all installed RMW implementations" - call rmw_get_serialization_format to check whether each implementation supports the serialization format we need, then use those to do conversion. With that architecture, we wouldn't need specific format plugins, it would be a generic "dynamic library search, load, call" implementation in a single class.

[Karsten1987]

First off, you're correct that we need a typesupport to create the actual ROS2 message. The general workflow for conversion is RMW_system -> Deserialize -> ROS2 message -> Serialize from ROS2 -> RMW_other.

However, while the CDR plugin relies on rmw_fastrtps_cpp to do the work, I believe we should be able to leverage the rosidl_typesupport_introspection_c/cpp to analyze the ROS2 message and pass in the respective fields to Fast-CDR's serialization/deserialization functionalities. Granted, this wouldn't come for free but would free us from any specific CDR rmw.

I am not completely sure I follow your last paragraph. What does "all" mean in "I think we might have to depend on RMW implementations for all type conversions". If I understand your idea correctly, there would no longer be any default CDR converter, but upon trying to convert any message from/to CDR we look for an installed RMW implementation which provides CDR? I think that could work as well - might even be easier and more elegant way, given that the default RMW will always be CDR in ROS2.

I am just not convinced that we should get rid of the plugin architecture though. I believe it makes sense to not solely rely on the RMW implementation, but give users the freedom to convert any message into a ROS2 message and then into CDR. In the bigger picture, we wanted to give rosbag2 users the chance to record any PDAP/UDP packages. That way we should have a converter plugin for custom protocols as well to convert these from/to a ROS2 message. Maybe we can combine both capabilities in your generic "library search, load, call" implementation.

[emersonknapp]

I am not completely sure I follow your last paragraph. What does "all" mean in "I think we might have to depend on RMW implementations for all type conversions". If I understand your idea correctly, there would no longer be any default CDR converter, but upon trying to convert any message from/to CDR we look for an installed RMW implementation which provides CDR? I think that could work as well - might even be easier and more elegant way, given that the default RMW will always be CDR in ROS2.

Yes - in the PR proposal I put up, there isn't any default converter, but instead one that looks for installed RMW implementations that provide the requested format (not CDR specifically). It is definitely less code and more flexible.

I am just not convinced that we should get rid of the plugin architecture though. I believe it makes sense to not solely rely on the RMW implementation, but give users the freedom to convert any message into a ROS2 message and then into CDR.

OK, I understand this, thanks for the context that rosidl_typesupport_introspection_cpp provides the information we would need. I can see the desire to provide a converter that does not implement the entire RMW API.

Maybe for an easy first-pass solution here, we could make this RMWImplementedConverter as the "default converter plugin" or "fallback converter plugin", which would be possible to load for any serialization formats, but may fail at initialization if it can't find a suitable RMW implementation. Format-specific plugins could be installed then that would implement serialization directly and be preferred before the RMWImplementedConverter. How does that approach sound?

In the bigger picture, we wanted to give rosbag2 users the chance to record any PDAP/UDP packages. That way we should have a converter plugin for custom protocols as well to convert these from/to a ROS2 message. Maybe we can combine both capabilities in your generic "library search, load, call" implementation.

In the implementation I put up in #669 - if there was a package that registered rmw_typesupport with the ament index, and implemented at least the three signatures rmw_get_serialization_format, rmw_serialize, and rmw_deserialize - then it would work here, regardless of how it was implemented. But maybe there is a drawback to this approach vs having an explicit rosbag2 plugin.

[emersonknapp]

Based on this discussion, I have an alternative implementation proposal in #670 - this adds a RMWImplementedConverter class that searches for an installed RMW implementation to use for conversion. It removes rosbag2_converter_default_plugins because the class is implemented and used directly within rosbag2_cpp as a fallback when no appropriate plugin is found. This proposal keeps the plugin infrastructure in place so that plugins may still be provided, and they are preferred over this fallback mechanism if present.