807136 - B2G Wifi: Only enable background scanning when it's needed

Status: RESOLVED FIXED

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

Description

[Blake Kaplan (:mrbkap) (inactive)]

The patch for bug 796640 always enables background scanning when we're disconnected. However, on phones where wpa_supplicant's sched_scan plays nicely with the driver, this is redundant. We should either avoid enabling background scanning as a build-time option or only enable it when the supplicant gets stuck (e.g. if we're stuck in the SCANNING state for more than 3 or so seconds).

Opinions are welcome.

I don't know if this has to block basecamp, given that this fix won't affect either Otoro or Unagi, since we know for a fact that they don't place nicely with sched_scan.

Comment 1

[Andrew Overholt [:overholt]]

Do we know if production devices will play nicely with sched_scan?  If we don't fix this, are we at risk of draining the battery more quickly?

Comment 2

[Andrew Overholt [:overholt]]

Blake, can you take this?

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

It appears that the patch for bug 807148 will fix this.

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

The patch for bug 807148 will make this better, but there's an even better fix for this bug: we can get rid of the timer added by that bug in favor of an android property (wifi.broken_schedule_scan?) and use that to determine whether or not to turn on background scanning. Chuck, would you be interested in trying to write the patch? We'd need some help on the b2g side to set the property on the proper devices, but it would be much cleaner than the current code that just landed on inbound. I think the biggest challenge is going to be to make sure that the driver isn't busy when we turn on background scanning.

Comment 5

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Hi Blake,
  For my understanding, you mean add a hard-coded property, say |wifi.broken_schedule_scan|, in gonk to indicate if supplicant can perform schedule scan properly. Then gecko reads this property to decide to enable the timer or not.
  Like what RIL does with property |ro.moz.ril.emergency_by_default|, is that right ?

  Could it be an issue on how to maintain the property value? For example, supplicant/driver is updated, and sadly from broken_schedule_scan = false to true, but gonk team forgot to test and update the value.

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to Chuck Lee [:chucklee] from comment #5)
>   Like what RIL does with property |ro.moz.ril.emergency_by_default|, is
> that right ?

Yep.

>   Could it be an issue on how to maintain the property value? For example,
> supplicant/driver is updated, and sadly from broken_schedule_scan = false to
> true, but gonk team forgot to test and update the value.

That's possible, I suppose. I'm not terribly worried about it happening, though. mwu: would we be notified if the driver people fix a big bug like that?

Comment 7

[Michael Wu [:mwu]]

(In reply to Blake Kaplan (:mrbkap) from comment #6)
> >   Could it be an issue on how to maintain the property value? For example,
> > supplicant/driver is updated, and sadly from broken_schedule_scan = false to
> > true, but gonk team forgot to test and update the value.
> 
> That's possible, I suppose. I'm not terribly worried about it happening,
> though. mwu: would we be notified if the driver people fix a big bug like
> that?

Probably, though I would think it's unlikely to get fixed unless we look at it ourselves or bug some vendor about it.

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

I'm not going to work on this in the short term. Chuck, if you have time, feel free to grab this.

Comment 9

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Attachment: Enable schedule scan recovery based on device property

Read predefined property "ro.moz.wifi.sched_scan_recover" in device profile, PRODUCT_PROPERTY_OVERRIDES, to determine if enable recovery timer or not.
The timer is default to enable, unless set "ro.moz.wifi.sched_scan_recover=false", so not need to change current profile.

Comment 10

[Michael Wu [:mwu]]

Comment on attachment 681377 [details] [diff] [review]
Enable schedule scan recovery based on device property

The general approach looks ok though I don't think taking systemlibs.js out of lazy load is the way we want to do it. ALL CAPS variable names should also be reserved for actual constants. mrbkap can review those js issues better though.

Comment 11

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Attachment: Enable schedule scan recovery based on device property, v1.1

Change variable for reading property to const.

Comment 12

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Attachment: Enable schedule scan recovery based on device property, v1.1

Change variable for reading property to constant.

Comment 13

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 681831 [details] [diff] [review]
Enable schedule scan recovery based on device property, v1.1

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

::: dom/wifi/WifiWorker.js
@@ -40,5 @@
>  // command always succeeds and we do a string/boolean check for the
>  // expected results).
>  var WifiManager = (function() {
>    function getSdkVersion() {
> -    Cu.import("resource://gre/modules/systemlibs.js");

I'd rather add the pref-getting to this function (and rename it to getStartupPrefs) than split this across the two places. The last time we did this, we used destructuring assignment, so we could do: { sdkVersion: parseInt(...), schedScanRecover: ... } and then

let { sdkVersion, schedScanRecover } = getStartupPrefs();

or whatever names you want.

Comment 14

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Attachment: Enable schedule scan recovery based on device property, v2

Change method of reading and providing system property as commented.

Comment 15

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

try server : https://tbpl.mozilla.org/?tree=Try&rev=3411f930a757

Comment 16

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Attachment: Enable schedule scan recovery based on device property, v2, after review

Final patch after review+.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c5150d0647f9

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c5150d0647f9