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

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

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

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

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

Bug 753245 comment 6 provides some justification for this workaround.

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

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

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

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.

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

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

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

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

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.

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

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

(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 :)

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

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

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.

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

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

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

Addressing nits, changing isSupportedDevice to deviceNeedsWorkaround.

(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

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

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

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

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

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

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

Pointer to Github pull-request

Comment on attachment 788504 [details] Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/11330 Adding felash@gmail.com as reviewer for this . I am OK with this code.

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

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 on attachment 788504 [details] Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/11330 \O/

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.

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.

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

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

Love you all!!!!!