694862 - Implement navigator.mozVibrate for B2G

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[Justin Lebar (not reading bugmail)]

Bug 679966 implements navigator.mozVibrate, but we need to implement it for B2G.

Comment 1

[Justin Lebar (not reading bugmail)]

Note that at some point (perhaps not here), we need to revisit the model of "only the foreground page can activate the vibrator."  Privileged apps ought to be able to vibrate the device even when in the background.

Comment 2

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

I don't know of use cases yet for raw vibrate() access from background pages.  We definitely want to allow background pages to indirectly frob the vibrator through notification alerts, e.g., but that will Just Work without changes to vibrate().

Comment 3

[Justin Lebar (not reading bugmail)]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #2)
> I don't know of use cases yet for raw vibrate() access from background
> pages.  We definitely want to allow background pages to indirectly frob the
> vibrator through notification alerts, e.g., but that will Just Work without
> changes to vibrate().

Okay.

We still need some way to keep vibrations started from webpages and vibrations started from notifications from stepping on each other.  But we can probably do this by maintaining a (depth-two?) stack of vibration arrays.  When a notification comes in, we push the notification vibration pattern onto the stack, play that pattern, and then pop it off and play whatever remains of the previous pattern.

Comment 4

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

I'm not /too/ worried about that ... the #1 priority would be ensuring the alert isn't stomped, but if a page is vibrating like crazy then I think it's likely the alert would be lost as noise anyway.  We would be saved by a visual indicator.  The flip side that I don't care much about is a page doing vibrate(10000) or something and an alert interrupting that.  You solution sounds good for both cases.

Another thing we can do is treat vibration patterns like audio samples and then "mix" multiple channels.  That wouldn't be too hard.  It's also worth looking at what android does.

Comment 5

[Justin Lebar (not reading bugmail)]

I agree; the main thing we need to do is ensure that the alert gets though, one way or another.

Comment 6

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(Sorry for the noise, test-driving our shiny new "OS" field.)

Comment 7

[Justin Lebar (not reading bugmail)]

The file Android frobs is

  /sys/class/timed_output/vibrator/enable

But that doesn't exist on my Galaxy S II running Gonk; the directory /sys/class/timed_output is empty.

Comment 8

[Justin Lebar (not reading bugmail)]

mwu, any idea why this file doesn't exist in Gonk?  Presumably we're not loading something or other...

Comment 9

[Michael Wu [:mwu]]

vibrator.ko isn't loaded. I attempted to insmod it but apparently the kernel doesn't like it.

Comment 10

[Justin Lebar (not reading bugmail)]

mwu says he fixed this, so the vibrator module can be loaded.  Just need to rebuild the kernel.

\o/

Comment 11

[Justin Lebar (not reading bugmail)]

Attachment: Patch v1

I tested this by adding my manual vibrator testcase to gaia.  I'll attach the gaia changes in a moment.

Comment 12

[Justin Lebar (not reading bugmail)]

Attachment: Gaia patch, v1

Comment 13

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

The Gaia code sounds fine but I feel uncomfortable to add an app to Gaia that is only used for testing purpose.

What about adding mozVibrate to the dialer's keys instead?

Comment 14

[Justin Lebar (not reading bugmail)]

Let's just add the Gaia app and delete it once the browser kind of works?

Comment 15

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

The browser should works now.
The keyboard should works too but for some reasons it's broken on the device, let me until tomorrow night to fix before landing the app please.

Comment 16

[Justin Lebar (not reading bugmail)]

At this point, half of the Gaia apps are black screens which say "Hello".

Once we have wifi, keyboard, browser, scrolling, etc, then we definitely don't need this app.  But for now, all the apps are clearly for testing, so I don't see why it's so bad to have an app for testing this.

Comment 17

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Justin Lebar [:jlebar] from comment #16)
> At this point, half of the Gaia apps are black screens which say "Hello".
> 
> Once we have wifi, keyboard, browser, scrolling, etc, then we definitely
> don't need this app.  But for now, all the apps are clearly for testing, so
> I don't see why it's so bad to have an app for testing this.

As I have said I just feel uncomfortable with this landing 'Vibrator' as an app by itself. What do you think of grouping this into a 'Demo' app that is a list of cool stuff we can do with the device?

Comment 18

[Justin Lebar (not reading bugmail)]

It's not a demo; it's a testcase.

If you want to rename the vibrator app "manual tests", that's fine.

Comment 19

[Robert Kaiser]

(In reply to Vivien Nicolas (:vingtetun) from comment #13)
> What about adding mozVibrate to the dialer's keys instead?

"Haptic" feedback to dialer key presses with vibration would be great in any case, I love my N9 doing that, it feels great, we should do that as well.

Comment 20

[Justin Lebar (not reading bugmail)]

Gal pointed out that we can go through libhardwarelegacy.so instead of touching this file.  The advantage would be that hardware manufacturers can customize this solib.

Comment 21

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 578752 [details] [diff] [review]
Patch v1

I would have been fine with this using pthread directly, but I don't
think it would have been less code.

>diff --git a/hal/gonk/GonkHal.cpp b/hal/gonk/GonkHal.cpp

>+class VibratorRunnable

>+  Mutex mMutex;
>+  CondVar mCondVar;

Use Monitor.

>+  nsTArray<uint32> mPattern;
>+  uint32 mIndex;
>+  bool mShuttingDown;

Document these params.

>+VibratorRunnable::FrobVibrator(uint32 duration)
>+{

Use vibrate_on() here.  It works with qemu too.

>+void
>+VibratorRunnable::CancelVibrate()
>+{

This needs to hold mMutex.  This should have tripped a fatal assert if
run in a debug build ...

>+ VibratorRunnable *sVibratorRunnable = NULL;

Nothing frees this, it's going to show up as a leak.

> void
>-GetCurrentBatteryInformation(hal::BatteryInformation* aBatteryInfo)
>+GetCurrentBatteryInformation(hal::BatteryInformation *aBatteryInfo)

You're regressing style! :)

Would like to see the next version.

Comment 22

[Justin Lebar (not reading bugmail)]

> Nothing frees this, it's going to show up as a leak.

It's a raw ptr...I need to free the runnable, even when the thread exits?

I also noticed I was missing a condvar notify in the shutdown observer.  Threads are hard.  :-/

Comment 23

[Justin Lebar (not reading bugmail)]

> You're regressing style! :)

Honestly, I can never keep track of where the *s go, so I just shouldn't touch them.  :)

Comment 24

[Justin Lebar (not reading bugmail)]

Comment on attachment 578753 [details] [diff] [review]
Gaia patch, v1

I pushed this to gaia: https://github.com/andreasgal/gaia/commit/4295bc5ac4f16111f4540f7371d206b9e6214fbb

Comment 25

[Justin Lebar (not reading bugmail)]

Attachment: Patch v2

Comment 26

[Justin Lebar (not reading bugmail)]

Attachment: Patch v2 (right version)

Comment 27

[Justin Lebar (not reading bugmail)]

This is ready to land, but is waiting for B2G --> m-c uplift to add GonkHal.cpp.

Comment 28

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

I'll happily rubberstamp a v0 patch to add it with only stubs.

Comment 29

[Justin Lebar (not reading bugmail)]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #28)
> I'll happily rubberstamp a v0 patch to add it with only stubs.

I don't think I'd gain anything by doing that.  It's gonk-only, so in theory, nobody else is landing in the B2G repository...  I'm happy to be patient!

Comment 30

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

I'd like to get this in asap.  Our friends want to use it.

Which bug are we blocked on here fir creating the file?

Comment 31

[Justin Lebar (not reading bugmail)]

At this point, isn't much of b2g landing blocked on the uplift?  GonkHal was created for the battery notifications, and touched by the API for enabling/disabling the screen.

I'm trying to get in touch with philikon to get an ETA on the uplift, but no luck yet.  I'd rather not spend time hacking a backport and make his uplift harder unless it's not going to be done soon.

Do our friends need a working vibrator on Gonk Right This Hour?  Or Before The Weekend?  What's the necessary timeframe here?  (Note that even after we land in m-c, the change needs to be pulled into doublec and then pulled into b2g.)

Comment 32

[Justin Lebar (not reading bugmail)]

mwu gave me leave to check this straight into b2g.  There won't be any upstream conflicts with m-c, since GonkHal doesn't even exist in m-c...

Comment 33

[Justin Lebar (not reading bugmail)]

Pushed to b2g.  mwu is taking care of the m-c landing.

https://github.com/cgjones/mozilla-central/commit/c9bdffa52e12b7c76a99517dca8dc7dc7e8eabff
https://github.com/cgjones/mozilla-central/commit/d8d7be98992b8579857f204c10141d420080c608
https://github.com/andreasgal/B2G/commit/d8b6add1aea679847f7233ec6483fcaa8670d09b

Comment 34

[Michael Wu [:mwu]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3e861cc550d4

Comment 35

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/3e861cc550d4

Comment 36

[Michael Wu [:mwu]]

As it turns out I didn't actually commit this the first time.

https://hg.mozilla.org/integration/mozilla-inbound/rev/95760811d627

Comment 37

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/95760811d627