Unify timestamp checks and fix headerless FPS checks

[bmchalenv]

Fixes #18 . Also migrates the time check logic to one shared class configured with flags. This unifies logic between node and message time allowing for them to both verify similar failure cases. The current checks that can be enabled are:

• checkFpsJitter: the normal check previously implemented where we produce diagnostic errors if the time between frames is ever outside of the FPS tolerance
• checkFpsWindow: a new check that uses the average frame rate from the FPS window to evaluate if it is out of range. This is useful for timestamps that have high variance and may trigger the FpsJitter check frequently.
• checkIncreasing: same check from before but also being applied to node time now too. Makes sure time is always increasing.

Alongside the new checks there is a ROS parameter gw_time_check_preset. This parameter accepts presets HeaderWithFallback, HeaderOnly, NodeOnly, and None. The HeaderWithFallback configures the tests to use message time when a message timestamp exists and node time when it does not. None does not change the configuration and allows the user to fully specify the checks they want to be enabled. The *Only presets enable flags specifically for Header or Node time.

[greptile-apps]

Greptile Summary

This PR unifies timestamp-check logic into a shared TimeSeriesState/TimeCheckPreset system, fixing incorrect FPS-checking behaviour on headerless topics (issue #18) and adding a gw_time_check_preset ROS parameter with four well-documented presets. The refactor is well-structured and the new tests cover the preset matrix thoroughly.

Two logic issues were found:

• setExpectedDt overrides preset-configured enabled state (greenwave_diagnostics.hpp:332-336). When setExpectedDt is called (e.g. during add_topics_from_parameters), it unconditionally sets node_ts_series_.enabled = true and msg_ts_series_.enabled = true, undoing what applyTimeCheckPreset() configured. For HeaderOnly this re-enables the node-time series; for NodetimeOnly with header-bearing messages it re-enables the message-time series. Because addJitter runs before the check_fps_jitter guard, jitter outlier counters (num_jitter_outliers_node, num_jitter_outliers_msg) and max_abs_jitter_* accumulate for disabled sources, publishing misleading diagnostic values.
• addJitter side-effects fire before the check_fps_jitter guard (greenwave_diagnostics.hpp:489-500). Even when check_fps_jitter = false, source.window.addJitter(...) is called first, which increments outlier_count and updates jitter statistics. Whether or not this is intentional design (collect stats but suppress errors), the behaviour should be documented or the call moved inside the guard to match the principle of least surprise.

Confidence Score: 3/5

• Safe to merge if the setExpectedDt / addJitter issues are acknowledged; no incorrect error states are generated, but published jitter-outlier diagnostic values will be misleading for some preset + frequency combinations.
• The preset infrastructure, documentation, and tests are solid. However, setExpectedDt() silently overrides the enabled flags that applyTimeCheckPreset() set, and the addJitter side-effect inside checkFpsJitter runs before the check_fps_jitter guard. Together these mean that num_jitter_outliers_node/msg and max_abs_jitter_node/msg accumulate for disabled sources — incorrect diagnostic data that downstream dashboards may act on.
• greenwave_monitor/include/greenwave_diagnostics.hpp — setExpectedDt (lines 332-336) and checkFpsJitter (lines 489-500) need attention.

Important Files Changed

Filename	Overview
greenwave_monitor/include/greenwave_diagnostics.hpp	Core refactor introducing TimeCheckPreset, GreenwaveDiagnosticsConfig.applyTimeCheckPreset(), and a unified TimeSeriesState struct. Has a logic issue: setExpectedDt() unconditionally re-enables both time-series, overriding the preset-configured enabled state, and addJitter() is called before the check_fps_jitter guard, causing jitter outlier counts to accumulate for sources that the preset intended to disable.
greenwave_monitor/src/greenwave_monitor.cpp	Adds gw_time_check_preset parameter parsing, timeCheckPresetFromString(), and wires the preset + has_msg_timestamp flag into add_topic(). The invalid-preset warning message accidentally lists kTimestampModeHeaderWithFallback twice (as the "default" value and again as the first valid preset), but it is otherwise correct. Logic for handling unknown types gracefully (fallback to no-header) is appropriate.
greenwave_monitor/include/greenwave_monitor.hpp	Minimal change — adds the time_check_preset_ member to hold the parsed preset value for use during add_topic(). Clean and correct.
greenwave_monitor/test/test_greenwave_diagnostics.cpp	Adds unit tests for all four presets (None, HeaderOnly, NodetimeOnly, HeaderWithFallback) and two variants of HeaderWithFallback. Tests correctly validate flag states after applyTimeCheckPreset(). The run_fps_sequence() helper relies on real wall-clock sleep, making it potentially flaky under system load, but the deliberately large FPS deviation (40 vs 100 Hz) provides sufficient margin.
greenwave_monitor/test/test_greenwave_monitor_preset.py	New integration test parametrized over 5 preset/message-type combinations. Uses a shared _temp_dir for YAML files — parallel parameterized test runs write to non-colliding filenames (preset name is part of the filename), so no race conditions. Structure and assertions look correct.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[GreenwaveDiagnostics constructor] --> B[applyTimeCheckPreset]
    B --> C{time_check_preset}
    C -->|HeaderOnly| D["node_ts.enabled=false\nnode_ts.check_fps_jitter=false\nmsg_ts.enabled=true\nmsg_ts.check_fps_jitter=true\nmsg_ts.check_increasing=true"]
    C -->|NodetimeOnly| E["node_ts.enabled=true\nnode_ts.check_fps_window=true\nnode_ts.check_increasing=true\nmsg_ts.enabled=false"]
    C -->|HeaderWithFallback| F{"has_msg_timestamp?"}
    F -->|yes| G["node_ts.enabled=true\nnode_ts.check_increasing=true\nmsg_ts.enabled=true\nmsg_ts.check_fps_jitter=true\nmsg_ts.check_increasing=true"]
    F -->|no| H["node_ts.enabled=true\nnode_ts.check_fps_window=true\nnode_ts.check_increasing=true\nmsg_ts.enabled=false"]
    C -->|None| I[No flags changed]

    D --> J[setExpectedDt called]
    E --> J
    G --> J
    H --> J
    I --> J

    J --> K["⚠️ node_ts.enabled=true\nmsg_ts.enabled=true\n(overrides preset!)"]

    K --> L[updateDiagnostics]
    L --> M[updateTimeSource node_ts]
    L --> N{has_msg_timestamp?}
    N -->|yes| O[updateTimeSource msg_ts]

    M --> P{node_ts.enabled?}
    P -->|yes| Q[checkFpsJitter\ncheckFpsWindow\ncheckIncreasing]
    Q --> R{check_fps_jitter?}
    R -->|no| S["⚠️ addJitter called\noutlier_count increments\nbut returns false"]
    R -->|yes| T[may return ERROR]
    P -->|no| U[update prev_ts only]

Loading

_{Last reviewed commit: c26f657}

[greptile-apps]

_{5 files reviewed, 1 comment}

_{Edit Code Review Agent Settings | Greptile}

[greptile-apps]

Additional Comments (2)

greenwave_monitor/include/greenwave_diagnostics.hpp, line 337
setExpectedDt unconditionally enables both sources, overriding preset configuration

setExpectedDt (and its mirror clearExpectedDt) hard-codes node_source_.enabled = true / msg_source_.enabled = true regardless of what the preset put in those flags. This silently breaks the semantic contract of every non-default preset:

• nodetime_only — after setExpectedDt, msg_source_.enabled flips to true. Because updateTimeSource is called for msg_source_ whenever has_msg_timestamp is true, checkFpsJitter is now executed for the message source. Even though msg_source_.check_fps_jitter = false prevents an error signal, addJitter inside checkFpsJitter still increments msg_source_.window.outlier_count. This means total_dropped_frames and num_jitter_outliers_msg in the published diagnostics will grow with spurious counts that are entirely unrelated to the node-time check the preset intended.

• header_only — node_source_.enabled flips to true after setExpectedDt. Since node_source_.check_fps_jitter = false, no error fires, but num_jitter_outliers_node is again inflated.

• clearExpectedDt has the opposite problem: it sets msg_source_.enabled = false, which prevents checkIncreasing from running for msg_source_ (via the early-return in updateTimeSource). Under the header_only preset check_increasing = true for message timestamps, so clearing the expected frequency also silences the increasing-timestamp guard — a side-effect the caller almost certainly does not intend.

This is the common startup path: add_topics_from_parameters calls setExpectedDt for every topic that has a frequency in the YAML config, so all three presets other than header_with_nodetime_fallback are affected on startup.

A fix would be to track a separate frequency_is_set boolean rather than conflating it with the preset-controlled enabled flags, or to store the preset-applied enabled state and restore it in clearExpectedDt:

void setExpectedDt(double expected_hz, double tolerance_percent)
{
  const std::lock_guard<std::mutex> lock(greenwave_diagnostics_mutex_);
  // Only enable sources that the preset allows, instead of force-enabling both.
  node_source_.enabled = diagnostics_config_.enable_node_time_diagnostics;
  msg_source_.enabled  = diagnostics_config_.enable_msg_time_diagnostics;
  ...
}

void clearExpectedDt()
{
  const std::lock_guard<std::mutex> lock(greenwave_diagnostics_mutex_);
  node_source_.enabled = diagnostics_config_.enable_node_time_diagnostics;
  msg_source_.enabled  = diagnostics_config_.enable_msg_time_diagnostics;
  ...
}

---

greenwave_monitor/src/greenwave_monitor.cpp, line 92
Duplicate preset name in warning log

The format string has 6 %s placeholders and passes kTimestampModeHeaderWithFallback for both the "using default" slot and the first entry in the valid-preset list. The resulting message reads: "Invalid time check preset '…', using default 'header_with_nodetime_fallback'. Valid presets are: header_with_nodetime_fallback, header_with_nodetime_fallback, header_only, nodetime_only, none" — the default appears twice.

    RCLCPP_WARN(
      this->get_logger(), "Invalid time check preset '%s', using default '%s'. Valid presets are: "
      "%s, %s, %s, %s",
      time_check_preset_str.c_str(), kTimestampModeHeaderWithFallback,
      kTimestampModeHeaderWithFallback, kTimestampModeHeaderOnly, kTimestampModeNodetimeOnly,
      kTimestampModeNone);
    time_check_preset_ = greenwave_diagnostics::TimeCheckPreset::HeaderWithFallback;

should be:

    RCLCPP_WARN(
      this->get_logger(), "Invalid time check preset '%s', using default '%s'. Valid presets are: "
      "%s, %s, %s, %s",
      time_check_preset_str.c_str(), kTimestampModeHeaderWithFallback,
      kTimestampModeHeaderOnly, kTimestampModeNodetimeOnly,
      kTimestampModeNone, kTimestampModeHeaderWithFallback);
    time_check_preset_ = greenwave_diagnostics::TimeCheckPreset::HeaderWithFallback;

Or more simply drop to four format args:

"… Valid presets are: %s, %s, %s, %s"

with kTimestampModeHeaderWithFallback, kTimestampModeHeaderOnly, kTimestampModeNodetimeOnly, kTimestampModeNone.

[greptile-apps]

Additional Comments (3)

greenwave_monitor/include/greenwave_diagnostics.hpp, line 336
setExpectedDt unconditionally overrides preset-configured enabled state

setExpectedDt forces node_ts_series_.enabled = true and msg_ts_series_.enabled = true regardless of the active preset. This silently undoes the configuration that applyTimeCheckPreset() set in the constructor:

• HeaderOnly preset — applyTimeCheckPreset() sets node_ts_series_.enabled = false. After setExpectedDt, it is true. checkFpsJitter is then called for node time, and because addJitter runs before the check_fps_jitter guard (line 490), node_ts_series_.window.outlier_count accumulates outliers. The diagnostic values num_jitter_outliers_node and max_abs_jitter_node become non-zero even though the user explicitly chose header-only checking.

• NodetimeOnly preset with header-bearing topics — applyTimeCheckPreset() sets msg_ts_series_.enabled = false. After setExpectedDt, it is true. The same addJitter side-effect causes num_jitter_outliers_msg to accumulate incorrectly.

The fix is to save the preset's intent for each series and restore it in setExpectedDt, or to gate enabled against the per-series check flags:

// Option A: re-enable only series that have at least one check configured
node_ts_series_.enabled =
  node_ts_series_.check_fps_jitter ||
  node_ts_series_.check_fps_window ||
  node_ts_series_.check_increasing;
msg_ts_series_.enabled =
  msg_ts_series_.check_fps_jitter ||
  msg_ts_series_.check_fps_window ||
  msg_ts_series_.check_increasing;

Correspondingly, clearExpectedDt already disables both — that direction is fine since the enabled flag is what prevents error generation.

---

greenwave_monitor/include/greenwave_diagnostics.hpp, line 500
addJitter side-effects fire before the check_fps_jitter guard

source.window.addJitter(...) is called unconditionally before the !source.check_fps_jitter early-return. addJitter has persistent side-effects: it increments outlier_count, updates sum_jitter_abs_us, and tracks max_abs_jitter_us. These values are published as num_jitter_outliers_node/num_jitter_outliers_msg and max_abs_jitter_node/max_abs_jitter_msg in every publishDiagnostics call.

This means that whenever a time series is enabled but check_fps_jitter = false (e.g., node time in the HeaderWithFallback preset after setExpectedDt is called), jitter statistics accumulate even though the jitter check is nominally off. Consumers of those diagnostic keys will see unexpectedly non-zero values.

If the intent is to collect jitter statistics unconditionally and only gate error reporting via check_fps_jitter, a comment explaining that design decision would prevent confusion. If the intent is to skip statistics collection too, move addJitter inside the if (source.check_fps_jitter) block:

  if (!source.check_fps_jitter) {
    return false;
  }
  const int64_t diff_us = timestamp_diff_us - diagnostics_config_.expected_dt_us;
  const int64_t abs_jitter_us = std::abs(diff_us);
  const bool missed_deadline =
    source.window.addJitter(abs_jitter_us, diagnostics_config_.jitter_tolerance_us);

---

greenwave_monitor/src/greenwave_monitor.cpp, line 92
Duplicate preset string in warning message

kTimestampModeHeaderWithFallback is passed twice: once as the "default" value and once as the first entry in the "Valid presets are:" list, so the rendered message reads:

Invalid time check preset 'foo', using default 'header_with_nodetime_fallback'.
Valid presets are: header_with_nodetime_fallback, header_only, nodetime_only, none

That is technically correct, but the structure makes it look like the default appears twice. A cleaner approach avoids repeating it:

    RCLCPP_WARN(
      this->get_logger(), "Invalid time check preset '%s', using default '%s'. "
      "Valid presets are: %s, %s, %s, %s",
      time_check_preset_str.c_str(), kTimestampModeHeaderWithFallback,
      kTimestampModeHeaderWithFallback, kTimestampModeHeaderOnly,
      kTimestampModeNodetimeOnly, kTimestampModeNone);

(No change in arguments — just noting the message text so a future reader isn't confused by the apparent duplication.)

_{Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!}