Pose controller dynamic reconfig [WIP]

yujinrobot / yujin_ocs

Yujin Robot's open-source control libraries

http://wiki.ros.org/yujin_ocs

242 stars 181 forks source link

Pose controller dynamic reconfig [WIP] #120

Open Marco-Andarcia opened 7 years ago

Marco-Andarcia commented 7 years ago

Added dynamic reconfigure to pose controller, this pull request depends on yocs_msgs pull request.

Test Procedure:

Checkout this pr
checkout yocs pr
build
launch apps from desktop
check dynamic reconfigure for pose controller autodocking.

Marco-Andarcia commented 7 years ago

@stonier I need access to yocs_msgs :/

Marco-Andarcia commented 7 years ago

@AlexReimann Can you check this pull request? daniel asked me to ask you to merge this one.

AlexReimann commented 7 years ago

Deadlocks because calling lock here and then calling controlPose here which tries to lock again.

3 possible ways to do this without c++11:

Use boost recursive mutex which can be locked multiple times by the same thread (c++11 version
Lock and unlock each time directly around the variable for which you need the locking
Make sure you don't try to lock again in the "child" function calls (this kind of often leads to bugs, especially if someone is not aware of when / where he has to lock later)

+1 with c++11:

Use atomic variables, e. g. std::Atomic<double> max_something with which you need no locking

Marco-Andarcia commented 7 years ago

@AlexReimann Already fixed the mutex logic. removed the lock from the controlPose() and controlOrientation() functions.

AlexReimann commented 7 years ago

You missed some locking (see additional commit).

Also this foobars the parameter loaded from the params file.

The dynamic reconfigure server is instantiated after the params are loaded from the files. So it will overwrite them (when it gets instantiated it triggers a reconfigure callback with the default values of the Config file).

The way to do this properly is to have the param file parameter names the same as the dyn reconfigure Config file parameters. This way the dynamic reconfigure server will overwrite its defaults with the once from the ros parameter server (which has the values from the params file).

AlexReimann commented 7 years ago

Seems like they already have the same name, going to do some more digging. It definitely foobars something.

AlexReimann commented 7 years ago

There was a bug about w_min not being assigned properly. Checkout the commit.