Skip to content
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

ContentFilteredTopic Design Proposal. #282

Open
wants to merge 17 commits into
base: gh-pages
Choose a base branch
from

Conversation

fujitatomoya
Copy link
Collaborator

@fujitatomoya fujitatomoya commented May 1, 2020

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
Copy link
Collaborator Author

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

@ros-discourse
Copy link

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
Copy link

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

@fujitatomoya
Copy link
Collaborator Author

@wjwwood @dirk-thomas

friendly ping, but not urgent.

@dirk-thomas
Copy link
Member

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

Copy link
Member

@ivanpauno ivanpauno left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not going to say that content filters aren't cool and have a lot of good use cases, but if we add support to them non-DDS based rmw implementations will become really difficult.

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

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?

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

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

articles/content_filtering.md Outdated Show resolved Hide resolved
articles/content_filtering.md Outdated Show resolved Hide resolved
articles/content_filtering.md Outdated Show resolved Hide resolved
@ivanpauno
Copy link
Member

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

@fujitatomoya
Copy link
Collaborator Author

@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
Copy link
Collaborator Author

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.

@gbiggs
Copy link
Member

gbiggs commented Jul 27, 2020

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.

@fujitatomoya
Copy link
Collaborator Author

fujitatomoya commented Jul 27, 2020

@gbiggs

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

@ivanpauno
Copy link
Member

  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.

@fujitatomoya
Copy link
Collaborator Author

It could be implemented in rcl instead.

i was thinking about that, it will be better.

@fujitatomoya fujitatomoya force-pushed the topic-20200501-content-filtering-draft branch from bd35300 to d11a267 Compare August 18, 2020 02:27
@ros-discourse
Copy link

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

@ivanpauno
Copy link
Member

@wjwwood friendly ping

@fujitatomoya
Copy link
Collaborator Author

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 fujitatomoya force-pushed the topic-20200501-content-filtering-draft branch from cd8ae77 to 1aa3f40 Compare October 21, 2020 05:21
@fujitatomoya fujitatomoya changed the title [Draft] Content Filtering Proposal. ContentFilteredTopic Design Proposal. Nov 2, 2020
@ros-discourse
Copy link

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

@ivanpauno
Copy link
Member

integrate everything into rcl only. but this does not seems to be reasonable since it cannot access the field for rclcpp(typesupport_cpp)

What message field do you need access to?
It's not clear to me why that's needed (I still need to take a deeper look to the fastrtps PR)

And, sqlite3 like parser and tokenizer needs to be implemented with C, this will be huge code maintenance. Any suggested approach? (expecting no external API changes.)

It would be fine IMO to implement it using CPP and provide a C interface.
I don't know what other thinks though.

@fujitatomoya
Copy link
Collaborator Author

What message field do you need access to?
It's not clear to me why that's needed (I still need to take a deeper look to the fastrtps PR)

based on the filtering expression and parameters, we need to check the content of each message, and then we can do filtering if this particular message should be dropped or not. these message types are user defined messages.
(off topic, Fast-DDS current Reader-Side CFT is much better performance than rcl filtering since it does not need to de-serialize the message before filtering. This is achieved by TypeObject, and i think RTI Connext DDS also has this advantage with TypeCode.)

It would be fine IMO to implement it using CPP and provide a C interface.

👍

Compile option to opt-in CFT in client would be the idea

off the top of my head, how about using environment variable such as ROS_DISABLE_LOANED_MESSAGES?
by default, CFT is disabled atm, it will return NOT SUPPORT, so that user cannot create subscription with CFT option nor set/get filtering expression and parameters.
user can explicitly define environmental variable to enable CFT?
of course, documentation / tutorial will be provided, how to enable / which implementation supports.

@wjwwood
Copy link
Member

wjwwood commented Mar 15, 2022

based on the filtering expression and parameters, we need to check the content of each message, and then we can do filtering if this particular message should be dropped or not. these message types are user defined messages.
(off topic, Fast-DDS current Reader-Side CFT is much better performance than rcl filtering since it does not need to de-serialize the message before filtering. This is achieved by TypeObject, and i think RTI Connext DDS also has this advantage with TypeCode.)

We might be able to use rosidl_typesupport_introspection_c{pp} for this. We will have to have a filter implementation for each rosidl type we support, right now being C and CPP. We could use just the C typesupport for the purposes of evaluating the filter, and then deliver the messages that pass in CPP... but that would require extra conversions and stuff.

The alternative (which isn't necessarily mutually exclusive) is to implement the fallback filters in each client library. This would be the most maintenance, but with shared code perhaps the least work. It will be the most performant as well because it would not necessarily rely on runtime type introspection (e.g. rosidl_typesupport_introspection* or DDS's TypeCode API) and could even be done at compile time in C++, in some cases.

Maybe we should have a dedicated meeting for this?

@wjwwood
Copy link
Member

wjwwood commented Mar 15, 2022

off the top of my head, how about using environment variable such as ROS_DISABLE_LOANED_MESSAGES?
by default, CFT is disabled atm, it will return NOT SUPPORT, so that user cannot create subscription with CFT option nor set/get filtering expression and parameters.
user can explicitly define environmental variable to enable CFT?
of course, documentation / tutorial will be provided, how to enable / which implementation supports.

The idea behind this is actually more like a compiler definition, so if you wanted to use this feature you'd have to add a compiler definition to your package to enable the feature for rclcpp. That was people wondering why they need to do this are prompted to read about it, and so that pull requests using the feature are more readily identified so we don't have it slip in prematurely to other core packages. But that was just an idea someone in the ROS 2 meeting proposed. We can discuss it further.

@fujitatomoya
Copy link
Collaborator Author

Maybe we should have a dedicated meeting for this?

will set up the meeting.

@fujitatomoya
Copy link
Collaborator Author

breaking-out discussion for CFT feature.

date: 03/17/2022 09:00 - 10:00 PDT

@ivanpauno @wjwwood @MiguelCompany @asorbini if i am mistaken, please let me know!

RMW CFT Interface for Humble

  • conclusion, we will take current interfaces.
  • discussion starts from Add support for content filtered topics rmw#302 (comment)
  • separating normal subscription from CFT subscription would be easier for maintenance.
  • having CFT as Reader QoS would be fine, creating another entity is complicated.
  • (not a justification but) besides, probably no time to adjust interfaces to create CFT subscription interfaces from now.
  • any performance degression for discovery or normal subscription with CFT enhancement with empty filtering and expression? No, it would be no impact, just calling empty interfaces would be all.

Compiler Option in rclcpp

  • this is notification for user application, that you are using CFT.
  • compiler definition to enable CFT in rclcpp, which is always ON for core libraries built.
  • user must add definition to build the application with CFT feature. (document also needs to be provided.)
  • having template or definition in header files to adjust the interfaces w/ or w/o CFT.
  • details need to be discussed.

Fallback function for Humble

  • Sony to consider best proposal, and discuss in MW WG meeting or breakout meeting.
  • we seem to have 2 options where to implement, either client rclcpp or rcl. (ContentFilteredTopic Design Proposal. #282 (comment), ContentFilteredTopic Design Proposal. #282 (comment) option 2 and 3 since 1 is not really feasible.)
  • internal interfaces can be changed, but not allowed to change data structure and type for user interfaces for ABI/API contract.
  • if ABI/API changes are required to support fallback implementation, these changes must be ready for Humble. (Mon. April 4, 2022 - API Freeze)
  • rclcpp efficiency w/o runtime introspection type support, but maintenance for each client library. (for example, when we support rclpy.) this would not be a right place to integrate the feature as design aspect, but we already have an exception such as intra-process communication.
  • rcl maintenance cost effective, support all client libraries. runtime introspection type support c/cpp required. performance could be worse, especially for taking serialized message with filter. (de-serialize the message for the filter and user application will receive the serialized message and de-serialize again...)
  • there could be difference about filtering expression and parameter between rmw implementation and fallback filter? we need to have clear strict specification in ROS 2 to keep the consistency about filtering expression and parameters between rmw CFT and fallback filtering.

Future Discussion / Note

  • with DDS CFT reader side, the message will not go in HistoryCache. filtering process happens before HistoryCache w/o de-serializing the message.
  • in case of fallback function, the message will be in HistoryCache w/o filtering if implementation itself does not support CFT.
  • this behavior is really different between fallback and DDS CFT.
  • to keep the consistency about this behavior, we could have a new interface provides user filter callback to determine the messages should be in HistoryCache?
  • this would be really complicated to rmw implementation, because DDS does not provide the interfaces to register the function for filtering.
  • even with fallback filtering, providing the same behavior not to wake up executer when message is filtered, this will be good performance for user application. (image that we need to take one message out of 1000 messages.)

@ivanpauno
Copy link
Member

internal interfaces can be changed, but not allowed to change data structure and type for user interfaces for ABI/API contract.

In rcl we're using the pimpl pattern almost everywhere, so if we're going to implement the fallback in rcl and the API is in place before the freeze it should be possible to backport the feature.

@MiguelCompany
Copy link

  • performance could be worse, especially for taking serialized message with filter. (de-serialize the message for the filter and user application will receive the serialized message and de-serialize again...)

This is not necessarily true. rcl_take will call rmw_take, which will desrialize the message on the user supplied destination. It will then proceed with the filtering, and then return if the filter passes or call rmw_take again and repeat if the filter does not pass.

@fujitatomoya
Copy link
Collaborator Author

This is not necessarily true. rcl_take will call rmw_take, which will desrialize the message on the user supplied destination. It will then proceed with the filtering, and then return if the filter passes or call rmw_take again and repeat if the filter does not pass.

if user wants to have serialized message with CFT via rmw_take_serialized_message, unnecessary de-serialization would be required?

@MiguelCompany
Copy link

if user wants to have serialized message with CFT via rmw_take_serialized_message, unnecessary de-serialization would be required?

In that case, yes.

@fujitatomoya
Copy link
Collaborator Author

@iuhilnehc-ynos @Barry-Xu-2018 i think we have misunderstanding for

we will enable CFT interfaces for rcl and rclcpp for Humble Release, Mon. April 4, 2022 - Freeze with Compile Option and Necessary Interface extension for fallback filtering.

i might miss something, let me know if i got anything wrong.

@fujitatomoya
Copy link
Collaborator Author

@MiguelCompany @asorbini

Do you have any implementation specific filtering expression and parameters for ContentFitleredTopic? Or as implementation spec, DDS v1.4 Annex B - Syntax for Queries and Filters just applies?

@MiguelCompany
Copy link

@fujitatomoya Fast DDS supports the following extensions from Annex B:

  • Single dimension array / sequence indexing with [<index>], where <index> is a decimal positive value.
  • Boolean constants FALSE, false, TRUE, true.
  • Operator !=, which is an alias of <>.
  • Operator like supports both _ and ? as the single character wildcard.
  • Operator like supports both % and * as the zero or more characters wildcard.
  • Operator match which supports std::regex expression matching.

@asorbini
Copy link

Do you have any implementation specific filtering expression and parameters for ContentFitleredTopic? Or as implementation spec, DDS v1.4 Annex B - Syntax for Queries and Filters just applies?

The supported syntax is described in the docs, but to provide some "highlights" of the extensions/limitations:

  • Access to individual members of arrays and sequences is supported via [<index>] operator.
  • A hex() function is provided to compare multiple bytes.
  • A MATCH operator is supported for comparing strings, which accepts "regular expressions" using a syntax similar to fnmatch().
  • Standard LIKE operator is not supported.
  • The "negated character set" operator (i.e. [!charlist] or [^charlist]) is not supported.

@fujitatomoya
Copy link
Collaborator Author

@MiguelCompany @asorbini thanks for the information.

@fujitatomoya
Copy link
Collaborator Author

@iuhilnehc-ynos i updated the header with current status and action items, #282 (comment).

add sample to demo_nodes_cpp

can you make demonstration code for CFT? i will be working on docs, and once demo code is ready, i would like to link it to docs.

@iuhilnehc-ynos
Copy link
Contributor

@fujitatomoya

I have created the demo for CFT. To make it simple, I just provided a simple way to set the content filter setting by using the SubscriptionOption, please refer to ros2/demos#557

@fujitatomoya
Copy link
Collaborator Author

@wjwwood @ivanpauno
CC: @iuhilnehc-ynos

Just FYI, all tasks are listed on #282 (comment).

@fujitatomoya
Copy link
Collaborator Author

@wjwwood @ivanpauno
CC: @iuhilnehc-ynos

I just made ros2/rcl#982 for Fallback filtering subscription

@ros-discourse
Copy link

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

https://discourse.ros.org/t/ros-2-tsc-meeting-minutes-2023-01-19/29423/1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.