Received-SPF: pass (google.com: domain of linux-bluetooth-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20200701192202.9508-1-sonnysasaka@chromium.org> <CABBYNZKsM52hLuNEYSqhHoiMXE5Kix_pp6AoCJeJ4rd6f-NyWg@mail.gmail.com>
In-Reply-To: <CABBYNZKsM52hLuNEYSqhHoiMXE5Kix_pp6AoCJeJ4rd6f-NyWg@mail.gmail.com>
From:   Sonny Sasaka <sonnysasaka@chromium.org>
Date:   Wed, 1 Jul 2020 23:06:09 -0700
Message-ID: <CAOxioN=0s1XebO6qg_kohi=s8Q8DkP6GXdqqqGL17QK2e=Dg4w@mail.gmail.com>
Subject: Re: [PATCH v2] client: Add battery percentage to info command
To:     Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Cc:     "linux-bluetooth@vger.kernel.org" <linux-bluetooth@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-bluetooth-owner@vger.kernel.org
Precedence: bulk

Hi Luiz,

On Wed, Jul 1, 2020 at 10:20 PM Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
>
> Hi Sonny,
>
> On Wed, Jul 1, 2020 at 12:24 PM Sonny Sasaka <sonnysasaka@chromium.org> wrote:
> >
> > This adds the Battery Percentage to the info command based on
> > org.bluez.Battery1 API. Example usage:
> >
> > [bluetooth]# info XX:XX:XX:XX:XX:XX
> > Device XX:XX:XX:XX:XX:XX (random)
> >         Name: ...
> >         Alias: ...
> >         ...
> >         Modalias: ...
> >         Battery Percentage: 100%
> > ---
> >  client/main.c | 43 +++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 43 insertions(+)
> >
> > diff --git a/client/main.c b/client/main.c
> > index 422da5593..4b787240e 100644
> > --- a/client/main.c
> > +++ b/client/main.c
> > @@ -65,6 +65,7 @@ static struct adapter *default_ctrl;
> >  static GDBusProxy *default_dev;
> >  static GDBusProxy *default_attr;
> >  static GList *ctrl_list;
> > +static GList *battery_proxies;
> >
> >  static const char *agent_arguments[] = {
> >         "on",
> > @@ -107,7 +108,9 @@ static void disconnect_handler(DBusConnection *connection, void *user_data)
> >         bt_shell_set_prompt(PROMPT_OFF);
> >
> >         g_list_free_full(ctrl_list, proxy_leak);
> > +       g_list_free_full(battery_proxies, proxy_leak);
> >         ctrl_list = NULL;
> > +       battery_proxies = NULL;
> >
> >         default_ctrl = NULL;
> >  }
> > @@ -445,6 +448,16 @@ done:
> >         g_free(desc);
> >  }
> >
> > +static void battery_added(GDBusProxy *proxy)
> > +{
> > +       battery_proxies = g_list_append(battery_proxies, proxy);
> > +}
> > +
> > +static void battery_removed(GDBusProxy *proxy)
> > +{
> > +       battery_proxies = g_list_remove(battery_proxies, proxy);
> > +}
> > +
> >  static void device_added(GDBusProxy *proxy)
> >  {
> >         DBusMessageIter iter;
> > @@ -539,6 +552,8 @@ static void proxy_added(GDBusProxy *proxy, void *user_data)
> >                 gatt_add_manager(proxy);
> >         } else if (!strcmp(interface, "org.bluez.LEAdvertisingManager1")) {
> >                 ad_manager_added(proxy);
> > +       } else if (!strcmp(interface, "org.bluez.Battery1")) {
> > +               battery_added(proxy);
> >         }
> >  }
> >
> > @@ -630,6 +645,8 @@ static void proxy_removed(GDBusProxy *proxy, void *user_data)
> >                 gatt_remove_manager(proxy);
> >         } else if (!strcmp(interface, "org.bluez.LEAdvertisingManager1")) {
> >                 ad_unregister(dbus_conn, NULL);
> > +       } else if (!strcmp(interface, "org.bluez.Battery1")) {
> > +               battery_removed(proxy);
> >         }
> >  }
> >
> > @@ -763,6 +780,20 @@ static struct adapter *find_ctrl_by_address(GList *source, const char *address)
> >         return NULL;
> >  }
> >
> > +static GDBusProxy *find_battery_by_path(GList *source, const char *path)
> > +{
> > +       GList *list;
> > +
> > +       for (list = g_list_first(source); list; list = g_list_next(list)) {
> > +               GDBusProxy *proxy = list->data;
> > +
> > +               if (strcmp(g_dbus_proxy_get_path(proxy), path) == 0)
> > +                       return proxy;
> > +       }
> > +
> > +       return NULL;
> > +}
> > +
> >  static GDBusProxy *find_proxy_by_address(GList *source, const char *address)
> >  {
> >         GList *list;
> > @@ -1606,8 +1637,10 @@ static struct GDBusProxy *find_device(int argc, char *argv[])
> >  static void cmd_info(int argc, char *argv[])
> >  {
> >         GDBusProxy *proxy;
> > +       GDBusProxy *battery_proxy;
> >         DBusMessageIter iter;
> >         const char *address;
> > +       unsigned char percentage;
> >
> >         proxy = find_device(argc, argv);
> >         if (!proxy)
> > @@ -1647,6 +1680,16 @@ static void cmd_info(int argc, char *argv[])
> >         print_property(proxy, "AdvertisingFlags");
> >         print_property(proxy, "AdvertisingData");
> >
> > +       battery_proxy = find_battery_by_path(battery_proxies,
> > +                                       g_dbus_proxy_get_path(proxy));
>
> I'd replace the lines below with just print_property(battery_proxy,
> "Percentage"); and just make print_property check for NULL proxy if it
> doesn't already.
I tried this but the result doesn't look quite good:
Device XX:XX:XX:XX:XX:XX (random)
        Name: ...
        ...
        Modalias: ...
        Percentage: 0x64

The name "Percentage" is not clear (what kind of percentage does it
mean) when shown in the "info" command. And also the format for byte
data type is hex and there is no percent sign. I tried to modify the
print_property function to support custom format but the code gets
overly complex and I think it's more straightforward to use custom
logic like in this patch.

>
> > +       if (battery_proxy && g_dbus_proxy_get_property(
> > +                               battery_proxy, "Percentage", &iter)) {
> > +               dbus_message_iter_get_basic(&iter, &percentage);
> > +               bt_shell_printf("\tBattery Percentage: %d%%\n", percentage);
> > +       } else {
> > +               bt_shell_printf("\tNo battery information\n");
> > +       }
> > +
> >         return bt_shell_noninteractive_quit(EXIT_SUCCESS);
> >  }
> >
> > --
> > 2.17.1
> >
>
>
> --
> Luiz Augusto von Dentz