Implement navigator.mozVibrate for B2G

RESOLVED FIXED in mozilla12

Status

()

Core
General
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: Justin Lebar (not reading bugmail), Assigned: Justin Lebar (not reading bugmail))

Tracking

(Blocks: 1 bug)

unspecified
mozilla12
ARM
Gonk (Firefox OS)
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

6 years ago
Bug 679966 implements navigator.mozVibrate, but we need to implement it for B2G.
(Assignee)

Comment 1

6 years ago
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().
(Assignee)

Comment 3

6 years ago
(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.
(Assignee)

Comment 5

6 years ago
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
(Assignee)

Comment 7

6 years ago
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.
(Assignee)

Comment 8

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

Comment 9

6 years ago
vibrator.ko isn't loaded. I attempted to insmod it but apparently the kernel doesn't like it.
(Assignee)

Comment 10

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

\o/
(Assignee)

Updated

6 years ago
Depends on: 706958
(Assignee)

Comment 11

6 years ago
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.
Attachment #578752 - Flags: review?(jones.chris.g)
(Assignee)

Comment 12

6 years ago
Created attachment 578753 [details] [diff] [review]
Gaia patch, v1
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?
(Assignee)

Comment 14

6 years ago
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.
(Assignee)

Comment 16

6 years ago
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?
(Assignee)

Comment 18

6 years ago
It's not a demo; it's a testcase.

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

Comment 19

6 years ago
(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.
(Assignee)

Comment 20

6 years ago
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)
(Assignee)

Comment 22

6 years ago
> 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.  :-/
(Assignee)

Comment 23

6 years ago
> You're regressing style! :)

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

Comment 24

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

I pushed this to gaia: https://github.com/andreasgal/gaia/commit/4295bc5ac4f16111f4540f7371d206b9e6214fbb
Attachment #578753 - Attachment is obsolete: true
Attachment #578753 - Flags: review?(21)
(Assignee)

Comment 25

6 years ago
Created attachment 579564 [details] [diff] [review]
Patch v2
Attachment #578752 - Attachment is obsolete: true
Attachment #579564 - Flags: review?(jones.chris.g)
(Assignee)

Comment 26

6 years ago
Created attachment 579600 [details] [diff] [review]
Patch v2 (right version)
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+
(Assignee)

Updated

6 years ago
Whiteboard: [needs dependency]
(Assignee)

Updated

6 years ago
Whiteboard: [needs dependency] → [needs landing]
(Assignee)

Comment 27

5 years ago
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.
(Assignee)

Comment 29

5 years ago
(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?
(Assignee)

Comment 31

5 years ago
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.)
(Assignee)

Comment 32

5 years ago
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...
(Assignee)

Updated

5 years ago
Blocks: 709468
Depends on: 696042
(Assignee)

Comment 33

5 years ago
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

Updated

5 years ago
Depends on: 711602
(Assignee)

Updated

5 years ago
No longer depends on: 711602
(Assignee)

Updated

5 years ago
Whiteboard: [needs landing]

Comment 34

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/3e861cc550d4
https://hg.mozilla.org/mozilla-central/rev/3e861cc550d4
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla12

Comment 36

5 years ago
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
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.