Open
Conversation
|
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: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds bidirectional opaque byte-level message channel support: new OpenXR NV extension header, C++ tracker interfaces and live OpenXR-backed implementation, Python bindings and retargeting source/sink nodes, FlatBuffers schema and pybind11 bindings, unit tests, and a CLI example script. Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant Source as MessageChannelSource
participant Tracker as MessageChannelTracker
participant Impl as LiveMessageChannelTrackerImpl
participant XR as OpenXR Runtime
App->>Source: poll_tracker(session)
Source->>Tracker: get_status(session)
Tracker->>Impl: get_status()
Impl->>XR: xrGetOpaqueDataChannelStateNV()
XR-->>Impl: status
Impl-->>Tracker: MessageChannelStatus
Tracker-->>Source: status
alt CONNECTED
Source->>Tracker: send_message(session, payload)
Tracker->>Impl: send_message(payload)
Impl->>XR: xrSendOpaqueDataChannelNV(payload)
XR-->>Impl: success
Source->>Tracker: get_messages(session)
Tracker->>Impl: get_messages()
Impl->>XR: xrReceiveOpaqueDataChannelNV(buffer)
XR-->>Impl: messages
Impl-->>Tracker: tracked messages
Tracker-->>Source: tracked messages
end
Source-->>App: outputs (messages, status)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 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/messageChannelTracker
branch
from
2df8e2b to
73ceed0
Compare
There was a problem hiding this comment.
Pull request overview
Adds a new “message channel” tracker end-to-end (FlatBuffers schema + C++ live tracker + Python bindings + retargeting-engine source/sink nodes) to support sending/receiving opaque message payloads with remote clients that implement XR_NV_opaque_data_channel.
Changes:
- Introduces
message_channel.fbsschema and Python schema bindings forMessageChannelMessages*types. - Adds
MessageChannelTracker(DeviceIO) andLiveMessageChannelTrackerImpl(OpenXR extension-backed) with factory wiring. - Adds retargeting-engine
MessageChannelSource/MessageChannelSink+ config helper, plus unit tests and an example script.
Reviewed changes
Copilot reviewed 25 out of 25 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/core/schema/python/schema_module.cpp | Registers message channel schema bindings in _schema module. |
| src/core/schema/python/schema_init.py | Exposes message channel schema types in Python package exports. |
| src/core/schema/python/message_channel_bindings.h | Adds pybind11 bindings for message channel schema types. |
| src/core/schema/python/CMakeLists.txt | Includes new binding header in schema python module build. |
| src/core/schema/fbs/message_channel.fbs | Defines FlatBuffers tables for message payloads, tracked batches, and MCAP records. |
| src/core/retargeting_engine_tests/python/test_message_channel_nodes.py | Adds unit tests for message channel source/sink behavior and queueing. |
| src/core/retargeting_engine/python/deviceio_source_nodes/message_channel_source.py | New DeviceIO source node (poll + flush outbound + publish inbound/status). |
| src/core/retargeting_engine/python/deviceio_source_nodes/message_channel_sink.py | New sink node that enqueues outbound batches. |
| src/core/retargeting_engine/python/deviceio_source_nodes/message_channel_config.py | Factory/config for creating paired source/sink nodes with shared tracker + bounded queue. |
| src/core/retargeting_engine/python/deviceio_source_nodes/deviceio_tensor_types.py | Adds tensor types and status enum for message channel tracked data. |
| src/core/retargeting_engine/python/deviceio_source_nodes/init.py | Exports new message channel nodes and tensor types. |
| src/core/python/deviceio_init.py | Exports MessageChannelTracker and MessageChannelStatus from isaacteleop.deviceio. |
| src/core/live_trackers/cpp/live_message_channel_tracker_impl.hpp | Declares live tracker impl and required extension. |
| src/core/live_trackers/cpp/live_message_channel_tracker_impl.cpp | Implements OpenXR channel create/status/send/receive + reconnect logic. |
| src/core/live_trackers/cpp/live_deviceio_factory.cpp | Wires tracker dispatch + impl creation for message channel tracker. |
| src/core/live_trackers/cpp/inc/live_trackers/live_deviceio_factory.hpp | Declares factory method for message channel impl creation. |
| src/core/live_trackers/cpp/CMakeLists.txt | Adds live message channel impl sources to the live_trackers library. |
| src/core/deviceio_trackers/python/tracker_bindings.cpp | Adds pybind11 bindings for MessageChannelTracker and MessageChannelStatus. |
| src/core/deviceio_trackers/python/deviceio_trackers_init.py | Exports message channel tracker/status from isaacteleop.deviceio_trackers. |
| src/core/deviceio_trackers/cpp/message_channel_tracker.cpp | Implements DeviceIO tracker wrapper methods delegating to impl. |
| src/core/deviceio_trackers/cpp/inc/deviceio_trackers/message_channel_tracker.hpp | Declares the new tracker API (uuid/name/max size + get_status/get_messages/send_message). |
| src/core/deviceio_trackers/cpp/CMakeLists.txt | Adds message channel tracker sources/headers to deviceio_trackers build. |
| src/core/deviceio_base/cpp/inc/deviceio_base/message_channel_tracker_base.hpp | Adds tracker impl interface + status enum base definitions. |
| examples/teleop_session_manager/python/message_channel_example.py | Example usage via TeleopSession + retargeting source/sink nodes. |
| deps/cloudxr/openxr_extensions/XR_NV_opaque_data_channel.h | Adds OpenXR extension header for XR_NV_opaque_data_channel. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
nvddr
force-pushed
the
devdeepr/messageChannelTracker
branch
from
73ceed0 to
8a05647
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@examples/teleop_session_manager/python/message_channel_example.py`:
- Line 1: The file starting with the shebang "#!/usr/bin/env python3" needs its
executable permission set so CI/pre-commit stops failing; run chmod +x on the
script and stage the permission change (e.g., git add --chmod=+x) so the
repository records the new executable bit for this file.
In `@src/core/deviceio_trackers/python/tracker_bindings.cpp`:
- Around line 103-107: The binding for MessageChannelTracker::send_message
currently expects a MessageChannelMessagesT object but callers pass raw bytes;
update the pybind lambda in send_message to accept raw payload bytes (e.g.,
py::bytes or py::buffer) instead of core::MessageChannelMessagesT, convert/cast
the Python bytes/buffer to the native payload type expected by
core::MessageChannelTracker::send_message, and call self.send_message(session,
converted_payload). Keep the py::arg names (session, message) and the docstring
but change the lambda signature and conversion so Python bytes are accepted
without raising TypeError.
In
`@src/core/retargeting_engine/python/deviceio_source_nodes/message_channel_sink.py`:
- Line 10: Replace the deprecated typing.Deque import with the modern
collections.deque: change the import from "from typing import TYPE_CHECKING,
Deque" to import deque from collections (keep TYPE_CHECKING), and then update
every type annotation that uses "Deque[...]" to "deque[...]" (i.e., the
deque-typed variable/attribute/parameter annotations in this module). Ensure
annotations remain valid under Python 3.9+ and retain any existing generic
parameters.
In
`@src/core/retargeting_engine/python/deviceio_source_nodes/message_channel_source.py`:
- Around line 62-69: When flushing the head batch from _outbound_queue in
poll_tracker(), protect against partial sends by catching exceptions from
_tracker.send_message; if send_message throws while iterating batch.data,
rebuild the batch to contain only the remaining unsent messages (the suffix) and
put that reconstructed batch back at the head of _outbound_queue, then break out
of the flush loop (stop trying to send more). Update the loop around iterating
batch.data (and the popleft logic) so popleft only happens after the entire
batch was successfully sent, and on exception you preserve the unsent payloads
in the queue and abort further flushing.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: acafa75f-3f06-4a44-a12d-2c1f5000caad
📒 Files selected for processing (25)
deps/cloudxr/openxr_extensions/XR_NV_opaque_data_channel.hexamples/teleop_session_manager/python/message_channel_example.pysrc/core/deviceio_base/cpp/inc/deviceio_base/message_channel_tracker_base.hppsrc/core/deviceio_trackers/cpp/CMakeLists.txtsrc/core/deviceio_trackers/cpp/inc/deviceio_trackers/message_channel_tracker.hppsrc/core/deviceio_trackers/cpp/message_channel_tracker.cppsrc/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_deviceio_factory.cppsrc/core/live_trackers/cpp/live_message_channel_tracker_impl.cppsrc/core/live_trackers/cpp/live_message_channel_tracker_impl.hppsrc/core/python/deviceio_init.pysrc/core/retargeting_engine/python/deviceio_source_nodes/__init__.pysrc/core/retargeting_engine/python/deviceio_source_nodes/deviceio_tensor_types.pysrc/core/retargeting_engine/python/deviceio_source_nodes/message_channel_config.pysrc/core/retargeting_engine/python/deviceio_source_nodes/message_channel_sink.pysrc/core/retargeting_engine/python/deviceio_source_nodes/message_channel_source.pysrc/core/retargeting_engine_tests/python/test_message_channel_nodes.pysrc/core/schema/fbs/message_channel.fbssrc/core/schema/python/CMakeLists.txtsrc/core/schema/python/message_channel_bindings.hsrc/core/schema/python/schema_init.pysrc/core/schema/python/schema_module.cpp
nvddr
force-pushed
the
devdeepr/messageChannelTracker
branch
from
8a05647 to
30d3abe
Compare
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 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/teleop_session_manager/python/message_channel_example.py`:
- Around line 55-60: The CLI option for --outbound-queue-capacity currently
accepts non-positive integers which lead to broken queue behavior; update
parsing for this flag (the parser.add_argument call for
"--outbound-queue-capacity") to validate that the value is > 0 — either by
supplying a custom type/validator that raises argparse.ArgumentTypeError for
non-positive values or by checking args.outbound_queue_capacity after parse_args
and calling parser.error with a clear message if it's <= 0; ensure the error
message mentions "--outbound-queue-capacity" so users know which flag is
invalid.
In `@src/core/live_trackers/cpp/live_message_channel_tracker_impl.hpp`:
- Around line 9-13: Header uses std::array (used around line referencing
std::array in live_message_channel_tracker_impl.hpp) but doesn't include
<array>, which can cause include-order-dependent build failures; add a direct
include for <array> at the top of live_message_channel_tracker_impl.hpp
alongside the existing includes (near XR_NV_opaque_data_channel.h, <memory>,
<string>, <vector>) so that symbols like std::array are defined regardless of
include order.
In
`@src/core/retargeting_engine/python/deviceio_source_nodes/message_channel_config.py`:
- Around line 25-31: Remove the redundant UUID length check from
message_channel_config() and keep a single validation in create_nodes():
specifically, eliminate the duplicate validation that checks
len(self.channel_uuid) != 16 (and any duplicate outbound_queue_capacity checks
if present) from the message_channel_config() path so that create_nodes() is the
single source of truth for these validations (ensure create_nodes() still raises
ValueError with the same messages for channel_uuid and outbound_queue_capacity).
- Around line 37-38: Add an explicit type annotation for the outbound_queue
variable to satisfy mypy: annotate outbound_queue as a deque containing the
expected element type (for example deque[bytes] or deque[Message]) when you
construct it with deque(maxlen=self.outbound_queue_capacity); update the
assignment in message_channel_config.py where outbound_queue is set (using deque
and self.outbound_queue_capacity) to include the proper typing (e.g.,
outbound_queue: deque[YourElementType] =
deque(maxlen=self.outbound_queue_capacity) and import deque typing if needed).
In
`@src/core/retargeting_engine/python/deviceio_source_nodes/message_channel_sink.py`:
- Around line 35-37: The _compute_fn currently assumes
inputs["messages_tracked"] has at least one element and does messages_tracked =
inputs["messages_tracked"][0], which can raise IndexError for empty groups;
update _compute_fn to first check that inputs.get("messages_tracked") is
non-empty (e.g., if inputs.get("messages_tracked") and
len(inputs["messages_tracked"])>0) and only then append the first element to
self._outbound_queue, otherwise skip processing or early-return/handle the empty
case appropriately so no indexing occurs on an empty list.
In
`@src/core/retargeting_engine/python/deviceio_source_nodes/message_channel_source.py`:
- Line 68: The call to self._tracker.send_message(deviceio_session,
message.payload) passes raw bytes (message.payload) but send_message expects a
MessageChannelMessages instance; fix by constructing the required
MessageChannelMessages from the bytes before calling send_message (e.g. create
msg_obj = MessageChannelMessages.from_bytes(message.payload) or
MessageChannelMessages(message.payload) depending on available constructor) and
then call self._tracker.send_message(deviceio_session, msg_obj); alternatively,
if you prefer to change bindings, update tracker_bindings.cpp (lines ~103-107)
so send_message accepts raw bytes directly and forwards them to the C++ API.
- Line 10: Replace the deprecated typing.Deque import with collections.deque:
remove Deque from the typing import and import deque from the collections
module, then update the type annotation that currently uses typing.Deque (the
one referenced on line 40) to use collections.deque (i.e. deque[...]) so the
code uses the modern, non-deprecated type; ensure any other references to
typing.Deque in this module (e.g., variable or parameter annotations) are
changed similarly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 06d7e8a2-69b8-4d59-be84-ca74c1f96936
📒 Files selected for processing (25)
deps/cloudxr/openxr_extensions/XR_NV_opaque_data_channel.hexamples/teleop_session_manager/python/message_channel_example.pysrc/core/deviceio_base/cpp/inc/deviceio_base/message_channel_tracker_base.hppsrc/core/deviceio_trackers/cpp/CMakeLists.txtsrc/core/deviceio_trackers/cpp/inc/deviceio_trackers/message_channel_tracker.hppsrc/core/deviceio_trackers/cpp/message_channel_tracker.cppsrc/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_deviceio_factory.cppsrc/core/live_trackers/cpp/live_message_channel_tracker_impl.cppsrc/core/live_trackers/cpp/live_message_channel_tracker_impl.hppsrc/core/python/deviceio_init.pysrc/core/retargeting_engine/python/deviceio_source_nodes/__init__.pysrc/core/retargeting_engine/python/deviceio_source_nodes/deviceio_tensor_types.pysrc/core/retargeting_engine/python/deviceio_source_nodes/message_channel_config.pysrc/core/retargeting_engine/python/deviceio_source_nodes/message_channel_sink.pysrc/core/retargeting_engine/python/deviceio_source_nodes/message_channel_source.pysrc/core/retargeting_engine_tests/python/test_message_channel_nodes.pysrc/core/schema/fbs/message_channel.fbssrc/core/schema/python/CMakeLists.txtsrc/core/schema/python/message_channel_bindings.hsrc/core/schema/python/schema_init.pysrc/core/schema/python/schema_module.cpp
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (4)
src/core/live_trackers/cpp/live_message_channel_tracker_impl.hpp (1)
9-12:⚠️ Potential issue | 🟡 MinorAdd missing
<array>include.Line 45 uses
std::array<uint8_t, MessageChannelTracker::CHANNEL_UUID_SIZE>but<array>is not directly included. This can cause include-order-dependent build failures.Proposed fix
`#include` <XR_NV_opaque_data_channel.h> +#include <array> `#include` <memory> `#include` <string> `#include` <vector>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/live_trackers/cpp/live_message_channel_tracker_impl.hpp` around lines 9 - 12, The header is missing the <array> include which is required for std::array usage (used with std::array<uint8_t, MessageChannelTracker::CHANNEL_UUID_SIZE>); add `#include` <array> to src/core/live_trackers/cpp/live_message_channel_tracker_impl.hpp so the compiler doesn't rely on include order (refer to the std::array usage and MessageChannelTracker::CHANNEL_UUID_SIZE to locate the spot).src/core/retargeting_engine/python/deviceio_source_nodes/message_channel_source.py (2)
10-10: 🧹 Nitpick | 🔵 TrivialUse
collections.dequeinstead of deprecatedtyping.Deque.
typing.Dequehas been deprecated since Python 3.9.Proposed fix
-from typing import Any, TYPE_CHECKING, Deque +from typing import Any, TYPE_CHECKING +from collections import dequeThen update line 40:
- outbound_queue: "Deque[MessageChannelMessagesTrackedT]", + outbound_queue: "deque[MessageChannelMessagesTrackedT]",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/retargeting_engine/python/deviceio_source_nodes/message_channel_source.py` at line 10, Replace the deprecated typing.Deque import with collections.deque by removing typing.Deque from the import list and importing from collections instead; update any type annotations that reference typing.Deque (e.g., variables or parameters declared as Deque[...] in this module, such as the message channel buffer or functions using Deque) to use collections.deque or typing.Iterable/Sequence as appropriate, and ensure any TYPE_CHECKING usage still works with the new import.
64-70:⚠️ Potential issue | 🟠 MajorPartial flush can cause duplicate message delivery on reconnection.
If
send_message()succeeds for some payloads in a batch but throws on a later one, the entire batch remains in the queue (line 70 only executes after all sends complete). On reconnect, the already-delivered payloads will be resent.Proposed fix: track progress and preserve only unsent messages
if self._last_status == MessageChannelConnectionStatus.CONNECTED: while self._outbound_queue: batch = self._outbound_queue[0] if batch.data: - for message in batch.data: - outbound_message = MessageChannelMessages(message.payload) - self._tracker.send_message(deviceio_session, outbound_message) - self._outbound_queue.popleft() + sent_count = 0 + try: + for message in batch.data: + outbound_message = MessageChannelMessages(message.payload) + self._tracker.send_message(deviceio_session, outbound_message) + sent_count += 1 + except Exception: + # Keep only unsent messages in the batch + if sent_count < len(batch.data): + batch.data = batch.data[sent_count:] + break + else: + self._outbound_queue.popleft() + else: + self._outbound_queue.popleft()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/retargeting_engine/python/deviceio_source_nodes/message_channel_source.py` around lines 64 - 70, The loop over self._outbound_queue can leave partially-sent batches intact if send_message raises, causing duplicates on reconnect; update the send loop to track progress per batch (iterate with an index or consume messages) and on exception remove only the successfully-sent messages from batch.data (or replace batch.data with the unsent suffix) and call self._outbound_queue.popleft() only when batch.data is fully sent; specifically modify the block that builds outbound_message (MessageChannelMessages) and calls self._tracker.send_message(deviceio_session, outbound_message) so that failed sends preserve only unsent messages in self._outbound_queue rather than discarding the whole batch.src/core/retargeting_engine/python/deviceio_source_nodes/message_channel_config.py (1)
62-65: 🧹 Nitpick | 🔵 TrivialDuplicate UUID validation remains.
The UUID length check at lines 62-65 duplicates the validation in
create_nodes()at lines 30-33. Sincemessage_channel_config()always callscreate_nodes(), the outer check is redundant.Optional simplification
def message_channel_config( name: str, channel_uuid: bytes, channel_name: str = "", max_message_size: int = 64 * 1024, outbound_queue_capacity: int = 256, ) -> tuple[MessageChannelSource, MessageChannelSink]: """Create source/sink nodes and shared tracker for a message channel.""" - if len(channel_uuid) != 16: - raise ValueError( - "message_channel_config: channel_uuid must be exactly 16 bytes" - ) return MessageChannelConfig(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/retargeting_engine/python/deviceio_source_nodes/message_channel_config.py` around lines 62 - 65, The length check for channel_uuid in message_channel_config duplicates the validation already performed in create_nodes; remove the redundant block that raises ValueError ("message_channel_config: channel_uuid must be exactly 16 bytes") from message_channel_config so that create_nodes is the single source of truth for UUID validation, leaving create_nodes' validation unchanged and ensuring message_channel_config simply calls create_nodes(channel_uuid, ...) without re-checking length.
🤖 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/teleop_session_manager/python/message_channel_example.py`:
- Around line 27-29: Move UUID validation into argparse by using
_parse_uuid_bytes as the type= callable for the --channel-uuid argument so
invalid input yields a proper argparse error instead of a raw ValueError; update
_parse_uuid_bytes to catch ValueError from uuid.UUID(uuid_text) and re-raise
argparse.ArgumentTypeError with a clear message (referencing the flag name),
then pass that function to add_argument for --channel-uuid so parse_args() will
surface friendly errors.
In `@src/core/live_trackers/cpp/live_deviceio_factory.cpp`:
- Around line 214-218: The create_message_channel_tracker_impl path currently
constructs LiveMessageChannelTrackerImpl without wiring MCAP recording, so
MessageChannelTracker traffic is silently skipped; update
LiveDeviceIOFactory::create_message_channel_tracker_impl to detect recording
(use writer_ and tracker->get_name()), build the typed
MessageChannelMcapChannels via create_mcap_channels(...) and pass those channels
(or the writer_/name) into the LiveMessageChannelTrackerImpl constructor instead
of the bare ctor; also add the matching plumbing in
live_message_channel_tracker_impl.hpp/.cpp to accept and use
MessageChannelMcapChannels and to emit MessageChannelMessagesRecord when
recording is enabled.
In `@src/core/live_trackers/cpp/live_message_channel_tracker_impl.cpp`:
- Around line 144-165: The code re-queries the system ID in resolve_system_id()
using XR_FORM_FACTOR_HEAD_MOUNTED_DISPLAY, which can diverge from the session's
actual system; instead return or use the session-stored XrSystemId (the one
saved at session creation) when building channels. Update resolve_system_id()
(or the call site in create_channel()) to fetch the already-recorded system id
(e.g., the session's stored system_id_ member or the system id on handles_ if
available) rather than calling get_system_fn_ with
XR_FORM_FACTOR_HEAD_MOUNTED_DISPLAY, and remove the hard-coded form factor query
so create_channel() uses the session's true XrSystemId.
- Around line 18-25: The constructor currently dereferences tracker in the
initializer list (receive_buffer_(tracker->max_message_size(), 0)) before
validating tracker; move the null check for tracker (and other runtime checks)
out of assert and into the constructor body, validate tracker != nullptr at the
start of LiveMessageChannelTrackerImpl::LiveMessageChannelTrackerImpl and throw
a std::runtime_error (or similar) if null, then initialize or resize
receive_buffer_ using tracker->max_message_size() after the check (or
default-construct receive_buffer_ in the initializer list and resize it in the
body); keep the other OpenXR handle checks as runtime checks rather than asserts
as needed.
- Around line 172-186: destroy_channel() currently ignores XrResult from
shutdown_fn_ and destroy_channel_fn_ (wrappers for
xrShutdownOpaqueDataChannelNV/xrDestroyOpaqueDataChannelNV) and is noexcept so
failures are silently lost; modify
LiveMessageChannelTrackerImpl::destroy_channel() to capture the XrResult
returned by shutdown_fn_(channel_) and destroy_channel_fn_(channel_), check for
!= XR_SUCCESS, and emit a clear diagnostic log including the result code and
channel_ (use the same logging facility used elsewhere in this class) while
still not throwing; after calling them, clear channel_ and null out
shutdown_fn_/destroy_channel_fn_ so state is consistent even on failure.
- Around line 59-79: The one-shot call to receive_fn_ in
LiveMessageChannelTrackerImpl can overflow receive_buffer_ if a remote message
exceeds max_message_size(); change to the NVIDIA two-call pattern: first call
receive_fn_(channel_, 0, &count_out, nullptr) to query pending bytes, handle
XR_ERROR_CHANNEL_NOT_CONNECTED_NV and other non-success results as before, then
if count_out == 0 return/break, validate count_out <= max_message_size() and
handle oversize messages (e.g., drop/log and return false or skip) instead of
proceeding, resize or ensure receive_buffer_.size() >= count_out, and perform
the second call receive_fn_(channel_, count_out, &count_out,
receive_buffer_.data()) to actually read and then push the message into
messages_.data. Ensure error handling and logging mirror previous behavior and
reference receive_fn_, channel_, receive_buffer_, max_message_size(), and
messages_.data.
---
Duplicate comments:
In `@src/core/live_trackers/cpp/live_message_channel_tracker_impl.hpp`:
- Around line 9-12: The header is missing the <array> include which is required
for std::array usage (used with std::array<uint8_t,
MessageChannelTracker::CHANNEL_UUID_SIZE>); add `#include` <array> to
src/core/live_trackers/cpp/live_message_channel_tracker_impl.hpp so the compiler
doesn't rely on include order (refer to the std::array usage and
MessageChannelTracker::CHANNEL_UUID_SIZE to locate the spot).
In
`@src/core/retargeting_engine/python/deviceio_source_nodes/message_channel_config.py`:
- Around line 62-65: The length check for channel_uuid in message_channel_config
duplicates the validation already performed in create_nodes; remove the
redundant block that raises ValueError ("message_channel_config: channel_uuid
must be exactly 16 bytes") from message_channel_config so that create_nodes is
the single source of truth for UUID validation, leaving create_nodes' validation
unchanged and ensuring message_channel_config simply calls
create_nodes(channel_uuid, ...) without re-checking length.
In
`@src/core/retargeting_engine/python/deviceio_source_nodes/message_channel_source.py`:
- Line 10: Replace the deprecated typing.Deque import with collections.deque by
removing typing.Deque from the import list and importing from collections
instead; update any type annotations that reference typing.Deque (e.g.,
variables or parameters declared as Deque[...] in this module, such as the
message channel buffer or functions using Deque) to use collections.deque or
typing.Iterable/Sequence as appropriate, and ensure any TYPE_CHECKING usage
still works with the new import.
- Around line 64-70: The loop over self._outbound_queue can leave partially-sent
batches intact if send_message raises, causing duplicates on reconnect; update
the send loop to track progress per batch (iterate with an index or consume
messages) and on exception remove only the successfully-sent messages from
batch.data (or replace batch.data with the unsent suffix) and call
self._outbound_queue.popleft() only when batch.data is fully sent; specifically
modify the block that builds outbound_message (MessageChannelMessages) and calls
self._tracker.send_message(deviceio_session, outbound_message) so that failed
sends preserve only unsent messages in self._outbound_queue rather than
discarding the whole batch.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6995ce40-eede-448f-be0f-fc46277a6b3d
📒 Files selected for processing (25)
deps/cloudxr/openxr_extensions/XR_NV_opaque_data_channel.hexamples/teleop_session_manager/python/message_channel_example.pysrc/core/deviceio_base/cpp/inc/deviceio_base/message_channel_tracker_base.hppsrc/core/deviceio_trackers/cpp/CMakeLists.txtsrc/core/deviceio_trackers/cpp/inc/deviceio_trackers/message_channel_tracker.hppsrc/core/deviceio_trackers/cpp/message_channel_tracker.cppsrc/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_deviceio_factory.cppsrc/core/live_trackers/cpp/live_message_channel_tracker_impl.cppsrc/core/live_trackers/cpp/live_message_channel_tracker_impl.hppsrc/core/python/deviceio_init.pysrc/core/retargeting_engine/python/deviceio_source_nodes/__init__.pysrc/core/retargeting_engine/python/deviceio_source_nodes/deviceio_tensor_types.pysrc/core/retargeting_engine/python/deviceio_source_nodes/message_channel_config.pysrc/core/retargeting_engine/python/deviceio_source_nodes/message_channel_sink.pysrc/core/retargeting_engine/python/deviceio_source_nodes/message_channel_source.pysrc/core/retargeting_engine_tests/python/test_message_channel_nodes.pysrc/core/schema/fbs/message_channel.fbssrc/core/schema/python/CMakeLists.txtsrc/core/schema/python/message_channel_bindings.hsrc/core/schema/python/schema_init.pysrc/core/schema/python/schema_module.cpp
nvddr
force-pushed
the
devdeepr/messageChannelTracker
branch
from
30d3abe to
a6a76f0
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
src/core/live_trackers/cpp/live_deviceio_factory.cpp (1)
214-218:⚠️ Potential issue | 🟠 MajorPreserve MCAP recording for message-channel trackers.
Every other
create_*_tracker_implpath gates typed MCAP channels behindshould_record(tracker), but this one always buildsLiveMessageChannelTrackerImplbare. Recording-enabled sessions will silently drop message-channel traffic.🛠️ Suggested wiring
std::unique_ptr<IMessageChannelTrackerImpl> LiveDeviceIOFactory::create_message_channel_tracker_impl( const MessageChannelTracker* tracker) { - return std::make_unique<LiveMessageChannelTrackerImpl>(handles_, tracker); + std::unique_ptr<MessageChannelMcapChannels> channels; + if (should_record(tracker)) + { + channels = LiveMessageChannelTrackerImpl::create_mcap_channels(*writer_, get_name(tracker)); + } + return std::make_unique<LiveMessageChannelTrackerImpl>(handles_, tracker, std::move(channels)); }This also needs the matching constructor /
create_mcap_channels(...)plumbing inlive_message_channel_tracker_impl.hppand.cpp.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/live_trackers/cpp/live_deviceio_factory.cpp` around lines 214 - 218, The LiveDeviceIOFactory::create_message_channel_tracker_impl currently always constructs LiveMessageChannelTrackerImpl and ignores MCAP recording; change it to check should_record(tracker) and, when true, call the create_mcap_channels(...) plumbing and pass the MCAP channel(s) into LiveMessageChannelTrackerImpl (ensure the new constructor overload in LiveMessageChannelTrackerImpl that accepts MCAP channels is used), otherwise construct the non-recording LiveMessageChannelTrackerImpl as before; also add the corresponding constructor and create_mcap_channels implementation in live_message_channel_tracker_impl.hpp/.cpp to wire the MCAP channels through.src/core/retargeting_engine/python/deviceio_source_nodes/message_channel_sink.py (1)
35-37:⚠️ Potential issue | 🟠 MajorGuard empty
messages_trackedgroups before indexing.Line 36 assumes slot
0is present. An empty input group raisesIndexErrorand aborts the node instead of becoming a no-op.🛠️ Suggested fix
def _compute_fn(self, inputs: RetargeterIO, outputs: RetargeterIO, context) -> None: - messages_tracked = inputs["messages_tracked"][0] - self._outbound_queue.append(messages_tracked) + group = inputs["messages_tracked"] + if len(group) == 0: + return + self._outbound_queue.append(group[0])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/retargeting_engine/python/deviceio_source_nodes/message_channel_sink.py` around lines 35 - 37, The _compute_fn currently indexes inputs["messages_tracked"][0] without checking for an empty group; update _compute_fn to first check that inputs contains a non-empty "messages_tracked" group (e.g. if inputs.get("messages_tracked") and len(inputs["messages_tracked"])>0 or simply if inputs["messages_tracked"]: ), only then assign messages_tracked = inputs["messages_tracked"][0] and append to self._outbound_queue; otherwise do nothing (no-op) so the node won't raise IndexError.
🤖 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/teleop_session_manager/python/message_channel_example.py`:
- Line 39: The main function is annotated to return int but currently returns
None on success; update the main() implementation in message_channel_example.py
(function name: main) to return an explicit integer success code (e.g., return
0) at the end of the function so the signature and actual return value match.
In `@src/core/live_trackers/cpp/live_message_channel_tracker_impl.cpp`:
- Around line 92-120: LiveMessageChannelTrackerImpl::send_message must guard
against a null channel_ before calling get_state_fn_ (and before send_fn_); add
a check that channel_ != XR_NULL_HANDLE at the start of send_message (same style
used in query_status()), and if it is XR_NULL_HANDLE return/throw an appropriate
error (consistent with existing behavior) so get_state_fn_ and send_fn_ are
never invoked with a null handle.
In
`@src/core/retargeting_engine/python/deviceio_source_nodes/message_channel_config.py`:
- Around line 41-44: Remove the stringified forward reference on the deque
annotation by changing outbound_queue: deque["MessageChannelMessagesTrackedT"]
to outbound_queue: deque[MessageChannelMessagesTrackedT]; then ensure
MessageChannelMessagesTrackedT is imported at runtime (not only under
TYPE_CHECKING) so the name is available for evaluation, e.g. add a normal import
for MessageChannelMessagesTrackedT alongside other runtime imports and keep
TYPE_CHECKING guards only for other conditional imports.
---
Duplicate comments:
In `@src/core/live_trackers/cpp/live_deviceio_factory.cpp`:
- Around line 214-218: The
LiveDeviceIOFactory::create_message_channel_tracker_impl currently always
constructs LiveMessageChannelTrackerImpl and ignores MCAP recording; change it
to check should_record(tracker) and, when true, call the
create_mcap_channels(...) plumbing and pass the MCAP channel(s) into
LiveMessageChannelTrackerImpl (ensure the new constructor overload in
LiveMessageChannelTrackerImpl that accepts MCAP channels is used), otherwise
construct the non-recording LiveMessageChannelTrackerImpl as before; also add
the corresponding constructor and create_mcap_channels implementation in
live_message_channel_tracker_impl.hpp/.cpp to wire the MCAP channels through.
In
`@src/core/retargeting_engine/python/deviceio_source_nodes/message_channel_sink.py`:
- Around line 35-37: The _compute_fn currently indexes
inputs["messages_tracked"][0] without checking for an empty group; update
_compute_fn to first check that inputs contains a non-empty "messages_tracked"
group (e.g. if inputs.get("messages_tracked") and
len(inputs["messages_tracked"])>0 or simply if inputs["messages_tracked"]: ),
only then assign messages_tracked = inputs["messages_tracked"][0] and append to
self._outbound_queue; otherwise do nothing (no-op) so the node won't raise
IndexError.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: dd80a33e-f5f9-4a67-8f0e-ffa55b4b509a
📒 Files selected for processing (25)
deps/cloudxr/openxr_extensions/XR_NV_opaque_data_channel.hexamples/teleop_session_manager/python/message_channel_example.pysrc/core/deviceio_base/cpp/inc/deviceio_base/message_channel_tracker_base.hppsrc/core/deviceio_trackers/cpp/CMakeLists.txtsrc/core/deviceio_trackers/cpp/inc/deviceio_trackers/message_channel_tracker.hppsrc/core/deviceio_trackers/cpp/message_channel_tracker.cppsrc/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_deviceio_factory.cppsrc/core/live_trackers/cpp/live_message_channel_tracker_impl.cppsrc/core/live_trackers/cpp/live_message_channel_tracker_impl.hppsrc/core/python/deviceio_init.pysrc/core/retargeting_engine/python/deviceio_source_nodes/__init__.pysrc/core/retargeting_engine/python/deviceio_source_nodes/deviceio_tensor_types.pysrc/core/retargeting_engine/python/deviceio_source_nodes/message_channel_config.pysrc/core/retargeting_engine/python/deviceio_source_nodes/message_channel_sink.pysrc/core/retargeting_engine/python/deviceio_source_nodes/message_channel_source.pysrc/core/retargeting_engine_tests/python/test_message_channel_nodes.pysrc/core/schema/fbs/message_channel.fbssrc/core/schema/python/CMakeLists.txtsrc/core/schema/python/message_channel_bindings.hsrc/core/schema/python/schema_init.pysrc/core/schema/python/schema_module.cpp
nvddr
force-pushed
the
devdeepr/messageChannelTracker
branch
2 times, most recently
from
a680247 to
cc601f9
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (4)
src/core/live_trackers/cpp/live_message_channel_tracker_impl.cpp (2)
30-40:⚠️ Potential issue | 🔴 CriticalAvoid dereferencing
trackerbefore validating constructor inputs.Line 33 dereferences
trackerin the initializer list before any runtime check. In release builds, Line 39assertis disabled, so a nulltrackercan crash immediately.Proposed fix
+namespace +{ +std::vector<uint8_t> make_receive_buffer_or_throw(const MessageChannelTracker* tracker) +{ + if (tracker == nullptr) + { + throw std::invalid_argument("LiveMessageChannelTrackerImpl: tracker cannot be null"); + } + return std::vector<uint8_t>(tracker->max_message_size(), 0); +} +} // namespace + LiveMessageChannelTrackerImpl::LiveMessageChannelTrackerImpl(const OpenXRSessionHandles& handles, const MessageChannelTracker* tracker, std::unique_ptr<MessageChannelMcapChannels> mcap_channels) - : handles_(handles), tracker_(tracker), receive_buffer_(tracker->max_message_size(), 0), + : handles_(handles), tracker_(tracker), receive_buffer_(make_receive_buffer_or_throw(tracker)), mcap_channels_(std::move(mcap_channels)) { - assert(handles_.instance != XR_NULL_HANDLE && "OpenXR instance handle cannot be null"); - assert(handles_.session != XR_NULL_HANDLE && "OpenXR session handle cannot be null"); - assert(handles_.xrGetInstanceProcAddr && "xrGetInstanceProcAddr cannot be null"); - assert(tracker_ != nullptr && "MessageChannelTracker cannot be null"); + if (handles_.instance == XR_NULL_HANDLE || handles_.session == XR_NULL_HANDLE || !handles_.xrGetInstanceProcAddr) + { + throw std::invalid_argument("LiveMessageChannelTrackerImpl: invalid OpenXR session handles"); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/live_trackers/cpp/live_message_channel_tracker_impl.cpp` around lines 30 - 40, The constructor currently dereferences tracker in the initializer list (receive_buffer_(tracker->max_message_size(), 0)) before the null checks; change the ctor to avoid that by removing tracker-dependent initialization from the member initializer list and instead initialize receive_buffer_ in the constructor body after the asserts (e.g., default-construct receive_buffer_ in the initializer list and then call receive_buffer_.resize(tracker_->max_message_size()) after verifying tracker_ is non-null). Update LiveMessageChannelTrackerImpl's initializer list and constructor body accordingly, leaving handles_, tracker_, and mcap_channels_ assignments intact.
229-233:⚠️ Potential issue | 🔴 CriticalGuard
channel_before querying OpenXR channel state.
query_status()callsget_state_fn_(channel_, ...)without checking forXR_NULL_HANDLE. After a failed reopen, this can propagate exceptions on subsequentupdate()/get_status()calls.Proposed fix
MessageChannelStatus LiveMessageChannelTrackerImpl::query_status() const { + if (channel_ == XR_NULL_HANDLE) + { + return MessageChannelStatus::DISCONNECTED; + } + XrOpaqueDataChannelStateNV channel_state{ XR_TYPE_OPAQUE_DATA_CHANNEL_STATE_NV }; XrResult state_result = get_state_fn_(channel_, &channel_state);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/live_trackers/cpp/live_message_channel_tracker_impl.cpp` around lines 229 - 233, The query_status() method currently calls get_state_fn_(channel_, ...) without checking channel_; add a guard at the start of LiveMessageChannelTrackerImpl::query_status() to detect XR_NULL_HANDLE (or invalid channel_) and return a safe MessageChannelStatus (e.g., closed/unknown) immediately instead of calling get_state_fn_; ensure any callers like update() / get_status() will get this safe result when channel_ is null so no OpenXR call is made after a failed reopen.src/core/live_trackers/cpp/live_message_channel_tracker_impl.hpp (1)
11-15:⚠️ Potential issue | 🟠 MajorAdd direct
<array>include to prevent include-order build failures.
std::arrayis used at Line 54 but<array>is not included in this header. This can fail depending on transitive includes.Proposed fix
`#include` <XR_NV_opaque_data_channel.h> +#include <array> `#include` <memory> `#include` <string> `#include` <string_view> `#include` <vector>Also applies to: 54-54
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/live_trackers/cpp/live_message_channel_tracker_impl.hpp` around lines 11 - 15, Add a direct include for <array> in the header to ensure std::array is available regardless of include order; locate the header where <XR_NV_opaque_data_channel.h>, <memory>, <string>, <string_view>, and <vector> are included and add the missing `#include` <array> near them so the use of std::array (referenced at the site using std::array in this header) no longer relies on transitive includes.src/core/retargeting_engine/python/deviceio_source_nodes/message_channel_source.py (1)
64-70:⚠️ Potential issue | 🟠 MajorPreserve only the unsent suffix when a flush fails.
If Line 69 succeeds for an early payload and a later
send_message()raises, the whole batch stays at the head of_outbound_queue. When the channel recovers, that already-delivered prefix is sent again. Only drop the batch after a full flush; on the first send failure, keep the failing element onward and stop flushing.Suggested fix
if self._last_status == MessageChannelConnectionStatus.CONNECTED: while self._outbound_queue: batch = self._outbound_queue[0] - if batch.data: - for message in batch.data: + pending = list(batch.data or []) + try: + for index, message in enumerate(pending): outbound_message = MessageChannelMessages(message.payload) self._tracker.send_message(deviceio_session, outbound_message) - self._outbound_queue.popleft() + except RuntimeError: + batch.data = pending[index:] + break + else: + self._outbound_queue.popleft()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/retargeting_engine/python/deviceio_source_nodes/message_channel_source.py` around lines 64 - 70, The current flush loop in message_channel_source.py iterates batch.data and if a later self._tracker.send_message(deviceio_session, outbound_message) raises, the already-sent prefix remains in the head batch and will be resent later; change the flush to track progress within each batch (e.g., iterate by index or consume items) so that on a send failure you slice batch.data to keep only the unsent suffix, replace or update the head of self._outbound_queue with that suffix, and stop flushing (do not popleft the batch); only popleft the batch when the entire batch.data has been successfully sent. Ensure you update references to _outbound_queue, batch.data, MessageChannelMessages and self._tracker.send_message accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/core/live_trackers/cpp/live_message_channel_tracker_impl.cpp`:
- Around line 30-40: The constructor currently dereferences tracker in the
initializer list (receive_buffer_(tracker->max_message_size(), 0)) before the
null checks; change the ctor to avoid that by removing tracker-dependent
initialization from the member initializer list and instead initialize
receive_buffer_ in the constructor body after the asserts (e.g.,
default-construct receive_buffer_ in the initializer list and then call
receive_buffer_.resize(tracker_->max_message_size()) after verifying tracker_ is
non-null). Update LiveMessageChannelTrackerImpl's initializer list and
constructor body accordingly, leaving handles_, tracker_, and mcap_channels_
assignments intact.
- Around line 229-233: The query_status() method currently calls
get_state_fn_(channel_, ...) without checking channel_; add a guard at the start
of LiveMessageChannelTrackerImpl::query_status() to detect XR_NULL_HANDLE (or
invalid channel_) and return a safe MessageChannelStatus (e.g., closed/unknown)
immediately instead of calling get_state_fn_; ensure any callers like update() /
get_status() will get this safe result when channel_ is null so no OpenXR call
is made after a failed reopen.
In `@src/core/live_trackers/cpp/live_message_channel_tracker_impl.hpp`:
- Around line 11-15: Add a direct include for <array> in the header to ensure
std::array is available regardless of include order; locate the header where
<XR_NV_opaque_data_channel.h>, <memory>, <string>, <string_view>, and <vector>
are included and add the missing `#include` <array> near them so the use of
std::array (referenced at the site using std::array in this header) no longer
relies on transitive includes.
In
`@src/core/retargeting_engine/python/deviceio_source_nodes/message_channel_source.py`:
- Around line 64-70: The current flush loop in message_channel_source.py
iterates batch.data and if a later self._tracker.send_message(deviceio_session,
outbound_message) raises, the already-sent prefix remains in the head batch and
will be resent later; change the flush to track progress within each batch
(e.g., iterate by index or consume items) so that on a send failure you slice
batch.data to keep only the unsent suffix, replace or update the head of
self._outbound_queue with that suffix, and stop flushing (do not popleft the
batch); only popleft the batch when the entire batch.data has been successfully
sent. Ensure you update references to _outbound_queue, batch.data,
MessageChannelMessages and self._tracker.send_message accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: dfb1ab64-1c4b-4a5a-9f84-54ca2494e4bc
📒 Files selected for processing (26)
deps/cloudxr/openxr_extensions/XR_NV_opaque_data_channel.hexamples/teleop_session_manager/python/message_channel_example.pysrc/core/deviceio_base/cpp/inc/deviceio_base/message_channel_tracker_base.hppsrc/core/deviceio_trackers/cpp/CMakeLists.txtsrc/core/deviceio_trackers/cpp/inc/deviceio_trackers/message_channel_tracker.hppsrc/core/deviceio_trackers/cpp/message_channel_tracker.cppsrc/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_deviceio_factory.cppsrc/core/live_trackers/cpp/live_message_channel_tracker_impl.cppsrc/core/live_trackers/cpp/live_message_channel_tracker_impl.hppsrc/core/mcap/cpp/inc/mcap/recording_traits.hppsrc/core/python/deviceio_init.pysrc/core/retargeting_engine/python/deviceio_source_nodes/__init__.pysrc/core/retargeting_engine/python/deviceio_source_nodes/deviceio_tensor_types.pysrc/core/retargeting_engine/python/deviceio_source_nodes/message_channel_config.pysrc/core/retargeting_engine/python/deviceio_source_nodes/message_channel_sink.pysrc/core/retargeting_engine/python/deviceio_source_nodes/message_channel_source.pysrc/core/retargeting_engine_tests/python/test_message_channel_nodes.pysrc/core/schema/fbs/message_channel.fbssrc/core/schema/python/CMakeLists.txtsrc/core/schema/python/message_channel_bindings.hsrc/core/schema/python/schema_init.pysrc/core/schema/python/schema_module.cpp
✅ Files skipped from review due to trivial changes (10)
- src/core/schema/python/CMakeLists.txt
- src/core/live_trackers/cpp/CMakeLists.txt
- src/core/deviceio_trackers/cpp/CMakeLists.txt
- src/core/live_trackers/cpp/inc/live_trackers/live_deviceio_factory.hpp
- src/core/schema/fbs/message_channel.fbs
- src/core/retargeting_engine_tests/python/test_message_channel_nodes.py
- src/core/deviceio_base/cpp/inc/deviceio_base/message_channel_tracker_base.hpp
- src/core/deviceio_trackers/python/tracker_bindings.cpp
- deps/cloudxr/openxr_extensions/XR_NV_opaque_data_channel.h
- src/core/deviceio_trackers/cpp/inc/deviceio_trackers/message_channel_tracker.hpp
🚧 Files skipped from review as they are similar to previous changes (9)
- src/core/python/deviceio_init.py
- src/core/schema/python/schema_init.py
- src/core/deviceio_trackers/python/deviceio_trackers_init.py
- src/core/schema/python/schema_module.cpp
- src/core/retargeting_engine/python/deviceio_source_nodes/message_channel_sink.py
- src/core/deviceio_trackers/cpp/message_channel_tracker.cpp
- examples/teleop_session_manager/python/message_channel_example.py
- src/core/live_trackers/cpp/live_deviceio_factory.cpp
- src/core/retargeting_engine/python/deviceio_source_nodes/init.py
nvddr
force-pushed
the
devdeepr/messageChannelTracker
branch
from
cc601f9 to
0632279
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (5)
src/core/retargeting_engine/python/deviceio_source_nodes/message_channel_sink.py (1)
35-37:⚠️ Potential issue | 🟠 MajorUnguarded index access will crash on empty input.
Line 36 assumes
inputs["messages_tracked"]has at least one element. If empty, this raisesIndexErrorand breaks compute.Proposed fix
def _compute_fn(self, inputs: RetargeterIO, outputs: RetargeterIO, context) -> None: - messages_tracked = inputs["messages_tracked"][0] - self._outbound_queue.append(messages_tracked) + group = inputs["messages_tracked"] + if len(group) == 0: + return + self._outbound_queue.append(group[0])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/retargeting_engine/python/deviceio_source_nodes/message_channel_sink.py` around lines 35 - 37, The _compute_fn currently does an unguarded access to inputs["messages_tracked"][0] which will raise IndexError if the list is empty; modify _compute_fn to check that inputs contains "messages_tracked" and that inputs["messages_tracked"] is non-empty before indexing (e.g., if not inputs.get("messages_tracked") or len(inputs["messages_tracked"]) == 0: return or skip), and only append to self._outbound_queue when a message is present; preserve existing behavior when an element exists and avoid changing the return signature.src/core/live_trackers/cpp/live_message_channel_tracker_impl.hpp (1)
12-16:⚠️ Potential issue | 🟡 MinorMissing
<array>include forstd::arrayusage.Line 55 uses
std::array, but<array>isn't included. This can cause include-order-dependent build breaks.Proposed fix
`#include` <XR_NV_opaque_data_channel.h> +#include <array> `#include` <memory>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/live_trackers/cpp/live_message_channel_tracker_impl.hpp` around lines 12 - 16, The header is missing the <array> include required for std::array usage; update src/core/live_trackers/cpp/live_message_channel_tracker_impl.hpp to add `#include` <array> near the other STL includes so the std::array used (around the std::array usage at the code block including line with std::array) is defined and to prevent include-order-dependent build failures.src/core/live_trackers/cpp/live_message_channel_tracker_impl.cpp (2)
235-243:⚠️ Potential issue | 🔴 CriticalNull handle passed to OpenXR after failed reopen.
After
try_reopen_channel()fails (returns false),channel_remainsXR_NULL_HANDLE. The nextupdate()call invokesquery_status()which passes the null handle toget_state_fn_()at line 238, causing UB or crash.Proposed fix
MessageChannelStatus LiveMessageChannelTrackerImpl::query_status() const { + if (channel_ == XR_NULL_HANDLE) + { + return MessageChannelStatus::DISCONNECTED; + } + XrOpaqueDataChannelStateNV channel_state{ XR_TYPE_OPAQUE_DATA_CHANNEL_STATE_NV }; XrResult state_result = get_state_fn_(channel_, &channel_state);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/live_trackers/cpp/live_message_channel_tracker_impl.cpp` around lines 235 - 243, query_status() currently calls get_state_fn_ with channel_ even when try_reopen_channel() failed and channel_ is XR_NULL_HANDLE, leading to UB/crash; update LiveMessageChannelTrackerImpl::query_status to first check if channel_ == XR_NULL_HANDLE (or otherwise invalid) and handle it safely (e.g., return a closed/invalid MessageChannelStatus or attempt reopen there), or ensure try_reopen_channel() only leaves channel_ set on success—use the symbols LiveMessageChannelTrackerImpl::query_status, try_reopen_channel(), channel_, and get_state_fn_ to locate the code and implement the null-handle guard and safe early return instead of calling get_state_fn_ with a null handle.
30-42:⚠️ Potential issue | 🔴 CriticalNull dereference in initializer list before assert validates.
Line 35 dereferences
tracker->max_message_size()before line 42's assert checkstracker_ != nullptr. In release builds (NDEBUGdefined), the assert is no-op, leaving UB on null input.Proposed fix
+namespace +{ +std::vector<uint8_t> make_receive_buffer(const MessageChannelTracker* tracker) +{ + if (tracker == nullptr) + { + throw std::invalid_argument("LiveMessageChannelTrackerImpl: tracker cannot be null"); + } + return std::vector<uint8_t>(tracker->max_message_size(), 0); +} +} // namespace + LiveMessageChannelTrackerImpl::LiveMessageChannelTrackerImpl(const OpenXRSessionHandles& handles, const MessageChannelTracker* tracker, std::unique_ptr<MessageChannelMcapChannels> mcap_channels) : handles_(handles), tracker_(tracker), - receive_buffer_(tracker->max_message_size(), 0), + receive_buffer_(make_receive_buffer(tracker)), time_converter_(handles), mcap_channels_(std::move(mcap_channels))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/live_trackers/cpp/live_message_channel_tracker_impl.cpp` around lines 30 - 42, The constructor currently dereferences tracker in the initializer list (receive_buffer_(tracker->max_message_size(), 0)) before the assert on tracker_ runs, causing UB for null tracker; fix by avoiding dereference in the initializer list: initialize receive_buffer_ to empty (or size 0) in the initializer list, then immediately assert/check tracker != nullptr (or throw) in the constructor body and resize receive_buffer_ using tracker->max_message_size(); update LiveMessageChannelTrackerImpl::LiveMessageChannelTrackerImpl to perform the null check on the incoming tracker parameter and then set receive_buffer_.src/core/retargeting_engine/python/deviceio_source_nodes/message_channel_source.py (1)
62-70:⚠️ Potential issue | 🟠 MajorPartial flush leaves already-sent payloads queued for replay.
If
send_message()throws after delivering some payloads, line 70'spopleft()never executes, leaving the entire batch queued. On reconnect, already-delivered messages get re-sent.Catch the failure, keep only the unsent suffix queued, and stop flushing.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/retargeting_engine/python/deviceio_source_nodes/message_channel_source.py` around lines 62 - 70, The flush loop in the connected branch can leave already-delivered messages in a batch when self._tracker.send_message(...) raises, because popleft() isn't executed; modify the while loop that iterates self._outbound_queue in the CONNECTED branch to wrap per-batch sending in a try/except: on exception, determine the index of the first unsent message in the current batch, replace the head batch in self._outbound_queue with a new batch containing only the unsent suffix (so delivered payloads are removed), stop further flushing (break the loop), and re-raise or log as appropriate; use MessageChannelMessages, deviceio_session and _tracker.send_message references to locate where to catch and adjust the batch before breaking out.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/core/live_trackers/cpp/live_message_channel_tracker_impl.cpp`:
- Around line 235-243: query_status() currently calls get_state_fn_ with
channel_ even when try_reopen_channel() failed and channel_ is XR_NULL_HANDLE,
leading to UB/crash; update LiveMessageChannelTrackerImpl::query_status to first
check if channel_ == XR_NULL_HANDLE (or otherwise invalid) and handle it safely
(e.g., return a closed/invalid MessageChannelStatus or attempt reopen there), or
ensure try_reopen_channel() only leaves channel_ set on success—use the symbols
LiveMessageChannelTrackerImpl::query_status, try_reopen_channel(), channel_, and
get_state_fn_ to locate the code and implement the null-handle guard and safe
early return instead of calling get_state_fn_ with a null handle.
- Around line 30-42: The constructor currently dereferences tracker in the
initializer list (receive_buffer_(tracker->max_message_size(), 0)) before the
assert on tracker_ runs, causing UB for null tracker; fix by avoiding
dereference in the initializer list: initialize receive_buffer_ to empty (or
size 0) in the initializer list, then immediately assert/check tracker !=
nullptr (or throw) in the constructor body and resize receive_buffer_ using
tracker->max_message_size(); update
LiveMessageChannelTrackerImpl::LiveMessageChannelTrackerImpl to perform the null
check on the incoming tracker parameter and then set receive_buffer_.
In `@src/core/live_trackers/cpp/live_message_channel_tracker_impl.hpp`:
- Around line 12-16: The header is missing the <array> include required for
std::array usage; update
src/core/live_trackers/cpp/live_message_channel_tracker_impl.hpp to add `#include`
<array> near the other STL includes so the std::array used (around the
std::array usage at the code block including line with std::array) is defined
and to prevent include-order-dependent build failures.
In
`@src/core/retargeting_engine/python/deviceio_source_nodes/message_channel_sink.py`:
- Around line 35-37: The _compute_fn currently does an unguarded access to
inputs["messages_tracked"][0] which will raise IndexError if the list is empty;
modify _compute_fn to check that inputs contains "messages_tracked" and that
inputs["messages_tracked"] is non-empty before indexing (e.g., if not
inputs.get("messages_tracked") or len(inputs["messages_tracked"]) == 0: return
or skip), and only append to self._outbound_queue when a message is present;
preserve existing behavior when an element exists and avoid changing the return
signature.
In
`@src/core/retargeting_engine/python/deviceio_source_nodes/message_channel_source.py`:
- Around line 62-70: The flush loop in the connected branch can leave
already-delivered messages in a batch when self._tracker.send_message(...)
raises, because popleft() isn't executed; modify the while loop that iterates
self._outbound_queue in the CONNECTED branch to wrap per-batch sending in a
try/except: on exception, determine the index of the first unsent message in the
current batch, replace the head batch in self._outbound_queue with a new batch
containing only the unsent suffix (so delivered payloads are removed), stop
further flushing (break the loop), and re-raise or log as appropriate; use
MessageChannelMessages, deviceio_session and _tracker.send_message references to
locate where to catch and adjust the batch before breaking out.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: c1a3eebe-6482-424b-a9e1-9848a29974dc
📒 Files selected for processing (28)
deps/cloudxr/openxr_extensions/XR_NV_opaque_data_channel.hexamples/teleop_session_manager/python/message_channel_example.pysrc/core/deviceio_base/cpp/inc/deviceio_base/message_channel_tracker_base.hppsrc/core/deviceio_trackers/AGENTS.mdsrc/core/deviceio_trackers/cpp/CMakeLists.txtsrc/core/deviceio_trackers/cpp/inc/deviceio_trackers/message_channel_tracker.hppsrc/core/deviceio_trackers/cpp/message_channel_tracker.cppsrc/core/deviceio_trackers/python/deviceio_trackers_init.pysrc/core/deviceio_trackers/python/tracker_bindings.cppsrc/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_message_channel_tracker_impl.cppsrc/core/live_trackers/cpp/live_message_channel_tracker_impl.hppsrc/core/mcap/cpp/inc/mcap/recording_traits.hppsrc/core/python/deviceio_init.pysrc/core/retargeting_engine/python/deviceio_source_nodes/__init__.pysrc/core/retargeting_engine/python/deviceio_source_nodes/deviceio_tensor_types.pysrc/core/retargeting_engine/python/deviceio_source_nodes/message_channel_config.pysrc/core/retargeting_engine/python/deviceio_source_nodes/message_channel_sink.pysrc/core/retargeting_engine/python/deviceio_source_nodes/message_channel_source.pysrc/core/retargeting_engine_tests/python/test_message_channel_nodes.pysrc/core/schema/fbs/message_channel.fbssrc/core/schema/python/CMakeLists.txtsrc/core/schema/python/message_channel_bindings.hsrc/core/schema/python/schema_init.pysrc/core/schema/python/schema_module.cpp
✅ Files skipped from review due to trivial changes (14)
- src/core/schema/python/CMakeLists.txt
- src/core/deviceio_trackers/AGENTS.md
- src/core/deviceio_trackers/cpp/CMakeLists.txt
- src/core/schema/python/schema_init.py
- src/core/live_trackers/cpp/CMakeLists.txt
- src/core/live_trackers/AGENTS.md
- src/core/schema/fbs/message_channel.fbs
- src/core/deviceio_trackers/python/deviceio_trackers_init.py
- src/core/retargeting_engine/python/deviceio_source_nodes/init.py
- src/core/retargeting_engine_tests/python/test_message_channel_nodes.py
- examples/teleop_session_manager/python/message_channel_example.py
- src/core/deviceio_base/cpp/inc/deviceio_base/message_channel_tracker_base.hpp
- src/core/retargeting_engine/python/deviceio_source_nodes/message_channel_config.py
- deps/cloudxr/openxr_extensions/XR_NV_opaque_data_channel.h
🚧 Files skipped from review as they are similar to previous changes (8)
- src/core/python/deviceio_init.py
- src/core/schema/python/schema_module.cpp
- src/core/live_trackers/cpp/inc/live_trackers/live_deviceio_factory.hpp
- src/core/live_trackers/cpp/live_deviceio_factory.cpp
- src/core/deviceio_trackers/cpp/message_channel_tracker.cpp
- src/core/schema/python/message_channel_bindings.h
- src/core/deviceio_trackers/python/tracker_bindings.cpp
- src/core/deviceio_trackers/cpp/inc/deviceio_trackers/message_channel_tracker.hpp
nvddr
force-pushed
the
devdeepr/messageChannelTracker
branch
from
0632279 to
32c3150
Compare
nvddr
force-pushed
the
devdeepr/messageChannelTracker
branch
from
32c3150 to
6587e81
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/core/retargeting_engine/python/deviceio_source_nodes/message_channel_sink.py (1)
36-38:⚠️ Potential issue | 🟠 MajorGuard
messages_trackedbefore indexing.
inputs["messages_tracked"][0]will raise on an empty group, so a no-data cycle can crash the node instead of being skipped.Suggested fix
def _compute_fn(self, inputs: RetargeterIO, outputs: RetargeterIO, context) -> None: - messages_tracked = inputs["messages_tracked"][0] - self._outbound_queue.append(messages_tracked) + group = inputs["messages_tracked"] + if len(group) == 0: + return + self._outbound_queue.append(group[0])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/retargeting_engine/python/deviceio_source_nodes/message_channel_sink.py` around lines 36 - 38, The code in _compute_fn reads inputs["messages_tracked"][0] unguarded and will raise if the group is empty; modify _compute_fn to first check that inputs contains "messages_tracked" and that inputs["messages_tracked"] is non-empty (or retrieve the first item with a safe default) and return/skip the cycle when there is no data, then append the tracked messages to self._outbound_queue only when the value is present.src/core/live_trackers/cpp/live_message_channel_tracker_impl.cpp (1)
35-40:⚠️ Potential issue | 🟠 MajorDon't rely on
assertfor constructor preconditions.In release builds these checks disappear, and Line 40 will dereference
tracker_even if a null pointer slips through. Validatehandlesandtracker_at runtime before using them.Suggested fix
LiveMessageChannelTrackerImpl::LiveMessageChannelTrackerImpl(const OpenXRSessionHandles& handles, const MessageChannelTracker* tracker, std::unique_ptr<MessageChannelMcapChannels> mcap_channels) : handles_(handles), tracker_(tracker), time_converter_(handles), mcap_channels_(std::move(mcap_channels)) { - assert(handles_.instance != XR_NULL_HANDLE && "OpenXR instance handle cannot be null"); - assert(handles_.session != XR_NULL_HANDLE && "OpenXR session handle cannot be null"); - assert(handles_.xrGetInstanceProcAddr && "xrGetInstanceProcAddr cannot be null"); - assert(tracker_ != nullptr && "MessageChannelTracker cannot be null"); + if (handles_.instance == XR_NULL_HANDLE || handles_.session == XR_NULL_HANDLE || + handles_.xrGetInstanceProcAddr == nullptr) + { + throw std::invalid_argument("LiveMessageChannelTrackerImpl: invalid OpenXR session handles"); + } + if (tracker_ == nullptr) + { + throw std::invalid_argument("LiveMessageChannelTrackerImpl: tracker cannot be null"); + } receive_buffer_.resize(tracker_->max_message_size(), 0);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/live_trackers/cpp/live_message_channel_tracker_impl.cpp` around lines 35 - 40, Replace the assert-based precondition checks with runtime validation and fail-fast behavior: in the LiveMessageChannelTrackerImpl constructor (or init function) check that handles_.instance != XR_NULL_HANDLE, handles_.session != XR_NULL_HANDLE, handles_.xrGetInstanceProcAddr != nullptr, and tracker_ != nullptr; if any check fails, throw a descriptive std::invalid_argument or std::runtime_error (or return an error) instead of asserting. Only call tracker_->max_message_size() and resize receive_buffer_ after these validations succeed so you never dereference a null tracker_. Ensure the thrown error message mentions which symbol failed (e.g., "handles_.instance is XR_NULL_HANDLE", "tracker_ is null") to aid debugging.
🤖 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_message_channel_tracker_impl.cpp`:
- Around line 94-100: The code in LiveMessageChannelTrackerImpl that handles
oversized incoming messages currently constructs std::vector<uint8_t>
discard(count_out), allowing an untrusted remote length (count_out) to cause an
unbounded allocation; change the drain logic to read and discard the payload in
bounded chunks instead: allocate a fixed-size buffer no larger than
tracker_->max_message_size() (or a safe constant chunk like 64KB), then loop
calling receive_fn_(channel_, remaining, &read, buffer.data()) and subtract read
until the entire count_out is consumed, avoiding any allocation sized by
count_out. Ensure you reference the receive/drain logic in
LiveMessageChannelTrackerImpl and replace the vector(count_out) allocation with
the safe chunked-read loop.
---
Duplicate comments:
In `@src/core/live_trackers/cpp/live_message_channel_tracker_impl.cpp`:
- Around line 35-40: Replace the assert-based precondition checks with runtime
validation and fail-fast behavior: in the LiveMessageChannelTrackerImpl
constructor (or init function) check that handles_.instance != XR_NULL_HANDLE,
handles_.session != XR_NULL_HANDLE, handles_.xrGetInstanceProcAddr != nullptr,
and tracker_ != nullptr; if any check fails, throw a descriptive
std::invalid_argument or std::runtime_error (or return an error) instead of
asserting. Only call tracker_->max_message_size() and resize receive_buffer_
after these validations succeed so you never dereference a null tracker_. Ensure
the thrown error message mentions which symbol failed (e.g., "handles_.instance
is XR_NULL_HANDLE", "tracker_ is null") to aid debugging.
In
`@src/core/retargeting_engine/python/deviceio_source_nodes/message_channel_sink.py`:
- Around line 36-38: The code in _compute_fn reads inputs["messages_tracked"][0]
unguarded and will raise if the group is empty; modify _compute_fn to first
check that inputs contains "messages_tracked" and that
inputs["messages_tracked"] is non-empty (or retrieve the first item with a safe
default) and return/skip the cycle when there is no data, then append the
tracked messages to self._outbound_queue only when the value is present.
🪄 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: Pro Plus
Run ID: 09c52d4b-c9d3-46b8-946b-ae4b39a359fd
📒 Files selected for processing (28)
deps/cloudxr/openxr_extensions/XR_NV_opaque_data_channel.hexamples/teleop_session_manager/python/message_channel_example.pysrc/core/deviceio_base/cpp/inc/deviceio_base/message_channel_tracker_base.hppsrc/core/deviceio_trackers/AGENTS.mdsrc/core/deviceio_trackers/cpp/CMakeLists.txtsrc/core/deviceio_trackers/cpp/inc/deviceio_trackers/message_channel_tracker.hppsrc/core/deviceio_trackers/cpp/message_channel_tracker.cppsrc/core/deviceio_trackers/python/deviceio_trackers_init.pysrc/core/deviceio_trackers/python/tracker_bindings.cppsrc/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_message_channel_tracker_impl.cppsrc/core/live_trackers/cpp/live_message_channel_tracker_impl.hppsrc/core/mcap/cpp/inc/mcap/recording_traits.hppsrc/core/python/deviceio_init.pysrc/core/retargeting_engine/python/deviceio_source_nodes/__init__.pysrc/core/retargeting_engine/python/deviceio_source_nodes/deviceio_tensor_types.pysrc/core/retargeting_engine/python/deviceio_source_nodes/message_channel_config.pysrc/core/retargeting_engine/python/deviceio_source_nodes/message_channel_sink.pysrc/core/retargeting_engine/python/deviceio_source_nodes/message_channel_source.pysrc/core/retargeting_engine_tests/python/test_message_channel_nodes.pysrc/core/schema/fbs/message_channel.fbssrc/core/schema/python/CMakeLists.txtsrc/core/schema/python/message_channel_bindings.hsrc/core/schema/python/schema_init.pysrc/core/schema/python/schema_module.cpp
✅ Files skipped from review due to trivial changes (16)
- src/core/schema/python/CMakeLists.txt
- src/core/deviceio_trackers/AGENTS.md
- src/core/deviceio_trackers/python/deviceio_trackers_init.py
- src/core/deviceio_trackers/cpp/CMakeLists.txt
- src/core/mcap/cpp/inc/mcap/recording_traits.hpp
- src/core/python/deviceio_init.py
- src/core/live_trackers/cpp/CMakeLists.txt
- src/core/live_trackers/AGENTS.md
- src/core/schema/python/schema_module.cpp
- src/core/schema/fbs/message_channel.fbs
- src/core/deviceio_base/cpp/inc/deviceio_base/message_channel_tracker_base.hpp
- src/core/retargeting_engine_tests/python/test_message_channel_nodes.py
- src/core/retargeting_engine/python/deviceio_source_nodes/message_channel_config.py
- examples/teleop_session_manager/python/message_channel_example.py
- deps/cloudxr/openxr_extensions/XR_NV_opaque_data_channel.h
- src/core/schema/python/schema_init.py
🚧 Files skipped from review as they are similar to previous changes (7)
- src/core/live_trackers/cpp/inc/live_trackers/live_deviceio_factory.hpp
- src/core/deviceio_trackers/cpp/message_channel_tracker.cpp
- src/core/deviceio_trackers/python/tracker_bindings.cpp
- src/core/schema/python/message_channel_bindings.h
- src/core/retargeting_engine/python/deviceio_source_nodes/deviceio_tensor_types.py
- src/core/retargeting_engine/python/deviceio_source_nodes/init.py
- src/core/live_trackers/cpp/live_message_channel_tracker_impl.hpp
nvddr
force-pushed
the
devdeepr/messageChannelTracker
branch
from
6587e81 to
b908766
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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_message_channel_tracker_impl.cpp`:
- Around line 98-117: The drain loop for oversized messages can loop forever if
receive_fn_ (xrReceiveOpaqueDataChannelNV) fails on the first chunk; change the
inner-loop so when drain_result != XR_SUCCESS or drained == 0 on the first
iteration you treat that as an unrecoverable drain failure: log the failure
(include count_out and drain_result), set remaining = 0 (or otherwise mark the
message as discarded) and break so the outer loop will not re-query the same
oversized message; ensure this logic references receive_fn_, receive_buffer_,
count_out, remaining and drained so the oversized-message path always advances
even if chunked drain fails.
🪄 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: Pro Plus
Run ID: 02909142-e67a-4bf5-88dc-500edf938ee7
📒 Files selected for processing (28)
deps/cloudxr/openxr_extensions/XR_NV_opaque_data_channel.hexamples/teleop_session_manager/python/message_channel_example.pysrc/core/deviceio_base/cpp/inc/deviceio_base/message_channel_tracker_base.hppsrc/core/deviceio_trackers/AGENTS.mdsrc/core/deviceio_trackers/cpp/CMakeLists.txtsrc/core/deviceio_trackers/cpp/inc/deviceio_trackers/message_channel_tracker.hppsrc/core/deviceio_trackers/cpp/message_channel_tracker.cppsrc/core/deviceio_trackers/python/deviceio_trackers_init.pysrc/core/deviceio_trackers/python/tracker_bindings.cppsrc/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_message_channel_tracker_impl.cppsrc/core/live_trackers/cpp/live_message_channel_tracker_impl.hppsrc/core/mcap/cpp/inc/mcap/recording_traits.hppsrc/core/python/deviceio_init.pysrc/core/retargeting_engine/python/deviceio_source_nodes/__init__.pysrc/core/retargeting_engine/python/deviceio_source_nodes/deviceio_tensor_types.pysrc/core/retargeting_engine/python/deviceio_source_nodes/message_channel_config.pysrc/core/retargeting_engine/python/deviceio_source_nodes/message_channel_sink.pysrc/core/retargeting_engine/python/deviceio_source_nodes/message_channel_source.pysrc/core/retargeting_engine_tests/python/test_message_channel_nodes.pysrc/core/schema/fbs/message_channel.fbssrc/core/schema/python/CMakeLists.txtsrc/core/schema/python/message_channel_bindings.hsrc/core/schema/python/schema_init.pysrc/core/schema/python/schema_module.cpp
✅ Files skipped from review due to trivial changes (16)
- src/core/deviceio_trackers/AGENTS.md
- src/core/deviceio_trackers/cpp/CMakeLists.txt
- src/core/schema/python/CMakeLists.txt
- src/core/live_trackers/cpp/CMakeLists.txt
- src/core/schema/python/schema_init.py
- src/core/mcap/cpp/inc/mcap/recording_traits.hpp
- src/core/python/deviceio_init.py
- src/core/deviceio_trackers/python/deviceio_trackers_init.py
- src/core/live_trackers/cpp/live_deviceio_factory.cpp
- src/core/live_trackers/AGENTS.md
- src/core/schema/fbs/message_channel.fbs
- src/core/retargeting_engine/python/deviceio_source_nodes/init.py
- src/core/retargeting_engine_tests/python/test_message_channel_nodes.py
- src/core/deviceio_base/cpp/inc/deviceio_base/message_channel_tracker_base.hpp
- src/core/retargeting_engine/python/deviceio_source_nodes/message_channel_config.py
- deps/cloudxr/openxr_extensions/XR_NV_opaque_data_channel.h
🚧 Files skipped from review as they are similar to previous changes (6)
- src/core/schema/python/schema_module.cpp
- src/core/live_trackers/cpp/inc/live_trackers/live_deviceio_factory.hpp
- src/core/deviceio_trackers/cpp/message_channel_tracker.cpp
- src/core/retargeting_engine/python/deviceio_source_nodes/message_channel_sink.py
- src/core/deviceio_trackers/python/tracker_bindings.cpp
- examples/teleop_session_manager/python/message_channel_example.py
nvddr
force-pushed
the
devdeepr/messageChannelTracker
branch
from
b908766 to
c0fe66e
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/core/retargeting_engine/python/deviceio_source_nodes/message_channel_source.py (1)
70-74:⚠️ Potential issue | 🔴 CriticalBlocked by binding fix:
MessageChannelMessagesvsMessageChannelMessagesT.Line 71 constructs
MessageChannelMessages(message.payload)but the pybind binding attracker_bindings.cpp:105expectsMessageChannelMessagesT. This will raiseTypeErrorat runtime. Once the binding is fixed to accept raw bytes, this code should passmessage.payloaddirectly instead of wrapping it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/retargeting_engine/python/deviceio_source_nodes/message_channel_source.py` around lines 70 - 74, The current code constructs MessageChannelMessages(message.payload) which conflicts with the pybind binding (MessageChannelMessagesT) and will throw a TypeError; after the binding is fixed to accept raw bytes, change the loop in message_channel_source.py so you no longer wrap payloads—pass message.payload directly into self._tracker.send_message (i.e., stop creating MessageChannelMessages and pass the raw bytes as the outbound payload to send_message using the existing deviceio_session parameter).src/core/deviceio_trackers/python/tracker_bindings.cpp (1)
103-106:⚠️ Potential issue | 🔴 CriticalType mismatch between binding and Python call site.
The binding expects
MessageChannelMessagesT(line 105), butmessage_channel_source.py:71constructsMessageChannelMessages(non-T version) and passes it tosend_message. These are distinct FlatBuffer types -MessageChannelMessagesTis the object-API table whileMessageChannelMessagesis a different generated type.The binding should accept raw bytes directly since
message.payloadis what gets used:Suggested fix
.def( "send_message", - [](const core::MessageChannelTracker& self, const core::ITrackerSession& session, - const core::MessageChannelMessagesT& message) { self.send_message(session, message.payload); }, - py::arg("session"), py::arg("message"), "Send a MessageChannelMessages payload over the message channel"); + [](const core::MessageChannelTracker& self, const core::ITrackerSession& session, py::bytes payload) + { + const std::string data = payload; + self.send_message(session, std::vector<uint8_t>(data.begin(), data.end())); + }, + py::arg("session"), py::arg("payload"), "Send raw payload bytes over the message channel");Then update
message_channel_source.pyto passmessage.payloaddirectly instead of wrapping it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/deviceio_trackers/python/tracker_bindings.cpp` around lines 103 - 106, The binding for core::MessageChannelTracker::send_message currently takes MessageChannelMessagesT (object API) which mismatches the Python call that constructs MessageChannelMessages and then uses message.payload; change the binding lambda to accept raw bytes (e.g., py::bytes or std::string_view) instead of MessageChannelMessagesT and call self.send_message(session, message_payload) with that bytes payload; also update the Python caller in message_channel_source.py to pass message.payload directly rather than the whole MessageChannelMessages object.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/core/deviceio_trackers/python/tracker_bindings.cpp`:
- Around line 103-106: The binding for core::MessageChannelTracker::send_message
currently takes MessageChannelMessagesT (object API) which mismatches the Python
call that constructs MessageChannelMessages and then uses message.payload;
change the binding lambda to accept raw bytes (e.g., py::bytes or
std::string_view) instead of MessageChannelMessagesT and call
self.send_message(session, message_payload) with that bytes payload; also update
the Python caller in message_channel_source.py to pass message.payload directly
rather than the whole MessageChannelMessages object.
In
`@src/core/retargeting_engine/python/deviceio_source_nodes/message_channel_source.py`:
- Around line 70-74: The current code constructs
MessageChannelMessages(message.payload) which conflicts with the pybind binding
(MessageChannelMessagesT) and will throw a TypeError; after the binding is fixed
to accept raw bytes, change the loop in message_channel_source.py so you no
longer wrap payloads—pass message.payload directly into
self._tracker.send_message (i.e., stop creating MessageChannelMessages and pass
the raw bytes as the outbound payload to send_message using the existing
deviceio_session parameter).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 3e33a13b-ff6b-4808-b466-62c131c57f31
📒 Files selected for processing (28)
deps/cloudxr/openxr_extensions/XR_NV_opaque_data_channel.hexamples/teleop_session_manager/python/message_channel_example.pysrc/core/deviceio_base/cpp/inc/deviceio_base/message_channel_tracker_base.hppsrc/core/deviceio_trackers/AGENTS.mdsrc/core/deviceio_trackers/cpp/CMakeLists.txtsrc/core/deviceio_trackers/cpp/inc/deviceio_trackers/message_channel_tracker.hppsrc/core/deviceio_trackers/cpp/message_channel_tracker.cppsrc/core/deviceio_trackers/python/deviceio_trackers_init.pysrc/core/deviceio_trackers/python/tracker_bindings.cppsrc/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_message_channel_tracker_impl.cppsrc/core/live_trackers/cpp/live_message_channel_tracker_impl.hppsrc/core/mcap/cpp/inc/mcap/recording_traits.hppsrc/core/python/deviceio_init.pysrc/core/retargeting_engine/python/deviceio_source_nodes/__init__.pysrc/core/retargeting_engine/python/deviceio_source_nodes/deviceio_tensor_types.pysrc/core/retargeting_engine/python/deviceio_source_nodes/message_channel_config.pysrc/core/retargeting_engine/python/deviceio_source_nodes/message_channel_sink.pysrc/core/retargeting_engine/python/deviceio_source_nodes/message_channel_source.pysrc/core/retargeting_engine_tests/python/test_message_channel_nodes.pysrc/core/schema/fbs/message_channel.fbssrc/core/schema/python/CMakeLists.txtsrc/core/schema/python/message_channel_bindings.hsrc/core/schema/python/schema_init.pysrc/core/schema/python/schema_module.cpp
✅ Files skipped from review due to trivial changes (16)
- src/core/deviceio_trackers/cpp/CMakeLists.txt
- src/core/schema/python/CMakeLists.txt
- src/core/deviceio_trackers/python/deviceio_trackers_init.py
- src/core/live_trackers/AGENTS.md
- src/core/live_trackers/cpp/CMakeLists.txt
- src/core/deviceio_trackers/AGENTS.md
- src/core/python/deviceio_init.py
- src/core/schema/python/schema_init.py
- src/core/retargeting_engine/python/deviceio_source_nodes/init.py
- src/core/schema/fbs/message_channel.fbs
- examples/teleop_session_manager/python/message_channel_example.py
- src/core/deviceio_base/cpp/inc/deviceio_base/message_channel_tracker_base.hpp
- src/core/retargeting_engine_tests/python/test_message_channel_nodes.py
- src/core/retargeting_engine/python/deviceio_source_nodes/message_channel_config.py
- src/core/deviceio_trackers/cpp/inc/deviceio_trackers/message_channel_tracker.hpp
- deps/cloudxr/openxr_extensions/XR_NV_opaque_data_channel.h
🚧 Files skipped from review as they are similar to previous changes (4)
- src/core/schema/python/schema_module.cpp
- src/core/live_trackers/cpp/inc/live_trackers/live_deviceio_factory.hpp
- src/core/retargeting_engine/python/deviceio_source_nodes/message_channel_sink.py
- src/core/schema/python/message_channel_bindings.h
rwiltz
approved these changes
Apr 15, 2026
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.
Adds a tracker to read and write message channel messages to/from the remote clients that support it.
Summary by CodeRabbit
New Features
Documentation
Tests