design icon indicating copy to clipboard operation
design copied to clipboard

ContentFilteredTopic Design Proposal.

Open fujitatomoya opened this issue 4 years ago • 69 comments

ROS 2 ContentFilteredTopic Feature Support.

  • adding content-filtered-topic interfaces for Humble API freeze
  • follow-ups for Humble Release
  • Fallback Filtering after Humble Release
  • /parameter_events & action feedback topics.
  • rclpy support

Signed-off-by: Tomoya.Fujita [email protected]

fujitatomoya avatar May 01 '20 05:05 fujitatomoya

@wjwwood @ivanpauno CC: @eboasson

would you give us some feedback what/how you feel about this feature extension? pointing you, because i usually have communication with you these layers, such as rmw/rcl/rclxxx.

also anyone, any idea and comments will do good.

fujitatomoya avatar May 01 '20 05:05 fujitatomoya

This pull request has been mentioned on ROS Discourse. There might be relevant details there:

https://discourse.ros.org/t/content-filtering-topic/13889/1

ros-discourse avatar May 01 '20 05:05 ros-discourse

This pull request has been mentioned on ROS Discourse. There might be relevant details there:

https://discourse.ros.org/t/ros-2-real-time-working-group-online-meeting-16-apr-29-2020-meeting-minutes/13999/4

ros-discourse avatar May 09 '20 08:05 ros-discourse

@wjwwood @dirk-thomas

friendly ping, but not urgent.

fujitatomoya avatar May 19 '20 01:05 fujitatomoya

Someone will take a look at this but likely only after the Foxy release is out.

dirk-thomas avatar May 21 '20 17:05 dirk-thomas

@fujitatomoya I see you have been working on draft PRs, but bare in mind that this has high chances of being rejected (specially, because how DDS specific is this feature). I would really wait for the design approval before having a full implementation.

Sorry for the delay in reviewing.

ivanpauno avatar Jul 23 '20 21:07 ivanpauno

@ivanpauno

really appreciate for checking on this and advice that is helpful.

I would really wait for the design approval before having a full implementation.

i was thinking that too, i would discuss more how we could make approach on this before implementation.

fujitatomoya avatar Jul 27 '20 01:07 fujitatomoya

Adding an optional feature sounds fine to me, but you're proposing making use of cft in some built-in use cases (parameter events and actions).

It does not have to be only for built-in use cases, but user classes also. that is what we want to discuss.

P.S.: parameter events and multi-client actions are a great example of where using cft is a performance win.

I believe so. Something we'd like to know from the others is is there any approach that we can solve the problems defined in this draft?. I mean, if there is and it is better, we will do that to support the requirement instead of cft.

How will parameter events work in the case the rmw implementation doesn't support content filtering? i.e. How will filtering happen in that case?

i guess that we have options,

  1. filtering will be done in subscription side. (not rmw implementation, but rclcpp/rclpy instead)
  2. return NOT_SUPPORT if rmw implementation does not support cft.

thinking about good user experience for it just works out-of-the-box, option-1 can be an actual choice. we can support this as well.

If we add this, I'm not sure if we should support the content filtering expressions supported in the DDS standard, or a custom designed one for ROS2 (with a way to convert between the two).

as far as i see, DDS standard expressions are really flexible so it would be nice to keep the standard.

fujitatomoya avatar Jul 27 '20 01:07 fujitatomoya

return NOT_SUPPORT if rmw implementation does not support cft.

This is tempting, but it would have the side-effect of preventing a non-DDS-based RMW being used for a node that decides it wants content filtering. I feel that whether or not to content-filter a topic should either be decidable by the system integrator outside of the node's implementation, or by the node implementer and done in the node's implementation (not in the RMW layer) if we want to remain completely portable.

I think that if we want content filtering, we would have to provide a generic interface for doing it that can internally switch between using RMW-provided content filtering when available or doing it at a higher layer when not available in the RMW layer. Without that generic support also being provided I think adding access to it in the RMW layer is risky.

gbiggs avatar Jul 27 '20 08:07 gbiggs

@gbiggs

thanks for sharing you thoughts! i do agree on that.

fujitatomoya avatar Jul 27 '20 11:07 fujitatomoya

  1. filtering will be done in subscription side. (not rmw implementation, but rclcpp/rclpy instead)
  2. return NOT_SUPPORT if rmw implementation does not support cft.

Yeah, in the case content filtered topics are added, I think that having a default implementation of subscription side filtering for when the rmw implementation don't support the feature is a good idea.

We will need full support of dds cft expressions somewhere.

but rclcpp/rclpy instead

It could be implemented in rcl instead.

ivanpauno avatar Jul 27 '20 14:07 ivanpauno

It could be implemented in rcl instead.

i was thinking about that, it will be better.

fujitatomoya avatar Jul 28 '20 00:07 fujitatomoya

This pull request has been mentioned on ROS Discourse. There might be relevant details there:

https://discourse.ros.org/t/rmw-proposal-content-filtered-topic-suport/16113/1

ros-discourse avatar Aug 27 '20 01:08 ros-discourse

@wjwwood friendly ping

ivanpauno avatar Aug 27 '20 14:08 ivanpauno

we are doing some test about efficiency of ContentFitleredTopic based on RTI Connext with scenarios.

  • Confirm filtering happens either on writer or reader side. (Network transmission if that happens or not.)

note,

  • DDS specification does not specify ContentFilteredTopic implementation, so it is up to implementation.
  • To support different DDS implementation, reader must be responsible to filter the message at the end.
  • filtering depends on how many actual messages need to be delivered to reader. that said, if # of receiver is more than 80%, it would be better to do the filtering process in the reader side. and vice-versa.

fujitatomoya avatar Oct 08 '20 04:10 fujitatomoya

This pull request has been mentioned on ROS Discourse. There might be relevant details there:

https://discourse.ros.org/t/cyclonedds-roadmap-multi-network-async-content-filtering-ql1-et-al/17561/5

ros-discourse avatar Nov 26 '20 05:11 ros-discourse

To add content-filtered-topic interfaces and an empty stub implementation for rmw_connext,rmw_fastrtps,rmw_cyclonedds

iuhilnehc-ynos avatar Feb 25 '21 02:02 iuhilnehc-ynos

@wjwwood @ivanpauno @clalancette CC: @iuhilnehc-ynos

friendly ping, ContentFiteredTopic 1st step is out here: https://github.com/ros2/design/pull/282#issuecomment-785522547

i know you are busy and there are other stuff to review in the queue. but we really appreciate if you have some time to take a look and give us feedback 😄 thanks in advance.

fujitatomoya avatar Feb 25 '21 09:02 fujitatomoya

Note for myself,

  • enabling ContentFilteredTopic, we need https://github.com/rticommunity/rmw_connextdds to be merged before this.
  • ideally, we need to follow-up with examples for https://github.com/ros2/demos/tree/master/demo_nodes_cpp.

fujitatomoya avatar Mar 11 '21 08:03 fujitatomoya

This pull request has been mentioned on ROS Discourse. There might be relevant details there:

https://discourse.ros.org/t/rmw-proposal-content-filtered-topic-suport/16113/7

ros-discourse avatar May 14 '21 03:05 ros-discourse

I'd like to suggest renaming the two new RMW functions used to access and set the content-filter expression and parameters to be slightly more appropriate.

Quoting myself from a comment in ros2/rmw_connextdds#68:

I feel like [get|set]_cft_expression_parameters() is a bit of a misnomer, since "parameters" are kind of an "advanced use case" and in many cases users might be interested in getting and setting only the expression.

I think something like [get|set]_content_filter() (without "expression" or "parameter") would be more appropriate. So, we could rename the functions to:

  • rmw_subscription_get_content_filter
  • rmw_subscription_set_content_filter

Does that sound reasonable?

asorbini avatar Oct 26 '21 05:10 asorbini

@asorbini it does for me.

fujitatomoya avatar Oct 26 '21 05:10 fujitatomoya

@iuhilnehc-ynos do you happen to have a repo file for CFT development?

fujitatomoya avatar Dec 08 '21 21:12 fujitatomoya

@iuhilnehc-ynos do you happen to have a repo file for CFT development?

@iuhilnehc-ynos friendly ping!

fujitatomoya avatar Dec 17 '21 16:12 fujitatomoya

@fujitatomoya

Sorry, I missed the https://github.com/ros2/design/pull/282#issuecomment-989205246. I need to re-check it. Some repositories might be rebased. (After that, I'll provide it in gist.github.com)

iuhilnehc-ynos avatar Dec 18 '21 04:12 iuhilnehc-ynos

@fujitatomoya

Please refer to https://raw.githubusercontent.com/iuhilnehc-ynos/ros2/topic-debug-connextdds-cft/ros2.repos And I think we need to update the https://github.com/ros2/design/pull/282#issue-610568380

Remove ros2/rmw_connext Update rticommunity/rmw_connextdds from https://github.com/rticommunity/rmw_connextdds/pull/12 to https://github.com/ros2/rmw_connextdds/pull/68

iuhilnehc-ynos avatar Dec 18 '21 15:12 iuhilnehc-ynos

@iuhilnehc-ynos thanks 👍

fujitatomoya avatar Dec 18 '21 17:12 fujitatomoya

@iuhilnehc-ynos

which one is official fix for CFT? https://github.com/ros2/rmw_connextdds/tree/asorbini/cft or https://github.com/ros2/rmw_connextdds/pull/12? could you confirm and check?

fujitatomoya avatar Jan 25 '22 00:01 fujitatomoya

@fujitatomoya

Please use https://github.com/ros2/rmw_connextdds/tree/asorbini/cft created in the https://github.com/ros2/rmw_connextdds/pull/68 by @asorbini , I'll close https://github.com/ros2/rmw_connextdds/pull/12.

iuhilnehc-ynos avatar Jan 25 '22 01:01 iuhilnehc-ynos

@ros2/team could you have some time to review this and shar comments?

fujitatomoya avatar Feb 02 '22 04:02 fujitatomoya