-
Notifications
You must be signed in to change notification settings - Fork 6
feat(s57): cold-cache mitigation — faster polling, timing logs, precompute, buffer param #20
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -4,7 +4,12 @@ | |||||||||||||||||||||||||||||||||||||||||||||||
| #include "geometry_msgs/msg/point_stamped.hpp" | ||||||||||||||||||||||||||||||||||||||||||||||||
| #include "tf2_geometry_msgs/tf2_geometry_msgs.hpp" | ||||||||||||||||||||||||||||||||||||||||||||||||
| #include "nav_msgs/msg/occupancy_grid.hpp" | ||||||||||||||||||||||||||||||||||||||||||||||||
| #include <algorithm> | ||||||||||||||||||||||||||||||||||||||||||||||||
| #include <chrono> | ||||||||||||||||||||||||||||||||||||||||||||||||
| #include <cmath> | ||||||||||||||||||||||||||||||||||||||||||||||||
| #include <cstdlib> | ||||||||||||||||||||||||||||||||||||||||||||||||
| #include <thread> | ||||||||||||||||||||||||||||||||||||||||||||||||
| #include <unordered_set> | ||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
| namespace s57_grids | ||||||||||||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -62,6 +67,40 @@ GridPublisher::on_configure(const rclcpp_lifecycle::State &state) | |||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||
| publish_costmaps_ = get_parameter("publish_costmaps").as_bool(); | ||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
| const double default_check_new_grids_period = check_new_grids_period_; | ||||||||||||||||||||||||||||||||||||||||||||||||
| if(!has_parameter("check_new_grids_period")) | ||||||||||||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||||||||||||
| declare_parameter("check_new_grids_period", rclcpp::ParameterValue(check_new_grids_period_)); | ||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||
| check_new_grids_period_ = get_parameter("check_new_grids_period").as_double(); | ||||||||||||||||||||||||||||||||||||||||||||||||
| if(!std::isfinite(check_new_grids_period_) || check_new_grids_period_ <= 0.0) | ||||||||||||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||||||||||||
| RCLCPP_WARN_STREAM(get_logger(), | ||||||||||||||||||||||||||||||||||||||||||||||||
| "Invalid check_new_grids_period value " << check_new_grids_period_ | ||||||||||||||||||||||||||||||||||||||||||||||||
| << " s; using " << default_check_new_grids_period << " s instead."); | ||||||||||||||||||||||||||||||||||||||||||||||||
| check_new_grids_period_ = default_check_new_grids_period; | ||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
| if(!has_parameter("robot_base_frame")) | ||||||||||||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||||||||||||
| declare_parameter("robot_base_frame", rclcpp::ParameterValue(robot_base_frame_)); | ||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||
| robot_base_frame_ = get_parameter("robot_base_frame").as_string(); | ||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
| const double default_precompute_radius = precompute_radius_; | ||||||||||||||||||||||||||||||||||||||||||||||||
| if(!has_parameter("precompute_radius")) | ||||||||||||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||||||||||||
| declare_parameter("precompute_radius", rclcpp::ParameterValue(precompute_radius_)); | ||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||
| precompute_radius_ = get_parameter("precompute_radius").as_double(); | ||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||
| precompute_radius_ = get_parameter("precompute_radius").as_double(); | |
| const double precompute_radius = get_parameter("precompute_radius").as_double(); | |
| if(std::isfinite(precompute_radius) && precompute_radius >= 0.0) | |
| { | |
| precompute_radius_ = precompute_radius; | |
| } | |
| else | |
| { | |
| RCLCPP_WARN_STREAM( | |
| get_logger(), | |
| "Invalid precompute_radius parameter (" << precompute_radius | |
| << "). Expected a finite value >= 0. Disabling precompute." | |
| ); | |
| precompute_radius_ = 0.0; | |
| } |
Copilot
AI
Apr 22, 2026
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.
check_new_grids_period_ is taken from a ROS parameter but not validated. Values <= 0, NaN/Inf, or very small values can result in a 0ms/negative timer period after int(1000.0 * check_new_grids_period_), which can throw or create a tight loop. Please validate the parameter (finite and > 0), and clamp the resulting timer duration to a sane minimum (e.g., >= 1ms) before creating the wall timer.
Copilot
AI
Apr 22, 2026
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.
getDatasets() unconditionally adds every returned label to requested_grids_to_publish_ and grid_request_start_times_. If a grid was already published earlier (so grid_publishers_ already contains the label and no new future will be started), these entries will never be erased because cleanup only happens when a pending future completes. This will leak memory over time and can also leave stale start times around indefinitely. Consider only inserting into requested_grids_to_publish_ / grid_request_start_times_ when the label is not already available (e.g., no existing publisher/grid and no pending future), or add a cleanup path that removes entries for labels that are already published/cached.
Copilot
AI
Apr 22, 2026
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.
The timing log is described as “wall-clock” but uses get_clock()->now() / rclcpp::Time (ROS time). With use_sim_time or time jumps, elapsed durations can be inaccurate or even negative. For latency measurements, record timestamps using a steady/monotonic source (e.g., std::chrono::steady_clock or an rclcpp::Clock(RCL_STEADY_TIME)) and compute elapsed from that.
| // Report time from first request to publish for this chart. | |
| // Useful for diagnosing cold-cache behaviour without recompiling. | |
| auto start_it = grid_request_start_times_.find(pg.first); | |
| if(start_it != grid_request_start_times_.end()) | |
| { | |
| auto elapsed = (get_clock()->now() - start_it->second).seconds(); | |
| RCLCPP_INFO_STREAM(get_logger(), | |
| "Grid ready: " << pg.first << " (" << elapsed << " s from request to publish)"); | |
| grid_request_start_times_.erase(start_it); | |
| } | |
| else | |
| { | |
| RCLCPP_INFO_STREAM(get_logger(), "Grid ready: " << pg.first); | |
| } | |
| // A request-to-publish latency should be measured with a steady clock. | |
| // Do not compute it from ROS time here, because ROS time may jump, | |
| // pause, or run under use_sim_time. | |
| auto start_it = grid_request_start_times_.find(pg.first); | |
| if(start_it != grid_request_start_times_.end()) | |
| { | |
| grid_request_start_times_.erase(start_it); | |
| } | |
| RCLCPP_INFO_STREAM(get_logger(), "Grid ready: " << pg.first); |
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.
This file now directly uses
std::max/std::min(and already usesstd::getenv) but doesn’t include<algorithm>/<cstdlib>explicitly. It may compile today due to transitive includes, but that’s not guaranteed. Consider adding the standard headers for the standard-library symbols used here to keep the translation unit self-contained.