cbandera / rosparam_handler

An easy wrapper for using parameters in ROS
Other
47 stars 26 forks source link

Set default values on parameter server if no value is set #22

Closed poggenhans closed 7 years ago

poggenhans commented 7 years ago

We found it useful to set the default parameters on the rosparam server if they were not present so that the full configuration of a node is visible.

artivis commented 7 years ago

Good idea. However this could be optional, with a default behavior that does not set the parameters on the server. One may not want to flood.

Not to break the API, you could overload the function

params_.fromParamServer();
bool set_params_on_server = true;
params_.fromParamServer(set_params_on_server); // no default arg 

or else add functions

bool set_params_on_server = true;
// Specify explicitly that we want the params
// to be set on the server.
params_.setParamsOnServer(set_params_on_server);
params_.fromParamServer();

// Retrieve the current behavior,
// true -> setting on server
// false -> not setting on server
bool are_params_set_on_server = params_.setParamsOnServer();
poggenhans commented 7 years ago

Thanks for the feedback. I personally don't see why flooding the param server could be an issue. I have used this change in cases with over 1500 different parameters on the server (one node alone uses 400) without any observable performance impact.

But I'm open for discussion. If there is a good reason to not put it all on the param server, I'll look for a way to make this an option. :wink:

artivis commented 7 years ago

From my side there is no particular reason apart from personal habits and use.
But I think this is still a reason good enough form making this feature optional, whatever the default value would be :+1:

cbandera commented 7 years ago

The default branch and the contribution guidline have changed. Please pull the latest changes from develop and create a new PullRequest against the new default branch. Thank you.

cbandera commented 7 years ago

I think it is a useful feature as well. As the rosparam_handler was thought to be a wrapper around the parameter handling by ROS, I would have even expected this to be the default behaviour. @poggenhans: Please reopen this PR and add a test, following the other examples.

poggenhans commented 7 years ago

@cbandera Tests are added and I merged with develop. The joy of reopening is yours :wink:.

cbandera commented 7 years ago

@poggenhans Thank you very much. What a joy! :smiley:

cbandera commented 7 years ago

I have updated the documentation. The PR looks good now. Thanks for the contribution.