Add OneXPlayer X2 support - #1452
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe change adds ONEXPLAYER X2 detection, device behavior, vendor HID handling, controller integration, rear-button mapping, OEM chord suppression, and UI and platform guards. It also preserves unrelated X1 EC register bits and corrects Intel MCHBAR address construction. ChangesONEXPLAYER X2 support
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant IDevice.GetCurrent
participant OneXPlayerX2
participant OneXPlayerOxpHidMonitor
participant ControllerManager
participant InputsManager
IDevice.GetCurrent->>OneXPlayerX2: create detected device
OneXPlayerX2->>OneXPlayerOxpHidMonitor: initialize X2 HID profile
OneXPlayerOxpHidMonitor-->>OneXPlayerX2: provide vendor button events
ControllerManager->>ControllerManager: add injected buttons to source mappings
ControllerManager->>OneXPlayerX2: assign X2 controller
InputsManager->>OneXPlayerX2: emit OEM1 pulse for physical chord
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a1d68c3103
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (3)
HandheldCompanion/Managers/ControllerManager.cs (1)
931-934: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate X2 controller construction.
The fallback at Line 975 already builds
OneXPlayerX2Controllerfor any VID whencontrolleris still null, and the generic fallback at Line 989 already buildsXInputController. This branch therefore duplicates both paths. It also swallows the construction exception with an emptycatch, while the fallback at Line 982 logs it.Consider removing this branch and relying on the two fallbacks, so one site constructs the X2 controller and one error-handling policy applies.
♻️ Proposed change
default: - if (IDevice.GetCurrent() is OneXPlayerX2) - try { controller = new OneXPlayerX2Controller(details); } catch { } - else - try { controller = new XInputController(details); } catch { } break;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@HandheldCompanion/Managers/ControllerManager.cs` around lines 931 - 934, Remove the special OneXPlayerX2/XInput construction branch near the controller-selection logic, including its empty catch blocks. Rely on the existing fallback paths that construct OneXPlayerX2Controller and XInputController when controller remains null, preserving their centralized exception logging and single construction site.HandheldCompanion/Controllers/IController.cs (1)
844-851: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
SourceButtonsbecomes mutable after construction.Before this change,
SourceButtonswas populated only in field initializers and inInitializeInputOutput.ControllerManager.SetTargetControllernow callsAddSourceButtonson a background thread atHandheldCompanion/Managers/ControllerManager.csLine 2312, while UI code reads the same list throughHasSourceButton.List<T>does not support concurrentAddandContains, so a reader can observe a resizing backing array.The logic itself is correct and idempotent. Consider merging the injected buttons inside
InitializeInputOutputorAttachDetails, so the list is complete before the controller becomes reachable from the UI.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@HandheldCompanion/Controllers/IController.cs` around lines 844 - 851, Remove the runtime mutation introduced by AddSourceButtons and merge injected buttons during controller initialization, such as InitializeInputOutput or AttachDetails, before the controller is exposed to UI readers. Preserve the existing filtering and deduplication behavior, and ensure HasSourceButton only reads a fully initialized SourceButtons list.HandheldCompanion/Devices/OneXPlayer/OneXPlayerOxpHidMonitor.cs (1)
259-273: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winTie the delayed intercept-enable to the monitor lifetime.
The delayed command runs in a detached task with no cancellation token. If
Close()orDispose()runs during the four-second delay, the task still callsWriteCommand, which dereferences the cleared_hidDevice. The exception is caught, so this is not a crash, but the write is not cancellable and can execute after disposal.Pass the monitor's cancellation token into the delay and re-check the device before the write.
♻️ Proposed change
case OxpHidInitProfile.X2: WriteCommand(0xB4, BuildRemapPage1(0x01)); Thread.Sleep(50); WriteCommand(0xB4, BuildRemapPage2(0x01, 0x67, 0x66)); - Task.Run(async () => - { - await Task.Delay(4000); - try { WriteCommand(0xB2, [0x01, 0x1F, 0x40, 0x03, 0x02, 0x03, 0x00, 0x00, 0x00, 0x01]); } - catch (Exception ex) { LogManager.LogWarning("X2 delayed intercept-enable failed: {0}", ex.Message); } - }); + CancellationToken token = _cancellationTokenSource?.Token ?? CancellationToken.None; + Task.Run(async () => + { + try + { + await Task.Delay(4000, token).ConfigureAwait(false); + if (!IsOpen) + return; + WriteCommand(0xB2, [0x01, 0x1F, 0x40, 0x03, 0x02, 0x03, 0x00, 0x00, 0x00, 0x01]); + } + catch (OperationCanceledException) { } + catch (Exception ex) { LogManager.LogWarning("X2 delayed intercept-enable failed: {0}", ex.Message); } + }, token); break;Note:
InitializeProfile()runs before_cancellationTokenSourceis assigned inTryOpenVendorInterface. Move the token creation above theInitializeProfile()call for this change to take effect.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@HandheldCompanion/Devices/OneXPlayer/OneXPlayerOxpHidMonitor.cs` around lines 259 - 273, Update the X2 delayed intercept-enable in InitializeProfile to use the monitor lifetime cancellation token, checking cancellation after the four-second delay and before WriteCommand. Move creation or assignment of _cancellationTokenSource in TryOpenVendorInterface before InitializeProfile runs, and ensure Close/Dispose cancellation prevents the delayed write after the HID device is cleared.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@HandheldCompanion/Devices/OneXPlayer/OneXPlayerOxpHidMonitor.cs`:
- Around line 34-37: Guard device access after Close can null _hidDevice: in
ReadLoop, capture _hidDevice in a local variable and exit normally when it is
null, then use that local for reads; in WriteCommand, check _hidDevice for null
before calling Write and return without writing when unavailable.
In `@HandheldCompanion/Devices/OneXPlayer/OneXPlayerX2.cs`:
- Around line 44-86: The three X2 power profiles currently use identical PL1/PL2
values in TDPOverrideValues. Update the Performance and Max Performance profile
initializers to use the intended split sustained/turbo envelope, reusing the
existing nTDP pattern of 25, 25, 35 where appropriate; preserve the Better
Battery profile unless hardware tuning confirms it should also change.
- Around line 15-16: Add the missing device_onexplayer_x2.png asset under
Resources/DeviceImages so the ProductIllustration value in the OneXPlayerX2
device definition resolves correctly, or update ProductIllustration to an
existing shared illustration name if no dedicated asset is available.
In `@HandheldCompanion/Managers/InputsManager.cs`:
- Around line 267-272: Update the physical-key tracking block in InputsManager
around PhysicalKeyDownAt so repeated key-down events do not overwrite an
existing timestamp; record Environment.TickCount64 only when hookKey is not
already tracked, while preserving removal on key-up.
- Around line 281-304: Remove the broad physical-key suppression block in the
keyboard input handler. If X2 suppression remains necessary, update the logic to
inspect only IDevice.GetCurrent() and silenced chords whose physical-key list
contains at least three keys, preserving normal key-plus-modifier typing.
In `@HandheldCompanion/Views/QuickPages/QuickKeyboardPage.xaml.cs`:
- Around line 295-297: Update the ToUnicodeEx result check in the keyboard label
assignment to use cnt != 0 while retaining the sb.Length > 0 guard, so negative
dead-key results with spacing characters are displayed. Revise the adjacent
comment to state that zero indicates no translation and does not describe dead
keys as the zero-result case.
---
Nitpick comments:
In `@HandheldCompanion/Controllers/IController.cs`:
- Around line 844-851: Remove the runtime mutation introduced by
AddSourceButtons and merge injected buttons during controller initialization,
such as InitializeInputOutput or AttachDetails, before the controller is exposed
to UI readers. Preserve the existing filtering and deduplication behavior, and
ensure HasSourceButton only reads a fully initialized SourceButtons list.
In `@HandheldCompanion/Devices/OneXPlayer/OneXPlayerOxpHidMonitor.cs`:
- Around line 259-273: Update the X2 delayed intercept-enable in
InitializeProfile to use the monitor lifetime cancellation token, checking
cancellation after the four-second delay and before WriteCommand. Move creation
or assignment of _cancellationTokenSource in TryOpenVendorInterface before
InitializeProfile runs, and ensure Close/Dispose cancellation prevents the
delayed write after the HID device is cleared.
In `@HandheldCompanion/Managers/ControllerManager.cs`:
- Around line 931-934: Remove the special OneXPlayerX2/XInput construction
branch near the controller-selection logic, including its empty catch blocks.
Rely on the existing fallback paths that construct OneXPlayerX2Controller and
XInputController when controller remains null, preserving their centralized
exception logging and single construction site.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 6e3f1a42-5a53-4bd7-949d-813aa3c1f536
📒 Files selected for processing (11)
HandheldCompanion/Controllers/IController.csHandheldCompanion/Controllers/OneXPlayer/OneXPlayerX2Controller.csHandheldCompanion/Devices/IDevice.csHandheldCompanion/Devices/OneXPlayer/OneXPlayerOxpHidMonitor.csHandheldCompanion/Devices/OneXPlayer/OneXPlayerX1.csHandheldCompanion/Devices/OneXPlayer/OneXPlayerX2.csHandheldCompanion/Managers/ControllerManager.csHandheldCompanion/Managers/InputsManager.csHandheldCompanion/Properties/Resources.resxHandheldCompanion/Views/QuickPages/QuickKeyboardPage.xaml.csHandheldCompanion/Views/QuickPages/QuickProfilesPage.xaml
ae2bb19 to
ff6e6c2
Compare
|
Thanks for the automated review — addressed the findings in the latest push:
All verified building on real ONEXPLAYER X2 hardware. |
Adds full support for the ONEXPLAYER X2 (Intel Arc G3 Extreme / Panther Lake). Device (OneXPlayerX2): - Detection, power/thermal envelope (nTDP 25/25/35, cTDP 3-35, GfxClock, CpuClock 4700), Intel power profiles with Endurance Gaming (IGCL), and the vendor-HID button mapping. TDP caps at Intel's rated 35 W configurable limit (matching OneXPlayer's own OneXConsole). Buttons: - M1/M2 back paddles -> L4/R4, Home -> OEM3, KB -> OEM2, Turbo -> OEM1. - Paddles only surface after a Gen2 intercept-enable sent ~4s after opening the vendor interface (firmware ignores it earlier); the XInput gamepad stays live. - Turbo works by fixing the EC 0xEB take-over: read-modify-write |= 0x40 with a settle re-check, instead of a clobbering write + '== 0x40' compare that failed on the register's 0x90 transient. Vendor HID monitor: - Open the MI_02 interface with its actual InputReportByteLength (X2 = 65 vs X1 = 64) and via OpenDevice() (the HidDevice constructor does not open); BuildCommand is report-length aware so command framing lands correctly. ReadLoop/WriteCommand guard against the handle being closed mid-run. Controller / UI: - OneXPlayerX2Controller (XInput-based) exposes the injected M1/M2 paddles as mappable L4/R4 with proper labels; wrapped only for the integrated controller (details.isInternal). A device->controller source-button bridge (IDevice.InjectedControllerButtons / IController.AddSourceButtons) owns the injected paddle set. - Suppress the KB button's LCtrl+LWin+RCtrl+O combo (which opens the Windows on-screen keyboard) via a silenced OEM chord plus a robust, order-independent suppressor in InputsManager, bounded by a recency window (records first-down time so auto-repeat can't defeat it); OEM2 still fires from the vendor HID event. - Hide AMD RSR in the quick-profiles popup on Intel GPUs. Fixes: - IndexOutOfRangeException in QuickKeyboardPage.RelabelAll when ToUnicodeEx returns no character (dead keys). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
ff6e6c2 to
4a93eb5
Compare
|
Follow-up on the "limit the X2 wrapper to the integrated controller" finding: on real hardware the X2's built-in gamepad enumerates as external ( |
KX.init() built the RAPL register address by concatenating the full
MCHBAR base string with the register offset: `mchbar = address +
pnt_limit` with address = "0xfedc0000" produced "0xfedc000059", and
get/set_limit then appended "a0"/"a4" to form "0xfedc000059a0" -- a
48-bit physical address that isn't mapped, so every /wrmem16 TDP write
was silently dropped and PL1/PL2 never changed.
The base string carries the MCHBAR base in its high 16 bits (low 16 are
zero), so take Substring(0,6) ("0xfedc") before appending the offset,
yielding the correct 0xFEDC59A0 / 0xFEDC59A4. Verified live on an
OneXPlayer X2 (Panther Lake): writing the RAPL register now reads back
the expected value and HWiNFO reflects the new PL1/PL2.
Regression from 4b94f3a, which switched mchbar_addresses from the full
register form ("0xfedc59A0") to the base form without updating the
concatenation in init(). This affected the KX MMIO path on all Intel
devices; it went unnoticed because most Intel handhelds override TDP via
OEM/EC paths, while the X2 relies on the KX MMIO path.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- InputsManager: raise the silenced-chord direct suppressor threshold from 2 to 3 physical keys so a normal key+single-modifier combo (e.g. a user typing Ctrl+O) can never be swallowed. The X2's KB firmware combo is 4 keys (LCtrl+LWin+RCtrl+O), so it remains covered. (CodeRabbit) - QuickKeyboardPage: ToUnicodeEx returns a negative value for a dead key and writes a spacing char to the buffer; the previous cnt > 0 check discarded that valid output, leaving stale labels on dead-key layouts. Accept cnt != 0 (the sb.Length guard still prevents indexing the empty zero/no-translation buffer). (CodeRabbit) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Addressed the latest automated review (pushed in 3127086):
Reviewed but intentionally not changed:
Also note this branch now includes a general fix (76e7cc2) for Intel MMIO TDP writes targeting a malformed 48-bit address — verified live on the X2 via HWiNFO. |
The X2 KB button emits a firmware LCtrl+LWin+RCtrl+O combo that could open the Windows on-screen keyboard. An extra order-independent suppressor was added in InputsManager to swallow it, but on-hardware testing confirms the standard silenced-chord path (the device's OEM2 chord with its longer flushInterval) already suppresses it reliably across single and rapid double presses. Drop the redundant block and its PhysicalKeyDownAt / OEMChordRecentMs bookkeeping to keep the global keyboard hook lean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Follow-up (03e60e1): removed the extra order-independent keyboard-chord suppressor that was added in On-hardware testing confirmed the standard silenced-chord path (the device's |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@HandheldCompanion/Devices/OneXPlayer/OneXPlayerX2.cs`:
- Around line 169-181: Update the X2 takeover path around WriteECReg to read the
current SuRwECRegInterface value, modify only TurboTakeoverMask while preserving
all other EC bits, and write the result back. After the firmware settle delay,
read the register again and fail the operation if the takeover bit does not
match enabled.
In `@HandheldCompanion/Managers/InputsManager.cs`:
- Around line 267-302: Update the OneXPlayerX2 Turbo handling to require a
physical Ctrl, Win, or Alt event before processing the chord. Set
args.SuppressKeyPress for every such physical modifier event before checking
X2TurboChordActive, and use X2TurboChordActive only to ensure the OEM1 pulse is
emitted once per chord.
- Around line 292-296: Update the Turbo pulse handling around X2TurboChordActive
and InjectState so each delayed release is associated with its pulse, or is
invalidated when a new Turbo chord starts; only the current pulse may invoke the
release callback, preventing older callbacks from clearing the active pulse
state.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 9fa0e85f-a46f-44c5-9c97-a033e1d129f9
📒 Files selected for processing (3)
HandheldCompanion/Devices/OneXPlayer/OneXPlayerX1.csHandheldCompanion/Devices/OneXPlayer/OneXPlayerX2.csHandheldCompanion/Managers/InputsManager.cs
🚧 Files skipped from review as they are similar to previous changes (1)
- HandheldCompanion/Devices/OneXPlayer/OneXPlayerX1.cs
|
Superseded by a cleaned, isolated replacement PR from a fresh branch. The replacement removes unrelated fixes and restructures the X2 integration to follow the repository's device/controller patterns. |
1 participant
Summary
Adds full support for the ONEXPLAYER X2 (Intel Arc G3 Extreme / Panther Lake) — a new Intel handheld with detachable controllers. Verified end-to-end on real hardware.
Device (
OneXPlayerX2)DMI detection, power/thermal envelope, Intel power profiles with Endurance Gaming (via IGCL), fan control, and vendor-HID button mapping. TDP values match Intel's published Arc G3 Extreme spec (8–35 W configurable, 15–25 W base, up to 80 W turbo):
nTDP {25,25,35},cTDP {3,35},GfxClock {100,2300},CpuClock 4700.Buttons
0x04EBthroughroot\WMI:SuRwECRegInterface, using the provider's little-endian packed argument0x40EB04. After take-over, firmware emits Ctrl+Win+Alt; the chord is recognized order-independently and exposed as a stable, remappable OEM1 pulse.Vendor-HID monitor
MI_02interface with its actualInputReportByteLength(X2 = 65 vs X1 = 64) and viaOpenDevice()— theHidDeviceconstructor does not open the handle.BuildCommandis report-length aware so command framing lands correctly on the X2.Controller / UI
OneXPlayerX2Controller(XInput-based) exposes the device-injected M1/M2 paddles as mappable L4/R4 with M1/M2 labels, and drops the duplicate Guide/Special entry (same physical button as OEM3). A small device→controller bridge (IDevice.InjectedControllerButtons/IController.AddSourceButtons) is the single owner of injected paddle buttons for the X1 family.LCtrl+LWin+RCtrl+O, which can open the Windows soft keyboard.InputsManagerrecognizes the distinctive two-Ctrl + Win chord on physical O-down and latches suppression through O-up; this is necessary because firmware releases every modifier before releasing O. Ordinary Ctrl+O remains unaffected. OEM2 fires independently from vendor HID ID0x24, so the button stays fully remappable.Fixes
IndexOutOfRangeExceptioninQuickKeyboardPage.RelabelAllwhenToUnicodeExreturns no character (dead keys).Testing
Verified on real ONEXPLAYER X2 hardware: Turbo/OEM1 works without OneXConsole, KB/OEM2 and Home/OEM3 fire reliably from the FE00 vendor HID interface, all OEM buttons remain remappable, the gamepad stays live, TDP/power profiles apply, Intel graphics options display correctly (RSR hidden), and the on-screen keyboard no longer crashes.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes