Add monotonic update time to deviceIO, refactor get extensions.#264
Draft
Add monotonic update time to deviceIO, refactor get extensions.#264
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughSessions and tracker implementations now require an explicit system-monotonic nanosecond timestamp for update calls; many per-tracker Changes
Sequence Diagram(s)sequenceDiagram
actor App
participant DeviceIOSession as DeviceIOSession\n(core)
participant TrackerImpl as TrackerImpl
participant OpenXR as OpenXR
App->>DeviceIOSession: step_time_ns = monotonic_now_ns()\nDeviceIOSession.update(step_time_ns)
DeviceIOSession->>TrackerImpl: impl.update(step_time_ns)
TrackerImpl->>OpenXR: convert_monotonic_ns_to_xrtime(step_time_ns)\nxrLocateSpace(..., XrTime)
OpenXR-->>TrackerImpl: pose/data
TrackerImpl-->>DeviceIOSession: samples / result
DeviceIOSession-->>App: aggregated data / success
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
nvddr
force-pushed
the
devdeepr/addTimeToUpdate
branch
13 times, most recently
from
eb96738 to
27fed4a
Compare
There was a problem hiding this comment.
Pull request overview
This PR updates the DeviceIO update flow to use an explicit system monotonic nanosecond timestamp for each tick, so the same monotonic time can be shared across session updates, tracker polling, and downstream pipelines/recording.
Changes:
- Change
DeviceIOSession/ITrackerSession/ITrackerImplupdate APIs to requiresystem_monotonic_time_nsand pass it through to live tracker implementations (which convert toXrTimeinternally as needed). - Centralize OpenXR extension aggregation via
LiveDeviceIOFactory::get_required_extensions()and remove per-trackerget_required_extensions()APIs. - Update plugins, examples, and TeleopSession to provide monotonic timestamps consistently.
Reviewed changes
Copilot reviewed 53 out of 53 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| src/plugins/manus/core/manus_hand_tracking_plugin.cpp | Passes monotonic ns into DeviceIOSession::update() (adds os_time include). |
| src/plugins/controller_synthetic_hands/synthetic_hands_plugin.cpp | Worker thread now updates DeviceIO with monotonic ns (adds os_time include). |
| src/core/teleop_session_manager_tests/python/test_teleop_session.py | Updates test mock DeviceIOSession.update() signature to accept timestamp. |
| src/core/teleop_session_manager/python/teleop_session.py | Creates/uses a monotonic-time ComputeContext so session + pipeline share the same tick time. |
| src/core/oxr/cpp/inc/oxr/oxr_session.hpp | Switches unique_ptr deleter typedefs to PFN destroy types. |
| src/core/mcap/python/mcap_init.py | Updates usage snippet to call session.update(time.monotonic_ns()). |
| src/core/live_trackers/cpp/live_head_tracker_impl.hpp | Changes tracker impl update signature to monotonic ns; tracks last update time in ns; adds required_extensions(). |
| src/core/live_trackers/cpp/live_head_tracker_impl.cpp | Converts monotonic ns to XrTime internally; updates MCAP timestamp construction. |
| src/core/live_trackers/cpp/live_hand_tracker_impl.hpp | Same monotonic update signature + required_extensions() for hand tracking ext. |
| src/core/live_trackers/cpp/live_hand_tracker_impl.cpp | Converts monotonic ns to XrTime; updates MCAP timestamps. |
| src/core/live_trackers/cpp/live_generic_3axis_pedal_tracker_impl.hpp | Update signature change; required_extensions() defers to SchemaTrackerBase. |
| src/core/live_trackers/cpp/live_generic_3axis_pedal_tracker_impl.cpp | Signature updated (timestamp unused). |
| src/core/live_trackers/cpp/live_full_body_tracker_pico_impl.hpp | Update signature change; required_extensions() returns XR_BD_body_tracking. |
| src/core/live_trackers/cpp/live_full_body_tracker_pico_impl.cpp | Converts monotonic ns to XrTime; updates MCAP timestamps. |
| src/core/live_trackers/cpp/live_frame_metadata_tracker_oak_impl.hpp | Update signature change; required_extensions() defers to SchemaTrackerBase. |
| src/core/live_trackers/cpp/live_frame_metadata_tracker_oak_impl.cpp | Signature updated (timestamp unused). |
| src/core/live_trackers/cpp/live_deviceio_factory.cpp | Adds get_required_extensions() aggregation via dynamic_cast + per-impl required_extensions(). |
| src/core/live_trackers/cpp/live_controller_tracker_impl.hpp | Update signature change; required_extensions() returns action-context ext. |
| src/core/live_trackers/cpp/live_controller_tracker_impl.cpp | Converts monotonic ns to XrTime; updates MCAP timestamps. |
| src/core/live_trackers/cpp/inc/live_trackers/live_deviceio_factory.hpp | Declares LiveDeviceIOFactory::get_required_extensions(). |
| src/core/deviceio_trackers/python/tracker_bindings.cpp | Removes OpenXR include dependency by hardcoding select hand joint constants in Python module attrs. |
| src/core/deviceio_trackers/cpp/inc/deviceio_trackers/head_tracker.hpp | Removes per-tracker get_required_extensions() from public API. |
| src/core/deviceio_trackers/cpp/inc/deviceio_trackers/hand_tracker.hpp | Removes per-tracker get_required_extensions() from public API. |
| src/core/deviceio_trackers/cpp/inc/deviceio_trackers/generic_3axis_pedal_tracker.hpp | Updates doc snippet for new update(system_monotonic_time_ns) call pattern. |
| src/core/deviceio_trackers/cpp/inc/deviceio_trackers/full_body_tracker_pico.hpp | Removes per-tracker get_required_extensions() from public API. |
| src/core/deviceio_trackers/cpp/inc/deviceio_trackers/frame_metadata_tracker_oak.hpp | Updates doc snippet for new update(system_monotonic_time_ns) call pattern. |
| src/core/deviceio_trackers/cpp/inc/deviceio_trackers/controller_tracker.hpp | Removes per-tracker get_required_extensions() from public API. |
| src/core/deviceio_trackers/cpp/head_tracker.cpp | Removes HeadTracker::get_required_extensions() implementation. |
| src/core/deviceio_trackers/cpp/hand_tracker.cpp | Removes HandTracker::get_required_extensions(); introduces local joint-count constant for names. |
| src/core/deviceio_trackers/cpp/generic_3axis_pedal_tracker.cpp | Removes Generic3AxisPedalTracker::get_required_extensions() implementation. |
| src/core/deviceio_trackers/cpp/full_body_tracker_pico.cpp | Removes FullBodyTrackerPico::get_required_extensions() implementation. |
| src/core/deviceio_trackers/cpp/frame_metadata_tracker_oak.cpp | Removes FrameMetadataTrackerOak::get_required_extensions() implementation. |
| src/core/deviceio_trackers/cpp/controller_tracker.cpp | Removes ControllerTracker::get_required_extensions() implementation and related include. |
| src/core/deviceio_session/python/session_bindings.cpp | Makes Python DeviceIOSession.update() require a monotonic ns argument. |
| src/core/deviceio_session/py_utils/inc/deviceio_py_utils/session.hpp | Updates wrapper update() to forward monotonic ns to native session. |
| src/core/deviceio_session/cpp/inc/deviceio_session/deviceio_session.hpp | Updates session API to update(int64_t system_monotonic_time_ns) and removes internal time_converter_. |
| src/core/deviceio_session/cpp/deviceio_session.cpp | Delegates required-extensions aggregation to LiveDeviceIOFactory; passes monotonic ns through to impls. |
| src/core/deviceio_base/cpp/inc/deviceio_base/tracker.hpp | Changes ITrackerImpl::update and ITrackerSession::update to use monotonic ns; removes ITracker::get_required_extensions(). |
| examples/schemaio/pedal_printer.cpp | Updates example to pass core::os_monotonic_now_ns() into session->update(). |
| examples/schemaio/frame_metadata_printer.cpp | Updates example to pass core::os_monotonic_now_ns() into session->update(). |
| examples/retargeting/python/sources_example.py | Updates example to call session.update(time.monotonic_ns()). |
| examples/oxr/python/test_synthetic_hands.py | Updates example to call deviceio_session.update(time.monotonic_ns()). |
| examples/oxr/python/test_oak_camera.py | Updates example to call session.update(time.monotonic_ns()). |
| examples/oxr/python/test_modular.py | Updates example to call session.update(time.monotonic_ns()). |
| examples/oxr/python/test_hand_inactive_on_plugin_stop.py | Updates example to call deviceio_session.update(time.monotonic_ns()). |
| examples/oxr/python/test_full_body_tracker.py | Updates example to call session.update(time.monotonic_ns()). |
| examples/oxr/python/test_extensions.py | Adds import time and updates quick update call to pass monotonic ns. |
| examples/oxr/python/test_controller_tracker.py | Updates example to call session.update(time.monotonic_ns()). |
| examples/oxr/python/modular_example_with_mcap.py | Updates example to call session.update(time.monotonic_ns()). |
| examples/oxr/python/modular_example.py | Updates example to call session.update(time.monotonic_ns()). |
| examples/oxr/cpp/oxr_simple_api_demo.cpp | Updates example to pass core::os_monotonic_now_ns() into session->update(). |
| examples/oxr/cpp/oxr_session_sharing.cpp | Uses one monotonic ns value for both sessions’ updates per tick. |
| examples/lerobot/record.py | Updates example to call session.update(time.monotonic_ns()). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| * "oak_camera", {StreamType_Color, StreamType_MonoLeft}); | ||
| * // ... create session with tracker ... | ||
| * session->update(); | ||
| * session->update(system_monotonic_time_ns); |
There was a problem hiding this comment.
The updated example now calls session->update(system_monotonic_time_ns) but doesn't show how to obtain system_monotonic_time_ns, making the snippet incomplete. Consider using core::os_monotonic_now_ns() inline (or documenting that callers should pass a monotonic ns timestamp).
Suggested change
| * session->update(system_monotonic_time_ns); | |
| * session->update(core::os_monotonic_now_ns()); |
| with DeviceIOSession.run(trackers, handles, config) as session: | ||
| while running: | ||
| session.update() # writes to MCAP automatically | ||
| session.update(time.monotonic_ns()) # writes to MCAP automatically |
There was a problem hiding this comment.
This usage snippet now calls time.monotonic_ns() but the example does not import time, so copy/pasting the documented example will fail. Update the docstring example to include the required import (or use a fully-qualified helper if available).
src/core/deviceio_trackers/cpp/inc/deviceio_trackers/generic_3axis_pedal_tracker.hpp
Outdated
Show resolved
Hide resolved
Comment on lines
+98
to
+102
| 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.
These hard-coded joint constants duplicate OpenXR hand joint indices. Without an explanatory comment or a single shared constant source, this can drift from the actual joint ordering and is hard to audit. Consider adding a comment referencing the OpenXR XR_EXT_hand_tracking joint index mapping (and/or defining these values in one shared header used by both C++ and bindings).
src/core/live_trackers/cpp/live_full_body_tracker_pico_impl.cpp
Outdated
Show resolved
Hide resolved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/live_trackers/cpp/live_deviceio_factory.cpp`:
- Around line 44-65: The loop in LiveDeviceIOFactory::get_required_extensions
silently ignores trackers that don't match any try_add_extensions
specialization; add detection and a warning when no specialization matched. For
each tracker in trackers, capture the boolean result of the chained
try_add_extensions calls (e.g. bool matched = try_add_extensions<...>(...) ||
... ); if matched is false, emit a log/warning (use your existing logging
facility) that includes identifying info about the tracker (e.g.
typeid(*tracker).name() or a virtual tracker->name() if available) so
unrecognized tracker types are visible and can be added to the chain.
In `@src/core/mcap/python/mcap_init.py`:
- Line 18: The docstring example in mcap_init.py calls time.monotonic_ns() but
never imports time; update the example to add an import time statement (e.g.,
alongside existing imports in the docstring) so the snippet runs as-is;
reference the example that shows session.update(time.monotonic_ns()) in the
mcap_init.py docstring and ensure the import appears before that usage.
In `@src/core/teleop_session_manager_tests/python/test_teleop_session.py`:
- Line 309: The test method update currently defines an unused parameter named
step_time_ns (def update(self, step_time_ns)) which triggers Ruff ARG002; rename
the parameter to _step_time_ns in the update method signature (and update any
local references if they exist) so the linter recognizes it as intentionally
unused, leaving the rest of the method body unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 58232d80-20d2-4643-af8d-baa65da708be
📒 Files selected for processing (53)
examples/lerobot/record.pyexamples/oxr/cpp/oxr_session_sharing.cppexamples/oxr/cpp/oxr_simple_api_demo.cppexamples/oxr/python/modular_example.pyexamples/oxr/python/modular_example_with_mcap.pyexamples/oxr/python/test_controller_tracker.pyexamples/oxr/python/test_extensions.pyexamples/oxr/python/test_full_body_tracker.pyexamples/oxr/python/test_hand_inactive_on_plugin_stop.pyexamples/oxr/python/test_modular.pyexamples/oxr/python/test_oak_camera.pyexamples/oxr/python/test_synthetic_hands.pyexamples/retargeting/python/sources_example.pyexamples/schemaio/frame_metadata_printer.cppexamples/schemaio/pedal_printer.cppsrc/core/deviceio_base/cpp/inc/deviceio_base/tracker.hppsrc/core/deviceio_session/cpp/deviceio_session.cppsrc/core/deviceio_session/cpp/inc/deviceio_session/deviceio_session.hppsrc/core/deviceio_session/py_utils/inc/deviceio_py_utils/session.hppsrc/core/deviceio_session/python/session_bindings.cppsrc/core/deviceio_trackers/cpp/controller_tracker.cppsrc/core/deviceio_trackers/cpp/frame_metadata_tracker_oak.cppsrc/core/deviceio_trackers/cpp/full_body_tracker_pico.cppsrc/core/deviceio_trackers/cpp/generic_3axis_pedal_tracker.cppsrc/core/deviceio_trackers/cpp/hand_tracker.cppsrc/core/deviceio_trackers/cpp/head_tracker.cppsrc/core/deviceio_trackers/cpp/inc/deviceio_trackers/controller_tracker.hppsrc/core/deviceio_trackers/cpp/inc/deviceio_trackers/frame_metadata_tracker_oak.hppsrc/core/deviceio_trackers/cpp/inc/deviceio_trackers/full_body_tracker_pico.hppsrc/core/deviceio_trackers/cpp/inc/deviceio_trackers/generic_3axis_pedal_tracker.hppsrc/core/deviceio_trackers/cpp/inc/deviceio_trackers/hand_tracker.hppsrc/core/deviceio_trackers/cpp/inc/deviceio_trackers/head_tracker.hppsrc/core/deviceio_trackers/python/tracker_bindings.cppsrc/core/live_trackers/cpp/inc/live_trackers/live_deviceio_factory.hppsrc/core/live_trackers/cpp/live_controller_tracker_impl.cppsrc/core/live_trackers/cpp/live_controller_tracker_impl.hppsrc/core/live_trackers/cpp/live_deviceio_factory.cppsrc/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/python/mcap_init.pysrc/core/oxr/cpp/inc/oxr/oxr_session.hppsrc/core/teleop_session_manager/python/teleop_session.pysrc/core/teleop_session_manager_tests/python/test_teleop_session.pysrc/plugins/controller_synthetic_hands/synthetic_hands_plugin.cppsrc/plugins/manus/core/manus_hand_tracking_plugin.cpp
💤 Files with no reviewable changes (9)
- src/core/deviceio_trackers/cpp/controller_tracker.cpp
- src/core/deviceio_trackers/cpp/full_body_tracker_pico.cpp
- src/core/deviceio_trackers/cpp/head_tracker.cpp
- src/core/deviceio_trackers/cpp/generic_3axis_pedal_tracker.cpp
- src/core/deviceio_trackers/cpp/frame_metadata_tracker_oak.cpp
- src/core/deviceio_trackers/cpp/inc/deviceio_trackers/hand_tracker.hpp
- src/core/deviceio_trackers/cpp/inc/deviceio_trackers/head_tracker.hpp
- src/core/deviceio_trackers/cpp/inc/deviceio_trackers/full_body_tracker_pico.hpp
- src/core/deviceio_trackers/cpp/inc/deviceio_trackers/controller_tracker.hpp
src/core/teleop_session_manager_tests/python/test_teleop_session.py
Outdated
Show resolved
Hide resolved
nvddr
force-pushed
the
devdeepr/addTimeToUpdate
branch
2 times, most recently
from
836f24d to
e755e24
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/core/deviceio_session/cpp/deviceio_session.cpp (1)
105-125:⚠️ Potential issue | 🟠 MajorReturn an aggregate failure status from
update().
impl->update(...)failures are only logged here; the session still returnstrueunconditionally. That drops the only error signal callers have to suppress downstream work on stale tracker state, e.g.src/plugins/manus/core/manus_hand_tracking_plugin.cpp:36-45.💡 Possible fix
bool DeviceIOSession::update(int64_t system_monotonic_time_ns) { + bool all_updates_succeeded = true; for (auto& [tracker, impl] : tracker_impls_) { if (!impl->update(system_monotonic_time_ns)) { + all_updates_succeeded = false; auto& count = tracker_update_failure_counts_[tracker]; count++; if (count == 1 || count % 1000 == 0) { std::cerr << "Warning: tracker '" << tracker->get_name() << "' update failed (count: " << count << ")" << std::endl; } } else { tracker_update_failure_counts_[tracker] = 0; } } - return true; + return all_updates_succeeded; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/deviceio_session/cpp/deviceio_session.cpp` around lines 105 - 125, DeviceIOSession::update currently logs individual impl->update failures but always returns true; change it to return an aggregate success flag: initialize a bool success = true at start of DeviceIOSession::update, set success = false whenever any impl->update(system_monotonic_time_ns) returns false (while keeping the existing tracker_update_failure_counts_ increment/logging and reset behavior), and return success at the end so callers receive false when any tracker update failed.
♻️ Duplicate comments (1)
src/core/teleop_session_manager_tests/python/test_teleop_session.py (1)
309-309: 🧹 Nitpick | 🔵 TrivialPrefix the unused mock parameter to satisfy Ruff ARG002.
The
step_time_nsparameter is intentionally unused in this mock; rename it to_step_time_nsto silence the lint warning.💡 Proposed fix
- def update(self, step_time_ns): + def update(self, _step_time_ns): self.update_count += 1 return True🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/teleop_session_manager_tests/python/test_teleop_session.py` at line 309, The mock method update currently declares an unused parameter named step_time_ns which triggers Ruff ARG002; rename that parameter to _step_time_ns in the update method signature (the mock's update function) so the linter recognizes it as intentionally unused, and update any internal references if present to use the new name.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/source/device/add_device.rst`:
- Line 160: The docs still reference the old session->update() signatures;
update every occurrence to use the new timestamped signature
session->update(int64_t system_monotonic_time_ns) (and where examples show
calling core::os_monotonic_now_ns(), use
session->update(core::os_monotonic_now_ns())). Also ensure related calls like
tracker->get_data(*session) remain consistent with the timestamped-update
workflow and adjust prose/examples to mention the parameter name
system_monotonic_time_ns.
In `@src/core/deviceio_session/cpp/deviceio_session.cpp`:
- Around line 87-89: The current DeviceIOSession::get_required_extensions simply
forwards to LiveDeviceIOFactory::get_required_extensions which unconditionally
adds XrTimeConverter extension set; modify the logic so that when the incoming
trackers list is empty you return only the minimal/common extensions (or an
empty vector) and do NOT include XrTimeConverter-related extensions—i.e., in
DeviceIOSession::get_required_extensions (or by adjusting
LiveDeviceIOFactory::get_required_extensions) add a special-case early return
for trackers.empty() before any code that appends XrTimeConverter extensions.
In `@src/core/teleop_session_manager/python/teleop_session.py`:
- Around line 224-231: DeviceIO update is being driven from
context.graph_time.real_time_ns which can be from another clock domain; change
the call to use a system monotonic timestamp instead. Locate the code in
teleop_session.py where step_time_ns is set from context.graph_time.real_time_ns
and used in self.deviceio_session.update(...), compute a monotonic time (e.g.
via time.monotonic_ns()) and pass that value to DeviceIOSession.update so
trackers always receive a monotonic timestamp.
In `@src/core/time_utils/cpp/CMakeLists.txt`:
- Around line 26-29: The CMake install currently writes export targets via
install(EXPORT time_utils-targets ...) but does not generate a package config
file for external consumers; add a packaged config step using
CMakePackageConfigHelpers and configure_package_config_file to produce
time_utils-config.cmake (from a time_utils-config.cmake.in) into the build dir
and install it alongside time_utils-targets.cmake so that
find_package(time_utils) can locate the exported targets; ensure you call
include(CMakePackageConfigHelpers), configure_package_config_file(...) to create
time_utils-config.cmake in CMAKE_CURRENT_BINARY_DIR and then install(FILES
"${CMAKE_CURRENT_BINARY_DIR}/time_utils-config.cmake" DESTINATION
lib/cmake/time_utils).
In `@src/plugins/controller_synthetic_hands/README.md`:
- Line 108: The README snippets call core::os_monotonic_now_ns() but are missing
the header; add the include for time_utils/os_time.hpp to both examples so
core::os_monotonic_now_ns() is available (insert `#include`
<time_utils/os_time.hpp> above the existing includes in the snippets that define
controller_tracker and the snippet that calls
deviceio_session->update(core::os_monotonic_now_ns());).
---
Outside diff comments:
In `@src/core/deviceio_session/cpp/deviceio_session.cpp`:
- Around line 105-125: DeviceIOSession::update currently logs individual
impl->update failures but always returns true; change it to return an aggregate
success flag: initialize a bool success = true at start of
DeviceIOSession::update, set success = false whenever any
impl->update(system_monotonic_time_ns) returns false (while keeping the existing
tracker_update_failure_counts_ increment/logging and reset behavior), and return
success at the end so callers receive false when any tracker update failed.
---
Duplicate comments:
In `@src/core/teleop_session_manager_tests/python/test_teleop_session.py`:
- Line 309: The mock method update currently declares an unused parameter named
step_time_ns which triggers Ruff ARG002; rename that parameter to _step_time_ns
in the update method signature (the mock's update function) so the linter
recognizes it as intentionally unused, and update any internal references if
present to use the new name.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 34d9441b-5513-4834-99ac-4c9c85731a10
📒 Files selected for processing (69)
docs/source/device/add_device.rstexamples/lerobot/record.pyexamples/oxr/cpp/oxr_session_sharing.cppexamples/oxr/cpp/oxr_simple_api_demo.cppexamples/oxr/python/modular_example.pyexamples/oxr/python/modular_example_with_mcap.pyexamples/oxr/python/test_controller_tracker.pyexamples/oxr/python/test_extensions.pyexamples/oxr/python/test_full_body_tracker.pyexamples/oxr/python/test_hand_inactive_on_plugin_stop.pyexamples/oxr/python/test_modular.pyexamples/oxr/python/test_oak_camera.pyexamples/oxr/python/test_session_sharing.pyexamples/oxr/python/test_synthetic_hands.pyexamples/retargeting/python/sources_example.pyexamples/schemaio/frame_metadata_printer.cppexamples/schemaio/pedal_printer.cppsrc/core/CMakeLists.txtsrc/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_session/py_utils/inc/deviceio_py_utils/session.hppsrc/core/deviceio_session/python/CMakeLists.txtsrc/core/deviceio_session/python/session_bindings.cppsrc/core/deviceio_trackers/cpp/CMakeLists.txtsrc/core/deviceio_trackers/cpp/controller_tracker.cppsrc/core/deviceio_trackers/cpp/frame_metadata_tracker_oak.cppsrc/core/deviceio_trackers/cpp/full_body_tracker_pico.cppsrc/core/deviceio_trackers/cpp/generic_3axis_pedal_tracker.cppsrc/core/deviceio_trackers/cpp/hand_tracker.cppsrc/core/deviceio_trackers/cpp/head_tracker.cppsrc/core/deviceio_trackers/cpp/inc/deviceio_trackers/controller_tracker.hppsrc/core/deviceio_trackers/cpp/inc/deviceio_trackers/frame_metadata_tracker_oak.hppsrc/core/deviceio_trackers/cpp/inc/deviceio_trackers/full_body_tracker_pico.hppsrc/core/deviceio_trackers/cpp/inc/deviceio_trackers/generic_3axis_pedal_tracker.hppsrc/core/deviceio_trackers/cpp/inc/deviceio_trackers/hand_tracker.hppsrc/core/deviceio_trackers/cpp/inc/deviceio_trackers/head_tracker.hppsrc/core/deviceio_trackers/python/tracker_bindings.cppsrc/core/live_trackers/cpp/CMakeLists.txtsrc/core/live_trackers/cpp/inc/live_trackers/live_deviceio_factory.hppsrc/core/live_trackers/cpp/live_controller_tracker_impl.cppsrc/core/live_trackers/cpp/live_controller_tracker_impl.hppsrc/core/live_trackers/cpp/live_deviceio_factory.cppsrc/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/python/mcap_init.pysrc/core/oxr/cpp/inc/oxr/oxr_session.hppsrc/core/oxr_utils/cpp/CMakeLists.txtsrc/core/oxr_utils/cpp/inc/oxr_utils/oxr_time.hppsrc/core/teleop_session_manager/python/teleop_session.pysrc/core/teleop_session_manager_tests/python/test_teleop_session.pysrc/core/time_utils/CMakeLists.txtsrc/core/time_utils/cpp/CMakeLists.txtsrc/core/time_utils/cpp/inc/time_utils/os_time.hppsrc/plugins/controller_synthetic_hands/README.mdsrc/plugins/controller_synthetic_hands/synthetic_hands_plugin.cppsrc/plugins/generic_3axis_pedal/generic_3axis_pedal_plugin.cppsrc/plugins/manus/core/manus_hand_tracking_plugin.cppsrc/plugins/oak/core/frame_sink.cpp
💤 Files with no reviewable changes (11)
- src/core/deviceio_trackers/cpp/CMakeLists.txt
- src/core/deviceio_trackers/cpp/inc/deviceio_trackers/controller_tracker.hpp
- src/core/deviceio_trackers/cpp/inc/deviceio_trackers/head_tracker.hpp
- src/core/deviceio_trackers/cpp/frame_metadata_tracker_oak.cpp
- src/core/deviceio_trackers/cpp/generic_3axis_pedal_tracker.cpp
- src/core/deviceio_trackers/cpp/inc/deviceio_trackers/hand_tracker.hpp
- src/core/deviceio_base/cpp/CMakeLists.txt
- src/core/deviceio_trackers/cpp/inc/deviceio_trackers/full_body_tracker_pico.hpp
- src/core/deviceio_trackers/cpp/controller_tracker.cpp
- src/core/deviceio_trackers/cpp/head_tracker.cpp
- src/core/deviceio_trackers/cpp/full_body_tracker_pico.cpp
Comment on lines
+26
to
+29
| install(EXPORT time_utils-targets | ||
| FILE time_utils-targets.cmake | ||
| NAMESPACE core:: | ||
| DESTINATION lib/cmake/time_utils |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding a config file for external package consumers.
The export targets are installed, but there's no time_utils-config.cmake generated. External consumers using find_package(time_utils) would need this config file to locate the exported targets. If this library is only consumed internally within the build tree, this is fine. Otherwise, consider adding:
include(CMakePackageConfigHelpers)
configure_package_config_file(
"${CMAKE_CURRENT_SOURCE_DIR}/time_utils-config.cmake.in"
"${CMAKE_CURRENT_BINARY_DIR}/time_utils-config.cmake"
INSTALL_DESTINATION lib/cmake/time_utils
)
install(FILES "${CMAKE_CURRENT_BINARY_DIR}/time_utils-config.cmake"
DESTINATION lib/cmake/time_utils)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/core/time_utils/cpp/CMakeLists.txt` around lines 26 - 29, The CMake
install currently writes export targets via install(EXPORT time_utils-targets
...) but does not generate a package config file for external consumers; add a
packaged config step using CMakePackageConfigHelpers and
configure_package_config_file to produce time_utils-config.cmake (from a
time_utils-config.cmake.in) into the build dir and install it alongside
time_utils-targets.cmake so that find_package(time_utils) can locate the
exported targets; ensure you call include(CMakePackageConfigHelpers),
configure_package_config_file(...) to create time_utils-config.cmake in
CMAKE_CURRENT_BINARY_DIR and then install(FILES
"${CMAKE_CURRENT_BINARY_DIR}/time_utils-config.cmake" DESTINATION
lib/cmake/time_utils).
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/core/deviceio_trackers/cpp/inc/deviceio_trackers/generic_3axis_pedal_tracker.hpp (1)
19-23:⚠️ Potential issue | 🟡 MinorUpdate the prose to match the new
update(...)API.The example on Line 36 was fixed, but the surrounding doc block still mentions
ITrackerSession::update()/update()with no timestamp on Lines 22 and 68. That leaves the public tracker docs describing two different signatures.✏️ Suggested doc cleanup
- * documented elsewhere. After each ``ITrackerSession::update()`` that includes this tracker, ``get_data(session)`` + * documented elsewhere. After each session update tick that includes this tracker, ``get_data(session)`` ... - * from the previous ``update()`` if that tick produced no new samples (see ``LiveGeneric3AxisPedalTrackerImpl`` + * from the previous tick if this tick produced no new samples (see ``LiveGeneric3AxisPedalTrackerImpl``Also applies to: 32-38, 64-69
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/deviceio_trackers/cpp/inc/deviceio_trackers/generic_3axis_pedal_tracker.hpp` around lines 19 - 23, Update the docblock to reference the new update(timestamp) API signature instead of the old ITrackerSession::update() / update() wording: change the sentences that say "After each ITrackerSession::update() that includes this tracker" and any other mentions (e.g., near get_data(session) and the LiveGeneric3AxisPedalTrackerImpl::update reference) to clearly state "After each update(...) call with the session timestamp (e.g., ITrackerSession::update(timestamp) or LiveGeneric3AxisPedalTrackerImpl::update(timestamp))" so the documentation consistently reflects the new timestamped update(...) API across the class (including the prose around Generic3AxisPedalOutput, get_data(session), and the LiveGeneric3AxisPedalTrackerImpl::update mention).
♻️ Duplicate comments (3)
src/plugins/controller_synthetic_hands/README.md (1)
108-108:⚠️ Potential issue | 🟡 MinorAdd the missing time-utils include in both code snippets.
Lines 108 and 149 call
core::os_monotonic_now_ns(), but the snippet headers omit#include <time_utils/os_time.hpp>, so the examples are incomplete.Also applies to: 149-149
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/plugins/controller_synthetic_hands/README.md` at line 108, The README code examples call core::os_monotonic_now_ns() but omit the header; add `#include` <time_utils/os_time.hpp> to the snippet headers so both examples compile—update the snippets around deviceio_session->update(core::os_monotonic_now_ns()) (and the similar call at the other snippet) to include the time_utils/os_time.hpp include line.docs/source/device/add_device.rst (1)
160-160:⚠️ Potential issue | 🟡 MinorComplete the API migration on this page to avoid contradictory docs.
Line 160 uses the new timestamped call, but Lines 120-121 and Lines 216-217 still describe
update(XrTime)/update()semantics. Please align all references toupdate(int64_t system_monotonic_time_ns).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/source/device/add_device.rst` at line 160, The page mixes old and new API forms; update all mentions and examples of the old session->update() and update(XrTime) semantics to the new signature session->update(int64_t system_monotonic_time_ns). Replace descriptive text and code snippets that refer to update() or update(XrTime) with the timestamped pattern (e.g., session->update(core::os_monotonic_now_ns())), ensure any loop examples show calling session->update(core::os_monotonic_now_ns()) followed by tracker->get_data(*session), and update explanatory wording to reference update(int64_t system_monotonic_time_ns) consistently (search for symbols: session->update, update(), update(XrTime), tracker->get_data).src/core/teleop_session_manager/python/teleop_session.py (1)
216-225:⚠️ Potential issue | 🟠 MajorKeep DeviceIO on the local monotonic clock.
Line 224 still derives
step_time_nsfrom caller-ownedcontext.graph_time.real_time_ns.DeviceIOSession::update(...)forwards that value into live tracker impls that convert monotonic ns toXrTime, so a custom context can freeze, rewind, or shift tracker queries onto the wrong clock domain.🔧 Suggested fix
- if context is None: - now_ns = time.monotonic_ns() - context = ComputeContext( - graph_time=GraphTime(sim_time_ns=now_ns, real_time_ns=now_ns) - ) - - step_time_ns = context.graph_time.real_time_ns + step_time_ns = time.monotonic_ns() + if context is None: + context = ComputeContext( + graph_time=GraphTime(sim_time_ns=step_time_ns, real_time_ns=step_time_ns) + )Also applies to: 230-231
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/teleop_session_manager/python/teleop_session.py` around lines 216 - 225, The code currently sets step_time_ns from the caller-owned context.graph_time.real_time_ns which can put DeviceIOSession updates onto the wrong clock domain; instead always use the local monotonic clock for DeviceIO operations: replace uses that set step_time_ns = context.graph_time.real_time_ns with step_time_ns = time.monotonic_ns(), leave ComputeContext/GraphTime creation intact, and apply the same change to the other occurrence around where step_time_ns is computed (lines noted in the review) so DeviceIOSession.update(...) and downstream live tracker conversions always get a monotonic timestamp.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@examples/oxr/python/modular_example_with_mcap.py`:
- Around line 73-74: The loop calling session.update(time.monotonic_ns()) should
check its boolean return value and break on failure to match other examples;
modify the while loop so it captures the return (e.g., ok =
session.update(time.monotonic_ns())) and if ok is False (or not truthy) break
out of the loop, preserving the same 30s window behavior and using the same
monotonic timestamp source.
In `@src/core/live_trackers/cpp/live_head_tracker_impl.hpp`:
- Around line 24-27: The required_extensions() implementation in
LiveHeadTrackerImpl currently returns an empty vector but must include the
extensions required by the owned time_converter_; update required_extensions()
to call XrTimeConverter::get_required_extensions() (or forward from the instance
time_converter_) and append/merge its returned vector into the result so
platform-specific XR extensions (e.g.,
XR_KHR_WIN32_CONVERT_PERFORMANCE_COUNTER_TIME_EXTENSION_NAME) are propagated;
ensure LiveHeadTrackerImpl::required_extensions() returns that combined list so
the constructor and update() time conversion using time_converter_ will have the
needed extensions available.
---
Outside diff comments:
In
`@src/core/deviceio_trackers/cpp/inc/deviceio_trackers/generic_3axis_pedal_tracker.hpp`:
- Around line 19-23: Update the docblock to reference the new update(timestamp)
API signature instead of the old ITrackerSession::update() / update() wording:
change the sentences that say "After each ITrackerSession::update() that
includes this tracker" and any other mentions (e.g., near get_data(session) and
the LiveGeneric3AxisPedalTrackerImpl::update reference) to clearly state "After
each update(...) call with the session timestamp (e.g.,
ITrackerSession::update(timestamp) or
LiveGeneric3AxisPedalTrackerImpl::update(timestamp))" so the documentation
consistently reflects the new timestamped update(...) API across the class
(including the prose around Generic3AxisPedalOutput, get_data(session), and the
LiveGeneric3AxisPedalTrackerImpl::update mention).
---
Duplicate comments:
In `@docs/source/device/add_device.rst`:
- Line 160: The page mixes old and new API forms; update all mentions and
examples of the old session->update() and update(XrTime) semantics to the new
signature session->update(int64_t system_monotonic_time_ns). Replace descriptive
text and code snippets that refer to update() or update(XrTime) with the
timestamped pattern (e.g., session->update(core::os_monotonic_now_ns())), ensure
any loop examples show calling session->update(core::os_monotonic_now_ns())
followed by tracker->get_data(*session), and update explanatory wording to
reference update(int64_t system_monotonic_time_ns) consistently (search for
symbols: session->update, update(), update(XrTime), tracker->get_data).
In `@src/core/teleop_session_manager/python/teleop_session.py`:
- Around line 216-225: The code currently sets step_time_ns from the
caller-owned context.graph_time.real_time_ns which can put DeviceIOSession
updates onto the wrong clock domain; instead always use the local monotonic
clock for DeviceIO operations: replace uses that set step_time_ns =
context.graph_time.real_time_ns with step_time_ns = time.monotonic_ns(), leave
ComputeContext/GraphTime creation intact, and apply the same change to the other
occurrence around where step_time_ns is computed (lines noted in the review) so
DeviceIOSession.update(...) and downstream live tracker conversions always get a
monotonic timestamp.
In `@src/plugins/controller_synthetic_hands/README.md`:
- Line 108: The README code examples call core::os_monotonic_now_ns() but omit
the header; add `#include` <time_utils/os_time.hpp> to the snippet headers so both
examples compile—update the snippets around
deviceio_session->update(core::os_monotonic_now_ns()) (and the similar call at
the other snippet) to include the time_utils/os_time.hpp include line.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 32e7e49f-87e3-40ab-b2f4-b4f8e9a04fbe
📒 Files selected for processing (77)
docs/source/device/add_device.rstexamples/lerobot/record.pyexamples/oxr/cpp/oxr_session_sharing.cppexamples/oxr/cpp/oxr_simple_api_demo.cppexamples/oxr/python/modular_example.pyexamples/oxr/python/modular_example_with_mcap.pyexamples/oxr/python/test_controller_tracker.pyexamples/oxr/python/test_extensions.pyexamples/oxr/python/test_full_body_tracker.pyexamples/oxr/python/test_hand_inactive_on_plugin_stop.pyexamples/oxr/python/test_modular.pyexamples/oxr/python/test_oak_camera.pyexamples/oxr/python/test_session_sharing.pyexamples/oxr/python/test_synthetic_hands.pyexamples/retargeting/python/sources_example.pyexamples/schemaio/frame_metadata_printer.cppexamples/schemaio/pedal_printer.cppsrc/core/CMakeLists.txtsrc/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_session/py_utils/inc/deviceio_py_utils/session.hppsrc/core/deviceio_session/python/CMakeLists.txtsrc/core/deviceio_session/python/session_bindings.cppsrc/core/deviceio_trackers/cpp/CMakeLists.txtsrc/core/deviceio_trackers/cpp/controller_tracker.cppsrc/core/deviceio_trackers/cpp/frame_metadata_tracker_oak.cppsrc/core/deviceio_trackers/cpp/full_body_tracker_pico.cppsrc/core/deviceio_trackers/cpp/generic_3axis_pedal_tracker.cppsrc/core/deviceio_trackers/cpp/hand_tracker.cppsrc/core/deviceio_trackers/cpp/head_tracker.cppsrc/core/deviceio_trackers/cpp/inc/deviceio_trackers/controller_tracker.hppsrc/core/deviceio_trackers/cpp/inc/deviceio_trackers/frame_metadata_tracker_oak.hppsrc/core/deviceio_trackers/cpp/inc/deviceio_trackers/full_body_tracker_pico.hppsrc/core/deviceio_trackers/cpp/inc/deviceio_trackers/generic_3axis_pedal_tracker.hppsrc/core/deviceio_trackers/cpp/inc/deviceio_trackers/hand_tracker.hppsrc/core/deviceio_trackers/cpp/inc/deviceio_trackers/head_tracker.hppsrc/core/deviceio_trackers/python/tracker_bindings.cppsrc/core/live_trackers/cpp/CMakeLists.txtsrc/core/live_trackers/cpp/inc/live_trackers/live_deviceio_factory.hppsrc/core/live_trackers/cpp/live_controller_tracker_impl.cppsrc/core/live_trackers/cpp/live_controller_tracker_impl.hppsrc/core/live_trackers/cpp/live_deviceio_factory.cppsrc/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/live_trackers/cpp/schema_tracker_base.cppsrc/core/mcap/python/mcap_init.pysrc/core/mcap_tests/cpp/test_mcap_tracker_channels.cppsrc/core/oxr/cpp/inc/oxr/oxr_session.hppsrc/core/oxr_utils/cpp/CMakeLists.txtsrc/core/oxr_utils/cpp/inc/oxr_utils/oxr_time.hppsrc/core/schema_tests/python/test_camera.pysrc/core/schema_tests/python/test_controller.pysrc/core/schema_tests/python/test_full_body.pysrc/core/schema_tests/python/test_hand.pysrc/core/schema_tests/python/test_head.pysrc/core/schema_tests/python/test_pedals.pysrc/core/teleop_session_manager/python/teleop_session.pysrc/core/teleop_session_manager_tests/python/test_teleop_session.pysrc/core/time_utils/CMakeLists.txtsrc/core/time_utils/cpp/CMakeLists.txtsrc/core/time_utils/cpp/inc/time_utils/os_time.hppsrc/plugins/controller_synthetic_hands/README.mdsrc/plugins/controller_synthetic_hands/synthetic_hands_plugin.cppsrc/plugins/generic_3axis_pedal/generic_3axis_pedal_plugin.cppsrc/plugins/manus/core/manus_hand_tracking_plugin.cppsrc/plugins/oak/core/frame_sink.cpp
💤 Files with no reviewable changes (11)
- src/core/deviceio_trackers/cpp/frame_metadata_tracker_oak.cpp
- src/core/deviceio_trackers/cpp/inc/deviceio_trackers/controller_tracker.hpp
- src/core/deviceio_trackers/cpp/inc/deviceio_trackers/head_tracker.hpp
- src/core/deviceio_trackers/cpp/inc/deviceio_trackers/hand_tracker.hpp
- src/core/deviceio_trackers/cpp/generic_3axis_pedal_tracker.cpp
- src/core/deviceio_trackers/cpp/inc/deviceio_trackers/full_body_tracker_pico.hpp
- src/core/deviceio_trackers/cpp/head_tracker.cpp
- src/core/deviceio_base/cpp/CMakeLists.txt
- src/core/deviceio_trackers/cpp/controller_tracker.cpp
- src/core/deviceio_trackers/cpp/full_body_tracker_pico.cpp
- src/core/deviceio_trackers/cpp/CMakeLists.txt
Comment on lines
+24
to
+27
| static std::vector<std::string> required_extensions() | ||
| { | ||
| return {}; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify that LiveHeadTrackerImpl depends on XrTimeConverter and that required_extensions currently returns {}.
rg -n -C3 'required_extensions\(|time_converter_|convert_monotonic_ns_to_xrtime|XrTimeConverter::get_required_extensions' \
src/core/live_trackers/cpp/live_head_tracker_impl.hpp \
src/core/live_trackers/cpp/live_head_tracker_impl.cpp \
src/core/oxr_utils/cpp/inc/oxr_utils/oxr_time.hppRepository: NVIDIA/IsaacTeleop
Length of output: 5294
required_extensions() must forward XrTimeConverter extension requirements.
LiveHeadTrackerImpl owns a time_converter_ member (line 42) that is initialized in the constructor and used in update() to convert monotonic time to XrTime. The XrTimeConverter::get_required_extensions() returns platform-specific OpenXR extensions (e.g., XR_KHR_WIN32_CONVERT_PERFORMANCE_COUNTER_TIME_EXTENSION_NAME on Windows). Returning an empty vector prevents these extensions from being declared, causing runtime failures when time conversion is attempted.
Proposed fix
static std::vector<std::string> required_extensions()
{
- return {};
+ return XrTimeConverter::get_required_extensions();
}🤖 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.hpp` around lines 24 - 27,
The required_extensions() implementation in LiveHeadTrackerImpl currently
returns an empty vector but must include the extensions required by the owned
time_converter_; update required_extensions() to call
XrTimeConverter::get_required_extensions() (or forward from the instance
time_converter_) and append/merge its returned vector into the result so
platform-specific XR extensions (e.g.,
XR_KHR_WIN32_CONVERT_PERFORMANCE_COUNTER_TIME_EXTENSION_NAME) are propagated;
ensure LiveHeadTrackerImpl::required_extensions() returns that combined list so
the constructor and update() time conversion using time_converter_ will have the
needed extensions available.
e755e24 to
0ca8fc0
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
src/core/time_utils/cpp/CMakeLists.txt (1)
26-29: 🧹 Nitpick | 🔵 Trivial
find_package(time_utils)packaging is still incomplete without a config file.This repeats earlier feedback: exporting targets alone is not enough for standard package discovery by external consumers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/time_utils/cpp/CMakeLists.txt` around lines 26 - 29, The CMake packaging only installs the exported targets file (time_utils-targets.cmake) but lacks a package config and version file needed for find_package(time_utils) to work; add generation and installation of time_utilsConfig.cmake and time_utilsConfigVersion.cmake by using configure_package_config_file and write_basic_package_version_file (or include a prewritten time_utilsConfig.cmake that loads the exported targets), ensure the config file references the installed time_utils-targets.cmake (NAMESPACE core::) and install both files alongside time_utils-targets.cmake so external consumers can call find_package(time_utils) successfully.examples/oxr/python/modular_example_with_mcap.py (1)
74-74:⚠️ Potential issue | 🟡 MinorHandle update failure in loop instead of ignoring return value.
Line [74] still discards the boolean result, so the recorder can continue on failed tracker updates.
Suggested fix
- session.update(time.monotonic_ns()) + if not session.update(time.monotonic_ns()): + print("Update failed") + break🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/oxr/python/modular_example_with_mcap.py` at line 74, The call to session.update(time.monotonic_ns()) currently ignores its boolean return; change the recorder loop to check that return value and handle failures (e.g., if not session.update(...): log an error via the recorder/logger, stop the recorder loop or raise an exception to prevent continuing on a failed tracker update). Locate the session.update call in the recorder loop and add an if-check that performs one of the above actions so failed updates are not silently ignored.src/core/teleop_session_manager/python/teleop_session.py (1)
216-231:⚠️ Potential issue | 🟠 MajorClock domain mismatch risk when caller provides a custom context.
When
contextis provided by the caller,step_time_nsis set tocontext.graph_time.real_time_ns(line 224), which may not originate from the system monotonic clock. TheDeviceIOSession.update()expects a system monotonic timestamp because trackers likeLiveHeadTrackerImplandLiveHandTrackerImpluse it to convert toXrTimeviaconvert_monotonic_ns_to_xrtime().Consider always using
time.monotonic_ns()for the DeviceIO update call to ensure clock consistency, while still using the context's timing for pipeline execution.🔧 Proposed fix
# Use provided context or create one from current monotonic time so the # same time is used for the session update and the pipeline. if context is None: now_ns = time.monotonic_ns() context = ComputeContext( graph_time=GraphTime(sim_time_ns=now_ns, real_time_ns=now_ns) ) - - step_time_ns = context.graph_time.real_time_ns + step_time_ns = now_ns + else: + # DeviceIO requires system monotonic ns from the local OS clock. + step_time_ns = time.monotonic_ns() # Check plugin health periodically🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/teleop_session_manager/python/teleop_session.py` around lines 216 - 231, The provided context may use a different clock domain, so avoid passing context.graph_time.real_time_ns to DeviceIO; keep using the caller's ComputeContext/GraphTime for pipeline work (ComputeContext, GraphTime) but compute a separate monotonic timestamp for DeviceIO: call time.monotonic_ns() to produce step_time_ns for deviceio_session.update(...) (so trackers like LiveHeadTrackerImpl/LiveHandTrackerImpl that convert via convert_monotonic_ns_to_xrtime() get a monotonic time), while leaving context and its graph_time untouched.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@examples/oxr/python/test_oak_camera.py`:
- Line 87: The call to session.update(time.monotonic_ns()) currently ignores its
boolean return; change the recording loop to check the return value from
session.update(...) (the session.update call) and on False immediately stop the
loop and surface failure (e.g., break the loop and propagate/raise an error or
return a non-zero status) so the run is not reported as successful when updates
fail. Ensure any cleanup still runs after aborting.
In `@src/core/deviceio_session/python/session_bindings.cpp`:
- Around line 47-50: The lambda in session_bindings.cpp currently uses
system_monotonic_time_ns.value_or(core::os_monotonic_now_ns()), which causes
core::os_monotonic_now_ns() to be evaluated eagerly; change this to an explicit
conditional: check system_monotonic_time_ns.has_value() (or use if
(system_monotonic_time_ns)) and only call core::os_monotonic_now_ns() when the
optional is empty, then pass the chosen int64_t t into self.update(t) so clock
reads occur lazily and only on the fallback path.
In `@src/core/deviceio_trackers/cpp/hand_tracker.cpp`:
- Around line 13-16: Replace the hardcoded constant NUM_HAND_JOINTS = 26 with
the OpenXR-provided constant XR_HAND_JOINT_COUNT_EXT so the code uses the
API-defined value; update the declaration of NUM_HAND_JOINTS and any other
places that currently use the literal 26 (e.g., the occurrences referenced
around the other usage) to reference XR_HAND_JOINT_COUNT_EXT, and ensure the
OpenXR header that defines XR_HAND_JOINT_COUNT_EXT is included so the symbol is
available.
---
Duplicate comments:
In `@examples/oxr/python/modular_example_with_mcap.py`:
- Line 74: The call to session.update(time.monotonic_ns()) currently ignores its
boolean return; change the recorder loop to check that return value and handle
failures (e.g., if not session.update(...): log an error via the
recorder/logger, stop the recorder loop or raise an exception to prevent
continuing on a failed tracker update). Locate the session.update call in the
recorder loop and add an if-check that performs one of the above actions so
failed updates are not silently ignored.
In `@src/core/teleop_session_manager/python/teleop_session.py`:
- Around line 216-231: The provided context may use a different clock domain, so
avoid passing context.graph_time.real_time_ns to DeviceIO; keep using the
caller's ComputeContext/GraphTime for pipeline work (ComputeContext, GraphTime)
but compute a separate monotonic timestamp for DeviceIO: call
time.monotonic_ns() to produce step_time_ns for deviceio_session.update(...) (so
trackers like LiveHeadTrackerImpl/LiveHandTrackerImpl that convert via
convert_monotonic_ns_to_xrtime() get a monotonic time), while leaving context
and its graph_time untouched.
In `@src/core/time_utils/cpp/CMakeLists.txt`:
- Around line 26-29: The CMake packaging only installs the exported targets file
(time_utils-targets.cmake) but lacks a package config and version file needed
for find_package(time_utils) to work; add generation and installation of
time_utilsConfig.cmake and time_utilsConfigVersion.cmake by using
configure_package_config_file and write_basic_package_version_file (or include a
prewritten time_utilsConfig.cmake that loads the exported targets), ensure the
config file references the installed time_utils-targets.cmake (NAMESPACE core::)
and install both files alongside time_utils-targets.cmake so external consumers
can call find_package(time_utils) successfully.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: ac14d69d-4636-4cf1-becf-c85359412a00
📒 Files selected for processing (77)
docs/source/device/add_device.rstexamples/lerobot/record.pyexamples/oxr/cpp/oxr_session_sharing.cppexamples/oxr/cpp/oxr_simple_api_demo.cppexamples/oxr/python/modular_example.pyexamples/oxr/python/modular_example_with_mcap.pyexamples/oxr/python/test_controller_tracker.pyexamples/oxr/python/test_extensions.pyexamples/oxr/python/test_full_body_tracker.pyexamples/oxr/python/test_hand_inactive_on_plugin_stop.pyexamples/oxr/python/test_modular.pyexamples/oxr/python/test_oak_camera.pyexamples/oxr/python/test_session_sharing.pyexamples/oxr/python/test_synthetic_hands.pyexamples/retargeting/python/sources_example.pyexamples/schemaio/frame_metadata_printer.cppexamples/schemaio/pedal_printer.cppsrc/core/CMakeLists.txtsrc/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_session/py_utils/inc/deviceio_py_utils/session.hppsrc/core/deviceio_session/python/CMakeLists.txtsrc/core/deviceio_session/python/session_bindings.cppsrc/core/deviceio_trackers/cpp/CMakeLists.txtsrc/core/deviceio_trackers/cpp/controller_tracker.cppsrc/core/deviceio_trackers/cpp/frame_metadata_tracker_oak.cppsrc/core/deviceio_trackers/cpp/full_body_tracker_pico.cppsrc/core/deviceio_trackers/cpp/generic_3axis_pedal_tracker.cppsrc/core/deviceio_trackers/cpp/hand_tracker.cppsrc/core/deviceio_trackers/cpp/head_tracker.cppsrc/core/deviceio_trackers/cpp/inc/deviceio_trackers/controller_tracker.hppsrc/core/deviceio_trackers/cpp/inc/deviceio_trackers/frame_metadata_tracker_oak.hppsrc/core/deviceio_trackers/cpp/inc/deviceio_trackers/full_body_tracker_pico.hppsrc/core/deviceio_trackers/cpp/inc/deviceio_trackers/generic_3axis_pedal_tracker.hppsrc/core/deviceio_trackers/cpp/inc/deviceio_trackers/hand_tracker.hppsrc/core/deviceio_trackers/cpp/inc/deviceio_trackers/head_tracker.hppsrc/core/deviceio_trackers/python/tracker_bindings.cppsrc/core/live_trackers/cpp/CMakeLists.txtsrc/core/live_trackers/cpp/inc/live_trackers/live_deviceio_factory.hppsrc/core/live_trackers/cpp/live_controller_tracker_impl.cppsrc/core/live_trackers/cpp/live_controller_tracker_impl.hppsrc/core/live_trackers/cpp/live_deviceio_factory.cppsrc/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/live_trackers/cpp/schema_tracker_base.cppsrc/core/mcap/python/mcap_init.pysrc/core/mcap_tests/cpp/test_mcap_tracker_channels.cppsrc/core/oxr/cpp/inc/oxr/oxr_session.hppsrc/core/oxr_utils/cpp/CMakeLists.txtsrc/core/oxr_utils/cpp/inc/oxr_utils/oxr_time.hppsrc/core/schema_tests/python/test_camera.pysrc/core/schema_tests/python/test_controller.pysrc/core/schema_tests/python/test_full_body.pysrc/core/schema_tests/python/test_hand.pysrc/core/schema_tests/python/test_head.pysrc/core/schema_tests/python/test_pedals.pysrc/core/teleop_session_manager/python/teleop_session.pysrc/core/teleop_session_manager_tests/python/test_teleop_session.pysrc/core/time_utils/CMakeLists.txtsrc/core/time_utils/cpp/CMakeLists.txtsrc/core/time_utils/cpp/inc/time_utils/os_time.hppsrc/plugins/controller_synthetic_hands/README.mdsrc/plugins/controller_synthetic_hands/synthetic_hands_plugin.cppsrc/plugins/generic_3axis_pedal/generic_3axis_pedal_plugin.cppsrc/plugins/manus/core/manus_hand_tracking_plugin.cppsrc/plugins/oak/core/frame_sink.cpp
💤 Files with no reviewable changes (11)
- src/core/deviceio_trackers/cpp/CMakeLists.txt
- src/core/deviceio_trackers/cpp/generic_3axis_pedal_tracker.cpp
- src/core/deviceio_trackers/cpp/inc/deviceio_trackers/hand_tracker.hpp
- src/core/deviceio_trackers/cpp/inc/deviceio_trackers/controller_tracker.hpp
- src/core/deviceio_trackers/cpp/inc/deviceio_trackers/head_tracker.hpp
- src/core/deviceio_trackers/cpp/controller_tracker.cpp
- src/core/deviceio_trackers/cpp/head_tracker.cpp
- src/core/deviceio_trackers/cpp/frame_metadata_tracker_oak.cpp
- src/core/deviceio_trackers/cpp/full_body_tracker_pico.cpp
- src/core/deviceio_base/cpp/CMakeLists.txt
- src/core/deviceio_trackers/cpp/inc/deviceio_trackers/full_body_tracker_pico.hpp
Comment on lines
+13
to
+16
| namespace | ||
| { | ||
| constexpr uint32_t NUM_HAND_JOINTS = 26; | ||
| } // namespace |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🌐 Web query:
In the latest OpenXR headers/spec, what is XR_HAND_JOINT_COUNT_EXT, and is it guaranteed to remain fixed across versions?
💡 Result:
XR_HAND_JOINT_COUNT_EXT is a preprocessor macro provided by XR_EXT_hand_tracking that specifies how many joint indices are defined by XrHandJointEXT. In the current OpenXR 1.1.57 headers/spec, it is:
#define XR_HAND_JOINT_COUNT_EXT 26(coveringXrHandJointEXTvalues0..25). [1][2]
Will it remain fixed across versions?
- It is fixed for the
XR_HAND_JOINT_SET_DEFAULT_EXTjoint set: the spec describes thatxrLocateHandJointsEXTreturns an array withcount == XR_HAND_JOINT_COUNT_EXT, indexable byXrHandJointEXT. [3] - OpenXR extension revisions are intended to be backward compatible, and changes that would require adding/removing API (types/commands) should be done via a new extension rather than revising an existing one. That strongly implies Khronos should not “silently” change the joint count for this extension in a way that breaks existing apps sized to
XR_HAND_JOINT_COUNT_EXT. [4] - If additional joints are needed, OpenXR already uses the pattern of defining new joint sets (e.g., a separate joint set for “hand with forearm”) rather than changing the default set’s shape/count. [3]
So: treat XR_HAND_JOINT_COUNT_EXT as stable (26) for XR_EXT_hand_tracking’s default joint set, but also treat it as an API-defined constant you should always read from the headers you compile against, not hardcode.
Sources: [1] XR_HAND_JOINT_COUNT_EXT reference page, [2] XrHandJointEXT reference page, [3] XrHandJointSetEXT reference page, [4] OpenXR “Procedures and Conventions” (extension revision compatibility guidance.
Bind hand-joint count to the OpenXR macro instead of hardcoding.
The code hardcodes NUM_HAND_JOINTS = 26, disconnecting it from XR_HAND_JOINT_COUNT_EXT. While the value is stable in the current OpenXR spec, applications should always depend on the API-defined constant, not the literal value. This ensures the code remains correct if your project updates to a newer OpenXR SDK, and it aligns with OpenXR best practices.
♻️ Proposed fix
namespace
{
-constexpr uint32_t NUM_HAND_JOINTS = 26;
+constexpr uint32_t NUM_HAND_JOINTS = XR_HAND_JOINT_COUNT_EXT;
} // namespace
...
- static_assert(joint_names.size() == NUM_HAND_JOINTS,
- "joint names count must match NUM_HAND_JOINTS (XR_HAND_JOINT_COUNT_EXT)");
+ static_assert(joint_names.size() == XR_HAND_JOINT_COUNT_EXT,
+ "joint names count must match XR_HAND_JOINT_COUNT_EXT");Also applies to: 65-66
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/core/deviceio_trackers/cpp/hand_tracker.cpp` around lines 13 - 16,
Replace the hardcoded constant NUM_HAND_JOINTS = 26 with the OpenXR-provided
constant XR_HAND_JOINT_COUNT_EXT so the code uses the API-defined value; update
the declaration of NUM_HAND_JOINTS and any other places that currently use the
literal 26 (e.g., the occurrences referenced around the other usage) to
reference XR_HAND_JOINT_COUNT_EXT, and ensure the OpenXR header that defines
XR_HAND_JOINT_COUNT_EXT is included so the symbol is available.
95c768c to
2fb7103
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
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/live_controller_tracker_impl.hpp (1)
14-29: 🧹 Nitpick | 🔵 TrivialKeep the NVX1 extension header out of this header surface.
This file only needs
XR_NVX1_action_context.hfor the inline extension-name helper. Declaringrequired_extensions()here and defining it in the.cppkeeps the vendor-specific OpenXR header out of every includer and better matches the “implementation detail” direction of this PR.♻️ Minimal change
-#include <XR_NVX1_action_context.h> ... - static std::vector<std::string> required_extensions() - { - return { XR_NVX1_ACTION_CONTEXT_EXTENSION_NAME }; - } + static std::vector<std::string> required_extensions();std::vector<std::string> LiveControllerTrackerImpl::required_extensions() { return { XR_NVX1_ACTION_CONTEXT_EXTENSION_NAME }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/live_trackers/cpp/live_controller_tracker_impl.hpp` around lines 14 - 29, Remove the XR_NVX1_action_context.h include from the header and change the inline definition of LiveControllerTrackerImpl::required_extensions() in live_controller_tracker_impl.hpp to a declaration only (keep the static std::vector<std::string> required_extensions(); in class LiveControllerTrackerImpl), then add the XR_NVX1_action_context.h include to live_controller_tracker_impl.cpp and provide the out-of-line definition std::vector<std::string> LiveControllerTrackerImpl::required_extensions() that returns { XR_NVX1_ACTION_CONTEXT_EXTENSION_NAME } so the vendor-specific OpenXR header is only pulled into the implementation file.
♻️ Duplicate comments (2)
src/core/teleop_session_manager/python/teleop_session.py (1)
216-225:⚠️ Potential issue | 🟠 MajorUse the local monotonic clock when polling DeviceIO.
Line 224 derives
step_time_nsfromcontext.graph_time.real_time_ns, and Line 231 passes that intoDeviceIOSession.update(). Callers can supplycontextfrom another clock domain, but tracker impls convert this argument as local system-monotonic nanoseconds, so custom contexts can produce incorrect OpenXR times and invalid tracker data.🔧 Proposed fix
- # Use provided context or create one from current monotonic time so the - # same time is used for the session update and the pipeline. + # When no context is provided, reuse one monotonic timestamp for both + # DeviceIO and the pipeline. Otherwise preserve the caller context, but + # still poll DeviceIO from the local monotonic clock. if context is None: - now_ns = time.monotonic_ns() + step_time_ns = time.monotonic_ns() context = ComputeContext( - graph_time=GraphTime(sim_time_ns=now_ns, real_time_ns=now_ns) + graph_time=GraphTime( + sim_time_ns=step_time_ns, + real_time_ns=step_time_ns, + ) ) - - step_time_ns = context.graph_time.real_time_ns + else: + step_time_ns = time.monotonic_ns()Also applies to: 230-231
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/teleop_session_manager/python/teleop_session.py` around lines 216 - 225, The code currently sets step_time_ns from context.graph_time.real_time_ns which may be in a different clock domain; instead compute step_time_ns using the local monotonic clock (time.monotonic_ns()) before calling DeviceIOSession.update(). Leave creation of ComputeContext/GraphTime as-is for missing contexts, but replace the assignment to step_time_ns with time.monotonic_ns() (use the same monotonic value used when creating context when context is None) so DeviceIOSession.update() always receives local system-monotonic nanoseconds.src/core/live_trackers/cpp/live_head_tracker_impl.hpp (1)
24-27: 🧹 Nitpick | 🔵 TrivialConsider returning
XrTimeConverter::get_required_extensions()for consistency.While
LiveDeviceIOFactory::get_required_extensions()unconditionally addsXrTimeConverter::get_required_extensions()for all trackers (seesrc/core/live_trackers/cpp/live_deviceio_factory.cpplines 48-49), returning an empty vector here is inconsistent with schema-based trackers likeLiveGeneric3AxisPedalTrackerImplwhich explicitly include these extensions.For maintainability and self-documenting code, consider aligning with the pattern used elsewhere:
Suggested change
static std::vector<std::string> required_extensions() { - return {}; + return XrTimeConverter::get_required_extensions(); }🤖 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.hpp` around lines 24 - 27, The required_extensions() implementation in LiveHeadTrackerImpl currently returns an empty vector; change it to return XrTimeConverter::get_required_extensions() so it matches the pattern used by LiveGeneric3AxisPedalTrackerImpl and the unconditional addition in LiveDeviceIOFactory::get_required_extensions(); update the function required_extensions() to call and return XrTimeConverter::get_required_extensions() for consistency and maintainability.
🤖 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/mcap_tests/cpp/test_mcap_tracker_channels.cpp`:
- Around line 88-92: The test currently writes identical values for
available_time_local_common_clock and sample_time_local_common_clock (via the
DeviceDataTimestamp passed to ch.write), which won't detect swapped-field
regressions; modify one readback case so DeviceDataTimestamp uses distinct
values (e.g., available_time_local_common_clock !=
sample_time_local_common_clock) when calling ch.write and then update the
assertions in the corresponding deserialization/readback check to assert both
timestamp fields separately (verify both available_time_local_common_clock and
sample_time_local_common_clock match the distinct values); look for usages of
ch.write, DeviceDataTimestamp, and the head_data/readback assertions in the
test_mcap_tracker_channels test and adjust that case (also apply the same change
to the other similar case around lines 104-109).
In `@src/core/time_utils/cpp/CMakeLists.txt`:
- Around line 17-29: The install rules in CMakeLists.txt hard-code destinations;
add include(GNUInstallDirs) near the top and update the three install
commands—install(TARGETS time_utils ...), install(DIRECTORY inc/time_utils ...),
and install(EXPORT time_utils-targets ...)—to use ${CMAKE_INSTALL_INCLUDEDIR}
for header installs and ${CMAKE_INSTALL_LIBDIR}/cmake/time_utils (or
${CMAKE_INSTALL_LIBDIR} with a cmake/time_utils suffix) for the export file
destination instead of literal "include" and "lib/cmake/time_utils".
---
Outside diff comments:
In `@src/core/live_trackers/cpp/live_controller_tracker_impl.hpp`:
- Around line 14-29: Remove the XR_NVX1_action_context.h include from the header
and change the inline definition of
LiveControllerTrackerImpl::required_extensions() in
live_controller_tracker_impl.hpp to a declaration only (keep the static
std::vector<std::string> required_extensions(); in class
LiveControllerTrackerImpl), then add the XR_NVX1_action_context.h include to
live_controller_tracker_impl.cpp and provide the out-of-line definition
std::vector<std::string> LiveControllerTrackerImpl::required_extensions() that
returns { XR_NVX1_ACTION_CONTEXT_EXTENSION_NAME } so the vendor-specific OpenXR
header is only pulled into the implementation file.
---
Duplicate comments:
In `@src/core/live_trackers/cpp/live_head_tracker_impl.hpp`:
- Around line 24-27: The required_extensions() implementation in
LiveHeadTrackerImpl currently returns an empty vector; change it to return
XrTimeConverter::get_required_extensions() so it matches the pattern used by
LiveGeneric3AxisPedalTrackerImpl and the unconditional addition in
LiveDeviceIOFactory::get_required_extensions(); update the function
required_extensions() to call and return
XrTimeConverter::get_required_extensions() for consistency and maintainability.
In `@src/core/teleop_session_manager/python/teleop_session.py`:
- Around line 216-225: The code currently sets step_time_ns from
context.graph_time.real_time_ns which may be in a different clock domain;
instead compute step_time_ns using the local monotonic clock
(time.monotonic_ns()) before calling DeviceIOSession.update(). Leave creation of
ComputeContext/GraphTime as-is for missing contexts, but replace the assignment
to step_time_ns with time.monotonic_ns() (use the same monotonic value used when
creating context when context is None) so DeviceIOSession.update() always
receives local system-monotonic nanoseconds.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 946c1630-90ac-4654-8546-471eef0d10f3
📒 Files selected for processing (82)
docs/source/device/add_device.rstexamples/lerobot/record.pyexamples/oxr/cpp/oxr_session_sharing.cppexamples/oxr/cpp/oxr_simple_api_demo.cppexamples/oxr/python/modular_example.pyexamples/oxr/python/modular_example_with_mcap.pyexamples/oxr/python/test_controller_tracker.pyexamples/oxr/python/test_extensions.pyexamples/oxr/python/test_full_body_tracker.pyexamples/oxr/python/test_hand_inactive_on_plugin_stop.pyexamples/oxr/python/test_modular.pyexamples/oxr/python/test_oak_camera.pyexamples/oxr/python/test_session_sharing.pyexamples/oxr/python/test_synthetic_hands.pyexamples/retargeting/python/sources_example.pyexamples/schemaio/frame_metadata_printer.cppexamples/schemaio/pedal_printer.cppsrc/core/CMakeLists.txtsrc/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_session/py_utils/inc/deviceio_py_utils/session.hppsrc/core/deviceio_session/python/CMakeLists.txtsrc/core/deviceio_session/python/session_bindings.cppsrc/core/deviceio_trackers/cpp/CMakeLists.txtsrc/core/deviceio_trackers/cpp/controller_tracker.cppsrc/core/deviceio_trackers/cpp/frame_metadata_tracker_oak.cppsrc/core/deviceio_trackers/cpp/full_body_tracker_pico.cppsrc/core/deviceio_trackers/cpp/generic_3axis_pedal_tracker.cppsrc/core/deviceio_trackers/cpp/hand_tracker.cppsrc/core/deviceio_trackers/cpp/head_tracker.cppsrc/core/deviceio_trackers/cpp/inc/deviceio_trackers/controller_tracker.hppsrc/core/deviceio_trackers/cpp/inc/deviceio_trackers/frame_metadata_tracker_oak.hppsrc/core/deviceio_trackers/cpp/inc/deviceio_trackers/full_body_tracker_pico.hppsrc/core/deviceio_trackers/cpp/inc/deviceio_trackers/generic_3axis_pedal_tracker.hppsrc/core/deviceio_trackers/cpp/inc/deviceio_trackers/hand_tracker.hppsrc/core/deviceio_trackers/cpp/inc/deviceio_trackers/head_tracker.hppsrc/core/deviceio_trackers/python/deviceio_trackers_init.pysrc/core/deviceio_trackers/python/tracker_bindings.cppsrc/core/live_trackers/cpp/CMakeLists.txtsrc/core/live_trackers/cpp/inc/live_trackers/live_deviceio_factory.hppsrc/core/live_trackers/cpp/live_controller_tracker_impl.cppsrc/core/live_trackers/cpp/live_controller_tracker_impl.hppsrc/core/live_trackers/cpp/live_deviceio_factory.cppsrc/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/live_trackers/cpp/schema_tracker_base.cppsrc/core/mcap/python/mcap_init.pysrc/core/mcap_tests/cpp/test_mcap_tracker_channels.cppsrc/core/oxr/cpp/inc/oxr/oxr_session.hppsrc/core/oxr_utils/cpp/CMakeLists.txtsrc/core/oxr_utils/cpp/inc/oxr_utils/oxr_time.hppsrc/core/python/deviceio_init.pysrc/core/schema/fbs/hand.fbssrc/core/schema/python/hand_bindings.hsrc/core/schema/python/schema_init.pysrc/core/schema_tests/python/test_camera.pysrc/core/schema_tests/python/test_controller.pysrc/core/schema_tests/python/test_full_body.pysrc/core/schema_tests/python/test_hand.pysrc/core/schema_tests/python/test_head.pysrc/core/schema_tests/python/test_pedals.pysrc/core/teleop_session_manager/python/teleop_session.pysrc/core/teleop_session_manager_tests/python/test_teleop_session.pysrc/core/time_utils/CMakeLists.txtsrc/core/time_utils/cpp/CMakeLists.txtsrc/core/time_utils/cpp/inc/time_utils/os_time.hppsrc/plugins/controller_synthetic_hands/README.mdsrc/plugins/controller_synthetic_hands/synthetic_hands_plugin.cppsrc/plugins/generic_3axis_pedal/generic_3axis_pedal_plugin.cppsrc/plugins/manus/core/manus_hand_tracking_plugin.cppsrc/plugins/oak/core/frame_sink.cpp
💤 Files with no reviewable changes (13)
- src/core/deviceio_trackers/cpp/CMakeLists.txt
- src/core/deviceio_trackers/cpp/generic_3axis_pedal_tracker.cpp
- src/core/deviceio_trackers/cpp/inc/deviceio_trackers/full_body_tracker_pico.hpp
- src/core/deviceio_trackers/cpp/full_body_tracker_pico.cpp
- src/core/deviceio_trackers/python/tracker_bindings.cpp
- src/core/deviceio_trackers/cpp/controller_tracker.cpp
- src/core/deviceio_trackers/cpp/inc/deviceio_trackers/controller_tracker.hpp
- src/core/deviceio_trackers/cpp/inc/deviceio_trackers/head_tracker.hpp
- src/core/deviceio_trackers/cpp/inc/deviceio_trackers/hand_tracker.hpp
- src/core/deviceio_trackers/cpp/frame_metadata_tracker_oak.cpp
- src/core/deviceio_base/cpp/CMakeLists.txt
- src/core/deviceio_trackers/python/deviceio_trackers_init.py
- src/core/deviceio_trackers/cpp/head_tracker.cpp
Comment on lines
+88
to
+92
| ch.write(0, | ||
| core::DeviceDataTimestamp(/*available_time_local_common_clock=*/1000000, | ||
| /*sample_time_local_common_clock=*/1000000, | ||
| /*sample_time_raw_device_clock=*/42), | ||
| head_data); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Use distinct local-common-clock values in one readback case.
All of the updated cases pass the same value for available_time_local_common_clock and sample_time_local_common_clock, so this suite still will not catch swapped-field wiring after the timestamp refactor. Make one case use different values and assert both fields after deserialization.
♻️ Minimal test hardening
ch.write(0,
core::DeviceDataTimestamp(/*available_time_local_common_clock=*/1000000,
- /*sample_time_local_common_clock=*/1000000,
+ /*sample_time_local_common_clock=*/900000,
/*sample_time_raw_device_clock=*/42),
head_data);
...
REQUIRE(record != nullptr);
REQUIRE(record->timestamp() != nullptr);
+ CHECK(record->timestamp()->available_time_local_common_clock() == 1000000);
+ CHECK(record->timestamp()->sample_time_local_common_clock() == 900000);
CHECK(record->timestamp()->sample_time_raw_device_clock() == 42);Also applies to: 104-109
🤖 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` around lines 88 - 92,
The test currently writes identical values for available_time_local_common_clock
and sample_time_local_common_clock (via the DeviceDataTimestamp passed to
ch.write), which won't detect swapped-field regressions; modify one readback
case so DeviceDataTimestamp uses distinct values (e.g.,
available_time_local_common_clock != sample_time_local_common_clock) when
calling ch.write and then update the assertions in the corresponding
deserialization/readback check to assert both timestamp fields separately
(verify both available_time_local_common_clock and
sample_time_local_common_clock match the distinct values); look for usages of
ch.write, DeviceDataTimestamp, and the head_data/readback assertions in the
test_mcap_tracker_channels test and adjust that case (also apply the same change
to the other similar case around lines 104-109).
Comment on lines
+17
to
+29
| install(TARGETS time_utils | ||
| EXPORT time_utils-targets | ||
| INCLUDES DESTINATION include | ||
| ) | ||
|
|
||
| install(DIRECTORY inc/time_utils | ||
| DESTINATION include | ||
| ) | ||
|
|
||
| install(EXPORT time_utils-targets | ||
| FILE time_utils-targets.cmake | ||
| NAMESPACE core:: | ||
| DESTINATION lib/cmake/time_utils |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Use GNUInstallDirs for the install/export paths.
Hard-coding include and lib/cmake/time_utils makes the install tree less portable on layouts that use a different libdir. Switching these destinations to ${CMAKE_INSTALL_INCLUDEDIR} and ${CMAKE_INSTALL_LIBDIR} avoids that packaging drift.
♻️ Minimal change
cmake_minimum_required(VERSION 3.20)
+include(GNUInstallDirs)
...
install(TARGETS time_utils
EXPORT time_utils-targets
- INCLUDES DESTINATION include
+ INCLUDES DESTINATION ${CMAKE_INSTALL_INCLUDEDIR}
)
...
install(DIRECTORY inc/time_utils
- DESTINATION include
+ DESTINATION ${CMAKE_INSTALL_INCLUDEDIR}
)
...
install(EXPORT time_utils-targets
FILE time_utils-targets.cmake
NAMESPACE core::
- DESTINATION lib/cmake/time_utils
+ DESTINATION ${CMAKE_INSTALL_LIBDIR}/cmake/time_utils
)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/core/time_utils/cpp/CMakeLists.txt` around lines 17 - 29, The install
rules in CMakeLists.txt hard-code destinations; add include(GNUInstallDirs) near
the top and update the three install commands—install(TARGETS time_utils ...),
install(DIRECTORY inc/time_utils ...), and install(EXPORT time_utils-targets
...)—to use ${CMAKE_INSTALL_INCLUDEDIR} for header installs and
${CMAKE_INSTALL_LIBDIR}/cmake/time_utils (or ${CMAKE_INSTALL_LIBDIR} with a
cmake/time_utils suffix) for the export file destination instead of literal
"include" and "lib/cmake/time_utils".
Move extension requirements from ITracker to live impls Extension knowledge is an OpenXR live-session concern, not a tracker concern. Remove get_required_extensions() from ITracker and deviceio_trackers. Each LiveXxxTrackerImpl now declares its own static required_extensions(), and LiveDeviceIOFactory aggregates them via dynamic_cast dispatch. DeviceIOSession::get_required_extensions() delegates to the factory.
nvddr
force-pushed
the
devdeepr/addTimeToUpdate
branch
from
2fb7103 to
0255673
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/core/schema/python/hand_bindings.h (1)
84-91:⚠️ Potential issue | 🟡 MinorKeep the Python-facing bounds text symbolic too.
Line 84 made the guard depend on
HandJoint_NUM_JOINTS, but the exception text and docstring still hard-code0-25. If the schema count changes again, the check stays correct while the diagnostics go stale.🛠️ Proposed fix
[](const HandJoints& self, size_t index) -> const HandJointPose* { - if (index >= static_cast<size_t>(HandJoint_NUM_JOINTS)) + constexpr size_t kHandJointCount = static_cast<size_t>(HandJoint_NUM_JOINTS); + if (index >= kHandJointCount) { - throw py::index_error("HandJoints index out of range (must be 0-25)"); + throw py::index_error( + std::string("HandJoints index out of range (must be 0-") + + std::to_string(kHandJointCount - 1) + ")"); } return (*self.poses())[index]; }, py::arg("index"), py::return_value_policy::reference_internal, - "Get the HandJointPose at the specified index (0-25).") + "Get the HandJointPose at the specified index (0 <= index < HandJoint.NUM_JOINTS).")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/schema/python/hand_bindings.h` around lines 84 - 91, The bounds-check throws and documents a hard-coded range "0-25" while the guard uses HandJoint_NUM_JOINTS; update the exception message and the docstring in the index accessor to reference the symbolic bound instead of the literal (e.g., use HandJoint_NUM_JOINTS or compute HandJoint_NUM_JOINTS - 1 in the message) so diagnostics stay correct when HandJoint_NUM_JOINTS changes; adjust the string passed to py::index_error and the trailing docstring argument for the accessor that returns (*self.poses())[index] to include the symbolic name/value rather than "0-25".
♻️ Duplicate comments (4)
docs/source/device/add_device.rst (1)
120-125:⚠️ Potential issue | 🟡 MinorFinish signature migration in the remaining prose blocks.
This page still references
update()without parameters in other spots (Line 143 and Line 210), which conflicts with the updatedupdate(int64_t target_monotonic_time_ns)guidance here.Also applies to: 216-217
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/source/device/add_device.rst` around lines 120 - 125, Update the remaining prose references that mention update() without parameters to the new signature update(int64_t target_monotonic_time_ns); specifically change any inline mentions (e.g. the narrative lines referencing update() at the other spots and the duplicated spots around lines noted) so they match the guidance already using update(int64_t target_monotonic_time_ns), and adjust any surrounding examples or descriptions that refer to calling update() so they show the required target_monotonic_time_ns parameter; keep other symbols like serialize_all(channel_index, callback) and m_schema_reader.read_all_samples(pending_records) unchanged.examples/oxr/python/test_oak_camera.py (1)
87-89:⚠️ Potential issue | 🟠 MajorPropagate update failure instead of only breaking the loop.
Line 87 stops the loop, but the function continues to success prints and
run_test()still returns success. This can mask real runtime failures.💡 Proposed fix
-def _run_schema_pusher( +def _run_schema_pusher( plugin, duration: float, tracker, stream_names: list[str], required_extensions: list[str], mcap_filename: str, -): +)-> bool: @@ - while time.time() - start_time < duration: + update_failed = False + while time.time() - start_time < duration: plugin.check_health() if not session.update(time.monotonic_ns()): print("Session update failed, stopping") + update_failed = True break @@ - print("-" * 80) + print("-" * 80) print() - print(f" ✓ Recording completed ({duration:.1f} seconds)") + if update_failed: + print(" ✗ Recording stopped due to DeviceIO update failure") + return False + print(f" ✓ Recording completed ({duration:.1f} seconds)") print(f" ✓ Processed {frame_count} update cycles") for name in stream_names: print(f" ✓ {name}: {metadata_samples.get(name, 0)} metadata samples") + return True @@ if mode == MODE_SCHEMA_PUSHER: - _run_schema_pusher( + ok = _run_schema_pusher( plugin, duration, tracker, stream_names, required_extensions, mcap_filename, ) + if not ok: + return False else: _run_recording_loop(plugin, duration)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/oxr/python/test_oak_camera.py` around lines 87 - 89, The session.update failure should be propagated instead of silently breaking: inside the loop where session.update(time.monotonic_ns()) is called, replace the break with an explicit error propagation (e.g., print an error and return False from the surrounding test function or raise a RuntimeError) so the caller (run_test()) receives a non-success result; update the signature/return handling of run_test() and its caller to treat the returned False/exception as test failure so the process exits non-zero or reports failure accordingly.src/core/teleop_session_manager/python/teleop_session.py (1)
224-231:⚠️ Potential issue | 🟠 MajorUse OS monotonic time for DeviceIO updates, not
context.graph_time.real_time_ns.
DeviceIOSession.update(...)should be driven from the system monotonic clock domain. Using caller-providedcontext.graph_time.real_time_nscan reintroduce cross-domain timestamp bugs when custom contexts are passed.🔧 Proposed fix
- step_time_ns = context.graph_time.real_time_ns + if context is None: + now_ns = time.monotonic_ns() + context = ComputeContext( + graph_time=GraphTime(sim_time_ns=now_ns, real_time_ns=now_ns) + ) + step_time_ns = now_ns + else: + # DeviceIO requires local OS monotonic ns even when caller provides context. + step_time_ns = time.monotonic_ns() # Check plugin health periodically if self.frame_count % 60 == 0: self._check_plugin_health() # Update DeviceIO session (polls trackers) with the same time as the context self.deviceio_session.update(step_time_ns)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/teleop_session_manager/python/teleop_session.py` around lines 224 - 231, Replace use of the caller-provided graph time when driving DeviceIO with the OS monotonic clock: instead of using context.graph_time.real_time_ns for DeviceIOSession updates in teleop_session.py, obtain a monotonic timestamp (e.g. time.monotonic_ns()) and pass that to self.deviceio_session.update(step_time_ns); keep the existing plugin health check and any other uses of context.graph_time unchanged and only switch the timestamp used by DeviceIOSession.update to the monotonic domain.src/core/live_trackers/cpp/live_head_tracker_impl.hpp (1)
24-27:⚠️ Potential issue | 🔴 Critical
required_extensions()returns empty buttime_converter_requires platform-specific extensions.This issue has already been flagged in a previous review. The
XrTimeConvertermember requires OpenXR time conversion extensions that are not being returned.🤖 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.hpp` around lines 24 - 27, The required_extensions() function currently returns an empty list but must advertise the OpenXR extensions needed by XrTimeConverter (time_converter_) so the runtime enables time conversion support; update required_extensions() to return the platform-specific extension names used by XrTimeConverter (e.g., the OpenXR time conversion extension(s) your XrTimeConverter implementation depends on), conditionally include platform-specific variants if needed, and ensure the list matches the capabilities checked/used by the XrTimeConverter class so time_converter_ initialization succeeds.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@examples/lerobot/record.py`:
- Line 133: The call to retrieve the wrist pose (wrist =
left_tracked.data.joints.poses(deviceio.HandJoint.WRIST)) needs to be formatted
with ruff to satisfy CI; run the ruff formatter (ruff format) on
examples/lerobot/record.py (and the other occurrence around the call at lines
near the second poses(...) call) and commit the resulting changes so the wrist
pose calls and surrounding code match the project's formatting rules.
In `@examples/oxr/python/test_extensions.py`:
- Around line 92-98: The CI formatting failure is due to a long chained call on
left_tracked.data.joints.poses(...).pose.position; reformat that expression
across multiple lines to satisfy ruff/black style: break the call arguments and
subsequent attribute accesses into separate indented lines so the call to
joints.poses(deviceio.HandJoint.WRIST) and the subsequent .pose.position access
are on their own lines; update the block around session.update,
hand.get_left_hand, and the retrieval expression
(left_tracked.data.joints.poses(deviceio.HandJoint.WRIST).pose.position) to use
the multi-line style.
In `@examples/oxr/python/test_session_sharing.py`:
- Around line 108-109: CI flagged a ruff formatting issue: break the long call
to left_tracked.data.joints.poses(deviceio.HandJoint.WRIST) into a multi-line
expression so it conforms to the formatter; update the assignment to span
multiple lines (e.g., call poses( with the enum on its own line and closing
paren on its own line) and keep subsequent .pose.position access on the
following line, referencing left_tracked.data.joints.poses and
deviceio.HandJoint.WRIST to locate the code to change.
---
Outside diff comments:
In `@src/core/schema/python/hand_bindings.h`:
- Around line 84-91: The bounds-check throws and documents a hard-coded range
"0-25" while the guard uses HandJoint_NUM_JOINTS; update the exception message
and the docstring in the index accessor to reference the symbolic bound instead
of the literal (e.g., use HandJoint_NUM_JOINTS or compute HandJoint_NUM_JOINTS -
1 in the message) so diagnostics stay correct when HandJoint_NUM_JOINTS changes;
adjust the string passed to py::index_error and the trailing docstring argument
for the accessor that returns (*self.poses())[index] to include the symbolic
name/value rather than "0-25".
---
Duplicate comments:
In `@docs/source/device/add_device.rst`:
- Around line 120-125: Update the remaining prose references that mention
update() without parameters to the new signature update(int64_t
target_monotonic_time_ns); specifically change any inline mentions (e.g. the
narrative lines referencing update() at the other spots and the duplicated spots
around lines noted) so they match the guidance already using update(int64_t
target_monotonic_time_ns), and adjust any surrounding examples or descriptions
that refer to calling update() so they show the required
target_monotonic_time_ns parameter; keep other symbols like
serialize_all(channel_index, callback) and
m_schema_reader.read_all_samples(pending_records) unchanged.
In `@examples/oxr/python/test_oak_camera.py`:
- Around line 87-89: The session.update failure should be propagated instead of
silently breaking: inside the loop where session.update(time.monotonic_ns()) is
called, replace the break with an explicit error propagation (e.g., print an
error and return False from the surrounding test function or raise a
RuntimeError) so the caller (run_test()) receives a non-success result; update
the signature/return handling of run_test() and its caller to treat the returned
False/exception as test failure so the process exits non-zero or reports failure
accordingly.
In `@src/core/live_trackers/cpp/live_head_tracker_impl.hpp`:
- Around line 24-27: The required_extensions() function currently returns an
empty list but must advertise the OpenXR extensions needed by XrTimeConverter
(time_converter_) so the runtime enables time conversion support; update
required_extensions() to return the platform-specific extension names used by
XrTimeConverter (e.g., the OpenXR time conversion extension(s) your
XrTimeConverter implementation depends on), conditionally include
platform-specific variants if needed, and ensure the list matches the
capabilities checked/used by the XrTimeConverter class so time_converter_
initialization succeeds.
In `@src/core/teleop_session_manager/python/teleop_session.py`:
- Around line 224-231: Replace use of the caller-provided graph time when
driving DeviceIO with the OS monotonic clock: instead of using
context.graph_time.real_time_ns for DeviceIOSession updates in
teleop_session.py, obtain a monotonic timestamp (e.g. time.monotonic_ns()) and
pass that to self.deviceio_session.update(step_time_ns); keep the existing
plugin health check and any other uses of context.graph_time unchanged and only
switch the timestamp used by DeviceIOSession.update to the monotonic domain.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c7899b31-d36d-45c2-978d-853f2198bcfb
📒 Files selected for processing (86)
docs/source/device/add_device.rstexamples/lerobot/record.pyexamples/oxr/cpp/oxr_session_sharing.cppexamples/oxr/cpp/oxr_simple_api_demo.cppexamples/oxr/python/modular_example.pyexamples/oxr/python/modular_example_with_mcap.pyexamples/oxr/python/test_controller_tracker.pyexamples/oxr/python/test_extensions.pyexamples/oxr/python/test_full_body_tracker.pyexamples/oxr/python/test_hand_inactive_on_plugin_stop.pyexamples/oxr/python/test_modular.pyexamples/oxr/python/test_oak_camera.pyexamples/oxr/python/test_session_sharing.pyexamples/oxr/python/test_synthetic_hands.pyexamples/retargeting/python/sources_example.pyexamples/schemaio/frame_metadata_printer.cppexamples/schemaio/pedal_printer.cppsrc/core/CMakeLists.txtsrc/core/cloudxr/python/CMakeLists.txtsrc/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_session/py_utils/inc/deviceio_py_utils/session.hppsrc/core/deviceio_session/python/CMakeLists.txtsrc/core/deviceio_session/python/session_bindings.cppsrc/core/deviceio_trackers/cpp/CMakeLists.txtsrc/core/deviceio_trackers/cpp/controller_tracker.cppsrc/core/deviceio_trackers/cpp/frame_metadata_tracker_oak.cppsrc/core/deviceio_trackers/cpp/full_body_tracker_pico.cppsrc/core/deviceio_trackers/cpp/generic_3axis_pedal_tracker.cppsrc/core/deviceio_trackers/cpp/hand_tracker.cppsrc/core/deviceio_trackers/cpp/head_tracker.cppsrc/core/deviceio_trackers/cpp/inc/deviceio_trackers/controller_tracker.hppsrc/core/deviceio_trackers/cpp/inc/deviceio_trackers/frame_metadata_tracker_oak.hppsrc/core/deviceio_trackers/cpp/inc/deviceio_trackers/full_body_tracker_pico.hppsrc/core/deviceio_trackers/cpp/inc/deviceio_trackers/generic_3axis_pedal_tracker.hppsrc/core/deviceio_trackers/cpp/inc/deviceio_trackers/hand_tracker.hppsrc/core/deviceio_trackers/cpp/inc/deviceio_trackers/head_tracker.hppsrc/core/deviceio_trackers/python/deviceio_trackers_init.pysrc/core/deviceio_trackers/python/tracker_bindings.cppsrc/core/live_trackers/cpp/CMakeLists.txtsrc/core/live_trackers/cpp/inc/live_trackers/live_deviceio_factory.hppsrc/core/live_trackers/cpp/live_controller_tracker_impl.cppsrc/core/live_trackers/cpp/live_controller_tracker_impl.hppsrc/core/live_trackers/cpp/live_deviceio_factory.cppsrc/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/live_trackers/cpp/schema_tracker_base.cppsrc/core/mcap/python/mcap_init.pysrc/core/mcap_tests/cpp/test_mcap_tracker_channels.cppsrc/core/oxr/cpp/inc/oxr/oxr_session.hppsrc/core/oxr_utils/cpp/CMakeLists.txtsrc/core/oxr_utils/cpp/inc/oxr_utils/oxr_time.hppsrc/core/python/deviceio_init.pysrc/core/retargeting_engine/python/CMakeLists.txtsrc/core/retargeting_engine_ui/python/CMakeLists.txtsrc/core/schema/fbs/hand.fbssrc/core/schema/python/hand_bindings.hsrc/core/schema/python/schema_init.pysrc/core/schema_tests/python/test_camera.pysrc/core/schema_tests/python/test_controller.pysrc/core/schema_tests/python/test_full_body.pysrc/core/schema_tests/python/test_hand.pysrc/core/schema_tests/python/test_head.pysrc/core/schema_tests/python/test_pedals.pysrc/core/teleop_session_manager/CMakeLists.txtsrc/core/teleop_session_manager/python/teleop_session.pysrc/core/teleop_session_manager_tests/python/test_teleop_session.pysrc/core/time_utils/CMakeLists.txtsrc/core/time_utils/cpp/CMakeLists.txtsrc/core/time_utils/cpp/inc/time_utils/os_time.hppsrc/plugins/controller_synthetic_hands/README.mdsrc/plugins/controller_synthetic_hands/synthetic_hands_plugin.cppsrc/plugins/generic_3axis_pedal/generic_3axis_pedal_plugin.cppsrc/plugins/manus/core/manus_hand_tracking_plugin.cppsrc/plugins/oak/core/frame_sink.cpp
💤 Files with no reviewable changes (17)
- src/core/deviceio_trackers/cpp/CMakeLists.txt
- src/core/teleop_session_manager/CMakeLists.txt
- src/core/retargeting_engine/python/CMakeLists.txt
- src/core/deviceio_trackers/cpp/generic_3axis_pedal_tracker.cpp
- src/core/deviceio_trackers/cpp/inc/deviceio_trackers/head_tracker.hpp
- src/core/deviceio_trackers/python/tracker_bindings.cpp
- src/core/deviceio_base/cpp/CMakeLists.txt
- src/core/deviceio_trackers/cpp/head_tracker.cpp
- src/core/deviceio_trackers/cpp/inc/deviceio_trackers/full_body_tracker_pico.hpp
- src/core/deviceio_trackers/cpp/full_body_tracker_pico.cpp
- src/core/deviceio_trackers/cpp/inc/deviceio_trackers/hand_tracker.hpp
- src/core/deviceio_trackers/cpp/inc/deviceio_trackers/controller_tracker.hpp
- src/core/cloudxr/python/CMakeLists.txt
- src/core/deviceio_trackers/cpp/controller_tracker.cpp
- src/core/deviceio_trackers/cpp/frame_metadata_tracker_oak.cpp
- src/core/retargeting_engine_ui/python/CMakeLists.txt
- src/core/deviceio_trackers/python/deviceio_trackers_init.py
|
|
||
| if left_tracked.data is not None and left_tracked.data.joints: | ||
| wrist = left_tracked.data.joints.poses(deviceio.JOINT_WRIST) | ||
| wrist = left_tracked.data.joints.poses(deviceio.HandJoint.WRIST) |
There was a problem hiding this comment.
Apply ruff formatting for wrist pose calls to unblock CI.
Pre-commit reports this file is reformatted by ruff format; please commit the formatter output so lint checks pass.
🧹 Formatter-aligned shape
- wrist = left_tracked.data.joints.poses(deviceio.HandJoint.WRIST)
+ wrist = left_tracked.data.joints.poses(
+ deviceio.HandJoint.WRIST
+ )
- wrist = right_tracked.data.joints.poses(deviceio.HandJoint.WRIST)
+ wrist = right_tracked.data.joints.poses(
+ deviceio.HandJoint.WRIST
+ )Also applies to: 139-139
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@examples/lerobot/record.py` at line 133, The call to retrieve the wrist pose
(wrist = left_tracked.data.joints.poses(deviceio.HandJoint.WRIST)) needs to be
formatted with ruff to satisfy CI; run the ruff formatter (ruff format) on
examples/lerobot/record.py (and the other occurrence around the call at lines
near the second poses(...) call) and commit the resulting changes so the wrist
pose calls and surrounding code match the project's formatting rules.
Comment on lines
+92
to
98
| if session.update(time.monotonic_ns()): | ||
| left_tracked = hand.get_left_hand(session) | ||
| head_tracked = head.get_head(session) | ||
| print(" ✅ Update successful") | ||
| if left_tracked.data is not None: | ||
| pos = left_tracked.data.joints.poses(deviceio.JOINT_WRIST).pose.position | ||
| pos = left_tracked.data.joints.poses(deviceio.HandJoint.WRIST).pose.position | ||
| print(f" Left wrist: [{pos.x:.3f}, {pos.y:.3f}, {pos.z:.3f}]") |
There was a problem hiding this comment.
Formatting issue flagged by CI pipeline.
The changes correctly pass the monotonic timestamp to session.update() and use the new HandJoint.WRIST enum. However, CI indicates that ruff format expects multi-line formatting for the joints.poses(...) call on lines 97-98.
🔧 Suggested fix for formatting
if left_tracked.data is not None:
- pos = left_tracked.data.joints.poses(deviceio.HandJoint.WRIST).pose.position
+ pos = left_tracked.data.joints.poses(
+ deviceio.HandJoint.WRIST
+ ).pose.position
print(f" Left wrist: [{pos.x:.3f}, {pos.y:.3f}, {pos.z:.3f}]")📝 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
| if session.update(time.monotonic_ns()): | |
| left_tracked = hand.get_left_hand(session) | |
| head_tracked = head.get_head(session) | |
| print(" ✅ Update successful") | |
| if left_tracked.data is not None: | |
| pos = left_tracked.data.joints.poses(deviceio.JOINT_WRIST).pose.position | |
| pos = left_tracked.data.joints.poses(deviceio.HandJoint.WRIST).pose.position | |
| print(f" Left wrist: [{pos.x:.3f}, {pos.y:.3f}, {pos.z:.3f}]") | |
| if session.update(time.monotonic_ns()): | |
| left_tracked = hand.get_left_hand(session) | |
| head_tracked = head.get_head(session) | |
| print(" ✅ Update successful") | |
| if left_tracked.data is not None: | |
| pos = left_tracked.data.joints.poses( | |
| deviceio.HandJoint.WRIST | |
| ).pose.position | |
| print(f" Left wrist: [{pos.x:.3f}, {pos.y:.3f}, {pos.z:.3f}]") |
🧰 Tools
🪛 GitHub Actions: Run linters using pre-commit
[error] 94-96: ruff format modified formatting in this file (ruff-format reformatted 3 files). Formatter changed joints.poses(...) call to a multi-line form.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@examples/oxr/python/test_extensions.py` around lines 92 - 98, The CI
formatting failure is due to a long chained call on
left_tracked.data.joints.poses(...).pose.position; reformat that expression
across multiple lines to satisfy ruff/black style: break the call arguments and
subsequent attribute accesses into separate indented lines so the call to
joints.poses(deviceio.HandJoint.WRIST) and the subsequent .pose.position access
are on their own lines; update the block around session.update,
hand.get_left_hand, and the retrieval expression
(left_tracked.data.joints.poses(deviceio.HandJoint.WRIST).pose.position) to use
the multi-line style.
Comment on lines
+108
to
109
| pos = left_tracked.data.joints.poses(deviceio.HandJoint.WRIST).pose.position | ||
| print(f" Left wrist: [{pos.x:6.3f}, {pos.y:6.3f}, {pos.z:6.3f}]") |
There was a problem hiding this comment.
Formatting issue flagged by CI pipeline.
The logic correctly uses deviceio.HandJoint.WRIST, but CI indicates ruff format expects multi-line formatting for this call.
🔧 Suggested fix for formatting
if left_tracked.data is not None:
- pos = left_tracked.data.joints.poses(deviceio.HandJoint.WRIST).pose.position
+ pos = left_tracked.data.joints.poses(
+ deviceio.HandJoint.WRIST
+ ).pose.position
print(f" Left wrist: [{pos.x:6.3f}, {pos.y:6.3f}, {pos.z:6.3f}]")📝 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
| pos = left_tracked.data.joints.poses(deviceio.HandJoint.WRIST).pose.position | |
| print(f" Left wrist: [{pos.x:6.3f}, {pos.y:6.3f}, {pos.z:6.3f}]") | |
| pos = left_tracked.data.joints.poses( | |
| deviceio.HandJoint.WRIST | |
| ).pose.position | |
| print(f" Left wrist: [{pos.x:6.3f}, {pos.y:6.3f}, {pos.z:6.3f}]") |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@examples/oxr/python/test_session_sharing.py` around lines 108 - 109, CI
flagged a ruff formatting issue: break the long call to
left_tracked.data.joints.poses(deviceio.HandJoint.WRIST) into a multi-line
expression so it conforms to the formatter; update the assignment to span
multiple lines (e.g., call poses( with the enum on its own line and closing
paren on its own line) and keep subsequent .pose.position access on the
following line, referencing left_tracked.data.joints.poses and
deviceio.HandJoint.WRIST to locate the code to change.
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
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.
Removes oxr dependencies from public APIs making it an implementation detail.
Summary by CodeRabbit
Breaking Changes
Refactor
Documentation