Hi folks!

I am writing to propose a patch for UPower. I have a broken battery (or
ACPI, or what it is, I really don't know) and it doesn't report
energy-rate. From the last version UPower is able to compute the
discharge (or charge) rate by sampling the energy over time, that is a
really nice feature for me!

The thing is that only two points are used to compute this rate, and so
the time estimate is quite going up and down. I thought it would be nice
to use a simple linear regression [1] to determine the rate, so I wrote
a patch to the test the idea, and it is working for me.

Do you think the patch could be included upstream? I think that many
broken-batteries people would be glad to have a consistent estimate of
the remaining lifetime.
I understand, though, that this change make the rate change slowly in
response to some events (like starting heavy computations or so...) but
I still think that taking the average on some more points is nicer.

I'm attaching the patch to this email.

P.S.: The memmove() call are not the best way to handle prepending new
entries in the array; if you think it's nicer I could write a simple
circular buffer to store energy_old values and timespecs.

Thanks again for your work :)

[1]: http://en.wikipedia.org/wiki/Linear_regression
-- 
Leonardo Robol <l...@robol.it>
diff --git a/src/linux/up-device-supply.c b/src/linux/up-device-supply.c
index 9b29713..6e21531 100644
--- a/src/linux/up-device-supply.c
+++ b/src/linux/up-device-supply.c
@@ -45,13 +45,15 @@
 #define UP_DEVICE_SUPPLY_COLDPLUG_UNITS_CHARGE		TRUE
 #define UP_DEVICE_SUPPLY_COLDPLUG_UNITS_ENERGY		FALSE
 
+#define UP_DEVICE_SUPPLY_ENERGY_OLD_LENGTH              4  /* Number of old energy values to keep cached */
+
 struct UpDeviceSupplyPrivate
 {
 	guint			 poll_timer_id;
 	gboolean		 has_coldplug_values;
 	gboolean		 coldplug_units;
-	gdouble			 energy_old;
-	GTimeVal		 energy_old_timespec;
+	gdouble			 *energy_old;
+	GTimeVal		 *energy_old_timespec;
 	gdouble			 rate_old;
 	guint			 unknown_retries;
 	gboolean		 enable_poll;
@@ -118,12 +120,17 @@ static void
 up_device_supply_reset_values (UpDeviceSupply *supply)
 {
 	UpDevice *device = UP_DEVICE (supply);
+	guint i;
 
 	supply->priv->has_coldplug_values = FALSE;
 	supply->priv->coldplug_units = UP_DEVICE_SUPPLY_COLDPLUG_UNITS_ENERGY;
 	supply->priv->rate_old = 0;
-	supply->priv->energy_old = 0;
-	supply->priv->energy_old_timespec.tv_sec = 0;
+
+	for (i = 0; i < UP_DEVICE_SUPPLY_ENERGY_OLD_LENGTH; ++i)
+		{
+			supply->priv->energy_old[i] = 0.0;
+			supply->priv->energy_old_timespec[i].tv_sec = 0;
+		}
 
 	/* reset to default */
 	g_object_set (device,
@@ -240,36 +247,96 @@ up_device_supply_get_online (UpDevice *device, gboolean *online)
 }
 
 /**
+ * up_device_supply_push_new_energy:
+ *
+ * Store the new energy in the list of old energies of the supply, so
+ * it can be used to determine the energy rate.
+ */
+static gboolean
+up_device_supply_push_new_energy (UpDeviceSupply * supply, gdouble energy)
+{
+	/* Check if the energy value has changed and, if that's the case,
+	 * store the new values in the buffer. */ 
+	if (supply->priv->energy_old[0] != energy)
+		{
+				/* Move old values of energy_old forward */
+				g_memmove (supply->priv->energy_old + 1,  
+				 	   supply->priv->energy_old,
+				 	   (UP_DEVICE_SUPPLY_ENERGY_OLD_LENGTH - 1) * sizeof (gdouble)); 
+				g_memmove (supply->priv->energy_old_timespec + 1,  
+				 	   supply->priv->energy_old_timespec, 
+				 	   (UP_DEVICE_SUPPLY_ENERGY_OLD_LENGTH - 1) * sizeof (GTimeVal)); 
+				
+				supply->priv->energy_old[0] = energy;
+				g_get_current_time (&supply->priv->energy_old_timespec[0]);
+
+				return TRUE;
+		}
+
+	return FALSE;
+}
+
+/**
  * up_device_supply_calculate_rate:
  **/
 static gdouble
 up_device_supply_calculate_rate (UpDeviceSupply *supply, gdouble energy)
 {
-	guint time_s;
+	gdouble rate;
+	gdouble sum_x;
 	GTimeVal now;
+	int i;
+
+	/* get the time difference from now and use linear regression to determine
+	* the discharge rate of the battery. */
+	g_get_current_time (&now);
+
+	/* Store the data on the new energy received */
+	up_device_supply_push_new_energy (supply, energy);
 
 	if (energy < 0.1f)
 		return 0.0f;
-
-	if (supply->priv->energy_old < 0.1f)
+	
+	if (supply->priv->energy_old[0] < 0.1f)
 		return 0.0f;
 
-	if (supply->priv->energy_old - energy < 0.01)
-		return supply->priv->rate_old;
+	/* Discharge rate */
+	rate = 0;
+
+	/* Used to compute the sum of the squared times difference */
+	sum_x = 0;
+
+	/* Don't use the new point obtained since it may cause instability in the
+	 * estimate */
+	now = supply->priv->energy_old_timespec[0];
+	for (i = 1; i < UP_DEVICE_SUPPLY_ENERGY_OLD_LENGTH; i++)
+		{
+			/* Only use this value if it seems valid */
+			if (supply->priv->energy_old_timespec[i].tv_sec && supply->priv->energy_old[i])
+				{
+					/* This is the square of t_i^2 */
+					sum_x += (now.tv_sec - supply->priv->energy_old_timespec[i].tv_sec) * 
+						 (now.tv_sec - supply->priv->energy_old_timespec[i].tv_sec);
+					
+					/* Sum the module of the energy difference */
+					rate += fabs ((supply->priv->energy_old_timespec[i].tv_sec - now.tv_sec) * 
+						      (energy - supply->priv->energy_old[i]));
+				}
+		}
 
-	/* get the time difference */
-	g_get_current_time (&now);
-	time_s = now.tv_sec - supply->priv->energy_old_timespec.tv_sec;
-	if (time_s == 0)
+	if (sum_x == 0)
 		return supply->priv->rate_old;
 
-	/* get the difference in charge */
-	energy = fabs (supply->priv->energy_old - energy);
-	if (energy < 0.1f)
+	/* Compute the discharge per hour, and not per second */
+	rate /= sum_x / 3600.0;
+
+	/* If the rate is zero, use the old rate. It will usually happens if no data
+	 * are in the buffer yet. If the rate is too high, i.e. more than 100W, 
+	 * don't use it. */
+	if (rate == 0 || rate > 100)
 		return supply->priv->rate_old;
 
-	/* probably okay */
-	return energy * 3600.0 / time_s;
+	return rate;
 }
 
 /**
@@ -719,17 +786,21 @@ up_device_supply_refresh_battery (UpDeviceSupply *supply)
 	if (time_to_full > (20 * 60 * 60))
 		time_to_full = 0;
 
-	/* set the old status if it hasn't changed */
-	if (supply->priv->energy_old != energy) {
-		supply->priv->energy_old = energy;
+	/* Check if the energy value has changed and, if that's the case,
+	 * store the new values in the buffer. */ 
+	if (up_device_supply_push_new_energy (supply, energy))
 		supply->priv->rate_old = energy_rate;
-		g_get_current_time (&supply->priv->energy_old_timespec);
-	}
 
 	/* we changed state */
 	g_object_get (device, "state", &old_state, NULL);
 	if (old_state != state)
-		supply->priv->energy_old = 0;
+		{
+			for (i = 0; i < UP_DEVICE_SUPPLY_ENERGY_OLD_LENGTH; ++i)
+				{
+					supply->priv->energy_old[i] = 0;
+					supply->priv->energy_old_timespec[i].tv_sec = 0;
+				}
+		}
 
 	g_object_set (device,
 		      "energy", energy,
@@ -919,6 +990,10 @@ up_device_supply_init (UpDeviceSupply *supply)
 	supply->priv->unknown_retries = 0;
 	supply->priv->poll_timer_id = 0;
 	supply->priv->enable_poll = TRUE;
+
+	/* Allocate the space for the stats on the battery charging - discharging */
+	supply->priv->energy_old = g_malloc_n (UP_DEVICE_SUPPLY_ENERGY_OLD_LENGTH, sizeof (gdouble));
+	supply->priv->energy_old_timespec = g_malloc_n (UP_DEVICE_SUPPLY_ENERGY_OLD_LENGTH, sizeof (GTimeVal));
 }
 
 /**
@@ -938,6 +1013,9 @@ up_device_supply_finalize (GObject *object)
 	if (supply->priv->poll_timer_id > 0)
 		g_source_remove (supply->priv->poll_timer_id);
 
+	g_free (supply->priv->energy_old);
+	g_free (supply->priv->energy_old_timespec);
+
 	G_OBJECT_CLASS (up_device_supply_parent_class)->finalize (object);
 }
 

Attachment: signature.asc
Description: OpenPGP digital signature

_______________________________________________
devkit-devel mailing list
devkit-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/devkit-devel

Reply via email to