Closed Bug 694862 Opened 8 years ago Closed 8 years ago

Implement navigator.mozVibrate for B2G

Categories

(Core :: General, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla12

People

(Reporter: justin.lebar+bug, Assigned: justin.lebar+bug)

References

Details

Attachments

(1 file, 3 obsolete files)

Bug 679966 implements navigator.mozVibrate, but we need to implement it for B2G.
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.
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().
(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.
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.
I agree; the main thing we need to do is ensure that the alert gets though, one way or another.
(Sorry for the noise, test-driving our shiny new "OS" field.)
OS: Linux → Gonk
Hardware: x86_64 → ARM
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.
mwu, any idea why this file doesn't exist in Gonk?  Presumably we're not loading something or other...
vibrator.ko isn't loaded. I attempted to insmod it but apparently the kernel doesn't like it.
mwu says he fixed this, so the vibrator module can be loaded.  Just need to rebuild the kernel.

\o/
Depends on: 706958
Attached patch Patch v1 (obsolete) — Splinter Review
I tested this by adding my manual vibrator testcase to gaia.  I'll attach the gaia changes in a moment.
Attachment #578752 - Flags: review?(jones.chris.g)
Attached patch Gaia patch, v1 (obsolete) — Splinter Review
Assignee: nobody → justin.lebar+bug
Status: NEW → ASSIGNED
Attachment #578753 - Flags: review?(21)
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?
Let's just add the Gaia app and delete it once the browser kind of works?
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.
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.
(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?
It's not a demo; it's a testcase.

If you want to rename the vibrator app "manual tests", that's fine.
(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.
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 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.
Attachment #578752 - Flags: review?(jones.chris.g)
> 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.  :-/
> You're regressing style! :)

Honestly, I can never keep track of where the *s go, so I just shouldn't touch them.  :)
Attached patch Patch v2 (obsolete) — Splinter Review
Attachment #578752 - Attachment is obsolete: true
Attachment #579564 - Flags: review?(jones.chris.g)
Attachment #579564 - Attachment is obsolete: true
Attachment #579564 - Flags: review?(jones.chris.g)
Attachment #579600 - Flags: review?(jones.chris.g)
Attachment #579600 - Flags: review?(jones.chris.g) → review+
Whiteboard: [needs dependency]
Whiteboard: [needs dependency] → [needs landing]
This is ready to land, but is waiting for B2G --> m-c uplift to add GonkHal.cpp.
I'll happily rubberstamp a v0 patch to add it with only stubs.
(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!
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?
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.)
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...
Blocks: 709468
Depends on: 696042
Depends on: 711602
No longer depends on: 711602
Whiteboard: [needs landing]
https://hg.mozilla.org/mozilla-central/rev/3e861cc550d4
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla12
As it turns out I didn't actually commit this the first time.

https://hg.mozilla.org/integration/mozilla-inbound/rev/95760811d627
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
https://hg.mozilla.org/mozilla-central/rev/95760811d627
Status: REOPENED → RESOLVED
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.