[PATCH BlueZ v3] battery: improve the display of the charge level

[PATCH BlueZ v3] battery: improve the display of the charge level

[Roman Smirnov]

The battery charge level may fluctuate due to uncalibrated
sensors. Commit smooths out such fluctuations.

The algorithm for determining uncalibrated sensors consists of
finding the number of changes in charge direction (i.e., "spikes").
If the number of spikes is zero, the device is charging or discharging.
If there is one spike, it may mean that the device has started charging
or has been disconnected from charging. If there are two or more spikes,
this is a clear indication of an uncalibrated sensor.

Check that the battery charge is fluctuating. If the battery charge
is fluctuating, use the average charge value, otherwise use the current
value.

Fixes: https://github.com/bluez/bluez/issues/1612
---
V2 -> V3: A queue is used instead of an array for the last charges,
a bug with the spikes variable increment has been fixed, and the
fluctuation check is called each time a new battery charge appears.

V1 -> V2: Smoothing is only applied to uncalibrated sensors, the
last 8 values are saved instead of 4, and the average value is used
for smoothing instead of the minimum value.

 src/battery.c | 70 +++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 70 insertions(+)

diff --git a/src/battery.c b/src/battery.c
index 59e4fc570..33079975c 100644
--- a/src/battery.c
+++ b/src/battery.c
@@ -33,10 +33,15 @@
 #define BATTERY_PROVIDER_MANAGER_INTERFACE "org.bluez.BatteryProviderManager1"
 
 #define BATTERY_MAX_PERCENTAGE 100
+#define LAST_CHARGES_SIZE 8
+#define MAX_CHARGE_STEP 5
 
 struct btd_battery {
 	char *path; /* D-Bus object path */
 	uint8_t percentage; /* valid between 0 to 100 inclusively */
+	struct queue *last_charges; /* last charges received */
+	float avg_charge; /* average battery charge */
+	bool is_fluctuating; /* true, if the battery sensor fluctuates */
 	char *source; /* Descriptive source of the battery info */
 	char *provider_path; /* The provider root path, if any */
 };
@@ -92,6 +97,10 @@ static struct btd_battery *battery_new(const char *path, const char *source,
 	battery = new0(struct btd_battery, 1);
 	battery->path = g_strdup(path);
 	battery->percentage = UINT8_MAX;
+	battery->last_charges = queue_new();
+	battery->avg_charge = 0;
+	battery->is_fluctuating = false;
+
 	if (source)
 		battery->source = g_strdup(source);
 	if (provider_path)
@@ -105,6 +114,9 @@ static void battery_free(struct btd_battery *battery)
 	if (battery->path)
 		g_free(battery->path);
 
+	if (battery->last_charges)
+		queue_destroy(battery->last_charges, NULL);
+
 	if (battery->source)
 		g_free(battery->source);
 
@@ -217,8 +229,49 @@ bool btd_battery_unregister(struct btd_battery *battery)
 	return true;
 }
 
+static void check_fluctuations(struct btd_battery *battery)
+{
+	const struct queue_entry *entry;
+	uint8_t spikes = 0;
+	int8_t step;
+	int8_t direction = 0;
+	int8_t prev_direction;
+	uint8_t *prev_charge;
+	uint8_t *next_charge;
+
+	for (entry = queue_get_entries(battery->last_charges); entry->next;
+	     entry = entry->next) {
+		prev_direction = direction;
+		prev_charge = entry->data;
+		next_charge = entry->next->data;
+		step = *next_charge - *prev_charge;
+
+		/*
+		 * The battery charge fluctuates too much,
+		 * which may indicate a battery problem, so
+		 * the actual value should be displayed.
+		 */
+		if (abs(step) >= MAX_CHARGE_STEP) {
+			battery->is_fluctuating = false;
+			return;
+		}
+
+		if (step > 0)
+			direction = 1;
+		else if (step < 0)
+			direction = -1;
+
+		if (direction != prev_direction && prev_direction)
+			spikes++;
+	}
+
+	battery->is_fluctuating = (spikes > 1) ? true : false;
+}
+
 bool btd_battery_update(struct btd_battery *battery, uint8_t percentage)
 {
+	uint8_t *p_percentage;
+
 	DBG("path = %s", battery->path);
 
 	if (!queue_find(batteries, NULL, battery)) {
@@ -231,6 +284,23 @@ bool btd_battery_update(struct btd_battery *battery, uint8_t percentage)
 		return false;
 	}
 
+	if (!battery->avg_charge)
+		battery->avg_charge = percentage;
+
+	/* exponential smoothing */
+	battery->avg_charge = battery->avg_charge * 0.7 + percentage * 0.3;
+	p_percentage = new0(uint8_t, 1);
+	*p_percentage = percentage;
+	queue_push_tail(battery->last_charges, p_percentage);
+
+	if (queue_length(battery->last_charges) == LAST_CHARGES_SIZE) {
+		check_fluctuations(battery);
+		queue_pop_head(battery->last_charges);
+	}
+
+	if (battery->is_fluctuating)
+		percentage = battery->avg_charge;
+
 	if (battery->percentage == percentage)
 		return true;
 
-- 
2.43.0

Re: [PATCH BlueZ v3] battery: improve the display of the charge level

[Bastien Nocera]

Hey,

Going to make a few comments inline. Those would be in addition to
Luiz' comments, and not meant to replace them.

On Tue, 2025-10-28 at 17:43 +0300, Roman Smirnov wrote:
> The battery charge level may fluctuate due to uncalibrated
> sensors. Commit smooths out such fluctuations.
> 
> The algorithm for determining uncalibrated sensors consists of
> finding the number of changes in charge direction (i.e., "spikes").
> If the number of spikes is zero, the device is charging or
> discharging.
> If there is one spike, it may mean that the device has started
> charging
> or has been disconnected from charging. If there are two or more
> spikes,
> this is a clear indication of an uncalibrated sensor.
> 
> Check that the battery charge is fluctuating. If the battery charge
> is fluctuating, use the average charge value, otherwise use the
> current
> value.
> 
> Fixes: https://github.com/bluez/bluez/issues/1612
> ---
> V2 -> V3: A queue is used instead of an array for the last charges,
> a bug with the spikes variable increment has been fixed, and the
> fluctuation check is called each time a new battery charge appears.
> 
> V1 -> V2: Smoothing is only applied to uncalibrated sensors, the
> last 8 values are saved instead of 4, and the average value is used
> for smoothing instead of the minimum value.
> 
>  src/battery.c | 70
> +++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 70 insertions(+)
> 
> diff --git a/src/battery.c b/src/battery.c
> index 59e4fc570..33079975c 100644
> --- a/src/battery.c
> +++ b/src/battery.c
> @@ -33,10 +33,15 @@
>  #define BATTERY_PROVIDER_MANAGER_INTERFACE
> "org.bluez.BatteryProviderManager1"
>  
>  #define BATTERY_MAX_PERCENTAGE 100
> +#define LAST_CHARGES_SIZE 8
> +#define MAX_CHARGE_STEP 5
>  
>  struct btd_battery {
>  	char *path; /* D-Bus object path */
>  	uint8_t percentage; /* valid between 0 to 100 inclusively */
> +	struct queue *last_charges; /* last charges received */

Instead of open-coding a queue, I think that a GArray would be better:
https://docs.gtk.org/glib/struct.Array.html

- limit ->len to LAST_CHARGES_SIZE
- ability to add items from either side, truncate the queue or remove
an arbitrary item

> +	float avg_charge; /* average battery charge */
> +	bool is_fluctuating; /* true, if the battery sensor
> fluctuates */
>  	char *source; /* Descriptive source of the battery info */
>  	char *provider_path; /* The provider root path, if any */
>  };
> @@ -92,6 +97,10 @@ static struct btd_battery *battery_new(const char
> *path, const char *source,
>  	battery = new0(struct btd_battery, 1);
>  	battery->path = g_strdup(path);
>  	battery->percentage = UINT8_MAX;
> +	battery->last_charges = queue_new();
> +	battery->avg_charge = 0;
> +	battery->is_fluctuating = false;
> +
>  	if (source)
>  		battery->source = g_strdup(source);
>  	if (provider_path)
> @@ -105,6 +114,9 @@ static void battery_free(struct btd_battery
> *battery)
>  	if (battery->path)
>  		g_free(battery->path);
>  
> +	if (battery->last_charges)
> +		queue_destroy(battery->last_charges, NULL);
> +
>  	if (battery->source)
>  		g_free(battery->source);
>  
> @@ -217,8 +229,49 @@ bool btd_battery_unregister(struct btd_battery
> *battery)
>  	return true;
>  }
>  
> +static void check_fluctuations(struct btd_battery *battery)

Instead of having this function, and quite complicated handling of that
same queue of battery levels in btd_battery_update(), it would be great
if the code was contained all within a function (or two) and used non-
BlueZ specific data types.

So that the code could be split off into its own battery helper, and
could have a unit test showing a few different cases.

> +{
> +	const struct queue_entry *entry;
> +	uint8_t spikes = 0;
> +	int8_t step;
> +	int8_t direction = 0;
> +	int8_t prev_direction;
> +	uint8_t *prev_charge;
> +	uint8_t *next_charge;
> +
> +	for (entry = queue_get_entries(battery->last_charges);
> entry->next;
> +	     entry = entry->next) {
> +		prev_direction = direction;
> +		prev_charge = entry->data;
> +		next_charge = entry->next->data;
> +		step = *next_charge - *prev_charge;
> +
> +		/*
> +		 * The battery charge fluctuates too much,
> +		 * which may indicate a battery problem, so
> +		 * the actual value should be displayed.
> +		 */
> +		if (abs(step) >= MAX_CHARGE_STEP) {
> +			battery->is_fluctuating = false;
> +			return;
> +		}
> +
> +		if (step > 0)
> +			direction = 1;
> +		else if (step < 0)
> +			direction = -1;
> +
> +		if (direction != prev_direction && prev_direction)
> +			spikes++;
> +	}
> +
> +	battery->is_fluctuating = (spikes > 1) ? true : false;
> +}
> +
>  bool btd_battery_update(struct btd_battery *battery, uint8_t
> percentage)
>  {
> +	uint8_t *p_percentage;
> +
>  	DBG("path = %s", battery->path);
>  
>  	if (!queue_find(batteries, NULL, battery)) {
> @@ -231,6 +284,23 @@ bool btd_battery_update(struct btd_battery
> *battery, uint8_t percentage)
>  		return false;
>  	}
>  
> +	if (!battery->avg_charge)
> +		battery->avg_charge = percentage;
> +
> +	/* exponential smoothing */
> +	battery->avg_charge = battery->avg_charge * 0.7 + percentage
> * 0.3;

Magic numbers should be #define's constants.

As Luiz mentioned, it would be great if there was some prior art
referenced, perhaps the reference implementation in another
application.

Or an explanation as to why this could needs to live here instead of,
say, upower, which deals with heuristics, dodgy hardware, etc.

Cheers

> +	p_percentage = new0(uint8_t, 1);
> +	*p_percentage = percentage;
> +	queue_push_tail(battery->last_charges, p_percentage);
> +
> +	if (queue_length(battery->last_charges) ==
> LAST_CHARGES_SIZE) {
> +		check_fluctuations(battery);
> +		queue_pop_head(battery->last_charges);
> +	}
> +
> +	if (battery->is_fluctuating)
> +		percentage = battery->avg_charge;
> +
>  	if (battery->percentage == percentage)
>  		return true;
>  

Re: [PATCH BlueZ v3] battery: improve the display of the charge level

[Luiz Augusto von Dentz]

Hi Bastien,

On Tue, Oct 28, 2025 at 11:01 AM Bastien Nocera <hadess@hadess.net> wrote:
>
> Hey,
>
> Going to make a few comments inline. Those would be in addition to
> Luiz' comments, and not meant to replace them.
>
> On Tue, 2025-10-28 at 17:43 +0300, Roman Smirnov wrote:
> > The battery charge level may fluctuate due to uncalibrated
> > sensors. Commit smooths out such fluctuations.
> >
> > The algorithm for determining uncalibrated sensors consists of
> > finding the number of changes in charge direction (i.e., "spikes").
> > If the number of spikes is zero, the device is charging or
> > discharging.
> > If there is one spike, it may mean that the device has started
> > charging
> > or has been disconnected from charging. If there are two or more
> > spikes,
> > this is a clear indication of an uncalibrated sensor.
> >
> > Check that the battery charge is fluctuating. If the battery charge
> > is fluctuating, use the average charge value, otherwise use the
> > current
> > value.
> >
> > Fixes: https://github.com/bluez/bluez/issues/1612
> > ---
> > V2 -> V3: A queue is used instead of an array for the last charges,
> > a bug with the spikes variable increment has been fixed, and the
> > fluctuation check is called each time a new battery charge appears.
> >
> > V1 -> V2: Smoothing is only applied to uncalibrated sensors, the
> > last 8 values are saved instead of 4, and the average value is used
> > for smoothing instead of the minimum value.
> >
> >  src/battery.c | 70
> > +++++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 70 insertions(+)
> >
> > diff --git a/src/battery.c b/src/battery.c
> > index 59e4fc570..33079975c 100644
> > --- a/src/battery.c
> > +++ b/src/battery.c
> > @@ -33,10 +33,15 @@
> >  #define BATTERY_PROVIDER_MANAGER_INTERFACE
> > "org.bluez.BatteryProviderManager1"
> >
> >  #define BATTERY_MAX_PERCENTAGE 100
> > +#define LAST_CHARGES_SIZE 8
> > +#define MAX_CHARGE_STEP 5
> >
> >  struct btd_battery {
> >       char *path; /* D-Bus object path */
> >       uint8_t percentage; /* valid between 0 to 100 inclusively */
> > +     struct queue *last_charges; /* last charges received */
>
> Instead of open-coding a queue, I think that a GArray would be better:
> https://docs.gtk.org/glib/struct.Array.html

We don't recommend using glib specific structures on new code, we
don't want new dependencies even if it is already supported on the
required version.

> - limit ->len to LAST_CHARGES_SIZE
> - ability to add items from either side, truncate the queue or remove
> an arbitrary item
>
> > +     float avg_charge; /* average battery charge */
> > +     bool is_fluctuating; /* true, if the battery sensor
> > fluctuates */
> >       char *source; /* Descriptive source of the battery info */
> >       char *provider_path; /* The provider root path, if any */
> >  };
> > @@ -92,6 +97,10 @@ static struct btd_battery *battery_new(const char
> > *path, const char *source,
> >       battery = new0(struct btd_battery, 1);
> >       battery->path = g_strdup(path);
> >       battery->percentage = UINT8_MAX;
> > +     battery->last_charges = queue_new();
> > +     battery->avg_charge = 0;
> > +     battery->is_fluctuating = false;
> > +
> >       if (source)
> >               battery->source = g_strdup(source);
> >       if (provider_path)
> > @@ -105,6 +114,9 @@ static void battery_free(struct btd_battery
> > *battery)
> >       if (battery->path)
> >               g_free(battery->path);
> >
> > +     if (battery->last_charges)
> > +             queue_destroy(battery->last_charges, NULL);
> > +
> >       if (battery->source)
> >               g_free(battery->source);
> >
> > @@ -217,8 +229,49 @@ bool btd_battery_unregister(struct btd_battery
> > *battery)
> >       return true;
> >  }
> >
> > +static void check_fluctuations(struct btd_battery *battery)
>
> Instead of having this function, and quite complicated handling of that
> same queue of battery levels in btd_battery_update(), it would be great
> if the code was contained all within a function (or two) and used non-
> BlueZ specific data types.
>
> So that the code could be split off into its own battery helper, and
> could have a unit test showing a few different cases.

I guess you are suggesting something to go into src/shared (e.g.
battery.c:bt_battery) so it can be unit tested, while I think this
would be nice to have Id leave this to Roman to decide since it may
require some work to put it together and then generate test cases that
cover fluctuation and other things we might want to check with the
code.

> > +{
> > +     const struct queue_entry *entry;
> > +     uint8_t spikes = 0;
> > +     int8_t step;
> > +     int8_t direction = 0;
> > +     int8_t prev_direction;
> > +     uint8_t *prev_charge;
> > +     uint8_t *next_charge;
> > +
> > +     for (entry = queue_get_entries(battery->last_charges);
> > entry->next;
> > +          entry = entry->next) {
> > +             prev_direction = direction;
> > +             prev_charge = entry->data;
> > +             next_charge = entry->next->data;
> > +             step = *next_charge - *prev_charge;

It might be a good idea to store the values as pointers (using
UINT_TO_PTR/PTR_TO_UINT), that way we avoid this kind of construct
above where you have to use pointers and risk having NULL pointers
bugs for example.

> > +
> > +             /*
> > +              * The battery charge fluctuates too much,
> > +              * which may indicate a battery problem, so
> > +              * the actual value should be displayed.
> > +              */
> > +             if (abs(step) >= MAX_CHARGE_STEP) {
> > +                     battery->is_fluctuating = false;
> > +                     return;
> > +             }
> > +
> > +             if (step > 0)
> > +                     direction = 1;
> > +             else if (step < 0)
> > +                     direction = -1;
> > +
> > +             if (direction != prev_direction && prev_direction)
> > +                     spikes++;
> > +     }
> > +
> > +     battery->is_fluctuating = (spikes > 1) ? true : false;
> > +}
> > +
> >  bool btd_battery_update(struct btd_battery *battery, uint8_t
> > percentage)
> >  {
> > +     uint8_t *p_percentage;
> > +
> >       DBG("path = %s", battery->path);
> >
> >       if (!queue_find(batteries, NULL, battery)) {
> > @@ -231,6 +284,23 @@ bool btd_battery_update(struct btd_battery
> > *battery, uint8_t percentage)
> >               return false;
> >       }
> >
> > +     if (!battery->avg_charge)
> > +             battery->avg_charge = percentage;
> > +
> > +     /* exponential smoothing */
> > +     battery->avg_charge = battery->avg_charge * 0.7 + percentage
> > * 0.3;
>
> Magic numbers should be #define's constants.
>
> As Luiz mentioned, it would be great if there was some prior art
> referenced, perhaps the reference implementation in another
> application.
>
> Or an explanation as to why this could needs to live here instead of,
> say, upower, which deals with heuristics, dodgy hardware, etc.

This is a good question, what is the algorithm upower used for
handling the battery level?

-- 
Luiz Augusto von Dentz

RE: [BlueZ,v3] battery: improve the display of the charge level

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 1262 bytes --]

This is automated email and please do not reply to this email!

Dear submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=1016771

---Test result---

Test Summary:
CheckPatch                    PENDING   0.30 seconds
GitLint                       PENDING   0.31 seconds
BuildEll                      PASS      20.97 seconds
BluezMake                     PASS      2726.05 seconds
MakeCheck                     PASS      20.53 seconds
MakeDistcheck                 PASS      191.62 seconds
CheckValgrind                 PASS      247.18 seconds
CheckSmatch                   PASS      318.21 seconds
bluezmakeextell               PASS      132.10 seconds
IncrementalBuild              PENDING   0.33 seconds
ScanBuild                     PASS      946.88 seconds

Details
##############################
Test: CheckPatch - PENDING
Desc: Run checkpatch.pl script
Output:

##############################
Test: GitLint - PENDING
Desc: Run gitlint
Output:

##############################
Test: IncrementalBuild - PENDING
Desc: Incremental build with the patches in the series
Output:



---
Regards,
Linux Bluetooth

Re: [PATCH BlueZ v3] battery: improve the display of the charge level

[Roman Smirnov]

On Tue, 2025-10-28 at 11:47 -0400, Luiz Augusto von Dentz wrote:
> Hi Bastien,
> 
> On Tue, Oct 28, 2025 at 11:01 AM Bastien Nocera <hadess@hadess.net> wrote:
> > 
> > Hey,
> > 
> > Going to make a few comments inline. Those would be in addition to
> > Luiz' comments, and not meant to replace them.
> > 
> > On Tue, 2025-10-28 at 17:43 +0300, Roman Smirnov wrote:
> > > The battery charge level may fluctuate due to uncalibrated
> > > sensors. Commit smooths out such fluctuations.
> > > 
> > > The algorithm for determining uncalibrated sensors consists of
> > > finding the number of changes in charge direction (i.e., "spikes").
> > > If the number of spikes is zero, the device is charging or
> > > discharging.
> > > If there is one spike, it may mean that the device has started
> > > charging
> > > or has been disconnected from charging. If there are two or more
> > > spikes,
> > > this is a clear indication of an uncalibrated sensor.
> > > 
> > > Check that the battery charge is fluctuating. If the battery charge
> > > is fluctuating, use the average charge value, otherwise use the
> > > current
> > > value.
> > > 
> > > Fixes: https://github.com/bluez/bluez/issues/1612
> > > ---
> > > V2 -> V3: A queue is used instead of an array for the last charges,
> > > a bug with the spikes variable increment has been fixed, and the
> > > fluctuation check is called each time a new battery charge appears.
> > > 
> > > V1 -> V2: Smoothing is only applied to uncalibrated sensors, the
> > > last 8 values are saved instead of 4, and the average value is used
> > > for smoothing instead of the minimum value.
> > > 
> > >  src/battery.c | 70
> > > +++++++++++++++++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 70 insertions(+)
> > > 
> > > diff --git a/src/battery.c b/src/battery.c
> > > index 59e4fc570..33079975c 100644
> > > --- a/src/battery.c
> > > +++ b/src/battery.c
> > > @@ -33,10 +33,15 @@
> > >  #define BATTERY_PROVIDER_MANAGER_INTERFACE
> > > "org.bluez.BatteryProviderManager1"
> > > 
> > >  #define BATTERY_MAX_PERCENTAGE 100
> > > +#define LAST_CHARGES_SIZE 8
> > > +#define MAX_CHARGE_STEP 5
> > > 
> > >  struct btd_battery {
> > >       char *path; /* D-Bus object path */
> > >       uint8_t percentage; /* valid between 0 to 100 inclusively */
> > > +     struct queue *last_charges; /* last charges received */
> > 
> > Instead of open-coding a queue, I think that a GArray would be better:
> > https://docs.gtk.org/glib/struct.Array.html
> 
> We don't recommend using glib specific structures on new code, we
> don't want new dependencies even if it is already supported on the
> required version.
> 
> > - limit ->len to LAST_CHARGES_SIZE
> > - ability to add items from either side, truncate the queue or remove
> > an arbitrary item
> > 
> > > +     float avg_charge; /* average battery charge */
> > > +     bool is_fluctuating; /* true, if the battery sensor
> > > fluctuates */
> > >       char *source; /* Descriptive source of the battery info */
> > >       char *provider_path; /* The provider root path, if any */
> > >  };
> > > @@ -92,6 +97,10 @@ static struct btd_battery *battery_new(const char
> > > *path, const char *source,
> > >       battery = new0(struct btd_battery, 1);
> > >       battery->path = g_strdup(path);
> > >       battery->percentage = UINT8_MAX;
> > > +     battery->last_charges = queue_new();
> > > +     battery->avg_charge = 0;
> > > +     battery->is_fluctuating = false;
> > > +
> > >       if (source)
> > >               battery->source = g_strdup(source);
> > >       if (provider_path)
> > > @@ -105,6 +114,9 @@ static void battery_free(struct btd_battery
> > > *battery)
> > >       if (battery->path)
> > >               g_free(battery->path);
> > > 
> > > +     if (battery->last_charges)
> > > +             queue_destroy(battery->last_charges, NULL);
> > > +
> > >       if (battery->source)
> > >               g_free(battery->source);
> > > 
> > > @@ -217,8 +229,49 @@ bool btd_battery_unregister(struct btd_battery
> > > *battery)
> > >       return true;
> > >  }
> > > 
> > > +static void check_fluctuations(struct btd_battery *battery)
> > 
> > Instead of having this function, and quite complicated handling of that
> > same queue of battery levels in btd_battery_update(), it would be great
> > if the code was contained all within a function (or two) and used non-
> > BlueZ specific data types.
> > 
> > So that the code could be split off into its own battery helper, and
> > could have a unit test showing a few different cases.
> 
> I guess you are suggesting something to go into src/shared (e.g.
> battery.c:bt_battery) so it can be unit tested, while I think this
> would be nice to have Id leave this to Roman to decide since it may
> require some work to put it together and then generate test cases that
> cover fluctuation and other things we might want to check with the
> code.
> 
> > > +{
> > > +     const struct queue_entry *entry;
> > > +     uint8_t spikes = 0;
> > > +     int8_t step;
> > > +     int8_t direction = 0;
> > > +     int8_t prev_direction;
> > > +     uint8_t *prev_charge;
> > > +     uint8_t *next_charge;
> > > +
> > > +     for (entry = queue_get_entries(battery->last_charges);
> > > entry->next;
> > > +          entry = entry->next) {
> > > +             prev_direction = direction;
> > > +             prev_charge = entry->data;
> > > +             next_charge = entry->next->data;
> > > +             step = *next_charge - *prev_charge;
> 
> It might be a good idea to store the values as pointers (using
> UINT_TO_PTR/PTR_TO_UINT), that way we avoid this kind of construct
> above where you have to use pointers and risk having NULL pointers
> bugs for example.
> 
> > > +
> > > +             /*
> > > +              * The battery charge fluctuates too much,
> > > +              * which may indicate a battery problem, so
> > > +              * the actual value should be displayed.
> > > +              */
> > > +             if (abs(step) >= MAX_CHARGE_STEP) {
> > > +                     battery->is_fluctuating = false;
> > > +                     return;
> > > +             }
> > > +
> > > +             if (step > 0)
> > > +                     direction = 1;
> > > +             else if (step < 0)
> > > +                     direction = -1;
> > > +
> > > +             if (direction != prev_direction && prev_direction)
> > > +                     spikes++;
> > > +     }
> > > +
> > > +     battery->is_fluctuating = (spikes > 1) ? true : false;
> > > +}
> > > +
> > >  bool btd_battery_update(struct btd_battery *battery, uint8_t
> > > percentage)
> > >  {
> > > +     uint8_t *p_percentage;
> > > +
> > >       DBG("path = %s", battery->path);
> > > 
> > >       if (!queue_find(batteries, NULL, battery)) {
> > > @@ -231,6 +284,23 @@ bool btd_battery_update(struct btd_battery
> > > *battery, uint8_t percentage)
> > >               return false;
> > >       }
> > > 
> > > +     if (!battery->avg_charge)
> > > +             battery->avg_charge = percentage;
> > > +
> > > +     /* exponential smoothing */
> > > +     battery->avg_charge = battery->avg_charge * 0.7 + percentage
> > > * 0.3;
> > 
> > Magic numbers should be #define's constants.
> > 
> > As Luiz mentioned, it would be great if there was some prior art
> > referenced, perhaps the reference implementation in another
> > application.
> > 
> > Or an explanation as to why this could needs to live here instead of,
> > say, upower, which deals with heuristics, dodgy hardware, etc.
> 
> This is a good question, what is the algorithm upower used for
> handling the battery level?
> 
I checked how upower works, and it does not handle such cases. For
example, here the charge percentage is taken directly from sysfs:
Link: https://gitlab.freedesktop.org/upower/upower/-/blob/master/src/linux/up-device-supply-battery.c?ref_type=heads#L514

If the battery does not send the charge percentage to sysfs, upower
calculates it based on the energy.cur variable, which is also read
from sysfs:
Link: https://gitlab.freedesktop.org/upower/upower/-/blob/master/src/up-device-battery.c?ref_type=heads#L316
Link: https://gitlab.freedesktop.org/upower/upower/-/blob/master/src/linux/up-device-supply-battery.c?ref_type=heads#L493

The only place where the accumulated statistics are used is here:
Link: https://gitlab.freedesktop.org/upower/upower/-/blob/master/src/up-device-battery.c?ref_type=heads#L99
However, I didn't find anything interesting here either.

In my opinion, charge level averaging should be implemented in bluez,
as there are applications that access bluez directly:
Link: https://github.com/KDE/bluez-qt/blob/master/src/battery.cpp#L30
Therefore, it would be better to filter the values on the bluez side
and then pass them on to third-party applications. This way, applications
will not have to implement the same logic over and over again.

The use of the average battery value is justified by the following
examples from the Linux kernel:
Link: https://elixir.bootlin.com/linux/v6.18-rc3/source/drivers/power/supply/adc-battery-helper.c#L132
Link: https://elixir.bootlin.com/linux/v6.17.4/source/drivers/power/supply/cw2015_battery.c#L306

Re: [PATCH BlueZ v3] battery: improve the display of the charge level

[Bastien Nocera]

On Tue, 2025-10-28 at 11:47 -0400, Luiz Augusto von Dentz wrote:
> Hi Bastien,
> 
> On Tue, Oct 28, 2025 at 11:01 AM Bastien Nocera <hadess@hadess.net>
> wrote:
> > 
> > Hey,
> > 
> > Going to make a few comments inline. Those would be in addition to
> > Luiz' comments, and not meant to replace them.
> > 
> > On Tue, 2025-10-28 at 17:43 +0300, Roman Smirnov wrote:
> > > The battery charge level may fluctuate due to uncalibrated
> > > sensors. Commit smooths out such fluctuations.
> > > 
> > > The algorithm for determining uncalibrated sensors consists of
> > > finding the number of changes in charge direction (i.e.,
> > > "spikes").
> > > If the number of spikes is zero, the device is charging or
> > > discharging.
> > > If there is one spike, it may mean that the device has started
> > > charging
> > > or has been disconnected from charging. If there are two or more
> > > spikes,
> > > this is a clear indication of an uncalibrated sensor.
> > > 
> > > Check that the battery charge is fluctuating. If the battery
> > > charge
> > > is fluctuating, use the average charge value, otherwise use the
> > > current
> > > value.
> > > 
> > > Fixes: https://github.com/bluez/bluez/issues/1612
> > > ---
> > > V2 -> V3: A queue is used instead of an array for the last
> > > charges,
> > > a bug with the spikes variable increment has been fixed, and the
> > > fluctuation check is called each time a new battery charge
> > > appears.
> > > 
> > > V1 -> V2: Smoothing is only applied to uncalibrated sensors, the
> > > last 8 values are saved instead of 4, and the average value is
> > > used
> > > for smoothing instead of the minimum value.
> > > 
> > >  src/battery.c | 70
> > > +++++++++++++++++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 70 insertions(+)
> > > 
> > > diff --git a/src/battery.c b/src/battery.c
> > > index 59e4fc570..33079975c 100644
> > > --- a/src/battery.c
> > > +++ b/src/battery.c
> > > @@ -33,10 +33,15 @@
> > >  #define BATTERY_PROVIDER_MANAGER_INTERFACE
> > > "org.bluez.BatteryProviderManager1"
> > > 
> > >  #define BATTERY_MAX_PERCENTAGE 100
> > > +#define LAST_CHARGES_SIZE 8
> > > +#define MAX_CHARGE_STEP 5
> > > 
> > >  struct btd_battery {
> > >       char *path; /* D-Bus object path */
> > >       uint8_t percentage; /* valid between 0 to 100 inclusively
> > > */
> > > +     struct queue *last_charges; /* last charges received */
> > 
> > Instead of open-coding a queue, I think that a GArray would be
> > better:
> > https://docs.gtk.org/glib/struct.Array.html
> 
> We don't recommend using glib specific structures on new code, we
> don't want new dependencies even if it is already supported on the
> required version.

OK, that's a shame because that might make some things easier. Are we
looking to replace existing glib data types with more generic C
datatypes, and maybe sprinkle some modern C on top of that, a-la
systemd?

It has FOREACH() loops for a lot of container-types, cleanup functions,
etc.

> > - limit ->len to LAST_CHARGES_SIZE
> > - ability to add items from either side, truncate the queue or
> > remove
> > an arbitrary item
> > 
> > > +     float avg_charge; /* average battery charge */
> > > +     bool is_fluctuating; /* true, if the battery sensor
> > > fluctuates */
> > >       char *source; /* Descriptive source of the battery info */
> > >       char *provider_path; /* The provider root path, if any */
> > >  };
> > > @@ -92,6 +97,10 @@ static struct btd_battery *battery_new(const
> > > char
> > > *path, const char *source,
> > >       battery = new0(struct btd_battery, 1);
> > >       battery->path = g_strdup(path);
> > >       battery->percentage = UINT8_MAX;
> > > +     battery->last_charges = queue_new();
> > > +     battery->avg_charge = 0;
> > > +     battery->is_fluctuating = false;
> > > +
> > >       if (source)
> > >               battery->source = g_strdup(source);
> > >       if (provider_path)
> > > @@ -105,6 +114,9 @@ static void battery_free(struct btd_battery
> > > *battery)
> > >       if (battery->path)
> > >               g_free(battery->path);
> > > 
> > > +     if (battery->last_charges)
> > > +             queue_destroy(battery->last_charges, NULL);
> > > +
> > >       if (battery->source)
> > >               g_free(battery->source);
> > > 
> > > @@ -217,8 +229,49 @@ bool btd_battery_unregister(struct
> > > btd_battery
> > > *battery)
> > >       return true;
> > >  }
> > > 
> > > +static void check_fluctuations(struct btd_battery *battery)
> > 
> > Instead of having this function, and quite complicated handling of
> > that
> > same queue of battery levels in btd_battery_update(), it would be
> > great
> > if the code was contained all within a function (or two) and used
> > non-
> > BlueZ specific data types.
> > 
> > So that the code could be split off into its own battery helper,
> > and
> > could have a unit test showing a few different cases.
> 
> I guess you are suggesting something to go into src/shared (e.g.
> battery.c:bt_battery) so it can be unit tested, while I think this
> would be nice to have Id leave this to Roman to decide since it may
> require some work to put it together and then generate test cases
> that
> cover fluctuation and other things we might want to check with the
> code.

Testing fluctuations is one thing, testing that we don't access the
array out-of-bounds was what I was really interested in.
7
> > > +{
> > > +     const struct queue_entry *entry;
> > > +     uint8_t spikes = 0;
> > > +     int8_t step;
> > > +     int8_t direction = 0;
> > > +     int8_t prev_direction;
> > > +     uint8_t *prev_charge;
> > > +     uint8_t *next_charge;
> > > +
> > > +     for (entry = queue_get_entries(battery->last_charges);
> > > entry->next;
> > > +          entry = entry->next) {
> > > +             prev_direction = direction;
> > > +             prev_charge = entry->data;
> > > +             next_charge = entry->next->data;
> > > +             step = *next_charge - *prev_charge;
> 
> It might be a good idea to store the values as pointers (using
> UINT_TO_PTR/PTR_TO_UINT), that way we avoid this kind of construct
> above where you have to use pointers and risk having NULL pointers
> bugs for example.
> 
> > > +
> > > +             /*
> > > +              * The battery charge fluctuates too much,
> > > +              * which may indicate a battery problem, so
> > > +              * the actual value should be displayed.
> > > +              */
> > > +             if (abs(step) >= MAX_CHARGE_STEP) {
> > > +                     battery->is_fluctuating = false;
> > > +                     return;
> > > +             }
> > > +
> > > +             if (step > 0)
> > > +                     direction = 1;
> > > +             else if (step < 0)
> > > +                     direction = -1;
> > > +
> > > +             if (direction != prev_direction && prev_direction)
> > > +                     spikes++;
> > > +     }
> > > +
> > > +     battery->is_fluctuating = (spikes > 1) ? true : false;
> > > +}
> > > +
> > >  bool btd_battery_update(struct btd_battery *battery, uint8_t
> > > percentage)
> > >  {
> > > +     uint8_t *p_percentage;
> > > +
> > >       DBG("path = %s", battery->path);
> > > 
> > >       if (!queue_find(batteries, NULL, battery)) {
> > > @@ -231,6 +284,23 @@ bool btd_battery_update(struct btd_battery
> > > *battery, uint8_t percentage)
> > >               return false;
> > >       }
> > > 
> > > +     if (!battery->avg_charge)
> > > +             battery->avg_charge = percentage;
> > > +
> > > +     /* exponential smoothing */
> > > +     battery->avg_charge = battery->avg_charge * 0.7 +
> > > percentage
> > > * 0.3;
> > 
> > Magic numbers should be #define's constants.
> > 
> > As Luiz mentioned, it would be great if there was some prior art
> > referenced, perhaps the reference implementation in another
> > application.
> > 
> > Or an explanation as to why this could needs to live here instead
> > of,
> > say, upower, which deals with heuristics, dodgy hardware, etc.
> 
> This is a good question, what is the algorithm upower used for
> handling the battery level?

upower doesn't have any of that sort of thing for device batteries, but
main system batteries have a lot of work-arounds for broken battery
controllers/ECs.

Re: [PATCH BlueZ v3] battery: improve the display of the charge level

[Bastien Nocera]

On Sat, 2025-11-01 at 11:43 +0000, Roman Smirnov wrote:
> On Tue, 2025-10-28 at 11:47 -0400, Luiz Augusto von Dentz wrote:
> > Hi Bastien,
> > 
> > On Tue, Oct 28, 2025 at 11:01 AM Bastien Nocera <hadess@hadess.net>
> > wrote:
> > > 
> > > Hey,
> > > 
> > > Going to make a few comments inline. Those would be in addition
> > > to
> > > Luiz' comments, and not meant to replace them.
> > > 
> > > On Tue, 2025-10-28 at 17:43 +0300, Roman Smirnov wrote:
> > > > The battery charge level may fluctuate due to uncalibrated
> > > > sensors. Commit smooths out such fluctuations.
> > > > 
> > > > The algorithm for determining uncalibrated sensors consists of
> > > > finding the number of changes in charge direction (i.e.,
> > > > "spikes").
> > > > If the number of spikes is zero, the device is charging or
> > > > discharging.
> > > > If there is one spike, it may mean that the device has started
> > > > charging
> > > > or has been disconnected from charging. If there are two or
> > > > more
> > > > spikes,
> > > > this is a clear indication of an uncalibrated sensor.
> > > > 
> > > > Check that the battery charge is fluctuating. If the battery
> > > > charge
> > > > is fluctuating, use the average charge value, otherwise use the
> > > > current
> > > > value.
> > > > 
> > > > Fixes: https://github.com/bluez/bluez/issues/1612
> > > > ---
> > > > V2 -> V3: A queue is used instead of an array for the last
> > > > charges,
> > > > a bug with the spikes variable increment has been fixed, and
> > > > the
> > > > fluctuation check is called each time a new battery charge
> > > > appears.
> > > > 
> > > > V1 -> V2: Smoothing is only applied to uncalibrated sensors,
> > > > the
> > > > last 8 values are saved instead of 4, and the average value is
> > > > used
> > > > for smoothing instead of the minimum value.
> > > > 
> > > >  src/battery.c | 70
> > > > +++++++++++++++++++++++++++++++++++++++++++++++++++
> > > >  1 file changed, 70 insertions(+)
> > > > 
> > > > diff --git a/src/battery.c b/src/battery.c
> > > > index 59e4fc570..33079975c 100644
> > > > --- a/src/battery.c
> > > > +++ b/src/battery.c
> > > > @@ -33,10 +33,15 @@
> > > >  #define BATTERY_PROVIDER_MANAGER_INTERFACE
> > > > "org.bluez.BatteryProviderManager1"
> > > > 
> > > >  #define BATTERY_MAX_PERCENTAGE 100
> > > > +#define LAST_CHARGES_SIZE 8
> > > > +#define MAX_CHARGE_STEP 5
> > > > 
> > > >  struct btd_battery {
> > > >       char *path; /* D-Bus object path */
> > > >       uint8_t percentage; /* valid between 0 to 100 inclusively
> > > > */
> > > > +     struct queue *last_charges; /* last charges received */
> > > 
> > > Instead of open-coding a queue, I think that a GArray would be
> > > better:
> > > https://docs.gtk.org/glib/struct.Array.html
> > 
> > We don't recommend using glib specific structures on new code, we
> > don't want new dependencies even if it is already supported on the
> > required version.
> > 
> > > - limit ->len to LAST_CHARGES_SIZE
> > > - ability to add items from either side, truncate the queue or
> > > remove
> > > an arbitrary item
> > > 
> > > > +     float avg_charge; /* average battery charge */
> > > > +     bool is_fluctuating; /* true, if the battery sensor
> > > > fluctuates */
> > > >       char *source; /* Descriptive source of the battery info
> > > > */
> > > >       char *provider_path; /* The provider root path, if any */
> > > >  };
> > > > @@ -92,6 +97,10 @@ static struct btd_battery *battery_new(const
> > > > char
> > > > *path, const char *source,
> > > >       battery = new0(struct btd_battery, 1);
> > > >       battery->path = g_strdup(path);
> > > >       battery->percentage = UINT8_MAX;
> > > > +     battery->last_charges = queue_new();
> > > > +     battery->avg_charge = 0;
> > > > +     battery->is_fluctuating = false;
> > > > +
> > > >       if (source)
> > > >               battery->source = g_strdup(source);
> > > >       if (provider_path)
> > > > @@ -105,6 +114,9 @@ static void battery_free(struct btd_battery
> > > > *battery)
> > > >       if (battery->path)
> > > >               g_free(battery->path);
> > > > 
> > > > +     if (battery->last_charges)
> > > > +             queue_destroy(battery->last_charges, NULL);
> > > > +
> > > >       if (battery->source)
> > > >               g_free(battery->source);
> > > > 
> > > > @@ -217,8 +229,49 @@ bool btd_battery_unregister(struct
> > > > btd_battery
> > > > *battery)
> > > >       return true;
> > > >  }
> > > > 
> > > > +static void check_fluctuations(struct btd_battery *battery)
> > > 
> > > Instead of having this function, and quite complicated handling
> > > of that
> > > same queue of battery levels in btd_battery_update(), it would be
> > > great
> > > if the code was contained all within a function (or two) and used
> > > non-
> > > BlueZ specific data types.
> > > 
> > > So that the code could be split off into its own battery helper,
> > > and
> > > could have a unit test showing a few different cases.
> > 
> > I guess you are suggesting something to go into src/shared (e.g.
> > battery.c:bt_battery) so it can be unit tested, while I think this
> > would be nice to have Id leave this to Roman to decide since it may
> > require some work to put it together and then generate test cases
> > that
> > cover fluctuation and other things we might want to check with the
> > code.
> > 
> > > > +{
> > > > +     const struct queue_entry *entry;
> > > > +     uint8_t spikes = 0;
> > > > +     int8_t step;
> > > > +     int8_t direction = 0;
> > > > +     int8_t prev_direction;
> > > > +     uint8_t *prev_charge;
> > > > +     uint8_t *next_charge;
> > > > +
> > > > +     for (entry = queue_get_entries(battery->last_charges);
> > > > entry->next;
> > > > +          entry = entry->next) {
> > > > +             prev_direction = direction;
> > > > +             prev_charge = entry->data;
> > > > +             next_charge = entry->next->data;
> > > > +             step = *next_charge - *prev_charge;
> > 
> > It might be a good idea to store the values as pointers (using
> > UINT_TO_PTR/PTR_TO_UINT), that way we avoid this kind of construct
> > above where you have to use pointers and risk having NULL pointers
> > bugs for example.
> > 
> > > > +
> > > > +             /*
> > > > +              * The battery charge fluctuates too much,
> > > > +              * which may indicate a battery problem, so
> > > > +              * the actual value should be displayed.
> > > > +              */
> > > > +             if (abs(step) >= MAX_CHARGE_STEP) {
> > > > +                     battery->is_fluctuating = false;
> > > > +                     return;
> > > > +             }
> > > > +
> > > > +             if (step > 0)
> > > > +                     direction = 1;
> > > > +             else if (step < 0)
> > > > +                     direction = -1;
> > > > +
> > > > +             if (direction != prev_direction &&
> > > > prev_direction)
> > > > +                     spikes++;
> > > > +     }
> > > > +
> > > > +     battery->is_fluctuating = (spikes > 1) ? true : false;
> > > > +}
> > > > +
> > > >  bool btd_battery_update(struct btd_battery *battery, uint8_t
> > > > percentage)
> > > >  {
> > > > +     uint8_t *p_percentage;
> > > > +
> > > >       DBG("path = %s", battery->path);
> > > > 
> > > >       if (!queue_find(batteries, NULL, battery)) {
> > > > @@ -231,6 +284,23 @@ bool btd_battery_update(struct btd_battery
> > > > *battery, uint8_t percentage)
> > > >               return false;
> > > >       }
> > > > 
> > > > +     if (!battery->avg_charge)
> > > > +             battery->avg_charge = percentage;
> > > > +
> > > > +     /* exponential smoothing */
> > > > +     battery->avg_charge = battery->avg_charge * 0.7 +
> > > > percentage
> > > > * 0.3;
> > > 
> > > Magic numbers should be #define's constants.
> > > 
> > > As Luiz mentioned, it would be great if there was some prior art
> > > referenced, perhaps the reference implementation in another
> > > application.
> > > 
> > > Or an explanation as to why this could needs to live here instead
> > > of,
> > > say, upower, which deals with heuristics, dodgy hardware, etc.
> > 
> > This is a good question, what is the algorithm upower used for
> > handling the battery level?
> > 
> I checked how upower works, and it does not handle such cases. For
> example, here the charge percentage is taken directly from sysfs:
> Link:
> https://gitlab.freedesktop.org/upower/upower/-/blob/master/src/linux/up-device-supply-battery.c?ref_type=heads#L514
> 
> If the battery does not send the charge percentage to sysfs, upower
> calculates it based on the energy.cur variable, which is also read
> from sysfs:
> Link:
> https://gitlab.freedesktop.org/upower/upower/-/blob/master/src/up-device-battery.c?ref_type=heads#L316
> Link:
> https://gitlab.freedesktop.org/upower/upower/-/blob/master/src/linux/up-device-supply-battery.c?ref_type=heads#L493
> 
> The only place where the accumulated statistics are used is here:
> Link:
> https://gitlab.freedesktop.org/upower/upower/-/blob/master/src/up-device-battery.c?ref_type=heads#L99
> However, I didn't find anything interesting here either.

I don't think I said that upower already did that, just that it was
probably a better place to implement it.

> In my opinion, charge level averaging should be implemented in bluez,
> as there are applications that access bluez directly:
> Link: https://github.com/KDE/bluez-qt/blob/master/src/battery.cpp#L30
> Therefore, it would be better to filter the values on the bluez side
> and then pass them on to third-party applications. This way,
> applications
> will not have to implement the same logic over and over again.

Having spent quite a lot of time working on the battery support in
bluez, the corresponding code in upower and GNOME Bluetooth as the
front-end, I can tell you that you shouldn't be getting your battery
levels directly from bluez because:
- it requires bluez to actually know that battery level (so only
supported for some audio profiles and Bluetooth LE BAS, leaving out a
lot of HID Classic devices)
- sometimes the Bluetooth levels are out-of-date because the device has
a separate connection through, for example, USB duplicates.

On desktop Linux, upower is the one source of truth.

Anyway, I don't mind which way this goes, but IMO it needs to be rigged
for automated testing.

Cheers

> The use of the average battery value is justified by the following
> examples from the Linux kernel:
> Link:
> https://elixir.bootlin.com/linux/v6.18-rc3/source/drivers/power/supply/adc-battery-helper.c#L132
> Link:
> https://elixir.bootlin.com/linux/v6.17.4/source/drivers/power/supply/cw2015_battery.c#L306