Closed
Bug 815070
Opened 12 years ago
Closed 12 years ago
[Wifi Tethering] Wait driver ready before turning on Wifi tethering
Categories
(Firefox OS Graveyard :: General, defect, P2)
Tracking
(blocking-basecamp:+, firefox18 fixed, firefox19 fixed)
People
(Reporter: vchang, Assigned: vchang)
References
Details
Attachments
(1 file, 3 obsolete files)
4.96 KB,
patch
|
mrbkap
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Updated•12 years ago
|
QA Contact: vchang
Assignee | ||
Updated•12 years ago
|
Assignee: nobody → vchang
QA Contact: vchang
Assignee | ||
Comment 1•12 years ago
|
||
Delay two seconds before issues Wifi Tethering commands.
Attachment #685074 -
Flags: review?(mrbkap)
Assignee | ||
Updated•12 years ago
|
blocking-basecamp: --- → ?
Updated•12 years ago
|
blocking-basecamp: ? → +
Can we do something smarter than waiting 2 seconds? Are there any callbacks/events we can wait for?
Comment 3•12 years ago
|
||
(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•12 years ago
|
||
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)
Comment 5•12 years ago
|
||
How about reading the delay time from property (ro.moz.wifi.*) defined per device?
Assignee | ||
Comment 6•12 years ago
|
||
> > + // 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.
Assignee | ||
Comment 7•12 years ago
|
||
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)
Updated•12 years ago
|
Priority: -- → P2
Comment 8•12 years ago
|
||
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)
Assignee | ||
Comment 9•12 years ago
|
||
(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.
Assignee | ||
Comment 10•12 years ago
|
||
Updated the patch to address comment 8
Attachment #688689 -
Flags: review?(mrbkap)
Assignee | ||
Comment 11•12 years ago
|
||
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 12•12 years ago
|
||
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+
Assignee | ||
Comment 13•12 years ago
|
||
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 14•12 years ago
|
||
Comment on attachment 689060 [details] [diff] [review]
Patch v1.3
Looks good. Thanks.
Attachment #689060 -
Flags: review?(mrbkap) → review+
Assignee | ||
Updated•12 years ago
|
Keywords: checkin-needed
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 :).
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Updated•12 years ago
|
status-firefox18:
--- → fixed
status-firefox19:
--- → fixed
Target Milestone: --- → B2G C2 (20nov-10dec)
Comment 17•12 years ago
|
||
Keywords: checkin-needed
You need to log in
before you can comment on or make changes to this bug.
Description
•