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

Allow ROS namespaces for planning request adapters: pass a NodeHandle for their initialization #1530

Merged

Conversation

henningkayser
Copy link
Member

This PR adds support for running multiple planning pipelines with different configurations at the same time. The planning including planning request adapters from separate namespaces. While the configuration of the planner plugin can already be scoped by passing a child namespace to the planning pipeline, the parameters of the planning request adapters are always queried from the private node handle from within the constructor. The parameter lookup is now moved to a new virtual initialize() function (empty by default) which expects the node handle to use. The planning pipeline calls this function on all constructed planning request adapters.

@henningkayser henningkayser force-pushed the pr-multiple_planning_pipelines branch from 272c3cb to e868ded Compare July 1, 2019 22:16
@rhaschke rhaschke changed the title Support running multiple planning pipeline configurations Allow ROS namespaces for planning request adapters: pass a NodeHandle for their initialization Jul 2, 2019
Copy link
Contributor

@rhaschke rhaschke left a comment

Choose a reason for hiding this comment

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

From the title, I was expecting a much larger contribution, namely supporting multiple planning pipelines (and planners) in the move_group node. To clarify the contribution of this PR, I changed the title.

There are more adapters in folder moveit_ros/planning/planning_request_adapter_plugins/src that need to be adapted to the new API.

Copy link
Contributor

@rhaschke rhaschke left a comment

Choose a reason for hiding this comment

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

Looks better already. Adaptation of remaining default adapters is still missing 😉

virtual void initialize(const ros::NodeHandle& node_handle)
{
ROS_WARN_NAMED("planning_request_adapter",
"Function initialize() is not implemented. All parameters should be"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"Function initialize() is not implemented. All parameters should be"
"Implementation of function initialize() is missing, thus preventing to load parameters from the passed NodeHandle / namespace."

Copy link
Member Author

Choose a reason for hiding this comment

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

Isn't this a bit confusing since not all adapters load parameters?

@henningkayser
Copy link
Member Author

From the title, I was expecting a much larger contribution, namely supporting multiple planning pipelines (and planners) in the move_group node. To clarify the contribution of this PR, I changed the title.

I'm actually looking into this, but I think it's a first good step to be able to actually run different planning pipelines without using the MoveGroup node.

There are more adapters in folder moveit_ros/planning/planning_request_adapter_plugins/src that need to be adapted to the new API.

You mean by adding an empty initialize() implementation?

Co-Authored-By: Robert Haschke <rhaschke@users.noreply.github.com>
@rhaschke
Copy link
Contributor

rhaschke commented Jul 2, 2019

There are more adapters in folder moveit_ros/planning/planning_request_adapter_plugins/src that need to be adapted to the new API.

You mean by adding an empty initialize() implementation?

If these adapters don't use parameters: yes. The best way to validate completeness of your API change within the code base is to make initialize() abstract.

@gavanderhoorn
Copy link
Contributor

From the title, I was expecting a much larger contribution, namely supporting multiple planning pipelines (and planners) in the move_group node. To clarify the contribution of this PR, I changed the title.

I'm actually looking into this, but I think it's a first good step to be able to actually run different planning pipelines without using the MoveGroup node.

@henningkayser: can you say anything about the approach you're taking / looking into?

@henningkayser
Copy link
Member Author

henningkayser commented Jul 2, 2019

@gavanderhoorn I didn't implement anything yet since I'm mostly accessing the core components directly. Probably the most straightforward approach would be to scope optional (or even all) planning pipeline configurations in sub-namespaces and pass the names as parameters to the MoveGroup node. Inside the MoveGroupContext we load all specified pipelines and instead of the member planning_pipeline_ we use a getter function to access the pipeline by name. The name itself should be an optional parameter inside the MotionPlanRequest to support both MoveGroupPlanService and MoveGroupAction capabilities.

Copy link
Contributor

@rhaschke rhaschke left a comment

Choose a reason for hiding this comment

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

There was still an adapter missing. Having added this one myself, I approve.

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.

None yet

4 participants