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

Status: RESOLVED FIXED

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

Description

[Vincent Chang[:vchang][changyihsin]]

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.

Comment 1

[Vincent Chang[:vchang][changyihsin]]

Attachment: Patch v1.0

Delay two seconds before issues Wifi Tethering commands.

Comment 2

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Can we do something smarter than waiting 2 seconds? Are there any callbacks/events we can wait for?

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

(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 4

[Blake Kaplan (:mrbkap) (inactive)]

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?

Comment 5

[Shian-Yow Wu [:swu]]

How about reading the delay time from property (ro.moz.wifi.*) defined per device?

Comment 6

[Vincent Chang[:vchang][changyihsin]]

> > +              // 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.

Comment 7

[Vincent Chang[:vchang][changyihsin]]

Attachment: Patch v1.1

1. Refactor the patch. 
2. cancel the timer to prevent running timeout callback unexpectedly.

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

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?

Comment 9

[Vincent Chang[:vchang][changyihsin]]

(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.

Comment 10

[Vincent Chang[:vchang][changyihsin]]

Attachment: Patch v1.2

Updated the patch to address comment 8

Comment 11

[Vincent Chang[:vchang][changyihsin]]

Comment on attachment 686483 [details] [diff] [review]
Patch v1.1

># HG changeset patch
># User Vincent Chang <[e-mail]>
># 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");

Comment 12

[Blake Kaplan (:mrbkap) (inactive)]

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.

Comment 13

[Vincent Chang[:vchang][changyihsin]]

Attachment: Patch v1.3

To be safe, ask for review again. Updated the patch to address comment 12

Comment 14

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 689060 [details] [diff] [review]
Patch v1.3

Looks good. Thanks.

Comment 15

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2451057af100
https://hg.mozilla.org/releases/mozilla-aurora/rev/9b2d24abeebe
https://hg.mozilla.org/releases/mozilla-beta/rev/a4aad866f079

Vincent, we need to get you commit access :).

Comment 17

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/2451057af100