Last Comment Bug 694862 - Implement navigator.mozVibrate for B2G
: Implement navigator.mozVibrate for B2G
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: General (show other bugs)
: unspecified
: ARM Gonk (Firefox OS)
: -- normal (vote)
: mozilla12
Assigned To: Justin Lebar (not reading bugmail)
:
Mentors:
Depends on: 679966 696042 706958
Blocks: 709468
  Show dependency treegraph
 
Reported: 2011-10-16 10:58 PDT by Justin Lebar (not reading bugmail)
Modified: 2011-12-22 03:47 PST (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1 (5.14 KB, patch)
2011-12-02 15:30 PST, Justin Lebar (not reading bugmail)
no flags Details | Diff | Splinter Review
Gaia patch, v1 (4.89 KB, patch)
2011-12-02 15:31 PST, Justin Lebar (not reading bugmail)
no flags Details | Diff | Splinter Review
Patch v2 (30.12 KB, patch)
2011-12-06 18:43 PST, Justin Lebar (not reading bugmail)
no flags Details | Diff | Splinter Review
Patch v2 (right version) (6.88 KB, patch)
2011-12-06 23:04 PST, Justin Lebar (not reading bugmail)
cjones.bugs: review+
Details | Diff | Splinter Review

Description Justin Lebar (not reading bugmail) 2011-10-16 10:58:04 PDT
Bug 679966 implements navigator.mozVibrate, but we need to implement it for B2G.
Comment 1 Justin Lebar (not reading bugmail) 2011-10-16 10:59:48 PDT
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 2011-10-16 14:50:23 PDT
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) 2011-10-16 15:08:07 PDT
(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 2011-10-16 15:16:28 PDT
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) 2011-10-16 15:36:25 PDT
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 2011-10-26 00:13:24 PDT
(Sorry for the noise, test-driving our shiny new "OS" field.)
Comment 7 Justin Lebar (not reading bugmail) 2011-11-17 14:34:32 PST
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) 2011-11-17 14:37:14 PST
mwu, any idea why this file doesn't exist in Gonk?  Presumably we're not loading something or other...
Comment 9 Michael Wu [:mwu] 2011-11-17 14:45:41 PST
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) 2011-11-22 17:17:03 PST
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) 2011-12-02 15:30:24 PST
Created attachment 578752 [details] [diff] [review]
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) 2011-12-02 15:31:28 PST
Created attachment 578753 [details] [diff] [review]
Gaia patch, v1
Comment 13 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2011-12-04 23:13:10 PST
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) 2011-12-04 23:32:54 PST
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) 2011-12-04 23:47:22 PST
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) 2011-12-04 23:49:43 PST
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) 2011-12-05 00:08:03 PST
(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) 2011-12-05 00:15:51 PST
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 2011-12-05 05:32:31 PST
(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) 2011-12-05 22:33:47 PST
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 2011-12-06 00:21:51 PST
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) 2011-12-06 00:30:09 PST
> 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) 2011-12-06 00:51:16 PST
> 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) 2011-12-06 18:36:28 PST
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) 2011-12-06 18:43:46 PST
Created attachment 579564 [details] [diff] [review]
Patch v2
Comment 26 Justin Lebar (not reading bugmail) 2011-12-06 23:04:18 PST
Created attachment 579600 [details] [diff] [review]
Patch v2 (right version)
Comment 27 Justin Lebar (not reading bugmail) 2011-12-15 08:46:03 PST
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 2011-12-15 16:06:07 PST
I'll happily rubberstamp a v0 patch to add it with only stubs.
Comment 29 Justin Lebar (not reading bugmail) 2011-12-16 08:18:18 PST
(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 2011-12-16 08:48:57 PST
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) 2011-12-16 09:10:38 PST
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) 2011-12-16 10:05:45 PST
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 35 Ed Morley [:emorley] 2011-12-21 04:33:23 PST
https://hg.mozilla.org/mozilla-central/rev/3e861cc550d4
Comment 36 Michael Wu [:mwu] 2011-12-21 18:00:04 PST
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] 2011-12-22 03:47:11 PST
https://hg.mozilla.org/mozilla-central/rev/95760811d627

Note You need to log in before you can comment on or make changes to this bug.