Skip to content

session-control: add SessionControlCapable interface#259

Open
abhichothani42 wants to merge 3 commits into
vr-02-slate-base-followerfrom
vr-03-session-control-capable
Open

session-control: add SessionControlCapable interface#259
abhichothani42 wants to merge 3 commits into
vr-02-slate-base-followerfrom
vr-03-session-control-capable

Conversation

@abhichothani42

@abhichothani42 abhichothani42 commented Jun 22, 2026

Copy link
Copy Markdown
Collaborator

Summary

Adds a push-based contract for hardware that drives SessionManager state transitions (start, stop-early, re-record, stop-session). The VR session-control component (later in the stack) and the keyboard source implement it.

Changes

  • SessionControlEvent enum: semantic intents, not raw inputs. kStart means "advance" — its effect (begin / stop-and-advance / skip-reset) is resolved by the host's event handler based on the current session phase.
  • SessionControlCapable mixin: set_callbacks(on_event, on_disconnect), start(), stop(). The source owns its own threading and keeps callbacks cheap; disconnect is a one-shot end-of-session signal.
  • Header-only interface, no dependencies.

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 22, 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 public session-control interface in the hardware layer to allow a push-based source (e.g., keyboard/VR input) to emit high-level session intents to the runtime session orchestration.

Changes:

  • Introduces SessionControlEvent to represent session-level control intents (start, stop-early, re-record, stop session).
  • Adds SessionControlCapable interface with callback registration plus start()/stop() lifecycle for event production.

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

Comment thread include/trossen_sdk/hw/session_control/session_control_capable.hpp Outdated
Comment thread include/trossen_sdk/hw/session_control/session_control_capable.hpp Outdated
Comment thread include/trossen_sdk/hw/session_control/session_control_capable.hpp 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.

The interface is header-only with nothing compiling it yet, so the "tested on hardware" box doesn't hold. Uncheck it.

@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.

  • The disconnect signal is supposed to fire only once. Right now each component has to get that right on its own. Can the base class guarantee it instead?
  • Please note which thread the callbacks run on, and that stop() finishes the worker before the callbacks go away. Also, kStart means different things at different points, so it is worth explaining at the definition. The design itself is good, keep it small.
  • This is just an interface with nothing using it yet, so "tested on hardware" cannot be true here. Worth unchecking.

virtual ~SessionControlCapable() = default;

/**
* @brief Install the event and disconnect callbacks.

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.

These functions are only safe to call from the main loop. This new design calls in from a separate thread, so the header should say clearly what a callback is allowed to call. Otherwise someone will call stop_episode() from the wrong place and cause a clash.

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, the header now spells out that callbacks run on the source's thread and must not call the SessionManager episode methods directly

@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.

changes requested

@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.

https://chat.google.com/room/AAQA0xpz-dM/TWmxsM3rgN0/TWmxsM3rgN0?cls=10

Check this for a suggestion of the session control rework

@abhichothani42

Copy link
Copy Markdown
Collaborator Author

"The disconnect signal is supposed to fire only once. Right now each component has to get that right on its own. Can the base class guarantee it instead?"
-> Moved it into the base now signal_disconnect() fires at most once per drop and arm_disconnect() re-arms on reconnect, so no component re-implements it.

Please note which thread the callbacks run on, and that stop() finishes the worker before the callbacks go away. Also, kStart means different things at different points, so it is worth explaining at the definition.
-> Documented at the enum definition.

-> Unchecked Tested on hardware

About session control rework
https://chat.google.com/room/AAQA0xpz-dM/TWmxsM3rgN0/TWmxsM3rgN0?cls=10
-> Done, added post_event/attach_control/drain to SessionManager in pr #271, and all three demos now go through it instead of touching the session from the callback thread. Details in that PR.

Add a push-based contract for hardware that drives SessionManager state transitions (start, stop-early, re-record, stop-session).

- SessionControlEvent enum: semantic intents, not raw inputs. kStart means "advance" and its effect is resolved by SessionManager based on the current phase (idle/recording/resetting).
- SessionControlCapable mixin: set_callbacks(on_event, on_disconnect), start(), stop(). The source owns its own threading and keeps callbacks cheap; disconnect is a one-shot end-of-session signal.

Header-only interface with no dependencies. The VR session-control component (later in the stack) and the keyboard source implement it.
@abhichothani42
abhichothani42 force-pushed the vr-02-slate-base-follower branch from d072bda to efc7944 Compare July 17, 2026 14:48
@abhichothani42
abhichothani42 force-pushed the vr-03-session-control-capable branch from a1887fa to 3e45218 Compare July 17, 2026 14:48
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