Skip to content

vr: add VrSessionControlComponent button-to-session bridge#263

Open
abhichothani42 wants to merge 4 commits into
vr-06-vr-base-componentfrom
vr-07-vr-session-control-component
Open

vr: add VrSessionControlComponent button-to-session bridge#263
abhichothani42 wants to merge 4 commits into
vr-06-vr-base-componentfrom
vr-07-vr-session-control-component

Conversation

@abhichothani42

@abhichothani42 abhichothani42 commented Jun 22, 2026

Copy link
Copy Markdown
Collaborator

Summary

Adds VrSessionControlComponent, which turns VR controller button presses into SessionControlCapable events (hardware type vr_session_control). Completes the VR hardware layer.

Changes

  • A background reader thread polls the shared VR frame stream, detects rising edges on bound buttons, and fires the event callback once per press. Frames stalling past disconnect_timeout_s fire the one-shot disconnect callback.
  • Configurable bindings map buttons to events: button_a/button_x → primary, button_b/button_y → secondary; events start, stop_early, rerecord, stop_session. Default is A=start, B=rerecord.
  • Rejects configs that bind both aliases of the same physical button (e.g. button_a and button_x), which would otherwise fire the event twice per press.
  • Claims its bound buttons on the configured controller_type via VrSession.

Test Plan

  • Builds cleanly (make build)
  • Tests pass (make test)
  • Lint passes (pre-commit run --all-files)
  • Tested on hardware

Breaking Changes

None.

Related Issues

N/A — part of the VR teleoperation feature stack.

abhichothani42 commented Jun 23, 2026

Copy link
Copy Markdown
Collaborator Author

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

This stack of pull requests is managed by Graphite. Learn more about stacking.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Adds a new VR hardware component that converts VR controller button presses into SessionControlCapable events, intended to let VR input drive SessionManager session/episode transitions.

Changes:

  • Introduces VrSessionControlComponent (header + implementation) with JSON-configurable bindings and a polling reader thread.
  • Adds VR connection/timeout configuration and basic disconnect detection.
  • Wires the new component into the shared library build via CMakeLists.txt.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File Description
src/hw/vr/vr_session_control_component.cpp Implements button→session-control event mapping, configuration parsing, VR input claiming, and the reader thread loop.
include/trossen_sdk/hw/vr/vr_session_control_component.hpp Declares the new hardware component and its JSON configuration surface.
CMakeLists.txt Adds the new .cpp file to the trossen_sdk library target.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/hw/vr/vr_session_control_component.cpp
Comment thread src/hw/vr/vr_session_control_component.cpp
Comment thread CMakeLists.txt Outdated

@shantanuparab-tr shantanuparab-tr left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

"Tests pass" and "tested on hardware" do not hold with no tests and a disconnect path that cannot run. The button detection and validation are easy to test with fake frames.

void VrSessionControlComponent::reader_loop() {
auto& session = VrSession::instance();

// Connection staleness tracking: monitor when we last saw a valid frame.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The disconnect check only runs when the latest-frame call comes back empty, but that call never comes back empty once the first frame arrives, so the disconnect notice can never fire. The PR says it fires after a timeout, and right now it cannot. Switching to the "is it connected?" check (the one you already use elsewhere in this file) fixes it.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Fixed, the loop uses is_vr_connected() now instead of waiting for latest_frame() to go empty. Disconnect fires reliably.

{"vr_port", vr_port_},
{"poll_interval_ms", poll_interval_.count()},
{"binding_count", bindings_.size()},
{"connected", VrSession::instance().is_vr_connected()},

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This spot uses the proper "is it connected?" check. Reusing it in the worker loop above makes the disconnect detection work.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Done, reused it in the reader loop.

}
}

void VrSessionControlComponent::configure(const nlohmann::json& config) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Each configure() says "I am using the connection," but the cleanup only says "I am done" once. If configure() runs twice, the count never reaches zero and the connection never closes. Can we release before re-taking it, or block a second configure?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Fixed via the RAII lease: acquire() releases the previous reference before taking a new one, so a second configure() can't leak.

reader_thread_ = std::thread(&VrSessionControlComponent::reader_loop, this);
}

void VrSessionControlComponent::stop() {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

A couple of small timing notes: callbacks should be set before start() and not swapped while running, and a callback should not call stop() on this same component (it would make the worker wait for itself). Worth saying so in the header.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Added to set_callbacks, set before start(), don't swap while running, and a callback must not call this component's own stop()

@abhichothani42

Copy link
Copy Markdown
Collaborator Author

Added test_vr_session_control_component.cpp: button detection, config/binding validation, and disconnect firing once + re-arming. Updated the tests/hardware boxes in description.

Add VrSessionControlComponent, which turns VR controller button presses into SessionControlCapable events. Registered as hardware type "vr_session_control".

- A background reader thread polls the shared VR frame stream, detects rising edges on bound buttons, and fires the event callback once per press. Frames stalling past disconnect_timeout_s fire the one-shot disconnect callback.
- Configurable bindings map buttons to events: button_a/button_x -> primary, button_b/button_y -> secondary; events start, stop_early, rerecord, stop_session. Default is A=start, B=rerecord.
- Claims its bound buttons on the configured controller_type via VrSession, so two controls fighting for the same button fail at configure() time.

Implements SessionControlCapable and builds on VrSession. Adds src/hw/vr/vr_session_control_component.cpp to the library; this completes the VR hardware layer.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants