[ROS2] grid_map_filters port

[Marwan99]

• Linted
• ROS related APIs updated

[ghost]

Congratulations 🎉. DeepCode analyzed your code in 0.1 seconds and we found no issues. Enjoy a moment of no bugs ☀️.

👉 View analysis in DeepCode’s Dashboard | Configure the bot

[SteveMacenski]

This probably should have been multiple PRs, this one was a little long for one go, but overall I didn't pick anything up with my pedantic radar. You've gotten good at this.

[SteveMacenski]

In file included from /opt/overlay_ws/src/grid_map/grid_map_filters/src/NormalVectorsFilter.cpp:12:
/usr/include/tbb/task_scheduler_init.h:21:154: note: #pragma message: TBB Warning: tbb/task_scheduler_init.h is deprecated. For details, please see Deprecated Features appendix in the TBB reference manual.
   21 | #pragma message("TBB Warning: tbb/task_scheduler_init.h is deprecated. For details, please see Deprecated Features appendix in the TBB reference manual.")
      |                                                                                                                                                          ^
In file included from /opt/overlay_ws/src/grid_map/grid_map_filters/src/NormalVectorsFilter.cpp:13:
/usr/include/tbb/tbb.h:21:154: note: #pragma message: TBB Warning: tbb.h contains deprecated functionality. For details, please see Deprecated Features appendix in the TBB reference manual.
   21 | #pragma message("TBB Warning: tbb.h contains deprecated functionality. For details, please see Deprecated Features appendix in the TBB reference manual.")
      |                                           

FYI a bunch of TBB updates from 20.04, tbb.h isn't valid anymore, you have to include the specific headers you require now. See what features of TBB are used and just include those (ex. tbb/parallel_for.hpp. I love TBB, but its really fickle)

[Marwan99]

Regarding b4774b9, as far as I understood the functionality of tbb::task_scheduler_init is depreciated and has not been replaced, so I removed it completely. The number of threads would be set automatically now when tbb::parallel_for is called.

Please let me know if that doesn't sound right.

[SteveMacenski]

c++: fatal error: Killed signal terminated program cc1plus

Ooof, that's the "I'm out of RAM" message. Usually I get around that by doing something like reducing the number of builds going on at once, but this is just 1 build going on...

[Marwan99]

Should we try something silly like that?

colcon build --parallel-workers 1 --packages-up-to grid_map_ros
colcon build --parallel-workers 1 --packages-select grid_map_filters # Add upcoming packges here

we can also give --executor sequential a try.

[SteveMacenski]

I don't know what separating them would do that the parallel workers wouldn't. You could try that but it looks to be just 1 package running but worth a shot. If that doesn't work, you could try your suggestion too. Maybe multiple runs will clear out some memory?

I don't have a good answer for this. The next step would be figuring out how to add more memory or reducing this builds memory requirements. Either by having more smaller libraries build or ask Circle for a larger RAM provision for this repo.

[Marwan99]

Both attempts failed. Last failure is interesting though, it failed in a section that has been not touched (container setup step), is it worth rerunning? Either way doesn't seem very stable.

Either by having more smaller libraries build

Excuse my ignorance, what do you mean by smaller library builds?

[SteveMacenski]

I mean break up the add_library() call from a bunch of files to just a few files and have multiple add_library()'s

[Marwan99]

For a couple of runs now the CI has been running out of memory when building rosbag2_cpp and not even coming close to building grid_map packages.

[SteveMacenski]

You could ask for a new release of it to get your changes into the distribution to avoid building here.

Short of that, I think we'd need to explore how to lower the CI memory, ask Circle for more resource provisions, or talk to OR about getting a special build farm setup for this repo.

[Marwan99]

You could ask for a new release of it to get your changes into the distribution to avoid building here.

grid_map/tools/ros2_dependencies.repos

Lines 7 to 15 in 07c8429

	ros2/rcpputils:
	type: git
	url: https://github.com/ros2/rcpputils.git
	version: master
	ros2/rosbag2:
	type: git
	url: https://github.com/ros2/rosbag2.git
	version: master

We are using rcpputils and rosbag2 from master, wouldn't moving to a rolling nightly docker image save us from having to build them in the underlay, saving us time in our CI runs. Most importantly, this will also allow us to mitigate the memory issues we are having now when building rosbag2_cpp.

[SteveMacenski]

wouldn't moving to a rolling nightly docker image save us from having to build them in the underlay,

Maybe, worth a try though

[Marwan99]

@SteveMacenski can you please rerun to verify that this build success is not a one-off?

[SteveMacenski]

Retriggered

[Marwan99]

@remod b7f4ae7 and daa32ac contain the requested changes, please review.

[maximilianwulf]

@remod b7f4ae7 and daa32ac contain the requested changes, please review.

Hey @Marwan99 sorry to answer that late, but I don't think we should go this way in this MR as it should be a port and it deviates too much from the ROS1 branch.
Additionally, I don't directly agree with @remod that we should remove the template yet. We see that we can do a lot of nice things with other representations, OpenCV mat objects, or other containers for deep learning. Of course at the current stage, the filters are not that flexible but it limits us in the future to extend/optimize them.

[maximilianwulf]

Thanks a lot, LGTM. One comment, if we so many changes we should really stick to only linting and API changes as described in the MR, otherwise it takes quite some time to review and to search for other changes.

One question, was the moving of the .cpp files from grid_map_filters/src to grid_map_filters/src /grid_map_filters enforced by ROS2?

[Marwan99]

@maximilianwulf templates and 2 libraries changes are now reverted.

One question, was the moving of the .cpp files from grid_map_filters/src to grid_map_filters/src /grid_map_filters enforced by ROS2?

This was just due to some refactoring I made when creating 2 libraries, should be back as before.

---

CI failure should be fixed once #253 is merged.

[maximilianwulf]

LGTM, so lets wait for the other MR to be finished.

[Marwan99]

@SteveMacenski The failure seems to be costamp2d related, any advice?

I think the failure is not related to this PR, all the PR related issues are now resolved AFAIK. Shall we merge this PR for now and address the new test failure separately.

[Marwan99]

Actually we can in theory use nav2 from binaries instead of building it, will give that a quick try.

[Marwan99]

@SteveMacenski Good to merge?

[SteveMacenski]

LGTM, @maximilianwulf ?

[maximilianwulf]

LGTM as well 👍

[maximilianwulf]

Let's go, sorry for delay.