Planner selector #2249

pabloinigoblasco · 2021-03-16T16:55:39Z

Basic Info

PR of the planner selector to be integrated in navigation2 main branch. With this BTNode we use a subscriber to enable the ability of selecting the current planner for the navigation.

Info
Ticket(s) this addresses	147
Primary OS tested on	(Ubuntu 20.04)
Robotic platform tested on	(ROS Rolling and Gazebo simulation turtlebot waffle)

Description of contribution in a few bullet points

Deeper discussion in issue 147, but the main features are:

reading from topics to select the planner
support for default planner (as static input port)
support to setting the name of the topic to support multiple planner selectors.

Description of documentation updates required from your changes

Documentation is planned to be added after the first revision of the PR.

Future work that may be required in bullet points

rclcpp::spin_some vs callback_groups

For Maintainers:

Check that any new parameters added are updated in navigation.ros.org
Check that any significant change is added to the migration guide
Check that any new functions have Doxygen added
Check that any new features have test coverage
Check that any new plugins is added to the plugins page

SteveMacenski · 2021-03-16T18:31:05Z

@pabloinigoblasco please copy-paste an un-modified PR template and fill it in. There's checklists and items there important for my own sanity when reviewing PRs to make sure things don't drop through the cracks and that you're prompted about things you may have forgotten to do.

Though I do understand this is just a WIP first go

SteveMacenski

LGTM from a high level, just some details to work out

nav2_behavior_tree/CMakeLists.txt

nav2_behavior_tree/include/nav2_behavior_tree/plugins/decorator/planner_selector_node.hpp

nav2_behavior_tree/plugins/decorator/planner_selector_node.cpp

nav2_behavior_tree/CMakeLists.txt

pabloinigoblasco · 2021-03-17T19:10:17Z

I updated the PR description to fit the default PR template.
Note: I am about to do a new commit that implements the review points.

pabloinigoblasco · 2021-03-17T21:14:21Z

What about this new version?

SteveMacenski

Overall LGTM, just a question for Sarthak and a test. Also the readme table update to include this new Bt node + the website update to include it I mention in above comments with an example.

nav2_behavior_tree/include/nav2_behavior_tree/plugins/action/planner_selector_node.hpp

nav2_behavior_tree/test/plugins/action/test_planner_selector_node.cpp

nav2_behavior_tree/include/nav2_behavior_tree/plugins/action/planner_selector_node.hpp

…nd main class doxygen brief

SteveMacenski

LGTM - just missing complete docs. Following up in comment about readme, but also still missing the navigation.ros.org updates I also described. But no more changes to code required. For the website docs:

Migration guide add this new BT node to the new BT node section https://navigation.ros.org/migration/Foxy.html#new-behavior-tree-nodes
Navigation plugins page add this new BT node https://navigation.ros.org/plugins/index.html
Configuration guide add page for the ports (ex https://navigation.ros.org/configuration/packages/bt-plugins/actions/ComputePathToPose.html)

nav2_behavior_tree/include/nav2_behavior_tree/plugins/action/planner_selector_node.hpp

pabloinigoblasco · 2021-03-22T18:21:15Z

LGTM - just missing complete docs. Following up in comment about readme, but also still missing the navigation.ros.org updates I also described. But no more changes to code required. For the website docs:

Migration guide add this new BT node to the new BT node section https://navigation.ros.org/migration/Foxy.html#new-behavior-tree-nodes

Navigation plugins page add this new BT node https://navigation.ros.org/plugins/index.html

Configuration guide add page for the ports (ex https://navigation.ros.org/configuration/packages/bt-plugins/actions/ComputePathToPose.html)

I am with these last tasks. However, I have a weird feeling with this PlanerSelector node about its "category":

Is it an Nav2 action plugin?
Means "ActionNode" for the nav2 stack to inherit from BTActionNode?
or at least to be an action client? If so, it looks like this PlannerSelector node is not an "Action".

This PlannerSelector is located inside plugins/action, is that correct?

For all these documentation tasks I would need to know its category.

pabloinigoblasco · 2021-03-22T21:01:49Z

LGTM - just missing complete docs. Following up in comment about readme, but also still missing the navigation.ros.org updates I also described. But no more changes to code required. For the website docs:

Migration guide add this new BT node to the new BT node section https://navigation.ros.org/migration/Foxy.html#new-behavior-tree-nodes

Navigation plugins page add this new BT node https://navigation.ros.org/plugins/index.html

Configuration guide add page for the ports (ex https://navigation.ros.org/configuration/packages/bt-plugins/actions/ComputePathToPose.html)

I did a pull request to the documentation respository:
ros-navigation/docs.nav2.org#152

SteveMacenski · 2021-03-22T23:31:05Z

Is it an Nav2 action plugin?

Yes, it belongs in the BT - Action subsection in the navigation plugins list, since its an action plugin (deriving from the action plugin type). It is a Behavior Tree Action node, it has nothing to do with ROS actions.

SteveMacenski

@naiveHobo you happy with this?

@pabloinigoblasco One more place to add it: https://github.com/ros-planning/navigation2/blob/main/nav2_behavior_tree/nav2_tree_nodes.xml for groot monitoring

SteveMacenski · 2021-03-23T00:15:17Z

@pabloinigoblasco pull in main or rebase in order to get CI building, its unhappy about some rclcpp changes a couple of weeks ago

naiveHobo

Just a small naming issue, otherwise LGTM!

nav2_behavior_tree/include/nav2_behavior_tree/plugins/action/planner_selector_node.hpp

nav2_behavior_tree/plugins/action/planner_selector_node.cpp

pabloinigoblasco · 2021-03-23T17:15:42Z

@pabloinigoblasco pull in main or rebase in order to get CI building, its unhappy about some rclcpp changes a couple of weeks ago

I have to analyze this thoroughly. I just merged the "main branch" and I am getting erros on nav2_utils and some rclcpp Exceptions and not sure why.

I am working on that.

naiveHobo · 2021-03-23T17:21:28Z

You're probably on an older version of ros2 master build, you'll have to update your local source ros2 repositories and build ros2 master again to get the latest changes. https://docs.ros.org/en/foxy/Installation/Maintaining-a-Source-Checkout.html#update-your-repositories

pabloinigoblasco · 2021-03-23T17:26:22Z

You're probably on an older version of ros2 master build, you'll have to update your local source ros2 repositories and build ros2 master again to get the latest changes. https://docs.ros.org/en/foxy/Installation/Maintaining-a-Source-Checkout.html#update-your-repositories

Thanks I will have a look to the link you say.

I was working on rolling not directly on the ros2 main branch. So, according to what you say, that should be the problem.

SteveMacenski · 2021-03-23T20:19:32Z

Sweet! This is good to go once CI passes + your documentation PR is completed (just 1 more missing thing so that the page is reachable)

SteveMacenski · 2021-03-23T20:44:46Z

@pabloinigoblasco thanks for this! The controller / goal checker should be just exactly equal to this with different names / topics!

* planner selector proposal * PR review changes: ros-navigation#2249 * minor, inline function in cpp file * Third PR for planner selector: default input_topic, additional test and main class doxygen brief * some more documentation on planner_selector * adding planner selector to nav_tree_nodes.xml * method namming convention in planner_selector

planner selector proposal

7eee852

pabloinigoblasco mentioned this pull request Mar 16, 2021

Planner selector concept: Design and implementation questions ros-navigation/docs.nav2.org#147

Closed

SteveMacenski requested changes Mar 16, 2021

View reviewed changes

pabloinigoblasco added 2 commits March 17, 2021 21:43

PR review changes: ros-navigation#2249

ff28145

minor, inline function in cpp file

8374ed1

SteveMacenski reviewed Mar 18, 2021

View reviewed changes

nav2_behavior_tree/include/nav2_behavior_tree/plugins/action/planner_selector_node.hpp Show resolved Hide resolved

nav2_behavior_tree/test/plugins/action/test_planner_selector_node.cpp Show resolved Hide resolved

naiveHobo reviewed Mar 18, 2021

View reviewed changes

nav2_behavior_tree/include/nav2_behavior_tree/plugins/action/planner_selector_node.hpp Show resolved Hide resolved

Third PR for planner selector: default input_topic, additional test a…

f27d6dd

…nd main class doxygen brief

SteveMacenski requested changes Mar 19, 2021

View reviewed changes

nav2_behavior_tree/include/nav2_behavior_tree/plugins/action/planner_selector_node.hpp Show resolved Hide resolved

some more documentation on planner_selector

ef31850

SteveMacenski approved these changes Mar 22, 2021

View reviewed changes

adding planner selector to nav_tree_nodes.xml

b170e25

SteveMacenski approved these changes Mar 23, 2021

View reviewed changes

Merge branch 'ros2_main' into feature/planner_selector

3f8abeb

naiveHobo approved these changes Mar 23, 2021

View reviewed changes

nav2_behavior_tree/include/nav2_behavior_tree/plugins/action/planner_selector_node.hpp Outdated Show resolved Hide resolved

nav2_behavior_tree/plugins/action/planner_selector_node.cpp Outdated Show resolved Hide resolved

method namming convention in planner_selector

059a939

SteveMacenski merged commit ae09b37 into ros-navigation:main Mar 23, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Planner selector #2249

Planner selector #2249

pabloinigoblasco commented Mar 16, 2021 •

edited by SteveMacenski

Loading

SteveMacenski commented Mar 16, 2021

SteveMacenski left a comment

pabloinigoblasco commented Mar 17, 2021 •

edited

Loading

pabloinigoblasco commented Mar 17, 2021

SteveMacenski left a comment •

edited

Loading

SteveMacenski left a comment •

edited

Loading

pabloinigoblasco commented Mar 22, 2021 •

edited

Loading

pabloinigoblasco commented Mar 22, 2021

SteveMacenski commented Mar 22, 2021

SteveMacenski left a comment •

edited

Loading

SteveMacenski commented Mar 23, 2021

naiveHobo left a comment

pabloinigoblasco commented Mar 23, 2021

naiveHobo commented Mar 23, 2021

pabloinigoblasco commented Mar 23, 2021 •

edited

Loading

SteveMacenski commented Mar 23, 2021

SteveMacenski commented Mar 23, 2021

Planner selector #2249

Planner selector #2249

Conversation

pabloinigoblasco commented Mar 16, 2021 • edited by SteveMacenski Loading

Basic Info

Description of contribution in a few bullet points

Description of documentation updates required from your changes

Future work that may be required in bullet points

For Maintainers:

SteveMacenski commented Mar 16, 2021

SteveMacenski left a comment

Choose a reason for hiding this comment

pabloinigoblasco commented Mar 17, 2021 • edited Loading

pabloinigoblasco commented Mar 17, 2021

SteveMacenski left a comment • edited Loading

Choose a reason for hiding this comment

SteveMacenski left a comment • edited Loading

Choose a reason for hiding this comment

pabloinigoblasco commented Mar 22, 2021 • edited Loading

pabloinigoblasco commented Mar 22, 2021

SteveMacenski commented Mar 22, 2021

SteveMacenski left a comment • edited Loading

Choose a reason for hiding this comment

SteveMacenski commented Mar 23, 2021

naiveHobo left a comment

Choose a reason for hiding this comment

pabloinigoblasco commented Mar 23, 2021

naiveHobo commented Mar 23, 2021

pabloinigoblasco commented Mar 23, 2021 • edited Loading

SteveMacenski commented Mar 23, 2021

SteveMacenski commented Mar 23, 2021

pabloinigoblasco commented Mar 16, 2021 •

edited by SteveMacenski

Loading

pabloinigoblasco commented Mar 17, 2021 •

edited

Loading

SteveMacenski left a comment •

edited

Loading

SteveMacenski left a comment •

edited

Loading

pabloinigoblasco commented Mar 22, 2021 •

edited

Loading

SteveMacenski left a comment •

edited

Loading

pabloinigoblasco commented Mar 23, 2021 •

edited

Loading