Skip to content

feat: post_event/attach_control queue for thread-safe session control#271

Open
abhichothani42 wants to merge 3 commits into
vr-03-session-control-capablefrom
vr-03b-session-manager-control
Open

feat: post_event/attach_control queue for thread-safe session control#271
abhichothani42 wants to merge 3 commits into
vr-03-session-control-capablefrom
vr-03b-session-manager-control

Conversation

@abhichothani42

@abhichothani42 abhichothani42 commented Jul 10, 2026

Copy link
Copy Markdown
Collaborator

Summary

SessionControlCapable sources (VR buttons, keyboard, future foot pedals, …) each run on their own thread and fire callbacks from that thread. But SessionManager's episode methods (start_episode, stop_episode, discard_*, shutdown) are single-threaded. Previously the VR demos called stop_episode() directly from the VR reader thread, racing the main loop that owns the session.

This PR adds a thread-safe event queue so a control source can only post an intent; SessionManager drains it and mutates the session only on the orchestrating thread.

Changes

  • Add SessionManager::post_event(SessionControlEvent) — thread-safe enqueue, callable from any thread; never touches session state.
  • Add SessionManager::attach_control(SessionControlCapable&) — installs post-event callbacks, starts the source, and remembers it so shutdown() stops (joins) it before teardown.
  • Add private drain_control_events(ControlPhase) — applies queued events on the main thread, phase-aware: kStart advances + skips reset while recording / resumes while resetting; kStopEarly stops the episode; kRerecord; kStopSession and disconnect end the session.
  • Drain the queue inside monitor_episode() and wait_for_reset(); shutdown() stops attached sources first so no queued event can drive a session being torn down.
  • Add test_session_manager_control.cpp (11 tests): queue behavior, attach/shutdown lifecycle, phase-aware mapping, and disconnect-fires-once through attach_control.

Test Plan

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

Breaking Changes

None. The new API is additive. With no attached control sources the queue stays empty, so existing keyboard-only demos (monitor_episode/wait_for_reset) behave exactly as before.

Related Issues

abhichothani42 commented Jul 10, 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

This PR introduces a thread-safe “session control” event queue in SessionManager so that push-based control sources (VR buttons, etc.) can post intents from their own threads while the session state continues to be mutated only from the orchestrating loop.

Changes:

  • Added SessionManager::post_event() (thread-safe enqueue) and SessionManager::attach_control() (installs callbacks, starts/stops sources via shutdown).
  • Implemented phase-aware draining of queued control events from monitor_episode() and wait_for_reset().
  • Added a new unit test suite (test_session_manager_control) and wired it into the test build.

Reviewed changes

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

File Description
src/runtime/session_manager.cpp Implements control-source lifecycle shutdown, event enqueueing, and phase-aware draining in episode/reset loops.
include/trossen_sdk/runtime/session_manager.hpp Exposes the new public APIs and adds private queue/flags used by control-event draining.
tests/test_session_manager_control.cpp Adds unit coverage for queueing/draining behavior and attach/shutdown lifecycle.
tests/CMakeLists.txt Builds and registers the new test binary with CTest.

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

Comment thread src/runtime/session_manager.cpp Outdated
Comment thread include/trossen_sdk/runtime/session_manager.hpp
Comment thread src/runtime/session_manager.cpp Outdated
Comment thread src/runtime/session_manager.cpp
* wait_for_reset() loop, so drive the session with those rather than
* start_async_monitoring().
*/
void attach_control(hw::session_control::SessionControlCapable& source);

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.

Can we have a shared_ptr here as we already have those for all hardware components. A raw pointer has the exact problems as mentioned in the note. Using shared pointer would gaurantee that the source outlives the session manager.

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.

dead938
Done. attach_control now takes std::shared_ptr and the manager holds it in control_sources_, so the source is guaranteed to outlive the manager's use of it.

@abhichothani42
abhichothani42 force-pushed the vr-03b-session-manager-control branch from a69375d to dead938 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