Bug: getParam returns true even if parameter was not set

[patrick-spieler-jpl]

The getParam method used to return true only when the parameter was actually set. And the current docstring still states that.

But in the ros2 branch the implementation defines a default parameter so it is always set, which breaks the previous behavior and documentation.

Instead there should be no default value set and the parameter getter should check for ParameterType::PARAMETER_NOT_SET.

[jonbinney]

This came up in the past and there was a reason for not changing it (don't remember if it was a good one). I'm traveling for the next week and a bit, but then I'll review your PR and figure out whether we can merge it.

[jonbinney]

I read through the code and some old PR comments to refresh my memory on this. The way things work now is that callers pass in a default value. If the parameter doesn't exist, then getParam() creates it with the default value and sets the output parameter to that default value. That's the current behavior, and for better or worse, it is probably relied upon by existing code in third party repos. So changing the return value would be a big change which could probably only happen between ROS distros, so it wouldn't land until the distro after kilted.

Ignoring the difficulty of changing it without unexpectedly breaking other code for a moment, we could have it return false when the parameter doesn't exist, but still create the parameter with the default value (as you do in https://github.com/ros/filters/pull/93/files). But that would be confusing. More likely we'd need a slightly more complex way of doing things.

For now what do you think of the following:

• fix the doc comment for getParam to reflect the current behavior
• add a hasParam(...) function to FilterBase so that people who don't want it automatically created can check that

We'll still have the option to change getParam later, although that would be painful. This package is mostly here to make it possible for people to port ROS1 nodes without too much work. It also puts a high value on backwards compatibility with filters already ported to ROS2 since we want to keep them working. I think there are better ways to design filters in ROS2; see the discussion here: #91 . That would probably be a new package in a new repo though.

[peci1]

I was just hit by this. If the purpose of this package is to help transition from ROS 1, then this behavior change is really a thing that doesn't help with good upgrade experience.

I suggest creating the more correct version with a different name and deprecating getParam.

[jonbinney]

@peci1 agreed that the current behavior isn't good - what you you suggest for the behavior of the new function? Never create a parameter that doesn't exist? Then we'd add another function if a user wants to create a parameter with a default value?

[peci1]

Well, ideally, the interface should be similar to that offered by NodeParametersInterface to give users the flexibility they know. A filter's interface would just always prepend the correct prefix. I don't have thought through all the consequences like param change callbacks, but I think it would be great.

[jonbinney]

That sounds reasonable at a high level, but I won't have time to work on it in the near future . If someone wants to really think through the design, I'd be open to reviewing a PR.