Skip to content
This repository has been archived by the owner on Aug 3, 2020. It is now read-only.

Allow supplying NodeHandle for initParam #168

Merged
merged 2 commits into from
Feb 10, 2017

Conversation

piyushk
Copy link
Contributor

@piyushk piyushk commented Dec 4, 2016

Previously, initParam always constructs a default NodeHandle, which
works correctly as long as robot_description param and the node
calling initParam are in the same namespace. This is not true,
when initParam is called via a ros_control controller running in
the global namespace (gazebo), but robot_descriptions still need
to be namespaced. This backwards compatible change allows setting
the NodeHandle.

See ros-controls/ros_controllers#244

@piyushk
Copy link
Contributor Author

piyushk commented Dec 5, 2016

Corresponding PR in ros_controllers: ros-controls/ros_controllers#245

@bmagyar
Copy link
Contributor

bmagyar commented Dec 6, 2016

Apart from providing backward compatibility with a consistent behaviour, this PR eases namespacing robot_description. It'd be a nice step towards multi robot systems and cases where multiple parts of the description are present in separate namespaces (proprietary control software).

👍 from me

@wjwwood wjwwood modified the milestone: Jade Release Jan 5, 2017
@wjwwood
Copy link
Member

wjwwood commented Jan 6, 2017

This would cause a ABI break as-is, and I'd prefer not to do that. You could instead add a new method, something like initParamWithNodeHandle, which took the additional second argument. We could consider adding the default parameter to initParam for ROS Lunar, but I'm not sure that there's a big advantage to that. Since you need to touch code to make use of this feature anyways, adding a new method shouldn't be any worse than extending the existing method's signature.

@bmagyar
Copy link
Contributor

bmagyar commented Jan 9, 2017

👍 Good idea @wjwwood , we'd need this in Kinetic

@wjwwood
Copy link
Member

wjwwood commented Jan 9, 2017

I can forward/back port anything when preparing for release.

@piyushk
Copy link
Contributor Author

piyushk commented Jan 10, 2017

Sounds good. I'm swamped on other things at the moment, and I'll try and get back to this in a week or two.

@piyushk piyushk force-pushed the piyushk/allow_supplying_nodehandle branch from 4073f54 to ab7c9d7 Compare January 30, 2017 22:58
@piyushk
Copy link
Contributor Author

piyushk commented Jan 30, 2017

@bmagyar, @wjwwood: Apologies for the delay. I didn't see a separate branch for the Jade release, but I've made the suggested change.

@wjwwood
Copy link
Member

wjwwood commented Jan 30, 2017

Looks like CI is failing for Indigo and Jade, is that expected?

@piyushk
Copy link
Contributor Author

piyushk commented Jan 30, 2017

@wjwwood: My bad. Forgot a return statement. Checks are passing now.

@wjwwood
Copy link
Member

wjwwood commented Jan 30, 2017

No worries that's why we have CI 😄.

@clalancette / @sloretz can you one of you guys review this change too?

Copy link
Contributor

@sloretz sloretz left a comment

Choose a reason for hiding this comment

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

I think package.xml may need to be updated too

@@ -44,6 +44,7 @@
#include <tinyxml.h>
#include <boost/shared_ptr.hpp>
#include <boost/weak_ptr.hpp>
#include <ros/ros.h>
Copy link
Contributor

Choose a reason for hiding this comment

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

@wjwwood correct me if I'm wrong, but I think the package.xml needs <build_export_depend>roscpp</build_export_depend> because of this addition

Copy link
Contributor Author

Choose a reason for hiding this comment

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

build_export_depend is unsupported in package.xml format 1, which is what urdf is using. I believe the format 1 build_depend did that behavior by default.

Copy link

@mathias-luedtke mathias-luedtke Jan 31, 2017

Choose a reason for hiding this comment

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

AFAIK in package format 1 "exported build depends" have to be declared as run_depend.
Source: http://www.ros.org/reps/rep-0127.html#run-depend-multiple and http://www.ros.org/reps/rep-0140.html#removing-run-depend

@@ -58,6 +59,8 @@ class Model: public ModelInterface
bool initFile(const std::string& filename);
/// \brief Load Model given the name of a parameter on the parameter server
bool initParam(const std::string& param);
/// \brief Load Model given the name of a parameter on the parameter server using provided nodehandle
bool initParamWithNodeHandle(const std::string& param, const ros::NodeHandle& nh = ros::NodeHandle());
Copy link
Contributor

Choose a reason for hiding this comment

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

Adding this looks good to me. It makes using the library easier.

I think the specific case of ros-controls/ros_controllers#244 could be solved by using NodeHandle::resolveName("robot_description") and passing the fully qualified name into initParam()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sloretz: You're right. However, after discussion, we decided to go this route.

@piyushk piyushk force-pushed the piyushk/allow_supplying_nodehandle branch from 01f76f9 to a9e5fb8 Compare January 31, 2017 16:13
@clalancette clalancette assigned sloretz and unassigned sloretz Feb 1, 2017
@piyushk
Copy link
Contributor Author

piyushk commented Feb 9, 2017

@wjwwood: Any chance we could push this one through since it's been approved? Thanks!

@sloretz sloretz merged commit 3384a0a into ros:kinetic-devel Feb 10, 2017
@piyushk
Copy link
Contributor Author

piyushk commented Feb 10, 2017

Awesome. Thanks!

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

Successfully merging this pull request may close these issues.

None yet

5 participants