726612 - Not getting Battery API charging events when plugging device into mains power

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Ben Francis [:benfrancis]]

STR:

1. Plug Samsung Galaxy SII flashed with latest B2G & Gaia into wall charger (not USB charger)

Expected: Charging indicator changes to charging state

Actual: No change

Works fine when charging via USB.

See https://github.com/andreasgal/gaia/issues/393 for original bug report.

Comment 1

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Interesting!  The driver probably reports a state we don't understand.

Comment 2

[Josh Matthews [:jdm]]

Feel free to redirect mentorship to someone else, Mounir.

Comment 3

[Mounir Lamouri (:mounir)]

Ben, by any chance, can you tell me what's in this file when your phone is plugged into a wall charger:
/sys/class/power_supply/battery/charging_source

Comment 4

[[:fabrice] Fabrice Desré]

Also, on an akami device, we don't have battery/charging_source at all.

ls /sys/class/power_supply/battery :
uevent
subsystem
device
status
health
present
technology
voltage_max_design
voltage_min_design
voltage_now
capacity
type
power

Comment 5

[Ben Francis [:benfrancis]]

On Mon, Feb 13, 2012 at 9:57 PM, <[e-mail]> wrote:
--- Comment #3 from Mounir Lamouri (:volkmar) (:mounir) <[e-mail]> 2012-02-13 13:57:07 PST ---
> Ben, by any chance, can you tell me what's in this file when your phone is
> plugged into a wall charger:
> /sys/class/power_supply/battery/charging_source

OK, dumb question. How can I look inside that file while the device is plugged into the mains, and not USB?

Comment 6

[Mounir Lamouri (:mounir)]

(In reply to Ben Francis from comment #5)
> On Mon, Feb 13, 2012 at 9:57 PM, <[e-mail]> wrote:
> --- Comment #3 from Mounir Lamouri (:volkmar) (:mounir) <[e-mail]>
> 2012-02-13 13:57:07 PST ---
> > Ben, by any chance, can you tell me what's in this file when your phone is
> > plugged into a wall charger:
> > /sys/class/power_supply/battery/charging_source
> 
> OK, dumb question. How can I look inside that file while the device is
> plugged into the mains, and not USB?

Oh indeed, there is no way to do that on B2G... Can you try to plug it to your wall and to USB and see if the bug happens?

Comment 7

[Mounir Lamouri (:mounir)]

Oups, forget that last comment...

Comment 8

[Ben Francis [:benfrancis]]

OK, I added a line of C++ to log the contents of charging_source (go me!) and it appears when you plug the device into the mains, you get 2 instead of 1 which is what you get when charging via USB, the event only fires for 1.

Comment 9

[Ben Francis [:benfrancis]]

Attachment: Charging source can be anything greater than zero to indicate charging, not just 1

Comment 10

[Mounir Lamouri (:mounir)]

Comment on attachment 596982 [details] [diff] [review]
Charging source can be anything greater than zero to indicate charging, not just 1

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

I don't like that solution. I would prefer if you could create three constants:
static const int BATTERY_NOT_CHARGING = 0;
static const int BATTERY_CHARGING_USB = 1;
static const int BATTERY_CHARGING_AC  = 2;

Then, you could do:
  aBatteryInfo->charging() = (chargingSrc == BATTERY_CHARGING_USB || chargingSrc == BATTERY_CHARGING_AC);

And you could add a check when in DEBUG that would verify if chargingSrc is equal to one of those three values. If not, it will print the value in the logs.

Comment 11

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

adb will talk TCP.  You can connect to the phone through wifi.  (If you have wifi working ...)

Comment 12

[Ben Francis [:benfrancis]]

Attachment: Charging when charging source is 1 or 2, not charging when 0. Log anything else when in DEBUG.

Changes to address Mounir's review. Feel free to re-write this if there are further changes you'd like, C++ is not my lingua franca :)

Comment 13

[Mounir Lamouri (:mounir)]

Comment on attachment 597029 [details] [diff] [review]
Charging when charging source is 1 or 2, not charging when 0. Log anything else when in DEBUG.

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

r=me with the nits fixed

::: hal/gonk/GonkHal.cpp
@@ +284,5 @@
>  
> +  #ifdef DEBUG
> +  if (!(chargingSrc == BATTERY_NOT_CHARGING ||
> +    chargingSrc == BATTERY_CHARGING_USB || 
> +    chargingSrc == BATTERY_CHARGING_AC)) {

I would prefer if (foo != a && foo != b [...]) {
But you can disagree.

Though, you should fix the indentation to:
if (something ||
    somethingElse) {

@@ +293,3 @@
>    aBatteryInfo->level() = capacity / 100;
> +  aBatteryInfo->charging() = (chargingSrc == BATTERY_CHARGING_USB ||
> +    chargingSrc == BATTERY_CHARGING_AC);

ditto for the indentation:
aBatteryInfo->charging() = (chargingSrc == BATTERY_CHARGING_USB ||
	                    chargingSrc == BATTERY_CHARGING_AC);

Comment 14

[Mounir Lamouri (:mounir)]

(In reply to Fabrice Desré [:fabrice] from comment #4)
> Also, on an akami device, we don't have battery/charging_source at all.

Could you open a bug for that and CC me?

Comment 15

[Ben Francis [:benfrancis]]

Attachment: Charging when charging source is 1 or 2, not charging when 0. Log anything else when in DEBUG.

Fixed indentation and boolean logic readability.

Comment 16

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/ebcae0510986