[Draft] CollisionMonitor: add CostmapSource + dataset-based bag test#5642
Conversation
|
@Lotusymt, your PR has failed to build. Please check CI outputs and resolve issues. |
There was a problem hiding this comment.
Checkout Linting / CI jobs that are failing. You may need to pull in main / rebase as well.
Please provide some comments inline in the test files so someone can understand what each are doing -- its a little difficult to read them and understand (1) fully what each do, (2) the difference in what one covers the other doesn't, and (3) the intent
But, by in large, this looks great!
nav2_collision_monitor/include/nav2_collision_monitor/costmap.hpp
Outdated
Show resolved
Hide resolved
nav2_collision_monitor/test/bags/cm_moving_obstacle/fake_cm_bag_source.py
Outdated
Show resolved
Hide resolved
|
Thanks for the detailed review, Steve! |
|
Just so you're aware, I'm leaving tomorrow morning for ROSCon so it might be a little while (a week or so) before I give it another review, but it is not forgotten! |
Signed-off-by: Mengting Yang <mengtiy5@uci.edu>
…st (v1) Signed-off-by: Mengting Yang <mengtiy5@uci.edu>
Signed-off-by: Mengting Yang <mengtiy5@uci.edu>
Signed-off-by: Mengting Yang <mengtiy5@uci.edu>
Signed-off-by: lotusymt <mengtiy5@uci.edu>
b4d7608 to
fc27b4c
Compare
Codecov Report❌ Patch coverage is
... and 20 files with indirect coverage changes 🚀 New features to boost your workflow:
|
|
Hi Steve, |
…e_test.cpp Signed-off-by: lotusymt <mengtiy5@uci.edu>
|
Hi @SteveMacenski, this PR is ready for review, and the follow-up docs PR has been opened: ros-navigation/docs.nav2.org#804. Thanks! |
|
Hi @SteveMacenski — friendly ping. All checks are green; docs in ros-navigation/docs.nav2.org#804. Let me know if you need any changes. Thanks! |
|
hi @Lotusymt, Steve is on vacation and will be back Nov 14 |
nav2_collision_monitor/test/bags/cm_moving_obstacle/fake_cm_bag_source.py
Show resolved
Hide resolved
Co-authored-by: Steve Macenski <stevenmacenski@gmail.com> Signed-off-by: Mengting Yang <87471734+Lotusymt@users.noreply.github.com>
|
@Lotusymt, your PR has failed to build. Please check CI outputs and resolve issues. |
Co-authored-by: Steve Macenski <stevenmacenski@gmail.com> Signed-off-by: Mengting Yang <87471734+Lotusymt@users.noreply.github.com>
Co-authored-by: Steve Macenski <stevenmacenski@gmail.com> Signed-off-by: Mengting Yang <87471734+Lotusymt@users.noreply.github.com>
Signed-off-by: lotusymt <mengtiy5@uci.edu>
Signed-off-by: lotusymt <mengtiy5@uci.edu>
b13d653 to
34dfb73
Compare
Signed-off-by: Mengting Yang <87471734+Lotusymt@users.noreply.github.com> Signed-off-by: lotusymt <mengtiy5@uci.edu>
34dfb73 to
5dd68d7
Compare
|
Hi Steve, I’ve updated the PR to address the issues you pointed out:
Right now, the uncovered lines are mainly the branch that handles malformed If you think we should explicitly cover this failure mode, I’m happy to add a small unit test that publishes invalid data and hits this branch. Please let me know your preference, and I can update the PR accordingly. Thank you! |
That would be nice, yes. It should be easy by sending it a bogus costmap manually in the unit tests. Also, do you think the costmap source should be added to the collision detector node as well? Finally, just need to update the migration guide in ros-navigation/docs.nav2.org#804 and we should be ready to merge! |
…h and collision detector) Signed-off-by: lotusymt <mengtiy5@uci.edu>
|
Thanks, Steve! I’ve added the unit test to cover that branch. I agree it’s more consistent to support CostmapSource in the collision Finally, I’ve updated the migration guide in ros-navigation/docs.nav2.org#804 Please let me know if you’d like any of these changes structured differently. |
|
@Lotusymt, your PR has failed to build. Please check CI outputs and resolve issues. |
|
Got some errors here in the build: Once that is resolved I approve the pair of PRs :-) |
| # distributed under the License is distributed on an "AS IS" BASIS, | ||
| # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| # See the License for the specific language governing permissions and | ||
| # See the License for the specific languazge governing permissions and |
There was a problem hiding this comment.
I think this is unrelated to this PR and needs to be reverted
There was a problem hiding this comment.
Thanks for catching that — I totally agree this change is unrelated to the PR.
I think it was accidentally modified earlier when I was experimenting with a small script to debug some linter issues.
I’ve now reverted the line back to the original license text, and the linter tests still pass.
| output='screen', | ||
| emulate_tty=True, # https://github.com/ros2/launch/issues/188 | ||
| parameters=[{'autostart': autostart}, {'node_names': lifecycle_nodes}], | ||
| parameters=[{'autostart': autostart}, |
| install(FILES collision_monitor_node_bag.yaml | ||
| DESTINATION share/${PROJECT_NAME}/test) | ||
|
|
||
| find_package(GTest REQUIRED) |
There was a problem hiding this comment.
Can we use ament_add_gtest instead?
There was a problem hiding this comment.
Thanks for pointing this out!
I kept this as an add_executable on purpose: the binary is a gtest-based helper that’s launched from collision_monitor_node_bag.launch.py.
If we register it with ament_add_gtest, colcon test / CTest would try to run it standalone without that launch setup, so it would block waiting for /clock and /cmd_vel or fail with no data.
There was a problem hiding this comment.
Ah, I think we can use ament_add_gtest_executable and ament_add_test instead,
navigation2/nav2_lifecycle_manager/test/CMakeLists.txt
Lines 20 to 36 in 60f3db2
There was a problem hiding this comment.
Good point, thank you for pointing that out! Your suggestion would definitely be more consistent with the existing style. Since this PR is already merged, I’ll keep the current version for now, but I’ll keep this pattern in mind for future tests.
| // Cost threshold (0–255). 253 = inscribed 254 = lethal; 255 = NO_INFORMATION. | ||
| const auto thresh_name = source_name_ + ".cost_threshold"; | ||
| // Minimal change (no range descriptor) | ||
| int v = node->declare_or_get_parameter<int>(thresh_name, 253); // declare if missing, else get |
There was a problem hiding this comment.
Since there is a default value, I don't think the template argument <int> is necessary here
dbd5e04 to
7b6a842
Compare
Signed-off-by: lotusymt <mengtiy5@uci.edu>
7b6a842 to
a8e912f
Compare
…os-navigation#5642) * Collision Monitor: add costmap sourced(unit tests) Signed-off-by: Mengting Yang <mengtiy5@uci.edu> * nav2_collision_monitor: modified CostmapSource + dataset-based bag test (v1) Signed-off-by: Mengting Yang <mengtiy5@uci.edu> * modified: nav2_collision_monitor/README.md Signed-off-by: Mengting Yang <mengtiy5@uci.edu> * - annotate files to indicate AI-assisted drafts Signed-off-by: Mengting Yang <mengtiy5@uci.edu> * collision_monitor: fix linter and address issues in the comment Signed-off-by: lotusymt <mengtiy5@uci.edu> * nav2_collision_monitor: fix lint, coverage, and collision_monitor_node_test.cpp Signed-off-by: lotusymt <mengtiy5@uci.edu> * Update nav2_collision_monitor/src/costmap.cpp Co-authored-by: Steve Macenski <stevenmacenski@gmail.com> Signed-off-by: Mengting Yang <87471734+Lotusymt@users.noreply.github.com> * Update nav2_collision_monitor/src/costmap.cpp Co-authored-by: Steve Macenski <stevenmacenski@gmail.com> Signed-off-by: Mengting Yang <87471734+Lotusymt@users.noreply.github.com> * Update nav2_collision_monitor/src/costmap.cpp Co-authored-by: Steve Macenski <stevenmacenski@gmail.com> Signed-off-by: Mengting Yang <87471734+Lotusymt@users.noreply.github.com> * Reverse format change Signed-off-by: lotusymt <mengtiy5@uci.edu> * Reverse format changes and comments Signed-off-by: lotusymt <mengtiy5@uci.edu> * Refactor in_range and count_stopped functions Signed-off-by: Mengting Yang <87471734+Lotusymt@users.noreply.github.com> Signed-off-by: lotusymt <mengtiy5@uci.edu> * added costmap source to collision detector and tests( for failure path and collision detector) Signed-off-by: lotusymt <mengtiy5@uci.edu> * fix configuration and test bugs Signed-off-by: lotusymt <mengtiy5@uci.edu> --------- Signed-off-by: Mengting Yang <mengtiy5@uci.edu> Signed-off-by: lotusymt <mengtiy5@uci.edu> Signed-off-by: Mengting Yang <87471734+Lotusymt@users.noreply.github.com> Co-authored-by: Steve Macenski <stevenmacenski@gmail.com>
…os-navigation#5642) * Collision Monitor: add costmap sourced(unit tests) Signed-off-by: Mengting Yang <mengtiy5@uci.edu> * nav2_collision_monitor: modified CostmapSource + dataset-based bag test (v1) Signed-off-by: Mengting Yang <mengtiy5@uci.edu> * modified: nav2_collision_monitor/README.md Signed-off-by: Mengting Yang <mengtiy5@uci.edu> * - annotate files to indicate AI-assisted drafts Signed-off-by: Mengting Yang <mengtiy5@uci.edu> * collision_monitor: fix linter and address issues in the comment Signed-off-by: lotusymt <mengtiy5@uci.edu> * nav2_collision_monitor: fix lint, coverage, and collision_monitor_node_test.cpp Signed-off-by: lotusymt <mengtiy5@uci.edu> * Update nav2_collision_monitor/src/costmap.cpp Co-authored-by: Steve Macenski <stevenmacenski@gmail.com> Signed-off-by: Mengting Yang <87471734+Lotusymt@users.noreply.github.com> * Update nav2_collision_monitor/src/costmap.cpp Co-authored-by: Steve Macenski <stevenmacenski@gmail.com> Signed-off-by: Mengting Yang <87471734+Lotusymt@users.noreply.github.com> * Update nav2_collision_monitor/src/costmap.cpp Co-authored-by: Steve Macenski <stevenmacenski@gmail.com> Signed-off-by: Mengting Yang <87471734+Lotusymt@users.noreply.github.com> * Reverse format change Signed-off-by: lotusymt <mengtiy5@uci.edu> * Reverse format changes and comments Signed-off-by: lotusymt <mengtiy5@uci.edu> * Refactor in_range and count_stopped functions Signed-off-by: Mengting Yang <87471734+Lotusymt@users.noreply.github.com> Signed-off-by: lotusymt <mengtiy5@uci.edu> * added costmap source to collision detector and tests( for failure path and collision detector) Signed-off-by: lotusymt <mengtiy5@uci.edu> * fix configuration and test bugs Signed-off-by: lotusymt <mengtiy5@uci.edu> --------- Signed-off-by: Mengting Yang <mengtiy5@uci.edu> Signed-off-by: lotusymt <mengtiy5@uci.edu> Signed-off-by: Mengting Yang <87471734+Lotusymt@users.noreply.github.com> Co-authored-by: Steve Macenski <stevenmacenski@gmail.com> Signed-off-by: Decwest <fumiyaonishi1016@gmail.com>
|
Hi all, I'm wondering if this feature will get backported to ROS Jazzy? Thanks! |
|
@arjangupta feel free to open a backport PR and I can review/merge relatively quickly |
…y-pick --continue --no-edit CollisionMonitor: add CostmapSource + dataset-based bag test (ros-navigation#5642) * Collision Monitor: add costmap sourced(unit tests) Signed-off-by: Mengting Yang <mengtiy5@uci.edu> * nav2_collision_monitor: modified CostmapSource + dataset-based bag test (v1) Signed-off-by: Mengting Yang <mengtiy5@uci.edu> * modified: nav2_collision_monitor/README.md Signed-off-by: Mengting Yang <mengtiy5@uci.edu> * - annotate files to indicate AI-assisted drafts Signed-off-by: Mengting Yang <mengtiy5@uci.edu> * collision_monitor: fix linter and address issues in the comment Signed-off-by: lotusymt <mengtiy5@uci.edu> * nav2_collision_monitor: fix lint, coverage, and collision_monitor_node_test.cpp Signed-off-by: lotusymt <mengtiy5@uci.edu> * Update nav2_collision_monitor/src/costmap.cpp Co-authored-by: Steve Macenski <stevenmacenski@gmail.com> Signed-off-by: Mengting Yang <87471734+Lotusymt@users.noreply.github.com> * Update nav2_collision_monitor/src/costmap.cpp Co-authored-by: Steve Macenski <stevenmacenski@gmail.com> Signed-off-by: Mengting Yang <87471734+Lotusymt@users.noreply.github.com> * Update nav2_collision_monitor/src/costmap.cpp Co-authored-by: Steve Macenski <stevenmacenski@gmail.com> Signed-off-by: Mengting Yang <87471734+Lotusymt@users.noreply.github.com> * Reverse format change Signed-off-by: lotusymt <mengtiy5@uci.edu> * Reverse format changes and comments Signed-off-by: lotusymt <mengtiy5@uci.edu> * Refactor in_range and count_stopped functions Signed-off-by: Mengting Yang <87471734+Lotusymt@users.noreply.github.com> Signed-off-by: lotusymt <mengtiy5@uci.edu> * added costmap source to collision detector and tests( for failure path and collision detector) Signed-off-by: lotusymt <mengtiy5@uci.edu> * fix configuration and test bugs Signed-off-by: lotusymt <mengtiy5@uci.edu> --------- Signed-off-by: Mengting Yang <mengtiy5@uci.edu> Signed-off-by: lotusymt <mengtiy5@uci.edu> Signed-off-by: Mengting Yang <87471734+Lotusymt@users.noreply.github.com> Co-authored-by: Steve Macenski <stevenmacenski@gmail.com>
…igation#5642) * Collision Monitor: add costmap sourced(unit tests) Signed-off-by: Mengting Yang <mengtiy5@uci.edu> * nav2_collision_monitor: modified CostmapSource + dataset-based bag test (v1) Signed-off-by: Mengting Yang <mengtiy5@uci.edu> * modified: nav2_collision_monitor/README.md Signed-off-by: Mengting Yang <mengtiy5@uci.edu> * - annotate files to indicate AI-assisted drafts Signed-off-by: Mengting Yang <mengtiy5@uci.edu> * collision_monitor: fix linter and address issues in the comment Signed-off-by: lotusymt <mengtiy5@uci.edu> * nav2_collision_monitor: fix lint, coverage, and collision_monitor_node_test.cpp Signed-off-by: lotusymt <mengtiy5@uci.edu> * Update nav2_collision_monitor/src/costmap.cpp Co-authored-by: Steve Macenski <stevenmacenski@gmail.com> Signed-off-by: Mengting Yang <87471734+Lotusymt@users.noreply.github.com> * Update nav2_collision_monitor/src/costmap.cpp Co-authored-by: Steve Macenski <stevenmacenski@gmail.com> Signed-off-by: Mengting Yang <87471734+Lotusymt@users.noreply.github.com> * Update nav2_collision_monitor/src/costmap.cpp Co-authored-by: Steve Macenski <stevenmacenski@gmail.com> Signed-off-by: Mengting Yang <87471734+Lotusymt@users.noreply.github.com> * Reverse format change Signed-off-by: lotusymt <mengtiy5@uci.edu> * Reverse format changes and comments Signed-off-by: lotusymt <mengtiy5@uci.edu> * Refactor in_range and count_stopped functions Signed-off-by: Mengting Yang <87471734+Lotusymt@users.noreply.github.com> Signed-off-by: lotusymt <mengtiy5@uci.edu> * added costmap source to collision detector and tests( for failure path and collision detector) Signed-off-by: lotusymt <mengtiy5@uci.edu> * fix configuration and test bugs Signed-off-by: lotusymt <mengtiy5@uci.edu> --------- Signed-off-by: Mengting Yang <mengtiy5@uci.edu> Signed-off-by: lotusymt <mengtiy5@uci.edu> Signed-off-by: Mengting Yang <87471734+Lotusymt@users.noreply.github.com> Co-authored-by: Steve Macenski <stevenmacenski@gmail.com> Co-authored-by: Arjan Gupta <arjangupta95@gmail.com>
@SteveMacenski , here you go! Somehow exactly 300 PRs after this one 😄 @Lotusymt @mini-1235 if you could also review, that would be awesome! |
…igation#5642) * Collision Monitor: add costmap sourced(unit tests) Signed-off-by: Mengting Yang <mengtiy5@uci.edu> * nav2_collision_monitor: modified CostmapSource + dataset-based bag test (v1) Signed-off-by: Mengting Yang <mengtiy5@uci.edu> * modified: nav2_collision_monitor/README.md Signed-off-by: Mengting Yang <mengtiy5@uci.edu> * - annotate files to indicate AI-assisted drafts Signed-off-by: Mengting Yang <mengtiy5@uci.edu> * collision_monitor: fix linter and address issues in the comment Signed-off-by: lotusymt <mengtiy5@uci.edu> * nav2_collision_monitor: fix lint, coverage, and collision_monitor_node_test.cpp Signed-off-by: lotusymt <mengtiy5@uci.edu> * Update nav2_collision_monitor/src/costmap.cpp Co-authored-by: Steve Macenski <stevenmacenski@gmail.com> Signed-off-by: Mengting Yang <87471734+Lotusymt@users.noreply.github.com> * Update nav2_collision_monitor/src/costmap.cpp Co-authored-by: Steve Macenski <stevenmacenski@gmail.com> Signed-off-by: Mengting Yang <87471734+Lotusymt@users.noreply.github.com> * Update nav2_collision_monitor/src/costmap.cpp Co-authored-by: Steve Macenski <stevenmacenski@gmail.com> Signed-off-by: Mengting Yang <87471734+Lotusymt@users.noreply.github.com> * Reverse format change Signed-off-by: lotusymt <mengtiy5@uci.edu> * Reverse format changes and comments Signed-off-by: lotusymt <mengtiy5@uci.edu> * Refactor in_range and count_stopped functions Signed-off-by: Mengting Yang <87471734+Lotusymt@users.noreply.github.com> Signed-off-by: lotusymt <mengtiy5@uci.edu> * added costmap source to collision detector and tests( for failure path and collision detector) Signed-off-by: lotusymt <mengtiy5@uci.edu> * fix configuration and test bugs Signed-off-by: lotusymt <mengtiy5@uci.edu> --------- Signed-off-by: Mengting Yang <mengtiy5@uci.edu> Signed-off-by: lotusymt <mengtiy5@uci.edu> Signed-off-by: Mengting Yang <87471734+Lotusymt@users.noreply.github.com> Co-authored-by: Steve Macenski <stevenmacenski@gmail.com> Co-authored-by: Arjan Gupta <arjangupta95@gmail.com> Signed-off-by: Arjan Gupta <arjangupta@vermeer.com>
Basic Info
Description of contribution in a few bullet points
CostmapSourceto Collision Monitor to allow collision checks against a subscribed local costmap (nav2_msgs/Costmap).CostmapSourceintoCollisionMonitor::configureSources()and expose params:*.topic,*.cost_threshold(0–255),*.treat_unknown_as_obstacle(bool).params/collision_monitor_params.yaml.README.md:test/collision_monitor_node_bag.cpp(metrics: time-to-stop, hold-stop%, time-to-resume, false-stop%).test/collision_monitor_node_bag.launch.pyto bring up CM, play a tiny bag, and run gtest.get_package_share_directory().Description of documentation updates required from your changes
.. warning::box on the Collision Monitor page describing the trade-offs of using a costmap source (persistence vs. latency/staleness), and list the new parameters.README.mdalready updated with a brief note and source entry.Description of how this change was tested
colcon test --packages-select nav2_collision_monitor; existing unit tests + the new launch test pass on Ubuntu.Future work that may be required in bullet points
For Maintainers:
backport-*.