pal-robotics / ddynamic_reconfigure

89 stars 77 forks source link

Fix problem when loading ros parameters in registerVariable #9

Open francisc0garcia opened 4 years ago

francisc0garcia commented 4 years ago

Method registerVariable (with callbacks) reads the corresponding ros parameter, but it can not return the updated value [current_value] after calling the method, because it isn't a reference. Now it is a reference.

Signed-off-by: Francisco Garcia Rosas francisco.garcia.rosas@fkie.fraunhofer.de

v-lopez commented 4 years ago

Hi @francisc0garcia I apologize for not looking at this sooner.

The idea with the callback version of the functions is that you modify the variable in the callback.

veimox commented 3 years ago

@v-lopez I think like @francisc0garcia that is a bug. The user, as per the documentation, expects that if the value is changed on the user domain the variable is updated.

I could understand that the user might also expect that, if the variable if changed either from outside or inside, the callback will be called. This could be handled either by :

  1. refactoring the library to use the reactive paradigm (e.g. using observable, boost::signals or RxCpp (checkout this benchmark ) or
  2. state in the documentation that it wont call the callback but that the value will be reflected on the /parameters_update topic.

Personally I rather use option 2 as option 1 might lead to circular updates and introduce a new paradigm that the user might not feel comfortable with. If so, then the documentation should express that the callback will only be called when the parameter is updated from the outside only and not from the inside.

veimox commented 3 years ago

Another way that i can think of would be to extend the API and add a new type CallbackPointerRegisteredParam that would have a different signature than CallbackRegisteredParam to avoid confusions. That way we keep backwards compatibility and we achieve our desired behavior. I have made a proposal in #19

v-lopez commented 3 years ago

I believe I did not understand the original issue from @francisc0garcia.

The issue is that if you modify the variable inside your application, the change is not reflected on the dynamic_reconfigure API.

The registerVariable with a callback is working as intended, because the variable you provide is just an initial value, it may be that the variable itself doesn't exist.

But I agree that there is a use case where you do have a variable that you update in your code, and can be updated externally, and also you want to be notified in a callback if it's updated.

In hindsight, I should have added an optional argument to the default RegisterVariable that is a function to be called on update.

I'm trying to find the best way of adding this without breaking API/ABI.

veimox commented 3 years ago

For me both #19 or #20 work, what are your thoughts?