1084156 - [WIFI] While screen on WiFi do not enter power save mode when there is no traffic

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Wifi, defect)

Description

[Kai-Zhen Li [:seinlin][:kli]]

When there is no traffic, WiFi should enter power save mode even the screen is on. This can decrease the power consumption.

Comment 1

[shawn ku [:sku]]

Woodduck AOSP v.s. FxOS: 35.56 mA v.s. 59.41 mA for wifi case.

Comment 2

[Kai-Zhen Li [:seinlin][:kli]]

Attachment: wifi-power-save.diff

Hi Chuck, could you have a look to this patch? With this patch, wifi always operates in power save mode and power save mode will only be disabled during the period of requesting IP address.

BTW, I notice that wifi will be turned off when screen lock and there is no option/pref to disable this feature.

Comment 3

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

Comment on attachment 8509203 [details] [diff] [review]
wifi-power-save.diff

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

If we implement the always-power-save policy in gaia, then we have also protect DHCP interval from power save mode in apps which can change wifi status, like settings app[1] and quick setting[2], or find a place in system app the handle them once for all(maybe in status bar? [3])
But I think we should not let apps to do the work, most app developers doesn't know that we might lost DHCP packet in power save mode.
I think it's better to implement the policy in gecko[4].

[1] https://github.com/mozilla-b2g/gaia/blob/master/apps/settings/js/modules/wifi_context.js#L139
[2] https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/quick_settings.js#L294
[3] https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/statusbar.js#L1286
[4] http://dxr.mozilla.org/mozilla-central/source/dom/wifi/WifiWorker.js#2085

Comment 4

[Vincent Chang[:vchang][changyihsin]]

Comment on attachment 8509203 [details] [diff] [review]
wifi-power-save.diff

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

::: apps/ftu/js/wifi.js
@@ +144,3 @@
>          if (event.status === 'connected') {
> +          // Turn on wifi power saving after wifi is fully connected.
> +          WifiManager.setPowerSavingMode(true);

Per Chuck's comments, let's handle this in gecko directly.

::: apps/system/js/wifi.js
@@ +226,5 @@
>        // turn wifi back on.
>        lock.set({ 'wifi.enabled': true });
>        lock.set({ 'wifi.disabled_by_wakelock': false });
>        window.addEventListener('wifi-enabled', function() {
> +        wifiManager.setPowerSavingMode(true);

Can we have a pref and let partner decided power save mode is on or off?

Comment 5

[Keven Kuo [:kkuo]]

Hi! Kai-Zhen,

Over to you since you are working on it.

--
Keven

Comment 6

[Ethan Tseng [:ethan]]

We had a discussion of how to exploit Power Save Mode or even aggressively turn off Wi-Fi in order to 
optimize power consumption.
The design is in a draft phase. You can refer to the diagram:
https://wiki.mozilla.org/images/c/c6/WifiPowerModeState.svg

Comment 7

[Henry Chang [:hchang]]

Hi Micheal,

According to a earlier mail thread "Bug 1093951 - Wifi multicast filter", you mentioned: 

"We just need “DRIVER SETSUSPENDMODE” IOCTL to driver. Once this IOCTL is received, driver will configure all offloads to firmware."

However, based on the power consumption measurement on Flame done by our device team,
the packet filter seemed to not effective in power saving mode. We are not
sure what made such difference. Flame uses different chipset from what
we were talking about. Could you help confirm if this driver behavior is identical
across all your drivers or only certain ones?

Thanks!

Comment 8

[Michael Vines [:m1] [:evilmachines]]

Yes behaviour is the same.  Maybe the power test wasn't quite conducted properly.

Comment 9

[Henry Chang [:hchang]]

(In reply to Michael Vines [:m1] [:evilmachines] from comment #8)
> Yes behaviour is the same.  Maybe the power test wasn't quite conducted
> properly.

Thanks! We will try to test it again properly.

Comment 10

[Henry Chang [:hchang]]

Hi Danny,

1) Connect to wpa_cli
   $ wpa_cli -i wlan0 -p /data/misc/wifi/sockets

While connecting to wpa_cli, 

You can use

DRIVER RXFILTER-STOP
DRIVER RXFILTER-REMOVE 1
DRIVER RXFILTER-REMOVE 2
DRIVER RXFILTER-REMOVE 3
DRIVER RXFILTER-START

to enable multicast/broadcast filter 

or use

'DRIVER SETSUSPENDMODE 1' to enter power saving mode, which should override
the previous RXFILTER configurations.

Please refer to [1] for more information about this. I also learn
from it :p

Thanks!

[1] http://androidxref.com/4.4.4_r1/xref/frameworks/base/wifi/java/android/net/wifi/WifiNative.java#337

Comment 11

[Danny Liang [:dliang]]

(In reply to Henry Chang [:henry] from comment #10)
> Hi Danny,
> 
> 1) Connect to wpa_cli
>    $ wpa_cli -i wlan0 -p /data/misc/wifi/sockets
> 
> While connecting to wpa_cli, 
> 
> You can use
> 
> DRIVER RXFILTER-STOP
> DRIVER RXFILTER-REMOVE 1
> DRIVER RXFILTER-REMOVE 2
> DRIVER RXFILTER-REMOVE 3
> DRIVER RXFILTER-START
> 
> to enable multicast/broadcast filter 
> 
Above commands don't work! The power consumption is still high due to a lot of wake up events by wifi.

> or use
> 
> 'DRIVER SETSUSPENDMODE 1' to enter power saving mode, which should override
> the previous RXFILTER configurations.
> 
This command works well, and the flight mode standby current with wifi connect is 8.96mA.
Receiving unicast packet every 1 second by ping, the flight mode standby current with wifi connect is up to 46.94mA.

Comment 12

[Danny Liang [:dliang]]

Attachment: wifi_suspend_mode.png

The left part of picture is the waveform of receiving unicast packet under suspend mode (SETSUSPENDMODE=1); the right part of picture is the waveform under suspend mode (SETSUSPENDMODE=1)

Comment 13

[Kai-Zhen Li [:seinlin][:kli]]

Amy, can you take this bug? You can discuss with Vincent Chang, Henry and also me :)

Comment 14

[Amy Chung [:Amy]]

ok

Comment 15

[Amy Chung [:Amy]]

Attachment: Current flow of wifi power mode

Comment 16

[Josh Cheng [:josh]]

Dear Amy,
Could you provide an ETA date? Thanks!

Comment 17

[Amy Chung [:Amy]]

Attachment: 0001-Bug-1084156-Turn-to-power-saving-mode-when-wifi-conn.patch

Comment 18

[Amy Chung [:Amy]]

Attachment: wifi init added power saving mode and allow off flag.

Comment 19

[Henry Chang [:hchang]]

Comment on attachment 8537019 [details] [diff] [review]
0001-Bug-1084156-Turn-to-power-saving-mode-when-wifi-conn.patch

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

Generally looks good! Thanks :) Besides, we currently only restore the power saving mode
whenever success of finishing DHCP request. We should restore power saving mode on failure
as well.

::: dom/wifi/WifiWorker.js
@@ +638,4 @@
>        }
>        netUtil.runDhcp(manager.ifname, function(data) {
>          dhcpInfo = data.info;
> +

Please remove this line

@@ +2156,4 @@
>            WifiManager.loopDetectionCount = 0;
>            self._startConnectionInfoTimer();
>            self._fireEvent("onassociate", { network: netToDOM(self.currentNetwork) });
> +        

Trailing spaces

@@ +2807,5 @@
>          this.setStaticIpMode(msg);
>          break;
> +      case "WifiManager:getPowerSavingMode":
> +        this.getPowerSavingMode(msg);
> +        break;

This should be no longer needed.

@@ +3419,5 @@
> +      });
> +    });
> +  },
> +*/
> +

Please remove the commented lines

Comment 20

[Amy Chung [:Amy]]

Attachment: 0001-Bug-1084156-WIFI-Turn-on-the-power-saving-mode-when-.patch

Henry, I added setting power saving mode wthen wifi init, enable and connection failed in WifiWorker. Could you review this patch?

Comment 21

[Amy Chung [:Amy]]

Attachment: 0001-Bug-1084156-Turn-to-power-saving-mode-when-wifi-conn.patch

Hi Henry,
I have combine two git commitments and remove some blank space.
Please help me to review this patch.

Thanks and Best Regards,
Amy Chung

Comment 22

[Henry Chang [:hchang]]

Comment on attachment 8540037 [details] [diff] [review]
0001-Bug-1084156-Turn-to-power-saving-mode-when-wifi-conn.patch

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

The biggest issue in this patch is it doesn't actually put wifi to power saving mode
after turned on. Will talk to you offline later!

::: dom/wifi/WifiWorker.js
@@ +2187,5 @@
>                this.prevState === "INTERFACE_DISABLED" ||
>                this.prevState === "INACTIVE" ||
>                this.prevState === "UNINITIALIZED")) {
> +          if(this.prevState === "INACTIVE")
> +          {

should be at the same line as |if|

@@ +2395,5 @@
>    // nsISettingsServiceCallback implementation.
>    var initWifiEnabledCb = {
>      handle: function handle(aName, aResult) {
> +      //Turn on power saving mode when wifi init
> +      self.setPowerSavingMode(true);

This is not the right timing to call |setPowerSavingMode| since
supplicant is not up and running yet.

@@ +2996,5 @@
>      const message = "WifiManager:setWifiEnabled:Return";
>      let self = this;
>      let enabled = msg.data;
> +    //Wifi enabled, turn on power saving mode
> +    self.setPowerSavingMode(true);

This is also not a good place to call setPowerSavingMode.

@@ +3002,5 @@
>      // No change.
>      if (enabled === WifiManager.enabled) {
>        this._sendMessage(message, true, true, msg);
>        return;
>      }

This line shouldn't be changed.

Comment 23

[Amy Chung [:Amy]]

Attachment: 0001-Bug-1084156-Turn-to-power-saving-mode-when-wifi-conn.patch

Dear Henry,
Please help me to confirm this patch,
thank you!

Amy Chung

Comment 24

[Amy Chung [:Amy]]

Attachment: 0001-Bug-1084156-Turn-to-power-saving-mode-when-wifi-conn.patch

Hi Henry,
Please help me to review this patch.
Thanks.

Amy

Comment 25

[Henry Chang [:hchang]]

Comment on attachment 8541121 [details] [diff] [review]
0001-Bug-1084156-Turn-to-power-saving-mode-when-wifi-conn.patch

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

Looks good to me. Thanks! One trivial thing you can do here is to
remove manager.setSuspendOptimizations and manager.setPowerMode 
and use manager.setPowerSavingMode in WifiWorker.setPowerSavingMode
instead.

::: dom/wifi/WifiWorker.js
@@ +862,4 @@
>      return true;
>    }
>  
> +  function setWifiPowerSavingMode(bool) {

I personally don't feel comfortable about the arg name "bool"
since it looks like a reserved word. (even though it isn't :p).
I'll leave you to decide to change the name or not.

Comment 26

[Amy Chung [:Amy]]

Attachment: 0001-Bug-1084156-Turn-to-power-saving-mode-when-wifi-conn.patch

fix nit, carry r+.

Comment 27

[Amy Chung [:Amy]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bc9195dc49c1

Comment 28

[Amy Chung [:Amy]]

Attachment: 0001-Bug-1084156-Turn-to-power-saving-mode-when-wifi-conn.patch

fix nit, carry r+

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/738e16a7341b

I didn't merge the pull request because it was lacking an r+ AFAICT.

Comment 30

[Ryan VanderMeulen [:RyanVM]]

Leaving open for the Gaia patch. Please resolve the bug if it isn't needed at this point.
https://hg.mozilla.org/mozilla-central/rev/738e16a7341b

Comment 31

[Kai-Zhen Li [:seinlin][:kli]]

Comment on attachment 8537028 [details] [review]
wifi init added power saving mode and allow off flag.

Bug 1114453 will take care gaia.

Comment 32

[Kai-Zhen Li [:seinlin][:kli]]

Ryan, Thanks!