Skip to content

feat(remote_device_bridge): remote plugin-session wire codec - #1321

Open
jkim89 wants to merge 7 commits into
jonghyunk/pr00-pusherio-carveoutfrom
jonghyunk/pr01-wire-codec
Open

feat(remote_device_bridge): remote plugin-session wire codec#1321
jkim89 wants to merge 7 commits into
jonghyunk/pr00-pusherio-carveoutfrom
jonghyunk/pr01-wire-codec

Conversation

@jkim89

@jkim89 jkim89 commented Oct 22, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Summary

First layer of the remote plugin session: a wire codec so a plugin can drive an IPluginSession that lives in another process.

What changes

  • New Teleop::remote_plugin_session library (src/plugins/remote_device_bridge/session/) with wire_codec.{hpp,cpp} as the single serialization point: 24-byte little-endian header + payload, 14 frame types (HELLO/ACK, OPEN_*, CLOSE, hand/schema/wrist samples, HEARTBEAT, ERROR, PING/PONG)
  • Schema samples carry the plugin's FlatBuffer bytes opaquely
  • New option BUILD_PLUGIN_REMOTE_DEVICE_BRIDGE (default ON, skipped on Windows), wired in src/plugins/CMakeLists.txt

What does not change

No plugin or runtime behavior; the library links only pusherio, never the OpenXR loader.

Testing

test_wire_codec.cpp (12 cases: round-trips, malformed input, contiguous builders). Fresh build: ctest 266/266 (pr00: 254/254).

Depends on

#1320

@coderabbitai

coderabbitai Bot commented Oct 22, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

Run configuration

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

@jkim89 jkim89 changed the title feat(remote_device_bridge): add the remote plugin-session wire codec feat(remote_device_bridge): remote plugin-session wire codec Oct 22, 2026
@jkim89 jkim89 mentioned this pull request Oct 22, 2026
Open
@jkim89
jkim89 requested a review from tengw-nv October 23, 2026 21:30
@jkim89
jkim89 marked this pull request as ready for review October 27, 2026 22:47
tengw-nv
tengw-nv reviewed Oct 29, 2026
View reviewed changes
Comment thread src/plugins/remote_device_bridge/session/inc/remote_plugin_session/wire_codec.hpp Outdated

@tengw-nv tengw-nv Oct 29, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will recommend breaking the wire_codec into smaller pieces.

  1. may call this remote_plugin_protocol or remote_frame_builder top level?
  2. I am not sure we need so many types here. Maybe just keep the Frame type sufficient. The rests are utility functions around the frame builders and frame parsers for different types of data input and output. So the enum and string conversion are just part inside the builder and parser utility function.

@jkim89 jkim89 Oct 30, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in the "split frame layer from payload codecs" commit:

  • protocol_frame.* (header/Frame/stamp/split),
  • protocol_payloads.* (builders/parsers),
  • namespace
    remote_plugin_protocol;

one-field payloads (open-hand, error, ping) are now plain values in the builder/parser signatures and to_string(FrameType) is gone. I kept structs only for the multi-field payloads (Hello, HelloAck, the three samples, Pong) since the parsers have to return several fields

jkim89 added a commit that referenced this pull request Oct 30, 2026
@jkim89 @claude
wire codec: split frame layer from payload codecs
7fb6b80 
Addresses the #1321 review: the codec becomes remote_plugin_protocol.
- protocol_frame.{hpp,cpp}: header constants, FrameType, FrameHeader, Frame,
  DecodeError, encode_header/decode_header, encode_frame, stamp_header,
  split_frame.
- protocol_payloads.{hpp,cpp}: one named builder/parser pair per frame type
  (encode_hello/decode_hello, ...), plus the sample frame builders.
- protocol_io.hpp (private): the little-endian Writer/Reader shared by both.
- Single-field payload structs are gone: OPEN_HAND, ERROR and PING build from
  and parse to XrHandEXT, std::string and int64_t.
- to_string(FrameType) is private to protocol_payloads.cpp, where parsers use
  it to name the frame in DecodeError messages.
- WristSample carries XrHandEXT instead of bool is_left; its wire byte keeps
  its meaning (1 = left).
The 24-byte header and every payload layout are unchanged.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Jonghyun Kim <jonghyunk@nvidia.com>
@jkim89
jkim89 force-pushed the jonghyunk/pr01-wire-codec branch from fe5f90f to 6452f88 Compare October 30, 2026 19:12
jkim89 and others added 7 commits October 30, 2026 14:34
@jkim89
feat(remote_device_bridge): add the remote plugin-session wire codec
caa52d5 
Add the Teleop::remote_plugin_session library with its only
serialization point: little-endian frames of a 24-byte header plus
payload, 14 frame types (HELLO/ACK, OPEN_*, CLOSE, hand/schema/wrist
samples, HEARTBEAT, ERROR, PING/PONG). Schema samples carry the
plugin's FlatBuffer bytes opaquely. The library links only pusherio,
never the OpenXR loader, and is gated by
BUILD_PLUGIN_REMOTE_DEVICE_BRIDGE (default ON, skipped on Windows).

Signed-off-by: Jonghyun Kim <jonghyunk@nvidia.com>
@jkim89 @claude
rebase onto pr00 review
90af869 
PluginSessionRequirements::wrist_tracking_pull is now an
optional<WristTrackingSourceConfig>, so HELLO carries the wrist config
(mode + both aim-to-wrist poses, same layout as OPEN_WRIST) behind its
presence byte instead of a bool. HELLO proto_version goes 1 -> 2 and
decode_hello rejects older versions, whose payload layout differs. The
24-byte frame header and kProtocolVersion are unchanged.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Jonghyun Kim <jonghyunk@nvidia.com>
@jkim89 @claude
wire codec: split frame layer from payload codecs
7e974fc 
Addresses the #1321 review: the codec becomes remote_plugin_protocol.
- protocol_frame.{hpp,cpp}: header constants, FrameType, FrameHeader, Frame,
  DecodeError, encode_header/decode_header, encode_frame, stamp_header,
  split_frame.
- protocol_payloads.{hpp,cpp}: one named builder/parser pair per frame type
  (encode_hello/decode_hello, ...), plus the sample frame builders.
- protocol_io.hpp (private): the little-endian Writer/Reader shared by both.
- Single-field payload structs are gone: OPEN_HAND, ERROR and PING build from
  and parse to XrHandEXT, std::string and int64_t.
- to_string(FrameType) is private to protocol_payloads.cpp, where parsers use
  it to name the frame in DecodeError messages.
- WristSample carries XrHandEXT instead of bool is_left; its wire byte keeps
  its meaning (1 = left).
The 24-byte header and every payload layout are unchanged.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Jonghyun Kim <jonghyunk@nvidia.com>
@jkim89 @claude
review round 1 (pr01)
7c320e9 
Applied the fact-checked (VERIFIED) findings on code this branch adds:
- Frame layer: kHeaderSize is derived from its fields; encode_header is a
  private detail that writes in place with named offsets; stamp_header takes
  the frame vector and only touches the header; decode_header's type bound
  follows kLastFrameType; FrameHeader::type has no default; split_frame's doc
  says the payload keeps the message buffer.
- PONG t3 stamping moves out of the frame layer into set_pong_t3, called
  where the pong is sent.
- Payload layer: FrameType comments keep only direction (layouts live with
  the builders); parsers pass the frame name to the reader directly, so the
  unreachable to_string is gone; builders share build_payload; HELLO accepts
  exactly kHelloVersion; history and future-tense comments go; size
  constants used only by tests move into the test; own private header first.
- Build: the remote_device_bridge block sits with the other gated plugins;
  the session CMake comment duplicating AGENTS.md is gone; the test target
  does not re-link pusherio; AGENTS.md describes the split accurately.
- Tests: no constant-against-literal checks; header goldens go through
  encode_frame + stamp_header.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Jonghyun Kim <jonghyunk@nvidia.com>
@jkim89 @claude
rebase onto pr00 review: retire OPEN_WRIST
92127c0 
pr00's sessions build the wrist source at construction from
PluginSessionRequirements::wrist_tracking_pull, which HELLO now carries in
full. OPEN_WRIST would only repeat that config, so frame type 6 is
unassigned and the parser rejects it; the wrist config codec remains as
part of HELLO. Other frame numbers, the 24-byte header and
kProtocolVersion are unchanged; HELLO version 2 already fences off older
peers.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Jonghyun Kim <jonghyunk@nvidia.com>
@jkim89 @claude
review round 2 (pr01)
c41c822 
Applied the fact-checked (VERIFIED) findings on code this branch adds:
- FrameHeader drops the version field (always kProtocolVersion) and
  value-initializes type to 0, which no frame uses.
- Hello drops proto_version; encode_hello writes kHelloVersion and
  decode_hello compares against it.
- Writer/Reader gain boolean(); encoders no longer spell out "? 1 : 0" and the
  parsers' to_bool helper is gone.
- store_le lives once in protocol_io.hpp; set_pong_t3 uses it.
- The ERROR comment no longer describes bridge behaviour; AGENTS.md keeps
  just the rule.
- Tests use kHeaderSize instead of 24; the nested test CMakeLists drops
  cmake_minimum_required.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Jonghyun Kim <jonghyunk@nvidia.com>
@jkim89 @claude
review round 3 (pr01)
c43a9c9 
Applied the fact-checked (VERIFIED) findings on code this branch adds:
- decode_header takes a std::span and checks its length; valid frame types
  come from an exhaustive switch, so no kLastFrameType or unassigned-type
  constant has to be kept in step with the enum.
- Header offsets are tied to the layout with a static_assert.
- Writer/Reader share store_le/load_le instead of three hand loops each.
- PONG fields are named ping_sent_ns / ping_received_ns / pong_sent_ns and
  set_pong_t3 becomes set_pong_sent_time.
- Own private header first in both protocol sources.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Jonghyun Kim <jonghyunk@nvidia.com>
@jkim89
jkim89 force-pushed the jonghyunk/pr01-wire-codec branch from 6452f88 to c43a9c9 Compare October 30, 2026 21:42
@jkim89

jkim89 commented Oct 30, 2026

Copy link
Copy Markdown
Contributor Author

Update (Oct 30): rebased onto the reviewed #1320. Per the review comment, wire_codec is split into a frame layer (header/Frame/stamp/split) and payload codecs; one-field payload structs are plain values; to_string(FrameType) is gone (only error messages used it). HELLO is now v2 and carries the optional wrist config; OPEN_WRIST (type 6) is removed; the 24-byte header and all other payloads are unchanged. Run through the review skill locally (3 rounds) before pushing.

This branch was successfully deployed

1 active deployment
dev — c43a9c9c Deployed Oct 30, 2026 by jkim89 via publish-wheel #6344
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tengw-nv tengw-nv tengw-nv left review comments

Assignees

@jkim89 jkim89

Labels

NVIDIA

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@jkim89 @tengw-nv @ivany-nv