Skip to content

Comments

Fix 3744#3745

Merged
SteveMacenski merged 11 commits intoros-navigation:mainfrom
nakai-omer:fix-3744
Sep 11, 2023
Merged

Fix 3744#3745
SteveMacenski merged 11 commits intoros-navigation:mainfrom
nakai-omer:fix-3744

Conversation

@nakai-omer
Copy link
Contributor

Basic Info

Info Please fill out this column
Ticket(s) this addresses #3744
Primary OS tested on Ubuntu
Robotic platform tested on Custom robot

Description of contribution in a few bullet points

  • Make collision montior's default params fit better the nav2 stack by changing the output cmd topic from cmd_vel to cmd_vel_nav

Description of documentation updates required from your changes

Need to update https://navigation.ros.org/tutorials/docs/using_collision_monitor.html
Will do in a separate PR to the relevant repo.

For Maintainers:

  • Check that any new parameters added are updated in navigation.ros.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

@nakai-omer
Copy link
Contributor Author

nakai-omer commented Aug 7, 2023

I am not sure this is the correct fix, but anyone that will need to integrate collision monitor with nav2_bringup, which I presume is the recommended way to launch nav2, will have an easier time, with less config tweaking needed.
If most systems publish from controller server straight to cmd_vel (not like navigation_launch.py is configured), then this change isn't a good one.

@codecov
Copy link

codecov bot commented Aug 7, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.12% 🎉

Comparison is base (0976e63) 90.17% compared to head (336c035) 90.30%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3745      +/-   ##
==========================================
+ Coverage   90.17%   90.30%   +0.12%     
==========================================
  Files         413      413              
  Lines       18186    18186              
==========================================
+ Hits        16400    16423      +23     
+ Misses       1786     1763      -23     
Files Changed Coverage Δ
...2_collision_monitor/src/collision_monitor_node.cpp 98.18% <100.00%> (ø)

... and 7 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@nakai-omer
Copy link
Contributor Author

nakai-omer commented Aug 10, 2023

Closing as solution was documentation only at the end
EDIT:
Reopened as using smoother is the default behavior, so there is a default config change

@nakai-omer nakai-omer closed this Aug 10, 2023
@nakai-omer nakai-omer reopened this Aug 16, 2023
@nakai-omer
Copy link
Contributor Author

Updated for using the smoother by default, just like the default flow in nav2_bringup

Copy link
Collaborator

@AlexeyMerzlyakov AlexeyMerzlyakov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please check and sync-up nav2_collision_monitor/README.md with latest documentation changes. Otherwise, this change should be enough for CM node.

@SteveMacenski
Copy link
Member

Yeah, make sure to pull in main

@SteveMacenski
Copy link
Member

SteveMacenski commented Sep 5, 2023

@nakai-omer rebase or pull in main and heed Alexey's comment #3745 (review)

I think we can merge then

@SteveMacenski
Copy link
Member

@nakai-omer if you had a few minutes, we could finish this up!

@nakai-omer
Copy link
Contributor Author

@AlexeyMerzlyakov Do you think the design image in nav2_collision_monitor/README.md should be updated to include the velocity smoother? Or it comes to show that cmd_vel is rooted in the controller server?

Secondly, there is a link to the configuration guide, that links to https://navigation.ros.org/configuration/packages/configuring-collision-monitor.html, you think it should link directly to https://navigation.ros.org/configuration/packages/collision_monitor/configuring-collision-monitor-node.html?

@SteveMacenski
Copy link
Member

I think those are fine

@nakai-omer
Copy link
Contributor Author

@SteveMacenski Thanks a lot, I have pulled from main.

@SteveMacenski SteveMacenski merged commit 427427a into ros-navigation:main Sep 11, 2023
enricosutera pushed a commit to enricosutera/navigation2 that referenced this pull request May 19, 2024
* Update collision_monitor_params.yaml

* Update collision_monitor_node.cpp

* Update default topics in collision_monitor_node.cpp

* Update default topics in collision_monitor_params.yaml

* Update collision_monitor_params.yaml

* Update collision_monitor_node.cpp

* Update default topics in collision_monitor_node.cpp

* Update default topics in collision_monitor_params.yaml

Signed-off-by: enricosutera <enricosutera@outlook.com>
Marc-Morcos pushed a commit to Marc-Morcos/navigation2 that referenced this pull request Jul 4, 2024
* Update collision_monitor_params.yaml

* Update collision_monitor_node.cpp

* Update default topics in collision_monitor_node.cpp

* Update default topics in collision_monitor_params.yaml

* Update collision_monitor_params.yaml

* Update collision_monitor_node.cpp

* Update default topics in collision_monitor_node.cpp

* Update default topics in collision_monitor_params.yaml
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants