* [PATCH BlueZ v3] battery: improve the display of the charge level @ 2025-10-28 14:43 Roman Smirnov 2025-10-28 14:57 ` Bastien Nocera 2025-10-28 16:06 ` [BlueZ,v3] " bluez.test.bot 0 siblings, 2 replies; 7+ messages in thread From: Roman Smirnov @ 2025-10-28 14:43 UTC (permalink / raw) To: linux-bluetooth; +Cc: 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH BlueZ v3] battery: improve the display of the charge level 2025-10-28 14:43 [PATCH BlueZ v3] battery: improve the display of the charge level Roman Smirnov @ 2025-10-28 14:57 ` Bastien Nocera 2025-10-28 15:47 ` Luiz Augusto von Dentz 2025-10-28 16:06 ` [BlueZ,v3] " bluez.test.bot 1 sibling, 1 reply; 7+ messages in thread From: Bastien Nocera @ 2025-10-28 14:57 UTC (permalink / raw) To: Roman Smirnov, linux-bluetooth 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; > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH BlueZ v3] battery: improve the display of the charge level 2025-10-28 14:57 ` Bastien Nocera @ 2025-10-28 15:47 ` Luiz Augusto von Dentz 2025-11-01 11:43 ` Roman Smirnov 2025-11-04 13:18 ` Bastien Nocera 0 siblings, 2 replies; 7+ messages in thread From: Luiz Augusto von Dentz @ 2025-10-28 15:47 UTC (permalink / raw) To: Bastien Nocera; +Cc: Roman Smirnov, linux-bluetooth 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH BlueZ v3] battery: improve the display of the charge level 2025-10-28 15:47 ` Luiz Augusto von Dentz @ 2025-11-01 11:43 ` Roman Smirnov 2025-11-04 15:54 ` Bastien Nocera 2025-11-04 13:18 ` Bastien Nocera 1 sibling, 1 reply; 7+ messages in thread From: Roman Smirnov @ 2025-11-01 11:43 UTC (permalink / raw) To: luiz.dentz@gmail.com, hadess@hadess.net; +Cc: linux-bluetooth@vger.kernel.org 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH BlueZ v3] battery: improve the display of the charge level 2025-11-01 11:43 ` Roman Smirnov @ 2025-11-04 15:54 ` Bastien Nocera 0 siblings, 0 replies; 7+ messages in thread From: Bastien Nocera @ 2025-11-04 15:54 UTC (permalink / raw) To: Roman Smirnov, luiz.dentz@gmail.com; +Cc: linux-bluetooth@vger.kernel.org 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH BlueZ v3] battery: improve the display of the charge level 2025-10-28 15:47 ` Luiz Augusto von Dentz 2025-11-01 11:43 ` Roman Smirnov @ 2025-11-04 13:18 ` Bastien Nocera 1 sibling, 0 replies; 7+ messages in thread From: Bastien Nocera @ 2025-11-04 13:18 UTC (permalink / raw) To: Luiz Augusto von Dentz; +Cc: Roman Smirnov, linux-bluetooth 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. ^ permalink raw reply [flat|nested] 7+ messages in thread
* RE: [BlueZ,v3] battery: improve the display of the charge level 2025-10-28 14:43 [PATCH BlueZ v3] battery: improve the display of the charge level Roman Smirnov 2025-10-28 14:57 ` Bastien Nocera @ 2025-10-28 16:06 ` bluez.test.bot 1 sibling, 0 replies; 7+ messages in thread From: bluez.test.bot @ 2025-10-28 16:06 UTC (permalink / raw) To: linux-bluetooth, r.smirnov [-- 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2025-11-04 15:58 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2025-10-28 14:43 [PATCH BlueZ v3] battery: improve the display of the charge level Roman Smirnov 2025-10-28 14:57 ` Bastien Nocera 2025-10-28 15:47 ` Luiz Augusto von Dentz 2025-11-01 11:43 ` Roman Smirnov 2025-11-04 15:54 ` Bastien Nocera 2025-11-04 13:18 ` Bastien Nocera 2025-10-28 16:06 ` [BlueZ,v3] " bluez.test.bot
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox