Closed Bug 852935 Opened 11 years ago Closed 10 years ago

Android Gamepad backend

Categories

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

Product:
Core
Component:
Hardware Abstraction Layer (HAL)
Platform:
ARM
Android
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla32

People

(Reporter: ted, Assigned: ted)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete)

Attachments

(1 file, 3 obsolete files)

Add Android gamepad backend. rs=smaug
57.28 KB, patch
Details | Diff | Splinter Review 
We should implement an Android gamepad backend. This should be fairly straightforward, the Android SDK has a sample app that handles game controller input:
http://stuff.mit.edu/afs/sipb/project/android/docs/resources/samples/ApiDemos/src/com/example/android/apis/view/GameControllerInput.html

This seems to handle all the cases we might care about, it worked on my crap android device with a built-in gamepad, and it worked with a paired Wii remote.
This should mostly just involve wiring up the Android gamepad events to call the proper methods on GamepadService:
http://mxr.mozilla.org/mozilla-central/source/dom/gamepad/GamepadService.h#44

The GamepadService is a singleton, you call GamepadService::GetService to get a hold of it. You call AddGamepad, which returns an index that you can then pass to NewButtonEvent/NewAxisMoveEvent.

On other platforms we have hal backends to do this, since it usually involves adding some other sort of listener. If we're already getting these events into the Android event loop then that's not necessary.
Assignee: nobody → malmasry
Won't be able to finish this :(
Assignee: malmasry → nobody
Attached patch Add Android gamepad backend (obsolete) — — Splinter Review 
Here's a mostly-working patch. It works pretty well on my Nexus 4. I'm going to have to add some extra code because ImputManager was added in SDK level 16 (JB), so the device added/removed code won't work on older platforms. I should be able to just catch new MotionEvent/KeyEvents with unknown gamepad devices and handle them there.

Two things to note:
1) I've only tested this with a Nyko Playpad Pro on my Nexus 4. It could certainly stand some testing with some other controller/device combinations.
2) I'm not sure how this is going to work out on Ouya. We're eating the gamepad events before the browser gets them, so if you have a content page that wants gamepad input it will get it and you won't be able to use the controller to navigate the browser UI. We might be able to implement a way to "break out" of giving the events to the page by using some special button that the page doesn't have access to, I don't know. I also don't have an Ouya to test on, so testing the experience there would be useful.
Assignee: nobody → ted
Status: NEW → ASSIGNED
Here's a test build with this patch if anyone feels inclined:
http://people.mozilla.com/~tmielczarek/fennec-31.0a1.en-US.android-arm.apk
Attached patch Add Android gamepad backend (obsolete) — — Splinter Review 
Updated patch. This works well on my Nexus 4 and my Galaxy Tab. Looking for feedback at first since this is probably the first patch I've ever written for Firefox on Android. Most of the code is in AndroidGamepadService.java, the rest is hooks for passing down events from views and the C++ hooks for starting/stopping the service from HAL, as well as passing data through GeckoEvents to content.
Attachment #8411118 - Flags: feedback?(bugmail.mozilla)
Attachment #8408995 - Attachment is obsolete: true
Comment on attachment 8411118 [details] [diff] [review]
Add Android gamepad backend

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

Looks pretty good to me! Assorted comments below. wesj and snorp are probably good final reviewers for this. I think bnicholson has access to the Ouya in SF/MV and should be able to test the behaviour on there.

::: mobile/android/base/AndroidGamepadService.java
@@ +22,5 @@
> +import java.util.TimerTask;
> +import org.mozilla.gecko.GeckoAppShell;
> +import org.mozilla.gecko.GeckoEvent;
> +import org.mozilla.gecko.util.GamepadUtils;
> +import org.mozilla.gecko.util.ThreadUtils;

Organize imports by blocks separated by empty line: org.mozilla.*, com.*, net.*, org.*, android.*, then java.*

https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Java_practices

@@ +29,5 @@
> +    public static final int kStandardGamepadButtons = 17;
> +    public static final int kStandardGamepadAxes = 4;
> +    // This is completely arbitrary.
> +    public static final float kTriggerPressedThreshold = 0.25f;
> +    public static final long kPollTimerPeriod = 1000; // milliseconds

Java style uses UPPERCASE_SEPARATED_BY_UNDERSCORE for static final stuff.

Also, if values are arbitrary it would be good to make it them prefs so people can tune and provide feedback.

@@ +31,5 @@
> +    // This is completely arbitrary.
> +    public static final float kTriggerPressedThreshold = 0.25f;
> +    public static final long kPollTimerPeriod = 1000; // milliseconds
> +
> +    static final Integer[] kKnownAxes = {MotionEvent.AXIS_X,

If these are not used outside this class then better to make them private. Leaving the without any visibility qualifiers defaults to "package scope" which is more visible than private.

@@ +112,5 @@
> +    private AndroidGamepadService() {
> +    }
> +
> +    public static void Startup() {
> +        if (!sStarted) {

Startup() should be startup() (lowercase). Ditto for Shutdown(). It would be good to also add a ThreadUtils.assertOnUiThread() on all the public functions in this class to enforce the thread safety model and to make the code more self-documenting.

@@ +233,5 @@
> +        } else if (!sGamepads.containsKey(deviceId)) {
> +            InputDevice device = ev.getDevice();
> +            if (device != null &&
> +                (device.getSources() & InputDevice.SOURCE_GAMEPAD)
> +                == InputDevice.SOURCE_GAMEPAD) {

Feel free to not wrap this so heavily. Nobody follows 80-char wrapping in Java-land.

::: mobile/android/base/GeckoEvent.java
@@ +856,5 @@
> +        GeckoEvent event = GeckoEvent.get(NativeGeckoEvent.GAMEPAD_DATA);
> +        event.mID = id;
> +        event.mAction = 1;
> +        event.mFlags = boolArrayToBitfield(valid);
> +        event.mCount = values.length;

Probably a good idea to warn if valid.length != values.length. I assume valid.length will always be <= 32 here.

::: widget/android/nsAppShell.cpp
@@ +597,5 @@
> +#ifdef MOZ_GAMEPAD
> +        nsRefPtr<mozilla::dom::GamepadService> svc =
> +            mozilla::dom::GamepadService::GetService();
> +        if (svc) {
> +            if (curEvent->Action() == 1) {

Instead of hard-coding 1 and 0 you should add a couple of ACTION_xxx constants to GeckoEvent.java and AndroidJavaWrappers.h. See ACTION_MAGNIFY_START for example.
Attachment #8411118 - Flags: feedback?(bugmail.mozilla) → feedback+
Thanks for the feedback, very useful for a first-time contributor to our Java codebase. :)
Attached patch Add Android gamepad backend (obsolete) — — Splinter Review 
Cleaned up the things that kats mentioned in his feedback, so I think this is ready for a proper review now.
Attachment #8413723 - Flags: review?(snorp)
Attachment #8411118 - Attachment is obsolete: true
Comment on attachment 8413723 [details] [diff] [review]
Add Android gamepad backend

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

Looks good with a couple nits

::: mobile/android/base/AndroidGamepadService.java
@@ +27,5 @@
> +import java.util.Timer;
> +import java.util.TimerTask;
> +
> +
> +public class AndroidGamepadService {

Usually things that end in Service implement an Android Service. AndroidGamepadManager maybe?

@@ +30,5 @@
> +
> +public class AndroidGamepadService {
> +    // This is completely arbitrary.
> +    private static final float TRIGGER_PRESSED_THRESHOLD = 0.25f;
> +    private static final long POLL_TIMER_PERIOD = 1000; // milliseconds

Do we want prefs for these?

@@ +37,5 @@
> +                                                 MotionEvent.AXIS_Y,
> +                                                 MotionEvent.AXIS_Z,
> +                                                 MotionEvent.AXIS_RZ
> +    };
> +    private static final LinkedHashSet<Integer> VALID_AXES = new LinkedHashSet<Integer>(Arrays.asList(KNOWN_AXES));

Why not use a bitfield for this? I see you convert it to one before sending to Gecko anyway.

@@ +41,5 @@
> +    private static final LinkedHashSet<Integer> VALID_AXES = new LinkedHashSet<Integer>(Arrays.asList(KNOWN_AXES));
> +
> +    // A list of gamepad button mappings. Axes are determined at
> +    // runtime, as they vary by Android version.
> +    private static final Integer[] TRIGGERS = {6, 7};

Maybe use an enum?

@@ +64,5 @@
> +                                                    KeyEvent.KEYCODE_BUTTON_THUMBR,
> +                                                    KeyEvent.KEYCODE_DPAD_UP,
> +                                                    KeyEvent.KEYCODE_DPAD_DOWN,
> +                                                    KeyEvent.KEYCODE_DPAD_LEFT,
> +                                                    KeyEvent.KEYCODE_DPAD_RIGHT

enum again
Attachment #8413723 - Flags: review?(snorp) → review+
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #11)
> > +
> > +public class AndroidGamepadService {
> > +    // This is completely arbitrary.
> > +    private static final float TRIGGER_PRESSED_THRESHOLD = 0.25f;
> > +    private static final long POLL_TIMER_PERIOD = 1000; // milliseconds
> 
> Do we want prefs for these?

I don't think so. They're somewhat arbitrary, but there's not much value in tweaking them.

> > +    private static final LinkedHashSet<Integer> VALID_AXES = new LinkedHashSet<Integer>(Arrays.asList(KNOWN_AXES));
> 
> Why not use a bitfield for this? I see you convert it to one before sending
> to Gecko anyway.

I don't think that will quite work, I need an ordered list of the enum values, I use that to map them to axis positions on the standard gamepad mapping.
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #12)
> (In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #11)
> > > +
> > > +public class AndroidGamepadService {
> > > +    // This is completely arbitrary.
> > > +    private static final float TRIGGER_PRESSED_THRESHOLD = 0.25f;
> > > +    private static final long POLL_TIMER_PERIOD = 1000; // milliseconds
> > 
> > Do we want prefs for these?
> 
> I don't think so. They're somewhat arbitrary, but there's not much value in
> tweaking them.
> 
> > > +    private static final LinkedHashSet<Integer> VALID_AXES = new LinkedHashSet<Integer>(Arrays.asList(KNOWN_AXES));
> > 
> > Why not use a bitfield for this? I see you convert it to one before sending
> > to Gecko anyway.
> 
> I don't think that will quite work, I need an ordered list of the enum
> values, I use that to map them to axis positions on the standard gamepad
> mapping.

Ah, I see now.
Here's the updated patch that I intend to land. Using enums actually made a lot of that code look nicer, good call. I had to tweak configure a bit to make sure I wasn't enabling this on b2g, and I had to tweak the DOM test_interfaces.html to match the new reality (got rs=smaug on IRC for that change).
Attachment #8413723 - Attachment is obsolete: true
For reference, try build where I broke B2G (but everything else looks good):
https://tbpl.mozilla.org/?tree=Try&rev=40de7579a002

Try build where I fixed B2G:
https://tbpl.mozilla.org/?tree=Try&rev=c8a7f18396af
https://hg.mozilla.org/mozilla-central/rev/885b5b8fd7d9
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Ted: in order to update the documentation, does this bug means that the Gamepad API is available on Firefox for Android since version 32?
Flags: needinfo?(ted)
Yes, that's correct.
Flags: needinfo?(ted)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: