Fix uninitialised parameters in image_publisher

[luca-della-vedova]

The image_publisher node ignored the return value of declare_parameter(), meaning that all the parameters were uninitialised.
This caused issues especially in the published rate, since it would have an undefined value and, if it happened to be < 0, it would crash straight away due to a timer being created with a duration < 0.

[SteveMacenski]

I see the return type of declare_parameter as rclcpp::ParameterValue. I'd rather if we did a declare call then a get_parameter call which returns the specific types. Another options that would be fine too is to call the declare_parameter("name", default).to_bool(); methods to make it really explicit.

I'm probably being overcautious. I did a quick survey over a few ROS2 projects and I don't see other folks doing this without calling the type explicitly or using declare then get.

[luca-della-vedova]

Hi Steve,

Thanks for the quick review!
I believe there is two versions of the declare_parameter function, one is the one you mention (returning a rclcpp::ParameterValue) and one templated and I believe the latter is the one being called, which deducts the auto return type from the initialization value passed as a default.
The implementation itself is in node_impl.hpp which, as you mention, returns the result of the templated class class .get() method.

Also the way I got the fix is by following the deprecated warnings thrown by colcon anyway (while fixing a separate issue that I will pull request when it's ready)

/Luca

[SteveMacenski]

oh awesome, I missed that. This seems all good to me then

@luca-della-vedova was this nodelet the only case of it in the metapackage or is this just the one you patched because you had a problem? I'm only asking if we should file a ticket for the rest or not. This PR is good to go as far as I'm concerned

[luca-della-vedova]

@luca-della-vedova was this nodelet the only case of it in the metapackage or is this just the one you patched because you had a problem? I'm only asking if we should file a ticket for the rest or not. This PR is good to go as far as I'm concerned

There is quite a lot of deprecated warnings all over the package, I'm slowly fixing them when I have time. I'm opening separate PRs to keep them easy to review and manageable.