Added transient local subscription qos profile parameter to map saver

[Michael-Equi]

Basic Info

Info	Please fill out this column
Ticket(s) this addresses	#1864
Primary OS tested on	Ubuntu
Robotic platform tested on	TB3 simulation

---

Description of contribution in a few bullet points

Added parameter to map saver that configures it to subscribe to the /map topic as transient local.

[mkhansenbot]

Shouldn't the default be 'true'?

[Michael-Equi]

Old behavior was false as it just used system default QoS, but if you think we should make this is the default I support that.

[SteveMacenski]

I could go either way on it, but I'd lean towards True as well

[SteveMacenski]

@mkhansenbot seems like you're interested, what's your review?

[mkhansenbot]

Looks good to me, thanks for changing the default to true, I think that's the correct behavior

[SteveMacenski]

@Michael-Equi please fix the linter errors

- nav2_map_server.cpplint whitespace/blank_line [3] (/opt/overlay_ws/src/navigation2/nav2_map_server/src/map_saver/map_saver.cpp:54)

  <<< failure message

    Redundant blank line at the end of a code block should be deleted.

  >>>

[SteveMacenski]

We don't have a house.world file in our package for that to be a valid default. If you'd like to propose adding one, we can talk about that for sure, but in another ticket / PR

[Michael-Equi]

Looks like my IDE did some automatic refactor with cached information about a house world I worked on for a bit. My bad for not checking.

[codecov]

Codecov Report

Merging #1871 into master will decrease coverage by 0.80%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1871      +/-   ##
==========================================
- Coverage   70.14%   69.34%   -0.81%     
==========================================
  Files         218      218              
  Lines       10586    10592       +6     
==========================================
- Hits         7426     7345      -81     
- Misses       3160     3247      +87     

Impacted Files	Coverage Δ
nav2_map_server/src/map_saver/map_saver.cpp	87.05% <100.00%> (+0.98%)	⬆️
...re/include/dwb_core/illegal_trajectory_tracker.hpp	40.00% <0.00%> (-60.00%)	⬇️
...roller/dwb_core/src/illegal_trajectory_tracker.cpp	25.00% <0.00%> (-55.00%)	⬇️
nav2_map_server/src/map_server/main.cpp	63.63% <0.00%> (-36.37%)	⬇️
...map_2d/include/nav2_costmap_2d/inflation_layer.hpp	91.66% <0.00%> (-8.34%)	⬇️
nav2_navfn_planner/src/navfn_planner.cpp	82.09% <0.00%> (-6.80%)	⬇️
nav2_costmap_2d/src/clear_costmap_service.cpp	17.89% <0.00%> (-6.32%)	⬇️
..._dwb_controller/dwb_core/src/dwb_local_planner.cpp	77.32% <0.00%> (-4.97%)	⬇️
...tree/include/nav2_behavior_tree/bt_action_node.hpp	79.26% <0.00%> (-4.88%)	⬇️
nav2_controller/src/nav2_controller.cpp	79.38% <0.00%> (-3.10%)	⬇️
... and 8 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 69977cd...f2a8165. Read the comment docs.

[QQting]

Could we also apply the transient_local QoS to the map saver in foxy-devel?

[SteveMacenski]

I believe that is already the case, but if it does not, we cannot change it. That is API breaking

[v-lopez]

This is not applied on foxy and we're forced to use a reimplement it to save maps.

Could you check this again? I believe it is not API breaking, at most ABI breaking due to declaring a new member variable. And on a header probably unused outside of this node.

Is it because of the QoS profile? What If I reworked it for foxy so by default it has the old behavior, and only if you set the param explicitely it has the new behavior?

For other people who come here, map can be saved by calling directly the service:
ros2 service call /map_saver/save_map nav2_msgs/srv/SaveMap "{map_topic: 'map', map_url: '/tmp/mymap', free_thresh: 0.25, occupied_thresh: 0.65}"

[SteveMacenski]

@v-lopez The main differences are the QoS settings now being used before and after the changes

• https://github.com/ros2/rmw/blob/master/rmw/include/rmw/qos_profiles.h#L51
• https://github.com/ros2/rmw/blob/master/rmw/include/rmw/qos_profiles.h#L90

The new one changes depending on RMW vendor and potentially even platform its being run on what the default settings are for that DDS vendor. I believe this counts as an ABI breaking change, correct me if I'm wrong.

Is it because of the QoS profile? What If I reworked it for foxy so by default it has the old behavior, and only if you set the param explicitely it has the new behavior?

Yes, I'd be OK considering a PR directly onto Foxy that maintains rclcpp::SystemDefaultsQoS() when map_subscribe_transient_local_ is not set - that should be easy to implement and solve the basic concern I have.

I understand its annoying, but one of the big points of ROS2 is maturity of software support and we cannot break ABI after release in a given distribution. It could have real ramifications of current users expecting certain behaviors. If you want the latest and greatest, main has all of this (and more) available! Nav2 is moving considerably faster than Navigation in ROS1 did for the better part of a decade, so part of that is that every new shiny feature isn't going to be in every distribution as we 'move fast and break things [ABI]' 😄. I'm always happy to field work-around PRs to maintain ABI for certain features in certain released distributions, but I myself will probably not spend my resources on it (e.g. would you rather me work on new-shiny-features or try to guess what people will find useful in legacy distros to individually backport them all 😆 ).

[v-lopez]

Alright I'll see if I can prepare a PR for foxy next week with this.

I understand the need for stability and for closing a version and focusing developments on a new one. But I also believe in putting the effort on these ABI compatible changes for at least the LTS versions.
As a company we're not going to ship robots with rolling or galactic, so our next viable LTS distribution is scheduled for May 2022.

As I am getting familiar with ROS2, I'm finding often small details like these, that are in no way game breakers, but if left unfixed worsen the experience of our users.

Thanks a lot for the lengthy explanation and for the work you're putting into this. It looks great.

[SteveMacenski]

Totally understand that, and happy to have industry people looking at this work to backport necessary things. But I think we'd both agree we'd rather have an awesome navigation system moving forward with a ba-jillion features than me and my team bogged down in backporting patches to every distribution when there's a question about ABI. For every 1 that someone brings up as a problem, there's 5 that no one really cares about. Picking which ones to spend time on and which not to is difficult (e.g. would you rather have new features requiring backport, or much fewer features at all to even backport).

Many companies are actually using our main branch on their robots - though I understand why someone the size of Pal might not want to do that.

[v-lopez]

100% agree with you.

I think you and your team should not spend time developing many backports, being open to backwards compatible contributions and leaving the work to whoever needs the backport is very reasonable.

We'll consider using main on our robots, but one of our targets is that anyone can "apt install" our robot public simulation and start using it.
In ROS1 they have to prepare huge workspaces and we'd like to steer away from that. But that "apt install" could be on rolling too, I had not considered it.

[SteveMacenski]

We also have docker containers available!

Definitely understand on apt-install.