Closed Bug 815070 Opened 8 years ago Closed 8 years ago

[Wifi Tethering] Wait driver ready before turning on Wifi tethering

Categories

(Firefox OS Graveyard :: General, defect, P2)

ARM
Gonk (Firefox OS)
defect

Tracking

(blocking-basecamp:+, firefox18 fixed, firefox19 fixed)

RESOLVED FIXED
B2G C2 (20nov-10dec)
blocking-basecamp +
Tracking Status
firefox18 --- fixed
firefox19 --- fixed

People

(Reporter: vchang, Assigned: vchang)

References

Details

Attachments

(1 file, 3 obsolete files)

In otoro and unagi platform, the Wifi driver takes longer time to get stable. We need to delay 2 second to start wpa_supplicant. The Wifi tethering also has this problem. We have to wait for Wifi driver ready before turning on Wifi Tethering.
QA Contact: vchang
Assignee: nobody → vchang
QA Contact: vchang
Attached patch Patch v1.0 (obsolete) — Splinter Review
Delay two seconds before issues Wifi Tethering commands.
Attachment #685074 - Flags: review?(mrbkap)
blocking-basecamp: --- → ?
blocking-basecamp: ? → +
Can we do something smarter than waiting 2 seconds? Are there any callbacks/events we can wait for?
(In reply to Jonas Sicking (:sicking) from comment #2)
> Can we do something smarter than waiting 2 seconds? Are there any
> callbacks/events we can wait for?

Unfortunately not. On some android devices, the driver uses a bootloader that waits for the proper amount of time; on Otoro and Unagi, however, there is no bootloader and no indication for when the network interface is fully up and running.
Comment on attachment 685074 [details] [diff] [review]
Patch v1.0

Review of attachment 685074 [details] [diff] [review]:
-----------------------------------------------------------------

I'm OK with the patch, but have a couple of comments to address before we land it.

::: dom/wifi/WifiWorker.js
@@ +1110,5 @@
> +            manager.state = "WIFITETHERING";
> +            gNetworkManager.setWifiTethering(enabled, WifiNetworkInterface, function(result) {
> +              // Pop out current request.
> +              callback(enabled);
> +              // Should we fire a dom event if we fail to set wifi tethering  ?

I wifi sets the pref back to "off" in this case. We should probably do that.

@@ +1118,5 @@
> +          // Driver startup on certain platforms takes longer than it takes
> +          // for us to return from loadDriver, so wait 2 seconds before
> +          // turning on Wifi tethering.
> +          timer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer);
> +          timer.init(doSetWifiTethering, 2000, Ci.nsITimer.TYPE_ONE_SHOT);

Can you extract the 2000 into a constant that we share between the two timers? Or, even better, can we factor the two loadDriver dances into a single function?
Attachment #685074 - Flags: review?(mrbkap)
How about reading the delay time from property (ro.moz.wifi.*) defined per device?
> > +              // Pop out current request.
> > +              callback(enabled);
> > +              // Should we fire a dom event if we fail to set wifi tethering  ?
> 
> I wifi sets the pref back to "off" in this case. We should probably do that.

We have done it in NetworkManager.js.  You can find it in below link. 
http://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/NetworkManager.js#646

Just not sure if it is good enough for user experience.  

> 
> @@ +1118,5 @@
> > +          // Driver startup on certain platforms takes longer than it takes
> > +          // for us to return from loadDriver, so wait 2 seconds before
> > +          // turning on Wifi tethering.
> > +          timer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer);
> > +          timer.init(doSetWifiTethering, 2000, Ci.nsITimer.TYPE_ONE_SHOT);
> 
> Can you extract the 2000 into a constant that we share between the two
> timers? Or, even better, can we factor the two loadDriver dances into a
> single function?

Sure, I will refactor and post a new patch soon.
Attached patch Patch v1.1 (obsolete) — Splinter Review
1. Refactor the patch. 
2. cancel the timer to prevent running timeout callback unexpectedly.
Attachment #685074 - Attachment is obsolete: true
Attachment #686483 - Flags: review?(mrbkap)
Priority: -- → P2
Comment on attachment 686483 [details] [diff] [review]
Patch v1.1

Review of attachment 686483 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/wifi/WifiWorker.js
@@ +1006,5 @@
>    manager.loopDetectionCount = 0;
>  
> +  const DRIVER_READY_WAIT = 2000;
> +  manager.waitDriverReadyTimer = {timer: null, onCancel: null, param: null};
> +  manager.cancelWaitDriverReadyTimer = function (needCancel) {

Why is this necessary? We have a bunch of machinery to ensure that we never have two requests to turn on or off wifi/tethering active at the same time, right?
Attachment #686483 - Flags: review?(mrbkap)
(In reply to Blake Kaplan (:mrbkap) from comment #8)
> Comment on attachment 686483 [details] [diff] [review]
> Patch v1.1
> 
> Review of attachment 686483 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/wifi/WifiWorker.js
> @@ +1006,5 @@
> >    manager.loopDetectionCount = 0;
> >  
> > +  const DRIVER_READY_WAIT = 2000;
> > +  manager.waitDriverReadyTimer = {timer: null, onCancel: null, param: null};
> > +  manager.cancelWaitDriverReadyTimer = function (needCancel) {
> 
> Why is this necessary? We have a bunch of machinery to ensure that we never
> have two requests to turn on or off wifi/tethering active at the same time,
> right?

I just worry about if we may handle the next request before the timer which is set by previous request is expired. We have to 100% sure that the callback in each request will be called. Because we depend on the callback to help us to pop up the next request.
Attached patch Patch v1.2 (obsolete) — Splinter Review
Updated the patch to address comment 8
Attachment #688689 - Flags: review?(mrbkap)
Comment on attachment 686483 [details] [diff] [review]
Patch v1.1

># HG changeset patch
># User Vincent Chang <vchang@mozilla.com>
># Date 1353923804 -28800
># Node ID 69fced762f542c148439691141a489800fb4b45f
># Parent  3c67034ba39c687b8da87d6ca26c86e3b0c3f3ac
>Bug 815070 - [Wifi Tethering] Wait driver ready before turning on Wifi tethering. r=mrbkap
>
>diff --git a/dom/wifi/WifiWorker.js b/dom/wifi/WifiWorker.js
>--- a/dom/wifi/WifiWorker.js
>+++ b/dom/wifi/WifiWorker.js
>@@ -1000,16 +1000,40 @@ var WifiManager = (function() {
>   // Initial state
>   manager.state = "UNINITIALIZED";
>   manager.enabled = false;
>   manager.supplicantStarted = false;
>   manager.connectionInfo = { ssid: null, bssid: null, id: -1 };
>   manager.authenticationFailuresCount = 0;
>   manager.loopDetectionCount = 0;
> 
>+  const DRIVER_READY_WAIT = 2000;
>+  manager.waitDriverReadyTimer = {timer: null, onCancel: null, param: null};
>+  manager.cancelWaitDriverReadyTimer = function (needCancel) {
>+    if (manager.waitDriverReadyTimer.timer) {
>+      manager.waitDriverReadyTimer.timer.cancel();
>+      manager.waitDriverReadyTimer.timer = null;
>+    }
>+    if (manager.waitDriverReadyTimer.onCancel) {
>+      if (needCancel) {
>+        manager.waitDriverReadyTimer.onCancel.call(this, manager.waitDriverReadyTimer.param);
>+      }
>+      manager.waitDriverReadyTimer.onCancel = null;
>+    }
>+  };
>+  manager.createWaitDriverReadyTimer = function (onTimeout, onCancel, param) {
>+    manager.cancelWaitDriverReadyTimer(true);
>+    manager.waitDriverReadyTimer.timer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer);
>+    manager.waitDriverReadyTimer.timer.initWithCallback(onTimeout,
>+                                                DRIVER_READY_WAIT,
>+                                                Ci.nsITimer.TYPE_ONE_SHOT);
>+    manager.waitDriverReadyTimer.onCancel = onCancel;
>+    manager.waitDriverReadyTimer.param = param;
>+  };
>+
>   // Public interface of the wifi service
>   manager.setWifiEnabled = function(enable, callback) {
>     if (enable === manager.enabled) {
>       callback("no change");
>       return;
>     }
> 
>     if (enable) {
>@@ -1040,19 +1064,18 @@ var WifiManager = (function() {
> 
>         prepareForStartup(function() {
>           loadDriver(function (status) {
>             if (status < 0) {
>               callback(status);
>               return;
>             }
> 
>-            let timer;
>             function doStartSupplicant() {
>-              timer = null;
>+              manager.cancelWaitDriverReadyTimer(false);
>               startSupplicant(function (status) {
>                 if (status < 0) {
>                   unloadDriver(function() {
>                     callback(status);
>                   });
>                   return;
>                 }
> 
>@@ -1061,22 +1084,22 @@ var WifiManager = (function() {
>                   callback(ok ? 0 : -1);
>                 });
>               });
>             }
> 
>             // Driver startup on certain platforms takes longer than it takes for us
>             // to return from loadDriver, so wait 2 seconds before starting
>             // the supplicant to give it a chance to start.
>-            timer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer);
>-            timer.init(doStartSupplicant, 2000, Ci.nsITimer.TYPE_ONE_SHOT);
>-          });
>+            manager.createWaitDriverReadyTimer(doStartSupplicant, callback, 0);
>+         });
>         });
>       });
>     } else {
>+      manager.cancelWaitDriverReadyTimer(true);
>       // Note these following calls ignore errors. If we fail to kill the
>       // supplicant gracefully, then we need to continue telling it to die
>       // until it does.
>       terminateSupplicant(function (ok) {
>         manager.connectionDropped(function () {
>           stopSupplicant(function (status) {
>             closeSupplicantConnection(function () {
>               manager.state = "UNINITIALIZED";
>@@ -1099,28 +1122,37 @@ var WifiManager = (function() {
>           return;
>         }
>         manager.ifname = ifname;
>         loadDriver(function (status) {
>           if (status < 0) {
>             callback(enabled);
>             return;
>           }
>-          WifiNetworkInterface.name = manager.ifname;
>-          manager.state = "WIFITETHERING";
>-          // Turning on wifi tethering.
>-          gNetworkManager.setWifiTethering(enabled, WifiNetworkInterface, function(result) {
>-            // Pop out current request.
>-            callback(enabled);
>-            // Should we fire a dom event if we fail to set wifi tethering  ?
>-            debug("Enable Wifi tethering result: " + (result ? result : "successfully"));
>-          });
>+
>+          function doStartWifiTethering() {
>+            manager.cancelWaitDriverReadyTimer(false);
>+            WifiNetworkInterface.name = manager.ifname;
>+            manager.state = "WIFITETHERING";
>+            gNetworkManager.setWifiTethering(enabled, WifiNetworkInterface, function(result) {
>+              // Pop out current request.
>+              callback(enabled);
>+              // Should we fire a dom event if we fail to set wifi tethering  ?
>+              debug("Enable Wifi tethering result: " + (result ? result : "successfully"));
>+            });
>+          }
>+
>+          // Driver startup on certain platforms takes longer than it takes
>+          // for us to return from loadDriver, so wait 2 seconds before
>+          // turning on Wifi tethering.
>+          manager.createWaitDriverReadyTimer(doStartWifiTethering, callback, enabled);
>         });
>       });
>     } else {
>+      manager.cancelWaitDriverReadyTimer(true);
>       manager.state = "UNINITIALIZED";
>       gNetworkManager.setWifiTethering(enabled, WifiNetworkInterface, function(result) {
>         // Should we fire a dom event if we fail to set wifi tethering  ?
>         debug("Disable Wifi tethering result: " + (result ? result : "successfully"));
>         // Unload wifi driver even if we fail to control wifi tethering.
>         unloadDriver(function(status) {
>           if (status < 0) {
>             debug("Fail to unload wifi driver");
Attachment #686483 - Attachment is obsolete: true
Comment on attachment 688689 [details] [diff] [review]
Patch v1.2

Review of attachment 688689 [details] [diff] [review]:
-----------------------------------------------------------------

One more non-nit comment here. I'm going to mark this r+ since it should be a pretty mechanical change.

::: dom/wifi/WifiWorker.js
@@ +1005,5 @@
>    manager.authenticationFailuresCount = 0;
>    manager.loopDetectionCount = 0;
>  
> +  const DRIVER_READY_WAIT = 2000;
> +  manager.waitDriverReadyTimer = null;

Last comment: we don't need to stick this on the manager because it's not needed outside of it, so these variables and functions can be declared simply with "var" and "function". If we need to use this API outside of the manager at some point, we can stick it on the manager then.
Attachment #688689 - Flags: review?(mrbkap) → review+
Attached patch Patch v1.3Splinter Review
To be safe, ask for review again. Updated the patch to address comment 12
Attachment #688689 - Attachment is obsolete: true
Attachment #689060 - Flags: review?(mrbkap)
Comment on attachment 689060 [details] [diff] [review]
Patch v1.3

Looks good. Thanks.
Attachment #689060 - Flags: review?(mrbkap) → review+
Target Milestone: --- → B2G C2 (20nov-10dec)
You need to log in before you can comment on or make changes to this bug.