Closed Bug 852361 Opened 7 years ago Closed 6 years ago

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

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set
major

Tracking

(blocking-b2g:leo+, b2g18 fixed, b2g-v1.1hd fixed, b2g-v1.2 fixed)

RESOLVED FIXED
blocking-b2g leo+
Tracking Status
b2g18 --- fixed
b2g-v1.1hd --- fixed
b2g-v1.2 --- fixed

People

(Reporter: leo.bugzilla.gaia, Assigned: leo.bugzilla.gaia)

References

Details

Attachments

(2 files, 2 obsolete files)

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
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: needinfo?
Resolution: --- → INVALID
Status: RESOLVED → REOPENED
Resolution: INVALID → ---
Attached file Pull Request URL 8726
Attachment #727097 - Flags: review?
Flags: needinfo?
Assignee: nobody → leo.bugzilla.gaia
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.
Attachment #727097 - Flags: review? → review-
Is it policy of ffos ? Why do you leave some space?
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.
blocking-b2g: --- → leo?
Adding qawanted to see if this equally impacts Unagis. None of our devices should shutdown before 0 or 1 percent is reached.
blocking-b2g: leo? → leo+
Keywords: qawanted
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.
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.
Flags: needinfo?(overholt)
Flags: needinfo?(ladamski)
(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.
Flags: needinfo?(overholt)
Flags: needinfo?(mwu)
Flags: needinfo?(dhylands)
Flags: needinfo?(mwu)
Flags: needinfo?(ladamski)
Awesome! I'll check this out on emulator first, then. We may not need to test on Unagi.
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.
Keywords: qawanted
er, if you repro using above, use lowercase on the telnet session. :)
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.
Assignee: leo.bugzilla.gaia → promise09th
Assignee: promise09th → leo.bugzilla.gaia
Assignee: leo.bugzilla.gaia → nobody
If we've confirmed that we should keep drained the device to 0% battery, let's adopt leo's patch here...
(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?
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?
Flags: needinfo?(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.
Flags: needinfo?(mchen)
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.
Assignee: nobody → ladamski
Triage agrees with comment 19
Assignee: ladamski → 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?
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.
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%.
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?
Seems quite reasonable to me!
Attachment #766196 - Flags: review? → review?(mounir)
Status: REOPENED → ASSIGNED
Attachment #727097 - Attachment is obsolete: true
Attachment #766198 - Flags: review?(alive)
Attachment #766198 - Attachment mime type: text/plain → text/html
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.
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 on attachment 766198 [details]
Gaia Pull Request - v1 - Shutdown at 0% battery.

I think this is enough.
Attachment #766198 - Flags: review?(alive) → review+
Comment on attachment 727097 [details]
Pull Request URL 8726

I just talked to alive and he is ok with taking this patch.
Attachment #727097 - Attachment is obsolete: false
Attachment #727097 - Flags: review- → review+
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!
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.
@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 on attachment 766198 [details]
Gaia Pull Request - v1 - Shutdown at 0% battery.

Let's go with the original PR here, #8726.
Attachment #766198 - Attachment is obsolete: true
To clarify, the gecko patch is NOT required for this fix. PR that landed is all that's needed to resolve this.
Assignee: aus → leo.bugzilla.gaia
Status: ASSIGNED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
Attachment #766196 - Attachment is obsolete: true
Attachment #766196 - Flags: review?(mounir)
Uplifted a7f1e921c67d5d3d4e0afaa06aca79320784e806 to:
v1-train: 22e0768fbcec94d049b071bdbed967ed651cdd2e
1.1hd: 22e0768fbcec94d049b071bdbed967ed651cdd2e
Depends on: 902633
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.
Duplicate of this bug: 902633
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(In reply to lsblakk@mozilla.com [: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?
Flags: needinfo?(jhford)
Backouts:

master: dfff18a
v1-train: d6afd22

v1.1.0hd will merge the v1-train backout over when they next do their v1-train merge.
Flags: needinfo?(jhford)
v1.1.0hd: d6afd220abe85fed88444d412baf98c707433b77
John,

Please confirm if this is fixed on 1.1 as well (not 1.1hd)
Flags: needinfo?(jhford)
(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.
Flags: needinfo?(jhford)
(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.
Status: REOPENED → RESOLVED
Closed: 7 years ago6 years ago
Resolution: --- → FIXED
This was backed out, so this isn't fixed. Reopening.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached video 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.
(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)
Can you clarify more about what you mean about a time updating problem?
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).
(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.
Flags: needinfo?(jhford)
Keywords: checkin-needed
(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
Flags: needinfo?(jhford) → needinfo?(jsmith)
That's correct. Just reland https://bugzilla.mozilla.org/show_bug.cgi?id=852361#c36.
Flags: needinfo?(jsmith)
(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 :)
Flags: needinfo?(jhford)
[v1-train f7afca0]
Flags: needinfo?(jhford)
v1.1.0hd: f7afca06d02ee34fcd82eb0020eb2162e5045ee7
Looks like we never relanded this on 1.2+. John - Can you land that on master & 1.2?
Flags: needinfo?(jhford)
Relanding:

[master 370cfa4]
[v1.2 0c75e06]
Flags: needinfo?(jhford)
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.