feat(deviceio): generate passthrough trackers from a TOML manifest - #853
Open
nv-jakob wants to merge 1 commit into
Open
feat(deviceio): generate passthrough trackers from a TOML manifest#853nv-jakob wants to merge 1 commit into
nv-jakob wants to merge 1 commit into
Conversation
Contributor
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe change adds TOML-based tracker manifest resolution and configure-time code generation for C++ tracker facades, live/replay implementations, factory wiring, recording traits, pybind bindings, and Python exports. CMake tracks generator inputs and includes generated build fragments. Existing generated tracker implementations replace several hand-maintained files, while selected trackers remain explicit. Documentation describes the generated workflow and boundaries. Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant CMake
participant Generator
participant Manifest
participant TrackerLibraries
CMake->>Generator: run during configure
Generator->>Manifest: load and resolve TOML entries
Manifest-->>Generator: resolved tracker contexts
Generator->>TrackerLibraries: emit sources and include fragments
TrackerLibraries->>TrackerLibraries: compile generated facade/live/replay code
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/source/device/add_device.rst`:
- Around line 141-146: Update the hand-written tracker reference section to
remove references to the migrated generic 3-axis pedal tracker implementation
files. Replace those paths with the corresponding generated build-tree
locations, or substitute a genuinely hand-written tracker example while
preserving the manual facade/live-replay implementation guidance.
- Around line 134-139: Update the documented generate_trackers.py invocation
under “Diagnose surprising defaults” to supply the required --out-dir and
--emit-cmake arguments using temporary output paths, while preserving the
--print-resolved behavior.
In `@src/core/codegen/generate_trackers.py`:
- Around line 486-495: The render_for_entry flow must handle single_collection
entries with record=false consistently with factory emission. Add a dedicated
non-recording render path that omits MCAP traits/readers and recording
constructors, or reject this combination during manifest validation; preserve
the existing render_single_in behavior for recorded entries and update the
related rendering logic around the noted factory handling.
In `@src/core/codegen/manifest.py`:
- Around line 124-127: Validate the resolved direction in the manifest flow
before calling _merge_defaults, accepting only "in" or "out" and rejecting any
other value instead of treating it as inbound or outbound. Add a regression test
covering an unknown direction and verify it fails before defaults are merged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: efb310bc-82d3-48f0-b1ab-fff88897b964
📒 Files selected for processing (68)
cmake/GenerateTrackers.cmakedocs/source/device/add_device.rstdocs/source/index.rstdocs/source/references/generated_trackers.rstsrc/core/AGENTS.mdsrc/core/CMakeLists.txtsrc/core/codegen/AGENTS.mdsrc/core/codegen/generate_trackers.pysrc/core/codegen/manifest.pysrc/core/codegen/templates.pysrc/core/codegen/test_manifest.pysrc/core/deviceio_base/AGENTS.mdsrc/core/deviceio_base/cpp/inc/deviceio_base/generic_3axis_pedal_tracker_base.hppsrc/core/deviceio_base/cpp/inc/deviceio_base/haptic_command_reader_tracker_base.hppsrc/core/deviceio_base/cpp/inc/deviceio_base/joint_state_tracker_base.hppsrc/core/deviceio_base/cpp/inc/deviceio_base/oglo_tactile_tracker_base.hppsrc/core/deviceio_base/cpp/inc/deviceio_base/se3_tracker_base.hppsrc/core/deviceio_session/AGENTS.mdsrc/core/deviceio_trackers/AGENTS.mdsrc/core/deviceio_trackers/cpp/CMakeLists.txtsrc/core/deviceio_trackers/cpp/generic_3axis_pedal_tracker.cppsrc/core/deviceio_trackers/cpp/haptic_command_reader_tracker.cppsrc/core/deviceio_trackers/cpp/inc/deviceio_trackers/generic_3axis_pedal_tracker.hppsrc/core/deviceio_trackers/cpp/inc/deviceio_trackers/haptic_command_reader_tracker.hppsrc/core/deviceio_trackers/cpp/inc/deviceio_trackers/joint_state_tracker.hppsrc/core/deviceio_trackers/cpp/inc/deviceio_trackers/oglo_tactile_tracker.hppsrc/core/deviceio_trackers/cpp/inc/deviceio_trackers/se3_tracker.hppsrc/core/deviceio_trackers/cpp/joint_state_tracker.cppsrc/core/deviceio_trackers/cpp/oglo_tactile_tracker.cppsrc/core/deviceio_trackers/cpp/se3_tracker.cppsrc/core/deviceio_trackers/defaults.tomlsrc/core/deviceio_trackers/python/CMakeLists.txtsrc/core/deviceio_trackers/python/deviceio_trackers_init.pysrc/core/deviceio_trackers/python/tracker_bindings.cppsrc/core/deviceio_trackers/trackers.tomlsrc/core/live_trackers/AGENTS.mdsrc/core/live_trackers/cpp/CMakeLists.txtsrc/core/live_trackers/cpp/inc/live_trackers/live_deviceio_factory.hppsrc/core/live_trackers/cpp/live_deviceio_factory.cppsrc/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_haptic_command_reader_tracker_impl.cppsrc/core/live_trackers/cpp/live_haptic_command_reader_tracker_impl.hppsrc/core/live_trackers/cpp/live_joint_state_tracker_impl.cppsrc/core/live_trackers/cpp/live_joint_state_tracker_impl.hppsrc/core/live_trackers/cpp/live_oglo_tactile_tracker_impl.cppsrc/core/live_trackers/cpp/live_oglo_tactile_tracker_impl.hppsrc/core/live_trackers/cpp/live_se3_tracker_impl.cppsrc/core/live_trackers/cpp/live_se3_tracker_impl.hppsrc/core/mcap/cpp/CMakeLists.txtsrc/core/mcap/cpp/inc/mcap/recording_traits.hppsrc/core/python/CMakeLists.txtsrc/core/replay_trackers/AGENTS.mdsrc/core/replay_trackers/cpp/CMakeLists.txtsrc/core/replay_trackers/cpp/inc/replay_trackers/replay_deviceio_factory.hppsrc/core/replay_trackers/cpp/replay_deviceio_factory.cppsrc/core/replay_trackers/cpp/replay_generic_3axis_pedal_tracker_impl.cppsrc/core/replay_trackers/cpp/replay_generic_3axis_pedal_tracker_impl.hppsrc/core/replay_trackers/cpp/replay_haptic_command_reader_tracker_impl.cppsrc/core/replay_trackers/cpp/replay_haptic_command_reader_tracker_impl.hppsrc/core/replay_trackers/cpp/replay_joint_state_tracker_impl.cppsrc/core/replay_trackers/cpp/replay_joint_state_tracker_impl.hppsrc/core/replay_trackers/cpp/replay_oglo_tactile_tracker_impl.cppsrc/core/replay_trackers/cpp/replay_oglo_tactile_tracker_impl.hppsrc/core/replay_trackers/cpp/replay_se3_tracker_impl.cppsrc/core/replay_trackers/cpp/replay_se3_tracker_impl.hppsrc/core/retargeting_engine_tests/python/test_haptic_devices.pysrc/haptic_devices/push_tensor.py
💤 Files with no reviewable changes (35)
- src/core/deviceio_trackers/cpp/oglo_tactile_tracker.cpp
- src/core/replay_trackers/cpp/replay_haptic_command_reader_tracker_impl.hpp
- src/core/deviceio_base/cpp/inc/deviceio_base/joint_state_tracker_base.hpp
- src/core/deviceio_base/cpp/inc/deviceio_base/oglo_tactile_tracker_base.hpp
- src/core/deviceio_trackers/cpp/inc/deviceio_trackers/joint_state_tracker.hpp
- src/core/replay_trackers/cpp/replay_generic_3axis_pedal_tracker_impl.hpp
- src/core/deviceio_base/cpp/inc/deviceio_base/haptic_command_reader_tracker_base.hpp
- src/core/replay_trackers/cpp/replay_se3_tracker_impl.cpp
- src/core/deviceio_trackers/cpp/haptic_command_reader_tracker.cpp
- src/core/deviceio_trackers/cpp/joint_state_tracker.cpp
- src/core/replay_trackers/cpp/replay_generic_3axis_pedal_tracker_impl.cpp
- src/core/replay_trackers/cpp/replay_oglo_tactile_tracker_impl.hpp
- src/core/deviceio_trackers/cpp/generic_3axis_pedal_tracker.cpp
- src/core/live_trackers/cpp/live_oglo_tactile_tracker_impl.hpp
- src/core/live_trackers/cpp/live_generic_3axis_pedal_tracker_impl.hpp
- src/core/live_trackers/cpp/live_oglo_tactile_tracker_impl.cpp
- src/core/deviceio_trackers/cpp/inc/deviceio_trackers/haptic_command_reader_tracker.hpp
- src/core/replay_trackers/cpp/replay_haptic_command_reader_tracker_impl.cpp
- src/core/live_trackers/cpp/live_joint_state_tracker_impl.hpp
- src/core/live_trackers/cpp/live_generic_3axis_pedal_tracker_impl.cpp
- src/core/deviceio_base/cpp/inc/deviceio_base/generic_3axis_pedal_tracker_base.hpp
- src/core/deviceio_trackers/cpp/inc/deviceio_trackers/se3_tracker.hpp
- src/core/replay_trackers/cpp/replay_oglo_tactile_tracker_impl.cpp
- src/core/live_trackers/cpp/live_joint_state_tracker_impl.cpp
- src/core/deviceio_trackers/cpp/se3_tracker.cpp
- src/core/live_trackers/cpp/live_haptic_command_reader_tracker_impl.hpp
- src/core/deviceio_trackers/cpp/inc/deviceio_trackers/generic_3axis_pedal_tracker.hpp
- src/core/replay_trackers/cpp/replay_se3_tracker_impl.hpp
- src/core/live_trackers/cpp/live_se3_tracker_impl.cpp
- src/core/replay_trackers/cpp/replay_joint_state_tracker_impl.hpp
- src/core/live_trackers/cpp/live_se3_tracker_impl.hpp
- src/core/replay_trackers/cpp/replay_joint_state_tracker_impl.cpp
- src/core/deviceio_trackers/cpp/inc/deviceio_trackers/oglo_tactile_tracker.hpp
- src/core/live_trackers/cpp/live_haptic_command_reader_tracker_impl.cpp
- src/core/deviceio_base/cpp/inc/deviceio_base/se3_tracker_base.hpp
nv-jakob
force-pushed
the
jakob/generate-deviceio
branch
2 times, most recently
from
July 30, 2026 15:28
9afbb34 to
67307c6
Compare
nv-jakob
force-pushed
the
jakob/generate-deviceio
branch
from
July 30, 2026 17:11
67307c6 to
a7fb344
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/core/codegen/generate_trackers.py`:
- Around line 897-907: Restrict _prune_stale to deleting only generator-owned
output files, preferably by filtering paths to the emitted tracker/header
extensions before unlinking; alternatively require a generator-written ownership
marker in out_dir and skip pruning when it is absent. Preserve pruning of stale
generated files while never deleting unrelated files from a caller-supplied
directory.
- Around line 684-691: Update the factory guards in the relevant generation
paths to require both recording and the single-endpoint shape before emitting
MCAP channel declarations or specialized constructors. Apply the same shape
check in _gen_replay_factory_methods and _gen_recording_traits, and ensure
recorded multi_endpoint entries use the multi implementation’s supported
constructors or are rejected during manifest validation.
In `@src/core/codegen/manifest.py`:
- Around line 106-113: Update the defaults merge logic to remove every known
direction table from the copied defaults root before applying the selected
overlay and base values, rather than popping only the table named by direction.
Preserve the existing selected-overlay precedence and ensure unrelated direction
tables cannot remain in the returned merged entry.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 90d6a4b5-bcde-45f6-9bff-d34433d3d32a
📒 Files selected for processing (68)
cmake/GenerateTrackers.cmakedocs/source/device/add_device.rstdocs/source/index.rstdocs/source/references/generated_trackers.rstsrc/core/AGENTS.mdsrc/core/CMakeLists.txtsrc/core/codegen/AGENTS.mdsrc/core/codegen/generate_trackers.pysrc/core/codegen/manifest.pysrc/core/codegen/templates.pysrc/core/codegen/test_manifest.pysrc/core/deviceio_base/AGENTS.mdsrc/core/deviceio_base/cpp/inc/deviceio_base/generic_3axis_pedal_tracker_base.hppsrc/core/deviceio_base/cpp/inc/deviceio_base/haptic_command_reader_tracker_base.hppsrc/core/deviceio_base/cpp/inc/deviceio_base/joint_state_tracker_base.hppsrc/core/deviceio_base/cpp/inc/deviceio_base/oglo_tactile_tracker_base.hppsrc/core/deviceio_base/cpp/inc/deviceio_base/se3_tracker_base.hppsrc/core/deviceio_session/AGENTS.mdsrc/core/deviceio_trackers/AGENTS.mdsrc/core/deviceio_trackers/cpp/CMakeLists.txtsrc/core/deviceio_trackers/cpp/generic_3axis_pedal_tracker.cppsrc/core/deviceio_trackers/cpp/haptic_command_reader_tracker.cppsrc/core/deviceio_trackers/cpp/inc/deviceio_trackers/generic_3axis_pedal_tracker.hppsrc/core/deviceio_trackers/cpp/inc/deviceio_trackers/haptic_command_reader_tracker.hppsrc/core/deviceio_trackers/cpp/inc/deviceio_trackers/joint_state_tracker.hppsrc/core/deviceio_trackers/cpp/inc/deviceio_trackers/oglo_tactile_tracker.hppsrc/core/deviceio_trackers/cpp/inc/deviceio_trackers/se3_tracker.hppsrc/core/deviceio_trackers/cpp/joint_state_tracker.cppsrc/core/deviceio_trackers/cpp/oglo_tactile_tracker.cppsrc/core/deviceio_trackers/cpp/se3_tracker.cppsrc/core/deviceio_trackers/defaults.tomlsrc/core/deviceio_trackers/python/CMakeLists.txtsrc/core/deviceio_trackers/python/deviceio_trackers_init.pysrc/core/deviceio_trackers/python/tracker_bindings.cppsrc/core/deviceio_trackers/trackers.tomlsrc/core/live_trackers/AGENTS.mdsrc/core/live_trackers/cpp/CMakeLists.txtsrc/core/live_trackers/cpp/inc/live_trackers/live_deviceio_factory.hppsrc/core/live_trackers/cpp/live_deviceio_factory.cppsrc/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_haptic_command_reader_tracker_impl.cppsrc/core/live_trackers/cpp/live_haptic_command_reader_tracker_impl.hppsrc/core/live_trackers/cpp/live_joint_state_tracker_impl.cppsrc/core/live_trackers/cpp/live_joint_state_tracker_impl.hppsrc/core/live_trackers/cpp/live_oglo_tactile_tracker_impl.cppsrc/core/live_trackers/cpp/live_oglo_tactile_tracker_impl.hppsrc/core/live_trackers/cpp/live_se3_tracker_impl.cppsrc/core/live_trackers/cpp/live_se3_tracker_impl.hppsrc/core/mcap/cpp/CMakeLists.txtsrc/core/mcap/cpp/inc/mcap/recording_traits.hppsrc/core/python/CMakeLists.txtsrc/core/replay_trackers/AGENTS.mdsrc/core/replay_trackers/cpp/CMakeLists.txtsrc/core/replay_trackers/cpp/inc/replay_trackers/replay_deviceio_factory.hppsrc/core/replay_trackers/cpp/replay_deviceio_factory.cppsrc/core/replay_trackers/cpp/replay_generic_3axis_pedal_tracker_impl.cppsrc/core/replay_trackers/cpp/replay_generic_3axis_pedal_tracker_impl.hppsrc/core/replay_trackers/cpp/replay_haptic_command_reader_tracker_impl.cppsrc/core/replay_trackers/cpp/replay_haptic_command_reader_tracker_impl.hppsrc/core/replay_trackers/cpp/replay_joint_state_tracker_impl.cppsrc/core/replay_trackers/cpp/replay_joint_state_tracker_impl.hppsrc/core/replay_trackers/cpp/replay_oglo_tactile_tracker_impl.cppsrc/core/replay_trackers/cpp/replay_oglo_tactile_tracker_impl.hppsrc/core/replay_trackers/cpp/replay_se3_tracker_impl.cppsrc/core/replay_trackers/cpp/replay_se3_tracker_impl.hppsrc/core/retargeting_engine_tests/python/test_haptic_devices.pysrc/haptic_devices/push_tensor.py
💤 Files with no reviewable changes (35)
- src/core/deviceio_base/cpp/inc/deviceio_base/se3_tracker_base.hpp
- src/core/deviceio_trackers/cpp/oglo_tactile_tracker.cpp
- src/core/replay_trackers/cpp/replay_haptic_command_reader_tracker_impl.cpp
- src/core/deviceio_trackers/cpp/se3_tracker.cpp
- src/core/deviceio_trackers/cpp/joint_state_tracker.cpp
- src/core/deviceio_trackers/cpp/haptic_command_reader_tracker.cpp
- src/core/deviceio_base/cpp/inc/deviceio_base/haptic_command_reader_tracker_base.hpp
- src/core/deviceio_base/cpp/inc/deviceio_base/joint_state_tracker_base.hpp
- src/core/replay_trackers/cpp/replay_joint_state_tracker_impl.hpp
- src/core/deviceio_trackers/cpp/generic_3axis_pedal_tracker.cpp
- src/core/replay_trackers/cpp/replay_generic_3axis_pedal_tracker_impl.hpp
- src/core/deviceio_trackers/cpp/inc/deviceio_trackers/generic_3axis_pedal_tracker.hpp
- src/core/replay_trackers/cpp/replay_oglo_tactile_tracker_impl.hpp
- src/core/replay_trackers/cpp/replay_generic_3axis_pedal_tracker_impl.cpp
- src/core/deviceio_base/cpp/inc/deviceio_base/generic_3axis_pedal_tracker_base.hpp
- src/core/replay_trackers/cpp/replay_joint_state_tracker_impl.cpp
- src/core/live_trackers/cpp/live_oglo_tactile_tracker_impl.cpp
- src/core/replay_trackers/cpp/replay_se3_tracker_impl.hpp
- src/core/deviceio_trackers/cpp/inc/deviceio_trackers/se3_tracker.hpp
- src/core/deviceio_trackers/cpp/inc/deviceio_trackers/joint_state_tracker.hpp
- src/core/deviceio_base/cpp/inc/deviceio_base/oglo_tactile_tracker_base.hpp
- src/core/replay_trackers/cpp/replay_haptic_command_reader_tracker_impl.hpp
- src/core/live_trackers/cpp/live_joint_state_tracker_impl.hpp
- src/core/live_trackers/cpp/live_oglo_tactile_tracker_impl.hpp
- src/core/deviceio_trackers/cpp/inc/deviceio_trackers/oglo_tactile_tracker.hpp
- src/core/replay_trackers/cpp/replay_se3_tracker_impl.cpp
- src/core/live_trackers/cpp/live_se3_tracker_impl.hpp
- src/core/deviceio_trackers/cpp/inc/deviceio_trackers/haptic_command_reader_tracker.hpp
- src/core/live_trackers/cpp/live_haptic_command_reader_tracker_impl.cpp
- src/core/replay_trackers/cpp/replay_oglo_tactile_tracker_impl.cpp
- src/core/live_trackers/cpp/live_se3_tracker_impl.cpp
- src/core/live_trackers/cpp/live_haptic_command_reader_tracker_impl.hpp
- src/core/live_trackers/cpp/live_generic_3axis_pedal_tracker_impl.cpp
- src/core/live_trackers/cpp/live_generic_3axis_pedal_tracker_impl.hpp
- src/core/live_trackers/cpp/live_joint_state_tracker_impl.cpp
🚧 Files skipped from review as they are similar to previous changes (7)
- src/core/deviceio_trackers/python/CMakeLists.txt
- src/core/mcap/cpp/CMakeLists.txt
- src/core/deviceio_session/AGENTS.md
- src/core/replay_trackers/AGENTS.md
- src/core/live_trackers/AGENTS.md
- src/core/deviceio_trackers/AGENTS.md
- src/core/deviceio_base/AGENTS.md
Comment on lines
+684
to
+691
| if ctx.direction == "in" and ctx.record: | ||
| yield f" std::unique_ptr<{ctx.mcap_channels_type}> channels;" | ||
| yield " if (should_record(tracker)) {" | ||
| yield f" channels = {ctx.live_impl}::create_mcap_channels(*writer_, get_name(tracker));" | ||
| yield " }" | ||
| yield f" return std::make_unique<{ctx.live_impl}>(handles_, tracker, std::move(channels));" | ||
| else: | ||
| yield f" return std::make_unique<{ctx.live_impl}>(handles_, tracker);" |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Factory guard keys off record alone, so a recorded multi_endpoint entry emits uncompilable C++.
The record branch declares {ctx.mcap_channels_type} and calls the 3-arg live constructor, but _gen_multi_live_header defines neither — only render_single_in does. _gen_replay_factory_methods (lines 707-710) has the same problem: it calls the 2-arg replay constructor while the multi replay impl is default-constructible only. haptic_command_reader escapes today purely because record = false; the first recorded multi-endpoint tracker breaks the build. Gate on shape as well (or reject the combination in manifest validation, as is done for single_collection + record=false).
🔧 Proposed fix
- if ctx.direction == "in" and ctx.record:
+ if ctx.direction == "in" and ctx.shape == "single_collection" and ctx.record:
yield f" std::unique_ptr<{ctx.mcap_channels_type}> channels;"Apply the same shape check in _gen_replay_factory_methods and _gen_recording_traits.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/core/codegen/generate_trackers.py` around lines 684 - 691, Update the
factory guards in the relevant generation paths to require both recording and
the single-endpoint shape before emitting MCAP channel declarations or
specialized constructors. Apply the same shape check in
_gen_replay_factory_methods and _gen_recording_traits, and ensure recorded
multi_endpoint entries use the multi implementation’s supported constructors or
are rejected during manifest validation.
Comment on lines
+897
to
+907
| def _prune_stale(out_dir: Path, keep: set[Path]) -> None: | ||
| """Delete previously generated files this run did not emit. | ||
|
|
||
| Renaming a tracker or a fragment otherwise leaves the old header on an include path, | ||
| where it keeps satisfying a stale #include instead of failing the build. | ||
| """ | ||
| if not out_dir.is_dir(): | ||
| return | ||
| for path in out_dir.rglob("*"): | ||
| if path.is_file() and path.resolve() not in keep: | ||
| path.unlink() |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
_prune_stale recursively deletes every file under a caller-supplied --out-dir.
There is no ownership marker or extension filter, so pointing --out-dir at an existing directory (the docs even suggest /tmp/isaac-teleop-trackers) wipes unrelated files. Restrict deletion to the extensions the generator emits, or refuse to prune a directory that lacks a generator-written marker file.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/core/codegen/generate_trackers.py` around lines 897 - 907, Restrict
_prune_stale to deleting only generator-owned output files, preferably by
filtering paths to the emitted tracker/header extensions before unlinking;
alternatively require a generator-written ownership marker in out_dir and skip
pruning when it is absent. Preserve pruning of stale generated files while never
deleting unrelated files from a caller-supplied directory.
Comment on lines
+106
to
+113
| root = dict(defaults_doc.get("defaults", {})) | ||
| overlay = root.pop(direction, {}) | ||
| if not isinstance(overlay, dict): | ||
| overlay = {} | ||
| merged = root | ||
| merged.update(overlay) | ||
| merged.update(base) | ||
| return merged |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Non-selected direction tables leak into the merged entry.
root.pop(direction, ...) removes only the chosen overlay; the other direction's table (e.g. [defaults.out] when direction = "in") stays in merged as a nested dict, gets placeholder-expanded, and lands in the resolved entry. Today it happens to resolve, but any placeholder that exists only in the other overlay will fail resolution for an unrelated entry. Drop every known direction table.
🔧 Proposed fix
- root = dict(defaults_doc.get("defaults", {}))
- overlay = root.pop(direction, {})
- if not isinstance(overlay, dict):
- overlay = {}
+ root = dict(defaults_doc.get("defaults", {}))
+ overlay = root.get(direction, {})
+ if not isinstance(overlay, dict):
+ overlay = {}
+ for candidate in VALID_DIRECTIONS:
+ root.pop(candidate, None)📝 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
| root = dict(defaults_doc.get("defaults", {})) | |
| overlay = root.pop(direction, {}) | |
| if not isinstance(overlay, dict): | |
| overlay = {} | |
| merged = root | |
| merged.update(overlay) | |
| merged.update(base) | |
| return merged | |
| root = dict(defaults_doc.get("defaults", {})) | |
| overlay = root.get(direction, {}) | |
| if not isinstance(overlay, dict): | |
| overlay = {} | |
| for candidate in VALID_DIRECTIONS: | |
| root.pop(candidate, None) | |
| merged = root | |
| merged.update(overlay) | |
| merged.update(base) | |
| return merged |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/core/codegen/manifest.py` around lines 106 - 113, Update the defaults
merge logic to remove every known direction table from the copied defaults root
before applying the selected overlay and base values, rather than popping only
the table named by direction. Preserve the existing selected-overlay precedence
and ensure unrelated direction tables cannot remain in the returned merged
entry.
jiwenc-nv
reviewed
Jul 30, 2026
jiwenc-nv
reviewed
Jul 30, 2026
| PICO vendor. Its ``collection_id`` and ``max_flatbuffer_size`` also arrive through | ||
| ``TrackerVendor::params`` at runtime instead of being fixed at generation time. | ||
|
|
||
| **Schema pybind bindings.** The field-by-field ``src/core/schema/python/*_bindings.h`` files are |
Collaborator
There was a problem hiding this comment.
@aristarkhovNV, I think you can follow up here for using flatbuffer objs rather than the manually coded bindings as the next step.
jiwenc-nv
requested changes
Jul 30, 2026
jiwenc-nv
left a comment
jiwenc-nv
left a comment
Collaborator
There was a problem hiding this comment.
let's use template files, rather than writing c++ code in python strings :)
nv-jakob
force-pushed
the
jakob/generate-deviceio
branch
from
July 30, 2026 23:48
a7fb344 to
c4f5a47
Compare
nv-jakob
force-pushed
the
jakob/generate-deviceio
branch
from
July 31, 2026 00:07
c4f5a47 to
5fe868c
Compare
nv-jakob
force-pushed
the
jakob/generate-deviceio
branch
from
July 31, 2026 11:52
5fe868c to
c889682
Compare
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
nv-jakob
force-pushed
the
jakob/generate-deviceio
branch
2 times, most recently
from
July 31, 2026 12:18
8638eca to
067a6d2
Compare
Six trackers were pure FlatBuffer passthrough over the OpenXR tensor extensions, and after normalizing type names their live and replay impls were the same file. The duplication had already caused drift: the se3 replay impl logged a missing-data gap once behind a warned_no_data_ flag while the oglo one wrote to std::cerr every frame — same intent, only one copy got the fix. Each new tracker also cost ~24 files of mechanical code plus edits to six shared registration points. Declare passthrough trackers in deviceio_trackers/trackers.toml with %placeholder% defaults in defaults.toml. At CMake configure time, manifest.py resolves entries, templates.py builds a TrackerGenContext per tracker, and generate_trackers.py emits C++ sources, generated_sources.cmake, and .inc fragments (factory dispatch, recording traits, pybind, Python __all__) that hand-written files include. C++ bodies live under codegen/templates/ (direction in/ and out/, plus fragments/ for splice-ins) as *.hpp.template / *.cpp.template with @key@ substitution via in_renderer.py; .template files sit outside the clang-format glob. Generation is stdlib-only (line-yielding templates, no Jinja) so bare configure-time Python does not need site-packages. Adding a passthrough tracker is now a .fbs schema plus ~10 lines of TOML. Migrate joint_state, se3_tracker, oglo_tactile, generic_3axis_pedal, and haptic_command (direction = out). Delete their hand-written stacks. All generated replay impls adopt warn-once-per-gap logging, so oglo's per-frame stderr spam is gone — a deliberate behavior change. direction = "out" generates a typed <Table>PushTracker around SchemaPusher, so PushTensorHapticDevice pushes HapticCommand instead of hand-rolling pack_haptic_command on the untyped TensorPushTracker bytes escape hatch. HapticCommandReaderTracker stays hand-written (read_all_samples with per-endpoint bucketing); a generated multi_endpoint shape was tried and removed. head, hand, controller, full_body, message_channel, and FrameMetadataTrackerOak remain hand-written. Docs and package AGENTS.md files describe the manifest workflow. Signed-off-by: Jakob Bornecrantz <tbornecrantz@nvidia.com>
nv-jakob
force-pushed
the
jakob/generate-deviceio
branch
from
July 31, 2026 12:40
067a6d2 to
180c436
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Six trackers were pure FlatBuffer passthrough over the OpenXR tensor extensions, and after normalizing type names their live and replay impls were the same file. The duplication had already caused drift: the se3 replay impl logged a missing-data gap once behind a warned_no_data_ flag while the oglo one wrote to std::cerr every frame -- same intent, only one copy got the fix. Each new tracker also cost ~24 files of mechanical code plus edits to six shared registration points.
Declare passthrough trackers in deviceio_trackers/trackers.toml instead, with %placeholder% defaults in defaults.toml, and generate the facade, base interface, and live/replay impls at CMake configure time via codegen/generate_trackers.py. Registration points (factory dispatch rows and methods, MCAP recording traits, pybind blocks, Python all) are emitted as .inc fragments that the hand-written files include, so hand-written trackers keep their rows. Adding a passthrough tracker is now a .fbs schema plus ~10 lines of TOML.
Migrate se3_tracker, oglo_tactile, joint_state, generic_3axis_pedal and haptic_command_reader, and delete their hand-written stacks. All generated replay impls adopt the warn-once-per-gap logging, so oglo's per-frame stderr spam is gone -- a deliberate behavior change, not an accident.
Add a typed producer direction: direction = "out" generates a
PushTracker wrapping SchemaPusher, so PushTensorHapticDevice now pushes a typed HapticCommand instead of hand-rolling pack_haptic_command over the untyped TensorPushTracker, which stays as the bytes escape hatch.head, hand, controller, full_body, message_channel and FrameMetadataTrackerOak remain hand-written; the .fbs schemas and their pybind bindings are out of scope. Docs and the AGENTS.md files for the affected packages are updated to describe the manifest workflow.
Description
Fixes #(issue)
Type of change
Testing
Checklist
SKIP=check-copyright-year pre-commit run --all-filesgit commit -s) per the DCOSummary by CodeRabbit