Yes, agreed.

Jaap, do you think we should do this before 2.14? It would mean adding new strings, so we would have to do it this week.

I'll try to come up with a patch this week. Patch is not difficult but I need the time. Let's try to get it in :-)

Okay, it should be pretty simple I guess of the gpm-prefs side of thing, we'll need to change the gconf keyss and then lastly the logic in the manager.

I'm currently finishing writing the patch. Will try to do a couple of test and attach what I have sofar. So you can test it :-)

Created attachment 57880 [details] [review]
Patch which I think should work

Richard / Jon

Can you try this patch? I think this works. (Haven't had my battery in a low status yet so don't know for sure)

There are no string changes sofar

Comments appreciated

I just changed that it uses remaining time. Not that we do modal dialog boxes and stuff when stuff is getting really critical. Better to add that in a seperate patch. So it's basically just as before + fixes so it will only hibernate or whatever and show notifications when your laptop battery is low and not when your mouse battery is low :-)

Initially I wasn't sure about hardcoding the:

BATTERY_LOW_REMAINING_TIME		(20 * 60) /* 20 minutes */
BATTERY_VERY_LOW_REMAINING_TIME		(10 * 60) /* 10 minutes */
BATTERY_CRITICAL_REMAINING_TIME		(5 * 60) /* 5 minutes */
BATTERY_LOW_PERCENTAGE			(10) /* 10 percent */

values, as some power users may want to change the settings, but then I guess it's okay as long as we provide sane defaults. It's not like we are KDE :-)

I'm happy to commit this in it's current state, and then we can work from this as a baseline. Is that okay Jaap? And wow, you work fast.

Thanks, Richard.

Please commit then.

Could you resync to todays cvs please (should just be a simple merge), and also add a ChangeLog entry. Many thanks.

Created attachment 57955 [details] [review]
Updated patch + Changelog

There is a spurious "j" in the schemas.in.

I haven't followed this whole conversation so forgive me.  I'm not really clear on why this change is desireable.  Here is one thing to consider... Correct me if I'm wrong.

Time is a computed quantity.  It is computed from the amount of charge remaining and the discharge/charge rate.  So the time relies on not only the charge but an accurate measurement of the change rate.  The change rate is highly variable.

In fact, the charge rate on my laptop varies from zero to huge numbers.

I think it is more failsafe to use limits based on normalized charge values.  Using a percentage as the limit only relies on an accurate meaure of the charge and max-charge.  These are usually non-volatile measurements.

So, unless I'm misunderstanding something then we should probably keep it as it is.  You can still remove the UI for it if you choose.

Jon, I think the bug has come about as quite a few people think that "10 minutes" is easier to understand than "10% of your battery capacity" .. my rate seems to pretty well give the time to completion.

I agree that we may be using the time as a primary measurement, instead of percentage which would be more "accurate".

Jaap, what do you think about this? Jon, could you test Jaap's patch, and see what sort of notifications you get under different conditions.

Thanks, R.

OK, thanks for catching me up.

If that is the primary motivation then couldn't we continue to trigger the notification based on the percentage and just calculate the time remaining for the notification message?

Another thing is that the charge rate is completely undefined in a few cases, such as at when AC is connected or disconnected. 

Percentage based doesn't really work because I can insert multiple batteries with multiple capacities so you the time you get notified are very different.

What do you mean with undefined. What I see with charge rate when I connect or disconnect is that the charge_rate is 0 or too low. This is not a problem because the notification and also hibernation will not be triggered by this.

A theoretical problem that can occur is that the remaining time goes first below 20 minutes if you do for example heavy processing and you get a notification then you stop immediately after the notification with heavy processing and then the remaining time goes up again above 20 minutes and you'll get a new notification a bit later. I guess in practice this is not very likely because battery events come at a pretty low rate. If it is a problem we can solve it pretty easily by remembering we already notified at 20 minutes.

So in my opinion there is not a problem the way we do it in the patch

>A theoretical problem that can occur is that the remaining time goes first
>below 20 minutes if you do for example heavy processing and you get a
>notification then you stop immediately after the notification with heavy
>processing and then the remaining time goes up again above 20 minutes and
>you'll get a new notification a bit later. I guess in practice this is not very

This wouldn't happen if we store the "last_notified" as you suggested.

I think the percentage argument is moot, as if we add the capacities, and the last_full capabilities then the percentage is just as accurate with multiple batteries.

I'll merge 90% of Jaap's patch, i.e. the removal of options, and the schema stuff, and them at least any subsequent (controversial) patches should be really small.

Richard.

Is your plan to attach a patch that does the remaining time stuff?? So we can discuss that and maybe add the last_notified bit??

I've commited 90% of your patch, as you'll see in CVS. I left gpm-manager with it's current logic (just hardcoded low to 10, and critical to 2), until we get some agreement from Jon.

Created attachment 58036 [details] [review]
new patch

Jaap, Jon, what about this patch as a compromise? It allows us to revert back easily to using the percentages if remaining time proves to be unreliable. Using the remaining time works fine for me btw.
Feel free to refactor the patch, but I really want to commit something like this soon.

I'm sorry but I just don't understand the motivation here at all.  Using percentage works with multiple batteries just as well as time, if not that is a more serious problem.

Percentage is always going to be more reliable.  You can always use the time remaining for the notification even if it is triggered by a percentage.

What I mean by charge rate being undefined is that charge rate is an average of some kind and there is no telling what the rate will be when there is a discontinuity (like plugging or unplugging).  But the main point is why on earth would you want to mess around with the charge rate at all when percentage tells you everything you need to know?

Ohh agreed. But then the user knows the first notification is at 20 minutes, the second at 10 minutes, rather than at some arbitary time.

Say I have a shitty old battery, and it warns me I'm "very low" at 10% -- this gives me 3 minutes to find the adapter, find a plug-point and switch to ac power.
Say I have a shiny new iBook, then my 10 percent "very low" warning happens when I have 20 minutes remaining.

Explaining to people this is per-time rather than per-percentage is much easier in my opinion. But I agree, per-percentage may be clearer for some people. A few people commented on this on the ubuntu discussions (page here: https://wiki.ubuntu.com/PowerManagementConfiguration) and I know Corey has some strong opinions on this for ubuntu. (i've cc'd him)

I agree that we could base the threshold points on the last_full capacities of the batteries, but then this gets even more confusing for the multi-battery case.

If we can say "you get warned at 20 minutes, 10 minutes, 5 minutes then 2 minutes" then it makes it very clear to any user what to expect.

I do understand your points tho.

If you use percentage (or perhaps percentage of last charge) you can take degradation into account.

If you have a shitty old battery that only lasts N minutes then you may get a critical warning immediately if you use a fixed time.  If you use a percentage then at least you'll get, say, 95% effective usage of the battery always before shutdown.

I think the old battery case is handled better with percentages if we take into account the last max charge.

Yes agreed, the percentage thing does work better for old batteries. I'm tempted to make this a gconf configurable, and just see what sort of response having one of the other gives. Like you say it's probably better to default to percentages, but I really want there to be both involved for the Ubuntu people, that said it was a feature-blocker not having it per-time. Doing one or the other just from changing a gconf key or an #ifdef is better than changing lots of stuff in otherwise fast moving code. You agree?

Jon,

Can I ask you 2 questions to get things clear?

1. If remaining time is reliable are you then in favor of remaining time?

2. What is the remaining_time (and charge_rate) when you unplug or plug in your ac adapter

Jaap 

I don't see a good way to handle the error intrinsic to the calculated time remaining.

Percentage is a monotonically decreasing quantity.  It will always go down when discharging.  There are very few constraints on the time remaining.  It is just not as trustworthy.  If we are going to do things like power down based on this value it ought to be as trustworthy as possible.

Beyond that, I think it is harder to deal with the aging battery problem when you have a fixed time limit for battery criticality.

So,

1. No I don't think it can be as reliable.
2. When I unplug my cable in the rate is zero, then something small like (1000), then starts to settle into a value if the system is quiet.

Guys, if I merge my patch, then we can just argue about the default setting... quite a few other bugs depend on this one, so I would like to get it sorted tonight.

Sure.  :)

Committed, thanks to both of you.

Jon, we'll give the per-time thing a stint in cvs, if it breaks, we'll just switch the default to per-percentage.

ChangeLog:

 * src/gpm-manager.c: Define the BATTERY_*_REMAINING_TIME constants.
 (maybe_notify_battery_power_changed): Only warn on primary battery, and also provide two methods for working out the notifications, to try to keep the peace for bug (#326743). This is based on a patch from Jaap, to change the notifications to time-remaining values rather than percentage charge.
 (power_battery_power_changed_cb): Invalidate the prior warning level as the power state has changed.
 (gpm_manager_init): Invalidate the warning level, and set the default to per-time rather than per-percentage. We can change this easily if this doesn't work in real world conditions, or perhaps make this a gconf configurable.
 (power_battery_power_changed_cb): Only do critical action on primary battery, fixing bug (#328228). Patch originally from Jaap.

Richard, go ahead and merge the patch. As you say we can get the exact details sorted out later. Just leave this bug open until we figure out what we want to do in the end.

Jon, I think you might have misread my first question. 

So please allow me to ask the question again:

1. If remaining_time is reliable are you then in favor for using remaining_time?


(Un)plugging behavior
--------------------

You are seeing exactly the same behaviour I am seeing (and probably everybody else) when plugging or unplugging AC. I.e. charge_rate of 0 and then a charge_rate that is too low. This is obvious because the charge_rate is an average over a period of x seconds and if the batteries were not discharging for a while during the beginning of that period the average will be lower then what it will be in the long run.

This will lead to a temporary remaining_time "Unknown" or a remaining_time that is too large. As said this is not really a problem because with if remaining_time is "Unknown" we won't do anything or if it is higher then it actually is also nothing will happen. OK theoretically we could get an extra notification if the actual remaining time is lower than 20 minutes but that remaining_time jumps up temporarily because we are just unplugging. This could be easily solved by just ignoring the first callback for remaining time after unplugging or plugging AC.

Aging batteries
---------------
Batteries have a certain energy capacity usually measured in mWh. When batteries come from the factory you will be able to load them to their design capacity. Due to reloading their capacity will drop which is represented in HAL by last_full_capacity. A load of the battery (in our case the laptop) just sucks  a certain mWh per hour out of the battery. So if you look at the current_charge you can calculate exactly how many minutes of battery life you have. And aging batteries are automatically taken into account into this. 

Power Down
----------
Power down on a percentage of a battery can never bring the user the optimal experience. As a user I want to be able to work as long as possible. So just a couple of minutes before my batteries are empty the system should shutdown.

If you want to use a percentage you want to use has to take into account what your battery capacity is and what the discharge rate is. If you just use a number on some systems you might hibernate with 1 hour working time remaining and on other systems 30 seconds remaining (hibernate would probably even fail).

charge_rate changes depending what you do with the computer but it's the best estimate there is, because the best estimate is to assume that the user is continuing what ever he or she is doing.

remaining_time is simply just the best estimate we have.

How are you going to determine the threshold for the percentage in your case?

I think the compelling part is your power down scenario.  In order to work around the errors (do we know the error bar for this measurement?) that I mentioned and the fact that the remaining_time is not a monotonically decreasing function of time we should probably require that the remaining time stay below the threshold for a certain number of "ticks" or update intervals.

Why can't we power down the first time we are below critical threshold of 5 minutes? 

If remaining_time equals 5min it means that if the current load (charge_rate) remains the same for the next 5 minutes our batteries will be dead. OK the charge_rate could change in the next 5 minutes depending on what you do with the computer. But does it really matter if we have 3 minutes left if we start at that moment a very heavy compute job that takes 3 min or maybe 7 minutes if we leave the computer idle?  

Actually remaining_time is pretty accurate if the load (thus charge_rate) would remain constant. The issue is that with powersaving features a laptop is using more or less power depending on what you do with it. So if the load changes (and thus charge_rate) the estimate of remaining_time changes. If for some reasons we find the remaining_time variations due to load variations to confusing for users we could always low pass filter (or average) the charge_rate over some longer time.

Jon, if this still does not convince you, I'm eager to hear how your solution works and why for a gpm user your solution is better. If your solution is better then we should go for that.

Created attachment 58679 [details] [review]
patch to do it like jaap suggested...

Jaap, I think I was wrong. I've tried the new done_notification_fully_charged, and it works fine on my toshiba, but not my iBook. Can you check the attached patch and tell me what you think?

Richard, why does it not work on the iBook with the existing code?
It should imho and if it doesn't I guess there's a bug in hal

I happens on my Toshiba also sometimes (don't you just love an intermittent problem). It's when the is_charging state changes before the percentage change (random time interval) so when the percentage goes 99->100 it's not charging and we can't add a check for that. We need to add the check else we get a 100% charged notifications (plural) a few times when on battery power (as the flag is cleared) and we are at state 100%. The code is too fragile at the moment, and this new code seems to work for me in all cases.
I'll commit the change as it fixes a bug for both the toshiba and the iBook, and we can always rework it if it causes subtle problems for other people (can't see how it could..) -- thanks for the review.

Can you do me a favor and first commit the BatteryKind enum and BatteryStatus patch?
Otherwise that patch does not apply anymore.

Another (better) idea on how to fix this. 

Because of HAL or ACPI or whatever we get in weird states is_charging FALSE when we are still charging etc.

I think it's better to fix this all in one place. I.e. gpm-power
There we already have some fixes to make sure we are in a decent state. So the best is to do it there in my opinion. Then the other modules don't have to worry about these peculiarities

Okay, I'll check your patch, and apply it first.

As for my patch, I think I'll apply it as it makes the code very robust and resistant to these errors. I'm not sure we can easily special-case the charging states in gpm-power. With my patch you can say "you get the notification when the global per-machine charge goes from 99% to 100%" as opposed to working out logic and potentially overridding the is_charging flags, or invalitating flags under certain contitions.

Richard.