ros2 / rmw_fastrtps

Implementation of the ROS Middleware (rmw) Interface using eProsima's Fast RTPS.
Apache License 2.0
157 stars 117 forks source link

Fix TypeSupport_impl.cpp file being in wrong package causing rmw_fastrtp_cpp to not be usable in the single type support case #594

Closed sloretz closed 2 years ago

sloretz commented 2 years ago

This fixes an unusual bug where TypeSupport_impl.cpp is in the wrong package. It's in rmw_fastrtps_shared_cpp but it should be in rmw_fastrtps_dynamic_cpp because that's were TypeSupport_impl.hpp is. It went unnoticed because rmw_fastrtps_dynamic_cpp links with rmw_fastrtps_shared_cpp, and nothing uses it in rmw_fastrtps_shared_cpp.

It being in the wrong package causes another bug where because rmw_fastrtps_cpp depends on rmw_fastrtps_shared_cpp, and it has one of rmw_fastrtps_dynamic_cpp's source files, it ends up depending on the introspection typesupport. That means the single typesupport case which uses dynamic linking instead of dlopen cannot be activated with rmw_fastrtps_cpp because there are always two typesupports when there should actually be just one.

@audrow @clalancette FYI added as reviewers to check that this qualifies as a bug fix since the RMW freeze is in effect

sloretz commented 2 years ago

Converted to draft because this isn't as straight forward to fix as I thought.

MiguelCompany commented 2 years ago

@sloretz TypeObject and the introspection type support are necessary for content filters. Package rmw_fastrtps_cpp is not single typesupport anymore

sloretz commented 2 years ago

Package rmw_fastrtps_cpp is not single typesupport anymore

That's unfortunate. Thanks for the info.

I see from https://github.com/ros2/rmw_fastrtps/pull/513/files#r823811515 that the introspection typesupport is used for field offsets in the serialized message. What amount of effort would it take to make rosidl_typesupport_fastrtps_c/cpp provide those offsets instead?

MiguelCompany commented 2 years ago

What amount of effort would it take to make rosidl_typesupport_fastrtps_c/cpp provide those offsets instead?

Apart from the offsets, the content filter support needs information about the name of the fields, their type, and their bounds, so it can perform semantic checks on the filter expression, and map the field names on it to the internal offset calculation functions. And then, there is no way to convey that information to Fast DDS, apart from the TypeObject.

We thought about adding the generation of the TypeObject to rosidl_typesupport_fastrtps_c/cpp, but this had two caveats:

So I think that if we want rmw_fastrtps_cpp to be single typesupport, we have the following options:

Thoughts?

sloretz commented 2 years ago

Thanks for explaining @MiguelCompany. I don't have any thoughts right now but I'll bring it up at the ROS 2 meeting next week.

I'll close this PR for now since it's intended behavior and not a bug.