-
Notifications
You must be signed in to change notification settings - Fork 247
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
Bugfix for rosbag2_cpp serialization converter #1814
Conversation
1686ebc
to
705ae8e
Compare
- Use rmw specific type support for rmw_serilize{deserialize} function calls. Note: It is ok for CycloneDDS to use introspection type support for rmw_serilize{deserialize} functions. However, for FastRTPS it must be rmw specific type support. e.g. rosidl_typesupport_cpp. Fix works for both CycloneDDS and FastRTPS rmw. Signed-off-by: Michael Orlov <[email protected]>
69b3c8e
to
1d37e18
Compare
Signed-off-by: Michael Orlov <[email protected]>
- Rationale: To make sure that the default serialization converter can serialize and deserialize messages with all supported rmw implementations. Since it uses rmw specific functions for serialization and deserialization inside. Signed-off-by: Michael Orlov <[email protected]>
Signed-off-by: Michael Orlov <[email protected]>
1d37e18
to
c35cca3
Compare
Signed-off-by: Michael Orlov <[email protected]>
Pulls: #1814 |
Signed-off-by: Michael Orlov <[email protected]>
@clalancette We are getting a new warning on RHEL build from the
It is from the: rosbag2/rosbag2_cpp/CMakeLists.txt Line 38 in e1c0cf9
This warning appeared because I enabled sanitizer by default and RHEL build doesn't support them for some reason. Please correct me if I am wrong, but if I understood correctly, merging with such a warning is not acceptable. |
Yes, that's correct; we shouldn't merge with that warning.
I have a slightly different suggestion here, which is to not enable sanitizers in this PR. It is not integral to this PR, so I think it should be done separately. In that separate PR, I think it is fine to skip the sanitizer on RHEL (or just downgrade the WARNING to STATUS), but at least it will be separate and easier to change/revert if we decide to do that later. |
This reverts commit 7241963. Signed-off-by: Michael Orlov <[email protected]>
Okay, I disabled sanitizers by default, as @clalancette suggested. We will enable them and try to workaround the warning on the RHEL build in a separate PR. |
https://github.com/Mergifyio backport jazzy iron |
✅ Backports have been created
|
* Bugfix for rosbag2 serialization converter - Use rmw specific type support for rmw_serilize{deserialize} function calls. Note: It is ok for CycloneDDS to use introspection type support for rmw_serilize{deserialize} functions. However, for FastRTPS it must be rmw specific type support. e.g. rosidl_typesupport_cpp. Fix works for both CycloneDDS and FastRTPS rmw. Signed-off-by: Michael Orlov <[email protected]> * Add test coverage for default rmv serialization format converter Signed-off-by: Michael Orlov <[email protected]> * Run test_serialization_converter for each rmw implementation - Rationale: To make sure that the default serialization converter can serialize and deserialize messages with all supported rmw implementations. Since it uses rmw specific functions for serialization and deserialization inside. Signed-off-by: Michael Orlov <[email protected]> * Address uncrustify formating warnings Signed-off-by: Michael Orlov <[email protected]> * Enable sanitizer by default Signed-off-by: Michael Orlov <[email protected]> * Address Windows build warnings Signed-off-by: Michael Orlov <[email protected]> * Revert "Enable sanitizer by default" This reverts commit 7241963. Signed-off-by: Michael Orlov <[email protected]> --------- Signed-off-by: Michael Orlov <[email protected]> (cherry picked from commit 6e82f52)
* Bugfix for rosbag2 serialization converter - Use rmw specific type support for rmw_serilize{deserialize} function calls. Note: It is ok for CycloneDDS to use introspection type support for rmw_serilize{deserialize} functions. However, for FastRTPS it must be rmw specific type support. e.g. rosidl_typesupport_cpp. Fix works for both CycloneDDS and FastRTPS rmw. Signed-off-by: Michael Orlov <[email protected]> * Add test coverage for default rmv serialization format converter Signed-off-by: Michael Orlov <[email protected]> * Run test_serialization_converter for each rmw implementation - Rationale: To make sure that the default serialization converter can serialize and deserialize messages with all supported rmw implementations. Since it uses rmw specific functions for serialization and deserialization inside. Signed-off-by: Michael Orlov <[email protected]> * Address uncrustify formating warnings Signed-off-by: Michael Orlov <[email protected]> * Enable sanitizer by default Signed-off-by: Michael Orlov <[email protected]> * Address Windows build warnings Signed-off-by: Michael Orlov <[email protected]> * Revert "Enable sanitizer by default" This reverts commit 7241963. Signed-off-by: Michael Orlov <[email protected]> --------- Signed-off-by: Michael Orlov <[email protected]> (cherry picked from commit 6e82f52) # Conflicts: # rosbag2_cpp/src/rosbag2_cpp/converter.cpp
* Bugfix for rosbag2 serialization converter - Use rmw specific type support for rmw_serilize{deserialize} function calls. Note: It is ok for CycloneDDS to use introspection type support for rmw_serilize{deserialize} functions. However, for FastRTPS it must be rmw specific type support. e.g. rosidl_typesupport_cpp. Fix works for both CycloneDDS and FastRTPS rmw. Signed-off-by: Michael Orlov <[email protected]> * Add test coverage for default rmv serialization format converter Signed-off-by: Michael Orlov <[email protected]> * Run test_serialization_converter for each rmw implementation - Rationale: To make sure that the default serialization converter can serialize and deserialize messages with all supported rmw implementations. Since it uses rmw specific functions for serialization and deserialization inside. Signed-off-by: Michael Orlov <[email protected]> * Address uncrustify formating warnings Signed-off-by: Michael Orlov <[email protected]> * Enable sanitizer by default Signed-off-by: Michael Orlov <[email protected]> * Address Windows build warnings Signed-off-by: Michael Orlov <[email protected]> * Revert "Enable sanitizer by default" This reverts commit 7241963. Signed-off-by: Michael Orlov <[email protected]> --------- Signed-off-by: Michael Orlov <[email protected]> (cherry picked from commit 6e82f52) Co-authored-by: Michael Orlov <[email protected]>
#1823) * Bugfix for rosbag2_cpp serialization converter (#1814) * Bugfix for rosbag2 serialization converter - Use rmw specific type support for rmw_serilize{deserialize} function calls. Note: It is ok for CycloneDDS to use introspection type support for rmw_serilize{deserialize} functions. However, for FastRTPS it must be rmw specific type support. e.g. rosidl_typesupport_cpp. Fix works for both CycloneDDS and FastRTPS rmw. Signed-off-by: Michael Orlov <[email protected]> * Add test coverage for default rmv serialization format converter Signed-off-by: Michael Orlov <[email protected]> * Run test_serialization_converter for each rmw implementation - Rationale: To make sure that the default serialization converter can serialize and deserialize messages with all supported rmw implementations. Since it uses rmw specific functions for serialization and deserialization inside. Signed-off-by: Michael Orlov <[email protected]> * Address uncrustify formating warnings Signed-off-by: Michael Orlov <[email protected]> * Enable sanitizer by default Signed-off-by: Michael Orlov <[email protected]> * Address Windows build warnings Signed-off-by: Michael Orlov <[email protected]> * Revert "Enable sanitizer by default" This reverts commit 7241963. Signed-off-by: Michael Orlov <[email protected]> --------- Signed-off-by: Michael Orlov <[email protected]> (cherry picked from commit 6e82f52) # Conflicts: # rosbag2_cpp/src/rosbag2_cpp/converter.cpp * Address merge conflicts Signed-off-by: Michael Orlov <[email protected]> * Fix uncrustfy warning due to the different versions of uncrustify Signed-off-by: Michael Orlov <[email protected]> * Replace ament_add_gmock_test with ament_add_gmock - Rationale: The ament_add_gmock_executable(..) and ament_add_gmock_test(..) macros are not available on Iron distro. Signed-off-by: Michael Orlov <[email protected]> * Add missing dependencies from the std_msgs Signed-off-by: Michael Orlov <[email protected]> * Adjust writer_->create_topic(..) call in tests - removed TopicId field initialization since it is absent on Iron Signed-off-by: Michael Orlov <[email protected]> --------- Signed-off-by: Michael Orlov <[email protected]> Co-authored-by: Michael Orlov <[email protected]>
#1823) * Bugfix for rosbag2_cpp serialization converter (#1814) * Bugfix for rosbag2 serialization converter - Use rmw specific type support for rmw_serilize{deserialize} function calls. Note: It is ok for CycloneDDS to use introspection type support for rmw_serilize{deserialize} functions. However, for FastRTPS it must be rmw specific type support. e.g. rosidl_typesupport_cpp. Fix works for both CycloneDDS and FastRTPS rmw. Signed-off-by: Michael Orlov <[email protected]> * Add test coverage for default rmv serialization format converter Signed-off-by: Michael Orlov <[email protected]> * Run test_serialization_converter for each rmw implementation - Rationale: To make sure that the default serialization converter can serialize and deserialize messages with all supported rmw implementations. Since it uses rmw specific functions for serialization and deserialization inside. Signed-off-by: Michael Orlov <[email protected]> * Address uncrustify formating warnings Signed-off-by: Michael Orlov <[email protected]> * Enable sanitizer by default Signed-off-by: Michael Orlov <[email protected]> * Address Windows build warnings Signed-off-by: Michael Orlov <[email protected]> * Revert "Enable sanitizer by default" This reverts commit 7241963. Signed-off-by: Michael Orlov <[email protected]> --------- Signed-off-by: Michael Orlov <[email protected]> (cherry picked from commit 6e82f52) # Conflicts: # rosbag2_cpp/src/rosbag2_cpp/converter.cpp * Address merge conflicts Signed-off-by: Michael Orlov <[email protected]> * Fix uncrustfy warning due to the different versions of uncrustify Signed-off-by: Michael Orlov <[email protected]> * Replace ament_add_gmock_test with ament_add_gmock - Rationale: The ament_add_gmock_executable(..) and ament_add_gmock_test(..) macros are not available on Iron distro. Signed-off-by: Michael Orlov <[email protected]> * Add missing dependencies from the std_msgs Signed-off-by: Michael Orlov <[email protected]> * Adjust writer_->create_topic(..) call in tests - removed TopicId field initialization since it is absent on Iron Signed-off-by: Michael Orlov <[email protected]> --------- Signed-off-by: Michael Orlov <[email protected]> Co-authored-by: Michael Orlov <[email protected]> (cherry picked from commit a269cd4) # Conflicts: # rosbag2_cpp/CMakeLists.txt
…) (backport #1823) (#1824) * [iron] Bugfix for rosbag2_cpp serialization converter (backport #1814) (#1823) * Bugfix for rosbag2_cpp serialization converter (#1814) * Bugfix for rosbag2 serialization converter - Use rmw specific type support for rmw_serilize{deserialize} function calls. Note: It is ok for CycloneDDS to use introspection type support for rmw_serilize{deserialize} functions. However, for FastRTPS it must be rmw specific type support. e.g. rosidl_typesupport_cpp. Fix works for both CycloneDDS and FastRTPS rmw. Signed-off-by: Michael Orlov <[email protected]> * Add test coverage for default rmv serialization format converter Signed-off-by: Michael Orlov <[email protected]> * Run test_serialization_converter for each rmw implementation - Rationale: To make sure that the default serialization converter can serialize and deserialize messages with all supported rmw implementations. Since it uses rmw specific functions for serialization and deserialization inside. Signed-off-by: Michael Orlov <[email protected]> * Address uncrustify formating warnings Signed-off-by: Michael Orlov <[email protected]> * Enable sanitizer by default Signed-off-by: Michael Orlov <[email protected]> * Address Windows build warnings Signed-off-by: Michael Orlov <[email protected]> * Revert "Enable sanitizer by default" This reverts commit 7241963. Signed-off-by: Michael Orlov <[email protected]> --------- Signed-off-by: Michael Orlov <[email protected]> (cherry picked from commit 6e82f52) # Conflicts: # rosbag2_cpp/src/rosbag2_cpp/converter.cpp * Address merge conflicts Signed-off-by: Michael Orlov <[email protected]> * Fix uncrustfy warning due to the different versions of uncrustify Signed-off-by: Michael Orlov <[email protected]> * Replace ament_add_gmock_test with ament_add_gmock - Rationale: The ament_add_gmock_executable(..) and ament_add_gmock_test(..) macros are not available on Iron distro. Signed-off-by: Michael Orlov <[email protected]> * Add missing dependencies from the std_msgs Signed-off-by: Michael Orlov <[email protected]> * Adjust writer_->create_topic(..) call in tests - removed TopicId field initialization since it is absent on Iron Signed-off-by: Michael Orlov <[email protected]> --------- Signed-off-by: Michael Orlov <[email protected]> Co-authored-by: Michael Orlov <[email protected]> (cherry picked from commit a269cd4) # Conflicts: # rosbag2_cpp/CMakeLists.txt * Address merge conflicts after automatic backporting Signed-off-by: Michael Orlov <[email protected]> * Adjust tests for humble Signed-off-by: Michael Orlov <[email protected]> * Address linker errors on RHEL 8 - Use rcpputils::fs instead of the std::filesystem in tests. Since RHEL8 doesn't have adequate support for the std::filesystem. Signed-off-by: Michael Orlov <[email protected]> --------- Signed-off-by: Michael Orlov <[email protected]> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> Co-authored-by: Michael Orlov <[email protected]>
This PR will address default rmw-implemented serialization converter failures when deserializing messages from the CDR format with rmw_fastrtps.
This PR addresses the issue described in the Default rmw implemented serialization converter fails to deserialize messages with rmw_fastrtps #1815
RCA and fix proposal please see in the Default rmw implemented serialization converter fails to deserialize messages with rmw_fastrtps #1815 (comment)
The fix uses rmw-specific type support for rmw_serilize{deserialize} function calls instead of the introspection type support.
Note: It is ok for CycloneDDS and ConnextDDS to use introspection type support for rmw_serilize{deserialize} functions. However, for FastRTPS it must be rmw specific type support. e.g.
rosidl_typesupport_cpp
.The fix should work for CycloneDDS, ConnextDDS and FastRTPS RMWs.