Dynamic params redesign #256
Conversation
| for (auto & client : parameters_clients_) { | ||
| auto sub = client->on_parameter_event(callback); | ||
| event_subscriptions_.push_back(sub); | ||
| user_callback_ = callback; |
There was a problem hiding this comment.
A comment about how to properly use the init_callback feature would be helpful.
| } | ||
| return false; | ||
| } | ||
|
|
There was a problem hiding this comment.
I'd like to see some comments about these member variables. In general, there could be more comments to help the reader of this code.
There was a problem hiding this comment.
Good suggestion, I added comments to the class now.
| @@ -32,29 +33,29 @@ int main(int argc, char ** argv) | |||
|
|
|||
| // Add Dynamic Reconfigure Client | |||
There was a problem hiding this comment.
I recommend changing the "Add parameters to server" comment to "Set parameters on the node". This avoids a potential misunderstanding about a "parameter server"
| @@ -120,9 +120,9 @@ void StaticLayer::onInitialize() | |||
| } | |||
|
|
|||
| dynamic_param_client_ = new nav2_dynamic_params::DynamicParamsClient(node_); | |||
| dynamic_param_client_->add_parameters({"enabled_static_layer"}); | |||
There was a problem hiding this comment.
It would be nice to have an add_parameters variant that takes a string, so we can drop the initializer list in the case of a single parameter.
There was a problem hiding this comment.
Added the variant in latest commit. When overloading with a string argument, I was getting that the call was ambiguous (assuming users call function like add_parameters("enabled_static_layer") and add_parameters({"foo", "bar"})) so I ended up passing a character array instead.
| @@ -64,22 +65,53 @@ int main(int argc, char ** argv) | |||
| param_validator->add_param("foo", rclcpp::ParameterType::PARAMETER_DOUBLE); | |||
| param_validator->add_param("bar", rclcpp::ParameterType::PARAMETER_INTEGER, {0, 10}); | |||
|
|
|||
| // Create a custom validation callback | |||
| std::function<rcl_interfaces::msg::SetParametersResult( | |||
There was a problem hiding this comment.
Can you do something to make this line easier to read? It took me at least a minute or two to parse this statement.
Use some using directives to shorten the type names so you can break the line at better points
There was a problem hiding this comment.
In latest commit, I added the using directives
| const std::vector<rclcpp::Parameter> &)> custom_validation_callback = [node]( | ||
| const std::vector<rclcpp::Parameter> & parameters) | ||
| -> rcl_interfaces::msg::SetParametersResult | ||
| { |
There was a problem hiding this comment.
Long functions like this a better as named functions, not lambdas.
There was a problem hiding this comment.
Right. I made them functions outside of main
… linters (except xmllint) & uncrustify
remove extra test node
bpwilcox
force-pushed
the
dynamic_params-redesign
branch
from
52d3bb3 to
61ce69d
Compare
This PR addresses some issues for redesign mentioned in the README of PR #196.
In particular, this PR modifies and adds the following features:
add_parameters()