- 
                Notifications
    You must be signed in to change notification settings 
- Fork 1.6k
[Draft] CollisionMonitor: add CostmapSource + dataset-based bag test #5642
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
base: main
Are you sure you want to change the base?
[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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!
| find_package(nav2_costmap_2d REQUIRED) | ||
| find_package(nav2_msgs REQUIRED) | ||
| find_package(nav2_util REQUIRED) | ||
| find_package(rosgraph_msgs REQUIRED) | 
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.
Add this to the build testing section
        
          
                nav2_collision_monitor/README.md
              
                Outdated
          
        
      |  | ||
| > **⚠️ when using CostmapSource** | ||
| > Collision Monitor normally **bypasses the costmap** to minimize reaction latency using fresh sensor data. | ||
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.
| > Use at your own caution or when using external costmap sources from derived sources. | 
| max_height: 0.5 | ||
| use_global_height: False | ||
| enabled: True | ||
| costmap: | 
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.
You can keep this in the file as an example, but please comment out so its not active. Also change the topic to local_costmap/costmap so namespaces works
| { | ||
| public: | ||
| CostmapSource( | ||
|  | 
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.
Remove extra sapce
| } | ||
| std::string source_topic; | ||
| getParameters(source_topic); | ||
| rclcpp::QoS qos = rclcpp::SystemDefaultsQoS(); | 
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.
Use Nav2 QoS profiles from nav2_ros_common
| nav2::declare_parameter_if_not_declared( | ||
| node, thresh_name, rclcpp::ParameterValue(253)); | ||
| int v = node->get_parameter(thresh_name).as_int(); | 
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.
Use the new declare_or_get_parameter API in nav2::LIfecycleNode
| const int idx = y * meta.size_x + x; | ||
|  | ||
| const uint8_t c = cm.data[idx]; | ||
| const bool is_obstacle = (c >= cost_threshold_ && c < 255) || | 
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.
Use the costmap values for UNKNOWN rather than hardcoding 255
| def __init__(self): | ||
| super().__init__('fake_cm_bag_source') | ||
| # Use sim time so /clock drives timestamps | ||
| # self.set_parameters([Parameter('use_sim_time', value=True)]) | 
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.
Commented out?
| // Window where obstacle exists in the bag | ||
| stop_window_start_ = this->declare_parameter<double>("stop_window_start", 3.0); | ||
| stop_window_end_ = this->declare_parameter<double>("stop_window_end", 8.0); | ||
|  | 
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.
| 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 <[email protected]>
…st (v1) Signed-off-by: Mengting Yang <[email protected]>
Signed-off-by: Mengting Yang <[email protected]>
Signed-off-by: Mengting Yang <[email protected]>
Signed-off-by: lotusymt <[email protected]>
b4d7608    to
    fc27b4c      
    Compare
  
    | Codecov Report❌ Patch coverage is  
 
 ... and 13 files with indirect coverage changes 🚀 New features to boost your workflow:
 | 
| Hi Steve, | 
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-*.