852361 - [System] When battery level become two percents, device is shut down

Status: RESOLVED FIXED

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

Description

[Leo]

1. Title : When battery level become two percents, device is shut down
2. Precondition : Battery level become two percents
3. Tester's Action : 
4. Detailed Symptom : Device is shut down
5. Expected : When battery level become zero, device is shut down
6. Reproducibility: Y
1) Frequency Rate : 100%
7. Gaia revision : de3e5b9205e6cb1a6bd0858a98d159272ad96d11

Comment 1

[Leo]

Attachment: Pull Request URL 8726

Comment 2

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

Comment on attachment 727097 [details]
Pull Request URL 8726

No, we should leave some space.
Even 1 or 1.5 you could argue(with reasons) but IMO 0.00 is not acceptable.

Comment 3

[Leo]

Is it policy of ffos ? Why do you leave some space?

Comment 4

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

Please see https://bugzilla.mozilla.org/show_bug.cgi?id=825798, thanks.

Comment 5

[Marco Chen [:mchen]]

Hi,

As I knew that the 0% of battery capacity reported to OS (android or FFOS) is not a real 0% on battery. In driver layer or in FW of battery monitor, there already reserved some battery capacity to prevent battery go into dead mode so OS can just do shutdown on 0%. That is ok. So I would suggest to shutdown at 0%.

This will leave the turning of battery capacity report to partner.

And in bootloader, it will do shutdown by more accurate detection. Ex: battery voltage or real battery capacity from battery monitor.

Comment 6

[Alex Keybl [:akeybl]]

Adding qawanted to see if this equally impacts Unagis. None of our devices should shutdown before 0 or 1 percent is reached.

Comment 7

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

This one's going to be a bear to repro on-device, since you essentially have to stare at a phone while it ticks down and not miss a moment.

Can we get a dev opinion as to whether this shutdown behavior is at the driver level or the Gecko level? If it's the latter it should be device-independent and we can test against emulator much, much more easily.

Comment 8

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

Talked to Alex, he suggested I needinfo Andrew and Lucas. 

We're looking for info on where this behavior is located in the codebase. If it's in Gecko or Gaia, emulator should be sufficient (where battery level can be directly manipulated) and will make this much easier to regression test. Also means we won't have to repeat the tests on different devices.

Comment 9

[Andrew Overholt [:overholt]]

(In reply to Geo Mealer [:geo] from comment #8)
> Talked to Alex, he suggested I needinfo Andrew and Lucas. 
> 
> We're looking for info on where this behavior is located in the codebase. If
> it's in Gecko or Gaia, emulator should be sufficient (where battery level
> can be directly manipulated) and will make this much easier to regression
> test. Also means we won't have to repeat the tests on different devices.

:mwu or :dhylands would likely know the answer to this.

Comment 10

[Dave Hylands [:dhylands]]

It looks like it's here:
https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/battery_manager.js#L23

Comment 11

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

Awesome! I'll check this out on emulator first, then. We may not need to test on Unagi.

Comment 12

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

This reproduces perfectly on emulator (building from today's code).

In the telnet session:

POWER CAPACITY 4
POWER CAPACITY 3
POWER CAPACITY 2

...shutdown.

Given that the code's in Gaia per comment 10 and it repros here, I don't see any reason to do anything but final verification on a device. This should be device independent.

Interesting fact: we shut down at 2% even if the battery's currently charging. I don't think I'd bother filing this, since moving the threshold to reported 0% would get rid of any weirdness here, but was interesting.

Comment 13

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

er, if you repro using above, use lowercase on the telnet session. :)

Comment 14

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

It also turns out that even if you are at 2% battery, and try to turn on the device, it will automatically shut itself off.

Comment 15

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

If we've confirmed that we should keep drained the device to 0% battery, let's adopt leo's patch here...

Comment 16

[Gregor Wagner [:gwagner]]

(In reply to Alive Kuo [:alive] from comment #15)
> If we've confirmed that we should keep drained the device to 0% battery,
> let's adopt leo's patch here...

We should make sure that we have enough power for a standard shutdown. If 0% is not enough (as seen in bug 825798) we shouldn't change it to 0%. Is 1% a compromise?

Comment 17

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

0% or 1% is OK for me but I wonder we don't know how to find the 'safe' value accurately..
Marco do you have any clue?

Comment 18

[Marco Chen [:mchen]]

Hi all,

That is about the definition of 0% of battery capacity.

Case 1: 0% really means the empty of battery.

  Disadvantage: Normally battery has a cut-off voltage (ex: 2.8V), once battery voltage is lower then it, battery will go into protection mode (ex: 0V or called dead battery). And the big loading current will drop the battery voltage especially it is on low level. In case of battery has 3% but the outgoing call will make big loading current from battery then it is easy to cause battery go into protection mode. So I don't suggestion this case.

Case 2: 0% is not a really empty but there is a reserved capacity outside the calculation of battery level. (ex: Battery or Fuel Gauge driver reserved 200mAh when battery has totally 1400 mAh. So 0% means that we used 1200mAh only)

  Advantage: there is no disadvantage on case 1.

Conclusion:
  So I suggest that we use "Case 2" above and leave the determination of how many capacity should be reserved outside the 0% to ODM/OEM/Brand company.
  As I knew some algorithm also check the battery voltage level to decide shutdown timing when battery is 0%. (ex: shutdown when 0% and voltage is less then 3.2V) but this can leave to ODM to customize it.

Comment 19

[Lucas Adamski [:ladamski]]

My 2c... seems like this bug is about the visual indicator not the underlying battery state.  Seems like we could simply lie about the power remaining and show 0% while leaving the 2% in reserve for safe shutdown.  Just like cars do with their fuel gauges.

Comment 20

[Alex Keybl [:akeybl]]

Triage agrees with comment 19

Comment 21

[Aus Lacroix [:aus]]

I guess a good question here would be where we want this magic deviation to live. We could make gecko report the battery level as -2% of actual always as a safety or we can make the gaia system app fudge what it shows the user only. Preferences?

Comment 22

[Aus Lacroix [:aus]]

After looking at the current code I feel like it would be best for the underlying gecko bits to report the level as 2% lower than actual at all times. There are too many potential spots where we are checking the battery level using said API to go around tweaking all of them and any time anyone else wants to display the battery level they would need to remember to reserve 2% so that their display is consistent with what we show the user in the notification / status area.

Comment 23

[Lucas Adamski [:ladamski]]

I suspect if we simply did -2% we'd have another bug that the phone can't be charged to 100%.  We should figure out some sort of curve/fudge that can span 0-100%.

Comment 24

[Aus Lacroix [:aus]]

Yep. I was thinking that when the battery level falls under 50% we would report 1% less and when it falls under 25% we'd report 2% less. Does that seem reasonable?

Comment 25

[Lucas Adamski [:ladamski]]

Seems quite reasonable to me!

Comment 26

[Aus Lacroix [:aus]]

Attachment: Patch - v1 - Reserve 1% at 50% and another 1% at 25%

Comment 27

[Aus Lacroix [:aus]]

Attachment: Gaia Pull Request - v1 - Shutdown at 0% battery.

Comment 28

[Marco Chen [:mchen]]

I think the magically reserved 2% battery capacity is redundant here. Because if BSP layer didn't do what I suggested on case 2 of comment 18 then it will still happen the automatic shutdown by hitting the cut-off voltage according to big current loading from battery.

If BSP layer already reserved battery capacity then what we do there is redundant and impact the life of daily use.

Comment 29

[Lucas Adamski [:ladamski]]

If I understand c28 correctly and there is already an adequate reserve to at 0% battery to shut the device down safely, then I agree that the extra 2% is not necessary and the original LEO patch is fine.

Comment 30

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

Comment on attachment 766198 [details]
Gaia Pull Request - v1 - Shutdown at 0% battery.

I think this is enough.

Comment 31

[Gregor Wagner [:gwagner]]

Comment on attachment 727097 [details]
Pull Request URL 8726

I just talked to alive and he is ok with taking this patch.

Comment 32

[Aus Lacroix [:aus]]

OK. Sounds like the underlying driver will reserve power already. Are we guaranteed that that will be the case for _ALL_ devices we're running on? If not, maybe it would be worthwhile exposing an extra attribute in the HAL Battery Information to indicate whether the driver will reserve power for shutdown or not and then decide at the battery manager level whether to reserve power.

If we're guaranteed that everyone is pretty smart and reserves power for shutdown, hooray!

Comment 33

[Michael Wu [:mwu]]

I don't think it's our job to guess what percent left is appropriate for the device. That's a job for the underlying drivers, and it shouldn't be terribly difficult for device vendors to adjust the driver's battery reporting to account for reserve.

Comment 34

[Aus Lacroix [:aus]]

@Michael Wu - Sounds good to me! We'll stick with the simple patch that changes auto-shutdown when battery is at 0% and scrap the rest.

Comment 35

[Kevin Grandon :kgrandon]

Comment on attachment 766198 [details]
Gaia Pull Request - v1 - Shutdown at 0% battery.

Let's go with the original PR here, #8726.

Comment 36

[Kevin Grandon :kgrandon]

Gaia patch has landed in master: https://github.com/mozilla-b2g/gaia/commit/a7f1e921c67d5d3d4e0afaa06aca79320784e806

Comment 37

[Aus Lacroix [:aus]]

To clarify, the gecko patch is NOT required for this fix. PR that landed is all that's needed to resolve this.

Comment 38

[John Ford [:jhford] CET/CEST Berlin Time]

Uplifted a7f1e921c67d5d3d4e0afaa06aca79320784e806 to:
v1-train: 22e0768fbcec94d049b071bdbed967ed651cdd2e

Comment 39

[John Ford [:jhford] CET/CEST Berlin Time]

1.1hd: 22e0768fbcec94d049b071bdbed967ed651cdd2e

Comment 40

[Montoya Clemmons (MClemmons)]

Issue still reproduces on latest Leo Moz Ril build. 

Environmental Variables
Build ID: 20130807071207
Gecko: http://hg.mozilla.org/releases/mozilla-b2g18/rev/11bb1b0eefff
Gaia: 60ca81600a080dae33058b0692ecaa213556c926
Platform Version: 18.1
Firmware Version: D300f08o 

New bug 902633 created for this issue.

Comment 42

[Lukas Blakk [:lsblakk] use ?needinfo]

This is not fixed (according to the filing of 902633).  Instead of opening a new bug, let's reopen this blocking, assigned bug and if necessary back out the changes that landed in order to try again to actually fix the issue rather than build on top of potentially regressing code.

Comment 43

[Jason Smith [:jsmith]]

(In reply to [e-mail] [:lsblakk] from comment #42)
> This is not fixed (according to the filing of 902633).  Instead of opening a
> new bug, let's reopen this blocking, assigned bug and if necessary back out
> the changes that landed in order to try again to actually fix the issue
> rather than build on top of potentially regressing code.

John - Can you back out the committed patches on master, v1-train, and v1-train HD?

Comment 44

[John Ford [:jhford] CET/CEST Berlin Time]

Backouts:

master: dfff18a
v1-train: d6afd22

v1.1.0hd will merge the v1-train backout over when they next do their v1-train merge.

Comment 45

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

v1.1.0hd: d6afd220abe85fed88444d412baf98c707433b77

Comment 46

[Preeti Raghunath(:Preeti)]

John,

Please confirm if this is fixed on 1.1 as well (not 1.1hd)

Comment 47

[John Ford [:jhford] CET/CEST Berlin Time]

(In reply to Preeti Raghunath(:Preeti) from comment #46)
> John,
> 
> Please confirm if this is fixed on 1.1 as well (not 1.1hd)

the fix that landed was backed out, so I think we are in a good state right now.  'Fixed' right now means that the patch was backed out, and that's the current state.

Comment 48

[Jason Smith [:jsmith]]

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) from comment #45)
> v1.1.0hd: d6afd220abe85fed88444d412baf98c707433b77

For the sake of consistency, this should be reset to affected to indicate a backout.

Comment 49

[Jason Smith [:jsmith]]

This was backed out, so this isn't fixed. Reopening.

Comment 50

[Leo]

Attachment: CAM00560.mp4

Why backing out this patch? In leo device, I see when battery level become 2%, device is not shut down

But, when battery level becom 0%, device is shut down.

I think bug 902633 is updating time problem...(Even if battery level is 1% or 0%, device doesn't update battery level). So, Bug 092633 is other issue.

I attach status of leo device(this patch is applied)

Please check this.

Comment 51

[Leo]

(In reply to Leo from comment #50)
> Created attachment 800522 [details]
> CAM00560.mp4
> 
> Why backing out this patch? In leo device, I see when battery level become
> 2%, device is not shut down
> 
> But, when battery level becom 0%, device is shut down.
> 
> I think bug 902633 is updating time problem...(Even if battery level is 1%
> or 0%, device doesn't update battery level). So, Bug 092633 is other issue.
> 
> I attach status of leo device(this patch is applied)
> 
> Please check this.

Please see 10 seconds in video(Battery level 2% -> 1%), and 1:30 in video(Battery level 1% -> 0% and shut down)

Comment 52

[Jason Smith [:jsmith]]

Can you clarify more about what you mean about a time updating problem?

Comment 53

[Leo]

When battery level is changed, window.navigator.battery send "levelchange" event.
I think that this event is not sent to system app in specific time.
Becase, when I use camera for consuming battery, sometimes battery real level is 0%, but battery level is still shown to 2% in settings app.
Our other engineer said it seems that battery level updating time is not matched real battery level.(Have some error).

Comment 54

[Jason Smith [:jsmith]]

(In reply to Leo from comment #53)
> When battery level is changed, window.navigator.battery send "levelchange"
> event.
> I think that this event is not sent to system app in specific time.
> Becase, when I use camera for consuming battery, sometimes battery real
> level is 0%, but battery level is still shown to 2% in settings app.
> Our other engineer said it seems that battery level updating time is not
> matched real battery level.(Have some error).

Ah. So if I understand what you are saying - this patch ensures that the device shuts down when we hit 0%, not 2%, even though that might not be what the UI says due to this Battery API --> System App integration.

Okay - so let's do this - we can reland this as this fixes the key blocking issue. The other bug we'll track fixing the UI inconsistency.

Comment 55

[John Ford [:jhford] CET/CEST Berlin Time]

(In reply to Jason Smith [:jsmith] from comment #54)
> (In reply to Leo from comment #53)
> > When battery level is changed, window.navigator.battery send "levelchange"
> > event.
> > I think that this event is not sent to system app in specific time.
> > Becase, when I use camera for consuming battery, sometimes battery real
> > level is 0%, but battery level is still shown to 2% in settings app.
> > Our other engineer said it seems that battery level updating time is not
> > matched real battery level.(Have some error).
> 
> Ah. So if I understand what you are saying - this patch ensures that the
> device shuts down when we hit 0%, not 2%, even though that might not be what
> the UI says due to this Battery API --> System App integration.
> 
> Okay - so let's do this - we can reland this as this fixes the key blocking
> issue. The other bug we'll track fixing the UI inconsistency.

So, to clarify, I should reland the change in comment 36, 38 and 39 and later backed out in 44 and 45

Comment 56

[Jason Smith [:jsmith]]

That's correct. Just reland https://bugzilla.mozilla.org/show_bug.cgi?id=852361#c36.

Comment 57

[Ryan VanderMeulen [:RyanVM]]

(In reply to Jason Smith [:jsmith] from comment #56)
> That's correct. Just reland
> https://bugzilla.mozilla.org/show_bug.cgi?id=852361#c36.

John, ping :)

Comment 58

[John Ford [:jhford] CET/CEST Berlin Time]

[v1-train f7afca0]

Comment 59

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

v1.1.0hd: f7afca06d02ee34fcd82eb0020eb2162e5045ee7

Comment 60

[Jason Smith [:jsmith]]

Looks like we never relanded this on 1.2+. John - Can you land that on master & 1.2?

Comment 61

[John Ford [:jhford] CET/CEST Berlin Time]

Relanding:

[master 370cfa4]
[v1.2 0c75e06]