Laptop battery indicator

[RJ Skerry-Ryan]

Add a widget to show the laptop battery level.

[Till Hofmann (morxa)]

I have an OS-specific class (inherited from an abstract class to define the interface), what's the best way to include it? I could
a) simply use #ifdef and name the classes specifically (e.g. ClassALinux) (this is basically the Abstract Factory pattern)
b) name all classes the same and put it in a OS-specific folder (e.g. linux/ClassA.*) and include the right class using the flags in depends.py,
or c) ....

I didn't find any example in the existing code, what's the preferred way?

(I asked this question in IRC some days ago but I didn't get an answer so I figured I'd try here instead)

[William Good (bkgood)]

We generally try to steer clear of platform-specific code, which is why you weren't able to find an example. I'd put each implementation of the ABC in its own file, then add the implementation to the list of files to build conditionally in the build files (so if windows, add the windows implementation to the list of .cpp files to build and link, etc). Then, use a static method in the ABC to return an instance of an implementation specific to the os. Something like:

// ifdef these if the headers contain os-specific code, or consider taking the os-specifics out and putting them in the cpp file
#include <WindowsImpl.h>
#include <LinuxImpl.h>
#include <OSXImpl.h>

class ABC {
public:
[...]
static ABC* batteryReader(...) {
#ifdef Q_OS_WIN
return new WindowsImpl(...);
#elif Q_OS_LINUX
return new LinuxImpl(...);
#elif Q_OS_MAC
return new OSXImpl(...);
#else
return NULL; // NULL implies an implementation does not exist
#endif
}
[...]
};

[Phillip Whelan (pwhelan)]

We will have to do this sort of thing if we want to implement a timer inside of Mixxx itself. There is no freely available cross platform library for high resolution timers, at least not that I know of. Porttime uses usleep on Linux, which I've tested and does not seem adequate for 1ms timers.

[Till Hofmann (morxa)]

Thanks Bill for your quick reply.
The problem isn't really a timer but I think there is no cross platform library for reading the status of the battery, so we have to find a different solution for every platform. If you know any cross platform library that can read battery status (charging, discharging, ...), battery percentage and current discharge rate, please let me know!

I almost finished the linux version, it's working, now I just have to create windows and mac stubs... I can't really finish it by myself since I don't have a windows or a mac to test it, but I'll go as far as I can.

[William Good (bkgood)]

Till: Qt Mobility seems to have battery info for Windows, Mac and Linux (see http://doc.qt.nokia.com/qtmobility-1.2/index.html ; the table near the bottom of the page). That said, the linking/distribution overhead of qt mobility may be such that we would rather just support platforms we can ourselves, no idea there.

Phillip: the porttime Linux implementation uses the select(3) system call which blocks for x microseconds. Whether that resolution/granularity is truly reached is likely system-dependent. See line 70 here: http://portmedia.svn.sourceforge.net/viewvc/portmedia/portmidi/trunk/porttime/ptlinux.c?revision=209&view=markup

[Till Hofmann (morxa)]

But doesn't Qt Mobility support Symbian only? I've seen the Qt Mobility QSystem classes but we can't use them since they work on Symbian only. This would be exactly what we need...

Another idea I had: I could put each platform specific class in it's own directory and name all the classes the same. It would look like this:

win/batteryreader.h
win/batteryreader.cpp
linux/batteryreader.h
linux/batteryreader.cpp
mac/batteryreader.h
mac/batteryreader.cpp

In addition, we'd have these cross plattform classes:
widget/wbattery.h
widget/wbattery.cpp

battery.h
battery.cpp

Battery either contains a BatteryReader* pointing to the plattform specific reader class or it is an abstract class which is implemented by the BatteryReader class(es).

This way, we could avoid using any #ifdef's. The only switch would be in depends.py. I think that's the cleaner solution.
The only disadvantage would be that you could not determine which plattform a class belongs to since they are all named the same.

[Sean M. Pappalardo (pegasus-renegadetech)]

No, Qt Mobility supports the Tier 2 platforms as well for most components. See http://doc.qt.nokia.com/qtmobility-1.2/index.html#platform-compatibility

This looks like what you want: http://doc.qt.nokia.com/qtmobility-1.2/qsystembatteryinfo.html but it's only available in v1.2 which doesn't look like it's been released yet. QSystemBatteryInfo is a member of "System Information" which is supported on all Tier 2 platforms according to the chart at the first link.

I echo Bill's concern of having to include another library if it's large, unless there's a way to just include the QSystemInfo part. (More of an issue for Windows and Mac packaging.)

If you instead go the DIY/platform-specific route, I like what you propose: putting platform-specific implementations into their own sub-directories and having Battery be abstract for the platform-specific parts, then just have depends.py select the correct cpp file at build time. (Then in the future if we need more platform-specific code for other things (God forbid,) it can go in those same directories and we can tell at a glance what's platform-specific.)

[Phillip Whelan (pwhelan)]

When I was talking about the timer (scheduling timer) I was referring to the possibility that we might have to include more platform-specific code in the future. This might be a good way to test the waters before we do it with more critical parts of our code.

[William Good (bkgood)]

I built QSystemInfo on Linux, the stripped binary is 384K, so good news there (2.9M with debug symbols). It wasn't able to read my battery status (although reading battery info on Linux looks to be pretty well implemented from browsing the code so who knows there, I got tired of mucking with it after a while) but I'd guess it'll be a viable solution one day in the not-so-distant future.

This was Qt Mobility 1.2, which was released last month (http://www.meegoexperts.com/2011/05/qt-mobility-1-2-0-released-meego-maemo/) although the Qt folks are doing an awful job of reporting it. I kinda get the feeling that this was something Nokia was behind and was just gaining steam when the WP7 announcement was made. Luckily it's not a horribly complex project so the community might adopt it. The API, at least for battery reading, seems well thought out and nice to work with.

Till, sorry this has been hijacked so badly. My preference is strongly against platform-specific directories, even if #ifdefs are ugly (and they are), but they could easily be quite minimal (as described previously).

[RJ Skerry-Ryan (rryan)]

In this case, I think I'd prefer N platform specific implementations of a
class that looks up the battery status. Ideally not in separate folders,
just name the classes like (for example) batterystatuswindows.cpp
batterystatuslinux.cpp batterystatusosx.cpp and use the same header file for
all 3 batterystatus.h and include the appropriate cpp in teh SConscript
based on the platform.

Qt Mobility has a solution here, but that would mean we would have to build
our own custom Qt or package qt mobility ourselves for Debian, which is
going to be a giant pain in the butt. For such a small amount of
functionality, I'm fine with adding platform specific classes.

On Fri, Jun 3, 2011 at 2:11 PM, Bill Good <email address hidden> wrote:

> I built QSystemInfo on Linux, the stripped binary is 384K, so good news
> there (2.9M with debug symbols). It wasn't able to read my battery
> status (although reading battery info on Linux looks to be pretty well
> implemented from browsing the code so who knows there, I got tired of
> mucking with it after a while) but I'd guess it'll be a viable solution
> one day in the not-so-distant future.
>
> This was Qt Mobility 1.2, which was released last month
> (http://www.meegoexperts.com/2011/05/qt-mobility-1-2-0-released-meego-
> maemo/) although the Qt folks are doing an awful job of reporting it. I
> kinda get the feeling that this was something Nokia was behind and was
> just gaining steam when the WP7 announcement was made. Luckily it's not
> a horribly complex project so the community might adopt it. The API, at
> least for battery reading, seems well thought out and nice to work with.
>
> Till, sorry this has been hijacked so badly. My preference is strongly
> against platform-specific directories, even if #ifdefs are ugly (and
> they are), but they could easily be quite minimal (as described
> previously).
>
> --
> You received this bug notification because you are a member of Mixxx
> Development Team, which is subscribed to Mixxx.
> https://bugs.launchpad.net/bugs/661446
>
> Title:
> Laptop battery indicator
>

[Till Hofmann (morxa)]

I think having one header file for all implementations is too restrictive since they might differ a lot and end in bad readability.

I created three classes now:
WBattery in widget/wbattery.h
Battery in battery/battery.h
BatteryLinux in battery/batterylinux.h

I'm not sure if I should put all classes in widget but I didn't since I figured they are not really widgets (even though they support a widget).

I also changed depends.py so it includes the right platform-specific class.
Battery objects are created through Battery:getBattery which returns a pointer to the right object (using #ifdefs).

The implementations for Mac and Windows are still missing. I can't do them since I have only Linux available.

I also added an example to the skin Outline1024x600-Netbook. You can copy the attached files to res/skins/Outline1024x600-Netbook/ if you wanna try it.

Check the diff and tell me what you think.

[Till Hofmann (morxa)]

I guess I can only upload one file at a time.

[William Good (bkgood)]

I've been fighting Windows quite a bit today so I'll write a Windows implementation in the next couple of days.

[William Good (bkgood)]

Also, the battery-100 and battery-000 images are the same (both look full).

[Till Hofmann (morxa)]

Right, I guess I made a mistake there... But battery-000 is never shown anyways since the battery never reaches 0% and it always shows the next image with bigger percentage (e.g. 5% shows the 20% image). So right now, the images aren't very good since it even shows the 20% when the battery has only 3%, but those images are for testing purposes only. I just took the Ubuntu images and colorized them. I guess skin designers want to design their own?

Anyway, do you think I should change the behavior (e.g. show the next image with lower percentage)? I wasn't sure what's the best.

I attached the updated graphics.zip with the correct battery-000.png

[William Good (bkgood)]

I'd show the closest available, so there'd be a mapping:
Charge -> Image
(90, +inf) -> 100%
(70, 90] -> 80%
(50, 70] -> 60%
(30, 50] -> 40%
(10, 30] -> 20%
(-inf, 10] -> 0%

and corresponding code (note that values outside of [0, kMaxSize] will give 100):
const int kChargeStep(20);
const int kMaxCharge(100);
int charge(GetChargePercentage());
int charge_image(0);
while (charge_image < kMaxCharge) {
    if (abs(charge - charge_image) <= kChargeStep / 2) break;
    charge_image += kChargeStep;
}
// charge_image should now hold the percentage corresponding to the proper image

[Till Hofmann (morxa)]

Hm but with a mapping like that you'd have to recompile to change step size.

I'm not sure if the steps should be predefined or if they should be changeable (e.g. by skin designers). I did a mapping like yours which checks what images are available and chooses the closest, I attached a diff. It's a bit more complicated and uses more memory but I guess it's still insignificant.

Also, I moved the whole process to the setup so the mapping is done only once. It takes more memory (because I create a map of 100 pointers) but all the work is done during setup.

If we want a mapping with fixed step size we could also do something like
(percentage + chargeStep/2) / chargeStep
which also gives the closest image available.

[RJ Skerry-Ryan (rryan)]

We should clean this up and merge it for Mixxx 1.12.0!

[RJ Skerry-Ryan (rryan)]

Hi Till -- I'm working on cleaning up your patch for inclusion in Mixxx 1.12.0. I'd like to commit your existing code to our git repository. What author name and email address should I use?

For example: "Till Hofmann <email address hidden>". Feel free to email me if you'd rather not post it here: <email address hidden>

Also, could you please sign our contributor agreement? This gives us permission to publish your code with Mixxx:

https://docs.google.com/a/mixxx.org/spreadsheet/viewform?usp=drive_web&formkey=dEpYN2NkVEFnWWQzbkFfM0ZYYUZ5X2c6MQ#gid=0

Thanks!

[RJ Skerry-Ryan (rryan)]

Till -- I found your email address via GitHub -- hopefully the one I used is fine. If not I can rewrite the history before merge.

Tonight I added an OS X implementation of Battery based on IOKit. Bill -- did you ever whip up a Windows implementation?

[RJ Skerry-Ryan (rryan)]

My work is ongoing: https://github.com/rryan/mixxx/tree/battery

[Max Linke (max-linke)]

I'm getting a new laptop that comes preinstalled with windows 8 sometime
this or next week. I'll plan to check if the wiki needs updating to
compile mixxx on W8 anyway. If you have any pointers where I can check
for libraries that I can use I'll give it a short try to get an
implementation working.

On Thu, 2014-01-16 at 04:51 +0000, RJ Ryan wrote:
> My work is ongoing: https://github.com/rryan/mixxx/tree/battery
>

[Till Hofmann (morxa)]

Hi RJ,

thanks for putting so much effort in finding my correct email - the one you chose is indeed the right one. I've signed the contributor agreement, let me know if I have to do anything else.

[Max Linke (max-linke)]

Till on what distribution did you test your patch? Debian does not have the file you check anymore

[Till Hofmann (morxa)]

Ubuntu 10.10 or Ubuntu 11.04, I don't remember. I just found libacpi which provides a library for accessing ACPI information: https://github.com/aravind/libacpi . Might be better than accessing the files directly

[jus (jus)]

https://github.com/mixxxdj/mixxx/pull/401

[Swiftb0y (swiftb0y)]

Mixxx now uses GitHub for bug tracking. This bug has been migrated to:
https://github.com/mixxxdj/mixxx/issues/5570