1107801 - Improve gamepad support on MacOS

Status: RESOLVED FIXED

(Core :: Hardware Abstraction Layer (HAL), defect)

Description

[Randall Barker [:rbarker]]

On MacOS, the gamepad API does not support a number of buttons on the device. Buttons missing include: left and right shoulder triggers, home and back button, thumb stick buttons. Additionally, analog buttons are not supported.

Comment 1

[Randall Barker [:rbarker]]

Attachment: Gamepad patch v1

Comment 2

[Randall Barker [:rbarker]]

Comment on attachment 8532371 [details] [diff] [review]
Gamepad patch v1

While playing with the gamepad API on MacOS, I encountered a crash in nightly when a thumb stick button was pressed. The code was hitting an assert because the button ids on MacOS are not sequential. While fixing this issue I noticed the full gamepad was not supported. This patch adds missing buttons and adds support for analog buttons. I think the d-pad/hatswitch is the only thing not supported now.

Comment 3

[Randall Barker [:rbarker]]

Attachment: Gamepad patch v2

Added dpad support from :ted

Comment 4

[Randall Barker [:rbarker]]

Comment on attachment 8532733 [details] [diff] [review]
Gamepad patch v2

Updated patch to include dpad support.

Comment 5

[Randall Barker [:rbarker]]

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=d93c51c17db3

Comment 6

[(not currently active) Ted Mielczarek]

Comment on attachment 8532733 [details] [diff] [review]
Gamepad patch v2

Review of attachment 8532733 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for fixing this up!

::: hal/cocoa/CocoaGamepad.cpp
@@ +32,5 @@
> +    id(aId),
> +    analog((aMax - aMin) > 1),
> +    element(aElement),
> +    min(aMin),
> +    max(aMax) {;}

The semicolon here seems extraneous.

@@ +90,5 @@
>    // Index given by our superclass.
>    uint32_t mSuperIndex;
>  
> +  const bool isDpad(IOHIDElementRef element) const
> +  {

The positioning of open curly braces is inconsistent with the rest of the file. I don't know what the Mozilla style guide recommends, but can you figure that out and just make them all match the style guide?

@@ +173,5 @@
> +                  usage == kBrakeUsage)) ||
> +               (usagePage == kButtonUsagePage) ||
> +               (usagePage == kConsumerPage &&
> +                 (usage == kHomeUsage ||
> +                  usage == kBackUsage))) {

This is pretty depressing, but that's life with the USB HID.

Comment 7

[(not currently active) Ted Mielczarek]

(In reply to Randall Barker [:rbarker] from comment #0)
> On MacOS, the gamepad API does not support a number of buttons on the
> device. Buttons missing include: left and right shoulder triggers, home and
> back button, thumb stick buttons. Additionally, analog buttons are not
> supported.

To clarify: USB HID is weird, and devices make use of it in weird ways, and Randall was testing with a controller that I don't have that does things in an exceptionally weird way.

Comment 8

[(not currently active) Ted Mielczarek]

I bet we want a similar fix on Windows. If you have access to a Windows machine I could point you at what to fix. If not I might just have to get one of those controllers.

Comment 9

[(not currently active) Ted Mielczarek]

Were you going to land this? I got one of these gamepads and was playing around with it and it still crashes. :-(

Comment 10

[Randall Barker [:rbarker]]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #9)
> Were you going to land this? I got one of these gamepads and was playing
> around with it and it still crashes. :-(

Yes, I saw that the e10s work was being done and files were being moved around and was going to update the patch after than landed. Should I stop waiting and just land?

Comment 11

[(not currently active) Ted Mielczarek]

Yeah, just land it, qdot can rebase around your changes.

Comment 12

[Randall Barker [:rbarker]]

Attachment: 0001-Bug-1107801-Improve-gamepad-support-on-MacOS-15032309-4f26c4d.patch

Rebased to latest code. Addressed comments. Carry forward r+ from :ted.mielczarek

Comment 13

[Randall Barker [:rbarker]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a9f5309a0ed6

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/03eeae957008

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I had to back this out for a test failure in https://hg.mozilla.org/integration/mozilla-inbound/rev/812ea3cdd997

https://treeherder.mozilla.org/logviewer.html#?job_id=7954903&repo=mozilla-inbound

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Relanded in https://hg.mozilla.org/integration/mozilla-inbound/rev/242d0bc4a44c as the failure happened on another repository where this patch didn't exist, so this couldn't be at fault.

Sorry for jumping the gun.

Comment 17

[(not currently active) Ted Mielczarek]

Given the test that was failing I don't blame you!

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/242d0bc4a44c