Bug 1101994 - Add a low-pass filter to LogShake's shake detection and refactor unit tests to be less redundant. r=mhenretty, r=gerard-majax
authorJames Hobin <hobinjk@gmail.com>
Wed, 10 Jun 2015 17:22:00 -0400
changeset 279686 139a0155fd44b7abe7527afa7fbd207e7b0a652c
parent 279685 9d4200cadeaf0cd4c65808cdd68b11ae534086d5
child 279687 2072c88502d2ee3e86e0e639ae526f23436a83f6
push id4932
push userjlund@mozilla.com
push dateMon, 10 Aug 2015 18:23:06 +0000
treeherdermozilla-beta@6dd5a4f5f745 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmhenretty, gerard-majax
bugs1101994
milestone41.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1101994 - Add a low-pass filter to LogShake's shake detection and refactor unit tests to be less redundant. r=mhenretty, r=gerard-majax
b2g/components/LogShake.jsm
b2g/components/test/unit/test_logshake.js
--- a/b2g/components/LogShake.jsm
+++ b/b2g/components/LogShake.jsm
@@ -46,19 +46,25 @@ XPCOMUtils.defineLazyServiceGetter(this,
 this.EXPORTED_SYMBOLS = ["LogShake"];
 
 function debug(msg) {
   dump("LogShake.jsm: "+msg+"\n");
 }
 
 /**
  * An empirically determined amount of acceleration corresponding to a
- * shake
+ * shake.
  */
 const EXCITEMENT_THRESHOLD = 500;
+/**
+ * The maximum fraction to update the excitement value per frame. This
+ * corresponds to requiring shaking for approximately 10 motion events (1.6
+ * seconds)
+ */
+const EXCITEMENT_FILTER_ALPHA = 0.2;
 const DEVICE_MOTION_EVENT = "devicemotion";
 const SCREEN_CHANGE_EVENT = "screenchange";
 const CAPTURE_LOGS_CONTENT_EVENT = "requestSystemLogs";
 const CAPTURE_LOGS_START_EVENT = "capture-logs-start";
 const CAPTURE_LOGS_ERROR_EVENT = "capture-logs-error";
 const CAPTURE_LOGS_SUCCESS_EVENT = "capture-logs-success";
 
 let LogShake = {
@@ -84,16 +90,21 @@ let LogShake = {
 
   /**
    * If a capture has been requested and is waiting for reads/parsing. Used for
    * debouncing.
    */
   captureRequested: false,
 
   /**
+   * The current excitement (movement) level
+   */
+  excitement: 0,
+
+  /**
    * Map of files which have log-type information to their parsers
    */
   LOGS_WITH_PARSERS: {
     "/dev/log/main": LogParser.prettyPrintLogArray,
     "/dev/log/system": LogParser.prettyPrintLogArray,
     "/dev/log/radio": LogParser.prettyPrintLogArray,
     "/dev/log/events": LogParser.prettyPrintLogArray,
     "/proc/cmdline": LogParser.prettyPrintArray,
@@ -117,16 +128,19 @@ let LogShake = {
     // }});
 
     // However, the screen is always on when we are being enabled because it is
     // either due to the phone starting up or a user enabling us directly.
     this.handleScreenChangeEvent({ detail: {
       screenEnabled: true
     }});
 
+    // Reset excitement to clear residual motion
+    this.excitement = 0;
+
     SystemAppProxy.addEventListener(CAPTURE_LOGS_CONTENT_EVENT, this, false);
     SystemAppProxy.addEventListener(SCREEN_CHANGE_EVENT, this, false);
 
     Services.obs.addObserver(this, "xpcom-shutdown", false);
   },
 
   /**
    * Handle an arbitrary event, passing it along to the proper function
@@ -191,19 +205,22 @@ let LogShake = {
     // There is a lag between disabling the event listener and event arrival
     // ceasing.
     if (!this.deviceMotionEnabled) {
       return;
     }
 
     var acc = event.accelerationIncludingGravity;
 
-    var excitement = acc.x * acc.x + acc.y * acc.y + acc.z * acc.z;
+    // Updates excitement by a factor of at most alpha, ignoring sudden device
+    // motion. See bug #1101994 for more information.
+    var newExcitement = acc.x * acc.x + acc.y * acc.y + acc.z * acc.z;
+    this.excitement += (newExcitement - this.excitement) * EXCITEMENT_FILTER_ALPHA;
 
-    if (excitement > EXCITEMENT_THRESHOLD) {
+    if (this.excitement > EXCITEMENT_THRESHOLD) {
       this.startCapture();
     }
   },
 
   startCapture: function() {
     if (this.captureRequested) {
       return;
     }
--- a/b2g/components/test/unit/test_logshake.js
+++ b/b2g/components/test/unit/test_logshake.js
@@ -10,125 +10,137 @@
 /* jshint -W097 */
 "use strict";
 
 const Cu = Components.utils;
 
 Cu.import("resource://gre/modules/LogCapture.jsm");
 Cu.import("resource://gre/modules/LogShake.jsm");
 
-// Force logshake to handle a device motion event with given components
-// Does not use SystemAppProxy because event needs special
-// accelerationIncludingGravity property
+const EVENTS_PER_SECOND = 6.25;
+const GRAVITY = 9.8;
+
+/**
+ * Force logshake to handle a device motion event with given components.
+ * Does not use SystemAppProxy because event needs special
+ * accelerationIncludingGravity property.
+ */
 function sendDeviceMotionEvent(x, y, z) {
   let event = {
     type: "devicemotion",
     accelerationIncludingGravity: {
       x: x,
       y: y,
       z: z
     }
   };
   LogShake.handleEvent(event);
 }
 
-// Send a screen change event directly, does not use SystemAppProxy due to race
-// conditions.
+/**
+ * Send a screen change event directly, does not use SystemAppProxy due to race
+ * conditions.
+ */
 function sendScreenChangeEvent(screenEnabled) {
   let event = {
     type: "screenchange",
     detail: {
       screenEnabled: screenEnabled
     }
   };
   LogShake.handleEvent(event);
 }
 
-function debug(msg) {
-  var timestamp = Date.now();
-  dump("LogShake: " + timestamp + ": " + msg);
+/**
+ * Mock the readLogFile function of LogCapture.
+ * Used to detect whether LogShake activates.
+ * @return {Array<String>} Locations that LogShake tries to read
+ */
+function mockReadLogFile() {
+  let readLocations = [];
+
+  LogCapture.readLogFile = function(loc) {
+    readLocations.push(loc);
+    return null; // we don't want to provide invalid data to a parser
+  };
+
+  // Allow inspection of readLocations by caller
+  return readLocations;
+}
+
+/**
+ * Send a series of events that corresponds to a shake
+ */
+function sendSustainedShake() {
+  // Fire a series of devicemotion events that are of shake magnitude
+  for (let i = 0; i < 2 * EVENTS_PER_SECOND; i++) {
+    sendDeviceMotionEvent(0, 2 * GRAVITY, 2 * GRAVITY);
+  }
+
 }
 
 add_test(function test_do_log_capture_after_shaking() {
   // Enable LogShake
   LogShake.init();
 
-  let readLocations = [];
-  LogCapture.readLogFile = function(loc) {
-    readLocations.push(loc);
-    return null; // we don't want to provide invalid data to a parser
-  };
+  let readLocations = mockReadLogFile();
 
-  // Fire a devicemotion event that is of shake magnitude
-  sendDeviceMotionEvent(9001, 9001, 9001);
+  sendSustainedShake();
 
   ok(readLocations.length > 0,
       "LogShake should attempt to read at least one log");
 
   LogShake.uninit();
   run_next_test();
 });
 
 add_test(function test_do_nothing_when_resting() {
   // Enable LogShake
   LogShake.init();
 
-  let readLocations = [];
-  LogCapture.readLogFile = function(loc) {
-    readLocations.push(loc);
-    return null; // we don't want to provide invalid data to a parser
-  };
+  let readLocations = mockReadLogFile();
 
-  // Fire a devicemotion event that is relatively tiny
-  sendDeviceMotionEvent(0, 9.8, 9.8);
+  // Fire several devicemotion events that are relatively tiny
+  for (let i = 0; i < 2 * EVENTS_PER_SECOND; i++) {
+    sendDeviceMotionEvent(0, GRAVITY, GRAVITY);
+  }
 
   ok(readLocations.length === 0,
       "LogShake should not read any logs");
 
-  debug("test_do_nothing_when_resting: stop");
   LogShake.uninit();
   run_next_test();
 });
 
 add_test(function test_do_nothing_when_disabled() {
-  debug("test_do_nothing_when_disabled: start");
   // Disable LogShake
   LogShake.uninit();
 
-  let readLocations = [];
-  LogCapture.readLogFile = function(loc) {
-    readLocations.push(loc);
-    return null; // we don't want to provide invalid data to a parser
-  };
+  let readLocations = mockReadLogFile();
 
-  // Fire a devicemotion event that would normally be a shake
-  sendDeviceMotionEvent(0, 9001, 9001);
+  // Fire a series of events that would normally be a shake
+  sendSustainedShake();
 
   ok(readLocations.length === 0,
       "LogShake should not read any logs");
 
   run_next_test();
 });
 
 add_test(function test_do_nothing_when_screen_off() {
   // Enable LogShake
   LogShake.init();
 
-
   // Send an event as if the screen has been turned off
   sendScreenChangeEvent(false);
 
-  let readLocations = [];
-  LogCapture.readLogFile = function(loc) {
-    readLocations.push(loc);
-    return null; // we don't want to provide invalid data to a parser
-  };
+  let readLocations = mockReadLogFile();
 
-  // Fire a devicemotion event that would normally be a shake
-  sendDeviceMotionEvent(0, 9001, 9001);
+  // Fire a series of events that would normally be a shake
+  sendSustainedShake();
 
   ok(readLocations.length === 0,
       "LogShake should not read any logs");
 
   // Restore the screen
   sendScreenChangeEvent(true);
 
   LogShake.uninit();
@@ -140,18 +152,18 @@ add_test(function test_do_log_capture_re
   LogShake.init();
 
   let readLocations = [];
   LogCapture.readLogFile = function(loc) {
     readLocations.push(loc);
     throw new Error("Exception during readLogFile for: " + loc);
   };
 
-  // Fire a devicemotion event that is of shake magnitude
-  sendDeviceMotionEvent(9001, 9001, 9001);
+  // Fire a series of events that would normally be a shake
+  sendSustainedShake();
 
   ok(readLocations.length > 0,
       "LogShake should attempt to read at least one log");
 
   LogShake.uninit();
   run_next_test();
 });
 
@@ -163,22 +175,44 @@ add_test(function test_do_log_capture_re
   LogCapture.readLogFile = function(loc) {
     readLocations.push(loc);
     LogShake.LOGS_WITH_PARSERS[loc] = function() {
       throw new Error("Exception during LogParser for: " + loc);
     };
     return null;
   };
 
-  // Fire a devicemotion event that is of shake magnitude
-  sendDeviceMotionEvent(9001, 9001, 9001);
+  // Fire a series of events that would normally be a shake
+  sendSustainedShake();
 
   ok(readLocations.length > 0,
       "LogShake should attempt to read at least one log");
 
   LogShake.uninit();
   run_next_test();
 });
 
+add_test(function test_do_nothing_when_dropped() {
+  // Enable LogShake
+  LogShake.init();
+
+  let readLocations = mockReadLogFile();
+
+  // We want a series of spikes to be ignored by LogShake. This roughly
+  // corresponds to the compare_stairs_sock graph on bug #1101994
+
+  for (let i = 0; i < 10 * EVENTS_PER_SECOND; i++) {
+    // Fire a devicemotion event that is at rest
+    sendDeviceMotionEvent(0, 0, GRAVITY);
+    // Fire a spike of motion
+    sendDeviceMotionEvent(0, 2 * GRAVITY, 2 * GRAVITY);
+  }
+
+  ok(readLocations.length === 0,
+      "LogShake should not read any logs");
+
+  LogShake.uninit();
+  run_next_test();
+});
+
 function run_test() {
-  debug("Starting");
   run_next_test();
 }