ros / dynamic_reconfigure

BSD 3-Clause "New" or "Revised" License
48 stars 111 forks source link

Add a wrapper class with locking #66

Closed AlexReimann closed 7 years ago

AlexReimann commented 7 years ago

Currently locking for the config objects needs to be done by the class using it. This is a pain in the ass if you share it between multiple objects. Writing a wrapper class outside of dynamic_reconfiguring is also problematic, as it has to be edited every time a parameter gets changed or added.

This adds an automatically generated wrapper class. It uses a boost mutex inside and makes the config parameters accessible with locking getter functions.

AlexReimann commented 7 years ago

I removed the "removing" of trailing spaces to make the PR more cleaner.

Feedback please :>

NikolausDemmel commented 7 years ago

I forgot to add that I don't have any authority here and of course thanks for the contribution!

AlexReimann commented 7 years ago

Added some docu and did some renaming.

Two points which maybe should be considered:

  1. Use normal mutex instead of recursive_mutex
  2. Use inhertiance instead of class with Config member (composition)
NikolausDemmel commented 7 years ago

Regarding inheritance: with inheritance, users get direct access to the public member variables going around the mutex, so I would have a slight preference for composition.

recursive mutex seems fine, but you could also make it a template parameter (with a default value), if you want.

NikolausDemmel commented 7 years ago

I have to say from your documentation I do not quite understand your use case? You say you want to use the same config instance in the user callback and in other classes. But the user callback gets a new object everytime anyway. How do you propose to update the "shard" copy in the user callback?

See https://github.com/ros/dynamic_reconfigure/blob/51c46e7c0acd70457889530e6744d581fe462648/include/dynamic_reconfigure/server.h#L218-L223

AlexReimann commented 7 years ago

I'm a busy today, can make an better example later this week. Kind of badly written explanation:

You have a class with a AllYourConfig config_ member and the callback function void reconfCB(AllYourConfig updated_config).
When the config gets update, e.g. from the reconfigure gui, reconfCB(updated_config) gets called from a thread outside.
So you can't do 'config_ = updatedconfig', because somewhere else you might be using `config, e.g.double velocity = config.maxVelocityand then you get access on the same objects from different threads (reconfigure CB thread and your node thread). So you have to add locking arounddouble velocity = config.maxVelocityandconfig_ = updatedconfig. Now I'm actually using thisconfig` not only in that class but also in other classes, which partly have their own threads (think path planner having a controller). So I need to add locking there also. And that gets complicated..

progtologist commented 7 years ago

@AlexReimann I am not entirely sure that this will not cause dead locks, as there is already a recursive mutex in the dynamic reconfigure server and the locking config can be used to change the values of the reconfiguration (causing a double lock maybe?). Some unit testing would be extremely helpful in getting this accepted (also, I don't have any authority here, just pointing some hints).

AlexReimann commented 7 years ago

Going to abandon this.. lock your callbacks guys!