Last Comment Bug 696042 - Battery API: B2G (without Android APIs) backend
: Battery API: B2G (without Android APIs) backend
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla12
Assigned To: Michael Wu [:mwu]
:
Mentors:
Depends on: 678694 713149 758103
Blocks: 709468 712396 712399 694862
  Show dependency treegraph
 
Reported: 2011-10-20 05:57 PDT by Mounir Lamouri (:mounir)
Modified: 2012-05-24 02:25 PDT (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Standalone sysfs reading POC (4.13 KB, text/x-c++src)
2011-10-20 06:00 PDT, Mounir Lamouri (:mounir)
no flags Details
Standalone uevent POC (3.79 KB, text/x-c++src)
2011-10-20 06:01 PDT, Mounir Lamouri (:mounir)
no flags Details
Simple battery backend for gonk (10.70 KB, patch)
2011-11-30 11:44 PST, Michael Wu [:mwu]
mounir: feedback-
Details | Diff | Splinter Review
Simpler battery backend for gonk (8.87 KB, patch)
2011-12-15 15:07 PST, Michael Wu [:mwu]
cjones.bugs: review+
Details | Diff | Splinter Review

Description Mounir Lamouri (:mounir) 2011-10-20 05:57:39 PDT
Bug 696038 should make Battery API works on B2G but we want to strip down as much Android API/Java code as possible so we might want to write our own backend directly using uevents and sysfs (this is what the Android API backend does).
Comment 1 Mounir Lamouri (:mounir) 2011-10-20 06:00:57 PDT
Created attachment 568374 [details]
Standalone sysfs reading POC
Comment 2 Mounir Lamouri (:mounir) 2011-10-20 06:01:22 PDT
Created attachment 568375 [details]
Standalone uevent POC
Comment 3 Mounir Lamouri (:mounir) 2011-10-20 06:03:56 PDT
Please read bug 678694 comment 14 (and around) to understand why we can't use uevents to update battery status in all Linux platforms. Though, we should be able to use that on B2G and maybe Android (I don't know if there is some sort of security thing here).
Comment 4 Mounir Lamouri (:mounir) 2011-11-22 17:11:39 PST
mwu, if you are working on this, could you put the patches here?
Comment 5 Michael Wu [:mwu] 2011-11-22 17:56:45 PST
I got this working on the b2g mozilla-central fork.
Comment 6 Mounir Lamouri (:mounir) 2011-11-22 18:01:34 PST
I might be interested to see the WIP patches if you can attach them here :)
Comment 8 Mounir Lamouri (:mounir) 2011-11-23 12:10:14 PST
Michael, it seems that you are adding the code in widget/ but I thought this code should live in hal/. Is there any reason why it's in widget in your patches? or am I misunderstanding the purpose of hal?
Comment 9 Michael Wu [:mwu] 2011-11-23 20:15:27 PST
(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #8)
> Michael, it seems that you are adding the code in widget/ but I thought this
> code should live in hal/. Is there any reason why it's in widget in your
> patches? or am I misunderstanding the purpose of hal?

It would complicate the code at the moment to not put it in widget.
Comment 10 Michael Wu [:mwu] 2011-11-30 11:44:02 PST
Created attachment 578031 [details] [diff] [review]
Simple battery backend for gonk

This is actually kinda crappy since it's easiest to put the battery listener in nsAppShell.cpp instead of hal. A better alternative might be to use observers to look at uevents, but I have no idea if anyone else will want to look at uevents. (and if not, that would be overkill)
Comment 11 Mounir Lamouri (:mounir) 2011-11-30 16:03:32 PST
Comment on attachment 578031 [details] [diff] [review]
Simple battery backend for gonk

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

Can't we implement something similar to what my attached code does? That means a helper class to read sysfs and a uevent observer/listener mechanism. Both of those being used by a battery manager class (a la UPower).
I believe sysfs might be used by other low-level code as uevents and I don't think it is adding that much work (it doesn't have to be too much flexible for the moment I guess).

In addition, ideally, we should be able to use this implementation for Linux when built without DBus (why not when UPower isn't present too...) so we shouldn't make this too much b2g/Gonk specific. Technically, uevents and sysfs are Linux stuff (as in the kernel).

I would be glad to work on this implementation but that would require setuping a B2G dev phone and the overhead seems a bit excessive for the moment.
Comment 12 Michael Wu [:mwu] 2011-11-30 16:38:03 PST
I didn't check your implementation before since the mime type didn't let me view it in the browser. However, from what I see, it involves significantly more code to achieve the same thing. I'm willing to make a more generic uevent thing, but without a second (potential) user listening to uevents, I'm not willing to add additional extra complexity.
Comment 13 Mounir Lamouri (:mounir) 2011-11-30 16:46:22 PST
(In reply to Michael Wu [:mwu] from comment #12)
> I didn't check your implementation before since the mime type didn't let me
> view it in the browser.

Bugzilla allows you to click on "Source" to see it (assuming it's not done by an extension).

> However, from what I see, it involves significantly
> more code to achieve the same thing. I'm willing to make a more generic
> uevent thing, but without a second (potential) user listening to uevents,
> I'm not willing to add additional extra complexity.

I will not fight to get a uevent listener/observer system even if I don't think it is that hard to implement. However, I think it's important to have sysfs/uevent battery backend not dependent on B2G or Gonk widget (IOW, working on non-B2G devices).
Comment 14 Michael Wu [:mwu] 2011-11-30 19:20:24 PST
(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #13)
> (In reply to Michael Wu [:mwu] from comment #12)
> > I didn't check your implementation before since the mime type didn't let me
> > view it in the browser.
> 
> Bugzilla allows you to click on "Source" to see it (assuming it's not done
> by an extension).
> 

Hrmm... I can't find it..

> > However, from what I see, it involves significantly
> > more code to achieve the same thing. I'm willing to make a more generic
> > uevent thing, but without a second (potential) user listening to uevents,
> > I'm not willing to add additional extra complexity.
> 
> I will not fight to get a uevent listener/observer system even if I don't
> think it is that hard to implement. However, I think it's important to have
> sysfs/uevent battery backend not dependent on B2G or Gonk widget (IOW,
> working on non-B2G devices).

Well, the Android backend also has a similar dependency. Not sure why a sysfs/uevent backend that doesn't depend on Gonk would be useful on other Linux platforms, since they should just use upower.
Comment 15 Mounir Lamouri (:mounir) 2011-12-01 01:43:38 PST
(In reply to Michael Wu [:mwu] from comment #14)
> (In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #13)
> > (In reply to Michael Wu [:mwu] from comment #12)
> > > I didn't check your implementation before since the mime type didn't let me
> > > view it in the browser.
> > 
> > Bugzilla allows you to click on "Source" to see it (assuming it's not done
> > by an extension).
> > 
> 
> Hrmm... I can't find it..

I checked with another browser and it didn't appear. Must have been added by Bugzilla Tweaks then.

> > > However, from what I see, it involves significantly
> > > more code to achieve the same thing. I'm willing to make a more generic
> > > uevent thing, but without a second (potential) user listening to uevents,
> > > I'm not willing to add additional extra complexity.
> > 
> > I will not fight to get a uevent listener/observer system even if I don't
> > think it is that hard to implement. However, I think it's important to have
> > sysfs/uevent battery backend not dependent on B2G or Gonk widget (IOW,
> > working on non-B2G devices).
> 
> Well, the Android backend also has a similar dependency. Not sure why a
> sysfs/uevent backend that doesn't depend on Gonk would be useful on other
> Linux platforms, since they should just use upower.

UPower isn't present in all GNU/Linux machines. The project is quite recent (<6 months I believe). Before, it was named DeviceKit-Power. In addition, some systems might not have UPower nor DeviceKit-Power. For example my desktop doesn't have upower installed and I did install it for my laptop especially to work on the Gecko implementation. It's also possible that KDE-based systems don't have those services too [1]. Finally, the upower backend is enabled only if we enable the dbus build config.
Given that there is no technical reason to prevent sysfs/uevent to be working on other Linux platforms, we should do our best to allow that and it doesn't seem highly complex.

[1] All recent Gnome-based desktop should have it, it's used by the Gnome battery manager.
Comment 16 Michael Wu [:mwu] 2011-12-01 06:48:30 PST
(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #15)
> > Well, the Android backend also has a similar dependency. Not sure why a
> > sysfs/uevent backend that doesn't depend on Gonk would be useful on other
> > Linux platforms, since they should just use upower.
> 
> UPower isn't present in all GNU/Linux machines. The project is quite recent
> (<6 months I believe). Before, it was named DeviceKit-Power. In addition,
> some systems might not have UPower nor DeviceKit-Power. For example my
> desktop doesn't have upower installed and I did install it for my laptop
> especially to work on the Gecko implementation. It's also possible that
> KDE-based systems don't have those services too [1]. Finally, the upower
> backend is enabled only if we enable the dbus build config.
> Given that there is no technical reason to prevent sysfs/uevent to be
> working on other Linux platforms, we should do our best to allow that and it
> doesn't seem highly complex.
> 

Using sysfs/uevent directly is a bad idea on normal Linux systems. At the very least, libudev should be used there instead, perhaps over dbus. But really, I don't see what's wrong with using upower, and I also don't see any real problems that are solved by doing upower's job ourselves. Recent versions of KDE also use upower so it's not some sort of gnome only thing.
Comment 17 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-12-02 12:30:24 PST
If we want to maintain the smallest amount of code, we should have a single backend for linux that just uses sysfs/uevents.  It's a shame that upower depends on glib and dbus, since we have absolutely no interest in either.  There's really not that much non-boilerplate code in upower, so I would be fine with keeping our own impl derived from upower.  Maybe we could interest the upower devs in having a core lib that we could share ...
Comment 18 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-12-05 19:56:54 PST
Comment on attachment 578031 [details] [diff] [review]
Simple battery backend for gonk

This patch has some problems
 - polling uevents on main thread messes with their priority wrt other events
 - SO_RCVBUFFORCE forces us to keep running b2g as root
 - not using SO_RCVBUFFORCE means ... I don't know; what happens when a uevent socket buffer fills?
 - using some event compression but not fully flushing the input buffer

Also the gonk glue code is gross.

I think it would actually be simpler to just spawn a new pthread from within hal to poll the uevent socket.  That thread can fire events back to the main thread as they arrive.  I don't see any use in dispatching these through the observer service, would just be overhead.  That setup would allow us to do real event compression too, and per event type.

Let's chat about how hard this would be implement, instead of the approach in this patch.
Comment 19 Philipp von Weitershausen [:philikon] 2011-12-12 15:22:23 PST
This patch is one of the big pieces that account for the divergence between mozilla-central and cjones's fork on GitHub that's used for B2G. Could we perhaps, in the interest of eliminating the fork, land it as is and deal with the issues outlined above in a follow-up bug?
Comment 20 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-12-12 18:58:03 PST
I'm not comfortable landing this patch on hg as is.

mwu, when will you be able to address the comments above?  The amount of code that needs to change should be small.
Comment 21 Mounir Lamouri (:mounir) 2011-12-13 03:20:16 PST
I agree: we shouldn't land something that we know isn't right. And in addition of Chris comments, there are incorrect implementation of the Battery API (like Notify called in EnableNotifications).

As I said to mwu, I will be willing to work on this if he has other priorities. That would be a good reason to finally install b2g on my dev phone ;)
Comment 22 Michael Wu [:mwu] 2011-12-15 15:07:26 PST
Created attachment 582119 [details] [diff] [review]
Simpler battery backend for gonk

This one is based on libhardware_legacy. Due to the complexity of trying to split out patches, I didn't bother and just patched on top of what's in the git repo. I think we should just land GonkHal.cpp all at once this is ready.
Comment 23 Mounir Lamouri (:mounir) 2011-12-15 23:53:58 PST
Comment on attachment 582119 [details] [diff] [review]
Simpler battery backend for gonk

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

You do not respect the coding style requiring:
if (cond) {
  do;
}

Also, it's quite disturbing to have some part of the code missing in that patch. For example GetCurrentBatteryInformation isn't implemented here because it was implemented in the previous patch...

::: hal/gonk/GonkHal.cpp
@@ +126,5 @@
> +  if (!sWatcher)
> +    sWatcher = new UEventWatcher();
> +  NS_ADDREF(sWatcher);
> +
> +  sWatcher->mRunning = true;

Start() method would have been nicer.

@@ +129,5 @@
> +
> +  sWatcher->mRunning = true;
> +  nsresult rv = NS_NewThread(&sWatcherThread, sWatcher);
> +  if (NS_FAILED(rv))
> +    NS_WARNING("Failed to get new thread for uevent watching");

That could be:
if (NS_FAILED(NS_NewThread(...))) {
  NS_WARNING(...);
}

@@ +135,5 @@
>  
>  void
>  DisableBatteryNotifications()
>  {
> +  sWatcher->mRunning = false;

Stop() method would have been nicer.
Comment 24 Mounir Lamouri (:mounir) 2011-12-15 23:59:33 PST
Comment on attachment 578031 [details] [diff] [review]
Simple battery backend for gonk

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

::: hal/gonk/GonkHal.cpp
@@ +70,5 @@
> +
> +void
> +GetCurrentBatteryInformation(hal::BatteryInformation* aBatteryInfo)
> +{
> +  FILE *capacityFile = fopen("/sys/class/power_supply/battery/capacity", "r");

On my Linux laptop, the way to get the capacity is to check charge_now and charge_full. I'm concerned with capacity being something like charge_now / charge_full_design. Could you double-check that?

@@ +76,5 @@
> +  if (capacityFile)
> +    fscanf(capacityFile, "%lf", &capacity);
> +  fclose(capacityFile);
> +
> +  FILE *chargingFile = fopen("/sys/class/power_supply/battery/charging_source", "r");

This is does not exist on my Linux laptop too. There is status instead. Does status exist on the phone?

@@ +83,5 @@
> +    fscanf(chargingFile, "%d", &chargingSrc);
> +  fclose(chargingFile);
> +
> +  aBatteryInfo->level() = capacity / 100;
> +  aBatteryInfo->charging() = chargingSrc == 1;

I guess there is no way to have a phone running with no battery but plugged?

@@ +84,5 @@
> +  fclose(chargingFile);
> +
> +  aBatteryInfo->level() = capacity / 100;
> +  aBatteryInfo->charging() = chargingSrc == 1;
> +  aBatteryInfo->remainingTime() = dom::battery::kUnknownRemainingTime;

Please, implement that. Otherwise, this is going to throw some assertions.
Comment 25 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-12-17 00:59:59 PST
Comment on attachment 582119 [details] [diff] [review]
Simpler battery backend for gonk

>diff --git a/hal/gonk/GonkHal.cpp b/hal/gonk/GonkHal.cpp

>+namespace {
>+
>+class BatteryUpdater : public nsRunnable {
>+public:
>+  NS_IMETHOD Run()
>+  {
>+    hal::BatteryInformation info;
>+    hal_impl::GetCurrentBatteryInformation(&info);
>+    hal::NotifyBatteryChange(info);

We're going to want to filter these change notifications.  There's
code in the android backend to do this, but it should be centralized.
Let's file a followup to centralize it.

We should also centralize estimating remaining battery time when the
system doesn't provide it.  That code can kick in when remainingTime
is some sentinel value.  That should be another followup, plz.  We'll
coordinate with mounir.

>+static bool sUEventInitialized = false;
>+static UEventWatcher *sWatcher = NULL;
>+static nsIThread *sWatcherThread = NULL;

>+  if (!sWatcher)
>+    sWatcher = new UEventWatcher();
>+  NS_ADDREF(sWatcher);
>+

We should use jlebar's new ClearOnShutdown for sWatcher and sWatcherThread.

>diff --git a/toolkit/library/Makefile.in b/toolkit/library/Makefile.in
>index 9e59b1f..b2cdbc1 100644
>--- a/toolkit/library/Makefile.in
>+++ b/toolkit/library/Makefile.in
>@@ -431,6 +431,7 @@ ifeq (gonk,$(MOZ_WIDGET_TOOLKIT))
> OS_LIBS += \
>   -lui \
>   -lmedia \
>+  -lhardware_legacy \
>   $(NULL)
> endif
> 

>diff --git a/widget/src/gonk/nsAppShell.h b/widget/src/gonk/nsAppShell.h

>+namespace mozilla {
>+bool ProcessNextEvent();
>+void NotifyEvent();
>+}

We don't need these for this patch, and they scare me a bit.  Remove
plz.

r=me with that.
Comment 26 Michael Wu [:mwu] 2011-12-20 09:41:18 PST
(In reply to Chris Jones [:cjones] [:warhammer] from comment #25)
> >diff --git a/widget/src/gonk/nsAppShell.h b/widget/src/gonk/nsAppShell.h
> 
> >+namespace mozilla {
> >+bool ProcessNextEvent();
> >+void NotifyEvent();
> >+}
> 
> We don't need these for this patch, and they scare me a bit.  Remove
> plz.
> 

This just backing out code I moved to GonkGlue.h, which is now removed again now that GonkHal doesn't need it. Probably not necessary to have in the header though.
Comment 27 Michael Wu [:mwu] 2011-12-20 10:41:44 PST
(In reply to Chris Jones [:cjones] [:warhammer] from comment #25)
> >+static bool sUEventInitialized = false;
> >+static UEventWatcher *sWatcher = NULL;
> >+static nsIThread *sWatcherThread = NULL;
> 
> >+  if (!sWatcher)
> >+    sWatcher = new UEventWatcher();
> >+  NS_ADDREF(sWatcher);
> >+
> 
> We should use jlebar's new ClearOnShutdown for sWatcher and sWatcherThread.
> 

That is pretty convenient. I think I just wanted a singleton to put this stuff in instead of some globals floating around, but this will be a bit better than manual refcounting.
Comment 28 Michael Wu [:mwu] 2011-12-20 10:47:51 PST
(In reply to Michael Wu [:mwu] from comment #27)
> (In reply to Chris Jones [:cjones] [:warhammer] from comment #25)
> > >+static bool sUEventInitialized = false;
> > >+static UEventWatcher *sWatcher = NULL;
> > >+static nsIThread *sWatcherThread = NULL;
> > 
> > >+  if (!sWatcher)
> > >+    sWatcher = new UEventWatcher();
> > >+  NS_ADDREF(sWatcher);
> > >+
> > 
> > We should use jlebar's new ClearOnShutdown for sWatcher and sWatcherThread.
> > 
> 
> That is pretty convenient. I think I just wanted a singleton to put this
> stuff in instead of some globals floating around, but this will be a bit
> better than manual refcounting.

Oh and the other thing - sWatcher and sWatcherThread aren't actually held during the life of the program. They're only kept while needed, so ClearOnShutdown isn't right here unless we do want to just keep these resources around forever. However, since that's the case, we might be able to use nsRefPtr and nsCOMPtr since they shouldn't be holding anything on shutdown.
Comment 29 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-12-20 11:09:24 PST
ClearOnShutdown should work fine with null pointers.
Comment 30 Michael Wu [:mwu] 2011-12-20 11:13:27 PST
(In reply to Chris Jones [:cjones] [:warhammer] from comment #29)
> ClearOnShutdown should work fine with null pointers.

So it would make sure we don't leak if we don't DisableBatteryNotifications()?
Comment 31 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-12-20 11:22:44 PST
Leak what?  It won't unref all refs.  If we leak battery observers, it's a bug somewhere other than this code.
Comment 32 Michael Wu [:mwu] 2011-12-20 11:28:22 PST
(In reply to Chris Jones [:cjones] [:warhammer] from comment #31)
> Leak what?  It won't unref all refs.  If we leak battery observers, it's a
> bug somewhere other than this code.

Leak sWatcher and sWatcherThread, in case DisableBatteryNotifications isn't called. Right now DisableBatteryNotifications is responsible for releasing sWatcher and sWatcherThread.
Comment 33 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-12-20 11:37:26 PST
As long as the statics are the last refs, then yes they'll be freed.  IIUC.
Comment 35 Justin Lebar (not reading bugmail) 2011-12-20 12:09:00 PST
(In reply to Chris Jones [:cjones] [:warhammer] from comment #33)
> As long as the statics are the last refs, then yes they'll be freed.  IIUC.

Aren't static destructors run after our leak-checking code runs?
Comment 36 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-12-20 12:15:08 PST
Yes, but ClearOnShutdown isn't run from the static dtor context.
Comment 37 Mounir Lamouri (:mounir) 2011-12-20 13:23:28 PST
You didn't fix the coding style issues (with the if's).
Also I forgot to point that but this backend actually has a major issue: it is not sync. Battery API backends should be able to support this use case:
if (navigator.mozBattery.level >= 0.5)

So when EnableNotifications is created, we should do a sync operation that get the battery state. Could you open a follow-up to fix that?
Comment 39 Landry Breuil (:gaston) 2012-02-07 09:48:38 PST
(In reply to Chris Jones [:cjones] [:warhammer] from comment #17)
> If we want to maintain the smallest amount of code, we should have a single
> backend for linux that just uses sysfs/uevents.  It's a shame that upower
> depends on glib and dbus, since we have absolutely no interest in either. 

Coming a bit late to the party, but upower provides a stable os-independant api to access battery levels and has been ported to FreeBSD and OpenBSD (last part by yours truly), whereas sysfs/uevents is linux-only, and a moving target ..
Comment 40 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-02-07 16:23:39 PST
Thank you, but if you read on a little further ...

> There's really not that much non-boilerplate code in upower, so I would be
> fine with keeping our own impl derived from upower.  Maybe we could interest
> the upower devs in having a core lib that we could share ...

If you're interested in creating such a library, we would more than happy to use it.

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