search

ros2 / rmw

The ROS Middleware (rmw) Interface.
Apache License 2.0
95 stars 67 forks source link

Add support for content filtered topics #302

Closed iuhilnehc-ynos closed 2 years ago

iuhilnehc-ynos commented 3 years ago

Related to https://github.com/ros2/design/pull/282 to add content-filtered-topic interfaces.

iuhilnehc-ynos commented 2 years ago

rebase and ready to implement CFT for rmw_fastrtps

fujitatomoya commented 2 years ago

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

fujitatomoya commented 2 years ago

@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 commented 2 years ago

@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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

@iuhilnehc-ynos Has been working on the rmw_fastrtps integration but we didn't want https://github.com/ros2/ros2/pull/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 commented 2 years ago

@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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

fujitatomoya commented 2 years ago

@iuhilnehc-ynos could you also share your thought?

fujitatomoya commented 2 years ago

@wjwwood can you review this?

fujitatomoya commented 2 years ago

@iuhilnehc-ynos

There are two cases(1. create a subscriber with the content filter at the beginning;

  1. 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.

fujitatomoya commented 2 years ago

@ivanpauno @wjwwood CC: @MiguelCompany @asorbini

could you do another final review before this goes in.

and i think that the followings need to be merged at the same time, right?

@iuhilnehc-ynos i think https://github.com/ros2/rclcpp/pull/1561 and https://github.com/ros2/rcl/pull/894 can be merged later w/o any problem? just checking cz 1st we need to have all green for above RMW changes before merge with mainline.

fujitatomoya commented 2 years ago

CI only with CFT RMW Changes: (https://gist.githubusercontent.com/fujitatomoya/e591118d872be65c4c3821f7c710b12f/raw/2abba2d25836f36e6d0392024d649bd04556be56/ros2.repos)

fujitatomoya commented 2 years ago

@iuhilnehc-ynos @asorbini CI fails with rmw_connextdds, we probably want to have stub implementation for rmw_connextdds at this moment since we need to fix the APIs soon. then we can probably address unstable issues for rmw_connextdds later to enable CFT after release?

Mon. March 21, 2022 - Alpha + RMW freeze

fujitatomoya commented 2 years ago

CI: same with https://github.com/ros2/rmw/pull/302#issuecomment-1071763251 but rmw_connextdds

fujitatomoya commented 2 years ago

CI still fails, i think we need to rebase https://github.com/ros2/rmw_connextdds/tree/asorbini/cft as @iuhilnehc-ynos mentioned before https://github.com/ros2/design/pull/282#issuecomment-1066287946, and fix a bit for rcutils_string_array_t https://github.com/ros2/rmw_connextdds/pull/68#issuecomment-1071618948

fujitatomoya commented 2 years ago

CI: (using https://github.com/ros2/rmw_connextdds/pull/77 rmw_connextdds stub, just to make sure this goes green.)

repo is https://gist.githubusercontent.com/fujitatomoya/312c1308d8dcd48991be734c9c7d23f1/raw/664b83b531bc1b85faab9a042edf37329a795867/ros2.repos

iuhilnehc-ynos commented 2 years ago

Rebased the 5 repositories, then re-run CI based on https://github.com/ros2/rmw/pull/302#issuecomment-1071971744

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

fujitatomoya commented 2 years ago

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.

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.

fujitatomoya commented 2 years ago

CI with https://github.com/ros2/rmw_connextdds/pull/68

fujitatomoya commented 2 years ago

CI again with https://github.com/ros2/rmw_connextdds/pull/68 (see https://github.com/ros2/rmw_connextdds/pull/68#issuecomment-1073074721)

asorbini commented 2 years ago

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

fujitatomoya commented 2 years ago

CI is green, i will merge this into mainline.

ivanpauno commented 2 years ago

@fujitatomoya are the other PRs ready as well?

e.g. https://github.com/ros2/rmw_connextdds/pull/68 has some conflicts

ivanpauno commented 2 years ago

@fujitatomoya are the other PRs ready as well?

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

Sorry, I see you merged https://github.com/ros2/rmw_connextdds/pull/77 instead.

fujitatomoya commented 2 years ago

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

fujitatomoya commented 2 years ago

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