[ROS2] port grid_map_ros #242

Marwan99 · 2020-06-25T01:41:18Z

rosbag replaced with rosbag2 (biggest change in this package port, see last 2 function definitions in GridMapRosConverter.cpp).
Updated grid_map_msgs::msg::GridMap to the updated message definition.
Updated nav_msgs::OccupancyGrid to nav2_msgs::msg::Costmap, consequently updated the API method function names.
Linted and uncrustified.

Marwan99 · 2020-06-25T01:43:55Z

CI expected to fail initially due to dependency on nav2_msgs should be resolved once a fix is applied in #240 and the fix merged here.

Marwan99 · 2020-06-25T01:45:18Z

And CI is not showing up again... Am I doing something wrong?? This is branched off ros2. Could that be because I am running CI on my fork?

CI from my fork: https://app.circleci.com/pipelines/github/Marwan99/grid_map/67/workflows/081cb8cc-f361-40ee-95be-3e4f63c07439/jobs/61/steps

grid_map_ros/package.xml

SteveMacenski · 2020-06-25T03:18:14Z

I think you really want to be using occupancy_grid rather than costmap message. The nav2_msgs/Costmap is really only used in 1 place for the costmap subscriber for the recovery server. The occupancy grid is the generic message published of the occupancy information and what rviz is displaying.

I could see an argument for supporting both though

Marwan99 · 2020-06-25T14:28:28Z

I thought nav_msgs was replaced with nav2_msgs, at least comparing OcuppancyGrid.msg with Costmap.msg convinced me with this idea as they looked very similar from a brief look. I have reverted back to OcuppancyGrid and removed Costmap, I can bring Costmap back if there is interest in supporting both, just let me know what you think.

CI expected to fail initially due to dependency on nav2_msgs should be resolved once a fix is applied in #240 and the fix merged here.

Doesn't apply anymore atm, dependency on nav2_msgs no longer in place.

And CI is not showing up again... Am I doing something wrong?? This is branched off ros2. Could that be because I am running CI on my fork?

Removing circleci from my fork fixed the above ^.

SteveMacenski · 2020-06-25T18:39:03Z

No, nav_msgs is still very much so used. nav2_msgs are an extension.

I think supporting both would be fine - you already did the work for nav2_msgs/Costmap so may as well keep it, it could come in handy. If someone subscribes to this other than the recovery server and wants to use some grid-maps-tooling then that would be valued so it doesn't have to be converted again into a occ grid first.

…d_map_ros

maximilianwulf

LGTM

…ion tests

Marwan99 · 2020-06-28T23:49:33Z

I am struggling atm with getting rosbag2 to work, specifically unable to create a bag file. However I suspect there might be a bug in the rosbag2 API so I submitted an issue ros2/rosbag2#448, but I am might be just doing something wrong. Anyhow, I should hopefully get some help from them on this issue soon.

Marwan99 · 2020-07-06T19:11:55Z

@SteveMacenski How long do you think we should wait for the rosbag2 issue to be fixed? Should we comment out the rosbag sections and move on for now?

SteveMacenski · 2020-07-06T19:28:55Z

is there any movement on it?

Marwan99 · 2020-07-22T19:35:00Z

CI is failing with Unable to fetch some archives when setting up the container. @SteveMacenski should we try a rerun?

Also we might need to add robag2 master branch as a dependency (the bug fix is on master and foxy is using the foxy branch), but lets wait till we get a CI run first.

SteveMacenski · 2020-07-22T19:50:04Z

@Marwan99 I re-ran the job

…d_map_ros

Marwan99 · 2020-07-22T20:02:07Z

Sorry @SteveMacenski same error happening again. Am I doing something wrong?

SteveMacenski · 2020-07-22T20:29:35Z

No, I think the server is just down for a little while. I looked and seems fine now, retriggered

SteveMacenski · 2020-07-22T22:23:23Z

Cool! now running, just actual errors now :-)

Marwan99 · 2020-07-23T00:53:45Z

The rcpputills API has been updated and we need to use the updated version instead of the released version to make use of the new API function rcpputils::fs::remove_all. I have added rcpputills as a dependency and it seems to be building fine in the UNDERLAY_WS. However, when building grid_map_ros it seems that rcpputills is being included from the ROS_WS and not from the UNDERLAY_WS as excpected. The sourcing sequence LGTM and I can't figure out what is causing the problem. Any suggestions?

Since we will also need to add rosbag2 (as we now need the master branch for the bug fix) as a dependency and build it as part of the UNDERLAY_WS, why don't we just move to a ros2 rolling/nighlty image instead of foxy? I think it will make things simpler and potentially smoother in the long run.

SteveMacenski · 2020-07-23T01:10:51Z

What is rcpputills?

However, when building grid_map_ros it seems that rcpputills is being included from the ROS_WS and not from the UNDERLAY_WS as excpected

Please be more specific here. What are you doing where and why. Are you saying that the foxy released versions aren't correct or the master ones have changed?

Marwan99 · 2020-07-23T01:37:06Z

I am referring to the rcpputils package.

For example on this line in saveLoad test, I want to use the rcpputils::fs::remove_all function to remove any rosbags files before and after running the tests, this clean up must be done as a rosbag is being created in this test and the rosbag2 API will throw an error if a rosbag already exists with the same name, thus the need to perform pre and post test clean up.

rcpputils::fs::remove_all was recently added to both the master and foxy branches (~ 9 days ago) and is not part of the foxy released version AFAIK.

Please let me know if you need more info.

SteveMacenski · 2020-07-23T01:45:11Z

huh, haven't run into that before, I thought you mean rclutils.

rcpputils::fs::remove_all was recently added to both the master and foxy branches (~ 9 days ago) and is not part of the foxy released version AFAIK.

Thanks, that's a problem statement I can help with. I see you added to the repos file, good, but it should be master branch if we're going to have to make things build from source to be up to date. For sourcing, source just the underlay, your issue is using the local setup bash files. Remove all local files and source the normal setup.bash so you get the full path of packages available. If you source /opt/ before building the underlay, then that's in the underlay path. If you then source the underlay, you'll get the opt stuff too if you do the full setup.bash. In general, I never use local_setup.bash for that reason. I want everything.

Marwan99 · 2020-07-23T18:46:50Z

Thanks for the help @SteveMacenski :)

Ready for review, to save you some time grid_map_ros/src/GridMapRosConverter.cpp is the file with the most significant changes.

Marwan99 · 2020-07-23T18:51:58Z

.circleci/config.yml

            cd $OVERLAY_WS && rosdep install -y --ignore-src --from-paths src
      - run:
          name: Debug Build
          command: |
-            source `find $ROS_WS -maxdepth 2 -name local_setup.bash | sort | head -1`
-            source $UNDERLAY_WS/install/local_setup.bash
+            source $UNDERLAY_WS/install/setup.bash


Is this line redundant? underlay_ws is already being sourced above. Same potential duplication with the sourcing in "Run Tests" below.

SteveMacenski

@maximilianwulf please review

grid_map_ros/CMakeLists.txt

grid_map_ros/src/GridMapRosConverter.cpp

maximilianwulf

Hey, LGTM. Thanks a lot.

grid_map_ros/CMakeLists.txt

grid_map_ros/package.xml

grid_map_ros/test/GridMapRosTest.cpp

maximilianwulf · 2020-07-24T09:49:51Z

grid_map_ros/test/GridMapRosTest.cpp

+  }
+}
+
+TEST(CostmapConversion, withMove)


Are these new unit tests?

Yeah, he added both nav_msgs/occupancygrid and nav2_msgs/costmap conversions. We use nav2_msgs/costmap in a couple places to publish raw costmaps for other processes to use (primarily for recovery actions that need some view of the world but aren't in the same process as the local costmap / controller).

That is very cool, thanks that you added them.

grid_map_ros/src/GridMapRosConverter.cpp

maximilianwulf

Hey, LGTM. Thanks a lot for the effort.

SteveMacenski · 2020-07-27T18:33:23Z

@Marwan99 this OK to go in?

Marwan99 · 2020-07-27T19:29:08Z

Yeah lets merge!

Marwan99 added 2 commits June 25, 2020 02:30

Updated CMakelists.txt and package.xml

636bab7

Ported & linted header, src and test files

1efc3c2

Marwan99 mentioned this pull request Jun 25, 2020

ROS2 Port #219

Closed

SteveMacenski reviewed Jun 25, 2020

View reviewed changes

grid_map_ros/package.xml Show resolved Hide resolved

undone the OccupancyGrid to Costmap changes

577c80e

Marwan99 added 3 commits June 25, 2020 20:00

Added conversion to and from Costmap msg

448b00f

Merge branch 'ros2' of https://github.com/ANYbotics/grid_map into gri…

046e6f2

…d_map_ros

removed typo

500d6c0

maximilianwulf reviewed Jun 26, 2020

View reviewed changes

Fixed value limits in costmap conversions and updated costmap convers…

af2e63e

…ion tests

rosbag section updated

5ef3d02

Merge branch 'ros2' of https://github.com/ANYbotics/grid_map into gri…

385fda6

…d_map_ros

re-triggering CI

b21033c

adding rcpputils as a dep to use rcpputils::fs::remove_all

8401e0c

Marwan99 force-pushed the grid_map_ros branch from 1c47169 to 8401e0c Compare July 23, 2020 18:01

Marwan99 added 3 commits July 23, 2020 19:04

Triggering CI

28e09e3

added rosbag2 as a dependency and changed rcpputils dependency to master

5db6472

fixed typo

c0319cd

Marwan99 commented Jul 23, 2020

View reviewed changes

SteveMacenski reviewed Jul 23, 2020

View reviewed changes

grid_map_ros/CMakeLists.txt Show resolved Hide resolved

grid_map_ros/src/GridMapRosConverter.cpp Show resolved Hide resolved

Marwan99 added 2 commits July 23, 2020 20:04

added missing macro

a7f5b33

fixed comparison of different signs

f8526f5

maximilianwulf reviewed Jul 24, 2020

View reviewed changes

some reformatting and typos fixed

a06a6bd

maximilianwulf approved these changes Jul 27, 2020

View reviewed changes

SteveMacenski approved these changes Jul 27, 2020

View reviewed changes

SteveMacenski merged commit 07c8429 into ANYbotics:ros2 Jul 27, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ROS2] port grid_map_ros #242

[ROS2] port grid_map_ros #242

Marwan99 commented Jun 25, 2020

Marwan99 commented Jun 25, 2020

Marwan99 commented Jun 25, 2020 •

edited

Loading

SteveMacenski commented Jun 25, 2020

Marwan99 commented Jun 25, 2020 •

edited

Loading

SteveMacenski commented Jun 25, 2020 •

edited

Loading

maximilianwulf left a comment

Marwan99 commented Jun 28, 2020

Marwan99 commented Jul 6, 2020

SteveMacenski commented Jul 6, 2020 •

edited

Loading

Marwan99 commented Jul 22, 2020 •

edited

Loading

SteveMacenski commented Jul 22, 2020

Marwan99 commented Jul 22, 2020

SteveMacenski commented Jul 22, 2020

SteveMacenski commented Jul 22, 2020

Marwan99 commented Jul 23, 2020

SteveMacenski commented Jul 23, 2020

Marwan99 commented Jul 23, 2020 •

edited

Loading

SteveMacenski commented Jul 23, 2020

Marwan99 commented Jul 23, 2020

Marwan99 Jul 23, 2020

SteveMacenski left a comment

maximilianwulf left a comment

maximilianwulf Jul 24, 2020

SteveMacenski Jul 24, 2020

maximilianwulf Jul 27, 2020

maximilianwulf left a comment

SteveMacenski commented Jul 27, 2020

Marwan99 commented Jul 27, 2020

[ROS2] port grid_map_ros #242

[ROS2] port grid_map_ros #242

Conversation

Marwan99 commented Jun 25, 2020

Marwan99 commented Jun 25, 2020

Marwan99 commented Jun 25, 2020 • edited Loading

SteveMacenski commented Jun 25, 2020

Marwan99 commented Jun 25, 2020 • edited Loading

SteveMacenski commented Jun 25, 2020 • edited Loading

maximilianwulf left a comment

Choose a reason for hiding this comment

Marwan99 commented Jun 28, 2020

Marwan99 commented Jul 6, 2020

SteveMacenski commented Jul 6, 2020 • edited Loading

Marwan99 commented Jul 22, 2020 • edited Loading

SteveMacenski commented Jul 22, 2020

Marwan99 commented Jul 22, 2020

SteveMacenski commented Jul 22, 2020

SteveMacenski commented Jul 22, 2020

Marwan99 commented Jul 23, 2020

SteveMacenski commented Jul 23, 2020

Marwan99 commented Jul 23, 2020 • edited Loading

SteveMacenski commented Jul 23, 2020

Marwan99 commented Jul 23, 2020

Marwan99 Jul 23, 2020

Choose a reason for hiding this comment

SteveMacenski left a comment

Choose a reason for hiding this comment

maximilianwulf left a comment

Choose a reason for hiding this comment

maximilianwulf Jul 24, 2020

Choose a reason for hiding this comment

SteveMacenski Jul 24, 2020

Choose a reason for hiding this comment

maximilianwulf Jul 27, 2020

Choose a reason for hiding this comment

maximilianwulf left a comment

Choose a reason for hiding this comment

SteveMacenski commented Jul 27, 2020

Marwan99 commented Jul 27, 2020

Marwan99 commented Jun 25, 2020 •

edited

Loading

Marwan99 commented Jun 25, 2020 •

edited

Loading

SteveMacenski commented Jun 25, 2020 •

edited

Loading

SteveMacenski commented Jul 6, 2020 •

edited

Loading

Marwan99 commented Jul 22, 2020 •

edited

Loading

Marwan99 commented Jul 23, 2020 •

edited

Loading