Declare_parameter in Dashing

Next Generation ROS
1

Hello everyone,

With Dashing, a new function declare_parameter has been introduced, cf. https://github.com/ros2/rclcpp/blob/master/rclcpp/include/rclcpp/node_interfaces/node_parameters_interface.hpp#L52 and https://github.com/ros2/rclcpp/blob/master/rclcpp/include/rclcpp/node.hpp#L371. I like this idea as it allows to detect mismatches in parameter names between nodes and launch configurations (for example due to typos).

Yet, once a parameter is declared, there seems no possibility to detect whether a value that is equal to the default value has been set explicitly or not. The function get_parameter(name, param) always returns true even if the parameter has not been set but just declared.

In the fmi_adapter package, parameters are created at runtime from a user file (a Functional Mock-up Unit, FMU). Thus, the parameter names depend on a user file which is itself passed via a mandatory parameter fmu_path. In the last days, I ported this package to Dashing and was faced with the issue that I had to declare the parameter names read from the FMU file. Unfortunately, I cannot provide meaningful default values at this stage. The FMU may contain algorithms to compute the default values own its own at a later stage. So, to decide whether the user actually provide a value for a parameter of the FMU or not, I decided to choose NaN as default value and then fall back to the FMU internal mechanism if get_parameter(name).as_double() return this NaN, cf. https://github.com/boschresearch/fmi_adapter_ros2/blob/dev/dashing/fmi_adapter/src/fmi_adapter/FMIAdapter.cpp#L582.

(I also tried the allow_undeclared flag, which did not work for me. Yet, this is also not an option for me as I would like to use the new mechanism due to the reason given in the first paragraph.)

Technically, it should be possible to distinguish whether a value for a parameter has been set or not ā€“ independent of the parameter declaration mechanism. The node_parameters_interface could provide a Boolean function to query for that. Of course, this introduces another level of complexity.

Are other developers faced with the same problem when porting their ROS 2 packages to Dashing? What do you think about the idea to introduce a Boolean function to query whether a parameter value has been set explicitly (even if it is equal to the default) or not?

Looking forward to your feedback!

3 Likes
2

There is a PARAMETER_NOT_SET type, if you just do node->declare_parameter(name) this will be the default type.

Thereā€™s also has_parameter() which you can use to conditionally declare or set, if youā€™re unsure if the parameter has been declared yet. So you could change your code to create a local data structure and provide that to the user, rather than declaring them implicitly, and let the user declare them. You could also let the user define as many as they like and then call function from you that will declare the rest automatically, using has_parameter() to know if the user has done this or not already.

The way it is implemented that would not be possible as declare is implicitly set to None if uninitialized (e.g. by using declare_parameter(name)). Weā€™d have to change the implementation to add an additional state like is_initialized to every parameter. Iā€™m not sure thatā€™s a good idea, but I could be convinced otherwise.

1 Like
3

Hi William, thank you very much for the detailed explanation. With the PARAMETER_NOT_SET type it works like a charm, cf. https://github.com/boschresearch/fmi_adapter_ros2/commit/252ac2cd871709206bb623b7d9f8d81f1894987f

4

Iā€™ve always used has_parameter to make sure a parameter value gets set when a node starts up but this now seems incompatible with declaring parameters. Would you consider having it return false if the parameter is declared, but not set? Or could we add another convenience function for this case?

5

At the moment, has_parameter is answering ā€œis it declaredā€, not is it ā€œsetā€. I can see why itā€™s confusing, perhaps the type should really be PARAMETER_TYPE_NONE or something.

Iā€™d recommend using a default value with declare parameter instead.

Thatā€™s a reasonable proposal I think, what do others think? If we do this weā€™d want a way to differentiate between ā€œdeclared and not setā€ and ā€œnot declaredā€. Might be worth an issue on rclcpp describing the proposal and motivation.

I could imagine having a is_parameter_set("foo") -> bool like method as a shortcut as well.

3 Likes
6

A function is_parameter_set("foo") -> bool would definitely be useful and likely the way to go to differentiate between ā€œdeclared and not setā€ and ā€œnot declaredā€.

7

Thanks for the quick reply!

Iā€™d recommend using a default value with declare parameter instead.

Iā€™ve come to suspect default values in many cases after many debugging sessions that ended in me discovering I hadnā€™t actually overwritten the default.

If we do this weā€™d want a way to differentiate between ā€œdeclared and not setā€ and ā€œnot declaredā€.

Itā€™s a bit unfortunate from the userā€™s point of view to have to deal with two different notions of a non-existent parameter. I personally donā€™t see much need to have to worry about checking if a parameter is not declared; whatever function Iā€™m using can just throw the ParameterNotDeclared exception. The first case is the interesting one to me.

Iā€™m sure itā€™s not possible for many reasons, but the ideal for me (as a user) would be to entirely remove the ability for rclcpp::Parameters to have a NOT_SET value.

8

From this discussion and some thoughts mulling over in my head, @wjwwood would it be possible to change the get_parameter method to be something like:

returnT get_parameter(args, args, bool declare_parameter=true) {
  if (!has_parameter(...)) {
    declare_parameter(...)
  }

  // normal
  // get_param
  // stuff
}

This way batteries included a get call is going to declare the parameter if not already? If not by default, having the default boolean not do it but have the option? The extra declaration I need to think about makes application-level code a bit less (elegant? readable? canā€™t think of the right word, but more ā€œoverhead stuffā€ to think about)

More than happy to submit a PR if thatā€™s something agreeable.

9

I suppose it is unfortunate, but you can also just always do something like get_parameter<string>("something") (pseudo code) and it will either: give you a string, throw because the type is not a string, or throw because itā€™s not declared. So if youā€™re ok with an exception for not being declared, canā€™t you just lean on the exception when itā€™s not set (and therefore cannot be coerced into a string) as well?

The idea behind it is that sometimes all values of a parameter are ā€œvalidā€ and thereā€™s no good way to make it optional as well without a second boolean ā€œis enabledā€ parameter. Having NOT_SET allows you to do this a bit more elegantly. Something like:

if is_set("foo"):
  do_a_thing_with_a_float(get("foo"))

Not saying this is a reason to have the extra type, but it is a pattern thatā€™s used a lot, especially in Python.

Thereā€™s already an option for this behavior:

I donā€™t recommend it, however. I donā€™t understand the need in most cases for using anything but a single declare parameter, as it returns the value just like get parameter does in most cases.

If you donā€™t expect the parameter to change during runtime, you can just do something like this in your main function or constructor:

auto scalar = node->declare_parameter<double>("some_scalar");
// ...
if (scalar > ...) ...
// ...
printf("some_scalar is: %s\n", std::to_string(scalar).c_str());
// etc...

And you never need to use get_parameter.

If you want to just call get_parameter everywhere you use it, thatā€™s fine too, you should just declare it during setup once. I donā€™t think thatā€™s too hard. It maps to variables in languages, you need to declare your variable before you assign it, and you need to initialize it (assign it the first time) before you read it.

10

Yes, this seems a reasonable option to use the template argument to force the type conversion. Though it doesnā€™t look like there currently is a templated version that returns the value.

Agreed this is reasonable, but again this particular usage doesnā€™t seem to exist currently in rclcpp (it requires a default value).

Taking a step back though, I trust you can sympathize if I express some frustration that in the end youā€™re suggesting my code doesnā€™t need to change except I should rewrite get_parameter as declare_parameter. If for ā€œmost casesā€ thatā€™s all thatā€™s needed, what problem is being solved here? Itā€™s still get_parameter, just now with a name with a less obvious meaning.

11

Sorry, youā€™re right, in both cases you need another step, either:

auto scalar = node->declare_parameter("foo").as_double();

Or:

double scalar;
node->declare_parameter("foo", scalar);

I think we could have a signature that returns T.

12

I will point out that there is a difference between get_parameter and declare_parameter in the case (I think) you are describing. In a world where you only have get_parameter, then you either get the value or you donā€™t, but the parameter is not ā€œvisibleā€ on the node in the latter scenario. That is, something like ros2 param list doesnā€™t show anything, and there is no external indication that a parameter is available to be set. Using declare_parameter, on the other hand, ensures that the parameter is always ā€œvisibleā€, even if itā€™s value is NOT_SET. I find that to be a highly desirable property of parameters.

2 Likes
13

I agree this is a nice property. But I donā€™t see how the new API is relevant to this. If at any point during execution I can declare a new parameter name, how is it different from making the name visible when first getting the parameter value? You can say the best practice is to declare all the names when the process starts but then you might as well say the best practice is to get all the values when it starts. And itā€™s almost certainly not going to be followed anyway because there will be libraries in other packages that will want to access parameters from the current node and will end up just declaring right before they access.

14

I realize this is covered ground and unlikely to change now so we donā€™t have to spend more time discussing it. Likely going forward I will just use and suggest others use declare_parameter as @wjwwood suggested above and not bother with the complication of declaring ahead of time.

15

Going down that road a little further just to play Devilā€™s advocateā€¦

I donā€™t recommend it, however.

Then why not just remove it?

I will point out that there is a difference between get_parameter and declare_parameter in the case (I think) you are describing. In a world where you only have get_parameter , then you either get the value or you donā€™t, but the parameter is not ā€œvisibleā€ on the node in the latter scenario. That is, something like ros2 param list doesnā€™t show anything, and there is no external indication that a parameter is available to be set. Using declare_parameter , on the other hand, ensures that the parameter is always ā€œvisibleā€, even if itā€™s value is NOT_SET .

This seems to me to be an argument to delete the existing get_parameter implementation, and rename declare_parameter ā†’ get_parameter, always available in all the tools and its accomplishing the same task of declaring and returning a value.

16

Because there are valid use cases that would need that capability. Itā€™s not recommended and the developer is required to explicitly override it which is them explicitly choosing to use that option which they should be aware of the consequences.

Implicitly declaring parameters at use time gets rid of the value of predeclaring parameters. We donā€™t have support for it right now, but weā€™d like to be able to do things like define interfaces including parameters, they could then be automatically declared by the node and then declaring new parameters would be disabled. Itā€™s also valuable to be able to audit the declared parameters separately from using them. As an example if two of us are writing code and I implicitly get and declare a param ā€˜fooā€™ as a double and then you implicitly get and declare a param ā€˜fooā€™ with integer itā€™s behavior will be dependent on runtime race conditions, and much harder to debug. Whereas itā€™s much easier to see, oh hey, two places are declaring the same parameter, thatā€™s not valid.

Itā€™s definitely a little bit more overhead to separately declare and then use the parameters. But one of the lessons weā€™ve learned and are building into ROS 2 is that being able to be knowingly correct is very important. This is along those lines of keeping the declarations separate from the usage.

1 Like
17

It used to be that ā€œmaking a parameter visibleā€ was done when you first set it. Really, declare parameter is a replacement for set parameter which also has meta data.

The reason it was introduced was to avoid what happened when you did something like this:

set_parameter("foo", 42, read_only=True)
set_parameter("foo", 43, read_only=False)

We didnā€™t want to have two different people set the same variable with different meta data. So we made an ā€œonly onceā€ activity (at least until you undeclare it) in declare parameter which lets you declare it and set meta data. For convenience you may also initialize it (set it) and get the resulting value (get the parameter), but neither of the last two things were its purpose.

I donā€™t think of declare parameter as a replacement for get or set parameter, but instead a replacement for something like this:

def declare_parameter(self, name, *, default_value=None, meta_data=None):
    if self.is_set(name):
        raise RuntimeError("parameter already declared")
    else:
        if name in self.parameter_overrides:
            default_value = self.parameter_overrides[name]
        self.set_parameter(name, default_value)
        self.set_parameter_meta_data(meta_data, ...)
    return self.get_parameter(name)

As I said, if you donā€™t want to mess with declaring parameters you can set these two settings to true (they default to false) and I think youā€™ll get mostly what you want:

But again, I personally think this will lead to more programming accidents in large systems. I could be wrong though.

I donā€™t understand why recommending you declare your parameter means you have to also not use get parameter?

Whatā€™s the problem with libraries declaring parameters before accessing them (if theyā€™re the only ones that care about the parameter)?

It certainly would have been a lot easier to implement, but the team felt we should have an easy way to get the old behavior, at least for now.

Also there arenā€™t many use cases for the other behavior (in my opinion), but in those few use cases some are quite important, like the parameter blackboard:

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

Which could be done without the help of those options, but it would be a lot harder and less clean.

What? I donā€™t see why you draw that conclusion. You can only call declare parameter onceā€¦

You should definitely still use get_parameter if you are checking to see if it has changed during runtime, or if you simply donā€™t want to pass references to the value throughout your code, or if you want to get several parameters at once.

18

My point was just that if new declarations can happen literally any time during node execution, I donā€™t see much functional difference from the alternative of implicitly declaring anytime get_parameter is called.

To summarize my thoughts, I understand the benefits of having a prior declaration to provide correctness guarantees. There are probably many instances where Iā€™ll love having the ability to provide declarations. I just donā€™t like that the default is that I have no choice but to declare every single parameter, because itā€™s forcing complexity on me instead of me choosing to make use of it. But I understand the consensus of the ROS2 team (as stated by @tfoote) is that being able to guarantee correctness is the priority at the cost of some developer overhead, so Iā€™m fine with just agreeing to disagree on the matter.

19

I tried to express this in my other post, but declare parameter does more than get parameter, it initializes the parameter (with stuff from the command-line or a default value or leaves it ā€œnot setā€) and it lets you set meta data. So regardless of when you use them (only at the beginning or anytime during run-time) they are functionally different.

If you donā€™t care about any of those uses of declare parameter, you may use just get_parameter, but you just have to flip one boolean flag when creating your nodeā€¦ Two if you want to take things from the command line too.

Thereā€™s also get_parameter_or which ā€œjust worksā€ even without those node option changes:

It will never throw and it never declares or sets the value of the parameter.

20

Sorry for the +1 post: however Iā€™d just like to emphasize that a method like this would be awesome to have!