GNOME Bugzilla – Bug 329027
UPS needs love
Last modified: 2006-02-16 00:30:13 UTC
When I remove the AC from my UPS, the icon in the notification area doesn't change. Also the tooltips appear to be wrong. May need some fixes on the hal side too.
Ugh, this is observed on Linux but I filed this from Safari on Mac OS X
Probably most of the fixes need to be in GPM. I already saw stuff in the code where I thought this won't work for UPS. After finishing a patch I'm currently writing I might go and look into this.
>Ugh, this is observed on Linux but I filed this from Safari on Mac OS X Righto -- you've gone all Mac on us. ;-) >After finishing a patch I'm currently writing I might go and look into this. Sure, I'll dig out my UPS from downstairs and start testing. Richard.
Sounds good - ideally g-p-m should have as little special casing for UPS's as conceptually it's just another battery. At some point we might want a UPS tab in g-p-p + some methods in HAL to do further configuration (apcupsd and nut seems to offer many crazy things) but at this point I'm not sure we even need that...
Btw http://xvsxp.com/power/ contains some interesting things about what Windows XP and Mac OS X does for UPS's - something to think about...
Created attachment 58857 [details] [review] test pacch Jon, Jaap, can you review this patch please? It unmuddles the primary, ups, and mouse notifications into seporate flags, so we don't get all muddled. David is right, the code is a mess, as mouse battery events are not processed correctly, neither UPS anymore. There is very little logic change, except the refactoring, and no string changes. I want to commit a cleanup like this before we release a new version as it should clean up a few bugs.
With this patch I get this output while running CVS HEAD [davidz@orion gnome-power-manager]$ src/gnome-power-manager --no-daemon --sm-disable ** (gnome-power-manager:8722): WARNING **: Unable to load icon gnome-dev-memory ** (gnome-power-manager:8722): WARNING **: Device is not being watched: /org/freedesktop/Hal/devices/usb_device_409_58_noserial_if0 ** (gnome-power-manager:8722): WARNING **: Device is not being watched: /org/freedesktop/Hal/devices/usb_device_409_58_noserial_if0 ** (gnome-power-manager:8722): WARNING **: Device is not being watched ** (gnome-power-manager:8722): WARNING **: Device is not being watched: /org/freedesktop/Hal/devices/usb_device_409_58_noserial_usbraw ** (gnome-power-manager:8722): WARNING **: Device is not being watched: /org/freedesktop/Hal/devices/usb_device_409_58_noserial_usbraw ** (gnome-power-manager:8722): WARNING **: Device is not being watched ** (gnome-power-manager:8722): WARNING **: Device is not being watched: /org/freedesktop/Hal/devices/usb_device_409_58_noserial ** (gnome-power-manager:8722): WARNING **: Device is not being watched: /org/freedesktop/Hal/devices/usb_device_409_58_noserial ** (gnome-power-manager:8722): WARNING **: Device is not being watched
I also get "Unknown time remaining" and "Unknown time until charged" which is weird as we indeed got these as properties in HAL.. The relevant lshal snippet is below. Maybe g-p-m is expecting some properties that are not there? E.g. when fixing ACPI reading in HAL we forgot to update the UPS polling code.. Richard? udi = '/org/freedesktop/Hal/devices/usb_device_51d_2_QB0435136106_if0_hiddev' battery.type = 'ups' (string) battery.reporting.last_full = 100 (0x64) (int) battery.reporting.design = 100 (0x64) (int) battery.charge_level.last_full = 100 (0x64) (int) battery.charge_level.design = 100 (0x64) (int) battery.is_rechargeable = true (bool) battery.vendor = 'APC' (string) battery.technology = 'PbAc' (string) battery.serial = 'QB0435136106 ' (string) battery.model = '818.w1.D' (string) battery.present = true (bool) battery.rechargeable.is_discharging = false (bool) battery.rechargeable.is_charging = true (bool) battery.remaining_time = 1035 (0x40b) (int) battery.reporting.unit = 'percent' (string) battery.reporting.percentage = 88 (0x58) (int) battery.reporting.current = 88 (0x58) (int) battery.charge_level.unit = 'percent' (string) battery.charge_level.percentage = 88 (0x58) (int) battery.charge_level.current = 88 (0x58) (int) info.udi = '/org/freedesktop/Hal/devices/usb_device_51d_2_QB0435136106_if0_hiddev' (string) info.addons = {'hald-addon-hid-ups'} (string list) hiddev.application_pages = {'Power Device Page'} (string list) hiddev.product = 'APC Back-UPS ES 650 FW:818.w1.D USB FW:w1' (string) linux.device_file = '/dev/hiddev0' (string) linux.subsystem = 'usb' (string) linux.hotplug_type = 2 (0x2) (int) hiddev.device = '/dev/hiddev0' (string) info.product = 'APC Back-UPS ES 650 FW:818.w1.D USB FW:w1' (string) info.capabilities = {'hiddev', 'battery'} (string list) info.category = 'hiddev' (string) info.parent = '/org/freedesktop/Hal/devices/usb_device_51d_2_QB0435136106_if0' (string) linux.sysfs_path_device = '/sys/devices/pci0000:00/0000:00:03.0/usb1/1-2/1-2:1.0' (string) linux.sysfs_path = '/sys/class/usb/hiddev0' (string)
To clarify comment 7 : the icon is still not right (it's still the power cable, I was expecting a battery showing approx how much juice is left) even when the UPS is unplugged... note this is on a desktop system.. not on a laptop..
Ohh yes, I've not done any UPS specific fixes yet, I've just seporated the warning and action code. I'll have a play with my UPS tonight.
Sure, no problem. I'll test this as soon as there is a patch! Rock on!
Richard, For a type do not use prior_warnings_t but use PriorWarnings. If you use lower case people might confuse it with a variable name. Use a GpmWarning enum for GPM_WARNING_* instead of an int type. Always use braces with if statements What happened to keyboard and pda?
Created attachment 59010 [details] [review] test patch Okay, thanks for the review. What about the following? It's a bit obscure in the string handling bits as I don't want to add new strings.
Richard, First I like the idea of the patch but I would do this typedef enum { GPM_WARNING_NONE = 0, GPM_WARNING_LOW = 1, GPM_WARNING_VERY_LOW = 2, GPM_WARNING_CRITICAL = 3, GPM_WARNING_ACTION = 4 } GpmWarning; and remove the PriorWarnings type as private variables you then use GpmWarning last_primary_warning; GpmWarning last_ups_warning; In the code you then do something like this if (current_warning > manager->priv->last_primary_warning) { show_warning (....) manager->priv->last_primary_warning = current_warning } and if (charging) { manager->priv->last_primary_warning = GPM_WARNING_NONE; } That way you don't need the struct with 3 members. There is a clear order of the notifications so I don't think we should need a struct if only a certain order of notifications is allowed What do you think of this
I think you're absolutely right. I've reworked the patch exactly as you suggested, and it's a lot simpler now! Cheers for the review, it helped a lot. 2006-02-10 Richard Hughes <richard@hughsie.com> * src/gpm-manager.c: Add GpmWarning struct so we can track the warning states of all the devices indervidually. Also add BATTERY_ACTION_PERCENTAGE and BATTERY_ACTION_REMAINING_TIME so we do the policy action some time after the last critical warning. (maybe_notify_battery_status_changed) Removed, as functionality was muddled. We need to split this per-batterytype, as we do very different things for each device, e.g. warn for mouse low, but policy and warnings for the primary battery. (gpm_manager_get_warning_type, battery_low_get_title): Abstract out the warning type and title generation from the background logic -- so we can start to split up the functions per-device. (battery_status_changed_primary, battery_status_changed_ups, battery_status_changed_misc): Add these different handlers that it's clear that these types have different actions and logic. These functions are a bit bodged w.r.t message strings, as we are in string freeze, and I can't add lots of new strings yet. (power_battery_status_changed_cb): Dont do policy here, leave it to the indervidual handlers for the type (e.g. battery_status_changed_primary) to do the actions. Makes logic lots clearer. I can now test the UPS stuff now (to fix David's original problem...), without my machine keep shutting down! Richard.
Created attachment 59137 [details] [review] Adds various UPS related fixes to g-p-m David, could you test the attached patch please? Also Jaap, coments please, as I think the tooltip stuff broke for you last time I touched it. :-) There are various different fixes required for UPS, surrounded by lots of comments! Now, instead of the broken tooltip, I get "UPS 13 minutes until charged (94%)". There's also a couple of new strings in there, which would have to be removed before I committed this into CVS, but nothing that it couldn't do without.
Yup, this fixes the tooltip (will attach screenshots) but... should it still say "Computer is running on AC Power" when running on a discharging UPS? Probably better to say "Computer is running on emergency battery power" or something? Also.. i thought the icon was supposed to be a battery? I think it makes sense to show a battery icon both when discharging / charging / fully charged. I also got several "battery low" notifications for the UPS a'la "UPS: Battery Low (16 minutes)"... even when charging the UPS. I probably shouldn't get them when charging. Should probably use lower limits for the UPS too as UPS batteries in general are a lot weaker than laptop batteries. I haven't yet tested this the UPS on laptop scenario but that's probably interesting too cf. http://xvsxp.com/power/images/ups11.jpg
Created attachment 59142 [details] tooltip when discharging
Created attachment 59143 [details] tooltip when charging
Ahh, I've been testing the UPS on my laptop, so havn't had those issues. I'll fix the icon thing today. >I also got several "battery low" notifications for the UPS a'la "UPS: Battery >Low (16 minutes)"... even when charging the UPS. I probably shouldn't get them Hmm. does lshal report the is_discharging and is_charging okay? Richard.
Created attachment 59146 [details] [review] test patch #2 David, could you try this one? It should fix the tooltip, icon, and also not display the notifications when charging. There's still the: /* FIXME: We need to display 'Computer is running on backup power' if on UPS */ which I need to fix too. Thanks, Richard.
Alright, tried the patch from comment 21. Progress! A few observations from my desktop system (ie. not a laptop) 1. Can confirm the FIXME in comment 21 (still displays "Computer is running no AC power") 2. With UPS attached g-p-m shows the right icon and things appears to work. 3. When starting without a UPS attached, hotplugging the UPS via USB makes g-p-m do the right thing, e.g. g-p-m reverts from gnome-dev-acadapter.png to gnome-dev-power-ups-x-of-8.png and it's like in 2. above 4. Removing the UPS doesn't do the right thing, e.g. g-p-m should revert to gnome-dev-acadapter.png as if I started without a UPS attached On my laptop system things almost works though the tooltip could use some love and I'd expect two icons as in http://xvsxp.com/power/images/ups11.jpg - will attach screenshot of UPS on laptop
Created attachment 59158 [details] UPS on laptop system I think that I should have two icons here and the 2nd line of the tool tip should read "Laptop battery 44 minutes remaining (62 %)" instead of "44 minutes remaining (62 %)"
I also note that we only have 8 icons for the UPS, namely gnome-power-ups-X-of-8.png. Suggest to include 8 more for when charging, e.g. gnome-power-ups-X-of-8-charging.png and one for fully charged e.g. gnome-power-ups-charged.png If you concur, I'll ask Diana to draw some nicer icons.
I'm not sure about the two icon thing. The interface guideline people would go mad. I'll work on the FIXME (this needs structural work to not be a bodge) and I'll also fix the hotplug removal, although that *should* just work. I'll tidy up the patch you tested, and commit it, as it's got some other nice tidyups too. Also with the tooltip, there is an explicit: + if (entry->battery_kind != GPM_POWER_BATTERY_KIND_PRIMARY) { + /* we don't display Laptop battery Foo" */ + g_string_append_printf (summary, "%s ", type_desc); + } So that the laptop battery thing is hidden -- is that wrong? Surely we should only clarify the device if there is a UPS in the system, but not if there is just a laptop with one primary battery. Jaap, can you confim the patch has not broken anything for you? And with the icons, I agree about charging and discharging. If Diana isn't too busy I would love some new ones. My GIMP skills are that of a 5 year old :-) Thanks, Richard.
In response to comment 20: >>I also got several "battery low" notifications for the UPS a'la "UPS: Battery >>Low (16 minutes)"... even when charging the UPS. I probably shouldn't get them > >Hmm. does lshal report the is_discharging and is_charging okay? Yes, what hal report seems fine although we don't report the change atomically (e.g. there will be times where we are neither charging nor discharging and there are times where we are both charging and discharging). We probably should fix this in HAL though it seems you do work around this in g-p-m for the time being, e.g. I see "battery_kind_cache_update: Sanity check kicked in! Multiple device object cannot be charging and discharging simultaneously!" as debug info from g-p-m. So right now I only a single notication right after pulling the AC from the UPS... like this "UPS : Battery Low (12 minutes)". I wonder why this is.. in gconf you have the keys /apps/gnome-power-manager/general/threshold_low /apps/gnome-power-manager/general/threshold_critical that speaks about percentages. Are you mistakenly using minutes? Rock on - David.
In response to comment 25: > I'm not sure about the two icon thing. Neither am I. suggest to just punt for now. > Surely we should only clarify the device if there is a UPS in the system, > but not if there is just a laptop with one primary battery. Agree, only prefix with "Laptop Battery " if there is an UPS. > And with the icons, I agree about charging and discharging. If Diana isn't > too busy I would love some new ones. My GIMP skills are that of a 5 > year old :-) I'll ask her the next time I'll see her.
In addition to my ramblings in comment 26 perhaps it would be useful with a general notification "DANGER ROBINSON! Your system is running on backup power! blah blah!!" when the UPS tells that it's AC has been removed. This is in contrast with the fact we don't want this when removing AC from a laptop. The former case is far more important.
> /apps/gnome-power-manager/general/threshold_low > /apps/gnome-power-manager/general/threshold_critical > that speaks about percentages. Are you mistakenly using minutes? These are not being used anymore. They're not in the lastest schema I think. If the battery supports time, then we use a time warning system (e.g. ups and primary) but other batteries (mice) are warned per-percent. This makes capacity independant of the notification, so you get the same notification time on a small and big laptop battery. >In addition to my ramblings in comment 26 perhaps it would be useful with a >general notification "DANGER ROBINSON! Your system is running on backup power! >blah blah!!" when the UPS tells that it's AC has been removed. Yes, agreed -- I'll add this as a FIXME for now, as it's getting late :-) I'll test the UPS stuff more tomorrow, and get you to try a new CVS + patches. Richard.
>> /apps/gnome-power-manager/general/threshold_low >> /apps/gnome-power-manager/general/threshold_critical >> that speaks about percentages. Are you mistakenly using minutes? > > These are not being used anymore. They're not in the lastest schema I think. > > If the battery supports time, then we use a time warning system (e.g. ups and > primary) but other batteries (mice) are warned per-percent. This makes capacity > independant of the notification, so you get the same notification time on a > small and big laptop battery. Sweet, I'm a big believer of removing preferences! Rock on. However, for UPS it might be clever to use percentage as these normally only give you around 10-20 minutes dependent on make and model. There's also the case of laptop batteries getting older and only holding like 40 minutes. So.. perhaps the general solution is to look at both time remaining and percentages? > I'll test the UPS stuff more tomorrow, and get you to try a new CVS + patches. Legendary. I'm ready to do more testing tomorrow. Rock on!
2006-02-12 Richard Hughes <richard@hughsie.com> * src/gpm-manager.c (get_stock_id): Refactor the code, leaving the logic mostly the same as before. Now it's obvious what's happening in the selection, and also allows a UPS icon to be chosen. (battery_status_changed_primary): Slight rearrangement for logic optimisation. (battery_status_changed_ups): Add a check for discharging. * src/gpm-power.c (battery_kind_cache_update): Make work with UPS's as they do not provide rate information, but do provide time information. (power_get_summary_for_battery_kind): Simplify the logic, and make "Unknown time remaining until changed" a thing of the past. Also show the primary battery type, if we have a UPS installed in the system so we know what the remaining time is refering to. (gpm_power_get_status_summary): If we are using a discharging UPS, then set our state to "Computer is running on backup power". Also remove the enum structure thing as it's only used once, and it's easier just to use the defines directly. I still need to do the "DANGER ROBINSON!" message. Can you confirm this works, as my UPS is downstairs, I am upstairs, and I am lazy. >Sweet, I'm a big believer of removing preferences! Rock on. However, for UPS it >might be clever to use percentage as these normally only give you around 10-20 >minutes dependent on make and model. There's also the case of laptop batteries >getting older and only holding like 40 minutes. So.. perhaps the general >solution is to look at both time remaining and percentages? The first warning message kicks in at 20 minutes, but if this proves to be unreliable, we can switch the notification method just for UPS's in a one line change :-) Richard.
Created attachment 59162 [details] [review] add discharging warnings You owe me one hour of sleep Mr Zeuthen. :-) Attached is a patch that should give you a warning. Other than compiling, it's completely untested. Currently the notification goes like this "Your system is running on backup power!" What else can it say? I guess it should explain a little. Richard.
Looks good, though you want space between "to" and "avoid", e.g. I get the word "toavoid". Message: [libnotify_event] gpm-tray-icon.c:613 (13:13:12): libnotify: Power Manager : You have approximately <b>20 minutes</b> of remaining UPS power (97%). Restore power to your computer toavoid losing data. Heh, we'll trade that hour of sleep that I owe you for several cold beers at GUADEC!
Also still missing support for unplugging the UPS via USB. I guess that and the new icons are the only issues left before we can close this bug?
I've fixed the toavoid thing, and I'll try and fix the hot-unplug thing now. Can you open another bug for the icons please, as this bug has got rather long! For now: 2006-02-13 Richard Hughes <richard@hughsie.com> * src/gpm-manager.c: Add an extra state GPM_WARNING_DISCHARGING so we can make the UPS and primary battery stuff common in the existing warning framework. (maybe_notify_on_ac_changed): Only clear the notification now, as we rely on the trueness of battery.is_discharging to do the warning. (gpm_manager_get_warning_type): Add the check for a now-discharging battery_status object. Man, this generic code is easy to change. (battery_low_get_title): Add a string for GPM_WARNING_DISCHARGING. (battery_status_changed_primary): Do the warning "The AC Power has been unplugged." here, for GPM_WARNING_DISCHARGING. (battery_status_changed_ups): Do the warning "Your system is running on backup power!" for GPM_WARNING_DISCHARGING. (battery_status_changed_misc): Ignore GPM_WARNING_DISCHARGING as this is not relevant for pda, mouse or keyboard devices. (gpm_manager_init): Set last primary warning to GPM_WARNING_DISCHARGING as we don't want to be notified on coldplug if we are on battery power.
hot-unplug of mice and ups now works for me: 2006-02-13 Richard Hughes <richard@hughsie.com> * src/gpm-power.h, src/gpm-power.c (gpm_power_class_init): Add battery removed proxy signal so we can use it in the manager. (add_battery): Do not query hal again, use the cached values for the capacity check. Also, only do this check if the battery is marked as present. (power_get_summary_for_battery_kind): Remove the complicated check for UPS's, as it's fragile and seems to break. Unconditionally display the battery type as a prefix for now, we can argue about the tooltip logic in a new bugzilla. (battery_device_cache_entry_update_all): Do more careful checking, (e.g. don't ask a mouse device for time_remaining) so we don't spew so many warnings. * src/gpm-manager.c (gpm_manager_init): Add hal_battery_removed_cb so we can update the icon and tooltip to fix #329027.
>If you concur, I'll ask Diana to draw some nicer icons. One thing before I forget : they need to be square, the GNOME HIG people get *very* upset with non-square icons.
Icon bug created : http://bugzilla.gnome.org/show_bug.cgi?id=331117 I'll close this bug now as I think I've addressed all your points.
I just tried g-p-m CVS and all my points have indeed been addressed. Good job Richard, I owe you a bunch of beer! I have sent a request for UPS icons via email to Diana for icons and Cc'ed you on it. Thanks again, David
David, Could you ask Diana to make the icons look more gnomey? Such that they maybe could be included in gnome-icon-theme. For that we would need png icons of 16x16, 22x22 and an svg icon Jaap
In response to comment 41 > Could you ask Diana to make the icons look more gnomey? Such that they maybe > could be included in gnome-icon-theme. For that we would need png icons of > 16x16, 22x22 and an svg icon I think the current icons match Bluecurve so it would probably be best to keep it that way. Btw, isn't the Tango guys doing Tango icons for g-p-m? I know they have some for NetworkManager which also ships the Bluecurve stuff in the tarball. At some point RH should probably move the Bluecurve ones to the redhat-artwork RPM however I don't think we're there just yet (code needs to settle) but maybe soon.