-
Notifications
You must be signed in to change notification settings - Fork 1.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Publish collision monitor state #3504
Publish collision monitor state #3504
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good in general, most are nit picks and a couple of questions
nav2_collision_monitor/include/nav2_collision_monitor/types.hpp
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are some items, I've mentioned, but in overall, this seems a good approach for me
nav2_collision_monitor/include/nav2_collision_monitor/types.hpp
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Otherwise LGTM, just need the complimentary documentation updates for the new parameter in the config guide + migration guide for the new feature
Thanks for the review @SteveMacenski and @AlexeyMerzlyakov! Created a PR for updating the config guide + migration guide too. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. On Alexey's final call to merge.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change reviewed: OK.
Local colcon test --packages-select nav2_collision_monitor
: OK
Codecov test: OK
Local TB3 simulation verification: OK (CM is working well and publishing a new topic)
This commits is cherry-picked from b8d077e. The collision monitor now has an additional parameter named "state_topic". If the state topic is set, the collision monitor will publish its status to the defined topic so that other nodes might perform actions based on the current state of the collision monitor.
This commits is cherry-picked from b8d077e. The collision monitor now has an additional parameter named "state_topic". If the state topic is set, the collision monitor will publish its status to the defined topic so that other nodes might perform actions based on the current state of the collision monitor. Signed-off-by: agennart <[email protected]>
Basic Info
Description of contribution in a few bullet points
nav2_msgs
-CollisionMonitorState.msg
DO_NOTHING
polygon_name
instead ofaction_type
(To handle multipleslowdown
polygons correctly)Description of documentation updates required from your changes
Future work that may be required in bullet points
cmd_vel
, but I think it will be very useful to publish the polygon state even when the robot is not moving too. Especially for cases like when thestop
polygon is triggered(regardless ofcmd_vel
speed), or when there are points inapproach
polygon where the robot will not be able to move. (Probably similar requirement as mentioned here)For Maintainers: