1116368 - [Battery] Implementation of battery charging remaining time

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Danny Liang [:dliang]]

This bug is to discuss the implementation of battery charging remaining time.
Currently, we didn't have calculation of battery charging remaining time, there are only two values, kUnknownRemainingTime and kDefaultRemainingTime.
http://dxr.mozilla.org/mozilla-central/source/hal/gonk/GonkHal.cpp?from=GonkHal.cpp&case=true#590-595

Comment 1

[Zibi Braniecki [:zbraniecki][:gandalf]]

I'm doing some tests and it seems that we do support it on Android - I am able to get calculations of dischargingTime and chargingTime out of the API on Firefox Beta on Android.

Can we reuse the same code for gonk?

Comment 2

[Danny Liang [:dliang]]

(In reply to Zibi Braniecki [:gandalf] from comment #1)
> I'm doing some tests and it seems that we do support it on Android - I am
> able to get calculations of dischargingTime and chargingTime out of the API
> on Firefox Beta on Android.
> 
> Can we reuse the same code for gonk?

May I know what API do you use to get/calculate charging remaining time?

Comment 3

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: batteryapitest.html

I created this very basic test app. It works on Chrome and Gecko.

Comment 4

[Zibi Braniecki [:zbraniecki][:gandalf]]

(In reply to Danny Liang [:dliang] from comment #2)
> (In reply to Zibi Braniecki [:gandalf] from comment #1)
> > I'm doing some tests and it seems that we do support it on Android - I am
> > able to get calculations of dischargingTime and chargingTime out of the API
> > on Firefox Beta on Android.
> > 
> > Can we reuse the same code for gonk?
> 
> May I know what API do you use to get/calculate charging remaining time?

I just used navigator.battery.dischargingTime and navigator.battery.chargingTime according to the MDN [0].
There's also a bug about updating our API to the latest version of the spec, but I believe it will not touch the internals, just expose the API differently.

[0] https://developer.mozilla.org/en-US/docs/Web/API/navigator.battery
[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1047119

Comment 5

[Danny Liang [:dliang]]

(In reply to Zibi Braniecki [:gandalf] from comment #4)
> (In reply to Danny Liang [:dliang] from comment #2)
> > (In reply to Zibi Braniecki [:gandalf] from comment #1)
> > > I'm doing some tests and it seems that we do support it on Android - I am

Sorry for misunderstanding, I am asking what API did you use to get/calculate charging remaining time on Android?

> > > able to get calculations of dischargingTime and chargingTime out of the API
> > > on Firefox Beta on Android.
> > > 
> > > Can we reuse the same code for gonk?
> > 
> > May I know what API do you use to get/calculate charging remaining time?
> 
> I just used navigator.battery.dischargingTime and
> navigator.battery.chargingTime according to the MDN [0].
> There's also a bug about updating our API to the latest version of the spec,
> but I believe it will not touch the internals, just expose the API
> differently.
> 
> [0] https://developer.mozilla.org/en-US/docs/Web/API/navigator.battery
> [1] https://bugzilla.mozilla.org/show_bug.cgi?id=1047119

Comment 6

[Zibi Braniecki [:zbraniecki][:gandalf]]

(In reply to Danny Liang [:dliang] from comment #5)

> Sorry for misunderstanding, I am asking what API did you use to
> get/calculate charging remaining time on Android?

Battery API. The one I linked as [0]. I attached an HTML file with a test. If you launch it in Firefox on Android it will properly calculate dischargingTime and chargingTime.

Comment 7

[Zibi Braniecki [:zbraniecki][:gandalf]]

What are the next steps here? I'd love to help with this as it's blocking a few bugs I'd like to work on soon.

Comment 8

[Danny Liang [:dliang]]

(In reply to Zibi Braniecki [:gandalf] from comment #7)
> What are the next steps here? I'd love to help with this as it's blocking a
> few bugs I'd like to work on soon.

Sorry for late reply. I think it's possible to implement the similar algorithm in gonk hal. I am busy on other on-going projects and put it into my to do list. It will be good if someone can take this bug.

Comment 9

[Zibi Braniecki [:zbraniecki][:gandalf]]

(In reply to Danny Liang [:dliang] from comment #8)
> Sorry for late reply. I think it's possible to implement the similar
> algorithm in gonk hal. I am busy on other on-going projects and put it into
> my to do list. It will be good if someone can take this bug.

So, does it mean that Android is not providing this to us and we have to implement our own algorithm in gonk/hal?

Firefox for Android is using something to get this data, either by implementing it or by using some lower level Android API to get this, right?

Danny, do you know anyone who might have time to guide me through this so that I can try working on that?

Comment 10

[Danny Liang [:dliang]]

(In reply to Zibi Braniecki [:gandalf] from comment #9)
> (In reply to Danny Liang [:dliang] from comment #8)
> > Sorry for late reply. I think it's possible to implement the similar
> > algorithm in gonk hal. I am busy on other on-going projects and put it into
> > my to do list. It will be good if someone can take this bug.
> 
> So, does it mean that Android is not providing this to us and we have to
> implement our own algorithm in gonk/hal?

I think so. Currently, gonk/hal just provide battery information include charging status and charging level. Charging remaining time should be calculated by charging level.

> 
> Firefox for Android is using something to get this data, either by
> implementing it or by using some lower level Android API to get this, right?
> 

In Android, the charging remaining time is calculated here: https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/GeckoBatteryManager.java

> Danny, do you know anyone who might have time to guide me through this so
> that I can try working on that?

I am not sure who is the proper owner for this part, maybe Michael Wu can help on this.
I am trying to have patch for this bug, it's under verify and maybe I can provide patch on 1/13.

Comment 11

[Danny Liang [:dliang]]

Attachment: Bug-1116368-Implementation-of-battery-charging-remai.patch

Comment 12

[Danny Liang [:dliang]]

Hi Zibi,
I have implemented the battery charging remaining time algorithm in GonkHal.cpp by follow GeckoBatteryManager.java. I have verified this patch on my side, could you please give me some feedback to see if it works? Then I will find proper owner to review this patch.

Comment 13

[Zibi Braniecki [:zbraniecki][:gandalf]]

Hi Danny,

I tested your patch and it works well! Thanks!

While testing with the testcase I attached in this bug against Flame device, I noticed a few things:

1) The dischargingTime and its event are not working. If that's a lot of work I can file a separate bug, but it would be nice to have both for bug 874345.
2) While testing the calculations I run the following experiment three times:

a) Open the testcase and press start
b) wait for charge to drop to 95%
c) plug a cable and observe until 100%
d) unnplug and wait until 95%

I did this three times. Observations:

Session 1:
[00][level] 0.95
[00][charging] true
[00][level] 0.96
[04][level] 0.97
[04][chargingTime] 720
[05][chargingTime] 686
[08][level] 0.98
[08][chargingTime] 480
[11][chargingTime] 300
[12][level] 0.99
[12][chargingTime] 220
[20][level] 1
[20][chargingTime] 0

I'm not sure if that's expected but the calculations only showed after 2% of charging. That means at least two minutes of wait until the chargingTime changes from Infinite to the first calculation. Is that per-design?

The first column is time since unplugged. The whole charge to took ~20min. So, the first estimation showed 12min charge time, corrected a minute later to 11min, then, with 12min left we got to 98% and estimation of 8min. With 9min left, the estimation changed to 5min and a minute later to 3.5 min. Then 8 minutes of silence and done.

That's not bad, but I'm wondering if we can somehow improve that. Notice that the estimation was correct between 95% and 99% with 1% charge per 3 minutes but the last percent took 8 minutes.

Session 2:
[00][level] 0.95
[01][chargingTime] 120
[03][chargingTime] Infinity
[04][level] 0.96
[04][chargingTime] 960
[04][level] 0.97
[04][chargingTime] 60
[04][level] 0.98
[04][chargingTime] 40
[08][level] 0.99
[08][chargingTime] 220
[11][chargingTime] 20
[13][level] 1
[13][chargingTime] 0

This session looks weirder. The initial switch from 120 to Infinity is weird. Not sure what caused it. Then the time is clearly off and varies a lot, but it doesn't seem like a calculation fault, since the timestamps for 0.96 to 0.98 are within the same minute. The whole charge took 13 minutes, and the best estimation from 0.96 showed 16 minutes.



Session 3:
[00][level] 0.95 
[00][charging] true
[02][level] 0.96
[03][chargingTime] 120
[05][chargingTime] 44
[06][level] 0.97
[06][chargingTime] 780
[08][chargingTime] 640
[09][chargingTime] 600
[10][level] 0.98
[10][chargingTime] 480
[15][level] 0.99
[15][chargingTime] 280
[20][level] 1
[20][chargingTime] 0

That's again a more normal estimation. Except of the flux around 03-05 min where the time dropped from 120 to 44 it looks like a good estimation range.

=====

I know it's going to even out over longer time and if I measured from 50% charge it would look better, so I'd say it's good to go :)

Comment 14

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8548758 [details] [diff] [review]
Bug-1116368-Implementation-of-battery-charging-remai.patch

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

I did some more tests including dicharging Flame to 0.2 and charging it to 1.

Additional observations:
 - the chargingTime number seems to be a float, which looks odd - 6999.999999999994 sec till full. I believe that according to the spec it should be an integer
 - the variation is really high. I'm not sure if it's related to the algorithm, but during this experiment I got results that were quite off from the reality. 

At 20% my chargingTime was 80 minutes (but it took 172 minutes to get from here to 100%)
At 30% my chargingTime was 116 minutes (but it took 139 minutes to get from here to 100%)
At 40% my chargingTime was 100 minutes (but it took 119 minutes to get from here to 100%)
At 70% my chargingTime was 30 minutes (but it took 82 minutes to get from here to 100%)
At 80% my chargingTime was 19 minutes (but it took 73 minutes to get from here to 100%)
At 90% my chargingTime was 9 minutes (but it took 60 minutes to get from here to 100%)

But on top of that, there's also a lot of fluctuation much more often. Like, at 0.31 it will be 9000 minutes, and one minute later it will be 4200 minutes and one minute later it will jump back to 7200 minutes.

I'm wondering if we could adjust the algorithm to minimize the fluctuations. Especially over time with longer charges I would expect the variation to be smaller.

 - there seems to be a fairly repetitive jump in the last few percent. For majority of the charge it charged at the rate of 1% per 2-3 minutes. It took nearly 60 minutes to get from 95% to 100%.

I'm currently building a log of a charge from 30% to 100% for macos, android tablet, nexus 5 and flame. I'll post the logs here.

Comment 15

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: charging log from flame

Comment 16

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: charging log from firefox nightly on nexus 7 (tablet)

Comment 17

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: charging log from firefox nightly on macbook air

Comment 18

[Zibi Braniecki [:zbraniecki][:gandalf]]

So, it got me thinking.

It seems that in Android we could get the current charging/discharging rate from their API: http://developer.android.com/reference/android/os/BatteryManager.html or even get ready to use "Battery remaining energy in nanowatt-hours, as a long integer. ".

If the latter is not available, could we store discharge rate over time and adjust weights to more recent results? I'm thinking of ways to "smoothen" the value because if the user will plug their phone and see "80 minutes left" and a minute later "130 minutes left" and then "40 minutes left" it doesn't make a really good UX.

I'm ok with us being off initially, but I believe it makes sense to aim for minimal variation after certain amount of data from the charge has been accrued.

I never wrote any code like this, but sth like:

var batteryChargeRate = [];

function onBatteryChargingUpdate(level) {
  var timestamp = new Date(); 
  batteryChargeRate.push([timestamp, level]);
}

function calculateChargeRate() {
  var now = new Date();
  var timeScale = now - batteryChargeRate[0][0];
  
  var sum = batteryChargeRate.reduce(function(v, prev) {
    var dtime = now - v[0];
    var weight = 1 - (dtime/timeScale)

    // I didn't figure out the math for calculatin the weight properly here :(

    return prev + (v[1] * weight);
  });

  return parseInt(sum/batteryChargeRate.length);
}

With good weight factor, it should give more value to the recent charge rate, but keep the average based on the whole charge started and in result minimize fluctuations.

Does it sound possible? :)

Comment 19

[Danny Liang [:dliang]]

(In reply to Zibi Braniecki [:gandalf] from comment #13)
> Hi Danny,
> 
> I tested your patch and it works well! Thanks!
> 
> While testing with the testcase I attached in this bug against Flame device,
> I noticed a few things:
> 
> 1) The dischargingTime and its event are not working. If that's a lot of
> work I can file a separate bug, but it would be nice to have both for bug
> 874345.
> 2) While testing the calculations I run the following experiment three times:
> 
> a) Open the testcase and press start
> b) wait for charge to drop to 95%
> c) plug a cable and observe until 100%
> d) unnplug and wait until 95%
> 
> I did this three times. Observations:
> 
> Session 1:
> [00][level] 0.95
> [00][charging] true
> [00][level] 0.96
> [04][level] 0.97
> [04][chargingTime] 720
> [05][chargingTime] 686
> [08][level] 0.98
> [08][chargingTime] 480
> [11][chargingTime] 300
> [12][level] 0.99
> [12][chargingTime] 220
> [20][level] 1
> [20][chargingTime] 0
> 
> I'm not sure if that's expected but the calculations only showed after 2% of
> charging. That means at least two minutes of wait until the chargingTime
> changes from Infinite to the first calculation. Is that per-design?
> 
No. Ideally, charging remaining time will be update at first level changed. I guess there might be some bugs in algorithm so that you saw the calculation after 2% update.

> The first column is time since unplugged. The whole charge to took ~20min.
> So, the first estimation showed 12min charge time, corrected a minute later
> to 11min, then, with 12min left we got to 98% and estimation of 8min. With
> 9min left, the estimation changed to 5min and a minute later to 3.5 min.
> Then 8 minutes of silence and done.
> 
> That's not bad, but I'm wondering if we can somehow improve that. Notice
> that the estimation was correct between 95% and 99% with 1% charge per 3
> minutes but the last percent took 8 minutes.
> 
Hmm, I think we might need more battery information to solve this problem. In general, charging activity keeps CC mode under 4.2V, then it became CV mode. At CV mode, the charging current is decreased so I think the charging remain time will be longer. It higher depended on vendor's battery algorithm. 

In my opinion, there are some limitation if we use the algorithm from https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/GeckoBatteryManager.java. If We want to get precise charging remaining time, we need to have charging current and battery size.

> Session 2:
> [00][level] 0.95
> [01][chargingTime] 120
> [03][chargingTime] Infinity
> [04][level] 0.96
> [04][chargingTime] 960
> [04][level] 0.97
> [04][chargingTime] 60
> [04][level] 0.98
> [04][chargingTime] 40
> [08][level] 0.99
> [08][chargingTime] 220
> [11][chargingTime] 20
> [13][level] 1
> [13][chargingTime] 0
> 
> This session looks weirder. The initial switch from 120 to Infinity is
> weird. Not sure what caused it. Then the time is clearly off and varies a
> lot, but it doesn't seem like a calculation fault, since the timestamps for
> 0.96 to 0.98 are within the same minute. The whole charge took 13 minutes,
> and the best estimation from 0.96 showed 16 minutes.
> 

This case is because the first update is MAYBE from 95.4 (round off value is 95) to 95.7 (round off value is 96), so the dtime was shorter. It will cause we got shorter charging remaining time by following algorithm: remainingTime = (double) dtime / dlevel * (1.0 - aBatteryInfo->level())

> [01][chargingTime] 120
> [03][chargingTime] Infinity
=> The charging remaining time is "Infinity" because we get shorter charging remaining time (120s) on last calculation. After 120s, the charging remaining time should be 0s, but we have no idea to update the time.
Currently, we are talking about the constant charging current, if the current is unstable, we will meet a problem is hard to update charging remaining time if the battery level keeps at the same level.

> 
> 
> Session 3:
> [00][level] 0.95 
> [00][charging] true
> [02][level] 0.96
> [03][chargingTime] 120
> [05][chargingTime] 44
> [06][level] 0.97
> [06][chargingTime] 780
> [08][chargingTime] 640
> [09][chargingTime] 600
> [10][level] 0.98
> [10][chargingTime] 480
> [15][level] 0.99
> [15][chargingTime] 280
> [20][level] 1
> [20][chargingTime] 0
> 
> That's again a more normal estimation. Except of the flux around 03-05 min
> where the time dropped from 120 to 44 it looks like a good estimation range.
> 
> =====
> 
> I know it's going to even out over longer time and if I measured from 50%
> charge it would look better, so I'd say it's good to go :)

Comment 20

[Danny Liang [:dliang]]

(In reply to Zibi Braniecki [:gandalf] from comment #14)
> Comment on attachment 8548758 [details] [diff] [review]
> Bug-1116368-Implementation-of-battery-charging-remai.patch
> 
> Review of attachment 8548758 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I did some more tests including dicharging Flame to 0.2 and charging it to 1.
> 
> Additional observations:
>  - the chargingTime number seems to be a float, which looks odd -
> 6999.999999999994 sec till full. I believe that according to the spec it
> should be an integer

My bad, I didn't notice the spec and I can fix it.

>  - the variation is really high. I'm not sure if it's related to the
> algorithm, but during this experiment I got results that were quite off from
> the reality. 
> 
> At 20% my chargingTime was 80 minutes (but it took 172 minutes to get from
> here to 100%)
> At 30% my chargingTime was 116 minutes (but it took 139 minutes to get from
> here to 100%)
> At 40% my chargingTime was 100 minutes (but it took 119 minutes to get from
> here to 100%)
> At 70% my chargingTime was 30 minutes (but it took 82 minutes to get from
> here to 100%)
> At 80% my chargingTime was 19 minutes (but it took 73 minutes to get from
> here to 100%)
> At 90% my chargingTime was 9 minutes (but it took 60 minutes to get from
> here to 100%)
> 

Hmm...this problem highly depended on low-level battery algorithm. In gonk level, we just used the battery level that update from battery driver. You can refer the static struct pc_temp_ocv_lut  pc_temp_ocv_id_2 in board-m7.c, this is battery level table included two parameters. temperature and battery voltage. As I know, this table is optimized and it's almost linear. So, if the partner didn't optimized the battery table, gonk hal will get battery level update with is non-linear. That's why I said this problem highly depended on low-level battery algorithm.

> But on top of that, there's also a lot of fluctuation much more often. Like,
> at 0.31 it will be 9000 minutes, and one minute later it will be 4200
> minutes and one minute later it will jump back to 7200 minutes.
> 
> I'm wondering if we could adjust the algorithm to minimize the fluctuations.
> Especially over time with longer charges I would expect the variation to be
> smaller.
> 
>  - there seems to be a fairly repetitive jump in the last few percent. For
> majority of the charge it charged at the rate of 1% per 2-3 minutes. It took
> nearly 60 minutes to get from 95% to 100%.
> 
> I'm currently building a log of a charge from 30% to 100% for macos, android
> tablet, nexus 5 and flame. I'll post the logs here.

Let me have a summary of current battery algorithm, the fluctuations of charging remaining time were caused by:
1. The round off problem of battery update
2. The battery update from battery driver is non-linear.
3. Charging current is decreased at final few levels

If we have to get precise charging remaining time, we might need following parameters from kernel side to adjust it.
1. charging current
2. battery capacity
3. coulomb counter if platform support

Comment 21

[Danny Liang [:dliang]]

Attachment: board-m7.c

Attachment is board config file of HTC M7, it included the battery table to make the battery level linear.

Comment 22

[Danny Liang [:dliang]]

(In reply to Zibi Braniecki [:gandalf] from comment #18)
> So, it got me thinking.
> 
> It seems that in Android we could get the current charging/discharging rate
> from their API:
> http://developer.android.com/reference/android/os/BatteryManager.html or
> even get ready to use "Battery remaining energy in nanowatt-hours, as a long
> integer. ".
> 
> If the latter is not available, could we store discharge rate over time and
> adjust weights to more recent results? I'm thinking of ways to "smoothen"
> the value because if the user will plug their phone and see "80 minutes
> left" and a minute later "130 minutes left" and then "40 minutes left" it
> doesn't make a really good UX.
> 
> I'm ok with us being off initially, but I believe it makes sense to aim for
> minimal variation after certain amount of data from the charge has been
> accrued.
> 
> I never wrote any code like this, but sth like:
> 
> var batteryChargeRate = [];
> 
> function onBatteryChargingUpdate(level) {
>   var timestamp = new Date(); 
>   batteryChargeRate.push([timestamp, level]);
> }
> 
> function calculateChargeRate() {
>   var now = new Date();
>   var timeScale = now - batteryChargeRate[0][0];
>   
>   var sum = batteryChargeRate.reduce(function(v, prev) {

May I know what does "v" mean?

>     var dtime = now - v[0];
>     var weight = 1 - (dtime/timeScale)
> 
>     // I didn't figure out the math for calculatin the weight properly here
> :(
> 
>     return prev + (v[1] * weight);
>   });
> 
>   return parseInt(sum/batteryChargeRate.length);
> }
> 
> With good weight factor, it should give more value to the recent charge
> rate, but keep the average based on the whole charge started and in result
> minimize fluctuations.
> 
> Does it sound possible? :)

I agree to use some statistical methods to reduce the fluctuations, it might work.

Comment 23

[Zibi Braniecki [:zbraniecki][:gandalf]]

Ok, I'd love to explore the polishing functions to get more consistent results, but at this point, I'd prefer to get what we have into 2.2 and file a follow up to update our algorithms for all platforms.

Would you agree?

If so, let's aim at landing this soon and backporting to 2.2. I already landed the UI on lockscreen, so we will bring value to the user in 2.2 if we get this patch landed.

Comment 24

[Zibi Braniecki [:zbraniecki][:gandalf]]

Ping?

Comment 25

[Danny Liang [:dliang]]

Attachment: Bug-1116368-Implementation-of-battery-charging-remaining-time_v2.patch

Hi Dave, could you help to review the patch for charging remaining time? or you can help me to find a proper reviewer?

Comment 26

[Dave Hylands [:dhylands]]

Comment on attachment 8571856 [details] [diff] [review]
Bug-1116368-Implementation-of-battery-charging-remaining-time_v2.patch

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

I think this needs some cleanup, so r-'ing for now.

::: hal/gonk/GonkHal.cpp
@@ +572,5 @@
>  GetCurrentBatteryInformation(hal::BatteryInformation* aBatteryInfo)
>  {
>    int charge;
> +  static bool previousCharging = false;
> +  static double previousLevel = 0, remainingTime = 0;

nit: I'd use 0.0 here rather than 0 since 0.0 is a double and 0 is an int.

@@ +596,2 @@
>      aBatteryInfo->remainingTime() = dom::battery::kUnknownRemainingTime;
> +	memset(&lastLevelChange, 0, sizeof(struct timespec));

nit: indentation

@@ +601,5 @@
> +    if (aBatteryInfo->level() == 1.0) {
> +      aBatteryInfo->remainingTime() = dom::battery::kDefaultRemainingTime;
> +    } else if (aBatteryInfo->level() != previousLevel){
> +      if (lastLevelChange.tv_sec != 0) {
> +        clock_gettime(CLOCK_REALTIME, &now);

Shouldn't this use CLOCK_MONOTONIC?

When crossing/changing timezone boundaries or the user changing the date/time CLOCK_REALTIME can jump forwards or backwards in time.

CLOCK_MONOTONIC doesn't.

@@ +605,5 @@
> +        clock_gettime(CLOCK_REALTIME, &now);
> +        dtime = now.tv_sec - lastLevelChange.tv_sec;
> +        dlevel = aBatteryInfo->level() - previousLevel;
> +
> +        if (dlevel < 0){

nit: dlevel is a double, so lets use 0.0 to remind the reader of that.

@@ +608,5 @@
> +
> +        if (dlevel < 0){
> +          aBatteryInfo->remainingTime() = dom::battery::kUnknownRemainingTime;
> +        } else {
> +          remainingTime = (double) round(dtime / dlevel * (1.0 - aBatteryInfo->level()));

You've got a divide by zero here if dlevel == 0.0

@@ +612,5 @@
> +          remainingTime = (double) round(dtime / dlevel * (1.0 - aBatteryInfo->level()));
> +          aBatteryInfo->remainingTime() = remainingTime;
> +        }
> +
> +        clock_gettime(CLOCK_REALTIME, &lastLevelChange);

Shouldn't this just do lastLevelChange = now; rather than making another system call?

@@ +618,5 @@
> +        clock_gettime(CLOCK_REALTIME, &lastLevelChange);
> +        aBatteryInfo->remainingTime() = dom::battery::kUnknownRemainingTime;
> +      }
> +
> +    } else if (aBatteryInfo->level() == previousLevel) {

This condition will always be true (since this is the exact negation of the test on line 603), so this can just be a plain else.

Comment 27

[Danny Liang [:dliang]]

(In reply to Dave Hylands [:dhylands] from comment #26)
> Comment on attachment 8571856 [details] [diff] [review]
> Bug-1116368-Implementation-of-battery-charging-remaining-time_v2.patch
> 
> Review of attachment 8571856 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think this needs some cleanup, so r-'ing for now.
> 

Thanks for review, I will fix the nits and submit the patch.

> > +
> > +        if (dlevel < 0){
> > +          aBatteryInfo->remainingTime() = dom::battery::kUnknownRemainingTime;
> > +        } else {
> > +          remainingTime = (double) round(dtime / dlevel * (1.0 - aBatteryInfo->level()));
> 
> You've got a divide by zero here if dlevel == 0.0

In this case, we won't get a divide by zero due to dlevel = aBatteryInfo->level() - previousLevel and it won't be zero in this scope.

Comment 28

[Danny Liang [:dliang]]

Attachment: Bug-1116368-Implementation-of-battery-charging-remaining-time_v3.patch

Comment 29

[Dave Hylands [:dhylands]]

Comment on attachment 8573791 [details] [diff] [review]
Bug-1116368-Implementation-of-battery-charging-remaining-time_v3.patch

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

r=me with nits addressed. No need to submit for re-review (although you can if you want - it isn't needed).

::: hal/gonk/GonkHal.cpp
@@ +605,5 @@
> +        clock_gettime(CLOCK_MONOTONIC, &now);
> +        dtime = now.tv_sec - lastLevelChange.tv_sec;
> +        dlevel = aBatteryInfo->level() - previousLevel;
> +
> +        if (dlevel < 0.0){

nit: space before open curly brace.

@@ +608,5 @@
> +
> +        if (dlevel < 0.0){
> +          aBatteryInfo->remainingTime() = dom::battery::kUnknownRemainingTime;
> +        } else {
> +          remainingTime = (double) round(dtime / dlevel * (1.0 - aBatteryInfo->level()));

If this really can't be called with dlevel == 0.0, then why not change the 

if (dlevel < 0.0) on line 609 to be if (dlevel <= 0.0)

and then I wouldn't care about the potential for divide by zero.

My concern is that you're calling aBattery->level() twice in a row, and assuming that both times you call the function that it will return the exact same result. Which may very well be the case, but you could also make it explicit by doing something like:

double currBatteryLevel = aBatteryInfo->level(); near the top of the function and then using currBatteryLevel thoughout the function. Then I'd know for sure that currBatteryLevel will stay the same throughout the function.

I'd still prefer to see the if dlevel < 0.0 changed to if dlevel <= 0.0 (just being paranoid).

@@ +613,5 @@
> +          aBatteryInfo->remainingTime() = remainingTime;
> +        }
> +
> +        lastLevelChange.tv_sec = now.tv_sec;
> +        lastLevelChange.tv_nsec = now.tv_nsec;

nit: You don't need to assign fields individually. You can just do structure assignment.

lastLevelChange = now;

Comment 30

[Danny Liang [:dliang]]

(In reply to Dave Hylands [:dhylands] from comment #29)
> Comment on attachment 8573791 [details] [diff] [review]
> Bug-1116368-Implementation-of-battery-charging-remaining-time_v3.patch
> 
> Review of attachment 8573791 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with nits addressed. No need to submit for re-review (although you can
> if you want - it isn't needed).
> 

Thanks for your review, I will fix following nits and attach patch again.

> ::: hal/gonk/GonkHal.cpp
> @@ +605,5 @@
> > +        clock_gettime(CLOCK_MONOTONIC, &now);
> > +        dtime = now.tv_sec - lastLevelChange.tv_sec;
> > +        dlevel = aBatteryInfo->level() - previousLevel;
> > +
> > +        if (dlevel < 0.0){
> 
> nit: space before open curly brace.
> 
> @@ +608,5 @@
> > +
> > +        if (dlevel < 0.0){
> > +          aBatteryInfo->remainingTime() = dom::battery::kUnknownRemainingTime;
> > +        } else {
> > +          remainingTime = (double) round(dtime / dlevel * (1.0 - aBatteryInfo->level()));
> 
> If this really can't be called with dlevel == 0.0, then why not change the 
> 
> if (dlevel < 0.0) on line 609 to be if (dlevel <= 0.0)
> 
> and then I wouldn't care about the potential for divide by zero.
> 
> My concern is that you're calling aBattery->level() twice in a row, and
> assuming that both times you call the function that it will return the exact
> same result. Which may very well be the case, but you could also make it
> explicit by doing something like:
> 
> double currBatteryLevel = aBatteryInfo->level(); near the top of the
> function and then using currBatteryLevel thoughout the function. Then I'd
> know for sure that currBatteryLevel will stay the same throughout the
> function.
> 
> I'd still prefer to see the if dlevel < 0.0 changed to if dlevel <= 0.0
> (just being paranoid).
> 
> @@ +613,5 @@
> > +          aBatteryInfo->remainingTime() = remainingTime;
> > +        }
> > +
> > +        lastLevelChange.tv_sec = now.tv_sec;
> > +        lastLevelChange.tv_nsec = now.tv_nsec;
> 
> nit: You don't need to assign fields individually. You can just do structure
> assignment.
> 
> lastLevelChange = now;
(In reply to Dave Hylands [:dhylands] from comment #29)
> Comment on attachment 8573791 [details] [diff] [review]
> Bug-1116368-Implementation-of-battery-charging-remaining-time_v3.patch
> 
> Review of attachment 8573791 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with nits addressed. No need to submit for re-review (although you can
> if you want - it isn't needed).
> 
> ::: hal/gonk/GonkHal.cpp
> @@ +605,5 @@
> > +        clock_gettime(CLOCK_MONOTONIC, &now);
> > +        dtime = now.tv_sec - lastLevelChange.tv_sec;
> > +        dlevel = aBatteryInfo->level() - previousLevel;
> > +
> > +        if (dlevel < 0.0){
> 
> nit: space before open curly brace.
> 
> @@ +608,5 @@
> > +
> > +        if (dlevel < 0.0){
> > +          aBatteryInfo->remainingTime() = dom::battery::kUnknownRemainingTime;
> > +        } else {
> > +          remainingTime = (double) round(dtime / dlevel * (1.0 - aBatteryInfo->level()));
> 
> If this really can't be called with dlevel == 0.0, then why not change the 
> 
> if (dlevel < 0.0) on line 609 to be if (dlevel <= 0.0)
> 
> and then I wouldn't care about the potential for divide by zero.
> 
> My concern is that you're calling aBattery->level() twice in a row, and
> assuming that both times you call the function that it will return the exact
> same result. Which may very well be the case, but you could also make it
> explicit by doing something like:
> 
> double currBatteryLevel = aBatteryInfo->level(); near the top of the
> function and then using currBatteryLevel thoughout the function. Then I'd
> know for sure that currBatteryLevel will stay the same throughout the
> function.
> 
> I'd still prefer to see the if dlevel < 0.0 changed to if dlevel <= 0.0
> (just being paranoid).
> 
> @@ +613,5 @@
> > +          aBatteryInfo->remainingTime() = remainingTime;
> > +        }
> > +
> > +        lastLevelChange.tv_sec = now.tv_sec;
> > +        lastLevelChange.tv_nsec = now.tv_nsec;
> 
> nit: You don't need to assign fields individually. You can just do structure
> assignment.
> 
> lastLevelChange = now;

Comment 31

[Danny Liang [:dliang]]

Attachment: Bug-1116368-Implementation-of-battery-charging-remaining-time_v4_reviewed.patch

Comment 32

[Danny Liang [:dliang]]

try result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6d43b3f16dd9

Comment 33

[Ryan VanderMeulen [:RyanVM]]

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

Comment 34

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/70f0148af7f1

Comment 35

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8575812 [details] [diff] [review]
Bug-1116368-Implementation-of-battery-charging-remaining-time_v4_reviewed.patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): feature
User impact if declined: users will not be able to see remaining charging time on the lockscreen
Testing completed: tests, master, try servers, manual testing with the device (buri and flame) in hand 
Risk to taking this patch (and alternatives if risky): none
String or UUID changes made by this patch: none

This patch exposes an API that we already have, to B2G. Gaia code is already handling that API and when enabled displays remaining charging time on the lockscreen.

That basically means that instead of this: https://bug1115921.bugzilla.mozilla.org/attachment.cgi?id=8546966 users would see this: https://bugzilla.mozilla.org/attachment.cgi?id=8546965

It definitely is not a blocker for 2.2 and I understand that it's late, but I think it's worth considering because impact to risk ratio is huge here.

Comment 36

[bhavana bajaj [:bajaj]]

Can QA first verify this on a 3.0 nightly, before we consider uplift ? Thanks!

Comment 37

[Shally Li（leave from Mozilla）]

    By the following steps,it will show remaining charging time on latest Flame v3.0 nightly (rate:5/5). Please see "remaining_time.png".

Steps:
1.Plug in the Charger.
2.Press Power button to lock screen.
3.Wait for some minutes,press Power button.
**It displays "78% Charged(22 mins remaining)" on the lockscreen.  


Flame 3.0 build (unaffected):
Build ID               20150312160232
Gaia Revision          eabe35cf054d47087b37c1ca7db8143717fbd7f3
Gaia Date              2015-03-12 18:01:49
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/42afc7ef5ccb
Gecko Version          39.0a1
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150312.194521
Firmware Date          Thu Mar 12 19:45:32 EDT 2015
Bootloader             L1TC000118D0

Comment 38

[Shally Li（leave from Mozilla）]

Attachment: remaining_time.png

Comment 39

[bhavana bajaj [:bajaj]]

Comment on attachment 8575812 [details] [diff] [review]
Bug-1116368-Implementation-of-battery-charging-remaining-time_v4_reviewed.patch

If we see any fallouts, we should back this out rather than forward fixing.

Comment 40

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/44b8b4f5787e

Comment 41

[Gabriela [:gaby2300]]

Works fine for me using base image v188 and the latest OTA update as follows:

Build ID               20150316010202
Gaia Revision          4868c56c0a3b7a1e51d55b24457e44a7709ea1ae
Gaia Date              2015-03-14 18:50:17
Gecko Revision         n/a
Gecko Version          39.0a1
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  39
Firmware Date          Thu Oct 16 18:19:14 CST 2014
Bootloader             L1TC00011880