Conversation
📝 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 docstrings
🧪 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.
| const MessageChannelStatus status = query_status(); | ||
| if (status == MessageChannelStatus::DISCONNECTED) | ||
| { | ||
| // Runtime/client disconnected: rebuild the channel object so it can reconnect. | ||
| try_reopen_channel(); | ||
| return false; | ||
| } | ||
|
|
||
| if (status != MessageChannelStatus::CONNECTED) | ||
| { | ||
| return false; | ||
| } | ||
|
|
||
| bool any_received = false; | ||
| while (true) | ||
| { | ||
| uint32_t count_out = 0; | ||
| XrResult recv_result = | ||
| receive_fn_(channel_, static_cast<uint32_t>(receive_buffer_.size()), &count_out, receive_buffer_.data()); | ||
| if (recv_result != XR_SUCCESS) | ||
| { | ||
| if (recv_result == XR_ERROR_CHANNEL_NOT_CONNECTED_NV) | ||
| { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
LiveMessageChannelTrackerImpl::update() returns false for normal, expected states (CONNECTING, SHUTTING, DISCONNECTED, and XR_ERROR_CHANNEL_NOT_CONNECTED_NV). DeviceIOSession treats update()==false as a failure and will emit recurring warnings, so this will spam logs whenever the channel isn’t connected. Consider returning true for non-error/non-data cases (while leaving messages_.data null) and reserve false for actual failures (e.g., unexpected XrResult).
| bool LiveMessageChannelTrackerImpl::try_reopen_channel() | ||
| { | ||
| try | ||
| { | ||
| destroy_channel(); | ||
| create_channel(); | ||
| return true; | ||
| } | ||
| catch (const std::exception& e) | ||
| { | ||
| std::cerr << "[LiveMessageChannelTrackerImpl] Failed to reopen message channel: " << e.what() << std::endl; | ||
| return false; | ||
| } | ||
| } | ||
|
|
||
| MessageChannelStatus LiveMessageChannelTrackerImpl::query_status() const | ||
| { | ||
| XrOpaqueDataChannelStateNV channel_state{ XR_TYPE_OPAQUE_DATA_CHANNEL_STATE_NV }; | ||
| XrResult state_result = get_state_fn_(channel_, &channel_state); | ||
| if (state_result != XR_SUCCESS) | ||
| { | ||
| throw std::runtime_error("LiveMessageChannelTrackerImpl: xrGetOpaqueDataChannelStateNV failed, result=" + | ||
| std::to_string(state_result)); | ||
| } |
There was a problem hiding this comment.
If try_reopen_channel() fails, destroy_channel() has already set channel_ to XR_NULL_HANDLE. The next update() will call query_status(), which unconditionally calls xrGetOpaqueDataChannelStateNV with channel_ and will likely throw/terminate on an invalid handle. Guard query_status()/update() against channel_ == XR_NULL_HANDLE (treat as DISCONNECTED/UNKNOWN and retry create), and/or adjust try_reopen_channel() to avoid leaving the instance in a permanently broken state after a failed reopen.
| for message in batch.data: | ||
| self._tracker.send_message(deviceio_session, message.payload) |
There was a problem hiding this comment.
MessageChannelTracker.send_message() Python binding takes a MessageChannelMessages object (see tracker_bindings.cpp), but this code passes message.payload (bytes). That will raise a TypeError at runtime and prevents queued outbound messages from being sent. Pass the MessageChannelMessages object to send_message, or update the pybind signature to accept raw bytes and adjust callers/tests accordingly.
| for message in batch.data: | |
| self._tracker.send_message(deviceio_session, message.payload) | |
| # send_message expects a MessageChannelMessages-style object, not raw bytes | |
| self._tracker.send_message(deviceio_session, batch) |
| obj->data = data; | ||
| return obj; | ||
| }), | ||
| py::arg("data")) | ||
| .def_property_readonly( | ||
| "data", | ||
| [](const MessageChannelMessagesTrackedT& self) -> std::vector<std::shared_ptr<MessageChannelMessagesT>> | ||
| { return self.data; }); |
There was a problem hiding this comment.
MessageChannelMessagesTrackedT::data is treated as nullable elsewhere (e.g., LiveMessageChannelTrackerImpl uses messages_.data.reset() and checks !messages_.data), but this binding assigns obj->data = data and exposes data as a std::vector return type. This either won’t compile (shared_ptr vs vector mismatch) or will lose the intended None/null semantics in Python. Update the constructor and property to match the underlying field type and preserve None when no messages were drained (e.g., store a shared_ptr<vector<...>> and have the getter return None when null).
| obj->data = data; | |
| return obj; | |
| }), | |
| py::arg("data")) | |
| .def_property_readonly( | |
| "data", | |
| [](const MessageChannelMessagesTrackedT& self) -> std::vector<std::shared_ptr<MessageChannelMessagesT>> | |
| { return self.data; }); | |
| obj->data = std::make_shared<std::vector<std::shared_ptr<MessageChannelMessagesT>>>(data); | |
| return obj; | |
| }), | |
| py::arg("data")) | |
| .def_property_readonly( | |
| "data", | |
| [](const MessageChannelMessagesTrackedT& self) -> py::object | |
| { | |
| if (!self.data) | |
| { | |
| return py::none(); | |
| } | |
| return py::cast(*self.data); | |
| }); |
| void LiveMessageChannelTrackerImpl::initialize_functions() | ||
| { | ||
| loadExtensionFunction(handles_.instance, handles_.xrGetInstanceProcAddr, "xrCreateOpaqueDataChannelNV", | ||
| reinterpret_cast<PFN_xrVoidFunction*>(&create_channel_fn_)); | ||
| loadExtensionFunction(handles_.instance, handles_.xrGetInstanceProcAddr, "xrDestroyOpaqueDataChannelNV", | ||
| reinterpret_cast<PFN_xrVoidFunction*>(&destroy_channel_fn_)); | ||
| loadExtensionFunction(handles_.instance, handles_.xrGetInstanceProcAddr, "xrGetOpaqueDataChannelStateNV", | ||
| reinterpret_cast<PFN_xrVoidFunction*>(&get_state_fn_)); | ||
| loadExtensionFunction(handles_.instance, handles_.xrGetInstanceProcAddr, "xrSendOpaqueDataChannelNV", | ||
| reinterpret_cast<PFN_xrVoidFunction*>(&send_fn_)); | ||
| loadExtensionFunction(handles_.instance, handles_.xrGetInstanceProcAddr, "xrReceiveOpaqueDataChannelNV", | ||
| reinterpret_cast<PFN_xrVoidFunction*>(&receive_fn_)); | ||
| loadExtensionFunction(handles_.instance, handles_.xrGetInstanceProcAddr, "xrShutdownOpaqueDataChannelNV", | ||
| reinterpret_cast<PFN_xrVoidFunction*>(&shutdown_fn_)); | ||
| loadExtensionFunction(handles_.instance, handles_.xrGetInstanceProcAddr, "xrGetSystem", | ||
| reinterpret_cast<PFN_xrVoidFunction*>(&get_system_fn_)); | ||
| } |
There was a problem hiding this comment.
loadExtensionFunction() only checks the XrResult and does not guarantee the output function pointer is non-null. This code should explicitly validate that all required PFN_* pointers were populated after loading (and throw a clear error if any are missing) before calling them later.
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
| @@ -0,0 +1,124 @@ | |||
| #!/usr/bin/env python3 | |||
There was a problem hiding this comment.
Set executable bit for shebang script.
Line [1] uses a shebang, but CI already reports this file is not executable. Please commit mode change to unblock pre-commit.
✅ Fix command
chmod +x examples/teleop_session_manager/python/message_channel_example.py
git add --chmod=+x examples/teleop_session_manager/python/message_channel_example.py🧰 Tools
🪛 GitHub Actions: Run linters using pre-commit
[error] 1-1: pre-commit hook 'check-shebang-scripts-are-executable' failed (exit code 1): file has a shebang but is not marked executable. Suggested fix: 'chmod +x examples/teleop_session_manager/python/message_channel_example.py' (or adjust executable bit in git).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@examples/teleop_session_manager/python/message_channel_example.py` at 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.
| .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"); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify that the binding and its Python call sites disagree on the second argument type.
rg -n -C2 'send_message\(' \
src/core/deviceio_trackers/python/tracker_bindings.cpp \
src/core/retargeting_engine/python/deviceio_source_nodes/message_channel_source.py \
src/core/retargeting_engine_tests/python/test_message_channel_nodes.pyRepository: NVIDIA/IsaacTeleop
Length of output: 1894
Fix the pybind binding to accept raw payload bytes, not a message object.
The binding at line 106 expects MessageChannelMessagesT, but Python callers at src/core/retargeting_engine/python/deviceio_source_nodes/message_channel_source.py:68 pass message.payload (raw bytes). The fake tracker at src/core/retargeting_engine_tests/python/test_message_channel_nodes.py:27 correctly models this interface. The binding will raise TypeError at runtime when invoked with bytes instead of a message object.
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");🤖 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 -
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.
| bool LiveMessageChannelTrackerImpl::try_reopen_channel() | ||
| { | ||
| try | ||
| { | ||
| destroy_channel(); | ||
| create_channel(); | ||
| return true; | ||
| } | ||
| catch (const std::exception& e) | ||
| { | ||
| std::cerr << "[LiveMessageChannelTrackerImpl] Failed to reopen message channel: " << e.what() << std::endl; | ||
| return false; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the reopen path can leave `channel_` null before the next `query_status()` call.
rg -n -C3 'update\(|try_reopen_channel|destroy_channel\(|create_channel\(|query_status\(' \
src/core/live_trackers/cpp/live_message_channel_tracker_impl.cppRepository: NVIDIA/IsaacTeleop
Length of output: 1851
🏁 Script executed:
#!/bin/bash
# Get full create_channel and query_status implementations
cat -n src/core/live_trackers/cpp/live_message_channel_tracker_impl.cpp | sed -n '162,207p'Repository: NVIDIA/IsaacTeleop
Length of output: 1563
🏁 Script executed:
#!/bin/bash
# Get full query_status implementation and surrounding context
cat -n src/core/live_trackers/cpp/live_message_channel_tracker_impl.cpp | sed -n '209,230p'Repository: NVIDIA/IsaacTeleop
Length of output: 1180
🏁 Script executed:
#!/bin/bash
# Check for any null handle guards elsewhere in the class
rg -n 'XR_NULL_HANDLE' src/core/live_trackers/cpp/live_message_channel_tracker_impl.cppRepository: NVIDIA/IsaacTeleop
Length of output: 324
🏁 Script executed:
#!/bin/bash
# Check the get_status() method and find its callers
rg -n 'get_status' src/core/live_trackers/cpp/live_message_channel_tracker_impl.cppRepository: NVIDIA/IsaacTeleop
Length of output: 135
🏁 Script executed:
#!/bin/bash
# Check the header file to see public interface
fd -e h 'live_message_channel_tracker' src/core/live_trackers/cpp/Repository: NVIDIA/IsaacTeleop
Length of output: 44
🏁 Script executed:
#!/bin/bash
# Find the header file
fd -e hpp 'live_message_channel_tracker' src/Repository: NVIDIA/IsaacTeleop
Length of output: 126
🏁 Script executed:
#!/bin/bash
# Check how update() is called and what exception handling wraps it
rg -n 'update\(' src/core/live_trackers/cpp/live_message_channel_tracker_impl.cpp -A2 -B2Repository: NVIDIA/IsaacTeleop
Length of output: 169
🏁 Script executed:
#!/bin/bash
# Search for callers of update() in the entire codebase
rg -n '\.update\(' src/ --type cpp --type hpp | head -20Repository: NVIDIA/IsaacTeleop
Length of output: 89
🏁 Script executed:
#!/bin/bash
# Find callers of update() in cpp and header files
rg -n '\.update\(' src/ -t cpp | head -20Repository: NVIDIA/IsaacTeleop
Length of output: 367
🏁 Script executed:
#!/bin/bash
# Check the header file content
cat -n src/core/live_trackers/cpp/live_message_channel_tracker_impl.hppRepository: NVIDIA/IsaacTeleop
Length of output: 2943
🏁 Script executed:
#!/bin/bash
# Also check if update() is called within try-catch blocks
rg -n -B5 '\.update\(' src/core/live_trackers/cpp/live_message_channel_tracker_impl.cppRepository: NVIDIA/IsaacTeleop
Length of output: 44
🏁 Script executed:
#!/bin/bash
# Check how the update() method handles exceptions from query_status()
cat -n src/core/live_trackers/cpp/live_message_channel_tracker_impl.cpp | sed -n '39,60p'Repository: NVIDIA/IsaacTeleop
Length of output: 772
🏁 Script executed:
#!/bin/bash
# Check the parent class/interface definition for update()
cat -n src/core/live_trackers/cpp/live_message_channel_tracker_impl.hpp | grep -A5 "update"Repository: NVIDIA/IsaacTeleop
Length of output: 354
🏁 Script executed:
#!/bin/bash
# Search for IMessageChannelTrackerImpl to understand the interface contract
fd 'message_channel_tracker.hpp' src/Repository: NVIDIA/IsaacTeleop
Length of output: 142
🏁 Script executed:
#!/bin/bash
# Check the interface definition
cat -n src/core/deviceio_trackers/cpp/inc/deviceio_trackers/message_channel_tracker.hpp | head -100Repository: NVIDIA/IsaacTeleop
Length of output: 2204
🏁 Script executed:
#!/bin/bash
# Check the callers of update() for exception handling context
for file in src/plugins/oak/main.cpp src/plugins/generic_3axis_pedal/main.cpp src/plugins/manus/app/main.cpp; do
echo "=== $file ==="
rg -n -B10 -A5 '\.update\(' "$file"
doneRepository: NVIDIA/IsaacTeleop
Length of output: 1909
🏁 Script executed:
#!/bin/bash
# Verify: check if update() has any try-catch wrapper in these calling contexts
# Also check the interface to see if update() is documented to throw
rg -n 'bool update' src/core/live_trackers/cpp/live_message_channel_tracker_impl.hpp -B3 -A3Repository: NVIDIA/IsaacTeleop
Length of output: 489
🏁 Script executed:
#!/bin/bash
# Check the entire update() implementation to see if there's any exception handling
cat -n src/core/live_trackers/cpp/live_message_channel_tracker_impl.cpp | sed -n '39,90p'Repository: NVIDIA/IsaacTeleop
Length of output: 2031
A failed reopen leaves channel_ null and causes an uncaught exception on the next update.
When try_reopen_channel() fails (line 198–199), destroy_channel() nullifies the handle before create_channel() throws. The exception is caught and the method returns false, but channel_ remains XR_NULL_HANDLE. On the next update() cycle, line 43 calls query_status() without exception handling, which then calls the OpenXR state function at line 212 with a null handle. This triggers an exception at lines 215–216 that propagates uncaught from update(), crashing callers that expect a boolean return (see src/plugins/*/main.cpp loops).
Add a null guard before the OpenXR call:
Null guard in query_status()
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);Also applies to: 209–216
| Queues raw message payloads for delivery by MessageChannelSource.poll_tracker(). | ||
| """ | ||
|
|
||
| from typing import TYPE_CHECKING, Deque |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🌐 Web query:
Is typing.Dequedeprecated in modern Python typing, and iscollections.deque[...] the recommended replacement?
💡 Result:
Yes. typing.Deque is a deprecated alias (deprecated since Python 3.9, per PEP 585’s move to “built-in/stdlib generics”). (docs.python.org)
For modern Python type annotations, the recommended replacement is to use the actual runtime class as a generic:
collections.deque[T](e.g.,collections.deque[int]orfrom collections import deque; deque[int]). (docs.python.org)
Citations:
- 1: https://docs.python.org/3/library/typing.html?utm_source=openai
- 2: https://docs.python.org/3/library/typing.html?utm_source=openai
🏁 Script executed:
cat -n src/core/retargeting_engine/python/deviceio_source_nodes/message_channel_sink.py | head -40Repository: NVIDIA/IsaacTeleop
Length of output: 1597
Use modern deque typing to align with Python 3.9+ standards.
typing.Deque is deprecated since Python 3.9 (PEP 585). Replace with collections.deque for type annotations.
♻️ Proposed update
-from typing import TYPE_CHECKING, Deque
+from collections import deque
+from typing import TYPE_CHECKINGThen update the annotation on lines 24-25:
- def __init__(
- self, name: str, outbound_queue: "Deque[MessageChannelMessagesTrackedT]"
- ) -> None:
+ def __init__(
+ self, name: str, outbound_queue: "deque[MessageChannelMessagesTrackedT]"
+ ) -> None:🧰 Tools
🪛 Ruff (0.15.7)
[warning] 10-10: typing.Deque is deprecated, use collections.deque instead
(UP035)
🤖 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`
at 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.
| # Flush queued outbound messages before polling inbound data. | ||
| if self._last_status == MessageChannelConnectionStatus.CONNECTED: | ||
| while self._outbound_queue: | ||
| batch = self._outbound_queue[0] | ||
| if batch.data: | ||
| for message in batch.data: | ||
| self._tracker.send_message(deviceio_session, message.payload) | ||
| self._outbound_queue.popleft() |
There was a problem hiding this comment.
Don't leave already-sent payloads queued after a partial flush.
Line 69 removes the batch only after every payload has been sent. If Line 68 succeeds for one payload and then send_message() throws on a later payload—LiveMessageChannelTrackerImpl::send_message() does this as soon as the channel is no longer connected—poll_tracker() aborts with the whole batch still at the head of the deque. The next reconnect replays the already-delivered prefix. 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 - 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.
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
| parser.add_argument( | ||
| "--outbound-queue-capacity", | ||
| type=int, | ||
| default=256, | ||
| help="Bounded outbound queue length", | ||
| ) |
There was a problem hiding this comment.
Validate outbound queue capacity as positive.
Non-positive values can silently produce unusable queue behavior for outbound messages.
💡 Suggested fix
+def _positive_int(value: str) -> int:
+ ivalue = int(value)
+ if ivalue <= 0:
+ raise argparse.ArgumentTypeError("must be > 0")
+ return ivalue
+
def main() -> int:
@@
parser.add_argument(
"--outbound-queue-capacity",
- type=int,
+ type=_positive_int,
default=256,
help="Bounded outbound queue length",
)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@examples/teleop_session_manager/python/message_channel_example.py` around
lines 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.
| #include <XR_NV_opaque_data_channel.h> | ||
| #include <memory> | ||
| #include <string> | ||
| #include <vector> | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
fd -e hpp -e hh -e h -e hxx src | while read -r f; do
if rg -n '\bstd::array\b' "$f" >/dev/null; then
if ! rg -n '^\s*#\s*include\s*<array>' "$f" >/dev/null; then
echo "Missing <array> include: $f"
rg -n '^\s*#\s*include|\bstd::array\b' "$f"
echo "----"
fi
fi
doneRepository: NVIDIA/IsaacTeleop
Length of output: 44
🏁 Script executed:
#!/bin/bash
# Check if file exists and show its contents
FILE="src/core/live_trackers/cpp/live_message_channel_tracker_impl.hpp"
echo "=== File existence check ==="
if [ -f "$FILE" ]; then
echo "File exists"
echo ""
echo "=== File size and line count ==="
wc -l "$FILE"
echo ""
echo "=== Lines 1-20 (includes section) ==="
head -20 "$FILE" | cat -n
echo ""
echo "=== Lines 40-50 (around line 45) ==="
sed -n '40,50p' "$FILE" | cat -n -v | sed 's/^/ /'
echo ""
echo "=== Search for std::array ==="
rg -n 'std::array' "$FILE" || echo "No std::array found"
echo ""
echo "=== Search for array include ==="
rg -n '#include.*array' "$FILE" || echo "No array include found"
else
echo "File does NOT exist at: $FILE"
echo ""
echo "=== Searching for file with similar name ==="
find . -name "*live_message_channel_tracker*" -type f
fiRepository: NVIDIA/IsaacTeleop
Length of output: 1701
Add missing direct include for std::array.
Line 45 uses std::array, but <array> is not included in this header. This can cause include-order-dependent build breaks.
Proposed fix
`#include` <XR_NV_opaque_data_channel.h>
+#include <array>
`#include` <memory>
`#include` <string>
`#include` <vector>📝 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.
| #include <XR_NV_opaque_data_channel.h> | |
| #include <memory> | |
| #include <string> | |
| #include <vector> | |
| `#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 - 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.
| def create_nodes(self) -> tuple[MessageChannelSource, MessageChannelSink]: | ||
| if len(self.channel_uuid) != 16: | ||
| raise ValueError( | ||
| "MessageChannelConfig.channel_uuid must be exactly 16 bytes" | ||
| ) | ||
| if self.outbound_queue_capacity <= 0: | ||
| raise ValueError("MessageChannelConfig.outbound_queue_capacity must be > 0") |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider consolidating duplicate UUID validation.
UUID length validation is performed both in create_nodes() (lines 26-29) and message_channel_config() (lines 56-59). Since message_channel_config() delegates to create_nodes(), the validation in message_channel_config() 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(Also applies to: 56-59
🧰 Tools
🪛 Ruff (0.15.7)
[warning] 27-29: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 31-31: Avoid specifying long messages outside the exception class
(TRY003)
🤖 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 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).
src/core/retargeting_engine/python/deviceio_source_nodes/message_channel_config.py
Outdated
Show resolved
Hide resolved
| def _compute_fn(self, inputs: RetargeterIO, outputs: RetargeterIO, context) -> None: | ||
| messages_tracked = inputs["messages_tracked"][0] | ||
| self._outbound_queue.append(messages_tracked) |
There was a problem hiding this comment.
Guard empty messages_tracked before indexing.
Line [36] assumes at least one element. An empty group will raise IndexError and break compute.
💡 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])📝 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.
| def _compute_fn(self, inputs: RetargeterIO, outputs: RetargeterIO, context) -> None: | |
| messages_tracked = inputs["messages_tracked"][0] | |
| self._outbound_queue.append(messages_tracked) | |
| def _compute_fn(self, inputs: RetargeterIO, outputs: RetargeterIO, context) -> None: | |
| group = inputs["messages_tracked"] | |
| if len(group) == 0: | |
| return | |
| self._outbound_queue.append(group[0]) |
🧰 Tools
🪛 Ruff (0.15.7)
[warning] 35-35: Unused method argument: outputs
(ARG002)
[warning] 35-35: Unused method argument: context
(ARG002)
🤖 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 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.
| Converts DeviceIO MessageChannelMessagesTrackedT wrapper data for graph use. | ||
| """ | ||
|
|
||
| from typing import Any, TYPE_CHECKING, Deque |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Use collections.deque instead of deprecated typing.Deque.
typing.Deque is deprecated since Python 3.9.
Proposed fix
-from typing import Any, TYPE_CHECKING, Deque
+from typing import Any, TYPE_CHECKING
+from collections import deque
+from collections.abc import MutableSequenceThen update the type hint on line 40:
- outbound_queue: "Deque[MessageChannelMessagesTrackedT]",
+ outbound_queue: deque["MessageChannelMessagesTrackedT"],🧰 Tools
🪛 Ruff (0.15.7)
[warning] 10-10: typing.Deque is deprecated, use collections.deque instead
(UP035)
🤖 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:
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.
src/core/retargeting_engine/python/deviceio_source_nodes/message_channel_source.py
Outdated
Show resolved
Hide resolved
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
| def _parse_uuid_bytes(uuid_text: str) -> bytes: | ||
| """Parse canonical UUID text to 16-byte payload.""" | ||
| return uuid.UUID(uuid_text).bytes |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Python argparse, is using a custom type=callable that raisesargparse.ArgumentTypeErrorthe recommended way to validate a UUID argument instead of callinguuid.UUID(...)afterparse_args()and lettingValueError escape?
💡 Result:
Yes—for a “this argument must be a UUID” check, using type= (a callable that either returns a uuid.UUID or raises argparse.ArgumentTypeError) is the argparse-idiomatic approach, because argparse’s type hook is explicitly meant to perform “type-checking and type conversions” during parsing, and it will catch ArgumentTypeError/TypeError/ValueError and turn them into a proper CLI error message. It also notes type should be used for simple conversions, with more complex validation done downstream—UUID parsing fits the “simple conversion” case. [1]
uuid.UUID(...) raises ValueError for malformed UUID strings (e.g., “badly formed hexadecimal UUID string”). [2] You can let that ValueError occur inside your type= callable (argparse will catch it), but raising ArgumentTypeError is the way to control the exact user-facing error text. [1]
Example:
import argparse, uuid
def uuid_type(s: str) -> uuid.UUID:
try:
return uuid.UUID(s)
except ValueError as e:
raise argparse.ArgumentTypeError(f"invalid UUID: {s!r}") from e
parser = argparse.ArgumentParser()
parser.add_argument("--id", type=uuid_type, required=True)
args = parser.parse_args()What’s not recommended (for user-friendly CLIs) is waiting until after parse_args() and letting a raw ValueError escape, because that bypasses argparse’s normal “nice” error reporting path. [1]
References:
[1] Python argparse docs (type= conversion/validation; caught exceptions; guidance on simple vs downstream validation): https://docs.python.org/3/library/argparse.html#type
[2] CPython uuid.UUID implementation raising ValueError for malformed hex input: https://github.com/python/cpython/blob/v3.14.3/Lib/uuid.py
🏁 Script executed:
fd message_channel_example.pyRepository: NVIDIA/IsaacTeleop
Length of output: 127
🏁 Script executed:
cat -n examples/teleop_session_manager/python/message_channel_example.pyRepository: NVIDIA/IsaacTeleop
Length of output: 4750
Validate --channel-uuid during argparse parsing.
Invalid UUID input currently fails after parse_args() (line 63), producing a raw ValueError traceback instead of a clear CLI error tied to the flag. Move the conversion into type= (line 45) and raise argparse.ArgumentTypeError to provide proper argparse error handling.
Suggested update
def _parse_uuid_bytes(uuid_text: str) -> bytes:
"""Parse canonical UUID text to 16-byte payload."""
- return uuid.UUID(uuid_text).bytes
+ try:
+ return uuid.UUID(uuid_text).bytes
+ except ValueError as exc:
+ raise argparse.ArgumentTypeError(
+ "--channel-uuid must be a valid UUID"
+ ) from exc
@@
parser.add_argument(
"--channel-uuid",
- type=str,
+ type=_parse_uuid_bytes,
required=True,
help="Message channel UUID (canonical form, e.g. 550e8400-e29b-41d4-a716-446655440000)",
)
@@
- channel_uuid = _parse_uuid_bytes(args.channel_uuid)
+ channel_uuid = args.channel_uuid🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@examples/teleop_session_manager/python/message_channel_example.py` around
lines 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.
| std::unique_ptr<IMessageChannelTrackerImpl> LiveDeviceIOFactory::create_message_channel_tracker_impl( | ||
| const MessageChannelTracker* tracker) | ||
| { | ||
| return std::make_unique<LiveMessageChannelTrackerImpl>(handles_, tracker); | ||
| } |
There was a problem hiding this comment.
Recording is silently bypassed for MessageChannelTracker.
All of the other live tracker factory methods thread writer_/get_name() into typed MCAP channels when recording is enabled, but this path always constructs LiveMessageChannelTrackerImpl bare. The PR already adds MessageChannelMessagesRecord, so recording-enabled sessions will still drop message-channel traffic from MCAP.
🛠️ Expected plumbing
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 MessageChannelMcapChannels / create_mcap_channels(...) support in src/core/live_trackers/cpp/live_message_channel_tracker_impl.hpp and .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 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.
| LiveMessageChannelTrackerImpl::LiveMessageChannelTrackerImpl(const OpenXRSessionHandles& handles, | ||
| const MessageChannelTracker* tracker) | ||
| : handles_(handles), tracker_(tracker), receive_buffer_(tracker->max_message_size(), 0) | ||
| { | ||
| 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"); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/core/live_trackers/cpp/live_message_channel_tracker_impl.cpp | head -50Repository: NVIDIA/IsaacTeleop
Length of output: 2002
🏁 Script executed:
cat -n src/core/live_trackers/cpp/live_message_channel_tracker_impl.hpp | head -60Repository: NVIDIA/IsaacTeleop
Length of output: 2796
🏁 Script executed:
# Search for other constructors in tracker implementations to see validation patterns
rg "LiveMessageChannelTrackerImpl::" src/core/live_trackers/cpp/ -A 3Repository: NVIDIA/IsaacTeleop
Length of output: 6530
🏁 Script executed:
# Check if similar OpenXR-based constructors in the codebase use exceptions or asserts
rg "assert.*XR_NULL_HANDLE" . -t cpp --max-count=5 -B 2 -A 2Repository: NVIDIA/IsaacTeleop
Length of output: 4397
Validate constructor inputs before using tracker in the initializer list.
Line 20 dereferences tracker->max_message_size() in the member initializer before Line 25 validates the pointer. In release builds where NDEBUG is defined, assert is no-op, leaving the code vulnerable to null-pointer dereference. The codebase already uses exceptions for runtime validation (e.g., std::runtime_error in send_message()); apply the same pattern here.
Suggested 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)
- : handles_(handles), tracker_(tracker), receive_buffer_(tracker->max_message_size(), 0)
+ : handles_(handles), tracker_(tracker), receive_buffer_(make_receive_buffer(tracker))
{
- 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");
+ }🧰 Tools
🪛 Cppcheck (2.20.0)
[style] 20-20: The function 'required_extensions' is never used.
(unusedFunction)
🤖 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 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.
| uint32_t count_out = 0; | ||
| XrResult recv_result = | ||
| receive_fn_(channel_, static_cast<uint32_t>(receive_buffer_.size()), &count_out, receive_buffer_.data()); | ||
| if (recv_result != XR_SUCCESS) | ||
| { | ||
| if (recv_result == XR_ERROR_CHANNEL_NOT_CONNECTED_NV) | ||
| { | ||
| return false; | ||
| } | ||
| throw std::runtime_error("LiveMessageChannelTrackerImpl: xrReceiveOpaqueDataChannelNV failed, result=" + | ||
| std::to_string(recv_result)); | ||
| } | ||
|
|
||
| if (count_out == 0) | ||
| { | ||
| break; | ||
| } | ||
|
|
||
| auto message = std::make_shared<MessageChannelMessagesT>(); | ||
| message->payload.assign(receive_buffer_.begin(), receive_buffer_.begin() + count_out); | ||
| messages_.data.push_back(message); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
NVIDIA CloudXR xrReceiveOpaqueDataChannelNV two-call pattern documentation
💡 Result:
NVIDIA’s CloudXR docs that explicitly describe the two-call pattern for receiving opaque data are in the CloudXR SDK “Unreal Engine Integration Guide” (even though the pattern is general OpenXR-style usage):
- Size query call (no buffer):
uint32_t receivedCount = 0;
xrReceiveOpaqueDataChannelNV(channel, 0, &receivedCount, nullptr);- Payload read call (allocate buffer, then read):
buffer.resize(receivedCount);
xrReceiveOpaqueDataChannelNV(channel, bufferCapacity, &receivedCount, buffer.data());The guide states: “the extension uses the standard two-call idiom” (first call queries available size; second call allocates and reads). It also notes a “typical limit” of 900 bytes (their example defines XR_NV_OPAQUE_BUF_SIZE as 900). [1]
For the broader CloudXR runtime-side overview of xrReceiveOpaqueDataChannelNV (parameters, lifecycle/state constraints, etc.), see “XR Opaque Data Channel” in the CloudXR SDK docs. [2]
Sources: [1] [2]
🏁 Script executed:
# Check if the file exists and examine context around the mentioned lines
find . -name "live_message_channel_tracker_impl.cpp" -type fRepository: NVIDIA/IsaacTeleop
Length of output: 128
🏁 Script executed:
# Search for xrReceiveOpaqueDataChannelNV usage in the codebase
rg "xrReceiveOpaqueDataChannelNV" -A 5 -B 5Repository: NVIDIA/IsaacTeleop
Length of output: 5737
🏁 Script executed:
# Look for receive_fn_ definitions and usage patterns
rg "receive_fn_" -B 3 -A 3Repository: NVIDIA/IsaacTeleop
Length of output: 2749
🏁 Script executed:
# Read the actual code around lines 59-79
cat -n src/core/live_trackers/cpp/live_message_channel_tracker_impl.cpp | sed -n '40,100p'Repository: NVIDIA/IsaacTeleop
Length of output: 2280
🏁 Script executed:
# Look for receive_buffer_ initialization and max_message_size usage
rg "receive_buffer_" -B 2 -A 2Repository: NVIDIA/IsaacTeleop
Length of output: 2312
🏁 Script executed:
# Check the header file for member variable definitions
cat -n src/core/live_trackers/cpp/live_message_channel_tracker_impl.hppRepository: NVIDIA/IsaacTeleop
Length of output: 2943
Adopt the NVIDIA-documented two-call receive pattern for message validation.
The CloudXR SDK specifies that xrReceiveOpaqueDataChannelNV should follow the standard two-call idiom: first query the pending byte count with (0, nullptr), then validate the size and perform the actual read. The current one-shot approach provides no protection if a remote client sends a message exceeding max_message_size(), converting what should be a handled error condition into an uncontrolled exception.
Suggested implementation
while (true)
{
uint32_t count_out = 0;
- XrResult recv_result =
- receive_fn_(channel_, static_cast<uint32_t>(receive_buffer_.size()), &count_out, receive_buffer_.data());
+ XrResult recv_result = receive_fn_(channel_, 0, &count_out, nullptr);
if (recv_result != XR_SUCCESS)
{
if (recv_result == XR_ERROR_CHANNEL_NOT_CONNECTED_NV)
{
return false;
@@
if (count_out == 0)
{
break;
}
+
+ if (count_out > receive_buffer_.size())
+ {
+ std::cerr << "[LiveMessageChannelTrackerImpl] Received payload exceeds max_message_size: "
+ << count_out << " > " << receive_buffer_.size() << std::endl;
+ return false;
+ }
+
+ recv_result = receive_fn_(channel_, count_out, &count_out, receive_buffer_.data());
+ if (recv_result != XR_SUCCESS)
+ {
+ if (recv_result == XR_ERROR_CHANNEL_NOT_CONNECTED_NV)
+ {
+ return false;
+ }
+ throw std::runtime_error("LiveMessageChannelTrackerImpl: xrReceiveOpaqueDataChannelNV failed, result=" +
+ std::to_string(recv_result));
+ }
auto message = std::make_shared<MessageChannelMessagesT>();
message->payload.assign(receive_buffer_.begin(), receive_buffer_.begin() + count_out);🤖 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 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.
| XrSystemId LiveMessageChannelTrackerImpl::resolve_system_id() const | ||
| { | ||
| XrSystemGetInfo get_info{ XR_TYPE_SYSTEM_GET_INFO }; | ||
| get_info.formFactor = XR_FORM_FACTOR_HEAD_MOUNTED_DISPLAY; | ||
|
|
||
| XrSystemId system_id = XR_NULL_SYSTEM_ID; | ||
| XrResult result = get_system_fn_(handles_.instance, &get_info, &system_id); | ||
| if (result != XR_SUCCESS) | ||
| { | ||
| throw std::runtime_error("LiveMessageChannelTrackerImpl: xrGetSystem failed, result=" + std::to_string(result)); | ||
| } | ||
| return system_id; | ||
| } | ||
|
|
||
| void LiveMessageChannelTrackerImpl::create_channel() | ||
| { | ||
| XrOpaqueDataChannelCreateInfoNV create_info{ XR_TYPE_OPAQUE_DATA_CHANNEL_CREATE_INFO_NV }; | ||
| create_info.systemId = system_id_; | ||
| create_info.uuid = channel_uuid_; | ||
|
|
||
| XrResult result = create_channel_fn_(handles_.instance, &create_info, &channel_); | ||
| if (result != XR_SUCCESS) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Prefer the session’s XrSystemId over re-querying with XR_FORM_FACTOR_HEAD_MOUNTED_DISPLAY.
NVIDIA’s integration guide stores the XrSystemId from session creation and reuses it for channel creation. Re-deriving it here hard-codes the form factor and can drift from the actual session system if the runtime setup changes. (docs.nvidia.com)
🤖 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 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.
| void LiveMessageChannelTrackerImpl::destroy_channel() noexcept | ||
| { | ||
| if (channel_ == XR_NULL_HANDLE) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| if (shutdown_fn_) | ||
| { | ||
| shutdown_fn_(channel_); | ||
| } | ||
| if (destroy_channel_fn_) | ||
| { | ||
| destroy_channel_fn_(channel_); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's verify the file exists and check the exact code at the specified lines
wc -l src/core/live_trackers/cpp/live_message_channel_tracker_impl.cppRepository: NVIDIA/IsaacTeleop
Length of output: 130
🏁 Script executed:
# Read the specific section mentioned in the review
sed -n '172,186p' src/core/live_trackers/cpp/live_message_channel_tracker_impl.cppRepository: NVIDIA/IsaacTeleop
Length of output: 337
🏁 Script executed:
# Get broader context around this function to understand the class structure
sed -n '150,200p' src/core/live_trackers/cpp/live_message_channel_tracker_impl.cppRepository: NVIDIA/IsaacTeleop
Length of output: 1479
🏁 Script executed:
# Search for how this class is structured and if there's a logging mechanism
head -100 src/core/live_trackers/cpp/live_message_channel_tracker_impl.cpp | grep -E '(`#include`|namespace|class|using)'Repository: NVIDIA/IsaacTeleop
Length of output: 276
🏁 Script executed:
# Check if there's a header file that defines the function signatures
fd -t f 'live_message_channel_tracker' | head -10Repository: NVIDIA/IsaacTeleop
Length of output: 191
🏁 Script executed:
# Look for other error handling patterns in similar cpp files
find . -name '*.cpp' -path '*/live_trackers/*' -type fRepository: NVIDIA/IsaacTeleop
Length of output: 617
🏁 Script executed:
# Search for XR_SUCCESS and error handling patterns in the codebase
rg 'XR_SUCCESS|XR_ERROR' --type cpp | head -20Repository: NVIDIA/IsaacTeleop
Length of output: 1813
🏁 Script executed:
# Read the header file to see the function types
cat src/core/live_trackers/cpp/live_message_channel_tracker_impl.hppRepository: NVIDIA/IsaacTeleop
Length of output: 2488
🏁 Script executed:
# Search for how shutdown_fn_ and destroy_channel_fn_ are used/defined
rg 'shutdown_fn_|destroy_channel_fn_' --type cpp --type hppRepository: NVIDIA/IsaacTeleop
Length of output: 89
🏁 Script executed:
# Search for any CloudXR or NVIDIA sample code references
rg -i 'cloudxr|nvidia.*sample|xrShutdownOpaqueDataChannelNV|xrDestroyOpaqueDataChannelNV' --type cpp --type hppRepository: NVIDIA/IsaacTeleop
Length of output: 89
🏁 Script executed:
# Search for function pointer definitions and uses
rg 'shutdown_fn_|destroy_channel_fn_' -A 2 -B 2Repository: NVIDIA/IsaacTeleop
Length of output: 3554
🏁 Script executed:
# Look at the OpenXR header file to understand return types
grep -n 'PFN_xr.*OpaqueDataChannel' src/core/live_trackers/cpp/live_message_channel_tracker_impl.hppRepository: NVIDIA/IsaacTeleop
Length of output: 458
🏁 Script executed:
# Check how xrShutdownOpaqueDataChannelNV is used elsewhere in the codebase
rg 'xrShutdownOpaqueDataChannelNV|xrDestroyOpaqueDataChannelNV'Repository: NVIDIA/IsaacTeleop
Length of output: 999
🏁 Script executed:
# Check the initialize_functions method to see how these are set up
sed -n '1,238p' src/core/live_trackers/cpp/live_message_channel_tracker_impl.cpp | grep -A 30 'void LiveMessageChannelTrackerImpl::initialize_functions'Repository: NVIDIA/IsaacTeleop
Length of output: 1951
🏁 Script executed:
# Check the OpenXR header to confirm return types
cat deps/cloudxr/openxr_extensions/XR_NV_opaque_data_channel.h | grep -A 2 "PFN_xrDestroyOpaqueDataChannelNV\|PFN_xrShutdownOpaqueDataChannelNV"Repository: NVIDIA/IsaacTeleop
Length of output: 497
🏁 Script executed:
# Look at other uses of send_fn_ and receive_fn_ to see error handling patterns
rg 'send_fn_|receive_fn_' -A 3 -B 1 src/core/live_trackers/cpp/live_message_channel_tracker_impl.cppRepository: NVIDIA/IsaacTeleop
Length of output: 1313
🏁 Script executed:
# Verify the try_reopen_channel flow and how destroy_channel impacts it
sed -n '190,220p' src/core/live_trackers/cpp/live_message_channel_tracker_impl.cppRepository: NVIDIA/IsaacTeleop
Length of output: 1103
🏁 Script executed:
# Check if XR_ERROR_CHANNEL_NOT_CONNECTED_NV is a valid constant
grep -r 'XR_ERROR_CHANNEL_NOT_CONNECTED_NV' deps/cloudxr/openxr_extensions/Repository: NVIDIA/IsaacTeleop
Length of output: 184
🏁 Script executed:
# Verify the noexcept constraint and why destroy_channel can't throw
sed -n '165,190p' src/core/live_trackers/cpp/live_message_channel_tracker_impl.cppRepository: NVIDIA/IsaacTeleop
Length of output: 643
🏁 Script executed:
# Check how other noexcept destructors or functions handle error cases
rg 'noexcept.*{' -A 10 src/core/live_trackers/cpp/ | head -40Repository: NVIDIA/IsaacTeleop
Length of output: 147
🏁 Script executed:
# Search for noexcept usage in the file
rg 'noexcept' -A 5 src/core/live_trackers/cpp/live_message_channel_tracker_impl.cppRepository: NVIDIA/IsaacTeleop
Length of output: 190
🏁 Script executed:
# Check the destructor and how destroy_channel is called
sed -n '1,100p' src/core/live_trackers/cpp/live_message_channel_tracker_impl.cpp | grep -A 10 '~LiveMessageChannelTrackerImpl'Repository: NVIDIA/IsaacTeleop
Length of output: 354
Surface shutdown/destroy failures from the XR APIs.
xrShutdownOpaqueDataChannelNV and xrDestroyOpaqueDataChannelNV return XrResult but are currently called without checking for errors. Since destroy_channel() is marked noexcept (called from the destructor), these failures cannot throw exceptions. Silent failures during teardown complicate reconnection diagnostics, especially when try_reopen_channel() calls destroy_channel() as part of its recovery path.
Suggested fix
if (shutdown_fn_)
{
- shutdown_fn_(channel_);
+ const XrResult shutdown_result = shutdown_fn_(channel_);
+ if (shutdown_result != XR_SUCCESS && shutdown_result != XR_ERROR_CHANNEL_NOT_CONNECTED_NV)
+ {
+ std::cerr << "[LiveMessageChannelTrackerImpl] xrShutdownOpaqueDataChannelNV failed: "
+ << shutdown_result << std::endl;
+ }
}
if (destroy_channel_fn_)
{
- destroy_channel_fn_(channel_);
+ const XrResult destroy_result = destroy_channel_fn_(channel_);
+ if (destroy_result != XR_SUCCESS)
+ {
+ std::cerr << "[LiveMessageChannelTrackerImpl] xrDestroyOpaqueDataChannelNV failed: "
+ << destroy_result << std::endl;
+ }
}📝 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.
| void LiveMessageChannelTrackerImpl::destroy_channel() noexcept | |
| { | |
| if (channel_ == XR_NULL_HANDLE) | |
| { | |
| return; | |
| } | |
| if (shutdown_fn_) | |
| { | |
| shutdown_fn_(channel_); | |
| } | |
| if (destroy_channel_fn_) | |
| { | |
| destroy_channel_fn_(channel_); | |
| } | |
| void LiveMessageChannelTrackerImpl::destroy_channel() noexcept | |
| { | |
| if (channel_ == XR_NULL_HANDLE) | |
| { | |
| return; | |
| } | |
| if (shutdown_fn_) | |
| { | |
| const XrResult shutdown_result = shutdown_fn_(channel_); | |
| if (shutdown_result != XR_SUCCESS && shutdown_result != XR_ERROR_CHANNEL_NOT_CONNECTED_NV) | |
| { | |
| std::cerr << "[LiveMessageChannelTrackerImpl] xrShutdownOpaqueDataChannelNV failed: " | |
| << shutdown_result << std::endl; | |
| } | |
| } | |
| if (destroy_channel_fn_) | |
| { | |
| const XrResult destroy_result = destroy_channel_fn_(channel_); | |
| if (destroy_result != XR_SUCCESS) | |
| { | |
| std::cerr << "[LiveMessageChannelTrackerImpl] xrDestroyOpaqueDataChannelNV failed: " | |
| << destroy_result << std::endl; | |
| } | |
| } |
🤖 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 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.
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
| sink.compute({"messages_tracked": tg}, {}) | ||
|
|
||
|
|
||
| def main() -> int: |
There was a problem hiding this comment.
Add explicit return value for main().
The function signature declares -> int but the function implicitly returns None on successful completion. Add return 0 at the end of main().
Suggested fix
time.sleep(0.01)
+
+ return 0
if __name__ == "__main__":🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@examples/teleop_session_manager/python/message_channel_example.py` at 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.
| void LiveMessageChannelTrackerImpl::send_message(const std::vector<uint8_t>& payload) const | ||
| { | ||
| if (payload.size() > tracker_->max_message_size()) | ||
| { | ||
| throw std::runtime_error("LiveMessageChannelTrackerImpl::send_message: payload size " + | ||
| std::to_string(payload.size()) + " exceeds max_message_size " + | ||
| std::to_string(tracker_->max_message_size())); | ||
| } | ||
|
|
||
| XrOpaqueDataChannelStateNV channel_state{ XR_TYPE_OPAQUE_DATA_CHANNEL_STATE_NV }; | ||
| XrResult state_result = get_state_fn_(channel_, &channel_state); | ||
| if (state_result != XR_SUCCESS) | ||
| { | ||
| throw std::runtime_error("LiveMessageChannelTrackerImpl: xrGetOpaqueDataChannelStateNV failed, result=" + | ||
| std::to_string(state_result)); | ||
| } | ||
| if (channel_state.state != XR_OPAQUE_DATA_CHANNEL_STATUS_CONNECTED_NV) | ||
| { | ||
| throw std::runtime_error("LiveMessageChannelTrackerImpl::send_message: channel is not connected"); | ||
| } | ||
|
|
||
| const uint8_t* payload_ptr = payload.empty() ? nullptr : payload.data(); | ||
| XrResult send_result = send_fn_(channel_, static_cast<uint32_t>(payload.size()), payload_ptr); | ||
| if (send_result != XR_SUCCESS) | ||
| { | ||
| throw std::runtime_error("LiveMessageChannelTrackerImpl: xrSendOpaqueDataChannelNV failed, result=" + | ||
| std::to_string(send_result)); | ||
| } | ||
| } |
There was a problem hiding this comment.
Add null handle guard before querying channel state.
send_message() calls get_state_fn_(channel_, ...) at line 102 without checking if channel_ is XR_NULL_HANDLE. After a failed reconnection attempt, channel_ can be null, and passing it to the OpenXR function will cause undefined behavior or throw an exception. This mirrors the query_status() vulnerability.
Proposed fix
void LiveMessageChannelTrackerImpl::send_message(const std::vector<uint8_t>& payload) const
{
if (payload.size() > tracker_->max_message_size())
{
throw std::runtime_error("LiveMessageChannelTrackerImpl::send_message: payload size " +
std::to_string(payload.size()) + " exceeds max_message_size " +
std::to_string(tracker_->max_message_size()));
}
+ if (channel_ == XR_NULL_HANDLE)
+ {
+ throw std::runtime_error("LiveMessageChannelTrackerImpl::send_message: channel is not open");
+ }
+
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 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.
| # deque(maxlen=N) provides bounded queueing and drops oldest on overflow. | ||
| outbound_queue: deque["MessageChannelMessagesTrackedT"] = deque( | ||
| maxlen=self.outbound_queue_capacity | ||
| ) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Remove quotes from type annotation.
The TYPE_CHECKING import is present, so the forward reference quotes around MessageChannelMessagesTrackedT are unnecessary.
Suggested fix
- outbound_queue: deque["MessageChannelMessagesTrackedT"] = deque(
+ outbound_queue: deque[MessageChannelMessagesTrackedT] = deque(
maxlen=self.outbound_queue_capacity
)And add the import at runtime:
if TYPE_CHECKING:
from isaacteleop.schema import MessageChannelMessagesTrackedT
+
+from isaacteleop.schema import MessageChannelMessagesTrackedT🧰 Tools
🪛 Ruff (0.15.7)
[warning] 42-42: Remove quotes from type annotation
Remove quotes
(UP037)
🤖 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 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.
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