Last Comment Bug 726612 - Not getting Battery API charging events when plugging device into mains power
: Not getting Battery API charging events when plugging device into mains power
Status: RESOLVED FIXED
[mentor=mounir][lang=c++]
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: ARM Gonk (Firefox OS)
: -- normal (vote)
: mozilla13
Assigned To: Ben Francis [:benfrancis]
:
: Andrew Overholt [:overholt]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-02-13 08:27 PST by Ben Francis [:benfrancis]
Modified: 2012-02-16 02:55 PST (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Charging source can be anything greater than zero to indicate charging, not just 1 (451 bytes, patch)
2012-02-14 05:18 PST, Ben Francis [:benfrancis]
mounir: review-
Details | Diff | Splinter Review
Charging when charging source is 1 or 2, not charging when 0. Log anything else when in DEBUG. (1.19 KB, patch)
2012-02-14 07:48 PST, Ben Francis [:benfrancis]
mounir: review+
Details | Diff | Splinter Review
Charging when charging source is 1 or 2, not charging when 0. Log anything else when in DEBUG. (1.21 KB, patch)
2012-02-15 09:16 PST, Ben Francis [:benfrancis]
mounir: checkin+
Details | Diff | Splinter Review

Description Ben Francis [:benfrancis] 2012-02-13 08:27:28 PST
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 2012-02-13 11:34:36 PST
Interesting!  The driver probably reports a state we don't understand.
Comment 2 Josh Matthews [:jdm] (on vacation until Dec 5) 2012-02-13 12:34:06 PST
Feel free to redirect mentorship to someone else, Mounir.
Comment 3 Mounir Lamouri (:mounir) 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
Comment 4 [:fabrice] Fabrice Desré 2012-02-13 16:10:14 PST
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] 2012-02-14 03:43:17 PST
On Mon, Feb 13, 2012 at 9:57 PM, <bugzilla-daemon@mozilla.org> wrote:
--- Comment #3 from Mounir Lamouri (:volkmar) (:mounir) <mounir@lamouri.fr> 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) 2012-02-14 03:46:30 PST
(In reply to Ben Francis from comment #5)
> On Mon, Feb 13, 2012 at 9:57 PM, <bugzilla-daemon@mozilla.org> wrote:
> --- Comment #3 from Mounir Lamouri (:volkmar) (:mounir) <mounir@lamouri.fr>
> 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) 2012-02-14 03:51:10 PST
Oups, forget that last comment...
Comment 8 Ben Francis [:benfrancis] 2012-02-14 05:09:31 PST
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] 2012-02-14 05:18:51 PST
Created attachment 596982 [details] [diff] [review]
Charging source can be anything greater than zero to indicate charging, not just 1
Comment 10 Mounir Lamouri (:mounir) 2012-02-14 05:26:01 PST
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 2012-02-14 07:09:40 PST
adb will talk TCP.  You can connect to the phone through wifi.  (If you have wifi working ...)
Comment 12 Ben Francis [:benfrancis] 2012-02-14 07:48:02 PST
Created attachment 597029 [details] [diff] [review]
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) 2012-02-14 09:46:17 PST
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) 2012-02-14 15:16:59 PST
(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] 2012-02-15 09:16:21 PST
Created attachment 597433 [details] [diff] [review]
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] 2012-02-16 02:55:53 PST
https://hg.mozilla.org/mozilla-central/rev/ebcae0510986

Note You need to log in before you can comment on or make changes to this bug.