Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pose controller dynamic reconfig [WIP] #120

Open
wants to merge 2 commits into
base: devel
Choose a base branch
from

Conversation

Marco-Andarcia
Copy link
Collaborator

@Marco-Andarcia Marco-Andarcia commented Nov 21, 2016

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
Copy link
Collaborator Author

@stonier I need access to yocs_msgs :/

@Marco-Andarcia
Copy link
Collaborator Author

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

Copy link
Contributor

@AlexReimann AlexReimann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just small stuffs, before merging


reconfig_server_ = boost::shared_ptr<dynamic_reconfigure::Server<yocs_msgs::PoseControllerConfig> >(
new dynamic_reconfigure::Server<yocs_msgs::PoseControllerConfig>(nh_));
reconfig_callback_func_ = boost::bind(&DiffDrivePoseControllerROS::reconfigCB, this, _1, _2);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of member variable, use a local variable.

///dynamic reconfigure server callback type
dynamic_reconfigure::Server<yocs_msgs::PoseControllerConfig>::CallbackType reconfig_callback_func_;

yocs_msgs::PoseControllerConfig test_;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Outdated, remove

@@ -248,4 +254,22 @@ void DiffDrivePoseControllerROS::disableCB(const std_msgs::EmptyConstPtr msg)
}
}

void DiffDrivePoseControllerROS::reconfigCB(yocs_msgs::PoseControllerConfig &config, uint32_t level)
{
dynamic_reconfig_mutex_.lock();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the wrong mutex, you have to lock with the same mutex as where you use the variables :/

@AlexReimann
Copy link
Contributor

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
Copy link
Collaborator Author

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

@Marco-Andarcia Marco-Andarcia changed the title Pose controller dynamic reconfig [Ready] Pose controller dynamic reconfig [WIP] Nov 30, 2016
@AlexReimann AlexReimann force-pushed the pose_controller_dynamic_reconfig branch from 89fdcdd to 68be41f Compare January 16, 2017 02:04
@AlexReimann
Copy link
Contributor

AlexReimann commented Jan 16, 2017

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
Copy link
Contributor

AlexReimann commented Jan 16, 2017

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

@AlexReimann AlexReimann force-pushed the pose_controller_dynamic_reconfig branch from 68be41f to 10cfae9 Compare January 16, 2017 03:35
@AlexReimann AlexReimann force-pushed the pose_controller_dynamic_reconfig branch from 10cfae9 to eb9ab14 Compare January 16, 2017 03:36
@AlexReimann
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants