Consider renaming `pre_set`, `on_set` and `post_set_parameters_callback`. about rclcpp HOT 4 CLOSED

• The name on_set_parameters_callback suggests that this is when the parameters are set, or should be set, while in fact the internal state should not be altered,

While I don't disagree, we've already renamed this callback 3 times. And since this is the oldest of the callbacks, it has the most users. So I'm not super inclined to change this one yet again.

• But in the example code, the post_set callback is used to update the node parameters.

No, that example code does not change the node parameters. It changes internal class state, which is a different thing. That is, there is a (private) database of node parameters that rclcpp tracks, and by the time that post_set is called, that database has been updated. The purpose of the post_set is to give the user the chance to make internal changes based on that, which is what the example shows. Maybe we should update the name of the member fields in the example code, and/or update the documentation?

from rclcpp.

So I'm not super inclined to change

That's why I used the wording "I propose to consider". ;-)

this one yet again.

I would rename all, or none.
Or maybe add the new names as an alias and also keep the current ones?
Anyway if it is deemed not important enough than that's fine as well.

No, that example code does not change the node parameters. It changes internal class state, which is a different thing.

Ok, I totally missed that the internal_tracked_class_parameter_X_ variables are of type double.
I assumed that this->declare_parameter() would return a rclcpp::Parameter, similar to this->get_parameter().
So I was expecting rather:

internal_tracked_class_parameter_1_ = this->declare_parameter("param1", 0.0).as_double();
[...]
internal_tracked_class_parameter_1_ = this->get_parameter("param1").as_double();

from rclcpp.

Ok, I totally missed that the internal_tracked_class_parameter_X_ variables are of type double.
I assumed that this->declare_parameter() would return a rclcpp::Parameter, similar to this->get_parameter().

It has a bunch of overloads, some of which return an rclcpp::ParameterValue and some of which return the type given based on template automagic; see

That said, I'm not sure whether we have examples showing all of those overloads (though we do seem to have tests for it).

Given what you've said so far, I think for now what we should do is to update the demos to make it clear that those are internal class variables, and possibly update the documentation to better describe what is going on here. @jrutgeer would you be willing to take a shot at doing those?

from rclcpp.

@clalancette Can you check this proposal ros2/demos#651? See also the open questions in the comment.

from rclcpp.