Modify the directory structure to allow for multiple implementations … #28

mjeronimo · 2018-08-21T21:22:18Z

…of a given task type.

Also create a shell BTNavigator class and introduce namespaces.

…of a given task type. Also create a shell BTNavigator class and introduce namespaces.

mhpanah · 2018-08-22T16:05:50Z

src/nav2_tasks/include/nav2_tasks/ComputePathToPoseTaskMessages.hpp

+namespace nav2_tasks
+{
+
+using ComputePathToPoseCommand = nav2_tasks::msg::PathEndPoints;


I suggest to rename ComputePathToPoseCommand to PathToPoseCommand since this is just a name for the command and the command itself is not performing the computation.

Not sure if removing 'Compute' from the message name is better...

I've used a consistent naming for the commands <Action>Command. Where action is a verb followed by any nouns. For example: Compute, Follow, Navigate, etc. So, you end up with <Verb><NounsThatVerbOperatesOn>Command.

mhpanah · 2018-08-22T16:07:42Z

src/nav2_tasks/include/nav2_tasks/ComputePathToPoseTaskMessages.hpp

+{
+
+using ComputePathToPoseCommand = nav2_tasks::msg::PathEndPoints;
+using ComputePathToPoseResult = nav2_tasks::msg::Path;


Same with ComputePathToPoseResult. This is a msg that provides a result and it's not a function to compute a result.

mhpanah · 2018-08-22T16:15:27Z

src/nav2_navigation/bt_navigator/src/BTNavigator.cpp

+    }
+
+    // Check if the planning task has completed
+    TaskStatus status = planner_->waitForResult(path, 100);


We need to define constants.

Could we do: planner_->waitForResult(path, 100ms) ?

ghost · 2018-08-22T23:34:28Z

src/mission_execution/CMakeLists.txt

  )

-target_link_libraries(mission_execution
+target_link_libraries(mission_executor


This line is unnecessary.

ghost · 2018-08-22T23:35:14Z

src/mission_execution/CMakeLists.txt


 include_directories(
  include
  ../task/include
-  ../navigation/include
+  ../nav2_tasks/include


These are unnecessary if nav2_tasks and simple_navigator are made packages.

ghost · 2018-08-22T23:44:17Z

src/nav2_control/dwa_controller/CMakeLists.txt

@@ -29,13 +29,13 @@ add_executable(dwa_controller
  )

 target_link_libraries(dwa_controller
-  robot
+  nav2_robot


We should be able to delete this call

ghost · 2018-08-22T23:44:31Z

src/nav2_control/dwa_controller/CMakeLists.txt

  )

 ament_target_dependencies(dwa_controller 
  rclcpp 
  std_msgs
-  nav2_msgs
+  nav2_tasks
  robot


Should this one be nav2_robot?

ghost · 2018-08-22T23:54:15Z

src/nav2_navigation/bt_navigator/src/BTNavigator.cpp

+}
+
+TaskStatus
+BTNavigator::executeAsync(const nav2_tasks::NavigateToPoseCommand::SharedPtr command)


This function and the executeAsync in simpleNavigator are very long and could be easily broken up into smaller logical chunks. I think if you do that, you could probably eliminate the need for that goto as well.

Behavior trees handle this nicely for the BTNavigator. I'll see about making a function for the SimpleNavigator.

* Cleaning up build exports and imports. * Adding auto uncrustify and cppcheck to the build. This will automatically run uncrustify, cppcheck, and cpplint whenever you run the test suite (eg. colcon test) Also, somewhat unrelated, I added whitespace to package.xml to make it a bit easier to read.

* Created the DijkstraPlanner based on the core algorithms from Navfn ROS1 nav package. * Updated the SimpleNavigator to use the new DijkstraPlanner * Created an initial WorldModel node and Costmap mockup to test the new planner. * Fixed bug with TaskClient. * Added a few new messages and services related to Costmap. Made a few updates to the Path messages.

…of a given task type. Also create a shell BTNavigator class and introduce namespaces.

…vigation2 into bt-navigation-shell

mjeronimo · 2018-08-28T19:34:40Z

src/nav2_util/src/Costmap.cpp

+
+  vector<uint8_t> costmapFree =
+    // 0 1 2 3 4 5 6 7 8 9
+  {o, o, o, o, o, o, o, o, o, o,    // 0


Unfortunately, this seems to be the format that cpplint wants.

orduno · 2018-08-28T21:44:16Z

Wondering if we could remove the nav2_ prefix...

orduno · 2018-08-28T21:46:59Z

Regarding the nav2_util folder structure, instead of having nav2_util/include, nav2_util/src, might be better if each folder was a package: nav2_util/costmap/, nav2_util/map_loader, etc. ?

mjeronimo · 2018-08-28T21:49:24Z

Regarding the nav2_prefix: the directories, as I understand the convention, should be treated as separate packages (potentially separate repos). The nav2_ prefix associates the various projects (similar to mbf_). One exception so far is that Mission Planning/Execution might be useful outside of this navigation project/set of projects. That's why I kept it "mission_planning" for now. We can add the prefix if necessary.

mjeronimo · 2018-08-28T21:51:32Z

About the utility directory structure. I'm thinking we have a single utility library of various, relatively simple, functions/classes. If something grows to be worth a separate package, we can make it so. In this case Costmap (so far) fits the former situation - a simple class that can be included in a utility library.

orduno

Looks good to me. Made two additional comments.

Co-authored-by: andriimaistruk <andriy.maistruk>

…segment_CM AUTO-1438 Use `/is_segment` collision monitor mode for segment action

* Return distance_traveled from output port * Read ports from on_tick * Implement for spin * Lint * Fix rebase errors * Revert "Implement for spin" This reverts commit 0acecdf. --------- Co-authored-by: redvinaa <[email protected]>

This section describes the new functionality of selecting in NavigateToPose the BT to use in bt_navigator

Modify the directory structure to allow for multiple implementations …

71be278

…of a given task type. Also create a shell BTNavigator class and introduce namespaces.

mjeronimo requested review from a user, orduno, mhpanah and mkhansenbot August 21, 2018 21:22

mhpanah approved these changes Aug 22, 2018

View reviewed changes

ghost reviewed Aug 22, 2018

View reviewed changes

Carl Delsey and others added 5 commits August 23, 2018 09:49

Modify the directory structure to allow for multiple implementations …

eb656f2

…of a given task type. Also create a shell BTNavigator class and introduce namespaces.

Merge branch 'bt-navigation-shell' of https://github.com/mjeronimo/na…

9b82ba4

…vigation2 into bt-navigation-shell

Use c++ duration in TaskClient::waitForResult

f714644

mjeronimo commented Aug 28, 2018

View reviewed changes

Michael Jeronimo added 2 commits August 28, 2018 12:48

Add a few missing stub files

391e8ae

Rename BTNavigator according to Google coding conventions

a7971e9

orduno approved these changes Aug 28, 2018

View reviewed changes

mjeronimo merged commit 67c1e66 into ros-navigation:ros2_devel Aug 28, 2018

mjeronimo deleted the bt-navigation-shell branch August 28, 2018 23:29

ghost referenced this pull request in logivations/navigation2 Mar 7, 2022

added namespace for costmap (#28)

2c31c43

Co-authored-by: andriimaistruk <andriy.maistruk>

mrmara mentioned this pull request Apr 29, 2022

AMCL change map / first_map_only = false #2931

Closed

doisyg added a commit to doisyg/navigation2 that referenced this pull request Nov 27, 2023

Merge pull request ros-navigation#28 from botsandus/AUTO-1438_use_is_…

acc173f

…segment_CM AUTO-1438 Use `/is_segment` collision monitor mode for segment action

jonipol mentioned this pull request Feb 23, 2024

SmacHybrid planner can crash to segmentation fault #4141

Closed

mini-1235 pushed a commit to mini-1235/navigation2 that referenced this pull request Feb 5, 2025

Adding a section for Selection of BT in bt_navigator (ros-navigation#28)

9db8be5

This section describes the new functionality of selecting in NavigateToPose the BT to use in bt_navigator

Modify the directory structure to allow for multiple implementations … #28

Modify the directory structure to allow for multiple implementations … #28

Uh oh!

Conversation

mjeronimo commented Aug 21, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

orduno commented Aug 28, 2018

Uh oh!

orduno commented Aug 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mjeronimo commented Aug 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mjeronimo commented Aug 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

orduno left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

orduno commented Aug 28, 2018 •

edited

Loading

mjeronimo commented Aug 28, 2018 •

edited

Loading

mjeronimo commented Aug 28, 2018 •

edited

Loading