ParameterEventHandler support ContentFiltering #2971
Conversation
Signed-off-by: Barry Xu <[email protected]>
|
|
||
| /// Configure which node parameter events will be received. | ||
| /** | ||
| * This function depends on middleware support for content filtering. |
There was a problem hiding this comment.
i would use rmw implementations instead of middleware. but i will leave this to you.
| * This function depends on middleware support for content filtering. | |
| * This function depends on rmw implementation support for content filtering. |
| quoted_node_names.push_back("'" + resolve_path(name) + "'"); | ||
| } | ||
|
|
||
| event_subscription_->set_content_filter(filter_expression, quoted_node_names); |
There was a problem hiding this comment.
i think this possibly throws the exception RCLError, if any error happens to set the filter expression and parameters? it should describe the doc section?
|
|
||
| event_subscription_->set_content_filter(filter_expression, quoted_node_names); | ||
|
|
||
| return event_subscription_->is_cft_enabled(); |
There was a problem hiding this comment.
i believe that this can be checked in the very 1st place in this function, and if that is false return immediately without any further processing.
There was a problem hiding this comment.
The function is_cft_enabled is not used to check whether the RMW Implementation supports CFT.
When CFT is not set, is_cft_enabled() returns false regardless of whether the RMW Implementation supports CFT or not.
Only when CFT is set and the RMW Implementation supports CFT does is_cft_enabled() return true.
| * If the nodes specified in this function is different from the nodes specified in | ||
| * add_parameter_callback(), the callback will never be called. |
There was a problem hiding this comment.
how about adding the same description in the doc section of add_parameter_callback?
| if (node_names.empty()) { | ||
| // Clear content filter | ||
| event_subscription_->set_content_filter(""); | ||
| return true; |
There was a problem hiding this comment.
this is against the doc section? even if the rmw implementation does not support cft, this still returns true here. that does not seem to be a design here?
There was a problem hiding this comment.
Here, I should call is_cft_enabled() to check whether CFT setting is still enabled.
is_cft_enabled() only indicates whether CFT is enabled in the configuration; it does not indicate whether the RMW implementation supports CFT.
If the RMW implementation doesn't support CFT, is_cft_enabled() always return false.
If the RMW implementation supports CFT, and only if CFT is setting, is_cft_enabled() can return true.
Signed-off-by: Barry Xu <[email protected]>
2 participants
Description
Fixes #2957
Is this user-facing behavior change?
New API
configure_nodes_filteraffects the behavior of the following two functions.The callback will only be called for parameter events from the specified nodes which are configured in this function.
The callback will only be called for parameter events from the specified nodes which are configured in this function and add_parameter_callback().
If the nodes specified in this function are different from the nodes specified in add_parameter_callback(), the callback will never be called.
Did you use Generative AI?
No
Additional Information
No