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

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?

(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 created this very basic test app. It works on Chrome and Gecko.

(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

(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

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

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.

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

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

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

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.

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

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

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

(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

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

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

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.

Ping?

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

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.

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

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

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

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.

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

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

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