Metapackage + Reorg + build script updates #107

SteveMacenski · 2018-09-27T01:43:51Z

Addressing #78 and #35 and #135

Adding a proper metapackage to the repository
bringup package
DWB planner is as a placeholder, in a nav2_controller metapackage until I think of a better way to handle this
Reorganizing packages to exist in the root directory in accordance to ROS conventions
Changing build scripts and documentation to not build the workspace in the package repository (which would make it impossible to build other things in the same workspace which is a necessity) -- now it builds in a navigation2_ws workspace. initial_ros_setup and build_all scripts updated with docs
Fixing some tab -> space conversion issues
merging the msgs packages - it is standard for a metapackage to contain 1 package for messages. It made the list of packages too long otherwise and most of those packages were completely empty anyhow. We shouldnt need 7 msgs packages for 5 messages and 1 service (where most of those are repeats from nav_msgs anyhow)

Notes for future work based on what I looked into:

Many of the messages are either near identical or identical to a nav_msgs counterpart. Is there a reason for this? The Path message is only geometry_msgs/Pose and not geometry_msgs/PoseStamped like in nav_msgs. That Stamp is really important for those of us that create custom planner/controllers.
Build script function download_navstack() hard-codes the ros-planning repo, in another PR I could change this to be a flag to use a fork instead - which would be useful for those of us without push permissions without having to add a remote.

crdelsey · 2018-09-27T20:14:52Z

To address your second "future work" item, there is a mechanism in the build script to override the navigation2 repo being used, however, it is undocumented. I figured nobody would ever need it but me.

If you create a VCS repo file called custom_nav2.repos and have it in the current working dir, the initial_ros_setup script will pull the navigation repo from there.

Here's the relevant snippet of code. It's kind of crude, but helped keep the script fairly simple.

if [ -f "custom_nav2.repos" ]; then #override default location for testing
    vcs import src < custom_nav2.repos
fi

crdelsey

Let's wait on merging this until the costmap changes are merged in a day or two
@SteveMacenski Is there any reason you did not move the dwb* packages to the root of the repo? They are still under nav2_dwb_controller
I opened issues Rename packages and folders along the lines of nav2_<type of package>_<specific_implementation> #110 and Create a class diagram that shows the relationship between packages in the repo #111 to make up for the loss of structure caused by flattening the tree

mjeronimo · 2018-09-27T22:19:09Z

@SteveMacenski I think it's an unfortunate ROS convention to eliminate directory hierarchy. I find that grouping related functionality and organizing into a hierarchy to be a quite common an useful tool. I believe you're suggesting that all projects should be in a single top-level directory. Carl has suggested ways to adapt to the loss of structure, but again, I don't see much of a benefit to flattening everything out.

crdelsey · 2018-09-28T00:58:18Z

nav2_msgs/package.xml

  <version>0.1.0</version>
-  <description>TODO</description>
+  <description>Messages and service files for the navigation2 stack</description>
+  <author>Michael Jeronimo, Steve Macenski</author>


We should avoid use of the author tag. Potentially anyone who has ever contributed a patch is an author. I'd expect the list to keep growing and growing over time, with no clear rule about when someone should be removed, or who gets top billing.

IMHO, the best solution is to not use this tag. The authorship is available in the git history.

crdelsey · 2018-09-28T00:59:19Z

navigation2/package.xml

+  <description>
+    ROS2 Navigation Stack
+  </description>
+  <author>Oregon Robotics Team, Steve Macenski</author>


Again, I'd prefer to leave out the author field.

SteveMacenski · 2018-09-28T13:33:41Z

I'll try to address here in order:

The way that dwb was ported into navigation2 is essentially a metapackage of itself. Having a metapackage inside another one isnt ideal but also has precedence. Long term, I don't really see dwb staying here and just making a ros2 branch inside of DLu's repo. Breaking all those packages out into the root would be over kill. It's not as if those sub-packages within dwb would likely ever be packaged separately from a package manager perspective, while the rest of these absolutely would be.
@mjeronimo it's convention and it makes alot of sense to me. These packages should not be looked at as a single project, but as a collection of projects. Many professional users will utilize only section of the navigation2 stack (like only AMCL, or global_planner but not costmap_2d, etc) so making that structure starts typing packages together that should always remain isolated. If they logically belong together, then they should be in the same package in my opinion
@crdelsey I have no beef with removing the author tag. I just took it from some other boiler plate I use.

I'd like to get a "a-ok" on these topics or continue the discussion to come to a resolution before I go and try to resolve these merge conflicts that have come up from some libs reorg

navigation2/package.xml

mjeronimo · 2018-09-29T01:10:18Z

@SteveMacenski Please proceed with the PR, address any review feedback, resolve the merge conflicts and we'll get it integrated. Thanks.

SteveMacenski · 2018-10-08T16:53:21Z

Just getting back from Madrid, I'll be working on this.

SteveMacenski · 2018-10-08T17:21:34Z

These merge conflicts are going to take me a long time to resolve, a great deal has changed

SteveMacenski · 2018-10-08T18:11:05Z

I'm putting a pin in this. So many dramatic changes have happened, I can't find the time anytime soon to redo all this without a guarantee that once done I'll have to restart again

crdelsey · 2018-10-08T18:12:55Z

@SteveMacenski What would be a good way to approach this? Maybe if we take it one directory at a time? If you let us know which one you want to tackle we can try to minimize changes in that area.

SteveMacenski · 2018-10-08T18:20:31Z

Files keep moving around, or being renamed, or generally removed without much public discussion, so I can't track what's moved and why -- and more importantly, what's currently being shifted around to know once I spend the 10+ hours to fix this, that I won't have to simply restart after this PR soaks and things outside this has changed.

If there's 2-3 days someone can give me the OK that major things aren't going to be moving, I can do it, but I would like to have any discussion on concerns about this PR before I go through it so I won't have to redo again then after from a lingering concern

edit: because of the packages refactor, it needs to be an all at once change or else it wont compile in that mean time. I'll take another stab at it this afternoon to see if I can figure out a way to get around all these merge issues

mkhansenbot · 2018-10-08T20:07:04Z

@SteveMacenski - we'll hold off on any merges until Wednesday if you think you can get this rebased and ready for merge. Is that enough time?

SteveMacenski · 2018-10-08T22:30:11Z

Feel free to merge PRs, but just try to not merge in things that include major refactoring (i.e. large number of files moved, removed, or added)

I can make that happen.

SteveMacenski · 2018-10-09T03:40:45Z

OK stayed up late to get this out as quick as possible to not block. It's all in place with the exception of the gtesting in dwb_plugins I get a linking issue with std err below when running tests. I'm a little confused why this would be an issue here when it wasn't in the past as far as I recall. Thoughts would be appreciated but the structure and refactor is complete @mkhansen-intel

--- stderr: dwb_plugins                                 
CMakeFiles/twist_gen_test.dir/test/twist_gen.cpp.o: In function `__static_initialization_and_destruction_0(int, int)':
twist_gen.cpp:(.text+0x944f): undefined reference to `testing::internal::MakeAndRegisterTestInfo(char const*, char const*, char const*, char const*, void const*, void (*)(), void (*)(), testing::internal::TestFactoryBase*)'
twist_gen.cpp:(.text+0x949c): undefined reference to `testing::internal::MakeAndRegisterTestInfo(char const*, char const*, char const*, char const*, void const*, void (*)(), void (*)(), testing::internal::TestFactoryBase*)'
twist_gen.cpp:(.text+0x94e9): undefined reference to `testing::internal::MakeAndRegisterTestInfo(char const*, char const*, char const*, char const*, void const*, void (*)(), void (*)(), testing::internal::TestFactoryBase*)'
twist_gen.cpp:(.text+0x9536): undefined reference to `testing::internal::MakeAndRegisterTestInfo(char const*, char const*, char const*, char const*, void const*, void (*)(), void (*)(), testing::internal::TestFactoryBase*)'
twist_gen.cpp:(.text+0x9583): undefined reference to `testing::internal::MakeAndRegisterTestInfo(char const*, char const*, char const*, char const*, void const*, void (*)(), void (*)(), testing::internal::TestFactoryBase*)'
CMakeFiles/twist_gen_test.dir/test/twist_gen.cpp.o:twist_gen.cpp:(.text+0x95d0): more undefined references to `testing::internal::MakeAndRegisterTestInfo(char const*, char const*, char const*, char const*, void const*, void (*)(), void (*)(), testing::internal::TestFactoryBase*)' follow
collect2: error: ld returned 1 exit status
gmake[2]: *** [twist_gen_test] Error 1
gmake[1]: *** [CMakeFiles/twist_gen_test.dir/all] Error 2
gmake: *** [all] Error 2
---
Failed   <<< dwb_plugins	[ Exited with code 2 ]

crdelsey · 2018-10-09T16:20:20Z

@SteveMacenski My gut feel is that this link issue you are seeing will go away with a clean build. I pulled your branch and it builds fine for me.

SteveMacenski · 2018-10-09T16:24:52Z

OK figured out my issue about an hour ago. Either way, feel free to look through this and comment, I'm done with needed things to get this out

Some big points to cover

costmap_2d -> nav2_costmap_2d since it is not a different package than costmap_2d in navigation since it's been ported over. It should have a new name to not confuse people. I imagine some improvements will be made here too.
nav2_controller is a placeholder metapackage (with a metapackage file) for all the dwb/robot_navigation stuff from Locus. Having a metapackage in another metapackage is OK by convention but unideal. I'm thinking about a better way to handle this however allowing them all in the root directory isn't probably the move.
Added a bringup package because of the loose launch files

crdelsey

The rebase looks good to me. Our process is to get two people to approve before committing, so as soon as somebody else takes a look and approves, we can get this in.

mkhansenbot

In general this is good, but very long. Can we squash some commits and next time can you submit smaller PRs?

mkhansenbot · 2018-10-09T21:25:06Z

nav2_costmap_2d/CHANGELOG.rst

@@ -1,12 +1,12 @@
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
-Changelog for package costmap_2d
+Changelog for package nav2_costmap_2d


Looks like a global search and replace went too far here, I think these changes in the CHANGELOG.rst should be backed out.

As this is a new package, I cleared the change log and noted the direct port from costmap_2d with the exact version number and date

Makes sense to me. Thanks!

mkhansenbot · 2018-10-09T21:33:57Z

nav2_costmap_2d/test/obstacle_tests.launch

@@ -0,0 +1,5 @@
+<launch>


Should this file be here? It's a ROS style launch file, I think it would need to be replaced for ROS2 with a python launch file.

So these have been floating around in this repo as of today. I agree that they would not work, but I dont think we should remove them unless we either make a ticket to bring back this same functionality or remove the tests outright.

I think given the scope of this PR already is large, I would just leave it and make a separate ticket after this is merged to enable unit testing of this in ROS2

It looks like this ticket actually already exists #121

@bpwilcox - does #121 mean to add these tests? It's not clear to me please verify what it means exactly

mkhansenbot · 2018-10-09T21:35:36Z

nav2_costmap_2d/test/simple_driving_test.xml

@@ -0,0 +1,14 @@
+<launch>


Same comment as above - not sure this test should be added unless it's going to be made to work in ROS2.

SteveMacenski · 2018-10-09T23:14:29Z

Yes, this is very large. It wasn't my intention when I started to cover so many things. Will break out in future. Feel free to squash commits as you see fit.

mjeronimo

I'm OK with address some remaining issues like header macro guards and linting in a separate PR. I'd like to get these changes in.

mjeronimo · 2018-10-10T03:52:04Z

nav2_controller/nav2_controller/package.xml

+  <description>
+    ROS2 controller (DWB) metapackage
+  </description>
+  <maintainer email="oregon.robotics.team@intel.com">Oregon Robotics Team</maintainer>


Let's remove any references to this ficticious e-mail address and use specific addresses. For this one, we can use Carl's info instead (along with steve's).

Will be in next PR

mjeronimo · 2018-10-10T03:52:50Z

nav2_costmap_2d/include/nav2_costmap_2d/array_parser.h

@@ -28,13 +28,13 @@
 *
 * author: Dave Hershberger
 */
-#ifndef COSTMAP_2D_ARRAY_PARSER_H
-#define COSTMAP_2D_ARRAY_PARSER_H
+#ifndef nav2_costmap_2d_ARRAY_PARSER_H


Looks like a global search and replace issue.

Will be in next PR

mjeronimo · 2018-10-10T03:53:06Z

nav2_costmap_2d/include/nav2_costmap_2d/cost_values.h

 {
 static const unsigned char NO_INFORMATION = 255;
 static const unsigned char LETHAL_OBSTACLE = 254;
 static const unsigned char INSCRIBED_INFLATED_OBSTACLE = 253;
 static const unsigned char FREE_SPACE = 0;
 }
-#endif  // COSTMAP_2D_COST_VALUES_H_
+#endif  // nav2_costmap_2d_COST_VALUES_H_


Global search and replace issue.

mjeronimo · 2018-10-10T03:53:52Z

nav2_costmap_2d/include/nav2_costmap_2d/costmap_2d_publisher.h

@@ -35,16 +35,16 @@
 * Author: Eitan Marder-Eppstein
 *         David V. Lu!!
 *********************************************************************/
-#ifndef COSTMAP_2D_COSTMAP_2D_PUBLISHER_H_
-#define COSTMAP_2D_COSTMAP_2D_PUBLISHER_H_
+#ifndef nav2_costmap_2d_nav2_costmap_2d_PUBLISHER_H_


We can have a separate PR to clean up header guards and address any linting issues.

mjeronimo · 2018-10-10T15:57:08Z

Integrating the work. We can deal with any remaining issues afterwards.

* changing oregon team email to Carls * changing header gaurds * gaurds galore

* change trees * modify pickup and execute goal bt, add check goal action type BT condition Node * draft * draft * create action server BT clients * draft * draft * draft * draft * add set lidar field action * add set lidar field action * add set lidar field action * add set lift level to BT * draft * add setusedepthcameras into BTs * draft * Added driving direction input port for adjust_pallet_goal action server (ros-navigation#107) * Added driving direction input port for adjust_pallet_goal action server * rm wrong inputs * Add bin_id as an input to the AdjustPalletGoal Action Server * modify a bit * modify a bit * modify a bit Co-authored-by: Andrii Maistruk <andriy.maistruk@logivations.com> Co-authored-by: Andrii Maistruk <71632363+andriimaistruk@users.noreply.github.com> * add setlidarfield action * add setusedepthcameras action * add setusedepthcameras action * add preload libs * add agvclosetopallet action msg * make it run a bit * make it run a bit * fix bool message processing * pallet id value range made bigger * change agv action type * fix pickup entry * draft * draft * looks important * enter driving direction for narrow pre approach * add follow path goal visualization in rviz * fix remapping names * add get_arm_position action server * add get_arm_position action server * add get_arm_position action server * store starting_pose * added action server to get amr position to blackboard * add pallet putdown master BT * fail if pickup failed eq (when entering pallet lidar at field 2 was not triggered) * include pallet putdown master BT * fix typo * Add nav2_behavior_tree::BtActionServer (ros-navigation#2010) * Add nav2_behavior_tree::BtActionServer Signed-off-by: Sarthak Mittal <sarthakmittal2608@gmail.com> * Fix cpplint errors Signed-off-by: Sarthak Mittal <sarthakmittal2608@gmail.com> * Remove unnecessary statements in BtActionServer Signed-off-by: Sarthak Mittal <sarthakmittal2608@gmail.com> * Make nav2_behavior_tree::BtActionServer a composable object * Add comments * Add on preempt callback, fix naming issues, and move tf to bt navigator * Add separate implementation header for BtActionServer * Fix cpplint error * Pass plugin library names as argument to BtActionServer * Remove action server getter and update onPreempt to not load BT * Fix unnecessary include * Fix function names Signed-off-by: Sarthak Mittal <sarthakmittal2608@gmail.com> * Fix typo (cherry picked from commit 6171087) * subtree commented and left for debugging purposes * subtree commented and left for debugging purposes * two level hierarchy related changes * two level hierarchy related changes * two level hierarchy related changes * making it work * making it work * wait master BT * add stdout logs * add execute goal higher bt navigator * redo bt navigator according to cherry for bt_action_server * add new package depend * making it launch * remove comment * fix blackboard variable name * fix blackboard variable name * fix blackboard variable name * fix convertfromstring to path type warning * add approach goal * changes requested * changes requested Co-authored-by: rdeniza <61455514+rdeniza@users.noreply.github.com> Co-authored-by: Sarthak Mittal <sarthakmittal2608@gmail.com> Co-authored-by: andriimaistruk <andriimaistruk>

* moved over * moving all messages into an internal msgs metapackage grouping * moving all the messages into a homologated package * making necessary changes for the msgs package homologation * adding metapackage * adding myself as a maintainer * build script and doc updates for package in workspace * dealing with merge issues * remove author fields * moving around * merge conflict v3 * remove costmap 2d from utils * killing src dir * remove costmap 2d clone ros-navigation#3 * remove extraneous file * kill dwa for some reason * dwb rectification * removing oregon author -> maintainer * controller -> nav2_controller * adding new msgs to new things * tags * bringup pkg * compiling with task status mode * getting the task status to compile for dwa * adding costmap2d into nav2 naming nominclature * adding controller metapackage * recursive definitions -- bringup cannot be inside of the metapackage * kill bad line * hopefully actual last git merge error... * changelog resolve

SteveMacenski added 9 commits September 26, 2018 14:36

moved over

f270f39

moving all messages into an internal msgs metapackage grouping

1aa2515

merge master

753acc0

moving all the messages into a homologated package

dee5d82

making necessary changes for the msgs package homologation

f73b089

adding metapackage

9e39290

adding myself as a maintainer

83b48c1

build script and doc updates for package in workspace

a6ff72a

dealing with merge issues

53e9eee

SteveMacenski changed the title ~~Steve devel~~ Metapackage + Reorg + build script updates Sep 27, 2018

crdelsey approved these changes Sep 27, 2018

View reviewed changes

crdelsey reviewed Sep 28, 2018

View reviewed changes

mhpanah reviewed Sep 28, 2018

View reviewed changes

navigation2/package.xml Show resolved Hide resolved

SteveMacenski closed this Oct 8, 2018

SteveMacenski reopened this Oct 8, 2018

SteveMacenski added 2 commits October 8, 2018 09:53

remove author fields

a9c58cd

merge conflicts try 1

6956dc8

SteveMacenski closed this Oct 8, 2018

SteveMacenski reopened this Oct 8, 2018

moving around

e90fdf2

adding new msgs to new things

5c9bc6b

tags

9c09620

SteveMacenski mentioned this pull request Oct 8, 2018

Create a navigation2_bringup package #135

Closed

SteveMacenski added 8 commits October 8, 2018 17:15

bringup pkg

c1fa056

compiling with task status mode

719c622

getting the task status to compile for dwa

6ea52aa

adding costmap2d into nav2 naming nominclature

da3cafe

adding controller metapackage

2d9cbdc

recursive definitions -- bringup cannot be inside of the metapackage

6636b0f

kill bad line

3f8e650

hopefully actual last git merge error...

c76f278

crdelsey requested review from mjeronimo and mkhansenbot October 9, 2018 16:53

crdelsey approved these changes Oct 9, 2018

View reviewed changes

mkhansenbot reviewed Oct 9, 2018

View reviewed changes

changelog resolve

bc168a2

mjeronimo approved these changes Oct 10, 2018

View reviewed changes

mjeronimo merged commit ac932ea into ros-navigation:master Oct 10, 2018

mkhansenbot mentioned this pull request Oct 11, 2018

Replacing costmap stub with costmap_2d package in WorldModel #138

Merged

crdelsey pushed a commit that referenced this pull request Oct 15, 2018

Cleanup from #107 (#158)

507e37d

* changing oregon team email to Carls * changing header gaurds * gaurds galore

crdelsey mentioned this pull request Oct 19, 2018

Repo formatting #35

Closed

Metapackage + Reorg + build script updates #107

Metapackage + Reorg + build script updates #107

Conversation

SteveMacenski commented Sep 27, 2018 • edited Loading

crdelsey commented Sep 27, 2018

crdelsey left a comment

Choose a reason for hiding this comment

mjeronimo commented Sep 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SteveMacenski commented Sep 28, 2018 • edited Loading

mjeronimo commented Sep 29, 2018

SteveMacenski commented Oct 8, 2018

SteveMacenski commented Oct 8, 2018

SteveMacenski commented Oct 8, 2018 • edited Loading

crdelsey commented Oct 8, 2018

SteveMacenski commented Oct 8, 2018 • edited Loading

mkhansenbot commented Oct 8, 2018

SteveMacenski commented Oct 8, 2018

SteveMacenski commented Oct 9, 2018 • edited Loading

crdelsey commented Oct 9, 2018

SteveMacenski commented Oct 9, 2018

crdelsey left a comment

Choose a reason for hiding this comment

mkhansenbot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SteveMacenski commented Oct 9, 2018

mjeronimo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjeronimo commented Oct 10, 2018

SteveMacenski commented Sep 27, 2018 •

edited

Loading

SteveMacenski commented Sep 28, 2018 •

edited

Loading

SteveMacenski commented Oct 8, 2018 •

edited

Loading

SteveMacenski commented Oct 8, 2018 •

edited

Loading

SteveMacenski commented Oct 9, 2018 •

edited

Loading