Draft
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/core/deviceio_trackers/python/tracker_bindings.cpp`:
- Around line 97-101: These exported Python constants (NUM_JOINTS, JOINT_PALM,
JOINT_WRIST, JOINT_THUMB_TIP, JOINT_INDEX_TIP) are magic numbers mapping to
OpenXR XR_HAND_JOINT_*_EXT indices; add a clear comment block above these
assignments in tracker_bindings.cpp that states they originate from the OpenXR
hand-tracking extension (e.g., XR_EXT_hand_tracking / XR_HAND_JOINT_*_EXT
constants), list which XR constant each integer corresponds to (e.g., JOINT_PALM
= XR_HAND_JOINT_PALM_EXT -> 0), note the OpenXR spec/version/extension used, and
mention these values are exported to the public Python API via
deviceio_trackers_init.py and deviceio_init.py so maintainers know the source
and meaning.
In `@src/core/live_trackers/cpp/inc/live_trackers/schema_tracker.hpp`:
- Line 63: The comment for SchemaTracker::update currently documents only the
out_latest parameter but the method signature includes a new int64_t log_time_ns
parameter; update the method's docblock in
src/core/live_trackers/cpp/inc/live_trackers/schema_tracker.hpp to add a short
description for log_time_ns (e.g., "log time in nanoseconds used for
versioning/timestamping") alongside the existing out_latest description so the
docblock matches the signature of void update(int64_t log_time_ns,
std::shared_ptr<NativeDataT>& out_latest).
In `@src/core/live_trackers/cpp/live_head_tracker_impl.cpp`:
- Around line 82-83: DeviceDataTimestamp is being populated with the same
monotonic value (last_update_time_ns_) which loses distinction between
available, local-sample, and device-clock times; change the DeviceDataTimestamp
construction used in mcap_channels_->write so available_time_local_common_clock
remains last_update_time_ns_, sample_time_local_common_clock is set to the best
estimate of when the sample was taken in the local/common clock (compute or
convert from a sample_time_ns or predicted display time if available), and
sample_time_raw_device_clock preserves the source device clock value (e.g., the
OpenXR XrTime if provided); if the runtime does not provide separate sample
times, add a brief TODO comment next to DeviceDataTimestamp usage explaining the
simplification and that future work should populate
sample_time_local_common_clock and sample_time_raw_device_clock from runtime
sample timestamps (reference symbols: DeviceDataTimestamp,
mcap_channels_->write, last_update_time_ns_, tracked_.data).
In `@src/core/mcap_tests/cpp/test_mcap_tracker_channels.cpp`:
- Line 88: The test calls to ch.write currently pass identical values for the
explicit log_time_ns parameter and the DeviceDataTimestamp fields, so they won't
detect regressions where msg.logTime is incorrectly taken from the timestamp
struct; update each call site (eg. ch.write(..., core::DeviceDataTimestamp(...),
head_data) used across test_mcap_tracker_channels.cpp) to use mismatched values
for the explicit log_time_ns and the DeviceDataTimestamp(...) arguments (choose
distinct integers) and add assertions that the resulting msg.logTime equals the
explicit log_time_ns parameter (not the DeviceDataTimestamp value) to validate
the new log_time_ns contract.
In `@src/core/mcap/cpp/inc/mcap/tracker_channels.hpp`:
- Around line 60-63: Update the timestamp binding docs to reflect that the MCAP
log timestamp is set from the explicit log_time_ns parameter (msg.logTime :=
log_time_ns) in the write() overloads that accept DeviceDataTimestamp, rather
than from available_time_local_common_clock; change any text mentioning
available_time_local_common_clock as the MCAP log timestamp to describe that
available_time_local_common_clock is a separate field carried in the message and
that log_time_ns is the authoritative MCAP log timestamp (apply the same change
to the other write(...) variant referenced at lines 91-92 and to the
timestamp_bindings docs).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 747a3a62-c65b-4486-a3e3-188e3f41127d
📒 Files selected for processing (27)
docs/source/device/add_device.rstsrc/core/deviceio_base/cpp/CMakeLists.txtsrc/core/deviceio_base/cpp/inc/deviceio_base/tracker.hppsrc/core/deviceio_session/cpp/CMakeLists.txtsrc/core/deviceio_session/cpp/deviceio_session.cppsrc/core/deviceio_session/cpp/inc/deviceio_session/deviceio_session.hppsrc/core/deviceio_trackers/cpp/CMakeLists.txtsrc/core/deviceio_trackers/cpp/hand_tracker.cppsrc/core/deviceio_trackers/cpp/inc/deviceio_trackers/hand_tracker.hppsrc/core/deviceio_trackers/python/tracker_bindings.cppsrc/core/live_trackers/cpp/CMakeLists.txtsrc/core/live_trackers/cpp/inc/live_trackers/schema_tracker.hppsrc/core/live_trackers/cpp/live_controller_tracker_impl.cppsrc/core/live_trackers/cpp/live_controller_tracker_impl.hppsrc/core/live_trackers/cpp/live_frame_metadata_tracker_oak_impl.cppsrc/core/live_trackers/cpp/live_frame_metadata_tracker_oak_impl.hppsrc/core/live_trackers/cpp/live_full_body_tracker_pico_impl.cppsrc/core/live_trackers/cpp/live_full_body_tracker_pico_impl.hppsrc/core/live_trackers/cpp/live_generic_3axis_pedal_tracker_impl.cppsrc/core/live_trackers/cpp/live_generic_3axis_pedal_tracker_impl.hppsrc/core/live_trackers/cpp/live_hand_tracker_impl.cppsrc/core/live_trackers/cpp/live_hand_tracker_impl.hppsrc/core/live_trackers/cpp/live_head_tracker_impl.cppsrc/core/live_trackers/cpp/live_head_tracker_impl.hppsrc/core/mcap/cpp/inc/mcap/tracker_channels.hppsrc/core/mcap_tests/cpp/test_mcap_tracker_channels.cppsrc/core/oxr/cpp/inc/oxr/oxr_session.hpp
💤 Files with no reviewable changes (5)
- src/core/deviceio_trackers/cpp/CMakeLists.txt
- src/core/deviceio_trackers/cpp/inc/deviceio_trackers/hand_tracker.hpp
- src/core/deviceio_base/cpp/CMakeLists.txt
- src/core/deviceio_trackers/cpp/hand_tracker.cpp
- src/core/deviceio_session/cpp/inc/deviceio_session/deviceio_session.hpp
Comment on lines
+97
to
+101
| m.attr("NUM_JOINTS") = 26; | ||
| m.attr("JOINT_PALM") = 0; | ||
| m.attr("JOINT_WRIST") = 1; | ||
| m.attr("JOINT_THUMB_TIP") = 5; | ||
| m.attr("JOINT_INDEX_TIP") = 10; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add comments documenting the origin and meaning of these magic numbers.
These hardcoded values replace OpenXR XR_HAND_JOINT_*_EXT constants but lack any documentation. Since these are exported through the public Python API (via deviceio_trackers_init.py and deviceio_init.py), future maintainers need to understand their source and meaning.
📝 Suggested documentation
+ // OpenXR hand joint indices (XR_HAND_JOINT_*_EXT) - hardcoded to avoid OpenXR header dependency.
+ // See: https://registry.khronos.org/OpenXR/specs/1.0/html/xrspec.html#XrHandJointEXT
m.attr("NUM_JOINTS") = 26;
- m.attr("JOINT_PALM") = 0;
- m.attr("JOINT_WRIST") = 1;
- m.attr("JOINT_THUMB_TIP") = 5;
- m.attr("JOINT_INDEX_TIP") = 10;
+ m.attr("JOINT_PALM") = 0; // XR_HAND_JOINT_PALM_EXT
+ m.attr("JOINT_WRIST") = 1; // XR_HAND_JOINT_WRIST_EXT
+ m.attr("JOINT_THUMB_TIP") = 5; // XR_HAND_JOINT_THUMB_TIP_EXT
+ m.attr("JOINT_INDEX_TIP") = 10; // XR_HAND_JOINT_INDEX_TIP_EXT📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| m.attr("NUM_JOINTS") = 26; | |
| m.attr("JOINT_PALM") = 0; | |
| m.attr("JOINT_WRIST") = 1; | |
| m.attr("JOINT_THUMB_TIP") = 5; | |
| m.attr("JOINT_INDEX_TIP") = 10; | |
| // OpenXR hand joint indices (XR_HAND_JOINT_*_EXT) - hardcoded to avoid OpenXR header dependency. | |
| // See: https://registry.khronos.org/OpenXR/specs/1.0/html/xrspec.html#XrHandJointEXT | |
| m.attr("NUM_JOINTS") = 26; | |
| m.attr("JOINT_PALM") = 0; // XR_HAND_JOINT_PALM_EXT | |
| m.attr("JOINT_WRIST") = 1; // XR_HAND_JOINT_WRIST_EXT | |
| m.attr("JOINT_THUMB_TIP") = 5; // XR_HAND_JOINT_THUMB_TIP_EXT | |
| m.attr("JOINT_INDEX_TIP") = 10; // XR_HAND_JOINT_INDEX_TIP_EXT |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/core/deviceio_trackers/python/tracker_bindings.cpp` around lines 97 -
101, These exported Python constants (NUM_JOINTS, JOINT_PALM, JOINT_WRIST,
JOINT_THUMB_TIP, JOINT_INDEX_TIP) are magic numbers mapping to OpenXR
XR_HAND_JOINT_*_EXT indices; add a clear comment block above these assignments
in tracker_bindings.cpp that states they originate from the OpenXR hand-tracking
extension (e.g., XR_EXT_hand_tracking / XR_HAND_JOINT_*_EXT constants), list
which XR constant each integer corresponds to (e.g., JOINT_PALM =
XR_HAND_JOINT_PALM_EXT -> 0), note the OpenXR spec/version/extension used, and
mention these values are exported to the public Python API via
deviceio_trackers_init.py and deviceio_init.py so maintainers know the source
and meaning.
| * are treated as common non-fatal conditions and do not throw. | ||
| */ | ||
| void update(std::shared_ptr<NativeDataT>& out_latest) | ||
| void update(int64_t log_time_ns, std::shared_ptr<NativeDataT>& out_latest) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Document the new log_time_ns parameter in the method comment.
The signature now has log_time_ns, but the doc block only documents out_latest. Keeping these in sync avoids API drift.
Suggested doc patch
- * `@param` out_latest If non-null and samples were read, receives the unpacked
+ * `@param` log_time_ns Monotonic nanoseconds used as MCAP log time for writes.
+ * `@param` out_latest If non-null and samples were read, receives the unpacked
* data from the last sample. Cleared when the tensor collection
* is absent.🤖 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` at line 63,
The comment for SchemaTracker::update currently documents only the out_latest
parameter but the method signature includes a new int64_t log_time_ns parameter;
update the method's docblock in
src/core/live_trackers/cpp/inc/live_trackers/schema_tracker.hpp to add a short
description for log_time_ns (e.g., "log time in nanoseconds used for
versioning/timestamping") alongside the existing out_latest description so the
docblock matches the signature of void update(int64_t log_time_ns,
std::shared_ptr<NativeDataT>& out_latest).
Comment on lines
+82
to
+83
| DeviceDataTimestamp timestamp(last_update_time_ns_, last_update_time_ns_, last_update_time_ns_); | ||
| mcap_channels_->write(0, last_update_time_ns_, timestamp, tracked_.data); |
There was a problem hiding this comment.
All DeviceDataTimestamp fields set identically loses semantic distinction.
Per src/core/schema/fbs/timestamp.fbs, the three fields have different meanings:
available_time_local_common_clock: when sample became available to recording systemsample_time_local_common_clock: when sample was takensample_time_raw_device_clock: measurement source's own clock
Setting all three to the same monotonic value removes the ability to measure pipeline latency (available - sample) and loses device-clock information. If the OpenXR runtime provides the actual sample time (e.g., the XrTime predicted display time), consider preserving that in sample_time_raw_device_clock and computing a more accurate sample_time_local_common_clock.
If this simplification is intentional for now, consider adding a brief comment or TODO indicating future refinement.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/core/live_trackers/cpp/live_head_tracker_impl.cpp` around lines 82 - 83,
DeviceDataTimestamp is being populated with the same monotonic value
(last_update_time_ns_) which loses distinction between available, local-sample,
and device-clock times; change the DeviceDataTimestamp construction used in
mcap_channels_->write so available_time_local_common_clock remains
last_update_time_ns_, sample_time_local_common_clock is set to the best estimate
of when the sample was taken in the local/common clock (compute or convert from
a sample_time_ns or predicted display time if available), and
sample_time_raw_device_clock preserves the source device clock value (e.g., the
OpenXR XrTime if provided); if the runtime does not provide separate sample
times, add a brief TODO comment next to DeviceDataTimestamp usage explaining the
simplification and that future work should populate
sample_time_local_common_clock and sample_time_raw_device_clock from runtime
sample timestamps (reference symbols: DeviceDataTimestamp,
mcap_channels_->write, last_update_time_ns_, tracked_.data).
| auto writer = open_writer(path); | ||
| HeadChannels ch(*writer, "tracking", core::HeadRecordingTraits::schema_name, { "head" }); | ||
| ch.write(0, core::DeviceDataTimestamp(1000000, 1000000, 42), head_data); | ||
| ch.write(0, 1000000, core::DeviceDataTimestamp(1000000, 1000000, 42), head_data); |
There was a problem hiding this comment.
Add a mismatch-value test to validate the new log_time_ns contract.
All updated call sites currently pass the same numeric value for log_time_ns and DeviceDataTimestamp(...), so these tests won’t catch a regression where msg.logTime is sourced from the timestamp struct instead of the explicit parameter.
✅ Minimal test hardening example
- ch.write(0, 1000000, core::DeviceDataTimestamp(1000000, 1000000, 42), head_data);
+ ch.write(0, 2000000, core::DeviceDataTimestamp(1000000, 1000000, 42), head_data);
...
- CHECK(view.message.logTime == 1000000);
+ CHECK(view.message.logTime == 2000000);Also applies to: 132-132, 164-165, 193-193, 207-209, 241-243
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/core/mcap_tests/cpp/test_mcap_tracker_channels.cpp` at line 88, The test
calls to ch.write currently pass identical values for the explicit log_time_ns
parameter and the DeviceDataTimestamp fields, so they won't detect regressions
where msg.logTime is incorrectly taken from the timestamp struct; update each
call site (eg. ch.write(..., core::DeviceDataTimestamp(...), head_data) used
across test_mcap_tracker_channels.cpp) to use mismatched values for the explicit
log_time_ns and the DeviceDataTimestamp(...) arguments (choose distinct
integers) and add assertions that the resulting msg.logTime equals the explicit
log_time_ns parameter (not the DeviceDataTimestamp value) to validate the new
log_time_ns contract.
Comment on lines
+60
to
+63
| void write(size_t channel_index, | ||
| int64_t log_time_ns, | ||
| const DeviceDataTimestamp& timestamp, | ||
| const std::shared_ptr<NativeDataT>& data) |
There was a problem hiding this comment.
Align timestamp contract docs with the new log_time_ns behavior.
write() now sets msg.logTime from the explicit log_time_ns input, but src/core/schema/python/timestamp_bindings.h (Line 18-35 in the provided snippet) still documents available_time_local_common_clock as the MCAP log timestamp. Please update the docs/bindings text to the new contract to avoid consumer-side misinterpretation.
Also applies to: 91-92
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/core/mcap/cpp/inc/mcap/tracker_channels.hpp` around lines 60 - 63, Update
the timestamp binding docs to reflect that the MCAP log timestamp is set from
the explicit log_time_ns parameter (msg.logTime := log_time_ns) in the write()
overloads that accept DeviceDataTimestamp, rather than from
available_time_local_common_clock; change any text mentioning
available_time_local_common_clock as the MCAP log timestamp to describe that
available_time_local_common_clock is a separate field carried in the message and
that log_time_ns is the authoritative MCAP log timestamp (apply the same change
to the other write(...) variant referenced at lines 91-92 and to the
timestamp_bindings docs).
nvddr
force-pushed
the
devdeepr/updateTsDeviceIO
branch
from
722ebe0 to
a46c45f
Compare
nvddr
force-pushed
the
devdeepr/updateTsDeviceIO
branch
2 times, most recently
from
72d32ed to
d150481
Compare
nvddr
force-pushed
the
devdeepr/updateTsDeviceIO
branch
from
d150481 to
8225c7d
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Use Update timestamp for the mcap timestamp.
Summary by CodeRabbit
Refactor
Documentation
Breaking Changes