-
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 1 commit
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 |
|---|---|---|
|
|
@@ -251,6 +251,30 @@ class ParameterEventHandler | |
| ParameterCallbackType callback, | ||
| const std::string & node_name = ""); | ||
|
|
||
| /// Configure which node parameter events will be received. | ||
| /** | ||
| * This function depends on middleware support for content filtering. | ||
| * If middleware doesn't support contentfilter, return false. | ||
| * | ||
| * If node_names is empty, the configured node filter will be cleared. | ||
| * | ||
| * If this function return true, only parameter events from the specified node will be received. | ||
| * It affects the behavior of the following two functions. | ||
| * - add_parameter_event_callback() | ||
| * The callback will only be called for parameter events from the specified nodes which are | ||
| * configured in this function. | ||
| * - add_parameter_callback() | ||
| * 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 is different from the nodes specified in | ||
| * add_parameter_callback(), the callback will never be called. | ||
|
Comment on lines
+273
to
+274
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. how about adding the same description in the doc section of add_parameter_callback?
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. Okay |
||
| * | ||
| * \param[in] node_names Node names to filter parameter events from. | ||
| * \returns true if configuring was successfully applied, false otherwise. | ||
| */ | ||
| RCLCPP_PUBLIC | ||
| bool configure_nodes_filter(const std::vector<std::string> & node_names); | ||
|
|
||
| /// Remove a parameter callback registered with add_parameter_callback. | ||
| /** | ||
| * The parameter name and node name are inspected from the callback handle. The callback handle | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -74,6 +74,35 @@ 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(""); | ||
| 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.
i would use
rmw implementationsinstead ofmiddleware. but i will leave this to you.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.
Updated