feat(s57_layer): parameterize tide invalidation threshold by rolker · Pull Request #18 · rolker/s57_tools

rolker · 2026-04-22T03:01:15Z

Summary

Add tide_invalidate_threshold parameter to S57Layer, default 0.01 m. Replaces the hardcoded 0.01 in updateBounds's tide-change check.
Field configs leave it unset → safe default preserved.
Sim configs (companion PR in rolker/ben_project11) override to 0.05 m, restoring roughly real-time-equivalent invalidation cadence under tide_speed_factor=10.

Closes #17. Companion: rolker/ben_project11#TBD.

Why

Deployment-log analysis of the sim run on deadpool (2026-04-21T22:03:55) showed 15 tide updates in 125 s of run, correlating with 7 GridBased plugin failed to plan: "Costmap timed out waiting for update" planner_server aborts and a Control loop missed its desired rate of 5.0000 Hz. Current loop rate is 2.0382 Hz controller_server warning. Each tide update invalidates all cached tile chart_count values and forces full regen. The 1 cm threshold is correct for field operation (~72 s/cycle near peak Portsmouth tide) but pathological under the 10× sim time scale.

Test plan

colcon build --packages-select s57_layer — clean.
colcon test --packages-select s57_layer — 21 tests, 0 errors, 0 failures, 5 skipped. TideOffsetTest 3/3 passing (default unchanged).
Reviewer: confirm parameter name and default. Optional: add a TideThresholdParameterTest exercising a non-default override.

Authored-By: Claude Code Agent
Model: Claude Opus 4.7 (1M context)

Make the previously-hardcoded 0.01 m threshold a parameter (tide_invalidate_threshold, default 0.01) so sim configs can override without affecting field defaults. Field operation (real tide ~0.5 m/hr near peak): 1 cm step fires invalidation roughly every 72 s — keep default. Sim runs with tide_speed_factor=10 fire ~10× as often (every ~8 s) which causes costmap stalls and planner_server timeouts (deployment-log analysis of deadpool sim run 2026-04-21T22:03:55 showed 15 tide updates in 125 s correlating with 7 "Costmap timed out" planner aborts and a "Control loop missed its desired rate" warning). Companion change in rolker/ben_project11 will set 0.05 m in ben_sim.yaml (sim-only, gated by is_simulator) which restores roughly real-time-equivalent invalidation cadence. Existing TideOffsetTest suite (3/3) passes unchanged: defaults preserved, threshold semantics identical for the existing test inputs (0.005 m below threshold; 1 m and 2.5 m above threshold). Closes #17 --- Authored-By: Claude Code Agent Model: Claude Opus 4.7 (1M context) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Copilot

Pull request overview

This PR parameterizes the tide-driven cache invalidation threshold in S57Layer, replacing the previously hardcoded 0.01 m cutoff so simulation configurations can reduce invalidation frequency under accelerated tide.

Changes:

Added tide_invalidate_threshold parameter (default 0.01) to S57Layer.
Wired the new parameter into the tide-change check used to invalidate cached tile state.
Extended the “tide correction enabled” log line to include the configured threshold.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
`s57_layer/src/s57_layer.h`	Adds the new `tide_invalidate_threshold_` member (default 0.01 m) with explanatory comment.
`s57_layer/src/s57_layer.cpp`	Declares/reads the new parameter and uses it in the tide offset invalidation comparison + logging.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-22T03:04:23Z


+  declareParameter("tide_invalidate_threshold", rclcpp::ParameterValue(tide_invalidate_threshold_));
+  node->get_parameter(name_+".tide_invalidate_threshold", tide_invalidate_threshold_);


tide_invalidate_threshold_ is loaded from a parameter but never validated. If it’s configured as a negative value, std::abs(...) > tide_invalidate_threshold_ will be true on every update, forcing continuous invalidations and potentially recreating the same timeout behavior this change is meant to mitigate. Consider clamping to >= 0 (and rejecting non-finite values) after reading the parameter, with a warning and fallback to the default.

Suggested change

declareParameter("tide_invalidate_threshold", rclcpp::ParameterValue(tide_invalidate_threshold_));

node->get_parameter(name_+".tide_invalidate_threshold", tide_invalidate_threshold_);

const auto default_tide_invalidate_threshold = tide_invalidate_threshold_;

declareParameter("tide_invalidate_threshold", rclcpp::ParameterValue(tide_invalidate_threshold_));

node->get_parameter(name_+".tide_invalidate_threshold", tide_invalidate_threshold_);

if(!std::isfinite(tide_invalidate_threshold_) || tide_invalidate_threshold_ < 0.0)

{

RCLCPP_WARN_STREAM(logger_,

"Invalid tide_invalidate_threshold value " << tide_invalidate_threshold_

<< "; using default " << default_tide_invalidate_threshold_ << " instead.");

tide_invalidate_threshold_ = default_tide_invalidate_threshold;

}

Copilot · 2026-04-22T03:04:23Z

      auto transform = tf_->lookupTransform(chart_datum_frame_, sea_surface_frame_, tf2::TimePointZero);
      double new_offset = transform.transform.translation.z;
-      if(std::abs(new_offset - tide_offset_) > 0.01)
+      if(std::abs(new_offset - tide_offset_) > tide_invalidate_threshold_)


This change introduces a new behavior knob (tide_invalidate_threshold_), but the existing tide tests only cover the default 0.01 m behavior. Adding a test that sets chart_layer.tide_invalidate_threshold to a non-default value and asserts the invalidation/no-invalidation behavior would prevent regressions in the parameter wiring.

Two follow-ups from Copilot review on PR #18: 1. Validate tide_invalidate_threshold after get_parameter. ROS 2 parameters from YAML are external input and warrant validation at that boundary. A negative configured value would make every tide change exceed the threshold (continuous invalidation = costmap stalls — exactly what this PR is meant to prevent). A non-finite value would silently disable invalidation. Reject both cases with RCLCPP_WARN and fall back to the default. 2. Add test coverage for the new parameter: - CustomThresholdAcceptsLargerChanges: declares chart_layer.tide_invalidate_threshold=0.05, exercises a 3 cm change (between default 0.01 and configured 0.05 — must NOT invalidate), then a 6 cm change (above 0.05 — must invalidate), then verifies recovery after updateCosts. - InvalidThresholdFallsBackToDefault: declares -1.0 (negative), exercises a 0.005 m change which would invalidate if validation was skipped but stays current with the restored 0.01 m default. s57_layer test suite now 23 tests (was 21), 0 failures, 0 errors. TideOffsetTest suite now 5/5 passing. Refs: #17 --- Authored-By: Claude Code Agent Model: Claude Opus 4.7 (1M context) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

rolker · 2026-04-22T03:28:54Z

Addressed both Copilot comments in 2nd commit:

Parameter validation (s57_layer.cpp:67) — after get_parameter, check std::isfinite(threshold) && threshold >= 0.0. Invalid values trigger RCLCPP_WARN and fall back to the default 0.01 m. Prevents the continuous-invalidation failure mode the bot identified (negative threshold) and the silent-no-invalidate failure mode (NaN/inf).
Test coverage for the new parameter:
- CustomThresholdAcceptsLargerChanges: declares chart_layer.tide_invalidate_threshold=0.05, exercises a 3 cm tide change (between default 0.01 and configured 0.05 — must NOT invalidate), then a 6 cm change (above 0.05 — must invalidate), then recovery after updateCosts.
- InvalidThresholdFallsBackToDefault: declares -1.0, exercises a 0.005 m change which would invalidate if validation was skipped but stays current with the restored 0.01 m default.

Test suite now 23 tests (was 21), 0 failures, 0 errors. TideOffsetTest 5/5 passing.

Authored-By: Claude Code Agent
Model: Claude Opus 4.7 (1M context)

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot AI review requested due to automatic review settings April 22, 2026 03:01

Copilot started reviewing on behalf of rolker April 22, 2026 03:01 View session

Copilot AI reviewed Apr 22, 2026

View reviewed changes

rolker requested a review from Copilot April 22, 2026 03:29

Copilot started reviewing on behalf of rolker April 22, 2026 03:29 View session

Copilot AI reviewed Apr 22, 2026

View reviewed changes

rolker merged commit fc0dc82 into jazzy Apr 22, 2026
5 checks passed

rolker deleted the feature/issue-17 branch April 22, 2026 03:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(s57_layer): parameterize tide invalidation threshold#18

feat(s57_layer): parameterize tide invalidation threshold#18
rolker merged 2 commits into
jazzyfrom
feature/issue-17

rolker commented Apr 22, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Apr 22, 2026

Uh oh!

Copilot AI Apr 22, 2026

Uh oh!

rolker commented Apr 22, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants


		declareParameter("tide_invalidate_threshold", rclcpp::ParameterValue(tide_invalidate_threshold_));
		node->get_parameter(name_+".tide_invalidate_threshold", tide_invalidate_threshold_);

-  declareParameter("tide_invalidate_threshold", rclcpp::ParameterValue(tide_invalidate_threshold_));
-  node->get_parameter(name_+".tide_invalidate_threshold", tide_invalidate_threshold_);
+  const auto default_tide_invalidate_threshold = tide_invalidate_threshold_;
+  declareParameter("tide_invalidate_threshold", rclcpp::ParameterValue(tide_invalidate_threshold_));
+  node->get_parameter(name_+".tide_invalidate_threshold", tide_invalidate_threshold_);
+  if(!std::isfinite(tide_invalidate_threshold_) || tide_invalidate_threshold_ < 0.0)
+  {
+    RCLCPP_WARN_STREAM(logger_,
+      "Invalid tide_invalidate_threshold value " << tide_invalidate_threshold_
+      << "; using default " << default_tide_invalidate_threshold_ << " instead.");
+    tide_invalidate_threshold_ = default_tide_invalidate_threshold;
+  }

Conversation

rolker commented Apr 22, 2026

Summary

Why

Test plan

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Apr 22, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 22, 2026

Choose a reason for hiding this comment

Uh oh!

rolker commented Apr 22, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants