Read only parameters

[sloretz]

This adds Node::create_parameter() and a boolean allow_undeclared_parameters to the node's constructor. Parameters may be given a type, a default value, and a bool indicating whether or not they're read only. Read only parameters are initialized, but may not be changed after initialization.

The features work except for a test failure in test_time_source.cpp, and I have no idea why. The failure symptom is the on_parameter_event callback in TimeSource does not get called.

The time_source test passes if I step through the problem area in GDB. That is, I set a breakpoint at the start of the failing test then set a break point inside TimeSource::on_parameter_event. If I hit 'c' for continue the test fails and the breakpoint is not reached. If I instead hit n repeatedly then the breakpoint is reached in the spin() call and the test passes.

There are a couple missing features. Since this PR adds the concept of declaring parameters, it should be possible for a node to error if a run-time user tries to set an undeclared parameter. This could be done with a method that throws if any undeclared parameters where provided, but a node-author would be responsible for calling this method after all parameters have been declared.

Other nodes may allow undeclared parameters (like the observation source parameters in obstacle layer in costmap 2d). If they pass allow_undeclared_parameters = true to the constructor then the initial parameters provided by a run-time user are immediately set on a node. A future enhancement could let a node say parameters starting with foo.bar. may be undeclared.

requires #488

[dejanpan]

@sloretz @mikaelarguedas could we do anything to get this picked up again and merged?

@anup-pem

[wjwwood]

@dejanpan I think everyone is busy with other tasks right now, so we don't have time to address it immediately. However, it may get done along with other parameter related work going on right now, but it would towards the end of that work (plan is to be done towards the end of the month). Otherwise I'll try to pick it up in the meantime if I can.

Looks like there are some missing features and an unresolved test failure (just based on @sloretz's post above).

[wjwwood]

Cross referencing these notes we made a while ago:

• https://gist.github.com/wjwwood/ea46f7cb1f1bbe218cd35889a8784029
• https://gist.github.com/sloretz/d30623a26d79ae50d05a82ece6ea51c1

[wjwwood]

I think I got this rebased correctly (I'll be building on this now at least), but some of the merges were not obvious due to recent changes and since it was based on #488 but not rebased on it before it was merged to master.

[dejanpan]

@wjwwood: should @anup-pem and I be reviewing this PR?

[wjwwood]

No, it's not ready for review yet (I will put the in review label and comment when that happens).

[wjwwood]

@sloretz could I trouble you to amend and then force push your commits on this branch, adding a sign-off while you do it? If you'd rather not then I can merge this with the DCO check failing.

[wjwwood]

Also, this pr and connected pr's are ready for review. I will have follow up pr's after it is merged.

[sloretz]

@sloretz could I trouble you to amend and then force push your commits on this branch, adding a sign-off while you do it?

I added signed-off-by to those commits and pushed to branch read_only_parameters_shane_dco. Mind double checking that I did it right? If it looks ok then this branch can be reset to that one.

[wjwwood]

It lgtm @sloretz, thanks.

[clalancette]

Initial review. I'm mostly concerned with the API (and how we extend it to the future), along with the change of not allowing undeclared parameters by default. The rest are smaller issues.

[clalancette]

I'm wondering if we want to make this more generic than just a boolean for read_only. For instance, how would we declare a parameter that is read-only, must be in range 0.0 - 1.0, and that the type must be a double? I'm not suggesting that we need to implement all of that in this PR, but I'd like to understand how the API could allow for things like that.

[wjwwood]

It will need to either evolve to include more arguments or a structure that defines the meta data for the parameter. I don't know which is better right now, so that's why I didn't change it, and presumably why @sloretz started it this way.

[clalancette]

I'm mostly worried about having to change the API later to add those additional features; if we don't do it now, then it will be an API break later and will be harder to do. Personally, it sounds like there are enough additional features we'd want to add that it would warrant adding a structure here. Thoughts?

[wjwwood]

I thought about this at the time, and I couldn't come up with how constraints might work (like must be in this range, or one of these choices, etc.) which put me off from deciding they should all live in a single struct, but I guess no matter what we come up with we can fit it in an options struct or "description"/"descriptor" struct, since that's the matching IDL equivalent (ParameterDescriptor.msg).

[wjwwood]

@sloretz I can't figure out what GitHub's issue is with displaying this correctly. It looks different locally with git-log. Also, DCO is still failing...

I think I'm going to rebuild it by cherry-picking each commit, one at a time.

[wjwwood]

CI:

• Linux
• Linux-aarch64
• macOS
• Windows

[jacobperron]

Latest changes look okay to me.

[jacobperron]

LGTM, one minor comment.

[nuclearsandwich]

• Linux
• Linux-aarch64
• macOS
• Windows (test_cli timeout, is a know flake)

[wjwwood]

Ok, merged 🎉!

I'll follow up by:

• adding documentation to the migration guide for dashing (Parameter changes for dashing ros2_documentation#187)
• making an issue to track the documentation/testing of list_parameters (document and test rclcpp::Node::list_parameters #721)
• making an issue to track the need to address callback concerns (only one right now, probably need more than one at a time) (address how parameter callbacks work #722)
• making an issue to track the need to fix parameter addressing and how to support sub-node properly (fixup parameters and sub-nodes #731)
• making an issue to track rclpy support (Enhance parameters API - declarations and descriptors rclpy#321)

[wjwwood]

Another TODO:

• update https://index.ros.org/doc/ros2/Tutorials/Node-arguments/#parameters

[wjwwood]

Documentation and tutorial updated in ros2/ros2_documentation#187