Bugfix inactive publishers

[pele1410]

Basic Info

Info	Please fill out this column
Ticket(s) this addresses	#5710
Primary OS tested on	Ubuntu 24.04 docker container
Robotic platform tested on	Gazebo simulation of a USV
Does this PR contain AI generated software?	No
Was this PR description generated by AI software?	No

---

Description of contribution in a few bullet points

Prevent the costmap_2d_publisher from trying to publish when it is not active.

Description of documentation updates required from your changes

N/A

Description of how this change was tested

Replicated the steps to produce the issue in #5710 and did not see the issue.

---

Future work that may be required in bullet points

This is not likely the root cause for the out-of-bounds index access but:

• it seems logically correct to prevent publication unless active
• it has the affect of side-stepping the issue

For Maintainers:

• Check that any new parameters added are updated in docs.nav2.org
• Check that any significant change is added to the migration guide
• Check that any new features OR changes to existing behaviors are reflected in the tuning guide
• Check that any new functions have Doxygen added
• Check that any new features have test coverage
• Check that any new plugins is added to the plugins page
• If BT Node, Additionally: add to BT's XML index of nodes for groot, BT package's readme table, and BT library lists
• Should this be backported to current distributions? If so, tag with backport-*.

[pele1410]

Updated against main branch instead of jazzy

[SteveMacenski]

Suggested change

	if(!active_) {
	if (!active_) {

[SteveMacenski]

Where is active_ ever set to true?

[pele1410]

That is a good question. I naively thought it was already used and working. Will look into this.

[pele1410]

Where is active_ ever set to true?

Ok, looking at the active_ variable, it is definitely never set. The doxy says it's based on the update frequency, but that's not a property of the costmap it's a property of the Costmap2DROS object. I don't know if this was a property that was moved at some point in time and active_ is just leftover cruft or if it's some unimplemented feature.

I see there are 2 options:

1. Just yank active_ and active() from the Costmap2DPublisher
2. Add a setter to so Costmap2DPublisher can set active_. This could be directly (void set_active(bool active)) or indirectly (void set_publish_rate (double rate)). The former seems to make more sense given there is no concept of a rate in the Costmap2DPublisher

I'm inclined to think this is just cruft, so should go with option 1. Up to you guys.

[pele1410]

I guess there is a 3rd option where the Costmap2DROS or Costmap2DPublisher integrate the actual enabled param for each layer into the decision to publish. I need to look into how that param gets plumbed through to see if it's available to use in the Costmap2DROS

Edit: I don't think this will work. I worked up a stub change where the Costmap2DROS simply doesn't create the publishers if the plugin isn't enabled; but that happens in on_configure stage and would be fine except most layers allow dynamic params which would mean the layer could get an updated "enable" value but the Costmap2DROS would have no way (without additional plumbing) to suddenly create a publisher for it.

Putting the check lower in Costmap2DROS::mapUpdateLoop there isn't any association with the layer Publisher and layer itself; and the latter is what has the isEnabled() flag.

The only way I can see to implement this functionality is to

1. Remove active as a concept from the Costmap2DPublisher
2. Pass a weak_pointer of the Layer to the Costmap2DPublisher so it can check on each publishCostmap call whether the layer is enabled or not.

[SteveMacenski]

@pele1410 any update?

[SteveMacenski]

@pele1410 hey any updates? We'd love this in before the holiday season to knock this out

[pele1410]

Sorry, got busy at work and haven't had a chance to circle back to this. Hoping to look into it over the Christmas break

[pele1410]

Figured out that I could use the costmap_ object, so I went with slightly different implementation of choice 3

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

Files with missing lines	Coverage Δ
...d/include/nav2_costmap_2d/costmap_2d_publisher.hpp	100.00% <ø> (ø)
nav2_costmap_2d/src/costmap_2d_publisher.cpp	85.71% <100.00%> (+0.09%)	⬆️

... and 5 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[mini-1235]

LGTM functionally, just remove the extra scope

[pele1410]

Do I need to specifically request for this to be backported to Kilted? Or should I do it myself?

[mini-1235]

Hmm, I believe this would break the ABI since the active_ member variable is being removed. We’d be happy to accept the PR if you could backport just the following logic manually instead:

auto const costmap_layer = dynamic_cast<CostmapLayer *>(costmap_);
  if (costmap_layer != nullptr && !costmap_layer->isEnabled()) {
    return;
  }

[pele1410]

Hmm, I believe this would break the ABI since the active_ member variable is being removed. We’d be happy to accept the PR if you could backport just the following logic manually instead:

auto const costmap_layer = dynamic_cast<CostmapLayer *>(costmap_);
  if (costmap_layer != nullptr && !costmap_layer->isEnabled()) {
    return;
  }

Can do. Is there a deprecation marking you would like added to active_ (and associated method) since it was removed in main?

[mini-1235]

Can do. Is there a deprecation marking you would like added to active_ (and associated method) since it was removed in main?

No, at least for now we don't do that in Nav2. It's something we might consider in the future