Move locks from move_base into individual planners

[AlexReimann]

During the execute cycle move base locks the local costmap in the controlling state without actually accessing the costmap.

What's the reasoning behind this?

It seems kind of to be done in the case the local planner wants to do something with the local costmap in the computeVelocityCommands function. This seems to be overkill and maybe restricting the performance.

In our case the computeVelocityCmmands sometimes takes a while, but we don't keep the updating of the local costmap locked during that time.

So.. could it be removed?

[AlexReimann]

fyi @stonier

[corot]

Same with the global planner. I think it makes sense to lock the costmaps when about to read them in the planners, so you don't read them right when they are cleaned (I have seen this happens often), but that should be the responsibility of the planners themselves, done in the right moment to minimize the blocking time.

[stonier]

+1

[mikeferguson]

To a large extent, the default planners all assume a consistent world state throughout a planning cycle. I concur that it probably should be in the planner itself, but that is made slightly difficult because while we can update all the default planners, we don't have the ability to update all the planners out there that may be relying on our costmap locking in move_base.

I would suggest that if we want to change this, we only do it in lunar and above if we do it very soon, or we do it in a later release if we cannot fix it ASAP. That limits the impact of the change on operational systems.

[mikeferguson]

Adding help wanted to note that maintainers do not currently plan to tackle this, but PRs are welcome!

[corot]

FYI, on the new move base flex, we plan to drop this locking and advice plugin developers to adapt their code.
As a showcase of why we think this is a good thing, the sbpl_lattice_planner can take an arbitrary time to compute a plan (configurable), while it just need some tens of milliseconds to make his own copy of the global costmap.

[mikeferguson]

I've changed the title of this ticket to the actionable fix we want to implement

[iluetkeb]

Can I ask for some clarification on the motivation here, please?

Firstly, it's the move_base node which combines perception and planning. Therefore, it is the place that knows whether there is a possibility for conflict on shared resources, and thus it should do the locking. It's a fairly easy mod to make the locking go away completely, for example.

Secondly, I don't see the point. @AlexReimann wrote this "may be impacting performance". What is meant by that? Given that the planner is the only consumer of the costmap, locking caused by the planner itself impacts nobody else.

@corot wrote that the global planner can take very long and that it's cheaper to copy the costmap. Firstly, that depends on the size of global costmap -- when it's large, copying is significant cost. We regularly run move_base in environments of 500mx500m at 10cm resolution. That's about 23MB of data per layer. Moreover, I again wonder what the problem is -- nobody but the global planner uses the costmap, so why worry that it's not being updated? Are you concerned with the CPU-spike due to batching the updates?