Closed Bug 678694 Opened 11 years ago Closed 11 years ago

Battery API

Categories

(Core :: General, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla10
Tracking Status
firefox10 - ---

People

(Reporter: gal, Assigned: mounir)

References

(Blocks 2 open bugs, )

Details

(Keywords: dev-doc-complete, Whiteboard: [secr:curtisk])

Attachments

(7 files, 13 obsolete files)

6.96 KB, patch
sicking
: review+
Details | Diff | Splinter Review
2.79 KB, patch
sicking
: review+
Details | Diff | Splinter Review
9.60 KB, patch
sicking
: review+
Details | Diff | Splinter Review
16.90 KB, patch
Details | Diff | Splinter Review
9.85 KB, patch
sicking
: review+
Details | Diff | Splinter Review
6.23 KB, patch
cjones
: superreview+
Details | Diff | Splinter Review
13.55 KB, patch
cjones
: review+
Details | Diff | Splinter Review
All the information required to implement the usual device status bar should be exposed. Available cell network, wireless network status, etc.
Blocks: webapi
FYI a new snapshot of the Battery Status Events has been released:

  http://www.w3.org/TR/2011/WD-battery-status-20110915/

The approach has changed a fair bit based on feedback, and I hope that this would be reasonably stable at this point. Feedback would be very much welcome!
Attached patch Draft patch for a Battery API (obsolete) — Splinter Review
Some choices in this implementation are different from the DAP Battery API specifications. I believe we will do some formal propositions later to the appropriate WC/TF.
Renaming this bug Battery API given that the network stuff my go with the Network API in bug 677166.
Summary: Device Status API → Battery API
Version: unspecified → Trunk
Attached patch Draft patch (obsolete) — Splinter Review
This patch has some changes and matches what the proposal in the wiki.
Attachment #561991 - Attachment is obsolete: true
Comment on attachment 562395 [details] [diff] [review]
Draft patch

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

::: dom/src/webapi/mozBatteryManager.cpp
@@ +94,5 @@
> +    if (AndroidBridge::Bridge()) {
> +      return NS_ERROR_FAILURE;
> +    }
> +
> +    AndroidBridge::Bridge()->RegisterBatteryManager(&mIsPlugged, &mLevel, hasBattery);

There is a typo bug here, you probably want to do the invert, otherwise the battery manager fails to initialize if call from the chrome process.

if (AndroidBridge::Bridge()) {
  AndroidBridge::Bridge()->RegisterBatteryManager(&mIsPlugged, &mLevel, hasBattery);
} else {
  return NS_ERROR_FAILURE;
}

@@ +174,5 @@
> +  if (mLevel <= sCriticalTreshold) {
> +    return BatteryStatus_Critical;
> +  }
> +
> +  // TODO: spces should change for <= !

spces => specs?
(In reply to Vivien Nicolas (:vingtetun) from comment #5)
> Comment on attachment 562395 [details] [diff] [review] [diff] [details] [review]
> Draft patch
> 
> Review of attachment 562395 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/src/webapi/mozBatteryManager.cpp
> @@ +94,5 @@
> > +    if (AndroidBridge::Bridge()) {
> > +      return NS_ERROR_FAILURE;
> > +    }
> > +
> > +    AndroidBridge::Bridge()->RegisterBatteryManager(&mIsPlugged, &mLevel, hasBattery);
> 
> There is a typo bug here, you probably want to do the invert, otherwise the
> battery manager fails to initialize if call from the chrome process.
> 
> if (AndroidBridge::Bridge()) {
>   AndroidBridge::Bridge()->RegisterBatteryManager(&mIsPlugged, &mLevel,
> hasBattery);
> } else {
>   return NS_ERROR_FAILURE;
> }

It's a typo, I should have write "if (!AndroidBridge::Bridge())".
Will fix that.

> @@ +174,5 @@
> > +  if (mLevel <= sCriticalTreshold) {
> > +    return BatteryStatus_Critical;
> > +  }
> > +
> > +  // TODO: spces should change for <= !
> 
> spces => specs?

Seriously? :)
Attached patch Draft patch (obsolete) — Splinter Review
With the fixes for Vivien and a few other minor changes.
Attachment #562395 - Attachment is obsolete: true
Depends on: 690670
Attached patch Draft patch (obsolete) — Splinter Review
This new draft patch is now based on bug 690670.
Assignee: nobody → mounir
Attachment #562796 - Attachment is obsolete: true
Status: NEW → ASSIGNED
For the battery backends, the plan is going to be the following:
- Android: using the Battery API provided by the SDK;
- GNU/Linux: using upower [1] with a fallback to a plain sysfs implementation if upower is disabled on compile time (should we have a run-time fallback? maybe later);
- B2G: for the moment, stick with a sysfs implementation.
- MacOS X and Windows: will come later. The goal is to have more than one backend implementation to make sure the API on top of them are generic enough. Given that I have a Mac, I could try to implement the MacOS X backend. I have no Windows system out of a VM so I will let someone else working on this.

upower is simply a layer on top of sysfs using dbus to inform of battery status (and changes). sysfs implementation will require listening to uevents to update the battery status.

[1] http://upower.freedesktop.org/
Attached file Standalone sysfs reading POC (obsolete) —
Simple standalone C/C++ file reading sysfs files to get battery information.
Attached file Standalone uevent POC (obsolete) —
Simple standalone C/C++ file using uevents to react to battery changes.

Next step is to hook those two things together in Gecko to have our uevent/sysfs backend implementation
Hmm, looks like this code doesn't react to battery charge changing or events are not sent very often. Will have to verify that tomorrow...
Nice work here Mounir. Did you figure out how to get charge events working?
(In reply to Andreas Gal :gal from comment #13)
> Nice work here Mounir. Did you figure out how to get charge events working?

Actually, I did some search and it seems that Android is getting some uevents for battery charge changing but it is not expected to happen for all systems. For what I understood, there are two types of uevents here, POWER_SUPPLY_CHANGED when the battery is plugged/unplugged and POWER_SUPPLY_UEVENT when the battery charge changes. It seems that to receive the latter, a loglevel has to be changed, see [1]. I'm not 100% sure this information is correct nor that I understood it correctly but what is certain is that we can't assume that uevents will occur regularly on all systems. Most Linux applications are regularly reading the sysfs (or ACPI...) information, for what I've seen.

I think we should still be able to use this implementation (sysfs/uevent) for B2G. Though, we will not be able to use it on Linux [2]. I did work on a upower implementation (PoC is done, I've to integrate that in Gecko now).

[1] https://groups.google.com/group/android-porting/browse_thread/thread/2d870f671bb68697/3f6a63bddb3bc5e7?#3f6a63bddb3bc5e7
[2] We could read the sysfs information regularly but that would be a bad idea because we would do something that upower is already doing and reduce the battery life...
Attached file Standalone uPower POC (obsolete) —
I began putting this code in Gecko but unfortunately, I have to work on a presentation (and a lighting talk) I'm giving at the end of the week. Might make slow me a bit for the next days.
Did a quick search for MacOS API. It seems that we could/should use this:
https://developer.apple.com/library/mac/#documentation/Darwin/Reference/IOKit/IOPowerSources_h/
just as a side note, sdwilsh tried to make a battery api time ago in bug 454660, there may be something useful there yet.
Depends on: 696038
Attachment #568363 - Flags: review?(khuey)
Attachment #568364 - Flags: review?(jonas)
Attached patch Part D - Dummy DOM code (obsolete) — Splinter Review
Attachment #568366 - Flags: review?(jonas)
IOW, when the battery state changes, the backend have to call NotifyObserver and BatteryManager will fire the correct events.
Attachment #568367 - Flags: review?(jonas)
Attached patch Part F - Hal boilerplate code (obsolete) — Splinter Review
Attachment #568368 - Flags: review?(jones.chris.g)
Attachment #564798 - Attachment is obsolete: true
Whiteboard: [needs review]
Blocks: 696038
No longer depends on: 696038
Blocks: 696041
Blocks: 696042