871916 - Remove Nexus S dependent code in system app

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::System, defect)

Description

[(inactive after 6/18) Alive Kuo [:alive]]

+++ This bug was initially created as a clone of Bug #753245 +++

See https://bugzilla.mozilla.org/show_bug.cgi?id=753245#c7, c8, c9, c10.

We shouldn't place any hardware dependent code in our code base.
And furthermore, the workaround for Nexus S seems failed now. 
We should remove it from Window Manager and Bootstrap.

Comment 1

[:gerard-majax]

(In reply to Alive Kuo [:alive] from comment #0)
> +++ This bug was initially created as a clone of Bug #753245 +++
> 
> See https://bugzilla.mozilla.org/show_bug.cgi?id=753245#c7, c8, c9, c10.
> 
> We shouldn't place any hardware dependent code in our code base.
> And furthermore, the workaround for Nexus S seems failed now. 
> We should remove it from Window Manager and Bootstrap.

It's still working, I've recently made some change on my local system to have it at a better place and this change improves battery life on my Nexus S. See the attached patch.

Comment 2

[:gerard-majax]

Attachment: Better battery-compliant Nexus S workaround

Comment 3

[Tapas[:tkundu on #b2g/gaia/memshrink/gfx] (always NI me)]

(In reply to Alexandre LISSY :gerard-majax from comment #2)
> Created attachment 757413 [details] [diff] [review]
> Better battery-compliant Nexus S workaround

Why do we need workaround for Nexus S device? Device motion sensor should be turned on only when app needs it . Otherwise, it wastes CPU/DSP power and performance.

In other words, I don't want to see gaia to listen for device motion sensor during screen turn off/on or during startup.

It should listen for device motion sensor only and only when any specific app asks it to do same.

Comment 4

[Tapas[:tkundu on #b2g/gaia/memshrink/gfx] (always NI me)]

Could you please change your code to reflect my comments or explain me why it is needed .

Thanks

Comment 5

[:gerard-majax]

(In reply to Tapas Kumar Kundu from comment #4)
> Could you please change your code to reflect my comments or explain me why
> it is needed .
> 
> Thanks

No, I can't explain why it's needed. All I can tell you is that without this hack, then the sensor just don't work (i.e., it keeps returning a fixed value) when requested by applications.

Comment 6

[Panos Astithas (he/him) [:past] (please ni?)]

Bug 753245 comment 6 provides some justification for this workaround.

Comment 7

[:gerard-majax]

(In reply to Panos Astithas [:past] from comment #6)
> Bug 753245 comment 6 provides some justification for this workaround.

Waou, thanks for this one !

Comment 8

[Tapas[:tkundu on #b2g/gaia/memshrink/gfx] (always NI me)]

(In reply to Panos Astithas [:past] from comment #6)
> Bug 753245 comment 6 provides some justification for this workaround.

Shouldn't we put this code in some hardware specific library ?

If we keep this code here in gaia then it will cause cpu usage and drain battery for all vendors. 

Please suggest.

Comment 9

[Tapas[:tkundu on #b2g/gaia/memshrink/gfx] (always NI me)]

m1:

Can you please suggest how to do it here properly as it is effecting all devices.

Comment 10

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

Perhaps we can add a build time preference (system property?), disabled by default of course, that is only enabled in the Nexus S build.  Might want to do this in Gecko rather than Gaia though.  Alternatively in gecko determine that you're running on an Nexus S, by reading "ro.product.device", and activate this hack.

Comment 11

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Tapas Kumar Kundu from comment #8)
> (In reply to Panos Astithas [:past] from comment #6)
> > Bug 753245 comment 6 provides some justification for this workaround.
> 
> Shouldn't we put this code in some hardware specific library ?
> 
> If we keep this code here in gaia then it will cause cpu usage and drain
> battery for all vendors. 
> 
> Please suggest.

I'm not really the right person to ask here. I'm sure Alive who filed this bug has thought about it more than I have.

Comment 12

[(inactive after 6/18) Alive Kuo [:alive]]

This discussion is more than my expect :)
I totally agree :m1,
all this kind of code should be pluggable. And this code should live in gecko.

I'll appreciate if anyone wants to help the gecko part before I remove this code from gaia,
otherwise I will do it on my own in a small progress..

Comment 13

[(inactive after 6/18) Alive Kuo [:alive]]

(In reply to Alive Kuo [:alive] from comment #12)
> otherwise I will do it on my own in a small progress..

*slow* I mean.

Comment 14

[:gerard-majax]

(In reply to Alive Kuo [:alive] from comment #12)
> This discussion is more than my expect :)
> I totally agree :m1,
> all this kind of code should be pluggable. And this code should live in
> gecko.
> 
> I'll appreciate if anyone wants to help the gecko part before I remove this
> code from gaia,
> otherwise I will do it on my own in a small progress..

I might be able to work on this next week. Yet I'm not sure this is limited to only Nexus S, per comment #6.

Comment 15

[:gerard-majax]

Another thing we have to think of when moving this hack to gecko is that we want to avoid holding a listener when the device must sleep, i.e., when the screen is off (which was the goal of the change proposed) : if we don't, then we will drain battery because the system won't be able to get into suspend.

Comment 16

[Tapas[:tkundu on #b2g/gaia/memshrink/gfx] (always NI me)]

(In reply to Alexandre LISSY :gerard-majax from comment #15)
> Another thing we have to think of when moving this hack to gecko is that we
> want to avoid holding a listener when the device must sleep, i.e., when the
> screen is off (which was the goal of the change proposed) : if we don't,
> then we will drain battery because the system won't be able to get into
> suspend.

I also agree on this. Currently, we are doing it only for light sensor. We need to do it for other sensor also.

Comment 17

[Tapas[:tkundu on #b2g/gaia/memshrink/gfx] (always NI me)]

Hi Alexandre,

I can help you in gecko part . Please let me know if you need any help.

Comment 18

[:gerard-majax]

(In reply to Tapas Kumar Kundu from comment #17)
> Hi Alexandre,
> 
> I can help you in gecko part . Please let me know if you need any help.

No it's okay, I just needed to find some time. I'm currently hacking in gecko 18 for this :)

Comment 19

[:gerard-majax]

I might need an uplift of bug 879695 for the 'screen-state-changed' feature.

Comment 20

[:gerard-majax]

(In reply to Alexandre LISSY :gerard-majax from comment #19)
> I might need an uplift of bug 879695 for the 'screen-state-changed' feature.

Of course this is if we want this on b2g18 also (I'm using it and it's working fine).

Comment 21

[:gerard-majax]

Attachment: Better battery-compliant sensors event listener on Gecko side

Please find attached the Gecko version of this Gaia workaround. As far as I could test, this works fine and allows us to remove the Gaia version.

Comment 22

[(inactive after 6/18) Alive Kuo [:alive]]

gaia patch is standby.
https://github.com/mozilla-b2g/gaia/pull/11330

Comment 23

[Tapas[:tkundu on #b2g/gaia/memshrink/gfx] (always NI me)]

Could you please enable this hack in gecko only for Nexus Device ? It should not be enabled for all devices as suggested by michael vines in https://bugzilla.mozilla.org/show_bug.cgi?id=871916#c10

Comment 24

[Tapas[:tkundu on #b2g/gaia/memshrink/gfx] (always NI me)]

Comment on attachment 785375 [details] [diff] [review]
Better battery-compliant sensors event listener on Gecko side

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

Could you please enable this hack in gecko only for Nexus Device ? It should not be enabled for all devices as suggested by michael vines in https://bugzilla.mozilla.org/show_bug.cgi?id=871916#c10

Comment 25

[:gerard-majax]

(In reply to Tapas Kumar Kundu from comment #24)
> Comment on attachment 785375 [details] [diff] [review]
> Better battery-compliant sensors event listener on Gecko side
> 
> Review of attachment 785375 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Could you please enable this hack in gecko only for Nexus Device ? It should
> not be enabled for all devices as suggested by michael vines in
> https://bugzilla.mozilla.org/show_bug.cgi?id=871916#c10

I don't understand your point, this is exactly what I'm doing in this patch: enabling only for crespo.

Comment 26

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

Comment on attachment 785375 [details] [diff] [review]
Better battery-compliant sensors event listener on Gecko side

nit: if this was my patch, I'd change isSupportedDevice() to something like deviceNeedsWorkaround(). "Is supported" to me implies that we want devices to exhibit this behavior this, but really we don't.

Comment 27

[:gerard-majax]

Attachment: Better battery-compliant sensors event listener on Gecko side, v2

Addressing nits, changing isSupportedDevice to deviceNeedsWorkaround.

Comment 28

[Tapas[:tkundu on #b2g/gaia/memshrink/gfx] (always NI me)]

(In reply to Alexandre LISSY :gerard-majax from comment #25)
> (In reply to Tapas Kumar Kundu from comment #24)
> > Comment on attachment 785375 [details] [diff] [review]
> > Better battery-compliant sensors event listener on Gecko side
> > 
> > Review of attachment 785375 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Could you please enable this hack in gecko only for Nexus Device ? It should
> > not be enabled for all devices as suggested by michael vines in
> > https://bugzilla.mozilla.org/show_bug.cgi?id=871916#c10
> 
> I don't understand your point, this is exactly what I'm doing in this patch:
> enabling only for crespo.

Sorry, I didn't go through your code. I will build it and try it today .  I will update my feedback here. Thanks a lot

Comment 29

[Tapas[:tkundu on #b2g/gaia/memshrink/gfx] (always NI me)]

(In reply to Alexandre LISSY :gerard-majax from comment #27)
> Created attachment 785904 [details] [diff] [review]
> Better battery-compliant sensors event listener on Gecko side, v2
> 
> Addressing nits, changing isSupportedDevice to deviceNeedsWorkaround.

Can you please post Gaia patch which will remove previous hack. Thanks a lot for your work.

Comment 30

[Tapas[:tkundu on #b2g/gaia/memshrink/gfx] (always NI me)]

(In reply to Tapas Kumar Kundu from comment #29)
> (In reply to Alexandre LISSY :gerard-majax from comment #27)
> > Created attachment 785904 [details] [diff] [review]
> > Better battery-compliant sensors event listener on Gecko side, v2
> > 
> > Addressing nits, changing isSupportedDevice to deviceNeedsWorkaround.
> 
> Can you please post Gaia patch which will remove previous hack. Thanks a lot
> for your work.

I saw your code in Gecko. But Gaia still has nexus dependent code. Can you please post a patch which will remove that code from Gaia.

Comment 31

[Tapas[:tkundu on #b2g/gaia/memshrink/gfx] (always NI me)]

Comment on attachment 785904 [details] [diff] [review]
Better battery-compliant sensors event listener on Gecko side, v2

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

I don't have access to nexus device. So, I cannot test your gecko code with nexus device. 

I tested it with latest gaia and gecko on a different device (not nexus). I disabled Nexus dependent code from gaia (bootstrap.js and window_manager.js) 
Now, it does not poll sensor always. It polls only when it is requested by app (like browser or other app). That is what we are expecting it to do.

So, I am fine with this Gecko patch. But please post a patch which disables  gaia nexus dependent code. I will give positive feedback once I see Gaia patch from you 
I also recommend you to add some other reviewer who can test it on nexus device. 

Thanks a lot for your work.

Comment 32

[:gerard-majax]

Alive already posted the gaia counterpart as part of comment #22.

Comment 33

[Tapas[:tkundu on #b2g/gaia/memshrink/gfx] (always NI me)]

(In reply to Alexandre LISSY :gerard-majax from comment #32)
> Alive already posted the gaia counterpart as part of comment #22.

I saw it . That patch simply moves gaia code from bootstrap.js and window_manager.js to screen_manager.js. This does not fulfill real objective to enable this hack only in Nexus device. 

Please delete nexus hardware dependent code from gaia completely.  This is required. I am OK with your gecko patch as it tries to enable hack only in nexus device.

Comment 34

[Tapas[:tkundu on #b2g/gaia/memshrink/gfx] (always NI me)]

Hi (In reply to Tapas Kumar Kundu from comment #33)
> (In reply to Alexandre LISSY :gerard-majax from comment #32)
> > Alive already posted the gaia counterpart as part of comment #22.
> 
> I saw it . That patch simply moves gaia code from bootstrap.js and
> window_manager.js to screen_manager.js. This does not fulfill real objective
> to enable this hack only in Nexus device. 
> 
> Please delete nexus hardware dependent code from gaia completely.  This is
> required. I am OK with your gecko patch as it tries to enable hack only in
> nexus device.

Sorry , I made a mistake . I looked into Gaia patch posted in comment #2 .  I am ok with Gaia patch posted in comment #22.

Thanks for your work. Please look into FFOS module owner (https://wiki.mozilla.org/Modules/FirefoxOS) and add reviewer for both gaia and gecko. 

Please also attach gaia patch as attachment. I am OK with your gaia (https://bugzilla.mozilla.org/show_bug.cgi?id=871916#c222) and gecko patch (https://bugzilla.mozilla.org/attachment.cgi?id=785904) .

Comment 35

[Tapas[:tkundu on #b2g/gaia/memshrink/gfx] (always NI me)]

(In reply to Tapas Kumar Kundu from comment #34)
> Hi (In reply to Tapas Kumar Kundu from comment #33)
> > (In reply to Alexandre LISSY :gerard-majax from comment #32)
> > > Alive already posted the gaia counterpart as part of comment #22.
> > 
> > I saw it . That patch simply moves gaia code from bootstrap.js and
> > window_manager.js to screen_manager.js. This does not fulfill real objective
> > to enable this hack only in Nexus device. 
> > 
> > Please delete nexus hardware dependent code from gaia completely.  This is
> > required. I am OK with your gecko patch as it tries to enable hack only in
> > nexus device.
> 
> Sorry , I made a mistake . I looked into Gaia patch posted in comment #2 . 
> I am ok with Gaia patch posted in comment #22.
> 
> Thanks for your work. Please look into FFOS module owner
> (https://wiki.mozilla.org/Modules/FirefoxOS) and add reviewer for both gaia
> and gecko. 
> 
> Please also attach gaia patch as attachment. I am OK with your gaia
> (https://bugzilla.mozilla.org/show_bug.cgi?id=871916#c222) and gecko patch
> (https://bugzilla.mozilla.org/attachment.cgi?id=785904) .

I am OK with your gaia (https://bugzilla.mozilla.org/show_bug.cgi?id=871916#c22) and gecko patch (https://bugzilla.mozilla.org/attachment.cgi?id=785904)

Comment 36

[Tapas[:tkundu on #b2g/gaia/memshrink/gfx] (always NI me)]

Comment on attachment 785904 [details] [diff] [review]
Better battery-compliant sensors event listener on Gecko side, v2

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

It looks good to me. This patch also needs following gaia patch:

https://bugzilla.mozilla.org/show_bug.cgi?id=871916#c22

Comment 37

[:gerard-majax]

Attachment: Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/11330

Pointer to Github pull-request

Comment 38

[Tapas[:tkundu on #b2g/gaia/memshrink/gfx] (always NI me)]

Comment on attachment 788504 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/11330

Adding [e-mail] as reviewer for this . I am OK with this code.

Comment 39

[Tapas[:tkundu on #b2g/gaia/memshrink/gfx] (always NI me)]

Comment on attachment 785904 [details] [diff] [review]
Better battery-compliant sensors event listener on Gecko side, v2

Changing reviewer. I am fine with this patch. But I am unable to test it on Nexus device. 

@mchen,

Could you please review it.

Comment 40

[Julien Wajsberg [:julienw]]

Comment on attachment 785904 [details] [diff] [review]
Better battery-compliant sensors event listener on Gecko side, v2

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

::: b2g/chrome/content/shell.js
@@ +1205,5 @@
> +      // On boot, enable the listener, screen will be on.
> +      window.addEventListener('devicemotion', this);
> +
> +      // Then listen for further screen state changes
> +      Services.obs.addObserver(this, 'screen-state-changed', false);

maybe I miss something, but I don't see where you handle this observer. Notably, if I read correctly the old Gaia  code, you're supposed to turn the devicemotion on for 2 seconds when the screen is turned on, right ?

By the way I don't see were you turn of the devicemotion event handler after 2 seconds at init either ?

Comment 41

[Julien Wajsberg [:julienw]]

Comment on attachment 788504 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/11330

Tim is better suited than me to review this.

Comment 42

[Julien Wajsberg [:julienw]]

I don't exactly get why this needs to be in gecko and why this can't be done in a deeper layer, but well...

Comment 43

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 788504 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/11330

\O/

Comment 44

[Marco Chen [:mchen]]

Comment on attachment 785904 [details] [diff] [review]
Better battery-compliant sensors event listener on Gecko side, v2

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

The new patch here will
  action 1. During screen is on, devicemotion will be enabled always.
  action 2. During screen is off, to disable the devicemotion event.

The old patch in Gaia now will
  action 5. To enable devicemotion event then disable it after 2 seconds.

The old workaround from http://dl.dropboxusercontent.com/u/8727858/physical-events/index.html will
  action 3. There are one devicemotion and deviceorientation enabled always.
  action 4. There are another one devicemotion and deviceorientation enabled temporarily within 2 seconds.

Q1: If we don't know what the combination from this old workaround is needed to avoid the sensor HAL/Driver issue, this gecko patch can't be reviewed properly.
ex: The old patch in Gaia now is Action 4 only. The new patch here is like Action 3 only.
    Or we need both action 3 + action 4.

Note: As I know that some orientation sensor is a virtual sensor which is combined by calculation of Accelerometer and Magnetic sensor.
So maybe to enable the accelerometer sensor accompany with orientation is needed.

Comment 45

[Tapas[:tkundu on #b2g/gaia/memshrink/gfx] (always NI me)]

Hi Marco,

Currently, Nexus sensor hack is degrading power usage in all device. 

Goal of this work is to enable sensor hack only in Nexus device and also to remove all Nexus dependent code from gaia. This will save power in all devices except Nexus. 


I think the best way to see whether this gecko patch is still applicable is to test it in nexus device. 

I tested on other platform and it is working as expected in comment#3 ( https://bugzilla.mozilla.org/show_bug.cgi?id=871916#c3)

Can you please test it on Nexus device. I don't have access to Nexus device.

Comment 46

[(inactive after 6/18) Alive Kuo [:alive]]

(In reply to Tapas Kumar Kundu from comment #45)
> Goal of this work is to enable sensor hack only in Nexus device and also to
> remove all Nexus dependent code from gaia. This will save power in all
> devices except Nexus. 

True, and the bug reporter would be happy :)

I'm fine to leave it in gaia if gaia knows current device needs this hack or not, but obviously gaia doesn't know.

Comment 47

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

Comment on attachment 785904 [details] [diff] [review]
Better battery-compliant sensors event listener on Gecko side, v2

Hey Fabrice, could you please r? or suggest somebody who can.  :gerard-majax is off and would be good to get this landed.  Ta!

Comment 48

[[:fabrice] Fabrice Desré]

Comment on attachment 785904 [details] [diff] [review]
Better battery-compliant sensors event listener on Gecko side, v2

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

r=me but can we get something to test on a nexus S before we land? I can do a build tomorrow if no one else jumps in.

::: b2g/chrome/content/shell.js
@@ +1168,5 @@
>  #endif
> +
> +#ifdef MOZ_WIDGET_GONK
> +let SensorsListener = {
> +  sensorsListenerDevices: [ 'crespo' ],

nit: no need for spaces around 'crespo'

@@ +1205,5 @@
> +      // On boot, enable the listener, screen will be on.
> +      window.addEventListener('devicemotion', this);
> +
> +      // Then listen for further screen state changes
> +      Services.obs.addObserver(this, 'screen-state-changed', false);

Julien, the observer is handled in the observe() method.

Comment 49

[Tapas[:tkundu on #b2g/gaia/memshrink/gfx] (always NI me)]

Attachment: Better battery-compliant sensors event listener on Gecko side, v2

Comment 50

[Tapas[:tkundu on #b2g/gaia/memshrink/gfx] (always NI me)]

Comment on attachment 796875 [details] [diff] [review]
Better battery-compliant sensors event listener on Gecko side, v2

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

Carrying r+ from fabrice #comments 48 (https://bugzilla.mozilla.org/show_bug.cgi?id=871916#c48)

Comment 51

[[:fabrice] Fabrice Desré]

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

Comment 52

[Ed Morley [:emorley]]

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

Comment 53

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

Gaia part of this bug:
https://github.com/mozilla-b2g/gaia/commit/8f6feb0b7f5f6848a05e89fc688041d2e49e2150

Comment 54

[(inactive after 6/18) Alive Kuo [:alive]]

Love you all!!!!!