Repository navigation
Conversation
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the
 |
jkim89
marked this pull request as ready for review
October 27, 2026 22:48
jkim89
force-pushed
the
jonghyunk/pr04-remote-plugin-session
branch
from
October 30, 2026 19:12
af64382 to
aca3660
Compare
jkim89
force-pushed
the
jonghyunk/pr05-remote-device-bridge
branch
from
October 30, 2026 19:12
7c0c89d to
145356f
Compare
remote_device_bridge runs next to the OpenXR runtime and serves RemotePluginSession clients over TCP and optionally WebSocket. It forks one process per client, each owning one OpenXRPluginSession, because the OpenXR loader allows one XrInstance per process. It applies latest-wins coalescing and a staleness guard, refuses an over-cap schema channel or an already-owned hand per channel, answers a full bridge within 500 ms, logs a 5 s status line and can write a per-sample stats CSV. HandInjector errors now carry the XrResult code for that refusal. Signed-off-by: Jonghyun Kim <jonghyunk@nvidia.com>
Adapt the bridge server, app and server tests to the new APIs: - codec: remote_plugin_protocol (protocol_frame/protocol_payloads) with the named encode_*/decode_* functions; numeric frame types replace to_string - sessions own the pull channel and wrist source: OPEN_PULL uses session->pull_channel(), WRIST_SAMPLEs stream from wrist_tracking_source() when HELLO declared wrist_tracking_pull; OPEN_WRIST handling removed - HELLO proto_version branching removed: every accepted client is PINGed, one-way only on no PONG or --clock-source=one-way - schema channels keep collection_id/max_flatbuffer_size (no config()) - app builds sessions with make_openxr_plugin_session_factory - default ports: remote_plugin_session::kDefaultBridgePort and kDefaultBridgeWsPort in remote_address.hpp set the bridge's --listen and --ws-listen defaults; parse_bridge_remote gives clients the default port of the address's scheme (tested in test_remote_address.cpp) - includes for tcp_transport.hpp and transport.hpp - latency_stats reads offset_source() as the estimator's Source enum - tests: fake session owns its pull channel; HELLO v0 test becomes a --clock-source=one-way test; status line now expects wrist samples - AGENTS.md: session factory and teardown-order lines Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Jonghyun Kim <jonghyunk@nvidia.com>
- Move latency_stats (SampleCsv, format_clock_stats, index_curl_m) from
session/ into server/ under namespace remote_device_bridge, with its test
- SampleCsv: unique_ptr FILE, one write(2) per row on O_APPEND, checked
header write, private format(), no --stats-csv flag name in errors
- format_clock_stats takes ages by value; schema_cap_error returns optional
- BridgeOptions: named default ports, optional app_name/stats_csv/
max_sample_age (chrono); ws_port() returns optional
- SessionFactory maps an app name to a core::PluginSessionFactory; main
passes make_openxr_plugin_session_factory directly
- ClientSession handshakes in its constructor (HandshakeRefused, transport
shut down on throw); send() returns void; reader reports malformed frames
through fail(); second HELLO is a session error
- Separate hand/schema channel structs, named counters, derived wrist
source and hand side, claim() returns optional owner, schema channel
created before insertion
- reap_finished()/stop_and_reap_all(); HandOwners in a unique_ptr with a
munmap deleter; fork failures log strerror; fatal child exits 1
- Log and usage text: plugin session wording, no WARN in message, usage
defaults from BridgeOptions{}, one strict integer parser for all flags
- Drop redundant Threads link, CMake/AGENTS duplicate comments, dtor
boilerplate, constant-only test assertions and test timing prints
Signed-off-by: Jonghyun Kim <jonghyunk@nvidia.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Applied the fact-checked (VERIFIED) findings on code this branch adds: - The bridge parent opens --stats-csv once; forked clients inherit the descriptor, so the static SampleCsv::create step and per-client reopen are gone. Row formatting is a free function in the .cpp. - The bridge-full HELLO_ACK is built where it is sent, not kept as a member. - ClientSession's constructor drops a try/catch its members already cover. - Removed the bridge's schema size check and its comment: OpenXRSchemaPushChannel::push_buffer already rejects oversized samples. - The refusal text no longer calls the configurable cap a runtime limit; AGENTS.md and the usage text stop restating PingScheduler constants. - Dropped an unused include(GNUInstallDirs). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Jonghyun Kim <jonghyunk@nvidia.com>
Applied the fact-checked (VERIFIED) findings on code this branch adds: - SampleCsv holds a plain O_APPEND descriptor instead of a FILE; write() reports a short or failed row and the bridge warns once. - The pull channel's pointer and id are one optional member. - HandOwners lives in a detail namespace instead of a public nested type. - reap_finished drops a client whose waitpid fails, so it stops counting against max_clients, and frees its hands. - BridgeServer's option errors name the BridgeOptions fields, not CLI flags. - The refusal-child doc comment sits on send_bridge_full. - One CMake guard covers both remote_device_bridge test directories. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Jonghyun Kim <jonghyunk@nvidia.com>
Smoke-test follow-up: a refused HELLO now tells the client why. - refuse_handshake sends HELLO_ACK(false) carrying the same reason the bridge logs before the connection closes. A HELLO v1 client already decodes HELLO_ACK, so it reports "bridge rejected the session: bad HELLO (protocol: unsupported HELLO version 1)" instead of "connection lost while waiting for HELLO_ACK". - New test: a v1 HELLO over TCP and over WebSocket gets HELLO_ACK(false) with the logged reason, then the close; the bridge keeps serving. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Jonghyun Kim <jonghyunk@nvidia.com>
Smoke-test follow-up: with wrist pull on, a client hang-up skipped the client's last CLOSE frames and the "client disconnected" line. - Cause: a failed send set m_done, and run() and the reader stopped at once. With wrist pull the bridge sends WristSamples at the tick rate, so the first send after the hang-up nearly always failed before run() had applied the CLOSEs the reader had queued or seen the end of the stream. - A failed send now only drops later sends. run() still ends through the reader's close, so the CLOSEs apply and the disconnect is logged as on the no-wrist path. A stalled link that never closes ends after 500 ms with "sending to the client failed; closing" (before, it ended silently). - New test: with wrist pull and a slow pull update, a hang-up logs "(pull) closed" and "client disconnected"; the fake pull channel gained a configurable update delay to hold the run thread inside tick(). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Jonghyun Kim <jonghyunk@nvidia.com>
jkim89
force-pushed
the
jonghyunk/pr04-remote-plugin-session
branch
from
October 30, 2026 21:42
aca3660 to
d1f8f97
Compare
jkim89
force-pushed
the
jonghyunk/pr05-remote-device-bridge
branch
from
October 30, 2026 21:42
c0d2395 to
69a0f6f
Compare
Contributor
Author
|
Update (Oct 30): rebased onto #1320–#1323. The bridge holds non-owning pointers and has no OPEN_WRIST handling. Two fixes from a loopback smoke run: a rejected HELLO now gets HelloAck{false, reason} before the close so a v1 client sees why; a failed send no longer tears the session down before the client's CLOSE frames are applied (it only drops later sends, and a stalled link ends after 500 ms with a warning). Synthetic loopback on the new wire format: 6/6 passed, age p99 0.4 ms, old v1 sender correctly rejected. |
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
remote_device_bridge: runs next to the OpenXR runtime and servesRemotePluginSessionclients.What changes
Teleop::remote_device_bridge_serverwith an injected session factory, plus theremote_device_bridgeapp andplugin.yamlXrInstanceper process); hand ownership shared via aMAP_SHAREDpage--max-schema-bytes, hand owned elsewhere); "bridge full" within 500 ms--ws-listen,--clock-source, a 5 s status line with age/RTT,--stats-csvHandInjectorerrors now carry theXrResultcodeWhat does not change
Local (non-remote) plugins and
OpenXRPluginSession.Testing
13 end-to-end cases running the real fork-per-client bridge over loopback against a fake session, plus
bridge_limits/latency_statsunits. Fresh build: ctest 316/316.Depends on
#1323