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).

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).

mwu, if you are working on this, could you put the patches here?

I got this working on the b2g mozilla-central fork.

I might be interested to see the WIP patches if you can attach them here :)

Here are the commits I made for that. https://github.com/cgjones/mozilla-central/commit/68a34a3ab8f12dc3dc5d282a529f36e312c03894 https://github.com/cgjones/mozilla-central/commit/438aa35660c81ac50fc31daeaa245fc0444e8440 https://github.com/cgjones/mozilla-central/commit/698e867564558ee734b56f367538e81fdb9ee9c4

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?

(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.

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 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.

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.

(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).

(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.

(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.

(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.

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 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.

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?

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.

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 ;)

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 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 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 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.

(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.

(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.

(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.

ClearOnShutdown should work fine with null pointers.

(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()?

Leak what? It won't unref all refs. If we leak battery observers, it's a bug somewhere other than this code.

(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.

As long as the statics are the last refs, then yes they'll be freed. IIUC.

(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?

Yes, but ClearOnShutdown isn't run from the static dtor context.

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?

(In reply to Michael Wu [:mwu] from comment #34) > https://hg.mozilla.org/integration/mozilla-inbound/rev/3e861cc550d4 https://hg.mozilla.org/mozilla-central/rev/3e861cc550d4

(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 ..

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.