Collision detector adaptation #41
Conversation
…n-detector-humble-brnach
tonynajjar
requested review from
HovorunB,
andriimaistruk and
jplapp
as code owners
nav2_collision_monitor/README.md
Outdated
| ## Collision Detector | ||
|
|
||
| Another node exists in the nav2_collision_monitor package called the Collision Detector. This node works similarly to the collision monitor | ||
| except that it does not affect the robot's velocity. It will only inform that data from the confirgured sources has been detected within the configured polygons via message to the `/collision_detector_state` topic. |
| state_pub_->publish(std::move(state_msg)); | ||
|
|
||
| // Publish polygons for better visualization | ||
| publishPolygons(); |
There was a problem hiding this comment.
should this be behind a flag? I remember there was sth for collision monitor
There was a problem hiding this comment.
The flag is actually in the polygon since you might want to publish only some polygons
| PolygonFront: | ||
| type: "polygon" | ||
| points: [0.3, 0.3, 0.3, -0.3, 0.0, -0.3, 0.0, 0.3] | ||
| action_type: "none" |
There was a problem hiding this comment.
I guess this is the design of nav2, but it feels weird to define a collision detector and then say it should do nothing
There was a problem hiding this comment.
Also discussed in the upstream PR here. We wanted to reuse the same Polygon object from the CM without changing it too much, and that one requires an action_type otherwise it throws an error. So the decision was to use DO_NOTHING action type which is specified by "none"
| @@ -0,0 +1,3 @@ | |||
| # Name of triggered polygon | |||
| string[] polygons | |||
| bool[] detections | |||
There was a problem hiding this comment.
I kinda liked having simple topics with True/False - with this, now we need to do list traversal to find a specific polygon, which will also prevent remapping like we could do with topics. Is that also a decision from nav2 maintainers? Sorry I didn't read the conversation there
There was a problem hiding this comment.
It was implicitely decided within the upstream PR yes. They didn't like the less contextual Bool and wanted me to reuse an existing message until it was seen that it was not suitable, so another one was created
There was a problem hiding this comment.
I kinda liked having simple topics with True/False - with this, now we need to do list traversal to find a specific polygon,
to be fair, very standard messages work that way, e.g. joint_states
which will also prevent remapping like we could do with topics
not sure i understood that. Normal topic remapping we can still to on that one topic 🤔
There was a problem hiding this comment.
The old approach also had its con in our isRegionFree BT node, we created only one subscriber depending on the input region, so it couldn't take dynamic data. To make it dynamic we'd need to create several subscribers or destroy and recreate
There was a problem hiding this comment.
alright, sounds good, let's do it as you proposed - anyway we don't have much choice
This PR removes our workaround to publish the collision monitor state without doing any action and replaces it with an approach approved by the Nav2 maintainers and will likely be merged upstream: ros-navigation#3500
Once merged, the master version will likely have some more minor changes, but it will then be easy to backport