-
Notifications
You must be signed in to change notification settings - Fork 193
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
base: gh-pages
Are you sure you want to change the base?
ContentFilteredTopic Design Proposal. #282
Conversation
@wjwwood @ivanpauno would you give us some feedback what/how you feel about this feature extension? also anyone, any idea and comments will do good. |
This pull request has been mentioned on ROS Discourse. There might be relevant details there: |
This pull request has been mentioned on ROS Discourse. There might be relevant details there: |
friendly ping, but not urgent. |
Someone will take a look at this but likely only after the Foxy release is out. |
There was a problem hiding this 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.
@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). Sorry for the delay in reviewing. |
really appreciate for checking on this and advice that is helpful.
i was thinking that too, i would discuss more how we could make approach on this before implementation. |
It does not have to be only for built-in use cases, but user classes also. that is what we want to discuss.
I believe so. Something we'd like to know from the others is
i guess that we have options,
thinking about good user experience for
as far as i see, DDS standard expressions are really flexible so it would be nice to keep the standard. |
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. |
thanks for sharing you thoughts! i do agree on that. |
Yeah, in the case content filtered topics are added, I think that having a default implementation of subscription side filtering for when the We will need full support of dds cft expressions somewhere.
It could be implemented in |
i was thinking about that, it will be better. |
bd35300
to
d11a267
Compare
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 |
@wjwwood friendly ping |
we are doing some test about efficiency of ContentFitleredTopic based on RTI Connext with scenarios.
note,
|
cd8ae77
to
1aa3f40
Compare
This pull request has been mentioned on ROS Discourse. There might be relevant details there: |
49e9adf
to
b3a22c0
Compare
What message field do you need access to?
It would be fine IMO to implement it using CPP and provide a C interface. |
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 the top of my head, how about using environment variable such as |
We might be able to use 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? |
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. |
will set up the meeting. |
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
Compiler Option in rclcpp
Fallback function for Humble
Future Discussion / Note
|
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. |
This is not necessarily true. |
if user wants to have serialized message with CFT via |
In that case, yes. |
@iuhilnehc-ynos @Barry-Xu-2018 i think we have misunderstanding for we will enable CFT interfaces for i might miss something, let me know if i got anything wrong. |
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? |
@fujitatomoya Fast DDS supports the following extensions from Annex B:
|
The supported syntax is described in the docs, but to provide some "highlights" of the extensions/limitations:
|
@MiguelCompany @asorbini thanks for the information. |
@iuhilnehc-ynos i updated the header with current status and action items, #282 (comment).
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. |
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 |
Signed-off-by: Tomoya.Fujita <Tomoya.Fujita@sony.com>
@wjwwood @ivanpauno Just FYI, all tasks are listed on #282 (comment). |
@wjwwood @ivanpauno I just made ros2/rcl#982 for |
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 |
ROS 2 ContentFilteredTopic Feature Support.
/parameter_events
& actionfeedback
topics.Signed-off-by: Tomoya.Fujita Tomoya.Fujita@sony.com