search

ros / dynamic_reconfigure

BSD 3-Clause "New" or "Revised" License
47 stars 112 forks source link

Added C++ Client class #63

Closed progtologist closed 7 years ago

progtologist commented 8 years ago

Added templated dynamic_reconfigure::Client for C++ users as requested in #4 .

progtologist commented 7 years ago

The unit test that fails is insignificant, it's just because the test is sleeping for a fixed amount of time to wait for the reconfiguration to happen and that amount is not enough for the build server!

jacquelinekay commented 7 years ago

Regarding the failing test, perhaps you could extend the hardcoded sleep time, retry the test if failed for a fixed number of iterations, or (complicated but awesome) add an option to the API to asynchronously notify when reconfiguration is complete.

progtologist commented 7 years ago

I wouldn't want to over-complicate the API, doing that might discourage potential users. If necessary, I could do something more complicated to make sure the tests pass on all occasions, but I wouldn't want to add "features" to the Client just to make tests easier (that's the wrong way of testing).

mikaelarguedas commented 7 years ago

Thanks @progtologist for the contribution! Sorry for the tremendous delay I'll review this asap

v4hn commented 7 years ago

@mikaelarguedas ping. Did this fall of your todo list? :) It would be great to see this merged and released soon! A c++ client makes dynamic reconfigure so much nicer to use in practice!

mikaelarguedas commented 7 years ago

unfortunately it did get lost in my everyday growing todo list. I'm planning on getting this in for the lunar release and will finish reviewing it in the next few days / week. It looks good and I'm not expecting much change to get this merged. Agreed that a c++ client would be great!

I wouldn't want to add "features" to the Client just to make tests easier (that's the wrong way of testing).

Agreed

sorry all for the delay

v4hn commented 7 years ago

Thanks a lot!

On Thu, Mar 02, 2017 at 12:56:51AM -0800, Mikael Arguedas wrote:

I'm planning on getting this in for the lunar release

Please consider adding this to kinetic (and maybe even indigo) too. This is a new submodule, so there shouldn't be any problems with releasing it into existing ROS distros.

mikaelarguedas commented 7 years ago

Yes I'm not planning on branching out, and if I end up doing so this will be backported and will land in every active rosdistro

mikaelarguedas commented 7 years ago

@progtologist Looks good to me. Thanks for contributing this! I made a few cosmetic changes and modified the tests a bit. I couldn't push on your branch, would you mind having a look at https://github.com/ros/dynamic_reconfigure/compare/ReconfigureClientCpp and comment + cherry-pick accordingly ? Thanks!

progtologist commented 7 years ago

@mikaelarguedas They all look good, thanks for the corrections! You may squash them all and merge!

mikaelarguedas commented 7 years ago

Sounds good, closing in favor of #78 then