Fix InflationLayer resource locking problem (#1931)

[daisukes]

Basic Info

Info	Please fill out this column
Ticket(s) this addresses	#1931
Primary OS tested on	Ubuntu 20.04
Robotic platform tested on	tb3

---

Description of contribution in a few bullet points

• fix resource locking problem of InflationLayer

Description of documentation updates required from your changes

N/A

[codecov]

Codecov Report

Merging #1936 into master will decrease coverage by 0.21%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1936      +/-   ##
==========================================
- Coverage   76.26%   76.05%   -0.22%     
==========================================
  Files         224      224              
  Lines       10951    10957       +6     
==========================================
- Hits         8352     8333      -19     
- Misses       2599     2624      +25     

Impacted Files	Coverage Δ
...map_2d/include/nav2_costmap_2d/inflation_layer.hpp	100.00% <100.00%> (ø)
nav2_costmap_2d/plugins/inflation_layer.cpp	99.39% <100.00%> (+0.01%)	⬆️
...av2_dwb_controller/dwb_critics/src/oscillation.cpp	69.87% <0.00%> (-19.28%)	⬇️
...v2_recoveries/include/nav2_recoveries/recovery.hpp	86.07% <0.00%> (-6.33%)	⬇️
nav2_recoveries/plugins/spin.cpp	89.47% <0.00%> (-5.27%)	⬇️
nav2_amcl/src/amcl_node.cpp	84.41% <0.00%> (-0.16%)	⬇️
...lifecycle_manager/src/lifecycle_manager_client.cpp	90.24% <0.00%> (+2.43%)	⬆️

---

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 909c6f2...e524132. Read the comment docs.

[SteveMacenski]

I don't think you need either the object or the method, its already provided for you since you derive from the costmap layer class which has a costmap2D in the inheritance tree https://github.com/ros-planning/navigation2/blob/aaa97902c1e1bb9a509c4ee0d88b880afb528f20/nav2_costmap_2d/include/nav2_costmap_2d/costmap_2d.hpp#L314

[daisukes]

I thought so first, but actually InflationLayer is not Costmap2D but just a Layer.
The other three layers (Obstacle, Voxel, Static) are derived from CostmapLayer which derived from Layer and Costmap2D. I also considered moving the mutex things from Costmap2D to Layer but they are used in LayeredCostmap which just uses Costmap2D as a private member.

So I needed to redefine them.
One possible alternative is having mutex things in LayeredCosmap and Layer for future additional plugins.
What do you think?

[SteveMacenski]

Huh got it, thanks. @AlexeyMerzlyakov maybe the costmap filter base class should just be a layer and not a costmap layer, then you won't have that pesky costmap_ internally https://github.com/ros-planning/navigation2/pull/1882/files. I think where we left it is we'd have a mask_costmap_ or similar so we could just get rid of that in general.

Thanks for that note, I hadn't recognized that subtly.

[AlexeyMerzlyakov]

maybe the costmap filter base class should just be a layer and not a costmap layer

Good point to have in CostmapFilter class from #1882 in order to avoid costmap_ resource potential mishandle in the future. I've added locally similar access_ into that class and changed inheritance and it looks to work fine.

BTW, from first sight it looks like CostmapLayer class is not locking its resources when making any work with them (e.g. during the updateWithMax(), updateWithOverwrite(), etc... routines). Why do not add working with the access lock into CostmapLayer as well?

Finally, this PR was merged, but I can not find any commit in the master/main branch related to. Am I wrong, or is it some merge problem?

[SteveMacenski]

You don't need the locking in update functions for costmap filters necessarily. Inflation needs it because it has an async callback function editing a cache table that is also used by update (that could be called at the same time). I think how we've designed the costmap filters layer, there's no chance for that because we don't even allow it to update anything until after we've received something over the topic and set the variable. We should though look over it again and verify that is true. The update with XYZ doesn't need a lock because we're updating the master_grid which is only edited by the plugins, in a single threaded order. The locks are to protect the local costmap_ isntances that can be edited by multiple things if there are callback functions being processed in the active state (like inflation, static, voxel, obstacles)

[AlexeyMerzlyakov]

OK, I've got your point. This indeed will be excessive.
However, I still have some doubts about CostmapLayer class: if compare it with Costmap2D class (from which it was inherited), we can see that there some work with costmap_ methods are being protected by a mutex (e.g. Costmap2D::resetMaps() or Costmap2D::resizeMap()). However, I can not see any callbacks inside Costmap2D; it looks like a costmap_ is being protected for async calls of above API methods from outside. Maybe there is some other intention why the mutex is added in Costmap2D only, I can not get it yet, but for now it seems little bit unclear.

[SteveMacenski]

Huh got it, thanks. @AlexeyMerzlyakov maybe the costmap filter base class should just be a layer and not a costmap layer, then you won't have that pesky costmap_ internally https://github.com/ros-planning/navigation2/pull/1882/files. I think where we left it is we'd have a mask_costmap_ or similar so we could just get rid of that in general.

Thanks for that note, I hadn't recognized that subtly.

[daisukes]

@SteveMacenski

As @AlexeyMerzlyakov mentioned, this PR has not been correctly merged into the main branch.
I think this is because of the change in the main branch name recently.
Can you deal with it here or do I need to make another PR?

[SteveMacenski]

That's really weird, I thought we synced things. Please resubmit this in a new PR to main. I'll look and see if there are other PRs like this I'll need to resolve.