-
Notifications
You must be signed in to change notification settings - Fork 481
ParameterEventHandler support ContentFiltering #2971
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
base: rolling
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -74,6 +74,38 @@ ParameterEventHandler::add_parameter_callback( | |
| return handle; | ||
| } | ||
|
|
||
| bool | ||
| ParameterEventHandler::configure_nodes_filter(const std::vector<std::string> & node_names) | ||
| { | ||
| if (node_names.empty()) { | ||
| // Clear content filter | ||
| event_subscription_->set_content_filter(""); | ||
| if (event_subscription_->is_cft_enabled()) { | ||
| return false; | ||
| } | ||
| return true; | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| } | ||
|
|
||
| std::string filter_expression; | ||
| size_t total = node_names.size(); | ||
| for (size_t i = 0; i < total; ++i) { | ||
| filter_expression += "node = %" + std::to_string(i); | ||
| if (i < total - 1) { | ||
| filter_expression += " OR "; | ||
| } | ||
| } | ||
|
|
||
| // Enclose each node name in "'". | ||
| std::vector<std::string> quoted_node_names; | ||
| for (const auto & name : node_names) { | ||
| quoted_node_names.push_back("'" + resolve_path(name) + "'"); | ||
| } | ||
|
|
||
| event_subscription_->set_content_filter(filter_expression, quoted_node_names); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes. |
||
|
|
||
| return event_subscription_->is_cft_enabled(); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The function is_cft_enabled is not used to check whether the RMW Implementation supports CFT. |
||
| } | ||
|
|
||
| void | ||
| ParameterEventHandler::remove_parameter_callback( | ||
| ParameterCallbackHandle::SharedPtr callback_handle) | ||
|
|
||
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.
how about adding the same description in the doc section of add_parameter_callback?
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.
Okay