758103 - I/Gecko ( 2204): ###!!! ASSERTION: Battery API: When charging and level at 1.0, remaining time should be 0. Please fix your backend!: 'Error', file /Volumes/mac/moz/b2ggecko/dom/battery/BatteryManager.cpp, line 158

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Gregor Wagner [:gwagner]]

Seen with a gecko debug build on sgs2 after letting the device sit for a while charging.

I/Gecko   ( 2204): ###!!! ASSERTION: Battery API: When charging and level at 1.0, remaining time should be 0. Please fix your backend!: 'Error', file /Volumes/mac/moz/b2ggecko/dom/battery/BatteryManager.cpp, line 158

Comment 2

[Johnny Stenback (:jst)]

We should fix this, but if the UI on the hardware that we end up shipping on behaves correctly I don't see a need to block on this.

Comment 4

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: Set remaining time to 0 if battery level is 1

This is a patch based on the comments in bug 816620.

I observed this problem on the PandaBoard. In addition to fixing the actual bug, the patch cleans up the implementation of the getter function and adds handling for interrupted system calls.

Comment 5

[Mounir Lamouri (:mounir)]

Comment on attachment 687715 [details] [diff] [review]
Set remaining time to 0 if battery level is 1

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

Coding style:
Don't do:
if (foo)
  bar;
But:
if (foo) {
  bar;
}

I haven't reviewed much GetCurrentBatteryCharging() because the C-style code of this function is a bit hard to read for me.
In C++, you shouldn't need labels to close a file descriptor, you should use a guard object for that. I haven't try to find why "out" label is used.

Also, note that you will need a review from a Gonk peer. Putting mwu as a reviewer to make sure that happens.

::: hal/gonk/GonkHal.cpp
@@ +352,5 @@
>        NewRunnableFunction(UnregisterBatteryObserverIOThread));
>  }
>  
> +static bool
> +GetCurrentBatteryCapacity(double& aCapacity)

Take a pointer. A common C++ convention is that pointers are out parameters and reference are in parameters, that way the caller can guess if this is going to be in or out.

@@ +358,5 @@
> +  FILE* capacityFile;
> +
> +  do {
> +    capacityFile = fopen("/sys/class/power_supply/battery/capacity", "r");
> +  } while (!capacityFile && errno == EINTR);

This pattern is un-familliar to me. Why do you put fopen (and later fclose) in loops?

@@ +381,5 @@
> +  return true;
> +}
> +
> +static bool
> +GetCurrentBatteryCharging(int& aCharging, double& aChargingTime)

Pointers, too.

@@ +486,5 @@
> +    aBatteryInfo->charging() = true;
> +    if (aBatteryInfo->level() < 1.0)
> +      aBatteryInfo->remainingTime() = dom::battery::kUnknownRemainingTime;
> +    else
> +      aBatteryInfo->remainingTime() = 0;

aBatteryInfo->remainingTime() = dom::battery::kDefaultRemainingTime;

Comment 6

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Hi,

And thanks so far. 

> I haven't reviewed much GetCurrentBatteryCharging() because the C-style code
> of this function is a bit hard to read for me.
> In C++, you shouldn't need labels to close a file descriptor, you should use
> a guard object for that. I haven't try to find why "out" label is used.

What class should I use for the guard object? I've been searching the Mozilla source code, but couldn't find anything.

> > +static bool
> > +GetCurrentBatteryCapacity(double& aCapacity)
> 
> Take a pointer. A common C++ convention is that pointers are out parameters
> and reference are in parameters, that way the caller can guess if this is
> going to be in or out.

I'm going to change this, but there is another convention that says to use references when the passed value cannot be NULL, and pointers when it can. That's what I had in mind here.
 
> @@ +358,5 @@
> > +  FILE* capacityFile;
> > +
> > +  do {
> > +    capacityFile = fopen("/sys/class/power_supply/battery/capacity", "r");
> > +  } while (!capacityFile && errno == EINTR);
> 
> This pattern is un-familliar to me. Why do you put fopen (and later fclose)
> in loops?

In Unix, long-running system calls might get aborted when the process receives a signal. In that case, the call fails and errno is set to EINTR. You typically don't see this in Linux, because glibc automatically restarts the system call in this case. I looked through Android's bionic libc, but couldn't find code for restarting system calls; so I added those loops to try as long as EINTR is returned (it's fairly boilerplate code). While the stdio functions are not system calls themselves, they are still affected, because they rely on system calls internally.

Comment 7

[Dave Hylands [:dhylands]]

(In reply to Thomas Zimmermann from comment #6)
> Hi,
> 
> And thanks so far. 
> 
> > I haven't reviewed much GetCurrentBatteryCharging() because the C-style code
> > of this function is a bit hard to read for me.
> > In C++, you shouldn't need labels to close a file descriptor, you should use
> > a guard object for that. I haven't try to find why "out" label is used.
> 
> What class should I use for the guard object? I've been searching the
> Mozilla source code, but couldn't find anything.

See ScopedClose: 
#include <mozilla/FileUtils.h>
https://mxr.mozilla.org/mozilla-central/source/xpcom/glue/FileUtils.h#54

Sample Usage:
https://mxr.mozilla.org/mozilla-central/source/widget/gonk/Framebuffer.cpp#141

> > @@ +358,5 @@
> > > +  FILE* capacityFile;
> > > +
> > > +  do {
> > > +    capacityFile = fopen("/sys/class/power_supply/battery/capacity", "r");
> > > +  } while (!capacityFile && errno == EINTR);
> > 
> > This pattern is un-familliar to me. Why do you put fopen (and later fclose)
> > in loops?
> 
> In Unix, long-running system calls might get aborted when the process
> receives a signal. In that case, the call fails and errno is set to EINTR.
> You typically don't see this in Linux, because glibc automatically restarts
> the system call in this case. I looked through Android's bionic libc, but
> couldn't find code for restarting system calls; so I added those loops to
> try as long as EINTR is returned (it's fairly boilerplate code). While the
> stdio functions are not system calls themselves, they are still affected,
> because they rely on system calls internally.

I would be inclined to use open/read/close though so that you can use ScopedClose.

I think that ScopedClose probably also needs to be changed to deal with EINTR. I'll file a bugfor that.

There is a macro in <unistd.h> called TEMP_FAILURE_RETRY which does the loop/errno check.

Comment 8

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Thanks Dave, I'll have a look.

I didn't know TEMP_FAILURE_RETRY was available in bionic. I always thought this is a GNU extension.

Comment 9

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: Set remaining time to 0 if battery level is 1 [v2]

This is an update to the previous patch. The major changes are the use of TEMP_FAILURE_RETRY and ScopedClose. Because of the latter, the file I/O has been rewritten to use file descriptors, and all gotos are gone.

Comment 10

[Mounir Lamouri (:mounir)]

Comment on attachment 689201 [details] [diff] [review]
Set remaining time to 0 if battery level is 1 [v2]

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

r=me with the comments applied.

::: hal/gonk/GonkHal.cpp
@@ +383,5 @@
>    static const int BATTERY_NOT_CHARGING = 0;
>    static const int BATTERY_CHARGING_USB = 1;
>    static const int BATTERY_CHARGING_AC  = 2;
>  
> +  *aChargingTime = dom::battery::kUnknownRemainingTime;

Don't pass |aChargingTime| to this method if you don't actually use it. It might give the wrong impression to the caller.

@@ +469,5 @@
> +    aBatteryInfo->charging() = true;
> +    if (aBatteryInfo->level() < 1.0) {
> +      aBatteryInfo->remainingTime() = dom::battery::kUnknownRemainingTime;
> +    } else {
> +      aBatteryInfo->remainingTime() = dom::battery::kDefaultRemainingTime;

I think this entire block should be something like:

int charging;

if (GetCurrentBatteryCharging(&charging)) {
  aBatteryInfo->charging() = charging;
} else {
  aBatteryInfo->charging() = true;
}

aBatteryInfo->remainingTime = dom::battery::kUnknownRemainingTime;

if (aBatteryInfo->charging() && aBatteryInfo->level == 1.0) {
  aBatteryInfo->remainingTime = dom::battery::kDefaultRemainingTime;
}

(feel free to use a ternary operator)

Comment 11

[Dave Hylands [:dhylands]]

Comment on attachment 689201 [details] [diff] [review]
Set remaining time to 0 if battery level is 1 [v2]

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

::: hal/gonk/GonkHal.cpp
@@ +352,5 @@
>        NewRunnableFunction(UnregisterBatteryObserverIOThread));
>  }
>  
> +static bool
> +GetCurrentBatteryCapacity(double* aCapacity)

This function name seems to be a poor choice (the /sys file name seems to be poor as well).

To me capacity implies how much the battery is capable of holding. This seems to be returning the current charge or level of the battery.

So I'd be inclined to call it GetCurrentBatteryLevel (this matches the field names in the BateryInformation structure).

@@ +374,5 @@
> +
> +  errno = 0;
> +  *aCapacity = strtod(capacityString, NULL);
> +  return !errno;
> +}

Since the bulk of the above function actually also appears below, I would prefer to factor this into a helper function, Something like the two functions here:
https://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/AutoMounter.cpp#99
but you'll want another version which parses a double.

The versions in AutoMounter are missing the TEMP_FAILURE_RETRY.

In fact, we should probably make these available somewhere in Gonk since they occur so frequently. we should factor these out into a common location. I created bug 819016 to do that, so I would probably just do the first level of factoring here and we can probably make your factored routines be the basis for the common routines in bug 819016

@@ +440,5 @@
> +      return false;
> +    }
> +
> +    *aCharging = !memcmp(chargingSrcString, "Charging", len) ||
> +                 !memcmp(chargingSrcString, "Full", len);

The strings read will contain a trailing newline so these comparisons won't work properly.

i.e. if you do: 

adb shell
cat /sys/class/power_supply/battery/status > /data/local/batt
exit
adb pull /data/local/batt
hd batt

then you'll see something like:

00000000  46 75 6c 6c 0a                                    |Full.|
00000005

Note: If you try to combine the adb shell and cat then you'll get a CR inserted before the NL

@@ +466,5 @@
> +    aBatteryInfo->charging() = charging;
> +    aBatteryInfo->remainingTime() = chargingTime;
> +  } else {
> +    aBatteryInfo->charging() = true;
> +    if (aBatteryInfo->level() < 1.0) {

Since this is floating point, shouldn't the comparison have some small epsilon for the comparison? i.e. 0.99999 should probably be considered the same as 1.0

Picking the right epsilon will probably be tricky (perhaps taking into consideration how many decimal places we show to the user).

Comment 12

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Thanks for the reviews!

(In reply to Dave Hylands [:dhylands] from comment #11)
> Comment on attachment 689201 [details] [diff] [review]
> Set remaining time to 0 if battery level is 1 [v2]
> 
> Review of attachment 689201 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: hal/gonk/GonkHal.cpp
> @@ +352,5 @@
> >        NewRunnableFunction(UnregisterBatteryObserverIOThread));
> >  }
> >  
> > +static bool
> > +GetCurrentBatteryCapacity(double* aCapacity)
> 
> This function name seems to be a poor choice (the /sys file name seems to be
> poor as well).
> 
> To me capacity implies how much the battery is capable of holding. This
> seems to be returning the current charge or level of the battery.
> 
> So I'd be inclined to call it GetCurrentBatteryLevel (this matches the field
> names in the BateryInformation structure).

The funny thing is that I first  named this function GetCurrentBatteryLevel, but then I thought that the use of 'capacity' on the original code might be for a reason. I'll change the naming to 'level'. :)

> @@ +374,5 @@
> > +
> > +  errno = 0;
> > +  *aCapacity = strtod(capacityString, NULL);
> > +  return !errno;
> > +}
> 
> Since the bulk of the above function actually also appears below, I would
> prefer to factor this into a helper function, Something like the two
> functions here:
> https://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/AutoMounter.
> cpp#99
> but you'll want another version which parses a double.
> 
> The versions in AutoMounter are missing the TEMP_FAILURE_RETRY.
> 
> In fact, we should probably make these available somewhere in Gonk since
> they occur so frequently. we should factor these out into a common location.
> I created bug 819016 to do that, so I would probably just do the first level
> of factoring here and we can probably make your factored routines be the
> basis for the common routines in bug 819016

I'll put some helpers into this file, and we can add generic variants later. I don't want to add even more changes to this patch.

> @@ +440,5 @@
> > +      return false;
> > +    }
> > +
> > +    *aCharging = !memcmp(chargingSrcString, "Charging", len) ||
> > +                 !memcmp(chargingSrcString, "Full", len);
> 
> The strings read will contain a trailing newline so these comparisons won't
> work properly.

I didn't notice that during testing. Thank you.

> 
> i.e. if you do: 
> 
> adb shell
> cat /sys/class/power_supply/battery/status > /data/local/batt
> exit
> adb pull /data/local/batt
> hd batt
> 
> then you'll see something like:
> 
> 00000000  46 75 6c 6c 0a                                    |Full.|
> 00000005
> 
> Note: If you try to combine the adb shell and cat then you'll get a CR
> inserted before the NL
> 
> @@ +466,5 @@
> > +    aBatteryInfo->charging() = charging;
> > +    aBatteryInfo->remainingTime() = chargingTime;
> > +  } else {
> > +    aBatteryInfo->charging() = true;
> > +    if (aBatteryInfo->level() < 1.0) {
> 
> Since this is floating point, shouldn't the comparison have some small
> epsilon for the comparison? i.e. 0.99999 should probably be considered the
> same as 1.0
> 
> Picking the right epsilon will probably be tricky (perhaps taking into
> consideration how many decimal places we show to the user).

I had a look into the encoding of floating-point numbers. Representing 1.0 seems possible. If we use an epsilon here, I'd have to use it in the upper layers as well. I was more worried about the division of (capacity = 100) by 100 really results in 1.0.

Comment 13

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: Set remaining time to 0 if battery level is 1 [v3]

V3 of the patch incorporates the reviews of previous versions.

- The remaining time is now separate from the charge level.

- I choose to rename 'GetCurrentBatteryCapacity' to 'GetCurrentBatteryCharge' instead of 'GetCurrentBatteryLevel'. This way the percentage value 'charge' is clearly distinguished from the dimension-less 'level'. Using type int for 'charge' makes the code a bit simpler.

- I also adapted the ReadSysFile functions, which simplifies the code a lot. Dave, I think there is a bug in ReadSysFile for strings. When you read 0 bytes, you access the byte before the string array when testing for '\n'. It's fixed in my copy.

- I left the floating-point comparison as it was. See my previous comments

Comment 14

[Dave Hylands [:dhylands]]

Comment on attachment 692284 [details] [diff] [review]
Set remaining time to 0 if battery level is 1 [v3]

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

::: hal/gonk/GonkHal.cpp
@@ +385,5 @@
> +  char valBuf[20];
> +  if (!ReadSysFile(aFilename, valBuf, sizeof(valBuf))) {
> +    return false;
> +  }
> +  return sscanf(valBuf, "%d", aVal) != 1;

Shouldn't this be == 1 ?

@@ +400,5 @@
> +    HAL_LOG(("charge level containes unknown value: %d", *aCharge));
> +  }
> +  #endif
> +
> +  return (*aCharge >= 0) && (*aCharge <= 100);

&& success

@@ +470,5 @@
> +  if (aBatteryInfo->charging() && (aBatteryInfo->level() < 1.0)) {
> +    aBatteryInfo->remainingTime() = dom::battery::kUnknownRemainingTime;
> +  } else {
> +    aBatteryInfo->remainingTime() = dom::battery::kDefaultRemainingTime;
> +  }

The logic of the above doesn't seem right to me. 

I thought the intent was:

if charging && (battery-level >= 1.0)
   return NoTimeRemaining

The logic as coded will return NoTimeRemaining if it's not charging, which feels wrong.

Comment 15

[Dave Hylands [:dhylands]]

(In reply to Dave Hylands [:dhylands] from comment #14)
> @@ +470,5 @@
> > +  if (aBatteryInfo->charging() && (aBatteryInfo->level() < 1.0)) {
> > +    aBatteryInfo->remainingTime() = dom::battery::kUnknownRemainingTime;
> > +  } else {
> > +    aBatteryInfo->remainingTime() = dom::battery::kDefaultRemainingTime;
> > +  }
> 
> The logic of the above doesn't seem right to me. 

I was confused. I have since discovered that remainingTime changes meaning based charging (so when charging is true, remainingTime = time until full charge, and when charging is false, remainingTime = time until battery dead)

So you can ignore that portion of my review comment and just fix the other 2 little issues.

Comment 16

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: Set remaining time to 0 if battery level is 1 [v4]

I made a similar assumption about the remaining time in my first attempt to fix the problem.

For v4 of the patch I fixed the invalid test of the scanf return value and the test for successfully reading a value from the sysfs file.

Comment 18

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: Set remaining time to 0 if battery level is 1 [v4]

Thanks for the reviews.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/52557de563d7

Comment 20

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/52557de563d7

Comment 22

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Comment on attachment 692974 [details] [diff] [review]
Set remaining time to 0 if battery level is 1 [v4]

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: 
Testing completed: 
Risk to taking this patch (and alternatives if risky): 
String or UUID changes made by this patch:

Comment 23

[Alex Keybl [:akeybl]]

Please provide rationale for landing this change in the v1.x timeframe, and mark with blocking-b2g:tef? if you believe it should block v1.0.

Comment 24

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Originally, I fixed this problem on a PandaBoard, where the incorrect battery status resulted in an incorrect battery icon and status string in the settings app and status bar. With the patch applied, Gonk returns the battery status according to Battery Status API standard.

In bug 811006, the problem was triggered on a Unagi phone, so I thought this patch might be worth considering for v1. I don't think it should be a blocker, though.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): -
User impact if declined: Possibly wrong battery status in UI
Testing completed: On my phones and PandaBoard
Risk to taking this patch (and alternatives if risky): Low
String or UUID changes made by this patch: -

Comment 25

[Lukas Blakk [:lsblakk] use ?needinfo]

Right now if we don't need it for 1.0.0 we're marking as tracking-b2g18 19+ and leaving the approval nom so this will be on our radar for the next v1.0-train.

Comment 26

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 692974 [details] [diff] [review]
Set remaining time to 0 if battery level is 1 [v4]

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

Please go ahead with uplift to the tip of mozilla-b2g18 branch for v1.0.1

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/d02aa2e38f40