Fix content filtering on Windows with modern Connext DDS (backport #226)#230
Merged
Conversation
* Fix content filtering on Windows with Connext 7 1. Fix a bug in writer-side filtering where it assumed the built-in SQL filter returned a discontiguous sequence of cookies. Fallback to taking addresses of contiguous cookies if necessary. 2. Disable RMW_CONNEXT_BUILTIN_CFT_COMPATIBILITY_MODE on Windows for Connext 6.0+ to enable serialized evaluation and internal registration, which is required for correct filtering of ROS 2 samples. Assisted-by: Gemini CLI:2.0-Flash * Fix redefinition of DDS_DomainParticipant_register_contentfilterI on Windows Modern RTI Connext headers (6.0.0+) already provide this declaration. Re-declaring it manually causes linkage errors on MSVC. Assisted-by: Gemini CLI:2.0-Flash * Fix incorrect preprocessor check for legacy Connext API RMW_CONNEXT_DDS_API_PRO_LEGACY is always defined (defaults to 0), so #if defined() was always true. Switching to #if value ensures correct skipping on modern Connext. Assisted-by: Gemini CLI:2.0-Flash * Apply @fgallegosalido suggestion to assert discontiguous buffer Signed-off-by: Shane Loretz <sloretz@intrinsic.ai> --------- Signed-off-by: Shane Loretz <sloretz@intrinsic.ai> Co-authored-by: Shane Loretz <sloretz@intrinsic.ai> (cherry picked from commit 9e3beb8)
Contributor
|
CI (build: |
Contributor
|
I think Windows CI is ok. The only test failure in window CI not in the nightly is rosbag2_transport.test_record_services.gtest.missing_result. It's weird because I'm using the Lyrical repos instead of the rolling repos, but Clara did say today the test failures are new due to an infra issue. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR fixes Content Filtering on Windows when using modern versions of RTI Connext DDS (6.x and 7.x).
The Problem
Previously, the
rmw_connextddsimplementation on Windows was forced into a "Compatibility Mode." This mode:This led to failures in
rcl,rclcpp, andrcl_actiontests where samples were received by subscribers despite being explicitly filtered out by SQL expressions.The Fix
CMakeLists.txtto only enableRMW_CONNEXT_BUILTIN_CFT_COMPATIBILITY_MODEon Windows for Connext versions older than 6.0.0. For modern versions, we now use the internalregister_contentfilterIAPI, which supports evaluation on CDR-serialized samples.custom_sql_filter.hppto use the official RTI declaration ofregister_contentfilterI(available in modern headers) to avoid linkage errors on MSVC.RTI_CustomSqlFilter_writer_evaluated_resultwhere it assumed RTI's built-in filter always returned a discontiguous sequence of cookies. It now correctly handles both contiguous and discontiguous sequences.Verification
Verified on Windows using a targeted CI run with Connext 7.7.0. All content filtering tests in
rcl,rclcpp, andrcl_actionpassed.CI Result (Windows 10, Connext 7.7.0): https://ci.ros2.org/job/ci_windows/27743/
Is this user-facing behavior change?
No, it fixes a regression in existing functionality.
Did you use Generative AI?
Yes. Gemini CLI:2.0-Flash [web_fetch, run_shell_command, replace, git, gh]
This is an automatic backport of pull request #226 done by [Mergify](https://mergify.com).