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

[Roman Smirnov <r.smirnov@omp.ru>]

On Mon, 2025-10-27 at 11:59 -0400, Luiz Augusto von Dentz wrote:
> Hi Roman,
> 
> On Mon, Oct 27, 2025 at 11:19 AM Roman Smirnov <r.smirnov@omp.ru> 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.
> 
> Is this method based on something used already? Or it is yet again
> something you came up with?
> 
I looked into the Linux kernel, where I found the use of the average
value instead of the minimum:
1. https://elixir.bootlin.com/linux/v6.17.4/source/drivers/power/supply/cw2015_battery.c#L306
2. https://elixir.bootlin.com/linux/v6.18-rc2/source/drivers/power/supply/adc-battery-helper.c#L121

I couldn't find an algorithm for determining uncalibrated sensors,
so I came up with my own.

> > Fixes: https://github.com/bluez/bluez/issues/1612
> > ---
> >  src/battery.c | 62 +++++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 62 insertions(+)
> > 
> > diff --git a/src/battery.c b/src/battery.c
> > index 59e4fc570..f97d9b8f3 100644
> > --- a/src/battery.c
> > +++ b/src/battery.c
> > @@ -33,10 +33,16 @@
> >  #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 */
> > +       uint8_t *last_charges; /* last charges received */
> > +       uint8_t lru_charge_id; /* oldest battery charge */
> > +       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 +98,11 @@ 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 = new0(uint8_t, LAST_CHARGES_SIZE);
> 
> I'd prefer to use a queue rather than an array for looking back.
> 
> > +       battery->lru_charge_id = 0;
> > +       battery->avg_charge = 0;
> > +       battery->is_fluctuating = false;
> > +
> >         if (source)
> >                 battery->source = g_strdup(source);
> >         if (provider_path)
> > @@ -105,6 +116,9 @@ static void battery_free(struct btd_battery *battery)
> >         if (battery->path)
> >                 g_free(battery->path);
> > 
> > +       if (battery->last_charges)
> > +               g_free(battery->last_charges);
> > +
> >         if (battery->source)
> >                 g_free(battery->source);
> > 
> > @@ -217,6 +231,39 @@ bool btd_battery_unregister(struct btd_battery *battery)
> >         return true;
> >  }
> > 
> > +static void check_fluctuations(struct btd_battery *battery)
> > +{
> > +       uint8_t spikes = 0;
> > +       int8_t step = 0;
> > +       int8_t direction = 0;
> > +       int8_t prev_direction;
> > +
> > +       for (uint8_t id = 0; id < LAST_CHARGES_SIZE - 1; id++) {
> > +               prev_direction = direction;
> > +               step = battery->last_charges[id] - battery->last_charges[id + 1];
> 
> I prefer to avoid this type of construct, even though it seems to be
> safe to access id + 1 since the loop is limited to LAST_CHARGES_SIZE -
> 1, I'd rather use a queue to access elements, anyway I also think it
> is probably a good idea to store the direction as well.
> 
I will prepare the next version, which will use a queue. Thank you for
your review.

> > +
> > +               /*
> > +                * The battery charge fluctuates too much,
> > +                * which may indicate a battery problem, so
> > +                * the actual value should be displayed.
> > +                */
> > +               if (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)
> >  {
> >         DBG("path = %s", battery->path);
> > @@ -231,6 +278,21 @@ 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;
> > +       battery->last_charges[battery->lru_charge_id] = percentage;
> > +
> > +       if (battery->lru_charge_id == LAST_CHARGES_SIZE - 1)
> > +               check_fluctuations(battery);
> > +
> > +       battery->lru_charge_id = (battery->lru_charge_id + 1) % LAST_CHARGES_SIZE;
> > +
> > +       if (battery->is_fluctuating)
> > +               percentage = battery->avg_charge;
> > +
> >         if (battery->percentage == percentage)
> >                 return true;
> > 
> > --
> > 2.43.0
> > 
> > 
> 
>