FIX: Filtering out axis jitter on Switch Pro / DualShock 4 / DualSense gamepads connected via HID (#1611)

Dmytro Ivanov · web-flow · commit efdcd78fc98e · 2022-11-23T16:50:23.000+01:00
diff --git a/Assets/Tests/InputSystem/Plugins/DualShockTests.cs b/Assets/Tests/InputSystem/Plugins/DualShockTests.cs
@@ -262,5 +262,149 @@ public void Devices_CanSetLightBarColorAndMotorSpeedsOnDualShockHID()
         Assert.That(receivedCommand.Value.blueColor, Is.EqualTo((byte)(0.5f * 255)));
     }
 
+    [Test]
+    [Category("Devices")]
+    public void Devices_DualSense_AxisJitter_DoesntMakeDeviceCurrent()
+    {
+        var device1 = InputSystem.AddDevice<DualSenseGamepadHID>();
+        var device2 = InputSystem.AddDevice<DualSenseGamepadHID>();
+        Assert.That(Gamepad.current, Is.EqualTo(device2));
+
+        // queuing state that is with-in axis dead zone doesn't make device current
+        InputSystem.QueueStateEvent(device1,
+            new DualSenseHIDInputReport
+            {
+                leftStickX = DualSenseGamepadHID.JitterMaskLow,
+                leftStickY = DualSenseGamepadHID.JitterMaskHigh,
+                rightStickX = DualSenseGamepadHID.JitterMaskHigh,
+                rightStickY = DualSenseGamepadHID.JitterMaskLow,
+                buttons0 = 8 // default dpad is at 8
+            });
+        InputSystem.Update();
+        Assert.That(Gamepad.current, Is.EqualTo(device2));
+
+        // queuing state that is outside of dead zone makes device current
+        InputSystem.QueueStateEvent(device1,
+            new DualSenseHIDInputReport
+            {
+                leftStickX = DualSenseGamepadHID.JitterMaskLow - 1,
+                leftStickY = DualSenseGamepadHID.JitterMaskHigh,
+                rightStickX = DualSenseGamepadHID.JitterMaskHigh,
+                rightStickY = DualSenseGamepadHID.JitterMaskLow,
+                buttons0 = 8 // default dpad is at 8
+            });
+        InputSystem.Update();
+        Assert.That(Gamepad.current, Is.EqualTo(device1));
+
+        // reset test
+        device2.MakeCurrent();
+        Assert.That(Gamepad.current, Is.EqualTo(device2));
+
+        // queuing state with button change makes device current
+        InputSystem.QueueStateEvent(device1,
+            new DualSenseHIDInputReport
+            {
+                leftStickX = DualSenseGamepadHID.JitterMaskLow,
+                leftStickY = DualSenseGamepadHID.JitterMaskHigh,
+                rightStickX = DualSenseGamepadHID.JitterMaskHigh,
+                rightStickY = DualSenseGamepadHID.JitterMaskLow,
+                buttons1 = 1,
+                buttons0 = 8 // default dpad is at 8
+            });
+        InputSystem.Update();
+        Assert.That(Gamepad.current, Is.EqualTo(device1));
+
+        // reset test
+        device2.MakeCurrent();
+        Assert.That(Gamepad.current, Is.EqualTo(device2));
+
+        // queuing state with trigger change makes device current
+        InputSystem.QueueStateEvent(device1,
+            new DualSenseHIDInputReport
+            {
+                leftStickX = DualSenseGamepadHID.JitterMaskLow,
+                leftStickY = DualSenseGamepadHID.JitterMaskHigh,
+                rightStickX = DualSenseGamepadHID.JitterMaskHigh,
+                rightStickY = DualSenseGamepadHID.JitterMaskLow,
+                buttons1 = 1,
+                leftTrigger = 1,
+                buttons0 = 8 // default dpad is at 8
+            });
+        InputSystem.Update();
+        Assert.That(Gamepad.current, Is.EqualTo(device1));
+    }
+
+    [Test]
+    [Category("Devices")]
+    public void Devices_DualShock4_AxisJitter_DoesntMakeDeviceCurrent()
+    {
+        var device1 = InputSystem.AddDevice<DualShock4GamepadHID>();
+        var device2 = InputSystem.AddDevice<DualShock4GamepadHID>();
+        Assert.That(Gamepad.current, Is.EqualTo(device2));
+
+        // queuing state that is with-in axis dead zone doesn't make device current
+        InputSystem.QueueStateEvent(device1,
+            new DualShock4HIDInputReport()
+            {
+                leftStickX = DualShock4GamepadHID.JitterMaskLow,
+                leftStickY = DualShock4GamepadHID.JitterMaskHigh,
+                rightStickX = DualShock4GamepadHID.JitterMaskHigh,
+                rightStickY = DualShock4GamepadHID.JitterMaskLow,
+                buttons1 = 8 // default dpad is at 8
+            });
+        InputSystem.Update();
+        Assert.That(Gamepad.current, Is.EqualTo(device2));
+
+        // queuing state that is outside of dead zone makes device current
+        InputSystem.QueueStateEvent(device1,
+            new DualShock4HIDInputReport
+            {
+                leftStickX = DualShock4GamepadHID.JitterMaskLow - 1,
+                leftStickY = DualShock4GamepadHID.JitterMaskHigh,
+                rightStickX = DualShock4GamepadHID.JitterMaskHigh,
+                rightStickY = DualShock4GamepadHID.JitterMaskLow,
+                buttons1 = 8 // default dpad is at 8
+            });
+        InputSystem.Update();
+        Assert.That(Gamepad.current, Is.EqualTo(device1));
+
+        // reset test
+        device2.MakeCurrent();
+        Assert.That(Gamepad.current, Is.EqualTo(device2));
+
+        // queuing state with button change makes device current
+        InputSystem.QueueStateEvent(device1,
+            new DualShock4HIDInputReport
+            {
+                leftStickX = DualShock4GamepadHID.JitterMaskLow,
+                leftStickY = DualShock4GamepadHID.JitterMaskHigh,
+                rightStickX = DualShock4GamepadHID.JitterMaskHigh,
+                rightStickY = DualShock4GamepadHID.JitterMaskLow,
+                buttons2 = 1,
+                buttons1 = 8 // default dpad is at 8
+            });
+        InputSystem.Update();
+        Assert.That(Gamepad.current, Is.EqualTo(device1));
+
+        // reset test
+        device2.MakeCurrent();
+        Assert.That(Gamepad.current, Is.EqualTo(device2));
+
+        // queuing state with trigger change makes device current
+        InputSystem.QueueStateEvent(device1,
+            new DualShock4HIDInputReport
+            {
+                leftStickX = DualShock4GamepadHID.JitterMaskLow,
+                leftStickY = DualShock4GamepadHID.JitterMaskHigh,
+                rightStickX = DualShock4GamepadHID.JitterMaskHigh,
+                rightStickY = DualShock4GamepadHID.JitterMaskLow,
+                buttons2 = 1,
+                leftTrigger = 1,
+                buttons1 = 8 // default dpad is at 8
+            });
+        InputSystem.Update();
+        Assert.That(Gamepad.current, Is.EqualTo(device1));
+    }
+
 #endif
 }
diff --git a/Assets/Tests/InputSystem/SwitchTests.cs b/Assets/Tests/InputSystem/SwitchTests.cs
@@ -80,6 +80,55 @@ private static SwitchProControllerHIDInputState StateWithButton(SwitchProControl
         }.WithButton(button);
     }
 
+    [Test]
+    [Category("Devices")]
+    public void Devices_SwitchPro_AxisJitter_DoesntMakeDeviceCurrent()
+    {
+        var device1 = InputSystem.AddDevice<SwitchProControllerHID>();
+        var device2 = InputSystem.AddDevice<SwitchProControllerHID>();
+        Assert.That(Gamepad.current, Is.EqualTo(device2));
+
+        // queuing state that is with-in axis dead zone doesn't make device current
+        InputSystem.QueueStateEvent(device1,
+            new SwitchProControllerHIDInputState
+            {
+                leftStickX = SwitchProControllerHID.JitterMaskLow,
+                leftStickY = SwitchProControllerHID.JitterMaskHigh,
+                rightStickX = SwitchProControllerHID.JitterMaskHigh,
+                rightStickY = SwitchProControllerHID.JitterMaskLow,
+            });
+        InputSystem.Update();
+        Assert.That(Gamepad.current, Is.EqualTo(device2));
+
+        // queuing state that is outside of dead zone makes device current
+        InputSystem.QueueStateEvent(device1,
+            new SwitchProControllerHIDInputState
+            {
+                leftStickX = SwitchProControllerHID.JitterMaskLow - 1,
+                leftStickY = SwitchProControllerHID.JitterMaskHigh,
+                rightStickX = SwitchProControllerHID.JitterMaskHigh,
+                rightStickY = SwitchProControllerHID.JitterMaskLow,
+            });
+        InputSystem.Update();
+        Assert.That(Gamepad.current, Is.EqualTo(device1));
+
+        // reset test
+        device2.MakeCurrent();
+        Assert.That(Gamepad.current, Is.EqualTo(device2));
+
+        // queuing state with button change makes device current
+        InputSystem.QueueStateEvent(device1,
+            new SwitchProControllerHIDInputState
+            {
+                leftStickX = SwitchProControllerHID.JitterMaskLow,
+                leftStickY = SwitchProControllerHID.JitterMaskHigh,
+                rightStickX = SwitchProControllerHID.JitterMaskHigh,
+                rightStickY = SwitchProControllerHID.JitterMaskLow,
+            }.WithButton(SwitchProControllerHIDInputState.Button.A));
+        InputSystem.Update();
+        Assert.That(Gamepad.current, Is.EqualTo(device1));
+    }
+
     [Test]
     [Category("Devices")]
     [TestCase(0x0f0d, 0x0092)]
diff --git a/Packages/com.unity.inputsystem/CHANGELOG.md b/Packages/com.unity.inputsystem/CHANGELOG.md
@@ -17,6 +17,7 @@ however, it has to be formatted properly to pass verification tests.
 ### Fixed
 - Fixed composite bindings incorrectly getting a control scheme assigned when pasting into input asset editor with a control scheme selected.
 - Fixed an issue on PS5 where device disconnected events that happen while the app is in the background are missed causing orphaned devices to hang around forever and exceptions when the same device is added again ([case UUM-7842](https://issuetracker.unity3d.com/product/unity/issues/guid/UUM-6744)).
+- Fixed Switch Pro, DualShock 4, DualSense gamepads becoming current on PC/macOS when no controls are changing ([case ISXB-223](https://issuetracker.unity3d.com/product/unity/issues/guid/ISXB-223))).
 
 ### Actions
 - Extended input action code generator (`InputActionCodeGenerator.cs`) to support optional registration and unregistration of callbacks for multiple callback instances via `AddCallbacks(...)` and `RemoveCallbacks(...)` part of the generated code. Contribution by [Ramobo](https://github.com/Ramobo) in [#889](https://github.com/Unity-Technologies/InputSystem/pull/889).
diff --git a/Packages/com.unity.inputsystem/InputSystem/InputManager.cs b/Packages/com.unity.inputsystem/InputSystem/InputManager.cs
@@ -3172,9 +3172,12 @@ private unsafe void OnUpdate(InputUpdateType updateType, ref InputEventBuffer ev
                             var haveChangedStateOtherThanNoise = true;
                             if (deviceIsStateCallbackReceiver)
                             {
+                                m_ShouldMakeCurrentlyUpdatingDeviceCurrent = true;
                                 // NOTE: We leave it to the device to make sure the event has the right format. This allows the
                                 //       device to handle multiple different incoming formats.
                                 ((IInputStateCallbackReceiver)device).OnStateEvent(eventPtr);
+
+                                haveChangedStateOtherThanNoise = m_ShouldMakeCurrentlyUpdatingDeviceCurrent;
                             }
                             else
                             {
@@ -3314,6 +3317,15 @@ private void InvokeAfterUpdateCallback(InputUpdateType updateType)
                 "InputSystem.onAfterUpdate");
         }
 
+        private bool m_ShouldMakeCurrentlyUpdatingDeviceCurrent;
+
+        // This is a dirty hot fix to expose entropy from device back to input manager to make a choice if we want to make device current or not.
+        // A proper fix would be to change IInputStateCallbackReceiver.OnStateEvent to return bool to make device current or not.
+        internal void DontMakeCurrentlyUpdatingDeviceCurrent()
+        {
+            m_ShouldMakeCurrentlyUpdatingDeviceCurrent = false;
+        }
+
         internal unsafe bool UpdateState(InputDevice device, InputEvent* eventPtr, InputUpdateType updateType)
         {
             Debug.Assert(eventPtr != null, "Received NULL event ptr");
diff --git a/Packages/com.unity.inputsystem/InputSystem/Plugins/DualShock/DualShockGamepadHID.cs b/Packages/com.unity.inputsystem/InputSystem/Plugins/DualShock/DualShockGamepadHID.cs
@@ -334,7 +334,7 @@ namespace UnityEngine.InputSystem.DualShock
     /// PS5 DualSense controller that is interfaced to a HID backend.
     /// </summary>
     [InputControlLayout(stateType = typeof(DualSenseHIDInputReport), displayName = "DualSense HID")]
-    public class DualSenseGamepadHID : DualShockGamepad, IEventMerger, IEventPreProcessor
+    public class DualSenseGamepadHID : DualShockGamepad, IEventMerger, IEventPreProcessor, IInputStateCallbackReceiver
     {
         // Gamepad might send 3 types of input reports:
         // - Minimal report, first byte is 0x01, observed size is 78, also can be 10
@@ -516,6 +516,9 @@ unsafe bool IEventPreProcessor.PreProcessEvent(InputEventPtr eventPtr)
                 return eventPtr.type != DeltaStateEvent.Type; // only skip delta state events
 
             var stateEvent = StateEvent.FromUnchecked(eventPtr);
+            if (stateEvent->stateFormat == DualSenseHIDInputReport.Format)
+                return true; // if someone queued DSVS directly, just use as-is
+
             var size = stateEvent->stateSizeInBytes;
             if (stateEvent->stateFormat != DualSenseHIDGenericInputReport.Format || size < sizeof(DualSenseHIDInputReport))
                 return false; // skip unrecognized state events otherwise they will corrupt control states
@@ -549,6 +552,50 @@ unsafe bool IEventPreProcessor.PreProcessEvent(InputEventPtr eventPtr)
                 return false; // skip unrecognized reportId
         }
 
+        public void OnNextUpdate()
+        {
+        }
+
+        // filter out three lower bits as jitter noise
+        internal const byte JitterMaskLow = 0b01111000;
+        internal const byte JitterMaskHigh = 0b10000111;
+
+        public unsafe void OnStateEvent(InputEventPtr eventPtr)
+        {
+            if (eventPtr.type == StateEvent.Type && eventPtr.stateFormat == DualSenseHIDInputReport.Format)
+            {
+                var currentState = (DualSenseHIDInputReport*)((byte*)currentStatePtr + m_StateBlock.byteOffset);
+                var newState = (DualSenseHIDInputReport*)StateEvent.FromUnchecked(eventPtr)->state;
+
+                var actuated =
+                    // we need to make device current if axes are outside of deadzone specifying hardware jitter of sticks around zero point
+                    newState->leftStickX<JitterMaskLow
+                                         || newState->leftStickX> JitterMaskHigh
+                    || newState->leftStickY<JitterMaskLow
+                                            || newState->leftStickY> JitterMaskHigh
+                    || newState->rightStickX<JitterMaskLow
+                                             || newState->rightStickX> JitterMaskHigh
+                    || newState->rightStickY<JitterMaskLow
+                                             || newState->rightStickY> JitterMaskHigh
+                    // we need to make device current if triggers or buttons state change
+                    || newState->leftTrigger != currentState->leftTrigger
+                    || newState->rightTrigger != currentState->rightTrigger
+                    || newState->buttons0 != currentState->buttons0
+                    || newState->buttons1 != currentState->buttons1
+                    || newState->buttons2 != currentState->buttons2;
+
+                if (!actuated)
+                    InputSystem.s_Manager.DontMakeCurrentlyUpdatingDeviceCurrent();
+            }
+
+            InputState.Change(this, eventPtr);
+        }
+
+        public bool GetStateOffsetForEvent(InputControl control, InputEventPtr eventPtr, ref uint offset)
+        {
+            return false;
+        }
+
         [StructLayout(LayoutKind.Explicit)]
         internal struct DualSenseHIDGenericInputReport
         {
@@ -665,7 +712,7 @@ public DualSenseHIDInputReport ToHIDInputReport()
     /// PS4 DualShock controller that is interfaced to a HID backend.
     /// </summary>
     [InputControlLayout(stateType = typeof(DualShock4HIDInputReport), hideInUI = true, isNoisy = true)]
-    public class DualShock4GamepadHID : DualShockGamepad, IEventPreProcessor
+    public class DualShock4GamepadHID : DualShockGamepad, IEventPreProcessor, IInputStateCallbackReceiver
     {
         public ButtonControl leftTriggerButton { get; protected set; }
         public ButtonControl rightTriggerButton { get; protected set; }
@@ -791,8 +838,10 @@ unsafe bool IEventPreProcessor.PreProcessEvent(InputEventPtr eventPtr)
                 return eventPtr.type != DeltaStateEvent.Type; // only skip delta state events
 
             var stateEvent = StateEvent.FromUnchecked(eventPtr);
-            var size = stateEvent->stateSizeInBytes;
+            if (stateEvent->stateFormat == DualShock4HIDInputReport.Format)
+                return true; // if someone queued D4VS directly, just use as-is
 
+            var size = stateEvent->stateSizeInBytes;
             if (stateEvent->stateFormat != DualShock4HIDGenericInputReport.Format || size < sizeof(DualShock4HIDGenericInputReport))
                 return false; // skip unrecognized state events otherwise they will corrupt control states
 
@@ -838,6 +887,50 @@ unsafe bool IEventPreProcessor.PreProcessEvent(InputEventPtr eventPtr)
             }
         }
 
+        public void OnNextUpdate()
+        {
+        }
+
+        // filter out three lower bits as jitter noise
+        internal const byte JitterMaskLow = 0b01111000;
+        internal const byte JitterMaskHigh = 0b10000111;
+
+        public unsafe void OnStateEvent(InputEventPtr eventPtr)
+        {
+            if (eventPtr.type == StateEvent.Type && eventPtr.stateFormat == DualShock4HIDInputReport.Format)
+            {
+                var currentState = (DualShock4HIDInputReport*)((byte*)currentStatePtr + m_StateBlock.byteOffset);
+                var newState = (DualShock4HIDInputReport*)StateEvent.FromUnchecked(eventPtr)->state;
+
+                var actuatedOrChanged =
+                    // we need to make device current if axes are outside of deadzone specifying hardware jitter of sticks around zero point
+                    newState->leftStickX<JitterMaskLow
+                                         || newState->leftStickX> JitterMaskHigh
+                    || newState->leftStickY<JitterMaskLow
+                                            || newState->leftStickY> JitterMaskHigh
+                    || newState->rightStickX<JitterMaskLow
+                                             || newState->rightStickX> JitterMaskHigh
+                    || newState->rightStickY<JitterMaskLow
+                                             || newState->rightStickY> JitterMaskHigh
+                    // we need to make device current if triggers or buttons state change
+                    || newState->leftTrigger != currentState->leftTrigger
+                    || newState->rightTrigger != currentState->rightTrigger
+                    || newState->buttons1 != currentState->buttons1
+                    || newState->buttons2 != currentState->buttons2
+                    || newState->buttons3 != currentState->buttons3;
+
+                if (!actuatedOrChanged)
+                    InputSystem.s_Manager.DontMakeCurrentlyUpdatingDeviceCurrent();
+            }
+
+            InputState.Change(this, eventPtr);
+        }
+
+        public bool GetStateOffsetForEvent(InputControl control, InputEventPtr eventPtr, ref uint offset)
+        {
+            return false;
+        }
+
         [StructLayout(LayoutKind.Explicit)]
         internal struct DualShock4HIDGenericInputReport
         {
diff --git a/Packages/com.unity.inputsystem/InputSystem/Plugins/Switch/SwitchProControllerHID.cs b/Packages/com.unity.inputsystem/InputSystem/Plugins/Switch/SwitchProControllerHID.cs

Original file line number	Diff line number	Diff line change
`@@ -3172,9 +3172,12 @@ private unsafe void OnUpdate(InputUpdateType updateType, ref InputEventBuffer ev`
`3172`	`3172`	`var haveChangedStateOtherThanNoise = true;`
`3173`	`3173`	`if (deviceIsStateCallbackReceiver)`
`3174`	`3174`	`{`
	`3175`	`+ m_ShouldMakeCurrentlyUpdatingDeviceCurrent = true;`
`3175`	`3176`	`// NOTE: We leave it to the device to make sure the event has the right format. This allows the`
`3176`	`3177`	`// device to handle multiple different incoming formats.`
`3177`	`3178`	`((IInputStateCallbackReceiver)device).OnStateEvent(eventPtr);`
	`3179`	`+`
	`3180`	`+ haveChangedStateOtherThanNoise = m_ShouldMakeCurrentlyUpdatingDeviceCurrent;`
`3178`	`3181`	`}`
`3179`	`3182`	`else`
`3180`	`3183`	`{`
`@@ -3314,6 +3317,15 @@ private void InvokeAfterUpdateCallback(InputUpdateType updateType)`
`3314`	`3317`	`"InputSystem.onAfterUpdate");`
`3315`	`3318`	`}`
`3316`	`3319`
	`3320`	`+ private bool m_ShouldMakeCurrentlyUpdatingDeviceCurrent;`
	`3321`	`+`
	`3322`	`+ // This is a dirty hot fix to expose entropy from device back to input manager to make a choice if we want to make device current or not.`
	`3323`	`+ // A proper fix would be to change IInputStateCallbackReceiver.OnStateEvent to return bool to make device current or not.`
	`3324`	`+ internal void DontMakeCurrentlyUpdatingDeviceCurrent()`
	`3325`	`+ {`
	`3326`	`+ m_ShouldMakeCurrentlyUpdatingDeviceCurrent = false;`
	`3327`	`+ }`
	`3328`	`+`
`3317`	`3329`	`internal unsafe bool UpdateState(InputDevice device, InputEvent* eventPtr, InputUpdateType updateType)`
`3318`	`3330`	`{`
`3319`	`3331`	`Debug.Assert(eventPtr != null, "Received NULL event ptr");`