Skip to content

Add latest channel support in schema_tracker.#389

Open
shaosu-nvidia wants to merge 1 commit intomainfrom
ssx/replay/latest_channel
Open

Add latest channel support in schema_tracker.#389
shaosu-nvidia wants to merge 1 commit intomainfrom
ssx/replay/latest_channel

Conversation

@shaosu-nvidia
Copy link
Copy Markdown
Contributor

@shaosu-nvidia shaosu-nvidia commented Apr 13, 2026

  • Enable pedals to write the latest channel

Summary by CodeRabbit

  • New Features
    • Enhanced MCAP recording to support optional latest-value tracking for pedal data, enabling efficient access to the most recent pedal data during playback.

- Enable pedals to write the latest channel
@shaosu-nvidia shaosu-nvidia requested a review from nvddr April 13, 2026 18:14
@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Apr 13, 2026

📝 Walkthrough

Walkthrough

This pull request adds optional support for writing an additional "latest-only" MCAP sample per update() call. The SchemaTracker class constructor now accepts an optional mcap_channel_latest_index parameter. When set, each sample processed during update() will be written to both the existing channel and the new latest channel. The PedalRecordingTraits compile-time metadata is updated to define the new "pedals_latest" channel, and the pedal tracker implementation is modified to pass the corresponding channel index to the schema tracker.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main change: adding support for a latest channel in the schema_tracker component, which is the core purpose of this PR.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch ssx/replay/latest_channel

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/core/live_trackers/cpp/inc/live_trackers/schema_tracker.hpp (1)

84-111: ⚠️ Potential issue | 🟠 Major

Guard latest-channel write on a valid decoded sample from this frame.

Line 84 initializes last_timestamp to a default value, and Line 108 only checks pointer presence. If all current samples fail GetRoot (Lines 88-91), this can write stale out_latest with an invalid timestamp.

Proposed fix
-        DeviceDataTimestamp last_timestamp{};
+        std::optional<DeviceDataTimestamp> last_timestamp;
         for (const auto& sample : samples_)
         {
             auto fb = flatbuffers::GetRoot<DataTableT>(sample.buffer.data());
             if (!fb)
             {
                 continue;
             }
@@
-            last_timestamp = sample.timestamp;
+            last_timestamp = sample.timestamp;
@@
-        if (mcap_channel_latest_index_ && mcap_channels_ && out_latest)
+        if (mcap_channel_latest_index_ && mcap_channels_ && out_latest && last_timestamp.has_value())
         {
-            mcap_channels_->write(*mcap_channel_latest_index_, last_timestamp, out_latest);
+            mcap_channels_->write(*mcap_channel_latest_index_, *last_timestamp, out_latest);
         }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/core/live_trackers/cpp/inc/live_trackers/schema_tracker.hpp` around lines
84 - 111, The loop may call mcap_channels_->write for the "latest" channel even
when every sample failed decoding, causing a stale out_latest/last_timestamp to
be written; introduce a validity flag (e.g., bool have_decoded = false) set to
true only after a successful GetRoot/UnPackTo and update last_timestamp there,
then change the final guard to require have_decoded &&
mcap_channel_latest_index_ && mcap_channels_ && out_latest before calling
mcap_channels_->write; refer to symbols samples_, GetRoot<DataTableT>,
UnPackTo(out_latest.get()), out_latest, last_timestamp,
mcap_channel_latest_index_, and mcap_channels_->write.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Outside diff comments:
In `@src/core/live_trackers/cpp/inc/live_trackers/schema_tracker.hpp`:
- Around line 84-111: The loop may call mcap_channels_->write for the "latest"
channel even when every sample failed decoding, causing a stale
out_latest/last_timestamp to be written; introduce a validity flag (e.g., bool
have_decoded = false) set to true only after a successful GetRoot/UnPackTo and
update last_timestamp there, then change the final guard to require have_decoded
&& mcap_channel_latest_index_ && mcap_channels_ && out_latest before calling
mcap_channels_->write; refer to symbols samples_, GetRoot<DataTableT>,
UnPackTo(out_latest.get()), out_latest, last_timestamp,
mcap_channel_latest_index_, and mcap_channels_->write.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 0b6bbe72-229e-4bea-be5b-88455c2c2a06

📥 Commits

Reviewing files that changed from the base of the PR and between af72817 and 8d0d0e9.

📒 Files selected for processing (3)
  • src/core/live_trackers/cpp/inc/live_trackers/schema_tracker.hpp
  • src/core/live_trackers/cpp/live_generic_3axis_pedal_tracker_impl.cpp
  • src/core/mcap/cpp/inc/mcap/recording_traits.hpp

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant