add a demo of content filter listener #557

iuhilnehc-ynos · 2022-04-07T07:50:28Z

Signed-off-by: Chen Lihui [email protected]

Signed-off-by: Chen Lihui <[email protected]>

iuhilnehc-ynos · 2022-04-07T07:55:53Z

As we know, the content filter has operations as below,
`RelOp ::= ‘=’ | ‘>’ | ‘>=’ | ‘<’ | ‘<=’ | ‘<>’ | like``

I expected that the keyword like can work as https://www.w3schools.com/sql/sql_ref_like.asp.
Unfortunately, it seems it doesn't work.

Is there a limitation in the DDS?

Signed-off-by: Chen Lihui <[email protected]>

MiguelCompany · 2022-04-07T08:13:23Z

@iuhilnehc-ynos On Fast DDS it should work as expected. See the test cases here

RTI Connext, according to this comment, does not support like

iuhilnehc-ynos · 2022-04-07T14:12:47Z

@MiguelCompany

RTI Connext, according to this comment, does not support like

Thanks for your information. Sorry, I didn't notice it before.

On Fast DDS it should work as expected.

After confirming, it works fine with rmw_fastrtps_cpp.

and comment that rmw_connextdds not supported currently Signed-off-by: Chen Lihui <[email protected]>

Signed-off-by: Chen Lihui <[email protected]>

ivanpauno

LGTM!

Could you add a test here?

ivanpauno · 2022-04-12T14:49:38Z

@wjwwood @clalancette any objection to merge this now?
We're past the API break, but nothing depends on the demos and it seems worthwile to have a demo of a new feature.

fujitatomoya · 2022-04-12T15:52:35Z

Once this demo is merged, i will link this demo to Humble Release Note. (see ros2/ros2_documentation#2396)

wjwwood · 2022-04-12T16:51:56Z

I don't mind it getting merged this late, following your logic, but @clalancette should give his thoughts. I didn't review the demo code yet.

fujitatomoya · 2022-04-12T17:09:48Z

CI:

Linux
Linux-aarch64
Windows

demo_nodes_cpp/src/topics/listener_content_filter.cpp

ivanpauno · 2022-04-12T18:57:54Z

@iuhilnehc-ynos On Fast DDS it should work as expected. See the test cases here

RTI Connext, according to ros2/design#282 (comment), does not support like

Could we modify the example to work with both?

Signed-off-by: Chen Lihui <[email protected]>

iuhilnehc-ynos · 2022-04-13T05:07:01Z

@ivanpauno

Could you add a test here?

Sorry, I don't know how to add a test for this listener_content_filter, it seems the current test_executables_tutorial.py.in can't work as I expect.

I expect that running with colcon test --packages-select demo_nodes_cpp --ctest-args -R test_tutorial_talker_listener_content_filter__rmw_cyclonedds_cpp should fail with the patch, but it doesn't.

What I mean is if the test in the above patch can be passed by rmw_cyclonedds_cpp, it seems there is no reason to add it.

demo_nodes_cpp/src/topics/listener_content_filter.cpp

ivanpauno · 2022-04-13T12:24:50Z

demo_nodes_cpp/src/topics/listener_content_filter.cpp

+    // 'Hello World: 10'.
+    rclcpp::SubscriptionOptions sub_options;
+    sub_options.content_filter_options.filter_expression = "data = %0";
+    sub_options.content_filter_options.expression_parameters = {"'Hello World: 10'"};


Maybe the number (currently 10) could be configurable by the user.
Maybe that makes the demo more interesting (?).
Feel free to ignore the comment though.

I actually agree with this comment. probably string chatter example is interesting enough to show this feature. how about filtering specific range of data? that would be one of the most common case?

ivanpauno · 2022-04-13T14:01:08Z

I expect that running with colcon test --packages-select demo_nodes_cpp --ctest-args -R test_tutorial_talker_listener_content_filter__rmw_cyclonedds_cpp should fail with the patch, but it doesn't.

Yes, that's not going to work because the test is waiting for the output in the file to be printed

demos/demo_nodes_cpp/test/test_executables_tutorial.py.in

Line 55 in 7a15d0b

proc_output.assertWaitFor(

, and that happens as well if no content filter is applied.
You'll need to adapt the test, or adapt the file so it doesn't match the output if "Content filter is not enabled since it's not supported" was logged before (I'm not sure if that's possible).

Please don't share a patch, they are hard to understand without being applied to the code to have more context.
It's better to push the commit and ask the question, as github gives more context (or if you don't want to push in this branch, you can do that in a new branch/fork and share the link to the github diff).

Signed-off-by: Chen Lihui <[email protected]>

fujitatomoya · 2022-04-15T05:00:26Z

@iuhilnehc-ynos all comments are addressed, right? can you confirm? so that we can ask final review to @ivanpauno @wjwwood

wjwwood

Just curious why not put this kind of thing in ros2/examples instead?

demo_nodes_cpp/src/topics/listener_content_filter.cpp

Signed-off-by: Chen Lihui <[email protected]>

ivanpauno

LGTM with @wjwwood approval

fujitatomoya · 2022-04-19T16:18:32Z

@wjwwood friendly ping.

wjwwood · 2022-04-19T19:18:03Z

demo_nodes_cpp/src/topics/listener_content_filter.cpp

+    } else {
+      RCLCPP_INFO(
+        this->get_logger(),
+        "subscribed to topic \"%s\" with filter expression \"data = 'Hello World: 10'\"",


This is really fragile, since if someone updated the expression and forgot to update this is would be wrong. Violates DRY.

Can we print out the filter expression and parameters from the content_filter_options directly instead?

wjwwood · 2022-04-19T19:19:02Z

demo_nodes_cpp/src/topics/listener_content_filter.cpp

+    // Create a subscription to the topic which can be matched with one or more compatible ROS
+    // publishers.
+    // Note that not all publishers on the same topic with the same type will be compatible:
+    // they must have compatible Quality of Service policies.


Is this comment block needed?

wjwwood · 2022-04-19T19:28:34Z

w.r.t. putting it in examples. I think having something here in demos is fine so long as we have a corresponding page about it or the executable is used in some documentation or tutorial. For example, the intra-process demos have this page which shows how to run it, what you should expect to see when you run it, and how it demonstrates that the intra-process feature is working:

https://github.com/ros2/ros2_documentation/blob/rolling/source/Tutorials/Intra-Process-Communication.rst

If we had something similar then this executable makes sense to me. Otherwise, I'd still say examples makes more sense. And I don't think you need a talker equivalent if it is put in examples, the purpose there is to show how to use the API, and it's not part of a tutorial or demo document that tells you to run it and observe some effect.

I hope that distinction makes sense.

fujitatomoya · 2022-04-19T20:58:35Z

@wjwwood @ivanpauno
CC: @iuhilnehc-ynos

i just created ros2/examples#341.

how about the following, sounds like a plan?

we could keep this demo. but this demo could be adjusted more demonstration perspective. IMO, i would use float or integer to simulate the sensing data, and filtering the specific range. this would be common use case and easy to get a picture how it works. any comments are welcome.
add ROS 2 ContentFilteredTopic for Humble Release Note. ros2_documentation#2396 can link this demonstration.
adding tutorial how to use, and that doc links to add ContentFilteredTopic example. examples#341

wjwwood · 2022-04-19T23:39:08Z

ros2/ros2_documentation#2396 can link this demonstration.
adding tutorial how to use, and that doc links to ros2/examples#341

I'd flip this around so that:

add ROS 2 ContentFilteredTopic for Humble Release Note. ros2_documentation#2396 links to the example (add ContentFilteredTopic example. examples#341)
a new how to use "tutorial" and have that doc link to this demo application (and use talker from demo_nodes_cpp as part of the "how to use")
After it is in place, add ROS 2 ContentFilteredTopic for Humble Release Note. ros2_documentation#2396 could also link to the tutorial/demo doc

We could set up a new example that uses floats or something like you said, but that would be for the demo/tutorial combo and not the minimal example.

Signed-off-by: Chen Lihui <[email protected]>

fujitatomoya · 2022-04-22T17:01:06Z

@iuhilnehc-ynos

We could set up a new example that uses floats or something like you said, but that would be for the demo/tutorial combo and not the minimal example.

can we address this? current code looks good to us, just adjusting data type for using float to simulate the situation to filtering the sensing data. for doing that, we need to add publisher as well.

how about following?

emergency temperature demonstration
- say publisher is publishing temperature data with std_msgs/msg/Float64. make a loop to publish data -100 to +150 F in order with certain offset.
- subscription use content filtering (if it is supported), and only receives data with “data < %0 OR data > %1” with {"-30.0", "100.0"}, then print emergency temperature.

i believe demonstrating with expected common use case would be much easier to get the picture of this feature for user.

Signed-off-by: Chen Lihui <[email protected]>

iuhilnehc-ynos · 2022-04-24T04:54:13Z

@ros-pull-request-builder retest this please

fujitatomoya

implementation looks good to me.

fujitatomoya · 2022-04-25T03:50:26Z

demo_nodes_cpp/CMakeLists.txt

+  src/topics/temperature_publisher.cpp
+  src/topics/temperature_subscriber.cpp


Suggested change

src/topics/temperature_publisher.cpp

src/topics/temperature_subscriber.cpp

src/topics/content_filtering_publisher.cpp

src/topics/content_filtering_subscriber.cpp

sorry i was not clear on this, i think files, classes and components can be general names for content filtering, so that user can tell these are related to content filtering.

ivanpauno

Example looks really nice!
Thanks for the work!

wjwwood · 2022-04-25T20:48:33Z

Do you guys have demo documentation to go with this? Something along the lines of this document: https://docs.ros.org/en/rolling/Tutorials/Intra-Process-Communication.html

fujitatomoya · 2022-04-25T21:20:27Z

@wjwwood i will be back with PR for that, let me have a couple of days.

wjwwood · 2022-04-25T23:40:52Z

Sure, no problem, I would just hold this until that is ready.

Signed-off-by: Chen Lihui <[email protected]>

fujitatomoya · 2022-04-27T22:45:00Z

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

i just opened ros2/ros2_documentation#2441, could you guys take a look at to give me some feedback?

fujitatomoya · 2022-04-29T00:40:38Z

either @wjwwood or @clalancette

can you merge this with ros2/ros2_documentation#2441 at the same time?

ivanpauno · 2022-04-29T13:20:16Z

can you merge this with ros2/ros2_documentation#2441 at the same time?

It seems that CI wasn't run after the last changes, running it:

Linux
Linux-aarch64
Windows

fujitatomoya · 2022-04-29T18:19:36Z

@ivanpauno thanks! all green, can you merge this?

fujitatomoya · 2022-05-02T15:45:20Z

@clalancette can you merge this? i already did ros2/ros2_documentation#2441

fujitatomoya · 2022-05-02T16:27:49Z

this is only required to backport humble package.

wjwwood · 2022-05-02T16:35:10Z

@Mergifyio backport humble

* add a demo of content filter listener Signed-off-by: Chen Lihui <[email protected]> * add warn message if content filter is not supported by DDS Signed-off-by: Chen Lihui <[email protected]> * keep `like` to make the demo simple and comment that rmw_connextdds not supported currently Signed-off-by: Chen Lihui <[email protected]> * remove the macro as it will be removed in the rclcpp Signed-off-by: Chen Lihui <[email protected]> * use = instead of like Signed-off-by: Chen Lihui <[email protected]> * address reviews Signed-off-by: Chen Lihui <[email protected]> * not make this configurable from the cli Signed-off-by: Chen Lihui <[email protected]> * remove comment and update log output Signed-off-by: Chen Lihui <[email protected]> * use temperature instead of chatter Signed-off-by: Chen Lihui <[email protected]> * rename Signed-off-by: Chen Lihui <[email protected]> (cherry picked from commit a1bc6dc)

mergify · 2022-05-02T16:35:33Z

backport humble

✅ Backports have been created

#559 add a demo of content filter listener (backport #557) has been created for branch humble

* add a demo of content filter listener Signed-off-by: Chen Lihui <[email protected]> * add warn message if content filter is not supported by DDS Signed-off-by: Chen Lihui <[email protected]> * keep `like` to make the demo simple and comment that rmw_connextdds not supported currently Signed-off-by: Chen Lihui <[email protected]> * remove the macro as it will be removed in the rclcpp Signed-off-by: Chen Lihui <[email protected]> * use = instead of like Signed-off-by: Chen Lihui <[email protected]> * address reviews Signed-off-by: Chen Lihui <[email protected]> * not make this configurable from the cli Signed-off-by: Chen Lihui <[email protected]> * remove comment and update log output Signed-off-by: Chen Lihui <[email protected]> * use temperature instead of chatter Signed-off-by: Chen Lihui <[email protected]> * rename Signed-off-by: Chen Lihui <[email protected]> (cherry picked from commit a1bc6dc) Co-authored-by: Chen Lihui <[email protected]>

add a demo of content filter listener

183f7cd

Signed-off-by: Chen Lihui <[email protected]>

add warn message if content filter is not supported by DDS

56e8580

Signed-off-by: Chen Lihui <[email protected]>

Chen Lihui added 2 commits April 7, 2022 22:24

keep like to make the demo simple

ac1c159

and comment that rmw_connextdds not supported currently Signed-off-by: Chen Lihui <[email protected]>

remove the macro as it will be removed in the rclcpp

cd4e05e

Signed-off-by: Chen Lihui <[email protected]>

iuhilnehc-ynos mentioned this pull request Apr 12, 2022

ContentFilteredTopic Design Proposal. ros2/design#282

Open

15 tasks

ivanpauno approved these changes Apr 12, 2022

View reviewed changes

fujitatomoya mentioned this pull request Apr 12, 2022

add ROS 2 ContentFilteredTopic for Humble Release Note. ros2/ros2_documentation#2396

Merged

fujitatomoya approved these changes Apr 12, 2022

View reviewed changes

wjwwood reviewed Apr 12, 2022

View reviewed changes

demo_nodes_cpp/src/topics/listener_content_filter.cpp Outdated Show resolved Hide resolved

demo_nodes_cpp/src/topics/listener_content_filter.cpp Outdated Show resolved Hide resolved

demo_nodes_cpp/src/topics/listener_content_filter.cpp Outdated Show resolved Hide resolved

use = instead of like

ce41317

Signed-off-by: Chen Lihui <[email protected]>

ivanpauno reviewed Apr 13, 2022

View reviewed changes

address reviews

7b5e21d

Signed-off-by: Chen Lihui <[email protected]>

wjwwood reviewed Apr 15, 2022

View reviewed changes

demo_nodes_cpp/src/topics/listener_content_filter.cpp Outdated Show resolved Hide resolved

not make this configurable from the cli

20f084e

Signed-off-by: Chen Lihui <[email protected]>

ivanpauno approved these changes Apr 18, 2022

View reviewed changes

wjwwood reviewed Apr 19, 2022

View reviewed changes

fujitatomoya mentioned this pull request Apr 19, 2022

add ContentFilteredTopic example. ros2/examples#341

Merged

remove comment and update log output

ef3b71a

Signed-off-by: Chen Lihui <[email protected]>

use temperature instead of chatter

58715d6

Signed-off-by: Chen Lihui <[email protected]>

fujitatomoya approved these changes Apr 25, 2022

View reviewed changes

ivanpauno approved these changes Apr 25, 2022

View reviewed changes

rename

094dbed

Signed-off-by: Chen Lihui <[email protected]>

fujitatomoya mentioned this pull request Apr 27, 2022

add tutorial for content filtering subscription. ros2/ros2_documentation#2441

Merged

wjwwood merged commit a1bc6dc into ros2:master May 2, 2022

mergify bot mentioned this pull request May 2, 2022

add a demo of content filter listener (backport #557) #559

Merged

		src/topics/temperature_publisher.cpp
		src/topics/temperature_subscriber.cpp

add a demo of content filter listener #557

add a demo of content filter listener #557

Conversation

iuhilnehc-ynos commented Apr 7, 2022

iuhilnehc-ynos commented Apr 7, 2022

MiguelCompany commented Apr 7, 2022

iuhilnehc-ynos commented Apr 7, 2022

ivanpauno left a comment

Choose a reason for hiding this comment

ivanpauno commented Apr 12, 2022

fujitatomoya commented Apr 12, 2022

wjwwood commented Apr 12, 2022 • edited Loading

fujitatomoya commented Apr 12, 2022

ivanpauno commented Apr 12, 2022

iuhilnehc-ynos commented Apr 13, 2022 • edited Loading

ivanpauno Apr 13, 2022

Choose a reason for hiding this comment

fujitatomoya Apr 13, 2022

Choose a reason for hiding this comment

ivanpauno commented Apr 13, 2022

fujitatomoya commented Apr 15, 2022

wjwwood left a comment

Choose a reason for hiding this comment

ivanpauno left a comment

Choose a reason for hiding this comment

fujitatomoya commented Apr 19, 2022

wjwwood Apr 19, 2022

Choose a reason for hiding this comment

wjwwood Apr 19, 2022

Choose a reason for hiding this comment

wjwwood commented Apr 19, 2022

fujitatomoya commented Apr 19, 2022 • edited Loading

wjwwood commented Apr 19, 2022 • edited Loading

fujitatomoya commented Apr 22, 2022

iuhilnehc-ynos commented Apr 24, 2022

fujitatomoya left a comment

Choose a reason for hiding this comment

fujitatomoya Apr 25, 2022

Choose a reason for hiding this comment

ivanpauno left a comment

Choose a reason for hiding this comment

wjwwood commented Apr 25, 2022

fujitatomoya commented Apr 25, 2022

wjwwood commented Apr 25, 2022

fujitatomoya commented Apr 27, 2022

fujitatomoya commented Apr 29, 2022

ivanpauno commented Apr 29, 2022 • edited Loading

fujitatomoya commented Apr 29, 2022

fujitatomoya commented May 2, 2022

fujitatomoya commented May 2, 2022

wjwwood commented May 2, 2022

mergify bot commented May 2, 2022

✅ Backports have been created

wjwwood commented Apr 12, 2022 •

edited

Loading

iuhilnehc-ynos commented Apr 13, 2022 •

edited

Loading

fujitatomoya commented Apr 19, 2022 •

edited

Loading

wjwwood commented Apr 19, 2022 •

edited

Loading

ivanpauno commented Apr 29, 2022 •

edited

Loading