FIX: Incorrect HID Stick values for LogicalMinimum with negative values #2246

jfreire-unity · 2025-09-26T12:13:38Z

Description

This PR addresses the issues encountered when parsing data for devices that have LogicalMinimum values with negative values. Seems we weren't taking 2's complement values into account.

Also includes fixes from user PR #2245

Testing status & QA

Tested with a Gamepad stick which has both signed short and byte for LogicalMin/Max of [-127,127] and [-32.768,32.767]

Overall Product Risks

Complexity: 0
Halo Effect: 1

Comments to reviewers

This is not easy to test without real devices that we don't have a layout for so I at least hope existing devices would still work.

One strategy could be to test this on macOS and comment this line:

InputSystem/Packages/com.unity.inputsystem/InputSystem/InputSystem.cs

Line 3798 in d1d72d5

XInputSupport.Initialize();

Then look if the sticks were well interpreted.

However, this only allows us to verify for devices that send Stick values as a signed short. For signed bytes I don't know of a device besides the ones users have reported in Discussions https://discussions.unity.com/t/input-system-reading-invalid-values-from-hall-effect-keyboards/1684840/3

Checklist

Before review:

Changelog entry added.
- Explains the change in Changed, Fixed, Added sections.
- For API change contains an example snippet and/or migration example.
- JIRA ticket linked, example (case %%). If it is a private issue, just add the case ID without a link.
- Jira port for the next release set as "Resolved".
Tests added/changed, if applicable.
- Functional tests Area_CanDoX, Area_CanDoX_EvenIfYIsTheCase, Area_WhenIDoX_AndYHappens_ThisIsTheResult.
- Performance tests.
- Integration tests.
Docs for new/changed API's.
- Xmldoc cross references are set correctly.
- Added explanation how the API works.
- Usage code examples added.
- The manual is updated, if needed.

During merge:

Commit message for squash-merge is prefixed with one of the list:
- NEW: ___.
- FIX: ___.
- DOCS: ___.
- CHANGE: ___.
- RELEASE: 1.1.0-preview.3.

After merge:

Create forward/backward port if needed. If you are blocked from creating a forward port now please add a task to ISX-1444.

Also creates a simple layout struct for a device with only a single analog stick where the HID report descriptor states that the x and y values are between -127 and 127.

jfreire-unity · 2025-09-29T07:35:32Z

Assets/Tests/InputSystem/Plugins/HIDTests.cs


    [StructLayout(LayoutKind.Explicit)]
-    struct SimpleJoystickLayout : IInputStateTypeInfo
+    struct SimpleJoystickLayoutWithStickUshort : IInputStateTypeInfo


Need to create a test with a report descriptor that fits this struct as well.

Copilot

Pull Request Overview

This PR fixes incorrect handling of HID stick values when LogicalMinimum contains negative values by properly implementing 2's complement parsing and sign handling.

Corrects data parsing to handle signed byte and short values using proper casting
Fixes a bit shift error in 32-bit integer parsing
Updates stick control format determination to use a proper method instead of simple boolean check

Reviewed Changes

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

File	Description
HIDParser.cs	Fixes signed value parsing with proper casting and corrects bit shift error
HID.cs	Updates stick format determination and adds debug logging
HIDTests.cs	Adds comprehensive test for signed logical min/max values and refactors test helpers

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

Packages/com.unity.inputsystem/InputSystem/Plugins/HID/HID.cs

Assets/Tests/InputSystem/Plugins/HIDTests.cs

Co-authored-by: Copilot <[email protected]>

jfreire-unity · 2025-10-02T13:51:17Z

Packages/com.unity.inputsystem/InputSystem/Plugins/HID/HID.cs


                    builder.AddControl(stickName + "/y")
-                        .WithFormat(yElement.isSigned ? InputStateBlock.FormatSBit : InputStateBlock.FormatBit)
+                        .WithFormat(yElement.DetermineFormat())


This change was included from #2245

Isn't it required to always determine format for any control? Also buttons support this (but I cannot recall ever seeing it in practise), but definitely e.g. triggers go into same territory

Yes, and we also did it before. Not sure why assigned it the SBit/Bit format before but we do read multiple "bits" if we had them (e.g. in stateBlock.ReadFloat()). So it might have been to deal with cases where we could use like "10 bits" to define an axis. I'll have a look and test this as well to see if there are any other changes needed. Thanks.

codecov-github-com · 2025-10-02T14:02:49Z

Codecov Report

Attention: Patch coverage is 97.82609% with 1 line in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
...y.inputsystem/InputSystem/Plugins/HID/HIDParser.cs	66.66%	1 Missing ⚠️

@@             Coverage Diff             @@
##           develop    #2246      +/-   ##
===========================================
+ Coverage    68.14%   76.72%   +8.58%     
===========================================
  Files          367      465      +98     
  Lines        53685    87947   +34262     
===========================================
+ Hits         36584    67479   +30895     
- Misses       17101    20468    +3367

Flag	Coverage Δ
inputsystem_MacOS_2021.3	`5.91% <0.00%> (?)`
inputsystem_MacOS_2021.3_project	`78.06% <97.82%> (?)`
inputsystem_MacOS_2022.3	`5.37% <0.00%> (?)`
inputsystem_MacOS_2022.3_project	`74.60% <97.82%> (?)`
inputsystem_MacOS_6000.0	`5.19% <0.00%> (?)`
inputsystem_MacOS_6000.0_project	`76.52% <97.82%> (?)`
inputsystem_MacOS_6000.2	`5.19% <0.00%> (?)`
inputsystem_MacOS_6000.2_project	`76.52% <97.82%> (?)`
inputsystem_MacOS_6000.3	`5.19% <0.00%> (?)`
inputsystem_MacOS_6000.3_project	`76.52% <97.82%> (?)`
inputsystem_MacOS_6000.4	`5.19% <0.00%> (?)`
inputsystem_MacOS_6000.4_project	`76.52% <97.82%> (?)`
inputsystem_Ubuntu_2021.3	`5.91% <0.00%> (?)`
inputsystem_Ubuntu_2021.3_project	`77.98% <97.82%> (?)`
inputsystem_Ubuntu_2022.3	`5.38% <0.00%> (?)`
inputsystem_Ubuntu_2022.3_project	`74.40% <97.82%> (?)`
inputsystem_Ubuntu_6000.0	`5.19% <0.00%> (?)`
inputsystem_Ubuntu_6000.0_project	`76.33% <97.82%> (?)`
inputsystem_Ubuntu_6000.2	`5.19% <0.00%> (?)`
inputsystem_Ubuntu_6000.2_project	`76.33% <97.82%> (?)`
inputsystem_Ubuntu_6000.3	`5.19% <0.00%> (?)`
inputsystem_Ubuntu_6000.3_project	`76.33% <97.82%> (?)`
inputsystem_Ubuntu_6000.4	`5.19% <0.00%> (?)`
inputsystem_Ubuntu_6000.4_project	`76.33% <97.82%> (?)`
inputsystem_Windows_2021.3	`5.91% <0.00%> (?)`
inputsystem_Windows_2021.3_project	`78.21% <97.82%> (?)`
inputsystem_Windows_2022.3	`5.37% <0.00%> (?)`
inputsystem_Windows_2022.3_project	`74.74% <97.82%> (?)`
inputsystem_Windows_6000.0	`5.19% <0.00%> (?)`
inputsystem_Windows_6000.0_project	`76.66% <97.82%> (?)`
inputsystem_Windows_6000.2	`5.19% <0.00%> (?)`
inputsystem_Windows_6000.2_project	`76.66% <97.82%> (?)`
inputsystem_Windows_6000.3	`5.19% <0.00%> (?)`
inputsystem_Windows_6000.3_project	`76.65% <97.82%> (?)`
inputsystem_Windows_6000.4	`5.19% <0.00%> (?)`
inputsystem_Windows_6000.4_project	`76.44% <97.82%> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
Assets/Tests/InputSystem/Plugins/HIDTests.cs	`98.37% <100.00%> (ø)`
...m.unity.inputsystem/InputSystem/Plugins/HID/HID.cs	`69.98% <ø> (ø)`
...y.inputsystem/InputSystem/Plugins/HID/HIDParser.cs	`83.24% <66.66%> (ø)`

... and 102 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

ekcoh

Thanks for looking into this. I am confused about our ReadData method, it is not yet clear to me how we handle sign-extended types of arbitrary bit sizes? E.g.
It's completely valid to have e.g. X with logical min -15 logical max 16, and hence Report size 5

ekcoh · 2025-10-03T07:40:42Z

Packages/com.unity.inputsystem/InputSystem/Plugins/HID/HID.cs


                    builder.AddControl(stickName + "/y")
-                        .WithFormat(yElement.isSigned ? InputStateBlock.FormatSBit : InputStateBlock.FormatBit)
+                        .WithFormat(yElement.DetermineFormat())


Isn't it required to always determine format for any control? Also buttons support this (but I cannot recall ever seeing it in practise), but definitely e.g. triggers go into same territory

Assets/Tests/InputSystem/Plugins/HIDTests.cs

jfreire-unity added 4 commits September 26, 2025 13:09

Add proposed fix by PR #2245

7cb59ce

Fix parsing signed bytes and incorrect byte reading for 4 bytes

9a5f397

Add test to validate stick values

bd804af

Also creates a simple layout struct for a device with only a single analog stick where the HID report descriptor states that the x and y values are between -127 and 127.

Cast short when concatenating bytes

8ddd35e

jfreire-unity changed the title ~~FIX: Incorrect HID Stick values for LogicalMin/Max with signed values~~ FIX: Incorrect HID Stick values for LogicalMinimum with negative values Sep 29, 2025

jfreire-unity commented Sep 29, 2025

View reviewed changes

jfreire-unity requested a review from Copilot September 29, 2025 07:35

Copilot AI reviewed Sep 29, 2025

View reviewed changes

Packages/com.unity.inputsystem/InputSystem/Plugins/HID/HID.cs Outdated Show resolved Hide resolved

Assets/Tests/InputSystem/Plugins/HIDTests.cs Outdated Show resolved Hide resolved

Apply suggestions from code review

637e5eb

Co-authored-by: Copilot <[email protected]>

jfreire-unity marked this pull request as ready for review October 2, 2025 13:50

jfreire-unity commented Oct 2, 2025

View reviewed changes

jfreire-unity requested review from ekcoh and Pauliusd01 October 2, 2025 13:56

ekcoh reviewed Oct 3, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

FIX: Incorrect HID Stick values for LogicalMinimum with negative values #2246

FIX: Incorrect HID Stick values for LogicalMinimum with negative values #2246

Uh oh!

jfreire-unity commented Sep 26, 2025 •

edited

Loading

Uh oh!

jfreire-unity Sep 29, 2025 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

jfreire-unity Oct 2, 2025

Uh oh!

ekcoh Oct 3, 2025 •

edited

Loading

Uh oh!

jfreire-unity Oct 3, 2025

Uh oh!

codecov-github-com bot commented Oct 2, 2025 •

edited

Loading

Uh oh!

ekcoh left a comment

Uh oh!

ekcoh Oct 3, 2025 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

FIX: Incorrect HID Stick values for LogicalMinimum with negative values #2246

Are you sure you want to change the base?

FIX: Incorrect HID Stick values for LogicalMinimum with negative values #2246

Uh oh!

Conversation

jfreire-unity commented Sep 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Testing status & QA

Overall Product Risks

Comments to reviewers

Checklist

Uh oh!

jfreire-unity Sep 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

jfreire-unity Oct 2, 2025

Choose a reason for hiding this comment

Uh oh!

ekcoh Oct 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jfreire-unity Oct 3, 2025

Choose a reason for hiding this comment

Uh oh!

codecov-github-com bot commented Oct 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

ekcoh left a comment

Choose a reason for hiding this comment

Uh oh!

ekcoh Oct 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jfreire-unity commented Sep 26, 2025 •

edited

Loading

jfreire-unity Sep 29, 2025 •

edited

Loading

ekcoh Oct 3, 2025 •

edited

Loading

codecov-github-com bot commented Oct 2, 2025 •

edited

Loading

ekcoh Oct 3, 2025 •

edited

Loading