[Battery] Implementation of battery discharging remaining time

RESOLVED FIXED

Status

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: gandalf, Assigned: gandalf)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

3 years ago
+++ This bug was initially created as a clone of Bug #1116368 +++

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

Updated

3 years ago
No longer blocks: 1115921
(Assignee)

Comment 1

3 years ago
Created attachment 8603679 [details] [diff] [review]
patch

Dave, this patch is parallel to patch from bug 1116368.

I tested it on Nexus 5 and it seems to work well. I'd like to use it for plotting battery chart over time.
Assignee: nobody → gandalf
Status: NEW → ASSIGNED
Attachment #8603679 - Flags: review?(dhylands)
(Assignee)

Comment 2

3 years ago
Just a reminder for myself: it seems that switching charging state uses previous state for calculation. We probably need to reset the previous state when charging changes.
Status: ASSIGNED → NEW
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #2)
> Just a reminder for myself: it seems that switching charging state uses
> previous state for calculation. We probably need to reset the previous state
> when charging changes.

lastLevelChange is cleared whenever the state changes from charging to discharging (or vice-versa).

I don't see the static variable remainingTime being reset though.
Comment on attachment 8603679 [details] [diff] [review]
patch

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

Looks good except for the point about remainingTime.

::: hal/gonk/GonkHal.cpp
@@ +691,5 @@
> +
> +    } else {
> +      clock_gettime(CLOCK_MONOTONIC, &now);
> +      dtime = now.tv_sec - lastLevelChange.tv_sec;
> +      if (dtime < remainingTime) {

It seems like remainingTime might not be set properly if you change state from charging to dischanrging and aBatteryInfo->level() == previousLevel

remainingTime would contain the remaining time to reach full charge, not the remainingTime until discharged. This would autocorrect fairly quickly, but that's the only case I don't see handled properly.
Attachment #8603679 - Flags: review?(dhylands) → review+
(Assignee)

Comment 5

3 years ago
Created attachment 8604392 [details] [diff] [review]
patch v2

Updated the patch to reset remainingTime. Carrying r+
Attachment #8603679 - Attachment is obsolete: true
Attachment #8604392 - Flags: review+
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
(Assignee)

Updated

3 years ago
Blocks: 1163867
(Assignee)

Updated

3 years ago
Blocks: 896847
No longer blocks: 1163867
(Assignee)

Updated

3 years ago
Status: NEW → ASSIGNED
(Assignee)

Updated

3 years ago
Blocks: 1163870
(Assignee)

Updated

3 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.