Add generic joystick support as input by tomasz-lewicki · Pull Request #110 · amazon-far/holosoma

tomasz-lewicki · 2026-05-14T01:39:50Z

This PR adds joystick controller as an input for holosoma_inference. Tested with Xbox controller.

kingb

Thanks for putting this together — the host-side evdev path is well-isolated and the threading model looks right. Net: approve with one must-fix and a handful of smaller items.

Single biggest concern, calling out at the top because it isn't obvious from the diff:

⚠️ Behavior change in `--task.use-joystick`

Before this PR, --task.use-joystick resolved to velocity_input = state_input = "interface" — i.e. the SDK wireless controller (the dongle/controller shipped with Unitree G1, Booster T1, etc.). After this PR it resolves to "joystick" → UsbJoystickInput reading /dev/input/event*.

For a user running on a real G1 with the Unitree controller and no USB gamepad, the same command they ran yesterday will now error out with RuntimeError: No USB joystick found via evdev. This is a silent semantic flip of a user-facing flag, and I want to make sure it's intentional before merge.

Questions / suggestions:

Is this intentional? If the goal is "on Thor, default to a host-side gamepad," that's a reasonable design — but it should be a deliberate decision, not a side effect.
The old behavior is still reachable via --task.velocity-input interface --task.state-input interface, but nothing surfaces that to the user.
Suggestion — keep the new "joystick" source, but pick one of:
- Have --task.use-joystick first try UsbJoystickInput, and if no USB gamepad is present, fall back to "interface". (Mirrors the existing darwin → keyboard fallback in _init_joystick_handler.)
- Add an explicit --task.use-sdk-controller shortcut for the old behavior, keep --task.use-joystick meaning what it now means, and add a release note.
- Or rename the new shortcut to --task.use-usb-joystick and leave --task.use-joystick pointing at "interface".
Whatever we pick, the PR description and the TaskConfig.use_joystick docstring should call out the change.

Must-fix

Test will fail to collect on macOS — usb_joystick.py does import evdev at module top level, and evdev only builds on Linux. The new test_joystick_maps_to_usb_joystick imports the module unconditionally. Wrap with pytest.importorskip("evdev") or defer the import evdev inside usb_joystick. (Inline.)
Negative device_index is silently accepted and indexes from the end. (Inline.)

Should-fix

Device identity is fragile across replugs — _list_gamepads() orders by evdev.list_devices() (alphabetical over /dev/input/event*), so today's index 0 may be tomorrow's index 1. dev.uniq (potentially populated with a serial / BT MAC) is the right primitive when available; see inline.
start() is a no-op because the thread is started in __init__ — breaks the protocol's lifecycle contract.
_KEY_LABEL doesn't include F1 (keyboard fallback, fine) or lowercase x (looks like a typo in JOYSTICK_COMMANDS). Pre-existing in joystick.py, worth a follow-up.

Out-of-scope / future-CR candidates

STICK_DEADZONE = 0.1 now duplicated here + interface.py. Pull into inputs/impl/joystick.py next to JOYSTICK_COMMANDS.
joystick_type: str = "xbox" is unused — either wire it through (per-controller layout) or drop it.
The stick-suppression-while-button-held behavior is inherited from InterfaceInput; the rationale there was that the SDK wire format couples sticks + keys in one frame, which doesn't apply to evdev. Current behavior freezes commanded velocity mid-chord — benign but not necessarily what the user wants. Worth re-examining together with InterfaceInput.

Nit

PR description says "Tested with Xbox controller" — please add the model (Series X? 360? wired vs wireless via dongle?). Different generations expose different evdev codes especially on triggers.

kingb · 2026-05-14T20:49:36Z

        flag_name: str | None = None
        if self.use_joystick:
-            shortcut = "interface"
+            shortcut = "joystick"


Behavior change — see the top-level review comment. This single character change ("interface" → "joystick") silently flips the meaning of --task.use-joystick for every existing G1/T1 user from "SDK wireless controller" to "USB gamepad on the host." Please confirm intent and either:

add a fallback so "joystick" reverts to "interface" when no USB gamepad is present, or

preserve --task.use-joystick → "interface" and add a separate --task.use-usb-joystick shortcut for the new behavior.

Whichever direction we pick, please update the use_joystick docstring just above to spell it out.

kingb · 2026-05-14T20:49:36Z

+    if source == "joystick":
+        from holosoma_inference.inputs.impl.usb_joystick import UsbJoystickInput
+
+        return UsbJoystickInput(device_index=policy.config.task.joystick_device)


Lazy-importing UsbJoystickInput here is the right call — it keeps macOS users (where evdev doesn't build) able to import this module as long as they don't request "joystick". ✓

One thing to watch: the test in inputs/tests/test_providers.py does from holosoma_inference.inputs.impl import usb_joystick at the top of the test method, which will fail to collect on macOS regardless. Suggest pytest.importorskip("evdev") there.

kingb · 2026-05-14T20:49:36Z

+
+import threading
+
+import evdev


macOS test-collection failure: importing this module on macOS crashes at this line because evdev only builds on Linux (it links against <linux/input.h>). The lazy import in inputs/__init__.py keeps production code safe, but tests/test_providers.py::test_joystick_maps_to_usb_joystick does from holosoma_inference.inputs.impl import usb_joystick unconditionally, which will fail collection on darwin.

Two options:

Easier: at the top of the test, pytest.importorskip("evdev").

More invasive: defer import evdev to inside the function bodies that use it, so this module can be imported on darwin (just not instantiated).

Option 1 is sufficient.

kingb · 2026-05-14T20:49:36Z

+            dev.close()
+            continue
+        abs_codes = {code for code, _ in caps[evdev.ecodes.EV_ABS]}
+        if {evdev.ecodes.ABS_X, evdev.ecodes.ABS_Y, evdev.ecodes.ABS_RX} <= abs_codes:


Requiring ABS_RX excludes single-stick / flight-style controllers but is fine for Xbox/Logitech-class gamepads. Worth a docstring note that this is gamepad-only by design — otherwise the next debug session for "my controller wasn't detected" goes through this whole function.

kingb · 2026-05-14T20:49:36Z

+            candidates.append(dev)
+        else:
+            dev.close()
+    return candidates


Device identity is fragile. evdev.list_devices() returns /dev/input/event* in alphabetical order, which is not stable across reboots/replugs. Today's joystick_device=0 could be tomorrow's =1.

For a single-controller setup it's fine; for multi-controller or robust-replug setups it's a footgun.

Suggestion (potentially future-CR): add an optional joystick_device_id: str = "" config field with this match order:

dev.uniq exact match (when populated, this is the closest evdev equivalent to a MAC — Bluetooth gamepads almost always have it; some wired USB gamepads do too if the vendor bothered with serials).

Substring match against the /dev/input/by-id/usb-VENDOR_PRODUCT_SERIAL-event-joystick symlink target.

Substring match against dev.name (warn on >1 match).

Fall back to joystick_device index when joystick_device_id is empty (today's behavior).

dev.uniq is the strongest primitive when populated — worth exploring even if we don't ship the full chain in this PR.

kingb · 2026-05-14T20:49:37Z

+            if cmd is not None:
+                commands.append(cmd)
+        self._last_label = label
+        return commands


Edge detection on _last_label can swallow rapid same-chord presses if both the press and release land between two poll_commands() cycles (~policy rl_rate, usually 50Hz). InterfaceInput has the same model and same theoretical bug, so not a regression — just noting that command repeat-rate is bounded by policy rate, not physical button rate.

kingb · 2026-05-14T20:49:37Z

+            span = info.max - info.min
+            if span <= 0:
+                return
+            normalized = (value - info.min) / span * 2.0 - 1.0  # → [-1, 1]


Operator precedence: (value - info.min) / span * 2.0 - 1.0 evaluates as ((value - info.min) / span) * 2.0 - 1.0 — correct, but adding outer parens reads more clearly:

normalized = ((value - info.min) / span) * 2.0 - 1.0

kingb · 2026-05-14T20:49:37Z

+        elif code == evdev.ecodes.ABS_Z:  # analog L2
+            self._set_bit(_BIT_L2, value > TRIGGER_THRESHOLD)
+        elif code == evdev.ecodes.ABS_RZ:  # analog R2
+            self._set_bit(_BIT_R2, value > TRIGGER_THRESHOLD)


TRIGGER_THRESHOLD = 128 assumes an unsigned 0–255 range. Some controllers (especially older Xbox 360 wired pads) report ABS_Z/ABS_RZ as signed -32768..32767, in which case value > 128 is true for the entire "trigger pressed past quarter" range including the resting position on some pads.

Safer: read info.min / info.max from dev.capabilities()[EV_ABS] and threshold at min + (max - min) * 0.5. Out of scope for this PR if Xbox-Series-X-only is the immediate target, but worth a TODO.

kingb · 2026-05-14T20:49:37Z

+        from holosoma_inference.inputs.impl import usb_joystick
+
+        sentinel = MagicMock()
+        monkeypatch.setattr(usb_joystick, "UsbJoystickInput", lambda **_: sentinel)


This unconditional import of usb_joystick will fail collection on macOS (where evdev doesn't build). Wrap with:

import pytest pytest.importorskip("evdev") from holosoma_inference.inputs.impl import usb_joystick

or at the class level if other tests in this class touch the same module.

kingb · 2026-05-14T20:49:37Z

        vel = self.config.task.velocity_input
        other = self.config.task.state_input
-        need_joystick = bool({"interface", "joystick"} & {vel, other})
+        need_joystick = "interface" in {vel, other}


This is the right semantic split — the SDK's wireless-controller path is only needed for "interface" channels now. ✓

Worth confirming on real hardware that a policy started with --task.velocity-input interface (no joystick channel) but use_joystick=True on the SDK still passes joystick-key events through correctly. The need_joystick boolean's meaning to create_interface is unchanged, but the set of code paths exercising it shrunk.

Add xbox

e85882f

tomasz-lewicki requested a review from kingb May 14, 2026 01:40

Ruff cleanup

0af2d28

tomasz-lewicki changed the title ~~Add Xbox Controller as input~~ Add generic joystick support as input May 14, 2026

kingb reviewed May 14, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add generic joystick support as input #110

Add generic joystick support as input #110
tomasz-lewicki wants to merge 2 commits into
mainfrom
dev/tomasz/add-xbox-controller

tomasz-lewicki commented May 14, 2026 •

edited

Loading

Uh oh!

kingb left a comment

Uh oh!

kingb May 14, 2026

Uh oh!

kingb May 14, 2026

Uh oh!

kingb May 14, 2026

Uh oh!

kingb May 14, 2026

Uh oh!

kingb May 14, 2026

Uh oh!

kingb May 14, 2026

Uh oh!

kingb May 14, 2026

Uh oh!

kingb May 14, 2026

Uh oh!

kingb May 14, 2026

Uh oh!

kingb May 14, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

tomasz-lewicki commented May 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kingb left a comment

Choose a reason for hiding this comment

⚠️ Behavior change in --task.use-joystick

Must-fix

Should-fix

Out-of-scope / future-CR candidates

Nit

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

tomasz-lewicki commented May 14, 2026 •

edited

Loading

⚠️ Behavior change in `--task.use-joystick`