Obstacle Layer is still current after enable/disable

[bektaskemal]

Bug report

After enabled parameter is updated, layer plugins like static or inflation sets current to false but obstacle layer doesn't? Is it a bug or is there a reason for it?

As a result of this, planner server doesn't wait for costmap update if obstacle layer is disabled/enabled.

https://github.com/ros-navigation/navigation2/blob/9fbae3e66ecc3b7315ca7cbb070468a564f5e111/nav2_costmap_2d/plugins/obstacle_layer.cpp#L311

[padhupradheep]

Hey, it would be nice if you call fill in the issue template as it is given. With the info regarding the distro, robot configs we can try to help you better.

planner server doesn't wait for costmap update if obstacle layer is disabled/enabled.

Do you mean the planner is planning right through the obstacle?

I right now tried placing an obstacle in front of the robot, when the obstacle layer was enabled, it just planned around and when disabled, it was just planning through the obstacle.

[SteveMacenski]

@bektaskemal this is a good point. This is from a time before we had logic that checked if the total-costmap was current if each of the layers were current or the non-current layers were disabled https://github.com/ros-navigation/navigation2/pull/3356.

So, the let you disable without making it non-current which was helpful when you wanted to toggle sources, which is often done in the sensor processing pipelines, but infrequently in the static/inflation layers. I think with the PR I linked to above, we can add in current_ = false where you link to when enabled_ = false and that would work out fine.

Can you open a PR and test that hypothesis?

[bektaskemal]

I quickly tested it and yes adding current_ = false makes it consistent with other layers and the behavior is LayeredCostmap::isCurrent() returns false after a layer is enabled.

But because of the isEnabled check there after disabling a layer LayeredCostmap::isCurrent() still returns true, which arguably might not be desired.

The potential solution I can think of, removing isEnabled check there and setting current true in updateCosts if the layer is not enabled. It should be okay since we set it back to false if enabled in the parameter callback.

if (!enabled_) {
    current_ = true;
    return;
  }

What do you think?

[SteveMacenski]

I'm not totally following your description. If dynamic parameters sets enabled_ = false we should set current = false and vise versa. Since current_ = current_ && ((*plugin)->isCurrent() || !(*plugin)->isEnabled()); at the Layered Costmap, it doesn't really matter and there should be no behavioral changes.

I think we could make that change or just leave it, it doesn't make a functional impact?

[bektaskemal]

So for me the main idea is I want LayeredCostmap::isCurrent() to return false after a layer is enabled or disabled so I can know an update is needed before using the costmap. Does that make sense?

Currently for obstacle layer, isCurrent returns true after either enabled or disabled because current_ is not updated.

If we make current_ false after disabled as you suggested, when we enabled it back isCurrent will return false which is nice.

But my question was can we or should we get the same behavior after it is disabled? Now we still get isCurrent true after a layer disabled because disabled layers are not checked in LayeredCostmap::isCurrent().

[SteveMacenski]

current_ is used to represent whether the data is fresh and not outdated (i.e. sensor has stopped publishing). If you've set enabled_ to false for a layer, then there is no data being processed / layer is unused, so current_ is not relevant.

If once enabled_ is flipped back to true, I could agree that having current_ be false initially until we get our first data in is sensible for safety. So we could set current_ = false when enabled_ is toggled to true, then the first set of data that comes in should change current_ = true once received.