Add support for content filtered topics

[iuhilnehc-ynos]

Related to ros2/design#282 to add content-filtered-topic interfaces.

[iuhilnehc-ynos]

rebase and ready to implement CFT for rmw_fastrtps

[fujitatomoya]

@ivanpauno @wjwwood @clalancette could you review this?
CC: @MiguelCompany @asorbini

[ivanpauno]

The rmw API looks good to me.

What is going to be the behavior for DDS vendors that don't support content filtering?

[fujitatomoya]

@ivanpauno

What is going to be the behavior for DDS vendors that don't support content filtering?

NOT SUPPORT will be returned to application for now.

https://github.com/ros2/design/pull/282/files#diff-0cb9ecc495b4e57ddcb33bd070916b8cc1a2c5a812c4ec2020f93b5bb858d197R221-R225

we have been discussing internally, but not implementing the rcl filtering function yet. this is something we could do as follow-up, but we are not positive on this approach.

[iuhilnehc-ynos]

@ivanpauno @fujitatomoya

What is going to be the behavior for DDS vendors that don't support content filtering?

There are two cases(1. create a subscriber with the content filter at the beginning; 2. set content filter after creating a subscriber that can be non-content filter) to set content filtering.

In the first case, it just returns the subscriber as normal with an is_cft_enabled flag that is false.
In the second, it returns NOT SUPPORT as @fujitatomoya mentioned.

[ivanpauno]

we have been discussing internally, but not implementing the rcl filtering function yet. this is something we could do as follow-up, but we are not positive on this approach.

I'm not sure if it's a good idea to add a feature only supported by one DDS vendor.
In that case, we're not going to be able to take much advantage of the feature.

@clalancette @wjwwood any opinion?

[MiguelCompany]

I'm not sure if it's a good idea to add a feature only supported by one DDS vendor.

@ivanpauno We've been working hard to have this feature on Fast DDS. It has just been released with v2.5.1

We are changing the Fast DDS branch for rolling in ros2/ros2#1241

@iuhilnehc-ynos Has been working on the rmw_fastrtps integration but we didn't want ros2/ros2#1241 to block the review of the RMW interfaces, so that's why we decided to start with the empty stub, and the content filter implementation on rmw_fastrtps will be done on a follow-up PR.

[ivanpauno]

@iuhilnehc-ynos Has been working on the rmw_fastrtps integration but we didn't want ros2/ros2#1241 to block the review of the RMW interfaces, so that's why we decided to start with the empty stub, and the content filter implementation on rmw_fastrtps will be done on a follow-up PR.

Well that sounds way better!
It's still a bit strange to me to add a major feature not supported by all rmw implementations.
Ideally, we should have a way to still use content filtering in the implementations that don't support the feature, even if in that case there's no performance benefit.

[fujitatomoya]

@MiguelCompany thanks for the follow-up

@ivanpauno sorry i did not mention that rmw_fastrtps status. that will be supported as well. (So rmw_fastrtps and rmw_connextdds will be supported with this PR for Humble.)

Ideally, we should have a way to still use content filtering in the implementations that don't support the feature, even if in that case there's no performance benefit.

okay we just wanted to discuss with community how we wanna go for sure. if that is the way to go, we are expecting that we could integrate this filtering in rcl. (expecting that no interfaces need to be changed.) that is something we can do, and we will do that after this.

[fujitatomoya]

Or if rmw_cyclonedds has a plan to support content filtering, we are also willing to work on rmw_cyclonedds to enable that. in this case, we do not really need filtering in rcl?

[ivanpauno]

Or if rmw_cyclonedds has a plan to support content filtering, we are also willing to work on rmw_cyclonedds to enable that. in this case, we do not really need filtering in rcl?

Maybe not in that case, but it will make implementing a fully featured rmw implementation much harder.

[fujitatomoya]

okay, sounds good to us. but can we do that as follow-up, we do not think that is required to have this PR in the mainline.
Since we have been waiting for a long time to have feedback, we would like to have some time to address rcl filtering if it is required after consensus. (but we will do that!)

[ivanpauno]

That sounds good to me.
@clalancette @wjwwood opinions?

[fujitatomoya]

@iuhilnehc-ynos could you also share your thought?

[fujitatomoya]

@wjwwood can you review this?

[fujitatomoya]

@iuhilnehc-ynos

There are two cases(1. create a subscriber with the content filter at the beginning;
2. set content filter after creating a subscriber that can be non-content filter) to set content filtering.
In the first case, it just returns the subscriber as normal with an is_cft_enabled flag that is false.

precisely this is more like, user can create a subscriber with content filtering option that include expression and parameters.
i thought that this was okay to create subscriber implicitly, and application can ask if CFT is enabled.
but it would be nice to fail the subscriber creation with error code? this is more direct message that user cannot use CFT with current RMW implementation?
what would you think? @ivanpauno @wjwwood (sorry in today's call, i mentioned this incorrectly.)

In the second, it returns NOT SUPPORT as @fujitatomoya mentioned.

this seems to be no problem.

[ivanpauno]

I have done a more detailed review now, I still have to take a look at the tests

[iuhilnehc-ynos]

Rebased the 5 repositories, then re-run CI based on #302 (comment)

• Linux
• Linux-aarch64
• Windows

Updated (linked error on windows, fixed in rmw_fastrtps):

• Windows

[fujitatomoya]

windows CI unstable, https://ci.ros2.org/job/ci_windows/16730/, this is not related to CFT RMW fix.

@ivanpauno @wjwwood
CC: @iuhilnehc-ynos

could you help us to merge the following PRs for RMW CFT? we do not have access permission for cyclone and connextdds.

• Add support for content filtered topics #302 (this PR)
• add content-filtered-topic interfaces rmw_implementation#181
• add stub for content filtered topic rmw_connextdds#77 (currently stub)
• Add content filter topic feature rmw_fastrtps#513
• Add content filter topic feature empty stub rmw_cyclonedds#289

rmw_connextdds is currently stub to push the interfaces, but the following can be pushed afterwards. (CC: @asorbini )
we tried to fix the problem, but for now we ended up having stub to get the interfaces at 1st.

• Add support for user-specified content filters rmw_connextdds#68 (CFT enabled, but some minor errors detected.)

[ivanpauno]

LGTM!

[fujitatomoya]

CI with ros2/rmw_connextdds#68

• Linux
• Linux-aarch64
• Windows

[fujitatomoya]

CI again with ros2/rmw_connextdds#68 (see ros2/rmw_connextdds#68 (comment))

• Linux
• Linux-aarch64
• Windows

[asorbini]

I started another Windows build since I should have now resolved the build issues:

[fujitatomoya]

CI is green, i will merge this into mainline.

• Add support for content filtered topics #302 (comment)
• Add support for content filtered topics #302 (comment)

[ivanpauno]

@fujitatomoya are the other PRs ready as well?

e.g. ros2/rmw_connextdds#68 has some conflicts

[ivanpauno]

@fujitatomoya are the other PRs ready as well?

e.g. ros2/rmw_connextdds#68 has some conflicts

Sorry, I see you merged ros2/rmw_connextdds#77 instead.

[fujitatomoya]

@ivanpauno we still have some unstable problems for ros2/rmw_connextdds#68, so we pushed stub version ros2/rmw_connextdds#77 to push the interfaces in the master.

[fujitatomoya]

yeah, i will check the unstable reasons but i am not sure if we can fix it in today. (CC: @asorbini )