[Draft/Discussion] Recursive parameter interface

[henningkayser]

@tylerjw here is a draft interface that could combine the parameter tree as implemented in ros2_params with the new NodeParameters interface. To me the only hurdle would be how callbacks would actually be handled in the parameter tree, or if we would only allow callbacks for top-level parameters and for the leafs (= node parameters).

[henningkayser]

There could also be something like bool testValueChange(const ParameterT& new_value) which triggers the same (chained) validity callbacks only without actually changing the value. Incoming parameter changes from other nodes can be rejected if they are not valid.

[tylerjw]

That seems useful. It could also mean that subsystems inside the same node could internally "test a config change" although I'm not sure that's a pattern we want to encourage. There are better ways for a node to communicate internally.

[henningkayser]

I'd argue that a node should probably not change its own parameters internally, except for setting defaults maybe. I mean, the use case of dynamic parameters is that other nodes (e.g. user interfaces) can change the modality of the nodes subsystem.

[henningkayser]

hmm... this would only really work if the node parameters are actually declared after all these calls...

[tylerjw]

Node parameters are loaded by the launch system after execution of the node starts once it provides the set_parameter interface which is started when you spin your ros node. Is that what you are talking about?

[henningkayser]

I mean that the declare() call will declare the node parameters which will already trigger all validity callbacks which have not been initialized at this point. Ideally, the builder functions describe(), warnIf(), etc are called before the parameter is actually declared. That means, either we need to simply run these before calling declare() which doesn't look as pretty or the actual declare calls of the node parameters is deferred to a NodeConfig::declareAll() function that is called after registering all the parameters.

[tylerjw]

I really like this builder syntax, the only thing I think it is missing is a default value. Maybe there would be a default(ParameterT ) method? There should also be support for allowing parameters to be unset (no default) but waiting for some timeout for them to be set. This makes sense because the way nodes are launched the parameters are sent to the node after they are started so they are always unset and it is possible to declare a parameter before it is set and then the parameter service interface will set it.

[henningkayser]

The default value can be set with the Parameter constructor: Parameter<double> double_param {"double param", 2.0 };.
For non-trivial types this becomes a little more difficult. Having the parameter name and default value part of that builder syntax could improve readability. How about initializing all parameters with default constructors and just setting the name and possible default with declare(). Undeclared parameters would simply throw an exception if they are used.

Suggested change

	declare(double_param)
	.describe("This is a very important double parameter", "should be greater than 0 and not 7")
	.warnIf([](const double& val, std::string& message) {
	message = "Value is 7, are you sure this is a good value?";
	return val == 7;
	})
	.errorIf([](const double& val, std::string& message) {
	message = "Value must not be negative!";
	return val < 0;
	});
	declare("double_param", double_param, 2.0 /* default */)
	.describe("This is a very important double parameter", "should be greater than 0 and not 7")
	.warnIf([](const double& val, std::string& message) {
	message = "Value is 7, are you sure this is a good value?";
	return val == 7;
	})
	.errorIf([](const double& val, std::string& message) {
	message = "Value must not be negative!";
	return val < 0;
	});

[tylerjw]

I take it these are the keys of the parameters. Would it make sense to have the keys supplied as a parameter to the declare function instead of here? I'm just trying to get all the meta-data about each parameter declared in one place.

[henningkayser]

Yes, that would make sense. We just need to make sure that it's not possible to change the name and to declare the parameter a second time.

[v4hn]

Is it really worth going for the explicit parameter members instead of any datatypes collected in a map?
It's somewhat cumbersome due to the repeated naming (unless you add pre-processor magic to repeat it for you) and you can't define the datatype, name, and additional data in the same command.
I'd prefer something like

declare<int>("parameter")
   .describe("my parameter")
   .errorIf("no prime number", [](auto x){ for(int i= 2; i < x; ++i) if(x%i == 0) return true; return false; });

[henningkayser]

I thought about this as well, but to me the explicit members make the config API much more usable since it doesn't require type casting and accessing the map using (const?) keys which might also fail. Actually, the current API doesn't even prevent you from using any and a member map as long as you specify the corresponding parameter template.

[v4hn]

It's clearly two alternatives with their separate pros and cons.
There might be a middle-way using variadic templates, but even if there is, it would add a lot of complexity and expertise to get it right.

Type-casting is somewhat annoying indeed, but so is having the type declared separate from the actual declaration of everything.

One idea to move all information into one place with your current approach could be a best-practice using uniform initialization. Something like

Parameter<int> int1_param{
		name("int1_param").
		.describe("This is a very important double parameter", "should be greater than 0 and not 7")
		.warnIf([](const double& val, std::string& message) {
			message = "Value is 7, are you sure this is a good value?";
			return val == 7;
		})
		.errorIf([](const double& val, std::string& message) {
			message = "Value must not be negative!";
			return val < 0;
		}) };

That should be possible if you allow Parameter construction only from the builder-types.

[tylerjw]

This is a nice interface for getting the data. I'm assuming this will lock a mutex to make sure when you are reading it isn't also being updated. Maybe this should throw an exception if the value has not been initialized (no default and value wasn't set). Maybe there should be a read(timeout) method for the parameters with some default value set for the timeout so you can specify the timeout if you want to per-parameter.

That might not be a good idea, we need to keep the interface simple and make it easy to follow a good pattern with it.

[henningkayser]

Yeah, the read is locked by a shared mutex. I don't know if a timeout is really needed at all. The only scenario I see is if that some thread locks write access and doesn't release the mutex for a long time, which would be a user error that definitely shouldn't happen. I guess that if we really wanted to protect for that, we could use an internal timeout that throws an exception if the value has been locked for too long.

[tylerjw]

I like what this could enable. ROS2 is supposed to be more event driven than ros1 and this will enable more event driven behaviors. I also like that the values are passed in by value here so you don't have to worry about the issues around references and them being updated somewhere else at the same time.

[henningkayser]

Right? ;) ... huh I guess I missed the reference, the apply() and onChange() calls would be read-locked so using const-ref should be fine. I think it can work both ways, copy or const-ref.

[tylerjw]

@henningkayser I made some small edits here: henningkayser#1

[v4hn]

@tylerjw talked to me about this effort and I had a look. I love the ideas!
I can't claim I understand enough for a thorough review, but I added a number of comments.

[v4hn]

Shouldn't that string only be set when the method returns true?

It can have advantages to define a predicate in such a way that it can return a message, but for many cases a single associated string (as I proposed in other comments) would suffice.
If you deem it necessary to have these dynamic strings, e.g., to say "<your_specified_joint> does not exist" in the message, maybe it makes sense to allow for two interfaces, one with the additional message and one without.
I implemented something similar in MTC a while ago

[henningkayser]

Very good point! I'd say we should offer both interfaces, one with static message (like in #17 (comment)) and one for dynamic strings.

[v4hn]

Getting the details right for checks like that might be a problem because usually robot models (or other core datastructures) are not loaded yet when you declare your parameters, right?

[henningkayser]

depends... Of course this example is not meant to make any actual sense, but technically parameters are already set to their overrides when you call declare(). That means, you could first create the RobotModel from your srdf parameter and then declare the map with proper validity checks.

[v4hn]

Though usually you would want to declare all parameters in the same code block (as in the NodeConfig class you want to provide), so entwined parsing and parameter interpretation will yield clumsy code again.
I don't have a proposal on how to improve that though.

[henningkayser]

True! But would there always be only one config per node? Plugins might have their own configs with defaults already initialized. Even if this example (joint names) is really not perfect, I definitely see the use case of declaring parameters (plugin configs) based on other parameters (plugin class name).

[v4hn]

Makes sense. I guess plugins should always be forced to have their parameters in dedicated namespace then, to avoid potential clashes because of redeclared parameters?

[v4hn]

Are you sure you actually need a shared_mutex here instead of a simple one?
The current API design only locks readers while it copies the values of parameters for the method return value, right?
The concept of a shared mutex really only offers better performance with long locking times for readers and rare write updates.

[henningkayser]

Are you sure you actually need a shared_mutex here instead of a simple one?

I think it has many advantages, but a simple one might work as well.

The current API design only locks readers while it copies the values of parameters for the method return value, right?

What do you mean with "only locks readers"? The ReadLockedPtr is a scoped lock that prevents write access. The read lock is shared so that multiple callers can access the value (const ref) at the same time. The WriteLockedPtr is also a scoped lock, but it's exclusive and allows write access (non-const ref).
Then there are two extra convenience operators which allow copy-get (operator()), copy-set (operator=) which would use read/write locks internally.

The concept of a shared mutex really only offers better performance with long locking times for readers and rare write updates.

I would expect that we have many many read calls and only some (declare/update callbacks) write calls.
The shared_mutex would allow many readers at the same time which would make it more efficient than a simple one as multiple threads don't need to be synchronized for reading.

[v4hn]

I would expect that we have many many read calls

True. That is quite a difference to ROS, where we fetch parameters only rarely via API.
Assuming you want to keep parallel access to the dynamic datastructures possible, the shared read-lock makes sense.

[v4hn]

I dislike the term additional_ because there is no simple contraints yet.
Usually this kind of information is just added to the description, but if you will support turing-complete validation predicates in the API, it makes a lot of sense to let authors explain them in text as well.

[henningkayser]

LOL, I don't like it either, it's just the field name of the description message. Since that one only applies to basic parameter types, we could also just rename this field to type_specification, unit, or simply comment (or just leave it). The reason I added this is because it would be nice to add this as inline-comment when generating yaml dumps.

[v4hn]

The name makes more sense in the message though because the other modeled constraints are listed below (ranges, etc.).
I don't think you directly model these other aspects here. I guess they are modeled in the underlying rclcpp API?

[henningkayser]

correct! This field wasn't really thought through. Basically, you should be able to specify these for supported rclcpp parameters but in the context of more generic types this doesn't make a lot of sense. I'm not even sure if we really want to model the rclcpp constraints at all if they basically act the same way as the callbacks in the end.

[v4hn]

Is it really worth going for the explicit parameter members instead of any datatypes collected in a map?
It's somewhat cumbersome due to the repeated naming (unless you add pre-processor magic to repeat it for you) and you can't define the datatype, name, and additional data in the same command.
I'd prefer something like

declare<int>("parameter")
   .describe("my parameter")
   .errorIf("no prime number", [](auto x){ for(int i= 2; i < x; ++i) if(x%i == 0) return true; return false; });

[v4hn]

I love the idea to support these predicates in the API.

I would consider to add the textual description together with each predicate here, instead of describing them independently in describe. See my example below.

[henningkayser]

So.. describe() doesn't really describe the predicates, it describes the parameter itself. I agree that we should extend the API to allow setting the predicate descriptions like you suggested below.

[v4hn]

Well, the additional_constraints field is currently set through describe() and should pretty much describe the predicates on these parameters. That's what I meant. :-)

[v4hn]

What do all these key strings contain? Shouldn't these all just be lists?

[henningkayser]

Hashes so that we can identify and remove them again? At least for the on_changed_callbacks_ this would be desirable. But this definitely doesn't have to be a feature for the MVP.

[v4hn]

A much simpler approach would just use std::list then and return the iterator as a handle.
The overhead of maps for this feels unjustified.

[henningkayser]

of course!

[v4hn]

Are you convinced it is actually useful to have a warnIf interface at all? I would argue that requests to set parameters either have to be valid or they are invalid. Allowing warnings here might support cases where the user knows better and sets parameters that trigger warnings (and they work). The result is a useless warning in an otherwise running system.

The other way around, if the warning actually indicates a failure, it should actually be an error anyway...

[henningkayser]

It's probably up to the user. To me it's useful to warn that some feature is disabled or if some parameter doesn't have an override even though the default is valid. Sometimes warnings are just error cases that can be handled. In the end this API would allow making warnings errors and vice-versa without having to refactor custom error handling somewhere else in the code.