Add teleop control pipeline for start/stop/reset via controller buttons

[rwiltz]

Description

• Add control_pipeline_builder to IsaacTeleopCfg for wiring a teleop
  control pipeline (start/stop/reset via VR controller buttons) into the
  session, with a G1 locomanipulation pipeline mapping left-hand buttons
  to DefaultTeleopStateManager signals.
• Add pipeline-aware reset on IsaacTeleopDevice so that reset() injects
  an ExecutionEvents(reset=True) pulse, causing stateful retargeters
  (hip height, SE3 last pose) to reinitialize their cross-step state.
• Add poll_control_events() helper and ControlEvents dataclass for
  scripts to consume start/stop/reset signals in a single call.

Fixes # (issue)

Type of change

• Bug fix (non-breaking change which fixes an issue)

Checklist

• I have read and understood the contribution guidelines
• I have run the pre-commit checks with ./isaaclab.sh --format
• I have made corresponding changes to the documentation
• My changes generate no new warnings
• I have added tests that prove my fix is effective or that my feature works
• I have updated the changelog and the corresponding version in the extension's config/extension.toml file
• I have added my name to the CONTRIBUTORS.md or my name already exists there

[isaaclab-review-bot]

🤖 Isaac Lab Review Bot

Summary

This PR adds a control pipeline abstraction for wiring VR controller buttons to teleop lifecycle signals (start/stop/reset), with a concrete G1 locomanipulation implementation using DefaultTeleopStateManager. It also integrates pipeline-aware reset into IsaacTeleopDevice and exposes a poll_control_events() helper for scripts to consume these signals. The overall design is sound — separating the control pipeline from the retargeting pipeline is the right call — but there is a logic gap in the STOPPED-state handling that would cause the kill button to silently fail.

Design Assessment

Design is sound. The separation of the control pipeline (GraphExecutable) from the retargeting pipeline (OutputCombiner) is clean and follows the existing builder-callable pattern to avoid deepcopy/pickle issues with C++ handles. The poll_control_events() duck-typed helper is a good approach for backward compatibility with non-IsaacTeleop devices. The one-shot reset pulse via _pending_reset is a reasonable design for injecting script-driven resets without changing the state machine.

Findings

🔴 Critical: poll_control_events ignores the STOPPED execution state — source/isaaclab_teleop/isaaclab_teleop/isaac_teleop_device.py:458-464

The poll_control_events function maps PAUSED → is_active=False and RUNNING → is_active=True, but does not handle STOPPED. When the user presses the kill button (Left B), _build_g1_control_pipeline documents that this forces STOPPED + reset. However, since STOPPED is not mapped, is_active remains None, and the caller's if ctrl.is_active is not None: guard means teleoperation_active is never set to False — the kill button only triggers a reset but does not actually stop teleoperation.

        if exec_events.execution_state == ExecutionState.PAUSED:
            result.is_active = False
        elif exec_events.execution_state == ExecutionState.RUNNING:
            result.is_active = True
        elif exec_events.execution_state == ExecutionState.STOPPED:
            result.is_active = False

Alternatively, if STOPPED should be semantically distinct from PAUSED (e.g., requiring a full session restart), consider adding a is_stopped field to ControlEvents so scripts can differentiate the two.

🟡 Warning: stop() does not clear _control_pipeline — source/isaaclab_teleop/isaaclab_teleop/session_lifecycle.py:256-283

stop() sets self._pipeline = None and self._session = None, but leaves self._control_pipeline intact. This means has_control_pipeline returns True after stop(), which could cause poll_control_events to attempt reading execution events from a torn-down session. While last_execution_events returns None when self._session is None (so no crash), the inconsistency means has_control_pipeline lies about the actual state. Consider clearing it alongside _pipeline:

self._session = None
self._pipeline = None
self._control_pipeline = None  # Clear alongside pipeline

🟡 Warning: Script-driven reset may mask control pipeline state for one frame — source/isaaclab_teleop/isaaclab_teleop/session_lifecycle.py:448-455

When _pending_reset is True, step() passes an explicit execution_events kwarg to self._session.step(). If the control pipeline simultaneously produces a state change in the same frame (e.g., user pressed the run toggle), the explicit execution_events likely overrides the control pipeline's output for that frame. The implementation preserves prev_state from the previous frame, which is reasonable, but a concurrent state transition from the control pipeline would be dropped for one frame. This is a minor timing edge case but worth a comment in the code explaining this trade-off.

🔵 Suggestion: Move poll_control_events import out of hot loop in record_demos.py — scripts/tools/record_demos.py:479

The from isaaclab_teleop import poll_control_events import is inside the while simulation_app.is_running() loop. While Python caches module imports after the first load, this is inconsistent with teleop_se3_agent.py which imports at function scope (line 189). For consistency and clarity, move the import to the top of inner_loop() or alongside the existing create_isaac_teleop_device import:

                if use_isaac_teleop:
                    ctrl = poll_control_events(teleop_interface)

with poll_control_events imported at the setup_teleop_device call site or at the top of inner_loop().

🔵 Suggestion: PR type should be "New feature", not "Bug fix" — PR description

The checklist marks this as "Bug fix (non-breaking change which fixes an issue)", but the PR primarily introduces new functionality: control_pipeline_builder, ControlEvents, poll_control_events(), and the G1 control pipeline. The CHANGELOG entry correctly separates "Added" and "Fixed" sections — the "Fixed" section about hip height reset is the bug fix component, but the bulk of the change is new feature work.

Test Coverage

• New code: Not tested. No tests are included for ControlEvents, poll_control_events, the reset injection logic, or the control pipeline builder.
• Existing test infrastructure: There appear to be no existing unit tests for the isaaclab_teleop module, so the lack of tests is consistent with the current state of the codebase. However, poll_control_events is a pure function with clear inputs/outputs that could be unit-tested with mock objects without requiring simulation.
• Gaps: The STOPPED state bug (Finding 1) would be immediately caught by a test that constructs ControlEvents from an ExecutionState.STOPPED event and asserts is_active == False.

CI Status

• ✅ pre-commit, labeler, detect changes — passing
• ⏳ Docker builds, docs, license-check, link-check — still pending

Verdict

Significant concerns

The STOPPED-state gap in poll_control_events is a correctness bug: the kill button (Left B) will trigger a reset but will not stop teleoperation, which is the opposite of its documented behavior ("force STOPPED + reset"). This should be fixed before merge. The remaining findings are minor cleanups that can be addressed at the author's discretion.

[rwiltz]

@greptile

[greptile-apps]

Greptile Summary

This PR adds message-channel-based teleop control (start/stop/reset) to the Isaac Teleop pipeline via a new control_channel_uuid field in IsaacTeleopCfg, a TeleopMessageProcessor that decodes Quest-client JSON or plain-text payloads, and a ControlEvents / poll_control_events API for script-side consumption. It also fixes the previously reported issues: reset latch is now consumed only after a successful session.step(), and the pipeline reset works even without a control channel via an independent _pending_pipeline_reset flag in TeleopSessionLifecycle.

Confidence Score: 5/5

Safe to merge — all previously reported P0/P1 issues are resolved and only P2 style observations remain.

The two critical issues from prior review threads (reset latch consumed before step completion; reset silently skipped without a control channel) are both addressed. The new _pending_pipeline_reset flag in TeleopSessionLifecycle decouples the device reset from the message processor, and _consume_pipeline_reset() is now called only after a successful session.step(). The three remaining findings are all P2: a redundant double-trigger in the teleop scripts, a misleading commented-out retargeters_to_tune lambda, and an unconsumed pipeline status output. None of these block correct runtime behavior.

scripts/environments/teleoperation/teleop_se3_agent.py and scripts/tools/record_demos.py (double-trigger pattern); source/isaaclab_tasks/isaaclab_tasks/manager_based/locomanipulation/pick_place/fixed_base_upper_body_ik_g1_env_cfg.py (commented retargeters_to_tune pattern)

Important Files Changed

Filename	Overview
source/isaaclab_teleop/isaaclab_teleop/session_lifecycle.py	Adds _pending_pipeline_reset latch to TeleopSessionLifecycle (independent of message processor) and moves latch consumption to after successful session.step(). Both previously reported P0 issues are resolved.
source/isaaclab_teleop/isaaclab_teleop/message_processor.py	New TeleopMessageProcessor handles JSON and plain-text payloads, tracks execution_state (running/paused), and exposes pending_pipeline_reset latch and should_reset per-frame pulse. Logic is clean and well-tested.
source/isaaclab_teleop/isaaclab_teleop/control_events.py	Adds ControlEvents dataclass, TELEOP_CONTROL_CHANNEL_UUID, and duck-typed poll_control_events helper. API design is clean and safe for non-IsaacTeleop devices.
source/isaaclab_teleop/isaaclab_teleop/isaac_teleop_device.py	Wires control_channel_uuid into the device and adds _on_control_command bridge. Reset now correctly propagates via both the command handler and the session lifecycle.
scripts/environments/teleoperation/teleop_se3_agent.py	Both the registered "RESET" callback and ctrl.should_reset from poll_control_events set the same flag, causing a redundant double-trigger on every reset message when using IsaacTeleop with a control channel.
source/isaaclab_tasks/isaaclab_tasks/manager_based/locomanipulation/pick_place/fixed_base_upper_body_ik_g1_env_cfg.py	The commented-out retargeters_to_tune lambda would call _build_g1_upper_body_pipeline() a second time, creating orphaned retargeters disconnected from the active pipeline.

Sequence Diagram

sequenceDiagram
    participant Script
    participant IsaacTeleopDevice
    participant TeleopSessionLifecycle
    participant TeleopMessageProcessor
    participant TeleopSession

    Script->>IsaacTeleopDevice: advance()
    IsaacTeleopDevice->>TeleopSessionLifecycle: step()

    TeleopSessionLifecycle->>TeleopSessionLifecycle: _build_execution_events()
    note right of TeleopSessionLifecycle: Check _pending_pipeline_reset OR processor.pending_pipeline_reset

    TeleopSessionLifecycle->>TeleopSession: session.step(execution_events)
    TeleopSession-->>TeleopSessionLifecycle: result (action + messages)

    opt execution_events was not None
        TeleopSessionLifecycle->>TeleopSessionLifecycle: _consume_pipeline_reset()
        TeleopSessionLifecycle->>TeleopMessageProcessor: consume_pipeline_reset()
    end

    TeleopSessionLifecycle->>TeleopMessageProcessor: process(control_messages)
    note right of TeleopMessageProcessor: Clears _should_reset, parses payload

    alt reset in payload
        TeleopMessageProcessor->>IsaacTeleopDevice: on_command(RESET)
        IsaacTeleopDevice->>IsaacTeleopDevice: fire(RESET) + self.reset()
        IsaacTeleopDevice->>TeleopSessionLifecycle: reset()
        TeleopSessionLifecycle->>TeleopMessageProcessor: request_reset()
    else start in payload
        TeleopMessageProcessor->>IsaacTeleopDevice: on_command(START)
    else stop in payload
        TeleopMessageProcessor->>IsaacTeleopDevice: on_command(STOP)
    end

    TeleopSessionLifecycle-->>IsaacTeleopDevice: action tensor
    IsaacTeleopDevice-->>Script: action tensor

    Script->>IsaacTeleopDevice: poll_control_events(teleop_interface)
    IsaacTeleopDevice-->>Script: ControlEvents(is_active, should_reset)

Loading

_{Reviews (2): Last reviewed commit: "Wire up teleop control states via messag..." | Re-trigger Greptile}

[rwiltz]

@greptile