Add isolated ONEXPLAYER X2 support - #1458
Conversation
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>
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>
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>
|
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 and X2Mini PRO support. It includes device detection, specialized XInput mappings, direct HID handling, WMI EC access, turbo takeover, vendor buttons, power profiles, and localized button labels. ChangesOneXPlayer X2 support
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant ControllerManager
participant IDevice
participant OneXPlayerX2
participant HidDevice
participant OneXPlayerWmiEc
ControllerManager->>IDevice: create controller for detected device
IDevice->>OneXPlayerX2: create X2 device
OneXPlayerX2->>HidDevice: initialize and read vendor HID reports
HidDevice-->>OneXPlayerX2: return button events
OneXPlayerX2->>OneXPlayerWmiEc: configure and verify turbo register
Possibly related PRs
🚥 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 |
|
Pushed a few tweaks. Looks good now. Please double check I didn't break anything. |
|
Thanks! I think we still need the initializeRemap but I'll test tonight and report back |
It wasn't called anywhere. |
|
Yeah I think a bug I ntroduced in the PR. Will confirm |
01ac320 to
6d4ba48
Compare
|
Okay I've tested with the changes and all seems to work, thanks! |
![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: 6d4ba481ea
ℹ️ 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".
| switch (details.GetProductID()) | ||
| { | ||
| case "0x028E": | ||
| try { controller = new OneXPlayerX2Controller(details); } catch { } |
There was a problem hiding this comment.
When any controller reports the standard Microsoft Xbox 360 VID/PID 045E:028E, this unconditional branch constructs OneXPlayerX2Controller, even on non-X2 systems. Ordinary Xbox 360-compatible controllers will consequently be named as an X2, expose nonexistent M1/M2 inputs, and lose Special from their mappable source buttons; gate this substitution on the current device being OneXPlayerX2/X2 Mini Pro rather than on this shared VID/PID alone.
Useful? React with 👍 / 👎.
| public OneXPlayerX2MiniPro() | ||
| { | ||
| ProductModel = "ONEXPLAYER X2Mini PRO"; | ||
| VendorHidInitProfile = OxpHidInitProfile.Apex; |
There was a problem hiding this comment.
On an X2 Mini Pro, assigning the initialization profile does not start the monitor: OneXPlayerApex/OneXPlayerX1.Open() never calls StartVendorHidListener(), and the inherited HID-arrival callback is filtered by vendorId and productIds, which this class leaves at their empty defaults. As a result, MapVendorButton() is never reached and the newly advertised OEM and paddle inputs remain unavailable; configure the vendor IDs or explicitly start the listener as the X2 class does.
Useful? React with 👍 / 👎.
| { | ||
| try | ||
| { | ||
| StartVendorHidListener(); |
There was a problem hiding this comment.
If the X2 vendor interface disconnects or resets after startup, this one-time call leaves OEM1/OEM2/OEM3 and M1/M2 unavailable until the application restarts. The inherited removal/arrival handlers only invoke Device_Removed/Device_Inserted for configured vendorId and productIds, but OneXPlayerX2 leaves those fields at their defaults, so the monitor is never disposed and reopened when FE00/1305 returns.
Useful? React with 👍 / 👎.
| Thread.Sleep(50); | ||
| byte actualValue = ec.ReadByte(TurboTakeoverRegister); | ||
|
|
||
| if (ec.ReadByte(0xEB) == value) |
There was a problem hiding this comment.
Every takeover verification reads legacy register 0x00EB even though the write and actualValue read target the explicitly distinct banked register 0x04EB. The success/failure log therefore depends on an unrelated register and can report either a failed successful write or a successful failed write; compare actualValue with value instead.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 79-104: Update the OneXPlayer HID initialization around HidDevice
candidate creation to obtain and use the interface’s output report length for
outgoing commands. Ensure WriteCommand and the buffer used by HidDevice.Write()
are sized from OutputReportByteLength rather than only GetInputReportByteLength,
while preserving the existing input report length for read operations and
handling unavailable output lengths consistently.
In `@HandheldCompanion/Devices/OneXPlayer/OneXPlayerX2.cs`:
- Around line 145-156: Update the verification condition in the TurboTakeover
write flow to compare the post-write value stored in actualValue against value,
rather than reading the unrelated 0xEB register. Keep the existing success and
failure logging behavior, using actualValue for the reported observed value.
In `@HandheldCompanion/Devices/OneXPlayer/OneXPlayerX2MiniPro.cs`:
- Around line 8-12: Update the OneXPlayerX2MiniPro constructor to explicitly
assign the model’s X2Mini PRO-specific nTDP and cTDP ranges, overriding the
inherited Apex defaults while preserving the existing ProductModel and
VendorHidInitProfile assignments.
In `@HandheldCompanion/Managers/ControllerManager.cs`:
- Around line 903-912: Update the OneXPlayer X2 branch in ControllerManager’s
controller-selection switch to require a host-device check in addition to
VID/PID 0x045E:0x028E. Only instantiate OneXPlayerX2Controller when the machine
is identified as OneXPlayer X2 hardware; otherwise leave the generic controller
handling unchanged.
🪄 Autofix
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: 177c2afd-e6a4-4a89-96b7-2ee668d5c513
📒 Files selected for processing (11)
HandheldCompanion/Controllers/OneXPlayer/OneXPlayerX2Controller.csHandheldCompanion/Devices/IDevice.csHandheldCompanion/Devices/OneXPlayer/OneXPlayerOxpHidMonitor.csHandheldCompanion/Devices/OneXPlayer/OneXPlayerWmiEc.csHandheldCompanion/Devices/OneXPlayer/OneXPlayerX1.csHandheldCompanion/Devices/OneXPlayer/OneXPlayerX2.csHandheldCompanion/Devices/OneXPlayer/OneXPlayerX2MiniPro.csHandheldCompanion/Managers/ControllerManager.csHandheldCompanion/Managers/InputsManager.csHandheldCompanion/Misc/KeyboardChord.csHandheldCompanion/Properties/Resources.resx
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@hidapi.net/HidDevice.cs`:
- Around line 62-65: Update the HidP_GetCaps result check in the HID
capabilities accessor to recognize HIDP_STATUS_SUCCESS (0x00110000) rather than
treating any nonzero result as failure. Return caps.OutputReportByteLength on
success, and throw IOException only for the expected failure status codes or via
the established NTSTATUS-success helper.
🪄 Autofix
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: 11875bd9-8ea0-449d-9ad4-7b8834218630
📒 Files selected for processing (8)
HandheldCompanion/Devices/IDevice.csHandheldCompanion/Devices/OneXPlayer/OneXPlayerApex.csHandheldCompanion/Devices/OneXPlayer/OneXPlayerOxpHidMonitor.csHandheldCompanion/Devices/OneXPlayer/OneXPlayerX1.csHandheldCompanion/Devices/OneXPlayer/OneXPlayerX2.csHandheldCompanion/Devices/OneXPlayer/OneXPlayerX2MiniPro.csHandheldCompanion/Managers/ControllerManager.cshidapi.net/HidDevice.cs
🚧 Files skipped from review as they are similar to previous changes (4)
- HandheldCompanion/Managers/ControllerManager.cs
- HandheldCompanion/Devices/OneXPlayer/OneXPlayerX2MiniPro.cs
- HandheldCompanion/Devices/OneXPlayer/OneXPlayerOxpHidMonitor.cs
- HandheldCompanion/Devices/OneXPlayer/OneXPlayerX1.cs
Replace the custom OXP HID monitor with shared HidLibrary handling in OneXPlayer devices, including remapping, button input, vibration, status recovery, and X2 intercept initialization.
Use the correct HIDP success status code when retrieving input and output report lengths.
|
@gsxdsm can you please test it now ? I've migrated a good portion of the code to adopt IDevice standard structure and classes. |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
HandheldCompanion/Devices/OneXPlayer/OneXPlayerX2MiniPro.cs (1)
18-27: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUse
OneXPlayerX2Controllerfor the X2 Mini Pro controller.
OneXPlayerX2MiniProdoes not overrideCreateController, so it falls back toIDevice.CreateController()and returnsnull. This means the controller factory constructs a genericXInputController, whoseInitializeInputOutput()does not addButtonFlags.L4orButtonFlags.R4;OneXPlayerX2already overrides this by returningnew OneXPlayerX2Controller(details). Add the same override toOneXPlayerX2MiniPro.🤖 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/OneXPlayerX2MiniPro.cs` around lines 18 - 27, Override CreateController in OneXPlayerX2MiniPro to return a new OneXPlayerX2Controller using the provided device details, matching the existing implementation in OneXPlayerX2 and ensuring the specialized L4/R4 button mappings are initialized.
🤖 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/OneXPlayerX1.cs`:
- Around line 483-489: Update the disconnect/cleanup flow around _buttonStates
so every currently pressed vendor button is passed through KeyRelease before
Array.Clear(_buttonStates). Preserve the existing device removal and disposal
behavior, and ensure release handling covers L4, R4, and held OEM buttons.
In `@HandheldCompanion/Devices/OneXPlayer/OneXPlayerX1Mini.cs`:
- Around line 82-88: Update HandleStatusReport so the 0xFE recovery path re-runs
device discovery after Device_Removed(), rather than relying on
Device_Inserted() with reScan false. Ensure the vendor HID device is
rediscovered and reinserted immediately after the status-triggered removal.
In `@HandheldCompanion/Devices/OneXPlayer/OneXPlayerX2.cs`:
- Around line 106-113: Update OneXPlayerX2.Device_Inserted to associate the
delayed initialization with the specific HID session, using a per-session
cancellation token or generation value captured before Task.Delay(4000). In
Device_Removed, cancel or invalidate that session; after the delay, verify it
remains current and active before calling WriteVendorHidCommand, preventing
stale initialization from targeting a replacement device.
---
Outside diff comments:
In `@HandheldCompanion/Devices/OneXPlayer/OneXPlayerX2MiniPro.cs`:
- Around line 18-27: Override CreateController in OneXPlayerX2MiniPro to return
a new OneXPlayerX2Controller using the provided device details, matching the
existing implementation in OneXPlayerX2 and ensuring the specialized L4/R4
button mappings are initialized.
🪄 Autofix
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: 0c689877-74d9-4b94-9cae-1f7573d4c61e
📒 Files selected for processing (6)
HandheldCompanion/Devices/OneXPlayer/OneXPlayerApex.csHandheldCompanion/Devices/OneXPlayer/OneXPlayerOxpHidMonitor.csHandheldCompanion/Devices/OneXPlayer/OneXPlayerX1.csHandheldCompanion/Devices/OneXPlayer/OneXPlayerX1Mini.csHandheldCompanion/Devices/OneXPlayer/OneXPlayerX2.csHandheldCompanion/Devices/OneXPlayer/OneXPlayerX2MiniPro.cs
💤 Files with no reviewable changes (1)
- HandheldCompanion/Devices/OneXPlayer/OneXPlayerOxpHidMonitor.cs
| protected override async void Device_Inserted(bool reScan = false) | ||
| { | ||
| if (reScan) | ||
| await WaitUntilReady(); | ||
|
|
||
| base.Device_Inserted(); | ||
| await Task.Delay(4000); | ||
| WriteVendorHidCommand(0xB2, [0x01, 0x1F, 0x40, 0x03, 0x02, 0x03, 0x00, 0x00, 0x00, 0x01]); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Cancel stale X2 initialization after HID removal.
The delayed continuation is not tied to the inserted HID session. If removal and reinsertion occur during Task.Delay(4000), this invocation calls WriteVendorHidCommand against the replacement stored in hidDevices. The replacement can receive duplicate or stale initialization commands.
Add a per-session cancellation token or generation value. Cancel or invalidate it in Device_Removed. Only write the X2 initialization command when the originally opened device is still the active device.
🤖 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/OneXPlayerX2.cs` around lines 106 - 113,
Update OneXPlayerX2.Device_Inserted to associate the delayed initialization with
the specific HID session, using a per-session cancellation token or generation
value captured before Task.Delay(4000). In Device_Removed, cancel or invalidate
that session; after the delay, verify it remains current and active before
calling WriteVendorHidCommand, preventing stale initialization from targeting a
replacement device.
Use the device's standard read loop when configuring OneXPlayer X1 remapping, replacing the vendor-specific loop call.
Force device insertion handling when the X1 Mini reports removal, ensuring the device is fully reinitialized after reconnect events.
Release any pressed vendor buttons before clearing their state when the OneXPlayer X1 device is removed, preventing stuck button events.
Three fixes to the migrated OneXPlayer vendor HID handling that broke the ONEXPLAYER X2: - IsReady() bound the first connected interface matching the VID/PID. On the X2 several collections match; it picked an input-only or 2-byte writable collection, causing CreateReport() to build a HidReport(0) (startup crash) or an index overflow. Select the vendor-defined collection by usage page 0xFF00 (the 65-byte mi_02 interface), falling back to the widest output report otherwise. - WriteVendorHidCommand() used CreateReport()/WriteReport(), which prepends a 0x00 report-id byte and shifts the whole command frame. The firmware misinterprets the shifted frame and flips the pad's reporting mode, killing the XInput gamepad while turbo/vendor buttons keep working. Emit the exact frame via raw device.Write() (command id in byte 0), matching the pre-migration raw write. - Guard WriteVendorHidCommand() against interfaces whose output report is missing or too small. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
On hidfilter, use the following logic: https://github.com/Valkirie/HandheldCompanion/blob/main/HandheldCompanion%2FDevices%2FMSI%2FClawA1M.cs#L180 |
Address review feedback: instead of the ad-hoc "match usage page 0xFF00, else widest output report" heuristic, declare the vendor collection through the shared hidFilters map (as MSI ClawA1M and the X1 Mini already do) and match on usage page + usage in IsReady(). The OneXPlayer classic vendor chip (0x1A86 / 0xFE00) carries the button and remap traffic on collection MI_02 (usage page 0xFF00, usage 0x0001), verified by enumerating the live X2 hardware. The X2's second chip (0x1305) has no hidFilters entry, so its MI_02 (a 9-byte input collection, not the buttons) is correctly skipped. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Looks good now! |
OneXPlayerX1Mini used the exact same vendor chip (0x1A86 / 0xFE00, usage FF00:0001) as its OneXPlayerX1 base, but re-declared vendorId, productIds, hidFilters and a byte-for-byte copy of IsReady(). Now that the base pulls the vendor interface via hidFilters (matching MSI ClawA1M), the Mini can simply inherit it, like OneXPlayerX1AMD/X1Intel already do. Removes the duplicate IsReady() override and the redundant PID_LED/ctor declarations. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Found one place to consolidate and merging main into this to make sure it works end to end |
2 participants
Summary
Add ONEXPLAYER X2 support with the device-specific behavior isolated under the existing OneXPlayer device and controller structure.
What changed
OneXPlayerWmiEc, preserving unrelated register bits and verifying takeover writes.Isolation and maintainability
InputsManager.OneXPlayerX2Controller, not injected into generic controllers at runtime.Validation
dotnet build HandheldCompanion/HandheldCompanion.csproj --no-restore -p:Platform=x64: 0 errors.Supersedes #1452.
Summary by CodeRabbit
New Features
Bug Fixes