I keep accidentally triggering it by setting the phone down on the couch. I feel like I need to shake the phone really hard (almost as if I'm angry) before it is triggered.

This is something I already noticed too, and reported to James. We should just adjust the shaking detection.

And here I thought it was too hard to trigger! The relevant constant is EXCITEMENT_THRESHOLD: https://mxr.mozilla.org/mozilla-central/source/b2g/components/LogShake.jsm#56 Smoothing the input acceleration with a low-pass filter may also be useful.

Just started to write an app, https://github.com/lissyx/shakeit This will allow us to: - record acceleration data as csv file for analysis of movements - experiment with new approach to detect shaking Current, only the recorder module is implemented. My plan is to start gathering some data first.

Resulting analysis of throwing the device on my desk

Resulting analysis of shaking my device

This is great Alex! How do the units from the graph (ie -25 - 25) translate to the units from the code (ie. THRESHOLD = 500)?

Values are just those from the devicemotionevent, as documented here: https://developer.mozilla.org/en-US/docs/Web/API/DeviceMotionEvent.accelerationIncludingGravity So x, y and z are m.s^-2, the charts are just with those values. And the threshold in gecko is just the result of |acc.x * acc.x + acc.y * acc.y + acc.z * acc.z|

I added a low pass filter which requires the phone to be in movement for ~10 "ticks" before registering the shake event. Very official testing involving dropping my phone on the ground, throwing it at a couch, and silly-walking while holding it has verified that the filter functions flawlessly.

(In reply to James Hobin from comment #8) > Created attachment 8613813 [details] [diff] [review] > bug1101994-logshake.patch > > I added a low pass filter which requires the phone to be in movement for ~10 > "ticks" before registering the shake event. Very official testing involving > dropping my phone on the ground, throwing it at a couch, and silly-walking > while holding it has verified that the filter functions flawlessly. Nice, can you provide implementation and results on my test app ?

Comment on attachment 8613813 [details] [diff] [review] bug1101994-logshake.patch Review of attachment 8613813 [details] [diff] [review]: ----------------------------------------------------------------- ::: b2g/components/LogShake.jsm @@ +203,5 @@ > var acc = event.accelerationIncludingGravity; > > + var newExcitement = acc.x * acc.x + acc.y * acc.y + acc.z * acc.z; > + // Low pass filter based on EXCITEMENT_FILTER_ALPHA > + this.excitement += (newExcitement - this.excitement) * EXCITEMENT_FILTER_ALPHA; Looks OK, but would be good to give proper documentation on how this will work for better understanding of further readers :)

Comment on attachment 8613813 [details] [diff] [review] bug1101994-logshake.patch I played around with this for a few minutes, and it seems to be much improved. I'd also like to see a button combination, since sometimes a shake is not always desirable. But that is orthogonal to this bug.

This is a graph comparing filtered and unfiltered excitement values. The y-axis is the excitement value, from x*x + y*y + z*z. The x-axis is time in milliseconds. 0-5000 is the phone at rest, 5000-10000 is a shake, 10000 onwards is gently shaking the device and dropping it from a height of one foot. The low pass filter successfully removes all of the peaks in the non-shaking region. Should I add a comment to the code explaining how the low-pass filter works?

Michael, Dale landed such kind of key combo. It's VolUp+VolDown, as documented in bug 1155411.

(In reply to James Hobin from comment #12) > Created attachment 8614259 [details] > comparison.png > > This is a graph comparing filtered and unfiltered excitement values. The > y-axis is the excitement value, from x*x + y*y + z*z. The x-axis is time in > milliseconds. 0-5000 is the phone at rest, 5000-10000 is a shake, 10000 > onwards is gently shaking the device and dropping it from a height of one > foot. The low pass filter successfully removes all of the peaks in the > non-shaking region. Nice, but this plot would be much better with legends and a title :) > > Should I add a comment to the code explaining how the low-pass filter works? I think it's still valuable.

I updated the comment to talk more about the rationale behind the filter and to mention this bug.

Comment on attachment 8614283 [details] [diff] [review] bug1101994-logshake.patch Review of attachment 8614283 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, we now just lack some tests :)

I added a test for ignoring a spike of motion due to dropping and refactored all of the other logshake tests to use an actual shake-type motion. This improves upon the previous approach of using a devicemotion event with impossibly high magnitude to trigger log capture. I also found a small bug in the LogCapture tests where it is too tightly coupled to a kernel property and filed bug 1171577 to track that fix.

Comment on attachment 8615454 [details] [diff] [review] Patch including unit tests and some test refactoring Review of attachment 8615454 [details] [diff] [review]: ----------------------------------------------------------------- Good work, and maybe we can even improve those tests! ::: b2g/components/test/unit/test_logshake.js @@ +192,5 @@ > + LogShake.init(); > + > + let readLocations = mockReadLogFile(); > + > + // Device motion events occur once ever 160 ms on average This test looks very good, I'd just like to know some rationale about it: - where does those value come from - can we avoid magic constants everywhere? - can we make sure we cover many common usecases? For instance, one case I can think of is: - user has his device in his pocket; biking on a non good road with lots of movements - user is running/doing is footing - user is running in the subway in paris and takes stairs very quickly because he's aleady late in the office :)

I tried to remove or explain all of the magic numbers. It's slightly difficult because most of them are determined by guesswork or by looking at the graphs. I had my weekly exercise testing on the stairs and will be posting data from that soon.

I intended this to be as dramatic a motion as possible and it still doesn't trigger LogShake Low pass filters are magical

Comment on attachment 8620715 [details] [diff] [review] bug1101994-logshake.patch Review of attachment 8620715 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me :) ::: b2g/components/LogShake.jsm @@ +210,5 @@ > > var acc = event.accelerationIncludingGravity; > > + // Updates excitement by a factor of at most alpha, ignoring sudden device > + // motion. See bug #1101994 for more information. Nit: how much sudden ?

(In reply to Alexandre LISSY :gerard-majax from comment #23) > Comment on attachment 8620715 [details] [diff] [review] > bug1101994-logshake.patch > > Review of attachment 8620715 [details] [diff] [review]: > ----------------------------------------------------------------- > > Looks good to me :) > > ::: b2g/components/LogShake.jsm > @@ +210,5 @@ > > > > var acc = event.accelerationIncludingGravity; > > > > + // Updates excitement by a factor of at most alpha, ignoring sudden device > > + // motion. See bug #1101994 for more information. > > Nit: how much sudden ? It is slightly hard to provide accurate values. The maximum ignored spike is 5 times the force of gravity, which Wikipedia approximates as the force applied by a dragster or the maximum expected acceleration during a luge run at the Whistler Sliding Centre.

Try build here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8df4474ff727 Clearing checkin-needed until the build is done (forgot Autolander wasn't a thing)

Had to rerun to unblock intermittent failure, looks good now

Can we close this now?

Sure looks like it. https://hg.mozilla.org/mozilla-central/rev/139a0155fd44