[collision_monitor] Add temporal debounce for min_points trigger by Neomelt · Pull Request #6006 · ros-navigation/navigation2

Neomelt · 2026-03-05T13:13:53Z

Basic Info

Info	Please fill out this column
Ticket(s) this addresses	#4364
Primary OS tested on	Ubuntu 24.04 (ROS 2 Jazzy)
Robotic platform tested on	bag replay (Livox pointcloud dataset), no physical robot yet
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

Adds temporal debounce / hysteresis to polygon min_points trigger evaluation in nav2_collision_monitor
Introduces two per-polygon parameters with backward-compatible defaults:
- trigger_consecutive_points (default 1)
- release_consecutive_points (default 1)
Integrates the debounce decision into processStopSlowdownLimit() for STOP / SLOWDOWN / LIMIT
Keeps previous behavior unchanged when both parameters remain 1
Adds runtime parameter update handling for these new parameters and resets internal debounce state on updates

Description of documentation updates required from your changes

Add parameter docs for:
- trigger_consecutive_points
- release_consecutive_points
Mention temporal debounce tuning guidance in collision monitor tuning docs (recommended baseline from this test: start with 3/3)

Description of how this change was tested

Build validation (Jazzy overlay):
- colcon build --packages-up-to nav2_collision_monitor --symlink-install --cmake-args -DBUILD_TESTING=OFF
Replay validation against issue scenario (same bag and runtime setup per run):
- bag: /home/neomelt/Downloads/indoor_2min_bag/
- source topic: /livox/lidar (pointcloud)
- command input: dedicated /cm_cmd_vel_stamped
- static TF added for replay wiring:
  - odom -> base_footprint
  - base_footprint -> livox_frame
Measured action_type switching count from /collision_monitor_state:
- 1/1: samples=16, switches=15
- 3/3: samples=10, switches=9
- 4/4: samples=10, switches=9
- 5/5: samples=10, switches=9
- 2/4: samples=15, switches=14
- 2/5: samples=15, switches=14
- 3/5: samples=10, switches=9
Summary: 3/3 reduces toggling versus baseline (15 -> 9, ~40% reduction in this replay)

Future work that may be required in bullet points

Add focused unit tests for debounce state transitions (trigger/release counters and reset behavior)
Extend validation on additional bags and a full nav stack setup (non-injected TF path)
Add physical robot validation to tune defaults per sensor modality

Closes #4364

Signed-off-by: Xlqmu <3212929002@qq.com>

codecov · 2026-03-05T13:46:05Z

Codecov Report

❌ Patch coverage is 96.29630% with 2 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
nav2_collision_monitor/src/polygon.cpp	96.15%	2 Missing ⚠️

Files with missing lines	Coverage Δ
..._collision_monitor/src/collision_detector_node.cpp	`97.26% <100.00%> (-0.04%)`	⬇️
...2_collision_monitor/src/collision_monitor_node.cpp	`97.01% <100.00%> (ø)`
nav2_collision_monitor/src/polygon.cpp	`96.89% <96.15%> (-0.15%)`	⬇️

... 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.

Signed-off-by: Xlqmu <3212929002@qq.com>

mini-1235

@gennartan would you mind taking a look and doing a review here?

Neomelt · 2026-03-09T01:48:02Z

btw,this is the bag I used for testing.
https://drive.google.com/file/d/1GYbeE3m7auEuSuT3_kBtzUTpf-1Lf6uU/view?usp=drive_link

gennartan

Overall, everything seems to be working correctly on my side and the changes look good 👍

I only left two small comments that should help the PR integrate a bit better with the rest of the project.

nav2_collision_monitor/src/polygon.cpp

nav2_collision_monitor/include/nav2_collision_monitor/polygon.hpp

Signed-off-by: Xlqmu <3212929002@qq.com>

Neomelt · 2026-03-09T11:27:37Z

Thanks, updated and pushed. @gennartan

Signed-off-by: Xlqmu <3212929002@qq.com>

gennartan

Seems good to me

Neomelt · 2026-03-13T23:59:03Z

Hi @SteveMacenski, gentle ping when you have a moment 🙏
This PR has an approval, requested updates were addressed, and CI is green.
Would you mind taking a final look for merge?
Happy to make any follow-up changes if needed. Thanks!

SteveMacenski · 2026-03-15T20:53:50Z

Review now - sorry for the delay, I've been slammed and trying to get to as much as I can each day :-)

SteveMacenski

This looks like a few requested changes, but really this is mostly pedantic for conciseness (extra work/complexity without functional difference) with a couple of small correction errors. Overall this is essentially perfect!

nav2_collision_monitor/src/collision_monitor_node.cpp

SteveMacenski · 2026-03-15T20:56:50Z

nav2_collision_monitor/src/collision_monitor_node.cpp

  }

-  if (polygon->getPointsInside(sources_collision_points_map) >= polygon->getMinPoints()) {
+  const int points_inside = polygon->getPointsInside(sources_collision_points_map);


Question (not asking for the change necessarily): Does it make sense potentially to have the getPointsInside now be an internal detail of an API like isTriggered() rather than externally getting the points in this application? Seems unimportant now to have points_inside in the code of the monitor/detector nodes if we're moving the triggering logic internal to the polygons

@Neomelt can you respond to my question and if you think that is better?

You’re right — sorry for the earlier brief reply.I agree that direction is better long-term: encapsulating getPointsInside() behind a trigger-oriented API would clean up monitor/detector logic.For this PR I kept scope focused on debounce and lifecycle/state consistency, maybe I can open a focused follow-up refactor PR for that API cleanup.

Would it be possible to do it here? Rename isTriggeredByPoints to isTriggered and get the getPointsInside first thing in the function for use.

nav2_collision_monitor/src/polygon.cpp

Signed-off-by: Xlqmu <3212929002@qq.com>

Neomelt · 2026-03-16T06:40:40Z

@SteveMacenski Thanks a lot for the detailed review — I applied your suggestions and pushed some changes.I also reran the tests on Jazzy locally and everything is green.When you get a chance, could you take another look at the PR？

SteveMacenski · 2026-03-16T17:22:58Z

@Neomelt approved minus the 2 items I reopened and tagged you in from the last review

…n API Signed-off-by: Neomelt <niezhenghua2004@gmail.com>

Neomelt · 2026-03-18T02:11:14Z

Pushed follow-up in 56bd139a to address the API encapsulation request (isTriggered + internalized getPointsInside) and updated call sites/tests accordingly.\n\nAlso updated docs in ros-navigation/docs.nav2.org#888 (detector page follow-up in 24cc271).\n\nCould you take another quick look when you have a moment? 🙏

Neomelt · 2026-03-18T04:10:19Z

Thanks for the direct and detailed feedback throughout this PR.
I appreciate the high bar — it helped me improve both the implementation and how I communicate design choices.🙏

SteveMacenski · 2026-03-18T04:18:48Z

Anytime and thanks for the help! This is a great feature to have :-)

feat(collision_monitor): add temporal debounce for min_points triggers

7bf7b7a

Signed-off-by: Xlqmu <3212929002@qq.com>

Neomelt added 2 commits March 5, 2026 21:55

test(collision_monitor): cover debounce trigger/release behavior

c891a2a

Signed-off-by: Xlqmu <3212929002@qq.com>

style(collision_monitor): satisfy uncrustify for debounce tests

c1e4668

Signed-off-by: Xlqmu <3212929002@qq.com>

mini-1235 reviewed Mar 8, 2026

View reviewed changes

gennartan reviewed Mar 9, 2026

View reviewed changes

nav2_collision_monitor/src/polygon.cpp Outdated Show resolved Hide resolved

nav2_collision_monitor/include/nav2_collision_monitor/polygon.hpp Show resolved Hide resolved

Refine trigger parameter validation and state initialization

737630a

Signed-off-by: Xlqmu <3212929002@qq.com>

Add coverage for trigger parameter validation

e2b2f23

Signed-off-by: Xlqmu <3212929002@qq.com>

gennartan approved these changes Mar 12, 2026

View reviewed changes

SteveMacenski reviewed Mar 15, 2026

View reviewed changes

Neomelt force-pushed the fix/4364-temporal-min-points branch 2 times, most recently from ff6198f to 46c2524 Compare March 16, 2026 06:20

refactor(collision_monitor): simplify debounce and reset lifecycle state

f27c4fb

Signed-off-by: Xlqmu <3212929002@qq.com>

Neomelt force-pushed the fix/4364-temporal-min-points branch from 46c2524 to f27c4fb Compare March 16, 2026 06:29

refactor(collision_monitor): encapsulate trigger evaluation in polygo…

56bd139

…n API Signed-off-by: Neomelt <niezhenghua2004@gmail.com>

SteveMacenski approved these changes Mar 18, 2026

View reviewed changes

SteveMacenski merged commit 7e657bf into ros-navigation:main Mar 18, 2026
18 checks passed

Conversation

Neomelt commented Mar 5, 2026

Basic Info

Description of contribution in a few bullet points

Description of documentation updates required from your changes

Description of how this change was tested

Future work that may be required in bullet points

Uh oh!

codecov bot commented Mar 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

mini-1235 left a comment

Choose a reason for hiding this comment

Uh oh!

Neomelt commented Mar 9, 2026

Uh oh!

gennartan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Neomelt commented Mar 9, 2026

Uh oh!

gennartan left a comment

Choose a reason for hiding this comment

Uh oh!

Neomelt commented Mar 13, 2026

Uh oh!

SteveMacenski commented Mar 15, 2026

Uh oh!

SteveMacenski left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

SteveMacenski Mar 15, 2026

Choose a reason for hiding this comment

Uh oh!

Neomelt Mar 16, 2026

Choose a reason for hiding this comment

Uh oh!

SteveMacenski Mar 16, 2026

Choose a reason for hiding this comment

Uh oh!

Neomelt Mar 17, 2026

Choose a reason for hiding this comment

Uh oh!

SteveMacenski Mar 17, 2026

Choose a reason for hiding this comment

Uh oh!

Neomelt Mar 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Neomelt commented Mar 16, 2026

Uh oh!

SteveMacenski commented Mar 16, 2026

Uh oh!

Neomelt commented Mar 18, 2026

Uh oh!

Uh oh!

Neomelt commented Mar 18, 2026

Uh oh!

SteveMacenski commented Mar 18, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

codecov bot commented Mar 5, 2026 •

edited

Loading

SteveMacenski left a comment •

edited

Loading

Neomelt Mar 18, 2026 •

edited

Loading