refactor: replace HandInjector with SchemaPusher pipeline for Haply device#325
Open
vickybot911 wants to merge 7 commits intoNVIDIA:mainfrom
Open
refactor: replace HandInjector with SchemaPusher pipeline for Haply device#325vickybot911 wants to merge 7 commits intoNVIDIA:mainfrom
vickybot911 wants to merge 7 commits intoNVIDIA:mainfrom
Conversation
Add a new plugin for Haply Robotics Inverse3 and VerseGrip devices, bridging the Haply SDK WebSocket interface to the Isaac Teleop framework via OpenXR hand tracking injection. Components: - haply_plugin_core: shared library (libIsaacTeleopPluginsHaply.so) implementing WebSocket client, JSON parsing, OpenXR HandInjector integration, and DeviceIO controller tracking - haply_hand_plugin: plugin executable running at 90Hz - haply_hand_tracker_printer: standalone CLI tool for verifying Haply device data without OpenXR dependencies Architecture: - Background I/O thread communicates with Haply SDK via WebSocket (ws://localhost:10001, configurable via HAPLY_WEBSOCKET_HOST/PORT) - Maps Inverse3 cursor position and VerseGrip orientation to OpenXR XrHandJointLocationEXT format (26 joints) - WRIST and PALM joints receive real tracked data; finger joints are placed at wrist position with VALID but not TRACKED flags - Controller aim pose provides root positioning (same as Manus plugin) - Exponential backoff reconnection on WebSocket errors No build-time SDK dependency — Haply SDK is a runtime service. Vendored: nlohmann/json (MIT, single header) for JSON parsing. Signed-off-by: Vicky <vickybot911@gmail.com>
- Add WebSocket payload size limit (16 MB) to prevent OOM on malformed frames - Add SO_RCVTIMEO (5s) connect timeout to prevent indefinite blocking - Replace std::atoi with std::stoul + range validation for port parsing - Add signal handler for graceful shutdown in plugin executable - Add TODO comment for unused m_grip_interpolant (future finger synthesis) - Fix British spelling: Synthesised → Synthesized - Apply same payload limit and port validation fixes to printer tool Signed-off-by: Vicky <vickybot911@gmail.com>
- Rename HAPLY_WS_HOST/HAPLY_WS_PORT to HAPLY_WEBSOCKET_HOST/HAPLY_WEBSOCKET_PORT in core plugin to match README and printer tool - Add SO_RCVTIMEO (5s) to printer tool's MiniWebSocket for consistency Signed-off-by: Vicky <vickybot911@gmail.com>
…evice
Rework the Haply Inverse3 + VerseGrip plugin to plumb raw joint/device
data directly through the Isaac Teleop pipeline instead of synthesizing
OpenXR hand tracking data.
This addresses the reviewer feedback that the plugin should NOT convert
Haply data to OpenXR hand joints, but instead push the raw device state
through FlatBuffers → SchemaPusher → SchemaTracker → IDeviceIOSource
for direct consumption in Isaac Lab via TensorGroup.
Changes:
1. FlatBuffers schema (haply_device.fbs):
- New schema with all Haply device fields: cursor position (3DOF),
velocity (3DOF), orientation quaternion, 4 buttons, handedness
- Tracked/Record wrappers following existing conventions
2. C++ Plugin (SchemaPusher side):
- New HaplyPlugin class replaces HaplyTracker
- Removes HandInjector, ControllerTracker, and all OpenXR hand joint
synthesis code
- Uses SchemaPusher to push HaplyDeviceOutput at 90Hz
- WebSocket I/O thread preserved unchanged
3. C++ ITracker (SchemaTracker side):
- New HaplyDeviceTracker following Generic3AxisPedalTracker pattern
- Reads HaplyDeviceOutput via SchemaTracker
- MCAP recording support via serialize_all()
- Python bindings exposed via pybind11
4. Python IDeviceIOSource:
- New HaplyDeviceSource node converts HaplyDeviceOutputT to
HaplyDeviceInput TensorGroup with 15 named fields
- HaplyDeviceInput/HaplyDeviceInputIndex tensor types added
- Full integration with TeleopSession pipeline
Signed-off-by: Vicky <vickybot911@gmail.com>
Remove haply_hand_tracking_plugin.cpp and haply_hand_tracking_plugin.hpp which are superseded by the new SchemaPusher-based haply_plugin.cpp and haply_plugin.hpp. Signed-off-by: Vicky <vickybot911@gmail.com>
📝 WalkthroughWalkthroughAdds Haply device integration: a Linux-only WebSocket-backed Haply plugin (C++), a DeviceIO Haply tracker with FlatBuffers schema, Python bindings and retargeting tensor/source nodes, diagnostic tools, CMake integration, presets, plugin manifest, and a mock WebSocket server for testing. Changes
Sequence Diagram(s)sequenceDiagram
participant HaplySDK as "Haply SDK (WebSocket)"
participant Plugin as "HaplyPlugin (C++)"
participant Schema as "SchemaPusher / DeviceIO Core"
participant Retarget as "Retargeting Engine (Python)"
HaplySDK->>Plugin: JSON telemetry (inverse3, wireless_verse_grip)
Plugin->>Plugin: parse JSON, update HaplyDeviceState (mutex)
Plugin->>Schema: push FlatBuffer payload + monotonic timestamp
Schema->>Retarget: samples available in collection/tensor
Retarget->>Retarget: HaplyDeviceSource polls tracker, converts to TensorGroup
Retarget->>Retarget: emit HaplyDeviceInput into pipeline
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CMakeLists.txt`:
- Line 146: The Haply plugin is Linux/x86_64-only but is unguarded where it’s
included (add_subdirectory(src/plugins/haply)), so add a platform check to
prevent inclusion on unsupported platforms; either add a guard around the
add_subdirectory call (preferred) that checks CMAKE_SYSTEM_NAME and
CMAKE_SYSTEM_PROCESSOR and skips adding src/plugins/haply with a clear status
message, or add an early-return platform check at the top of
src/plugins/haply/CMakeLists.txt that emits a status message and returns when
not Linux x86_64.
In `@src/plugins/haply/app/CMakeLists.txt`:
- Around line 4-25: The executable target name haply_hand_plugin doesn't match
the binary name expected in plugin.yaml (haply_device_plugin), so set the
target's output name to the declared runtime name: update the CMake target
haply_hand_plugin (the same target used in add_executable and
set_target_properties) to export/build as haply_device_plugin by adding the
appropriate property (e.g., OUTPUT_NAME/RUNTIME_OUTPUT_NAME) so the produced
binary matches plugin.yaml's ./haply_device_plugin.
In `@src/plugins/haply/core/haply_plugin.cpp`:
- Around line 430-431: Replace the fragile fixed delay (the
std::this_thread::sleep_for(std::chrono::milliseconds(500)) waiting for
connection) with a proper synchronization primitive: add a
std::condition_variable, std::mutex and a bool flag (e.g., first_packet_received
or haply_connected) and have the startup code wait on
condition_variable.wait_for with a predicate that checks the flag (and a
reasonable timeout to avoid deadlock). In the connection/data-receive handler
(the function that processes the first successful connection or incoming device
data), set the flag and call condition_variable.notify_one() so the waiting
thread proceeds only after real data has arrived. Ensure the wait uses the
predicate form to avoid spurious-wake issues and remove the fixed sleep call
(replace the symbol std::this_thread::sleep_for).
- Around line 543-633: The code currently holds m_state_mutex for the entire
JSON parsing and the ws.send_text call; parse the JSON into local
variables/structures first (extract inverse3 info like inverse3_device_id,
cursor_position/velocity, handedness and wireless_verse_grip info like
versegrip_device_id, orientation, buttons) without locking, then acquire
m_state_mutex only to copy those locals into m_state and set m_state.has_data,
release the lock, and perform ws.send_text (and any other I/O) outside the
mutex; update logic around inverse3_device_id and orientation/buttons to use the
local values so get_raw_state() and push_current_state() are not blocked by
parsing or network send.
In `@src/plugins/haply/inc/core/haply_plugin.hpp`:
- Around line 75-107: The class HaplyWebSocket manages a raw file descriptor fd_
but only deletes copy ctor/assign, leaving the compiler-generated move
ctor/assign which would move the fd value and risk double-close or
use-after-close; explicitly delete the move operations by adding
"HaplyWebSocket(HaplyWebSocket&&) = delete;" and "HaplyWebSocket&
operator=(HaplyWebSocket&&) = delete;" to the class declaration (near the
existing deleted copy ops) so moves are prohibited and ownership remains clear.
In `@src/plugins/haply/tools/haply_hand_tracker_printer.cpp`:
- Around line 37-303: The MiniWebSocket implementation duplicates
HaplyWebSocket; extract their common functionality (handshake, framing,
send_frame, recv_frame/recv_text, send_raw, recv_raw, and connection management)
into a single header-only utility (e.g., a shared class or free functions) and
have both MiniWebSocket and HaplyWebSocket delegate to or inherit from that
utility; update MiniWebSocket::connect, send_frame, recv_text, send_raw,
recv_raw, close, and is_connected to call the shared implementation and remove
duplicated code so future fixes apply to both places.
🪄 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: 1ac60ea9-82ac-4acf-8c2e-7213c2ec6c5f
📒 Files selected for processing (27)
CMakeLists.txtsrc/core/deviceio/cpp/CMakeLists.txtsrc/core/deviceio/cpp/haply_device_tracker.cppsrc/core/deviceio/cpp/inc/deviceio/haply_device_tracker.hppsrc/core/deviceio/python/deviceio_bindings.cppsrc/core/deviceio/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/haply_source.pysrc/core/retargeting_engine/python/tensor_types/__init__.pysrc/core/retargeting_engine/python/tensor_types/indices.pysrc/core/retargeting_engine/python/tensor_types/standard_types.pysrc/core/schema/fbs/haply_device.fbssrc/plugins/haply/.gitignoresrc/plugins/haply/CMakeLists.txtsrc/plugins/haply/CMakePresets.jsonsrc/plugins/haply/CMakePresets.json.licensesrc/plugins/haply/README.mdsrc/plugins/haply/app/CMakeLists.txtsrc/plugins/haply/app/main.cppsrc/plugins/haply/core/CMakeLists.txtsrc/plugins/haply/core/haply_plugin.cppsrc/plugins/haply/inc/core/haply_plugin.hppsrc/plugins/haply/plugin.yamlsrc/plugins/haply/third_party/nlohmann/json.hppsrc/plugins/haply/tools/CMakeLists.txtsrc/plugins/haply/tools/haply_hand_tracker_printer.cpp
Comment on lines
+430
to
+431
| // Wait briefly for connection to establish | ||
| std::this_thread::sleep_for(std::chrono::milliseconds(500)); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Fixed sleep for connection establishment is fragile.
The 500ms sleep to wait for connection is a code smell. If the connection takes longer, the OpenXR session may be created without valid device data. Consider using a condition variable to signal when the first successful connection and data reception occurs.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/plugins/haply/core/haply_plugin.cpp` around lines 430 - 431, Replace the
fragile fixed delay (the
std::this_thread::sleep_for(std::chrono::milliseconds(500)) waiting for
connection) with a proper synchronization primitive: add a
std::condition_variable, std::mutex and a bool flag (e.g., first_packet_received
or haply_connected) and have the startup code wait on
condition_variable.wait_for with a predicate that checks the flag (and a
reasonable timeout to avoid deadlock). In the connection/data-receive handler
(the function that processes the first successful connection or incoming device
data), set the flag and call condition_variable.notify_one() so the waiting
thread proceeds only after real data has arrived. Ensure the wait uses the
predicate form to avoid spurious-wake issues and remove the fixed sleep call
(replace the symbol std::this_thread::sleep_for).
Comment on lines
+37
to
+303
| class MiniWebSocket | ||
| { | ||
| public: | ||
| MiniWebSocket() = default; | ||
| ~MiniWebSocket() | ||
| { | ||
| close(); | ||
| } | ||
|
|
||
| MiniWebSocket(const MiniWebSocket&) = delete; | ||
| MiniWebSocket& operator=(const MiniWebSocket&) = delete; | ||
|
|
||
| bool connect(const std::string& host, uint16_t port, const std::string& path = "/") | ||
| { | ||
| close(); | ||
|
|
||
| struct addrinfo hints | ||
| { | ||
| }; | ||
| hints.ai_family = AF_INET; | ||
| hints.ai_socktype = SOCK_STREAM; | ||
| struct addrinfo* res = nullptr; | ||
| std::string port_str = std::to_string(port); | ||
| if (getaddrinfo(host.c_str(), port_str.c_str(), &hints, &res) != 0 || !res) | ||
| { | ||
| return false; | ||
| } | ||
|
|
||
| fd_ = ::socket(res->ai_family, res->ai_socktype, res->ai_protocol); | ||
| if (fd_ < 0) | ||
| { | ||
| freeaddrinfo(res); | ||
| return false; | ||
| } | ||
|
|
||
| if (::connect(fd_, res->ai_addr, res->ai_addrlen) < 0) | ||
| { | ||
| freeaddrinfo(res); | ||
| ::close(fd_); | ||
| fd_ = -1; | ||
| return false; | ||
| } | ||
| freeaddrinfo(res); | ||
|
|
||
| int flag = 1; | ||
| setsockopt(fd_, IPPROTO_TCP, TCP_NODELAY, &flag, sizeof(flag)); | ||
|
|
||
| // Set receive timeout to prevent indefinite blocking | ||
| struct timeval tv; | ||
| tv.tv_sec = 5; | ||
| tv.tv_usec = 0; | ||
| setsockopt(fd_, SOL_SOCKET, SO_RCVTIMEO, &tv, sizeof(tv)); | ||
|
|
||
| // Random WebSocket key | ||
| std::random_device rd; | ||
| std::mt19937 gen(rd()); | ||
| std::uniform_int_distribution<> dis(0, 255); | ||
| uint8_t key_bytes[16]; | ||
| for (int i = 0; i < 16; ++i) | ||
| { | ||
| key_bytes[i] = static_cast<uint8_t>(dis(gen)); | ||
| } | ||
|
|
||
| static const char* b64 = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+/"; | ||
| std::string ws_key; | ||
| ws_key.reserve(24); | ||
| for (int i = 0; i < 16; i += 3) | ||
| { | ||
| uint32_t n = (static_cast<uint32_t>(key_bytes[i]) << 16); | ||
| if (i + 1 < 16) | ||
| n |= (static_cast<uint32_t>(key_bytes[i + 1]) << 8); | ||
| if (i + 2 < 16) | ||
| n |= static_cast<uint32_t>(key_bytes[i + 2]); | ||
| ws_key += b64[(n >> 18) & 0x3F]; | ||
| ws_key += b64[(n >> 12) & 0x3F]; | ||
| ws_key += (i + 1 < 16) ? b64[(n >> 6) & 0x3F] : '='; | ||
| ws_key += (i + 2 < 16) ? b64[n & 0x3F] : '='; | ||
| } | ||
|
|
||
| std::string req = "GET " + path + " HTTP/1.1\r\nHost: " + host + ":" + std::to_string(port) + | ||
| "\r\nUpgrade: websocket\r\nConnection: Upgrade\r\nSec-WebSocket-Key: " + ws_key + | ||
| "\r\nSec-WebSocket-Version: 13\r\n\r\n"; | ||
|
|
||
| if (!send_raw(req.data(), req.size())) | ||
| { | ||
| close(); | ||
| return false; | ||
| } | ||
|
|
||
| std::string resp; | ||
| char buf; | ||
| while (resp.size() < 4096) | ||
| { | ||
| if (!recv_raw(&buf, 1)) | ||
| { | ||
| close(); | ||
| return false; | ||
| } | ||
| resp += buf; | ||
| if (resp.size() >= 4 && resp.substr(resp.size() - 4) == "\r\n\r\n") | ||
| { | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| if (resp.find("101") == std::string::npos) | ||
| { | ||
| close(); | ||
| return false; | ||
| } | ||
|
|
||
| return true; | ||
| } | ||
|
|
||
| bool send_text(const std::string& payload) | ||
| { | ||
| return send_frame(0x01, payload.data(), payload.size()); | ||
| } | ||
|
|
||
| bool recv_text(std::string& out) | ||
| { | ||
| if (fd_ < 0) | ||
| return false; | ||
| out.clear(); | ||
| bool fin = false; | ||
| while (!fin) | ||
| { | ||
| uint8_t hdr[2]; | ||
| if (!recv_raw(hdr, 2)) | ||
| return false; | ||
| fin = (hdr[0] & 0x80) != 0; | ||
| uint8_t opcode = hdr[0] & 0x0F; | ||
| bool masked = (hdr[1] & 0x80) != 0; | ||
| uint64_t plen = hdr[1] & 0x7F; | ||
| if (plen == 126) | ||
| { | ||
| uint8_t e[2]; | ||
| if (!recv_raw(e, 2)) | ||
| return false; | ||
| plen = (uint64_t(e[0]) << 8) | e[1]; | ||
| } | ||
| else if (plen == 127) | ||
| { | ||
| uint8_t e[8]; | ||
| if (!recv_raw(e, 8)) | ||
| return false; | ||
| plen = 0; | ||
| for (int i = 0; i < 8; ++i) | ||
| plen = (plen << 8) | e[i]; | ||
| } | ||
| uint8_t mask[4] = {}; | ||
| if (masked && !recv_raw(mask, 4)) | ||
| return false; | ||
| constexpr uint64_t kMaxPayloadSize = 16 * 1024 * 1024; // 16 MB | ||
| if (plen > kMaxPayloadSize) | ||
| { | ||
| std::cerr << "[MiniWebSocket] Payload too large: " << plen << " bytes" << std::endl; | ||
| return false; | ||
| } | ||
| std::vector<uint8_t> data(plen); | ||
| if (plen > 0) | ||
| { | ||
| if (!recv_raw(data.data(), plen)) | ||
| return false; | ||
| if (masked) | ||
| for (size_t i = 0; i < plen; ++i) | ||
| data[i] ^= mask[i % 4]; | ||
| } | ||
| if (opcode == 0x08) | ||
| { | ||
| send_frame(0x08, data.data(), data.size()); | ||
| return false; | ||
| } | ||
| if (opcode == 0x09) | ||
| { | ||
| send_frame(0x0A, data.data(), data.size()); | ||
| continue; | ||
| } | ||
| if (opcode == 0x0A) | ||
| continue; | ||
| out.append(reinterpret_cast<char*>(data.data()), data.size()); | ||
| } | ||
| return true; | ||
| } | ||
|
|
||
| void close() | ||
| { | ||
| if (fd_ >= 0) | ||
| { | ||
| send_frame(0x08, nullptr, 0); | ||
| ::close(fd_); | ||
| fd_ = -1; | ||
| } | ||
| } | ||
|
|
||
| bool is_connected() const | ||
| { | ||
| return fd_ >= 0; | ||
| } | ||
|
|
||
| private: | ||
| bool send_raw(const void* d, size_t len) | ||
| { | ||
| const uint8_t* p = static_cast<const uint8_t*>(d); | ||
| size_t s = 0; | ||
| while (s < len) | ||
| { | ||
| ssize_t n = ::send(fd_, p + s, len - s, MSG_NOSIGNAL); | ||
| if (n <= 0) | ||
| return false; | ||
| s += size_t(n); | ||
| } | ||
| return true; | ||
| } | ||
|
|
||
| bool recv_raw(void* d, size_t len) | ||
| { | ||
| uint8_t* p = static_cast<uint8_t*>(d); | ||
| size_t r = 0; | ||
| while (r < len) | ||
| { | ||
| ssize_t n = ::recv(fd_, p + r, len - r, 0); | ||
| if (n <= 0) | ||
| return false; | ||
| r += size_t(n); | ||
| } | ||
| return true; | ||
| } | ||
|
|
||
| bool send_frame(uint8_t opcode, const void* payload, size_t len) | ||
| { | ||
| if (fd_ < 0) | ||
| return false; | ||
| std::vector<uint8_t> f; | ||
| f.reserve(14 + len); | ||
| f.push_back(0x80 | opcode); | ||
| if (len <= 125) | ||
| f.push_back(0x80 | uint8_t(len)); | ||
| else if (len <= 65535) | ||
| { | ||
| f.push_back(0x80 | 126); | ||
| f.push_back(uint8_t((len >> 8) & 0xFF)); | ||
| f.push_back(uint8_t(len & 0xFF)); | ||
| } | ||
| else | ||
| { | ||
| f.push_back(0x80 | 127); | ||
| for (int i = 7; i >= 0; --i) | ||
| f.push_back(uint8_t((len >> (8 * i)) & 0xFF)); | ||
| } | ||
| std::random_device rd; | ||
| std::mt19937 gen(rd()); | ||
| std::uniform_int_distribution<> dis(0, 255); | ||
| uint8_t mask[4]; | ||
| for (int i = 0; i < 4; ++i) | ||
| { | ||
| mask[i] = uint8_t(dis(gen)); | ||
| f.push_back(mask[i]); | ||
| } | ||
| const uint8_t* src = static_cast<const uint8_t*>(payload); | ||
| for (size_t i = 0; i < len; ++i) | ||
| f.push_back(src[i] ^ mask[i % 4]); | ||
| return send_raw(f.data(), f.size()); | ||
| } | ||
|
|
||
| int fd_ = -1; | ||
| }; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider extracting shared WebSocket implementation.
The MiniWebSocket class duplicates significant functionality from HaplyWebSocket in haply_plugin.cpp. While the comment on line 32 explains this is intentional for standalone operation, consider extracting the WebSocket implementation to a shared header-only utility to reduce maintenance burden and ensure bug fixes propagate to both implementations.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/plugins/haply/tools/haply_hand_tracker_printer.cpp` around lines 37 -
303, The MiniWebSocket implementation duplicates HaplyWebSocket; extract their
common functionality (handshake, framing, send_frame, recv_frame/recv_text,
send_raw, recv_raw, and connection management) into a single header-only utility
(e.g., a shared class or free functions) and have both MiniWebSocket and
HaplyWebSocket delegate to or inherit from that utility; update
MiniWebSocket::connect, send_frame, recv_text, send_raw, recv_raw, close, and
is_connected to call the shared implementation and remove duplicated code so
future fixes apply to both places.
Port the mock_haply_server.py from PR NVIDIA#268 for integration testing without hardware. Also rename environment variables from HAPLY_WEBSOCKET_HOST/PORT to HAPLY_WS_HOST/PORT for consistency with the shorter naming convention. This incorporates the remaining changes from PR NVIDIA#268 that were not included in the initial SchemaPusher rework commit. Signed-off-by: Vicky <vickybot911@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/plugins/haply/core/haply_plugin.cpp`:
- Around line 633-637: Remove the redundant ws.close() call at the end of the
outer scope: the socket is already closed inside the inner loop, so delete the
second ws.close() to avoid duplicate close calls; locate the ws variable and its
close() invocation(s) in haply_plugin.cpp (the inner loop close at the end of
the loop should remain, remove the outer-scope ws.close() that immediately
follows the loop).
- Around line 122-130: Check the return values of both setsockopt calls that set
SO_RCVTIMEO and TCP_NODELAY (the calls using fd_, &tv and &flag respectively)
and log a warning if either returns -1; use the same logger used elsewhere in
this file (or a suitable logging facility) to emit a clear message including
errno/strerror to aid debugging while keeping the calls non-fatal. Ensure you
reference the specific setsockopt calls that use SOL_SOCKET/SO_RCVTIMEO with tv
and IPPROTO_TCP/TCP_NODELAY with flag.
In `@src/plugins/haply/tests/mock_haply_server.py`:
- Around line 154-156: Replace deprecated asyncio.get_event_loop().time() calls
inside the coroutine loop with a non-deprecated clock: use
asyncio.get_running_loop().time() (or time.monotonic() to match the rest of the
file) when setting frame_start in the while True loop (reference the frame_start
assignment and the surrounding coroutine in mock_haply_server.py); apply the
same replacement to the other occurrence noted around the frame handling (the
second get_event_loop() use around line 182).
- Around line 74-78: Remove the unused dt variable and update the comment:
delete the line defining dt = 0.005 and change the preceding comment to
accurately state that vx, vy, vz are the analytical time-derivatives of the
Lissajous position functions (not a numerical derivative), referencing the
velocity calculations where vx, vy, vz are computed so reviewers can locate and
verify the change.
🪄 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: ea2c6b03-5329-446e-8120-d43f849de895
📒 Files selected for processing (2)
src/plugins/haply/core/haply_plugin.cppsrc/plugins/haply/tests/mock_haply_server.py
- Fix executable name mismatch: rename haply_hand_plugin to haply_device_plugin in CMakeLists to match plugin.yaml (Critical) - Add move operation deletions to HaplyWebSocket to prevent double-close bugs on fd_ (Major) - Add Linux platform guard in top-level Haply CMakeLists (Minor) - Check setsockopt return values and log warnings on failure - Reduce mutex hold time: parse JSON into locals first, then briefly lock to update shared state; WebSocket send moved outside lock scope - Remove redundant ws.close() call at end of io_loop - Remove unused dt variable in mock server - Replace deprecated asyncio.get_event_loop().time() with time.monotonic() in mock server Signed-off-by: Vicky <vickybot911@gmail.com>
Author
|
Thanks for the thorough review @coderabbitai! 🙏 I've addressed all findings in the latest push: 🔴 Critical:
🟠 Major:
🟡 Minor:
🔵 Nitpicks (all addressed):
Not addressed (intentional):
|
|
Tip For best results, initiate chat on the files or code changes.
Regarding the intentional non-fixes:
The PR looks good to merge. 🚀 |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/plugins/haply/core/haply_plugin.cpp`:
- Around line 200-205: Replace the loose substring check on response.find("101")
with a precise HTTP status-line validation: in the function/method where
response is inspected (the block invoking close(), std::cerr, and returning
false), verify the response starts with a proper status line containing the 101
code (e.g., begins with "HTTP/1.1 101" or generally matches
/^HTTP\/\d+\.\d+\s+101/), then only proceed; otherwise call close(), log the
truncated response as before, and return false. Use the existing response
variable and surrounding logic (the close() call and std::cerr logging) to keep
behavior consistent while tightening the check.
- Around line 652-669: The code re-acquires m_state_mutex to copy
m_state.inverse3_device_id into device_id even though that value was already
available before m_state was moved; capture inverse3_device_id into a local
(device_id) before performing the move/assignment so you can avoid the second
lock. Specifically, read m_state.inverse3_device_id into device_id while holding
m_state_mutex once (use the existing m_state_mutex, m_state and
inverse3_device_id symbols), then use that local device_id for building the JSON
and calling ws.send_text without re-locking.
🪄 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: 50f339ba-4582-4537-8332-a45aad919b47
📒 Files selected for processing (5)
src/plugins/haply/CMakeLists.txtsrc/plugins/haply/app/CMakeLists.txtsrc/plugins/haply/core/haply_plugin.cppsrc/plugins/haply/inc/core/haply_plugin.hppsrc/plugins/haply/tests/mock_haply_server.py
Comment on lines
+200
to
+205
| if (response.find("101") == std::string::npos) | ||
| { | ||
| close(); | ||
| std::cerr << "[HaplyWebSocket] Upgrade failed, response: " << response.substr(0, 80) << std::endl; | ||
| return false; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
HTTP upgrade validation could be more precise.
The check response.find("101") could theoretically match other patterns. For a test/development tool connecting to a known mock server, this is acceptable, but for robustness consider checking for "HTTP/1.1 101" at the start of the response.
♻️ Optional improvement
- if (response.find("101") == std::string::npos)
+ if (response.find("HTTP/1.1 101") == std::string::npos &&
+ response.find("HTTP/1.0 101") == std::string::npos)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (response.find("101") == std::string::npos) | |
| { | |
| close(); | |
| std::cerr << "[HaplyWebSocket] Upgrade failed, response: " << response.substr(0, 80) << std::endl; | |
| return false; | |
| } | |
| if (response.find("HTTP/1.1 101") == std::string::npos && | |
| response.find("HTTP/1.0 101") == std::string::npos) | |
| { | |
| close(); | |
| std::cerr << "[HaplyWebSocket] Upgrade failed, response: " << response.substr(0, 80) << std::endl; | |
| return false; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/plugins/haply/core/haply_plugin.cpp` around lines 200 - 205, Replace the
loose substring check on response.find("101") with a precise HTTP status-line
validation: in the function/method where response is inspected (the block
invoking close(), std::cerr, and returning false), verify the response starts
with a proper status line containing the 101 code (e.g., begins with "HTTP/1.1
101" or generally matches /^HTTP\/\d+\.\d+\s+101/), then only proceed; otherwise
call close(), log the truncated response as before, and return false. Use the
existing response variable and surrounding logic (the close() call and std::cerr
logging) to keep behavior consistent while tightening the check.
Comment on lines
+652
to
+669
| // Send a command to keep receiving updates (set zero force) — outside lock | ||
| std::string device_id; | ||
| { | ||
| std::lock_guard<std::mutex> lock(m_state_mutex); | ||
| device_id = m_state.inverse3_device_id; | ||
| } | ||
| if (!device_id.empty()) | ||
| { | ||
| json cmd; | ||
| cmd["inverse3"] = json::array(); | ||
| json dev; | ||
| dev["device_id"] = device_id; | ||
| dev["commands"]["set_cursor_force"]["values"]["x"] = 0; | ||
| dev["commands"]["set_cursor_force"]["values"]["y"] = 0; | ||
| dev["commands"]["set_cursor_force"]["values"]["z"] = 0; | ||
| cmd["inverse3"].push_back(dev); | ||
| ws.send_text(cmd.dump()); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Minor: Device ID could be captured before the move.
The device_id is re-read from m_state after it was just moved there. You could capture it before the move to avoid the extra lock acquisition, but this is a minor optimization and the current code is correct.
♻️ Optional optimization
+ std::string device_id_for_command;
+
// Brief lock to update shared state
{
std::lock_guard<std::mutex> lock(m_state_mutex);
if (has_inverse3)
{
+ device_id_for_command = inverse3_device_id; // capture before move
m_state.inverse3_device_id = std::move(inverse3_device_id);
// ...
}
// ...
}
// Send a command to keep receiving updates (set zero force) — outside lock
- std::string device_id;
- {
- std::lock_guard<std::mutex> lock(m_state_mutex);
- device_id = m_state.inverse3_device_id;
- }
- if (!device_id.empty())
+ if (!device_id_for_command.empty())
{
// ...
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/plugins/haply/core/haply_plugin.cpp` around lines 652 - 669, The code
re-acquires m_state_mutex to copy m_state.inverse3_device_id into device_id even
though that value was already available before m_state was moved; capture
inverse3_device_id into a local (device_id) before performing the
move/assignment so you can avoid the second lock. Specifically, read
m_state.inverse3_device_id into device_id while holding m_state_mutex once (use
the existing m_state_mutex, m_state and inverse3_device_id symbols), then use
that local device_id for building the JSON and calling ws.send_text without
re-locking.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Rework the Haply Inverse3 + VerseGrip plugin to plumb raw device data directly through the Isaac Teleop pipeline via FlatBuffers, replacing the previous approach of synthesizing OpenXR hand tracking data.
This addresses the feedback on PRs #267 and #268 that the plugin should not convert Haply joint angles to OpenXR hand joints, but instead push them directly through the SchemaPusher → SchemaTracker → IDeviceIOSource pipeline for consumption in Isaac Lab via TensorGroup.
Changes
1. FlatBuffers Schema (
haply_device.fbs)2. C++ Plugin — SchemaPusher side (
haply_plugin.cpp)HaplyPluginclass replacesHaplyTrackerHaplyDeviceOutputFlatBuffers at 90Hzhaply_hand_tracking_plugin.cpp/hppfiles3. C++ ITracker — SchemaTracker side (
HaplyDeviceTracker)Generic3AxisPedalTrackerpatternHaplyDeviceOutputviaSchemaTrackerserialize_all()4. Python IDeviceIOSource (
HaplyDeviceSource)HaplyDeviceOutputT→HaplyDeviceInputTensorGroupHaplyDeviceInputIndexenumArchitecture
Before (PR #267):
❌ Lossy conversion, finger joints faked at wrist position
After (this PR):
✅ Raw device data preserved end-to-end
Testing
Supersedes
This PR supersedes #267 (feat: add Haply Robotics hand tracking plugin) and #268 (test: add mock Haply SDK server).
Signed-off-by: Vicky vickybot911@gmail.com
Summary by CodeRabbit