Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
7cb59ce
Add proposed fix by PR #2245
jfreire-unity Sep 26, 2025
9a5f397
Fix parsing signed bytes and incorrect byte reading for 4 bytes
jfreire-unity Sep 26, 2025
bd804af
Add test to validate stick values
jfreire-unity Sep 26, 2025
8ddd35e
Cast short when concatenating bytes
jfreire-unity Sep 27, 2025
637e5eb
Apply suggestions from code review
jfreire-unity Sep 29, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
139 changes: 118 additions & 21 deletions Assets/Tests/InputSystem/Plugins/HIDTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -218,27 +218,9 @@ public void Devices_CanCreateGenericHID_FromDeviceWithBinaryReportDescriptor()

// The HID report descriptor is fetched from the device via an IOCTL.
var deviceId = runtime.AllocateDeviceId();
unsafe
{
runtime.SetDeviceCommandCallback(deviceId,
(id, commandPtr) =>
{
if (commandPtr->type == HID.QueryHIDReportDescriptorSizeDeviceCommandType)
return reportDescriptor.Length;

if (commandPtr->type == HID.QueryHIDReportDescriptorDeviceCommandType
&& commandPtr->payloadSizeInBytes >= reportDescriptor.Length)
{
fixed(byte* ptr = reportDescriptor)
{
UnsafeUtility.MemCpy(commandPtr->payloadPtr, ptr, reportDescriptor.Length);
return reportDescriptor.Length;
}
}
SetDeviceCommandCallbackToReturnReportDescriptor(deviceId, reportDescriptor);

return InputDeviceCommand.GenericFailure;
});
}
// Report device.
runtime.ReportNewInputDevice(
new InputDeviceDescription
Expand Down Expand Up @@ -309,6 +291,111 @@ public void Devices_CanCreateGenericHID_FromDeviceWithBinaryReportDescriptor()
////TODO: check hat switch
}

// This is used to mock out the IOCTL the HID device driver would use to return
// the report descriptor and its size.
unsafe void SetDeviceCommandCallbackToReturnReportDescriptor(int deviceId, byte[] reportDescriptor)
{
runtime.SetDeviceCommandCallback(deviceId,
(id, commandPtr) =>
{
if (commandPtr->type == HID.QueryHIDReportDescriptorSizeDeviceCommandType)
return reportDescriptor.Length;

if (commandPtr->type == HID.QueryHIDReportDescriptorDeviceCommandType
&& commandPtr->payloadSizeInBytes >= reportDescriptor.Length)
{
fixed(byte* ptr = reportDescriptor)
{
UnsafeUtility.MemCpy(commandPtr->payloadPtr, ptr, reportDescriptor.Length);
return reportDescriptor.Length;
}
}

return InputDeviceCommand.GenericFailure;
});
}

[Test]
[Category("HID Devices")]
public void Devices_CanCrateGenericHID_WithSignedLogicalMinAndMaxSticks()
{
// This is a HID report descriptor for a simple device with two analog sticks;
// Similar to a user that reported an issue in Discussions:
// https://discussions.unity.com/t/input-system-reading-invalid-values-from-hall-effect-keyboards/1684840/3
var reportDescriptor = new byte[]
{
0x05, 0x01, // Usage Page (Generic Desktop Ctrls)
0x09, 0x05, // Usage (Game Pad)
0xA1, 0x01, // Collection (Application)
0x85, 0x01, // Report ID (1)
0x05, 0x01, // Usage Page (Generic Desktop Ctrls)
0x09, 0x30, // Usage (X)
0x09, 0x31, // Usage (Y)
0x15, 0x81, // Logical Minimum (-127)
0x25, 0x7F, // Logical Maximum (127)
0x75, 0x08, // Report Size (8)
0x95, 0x02, // Report Count (2)
0x81, 0x02, // Input (Data,Var,Abs)
0xC0, // End Collection
};

// The HID report descriptor is fetched from the device via an IOCTL.
var deviceId = runtime.AllocateDeviceId();

// Callback to return the desired report descriptor.
SetDeviceCommandCallbackToReturnReportDescriptor(deviceId, reportDescriptor);

// Report device.
runtime.ReportNewInputDevice(
new InputDeviceDescription
{
interfaceName = HID.kHIDInterface,
manufacturer = "TestVendor",
product = "TestHID",
capabilities = new HID.HIDDeviceDescriptor
{
vendorId = 0x321,
productId = 0x432
}.ToJson()
}.ToJson(), deviceId);

InputSystem.Update();

var device = (Joystick)InputSystem.GetDeviceById(deviceId);
Assert.That(device, Is.Not.Null);
Assert.That(device, Is.TypeOf<Joystick>());

// Stick vector 2 should be centered at (0,0).
Assert.That(device.stick.ReadValue(), Is.EqualTo(new Vector2(0f, 0f)).Using(Vector2EqualityComparer.Instance));

// Queue event with stick pushed to bottom. We assume Y axis is inverted by default in HID devices.
// See HID.HIDElementDescriptor.DetermineParameters()
InputSystem.QueueStateEvent(device, new SimpleJoystickLayoutWithStickByte { reportId = 1, x = 0, y = 127 });
InputSystem.Update();

Assert.That(device.stick.ReadValue() , Is.EqualTo(new Vector2(0f, -1f)).Using(Vector2EqualityComparer.Instance));

InputSystem.QueueStateEvent(device, new SimpleJoystickLayoutWithStickByte { reportId = 1, x = 0, y = -127 });
InputSystem.Update();

Assert.That(device.stick.ReadValue(), Is.EqualTo(new Vector2(0f, 1f)).Using(Vector2EqualityComparer.Instance));

InputSystem.QueueStateEvent(device, new SimpleJoystickLayoutWithStickByte { reportId = 1, x = 127, y = 0 });
InputSystem.Update();

Assert.That(device.stick.ReadValue() , Is.EqualTo(new Vector2(1f, 0f)).Using(Vector2EqualityComparer.Instance));

InputSystem.QueueStateEvent(device, new SimpleJoystickLayoutWithStickByte { reportId = 1, x = -127, y = 0 });
InputSystem.Update();

Assert.That(device.stick.ReadValue(), Is.EqualTo(new Vector2(-1f, 0f)).Using(Vector2EqualityComparer.Instance));

InputSystem.QueueStateEvent(device, new SimpleJoystickLayoutWithStickByte { reportId = 1, x = 127, y = 127 });
InputSystem.Update();

Assert.That(device.stick.ReadValue(), Is.EqualTo(new Vector2(0.7071f, -0.7071f)).Using(Vector2EqualityComparer.Instance));
}

[Test]
[Category("Devices")]
public void Devices_CanCreateGenericHID_FromDeviceWithParsedReportDescriptor()
Expand Down Expand Up @@ -1026,7 +1113,7 @@ public void Devices_GenericHIDConvertsXAndYUsagesToStickControl()
}

[StructLayout(LayoutKind.Explicit)]
struct SimpleJoystickLayout : IInputStateTypeInfo
struct SimpleJoystickLayoutWithStickUshort : IInputStateTypeInfo
Copy link
Collaborator Author

@jfreire-unity jfreire-unity Sep 29, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

{
[FieldOffset(0)] public byte reportId;
[FieldOffset(1)] public ushort x;
Expand All @@ -1035,6 +1122,16 @@ struct SimpleJoystickLayout : IInputStateTypeInfo
public FourCC format => new FourCC('H', 'I', 'D');
}

[StructLayout(LayoutKind.Explicit)]
struct SimpleJoystickLayoutWithStickByte : IInputStateTypeInfo
{
[FieldOffset(0)] public byte reportId;
[FieldOffset(1)] public sbyte x;
[FieldOffset(2)] public sbyte y;

public FourCC format => new FourCC('H', 'I', 'D');
}

[Test]
[Category("Devices")]
public void Devices_GenericHIDXAndYDrivesStickControl()
Expand Down Expand Up @@ -1069,7 +1166,7 @@ public void Devices_GenericHIDXAndYDrivesStickControl()
Assert.That(device, Is.TypeOf<Joystick>());
Assert.That(device["Stick"], Is.TypeOf<StickControl>());

InputSystem.QueueStateEvent(device, new SimpleJoystickLayout { reportId = 1, x = ushort.MaxValue, y = ushort.MinValue });
InputSystem.QueueStateEvent(device, new SimpleJoystickLayoutWithStickUshort { reportId = 1, x = ushort.MaxValue, y = ushort.MinValue });
InputSystem.Update();

Assert.That(device["stick"].ReadValueAsObject(),
Expand Down
5 changes: 3 additions & 2 deletions Packages/com.unity.inputsystem/InputSystem/Plugins/HID/HID.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -257,6 +257,7 @@ internal static unsafe HIDDeviceDescriptor ReadHIDDeviceDescriptor(ref InputDevi

// Update the descriptor on the device with the information we got.
deviceDescription.capabilities = hidDeviceDescriptor.ToJson();
Debug.Log($"Parsing HID descriptor from JSON for device '{deviceDescription.capabilities}'");
}
else
{
Expand Down Expand Up @@ -385,7 +386,7 @@ public InputControlLayout Build()
var yElementParameters = yElement.DetermineParameters();

builder.AddControl(stickName + "/x")
.WithFormat(xElement.isSigned ? InputStateBlock.FormatSBit : InputStateBlock.FormatBit)
.WithFormat(xElement.DetermineFormat())
.WithByteOffset((uint)(xElement.reportOffsetInBits / 8 - byteOffset))
.WithBitOffset((uint)(xElement.reportOffsetInBits % 8))
.WithSizeInBits((uint)xElement.reportSizeInBits)
Expand All @@ -394,7 +395,7 @@ public InputControlLayout Build()
.WithProcessors(xElement.DetermineProcessors());

builder.AddControl(stickName + "/y")
.WithFormat(yElement.isSigned ? InputStateBlock.FormatSBit : InputStateBlock.FormatBit)
.WithFormat(yElement.DetermineFormat())
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change was included from #2245

Copy link
Collaborator

@ekcoh ekcoh Oct 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

.WithByteOffset((uint)(yElement.reportOffsetInBits / 8 - byteOffset))
.WithBitOffset((uint)(yElement.reportOffsetInBits % 8))
.WithSizeInBits((uint)yElement.reportSizeInBits)
Expand Down
6 changes: 3 additions & 3 deletions Packages/com.unity.inputsystem/InputSystem/Plugins/HID/HIDParser.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -267,7 +267,7 @@ private unsafe static int ReadData(int itemSize, byte* currentPtr, byte* endPtr)
{
if (currentPtr >= endPtr)
return 0;
return *currentPtr;
return (sbyte)*currentPtr;
}

// Read short.
Expand All @@ -277,7 +277,7 @@ private unsafe static int ReadData(int itemSize, byte* currentPtr, byte* endPtr)
return 0;
var data1 = *currentPtr;
var data2 = *(currentPtr + 1);
return (data2 << 8) | data1;
return (short)(data2 << 8) | data1;
}

// Read int.
Expand All @@ -291,7 +291,7 @@ private unsafe static int ReadData(int itemSize, byte* currentPtr, byte* endPtr)
var data3 = *(currentPtr + 2);
var data4 = *(currentPtr + 3);

return (data4 << 24) | (data3 << 24) | (data2 << 8) | data1;
return (data4 << 24) | (data3 << 16) | (data2 << 8) | data1;
}

Debug.Assert(false, "Should not reach here");
Expand Down