Pass initial parameter values to node constructor #486
Conversation
sloretz
force-pushed
the
init_params_via_constructor
branch
from
9182182
to
ec0500b
Compare
|
CI with 28ad77b
|
sloretz
added
in review
| @@ -50,6 +51,15 @@ NodeParameters::NodeParameters( | |||
| rmw_qos_profile_parameter_events, | |||
| use_intra_process, | |||
| allocator); | |||
|
|
|||
| // TODO(sloretz) store initial values and use them when a parameter is created | |||
There was a problem hiding this comment.
can you explain this TODO a bit more for my understanding please? The impression I get is that one day we want the node to store a list of possible initial parameter values, but not actually set those parameters until a different step?
What we are doing here -- which at the moment is essentially passing in a list of initial parameters -- is not the end goal? Why not?
I have seen in the parent ticket that this is "Needed to initialize a node with parameters generated by roslaunch after it merges multiple yaml files together." but it wasn't enough for me to answer my own question
There was a problem hiding this comment.
The impression I get is that one day we want the node to store a list of possible initial parameter values, but not actually set those parameters until a different step?
Yeah, the TODO says what should happen here when (#475) is implemented. The reason for having a different step is to allow a node author to define parameters, including things like a read_only parameter that can be initialized but not modified. The parameter shouldn't be set until a node-author finishes defining parameters so that it's possible to error when a run-time user has a typo in their configuration.
What we are doing here -- which at the moment is essentially passing in a list of initial parameters -- is not the end goal? Why not?
Passing in parameters via constructor will still exist, but setting them right away instead of storing them is the back-up plan. If the whole feature doesn't get in by bouncy release then this at least allows run-time users to initialize parameters via the command line.
There was a problem hiding this comment.
OK, 475 seems to be the missing piece in the explanation puzzle, thanks for explaining
There was a problem hiding this comment.
Referenced issue in TODO comment f30c64c
| { | ||
| rclcpp::init(0, NULL); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Nit: I know other tests don't all do it but it seems cleaner to have a tear down calling rclcpp::shutdown rather then just exiting the process
There was a problem hiding this comment.
Added call to shutdown() in 04cb6a9
rclcpp/include/rclcpp/node.hpp
Outdated
| @@ -96,6 +96,7 @@ class Node : public std::enable_shared_from_this<Node> | |||
| const std::string & namespace_, | |||
| rclcpp::Context::SharedPtr context, | |||
| const std::vector<std::string> & arguments, | |||
| const std::vector<Parameter> & initial_values, | |||
There was a problem hiding this comment.
In my opinion, as a user, initial_values is not descriptive enough; I think we're relying on the type to convey extra information. When I first looked at this PR I thought "initial values for what?" (and then as you saw, I wondered why it wasn't just called "initial parameters", especially since it sets the names as well). Were there other names that you considered?
There was a problem hiding this comment.
Were there other names that you considered?
There were not
changed to initial_paramters plus added missing doxygen \param[in] initial_parameters in 29046af
|
CI as of ad83f5a
|
PR complete, waiting for #481 to be merged first, then this needs to be targeted atmaster.This allows the initial parameter values for a node to be passed in via it's constructor. These parameters are then immediately set on the node.
edit: invoked CI wrong, will wait to run until #481 is in.blocks #477
requires #481