-
Notifications
You must be signed in to change notification settings - Fork 72
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
Introducing content-filtering topics #901
Introducing content-filtering topics #901
Conversation
makes makes this feature available to rclnodejs developers. node.js - added contentFilter to Options - added static getDefaultOptions() - updated createSubscription() to support contentFilter node.d.ts - added content-filter types subscription.js - isContentFilteringEnabled() - setContentFilter() - clearContentFilter() subscription.d.ts - updated with content-filter api rcl_bindings.cpp - added content-filtering to CreateSubscription() rmw.js - new class for identifying the current ROS middleware test-subscription-content-filter.js - test cases for content-filters test/blocklist.json - added test-subscription-content-filter.js for Windows and Mac OS examples: - publisher-content-filtering-example.js - subscription-content-filtering-example.js package.json - added build/rebuild scripts for convenience
78cc8b5
to
6c63a81
Compare
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.
Thanks for submitting this huge PR and implementing the content filter feature!
lib/subscription.js
Outdated
*/ | ||
setContentFilter(contentFilter) { | ||
if (!contentFilter) return false; | ||
|
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.
Do we need to check this.isContentFilteringEnabled()
here?
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.
re: isContentFilteringEnabled()
, this will only return true if both:
- the RMW implementation support content-filtering topics (CTF) and
- *the subscription is currently configured with a valid content-filter.
So the use of isContentFilteringEnabled() is limited to identifying if a subscription currently configured with a valid content-filter. I'm not aware of an rcl level api for identifying if an RMW implementation support CTFs.
I found this bit of low level code in the fastrtps impl that implements isContentFilteringEnabled()
.
@minggangw do you think it would be more clear to developers if we renamed isContentFilteringEnabled() to something like: hasContentFilter()
?
I originally implemented without the guard condition like this:
setContentFilter(contentFilter) {
return contentFilter && contentFilter.expression
? rclnodejs.setContentFilter(this.handle, contentFilter)
: this.clearContentFilter();
}
At the rcl level, passing null or empty string for conentFilter.expression
will clear the current content-filter. I propose that we use the implementation above. Thoughts?
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.
It sounds that hasContentFilter()
is more clear.
At the rcl level, passing null or empty string for conentFilter.expression will clear the current content-filter. I propose that we use the implementation above. Thoughts?
Agree, we can remove if (!contentFilter) return false;
here, thanks
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.
LGTM! Thanks for making this big change!
* ROS Humble introduced the content-filtering topics feature. This PR makes makes this feature available to rclnodejs developers. node.js - added contentFilter to Options - added static getDefaultOptions() - updated createSubscription() to support contentFilter node.d.ts - added content-filter types subscription.js - isContentFilteringEnabled() - setContentFilter() - clearContentFilter() subscription.d.ts - updated with content-filter api rcl_bindings.cpp - added content-filtering to CreateSubscription() rmw.js - new class for identifying the current ROS middleware test-subscription-content-filter.js - test cases for content-filters test/blocklist.json - added test-subscription-content-filter.js for Windows and Mac OS examples: - publisher-content-filtering-example.js - subscription-content-filtering-example.js package.json - added build/rebuild scripts for convenience * Delete obsolete ./test.js * implements recommended PR feedback
ROS Humble introduced the content-filtering topics feature. This PR makes this feature available to rclnodejs developers.
Resources
DDS 1.4 specification, Annex B
Content-filtering tutorial
ros2/design#282
subscription.h
Supported RMWs
Content-filtering is supported by the following RMWs as of the Humble release:
Note-1: rmw_cyclonedds_cpp does NOT support content-filtering yet.
Note-2: Our experience is that content-filtering on rmw_fastrtps is only works on the Linux versions of ROS Humble and Rolling. Therefore, the test/blocklist.json excludes the test-subscription-content-filter.js test on Windows and Mac.
Key changes
node.js
node.d.ts
subscription.js
subscription.d.ts
rcl_bindings.cpp
rmw.js
test-subscription-content-filter.js
examples:
package.json