[PATCH BlueZ v2 1/7] battery: Add the internal Battery API

[PATCH BlueZ v2 1/7] battery: Add the internal Battery API

[Sonny Sasaka]

This patch adds an API for internal BlueZ code to expose battery
information to org.bluez.Battery1 interface. The motivation behind this
is that there is going to be other places than GATT BAS handler that
exposes battery information, for example internal plugins and the
BatteryProvider1 D-Bus API for external clients.

Reviewed-by: Daniel Winkler <danielwinkler@google.com>

---
 Makefile.am   |   3 +-
 src/battery.c | 191 ++++++++++++++++++++++++++++++++++++++++++++++++++
 src/battery.h |  15 ++++
 3 files changed, 208 insertions(+), 1 deletion(-)
 create mode 100644 src/battery.c
 create mode 100644 src/battery.h

diff --git a/Makefile.am b/Makefile.am
index 56279c4ba..b19c1d40f 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -294,7 +294,8 @@ src_bluetoothd_SOURCES = $(builtin_sources) \
 			src/device.h src/device.c \
 			src/dbus-common.c src/dbus-common.h \
 			src/eir.h src/eir.c \
-			src/adv_monitor.h src/adv_monitor.c
+			src/adv_monitor.h src/adv_monitor.c \
+			src/battery.h src/battery.c
 src_bluetoothd_LDADD = lib/libbluetooth-internal.la \
 			gdbus/libgdbus-internal.la \
 			src/libshared-glib.la \
diff --git a/src/battery.c b/src/battery.c
new file mode 100644
index 000000000..87a6b91fb
--- /dev/null
+++ b/src/battery.c
@@ -0,0 +1,191 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ *
+ *  BlueZ - Bluetooth protocol stack for Linux
+ *
+ *  Copyright (C) 2020  Google LLC
+ *
+ *
+ */
+
+#ifdef HAVE_CONFIG_H
+#include <config.h>
+#endif
+
+#define _GNU_SOURCE
+#include <stdbool.h>
+#include <stdint.h>
+#include <glib.h>
+
+#include "gdbus/gdbus.h"
+#include "lib/bluetooth.h"
+#include "src/shared/queue.h"
+#include "src/shared/util.h"
+#include "battery.h"
+#include "dbus-common.h"
+#include "adapter.h"
+#include "log.h"
+
+#define BATTERY_INTERFACE "org.bluez.Battery1"
+
+#define BATTERY_MAX_PERCENTAGE 100
+
+struct btd_battery {
+	char *path; /* D-Bus object path, owns pointer */
+	uint8_t percentage; /* valid between 0 to 100 inclusively */
+};
+
+static struct queue *batteries = NULL;
+
+static void battery_add(struct btd_battery *battery)
+{
+	if (!batteries)
+		batteries = queue_new();
+
+	queue_push_head(batteries, battery);
+}
+
+static void battery_remove(struct btd_battery *battery)
+{
+	queue_remove(batteries, battery);
+	if (queue_isempty(batteries)) {
+		queue_destroy(batteries, NULL);
+		batteries = NULL;
+	}
+}
+
+static bool match_path(const void *data, const void *user_data)
+{
+	const struct btd_battery *battery = data;
+	const char *path = user_data;
+
+	return g_strcmp0(battery->path, path) == 0;
+}
+
+static struct btd_battery *battery_new(const char *path)
+{
+	struct btd_battery *battery;
+
+	battery = new0(struct btd_battery, 1);
+	battery->path = g_strdup(path);
+	battery->percentage = UINT8_MAX;
+
+	return battery;
+}
+
+static void battery_free(struct btd_battery *battery)
+{
+	if (battery->path)
+		g_free(battery->path);
+
+	free(battery);
+}
+
+static gboolean property_percentage_get(const GDBusPropertyTable *property,
+					DBusMessageIter *iter, void *data)
+{
+	struct btd_battery *battery = data;
+
+	dbus_message_iter_append_basic(iter, DBUS_TYPE_BYTE,
+				       &battery->percentage);
+
+	return TRUE;
+}
+
+static gboolean property_percentage_exists(const GDBusPropertyTable *property,
+					   void *data)
+{
+	struct btd_battery *battery = data;
+
+	return battery->percentage <= BATTERY_MAX_PERCENTAGE;
+}
+
+static const GDBusPropertyTable battery_properties[] = {
+	{ "Percentage", "y", property_percentage_get, NULL,
+	  property_percentage_exists },
+	{}
+};
+
+struct btd_battery *btd_battery_register(const char *path)
+{
+	struct btd_battery *battery;
+
+	DBG("path = %s", path);
+
+	if (queue_find(batteries, match_path, path)) {
+		error("error registering battery: path exists");
+		return NULL;
+	}
+
+	if (!g_str_has_prefix(path, "/")) {
+		error("error registering battery: invalid D-Bus object path");
+		return NULL;
+	}
+
+	battery = battery_new(path);
+	battery_add(battery);
+
+	if (!g_dbus_register_interface(btd_get_dbus_connection(), battery->path,
+				       BATTERY_INTERFACE, NULL, NULL,
+				       battery_properties, battery, NULL)) {
+		error("error registering D-Bus interface for %s",
+		      battery->path);
+
+		battery_remove(battery);
+		battery_free(battery);
+
+		return NULL;
+	}
+
+	DBG("registered Battery object: %s", battery->path);
+
+	return battery;
+}
+
+bool btd_battery_unregister(struct btd_battery *battery)
+{
+	DBG("path = %s", battery->path);
+
+	if (!queue_find(batteries, NULL, battery)) {
+		error("error unregistering battery: "
+		      "battery %s is not registered",
+		      battery->path);
+		return false;
+	}
+
+	if (!g_dbus_unregister_interface(btd_get_dbus_connection(),
+					 battery->path, BATTERY_INTERFACE)) {
+		error("error unregistering battery %s from D-Bus interface",
+		      battery->path);
+		return false;
+	}
+
+	battery_remove(battery);
+	battery_free(battery);
+
+	return true;
+}
+
+bool btd_battery_update(struct btd_battery *battery, uint8_t percentage)
+{
+	DBG("path = %s", battery->path);
+
+	if (!queue_find(batteries, NULL, battery)) {
+		error("error updating battery: battery is not registered");
+		return false;
+	}
+
+	if (percentage > BATTERY_MAX_PERCENTAGE) {
+		error("error updating battery: percentage is not valid");
+		return false;
+	}
+
+	if (battery->percentage == percentage)
+		return true;
+
+	battery->percentage = percentage;
+	g_dbus_emit_property_changed(btd_get_dbus_connection(), battery->path,
+				     BATTERY_INTERFACE, "Percentage");
+
+	return true;
+}
diff --git a/src/battery.h b/src/battery.h
new file mode 100644
index 000000000..9c69b7afa
--- /dev/null
+++ b/src/battery.h
@@ -0,0 +1,15 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/*
+ *
+ *  BlueZ - Bluetooth protocol stack for Linux
+ *
+ *  Copyright (C) 2020  Google LLC
+ *
+ *
+ */
+
+struct btd_battery;
+
+struct btd_battery *btd_battery_register(const char *path);
+bool btd_battery_unregister(struct btd_battery *battery);
+bool btd_battery_update(struct btd_battery *battery, uint8_t percentage);
-- 
2.26.2

[PATCH BlueZ v2 2/7] profiles/battery: Refactor to use battery library

[Sonny Sasaka]

This refactors profiles/battery to use the internal battery library that
handles the D-Bus intricacies so that profiles/battery only handles the
GATT BAS concerns.

Reviewed-by: Daniel Winkler <danielwinkler@google.com>

---
 profiles/battery/battery.c | 51 +++++++++++---------------------------
 1 file changed, 15 insertions(+), 36 deletions(-)

diff --git a/profiles/battery/battery.c b/profiles/battery/battery.c
index 13c80d05c..e1a61dd0b 100644
--- a/profiles/battery/battery.c
+++ b/profiles/battery/battery.c
@@ -23,14 +23,11 @@
 
 #include <glib.h>
 
-#include "gdbus/gdbus.h"
-
 #include "lib/bluetooth.h"
 #include "lib/hci.h"
 #include "lib/sdp.h"
 #include "lib/uuid.h"
 
-#include "src/dbus-common.h"
 #include "src/shared/util.h"
 #include "src/shared/att.h"
 #include "src/shared/queue.h"
@@ -42,6 +39,7 @@
 #include "src/profile.h"
 #include "src/service.h"
 #include "src/log.h"
+#include "src/battery.h"
 #include "attrib/att.h"
 
 #define BATTERY_INTERFACE "org.bluez.Battery1"
@@ -50,7 +48,7 @@
 
 /* Generic Attribute/Access Service */
 struct batt {
-	char *path; /* D-Bus path of device */
+	struct btd_battery *battery;
 	struct btd_device *device;
 	struct gatt_db *db;
 	struct bt_gatt_client *client;
@@ -69,6 +67,8 @@ static void batt_free(struct batt *batt)
 	bt_gatt_client_unref(batt->client);
 	btd_device_unref(batt->device);
 	g_free (batt->initial_value);
+	if (batt->battery)
+		btd_battery_unregister(batt->battery);
 	g_free(batt);
 }
 
@@ -81,11 +81,9 @@ static void batt_reset(struct batt *batt)
 	batt->client = NULL;
 	g_free (batt->initial_value);
 	batt->initial_value = NULL;
-	if (batt->path) {
-		g_dbus_unregister_interface(btd_get_dbus_connection(),
-					    batt->path, BATTERY_INTERFACE);
-		g_free(batt->path);
-		batt->path = NULL;
+	if (batt->battery) {
+		btd_battery_unregister(batt->battery);
+		batt->battery = NULL;
 	}
 }
 
@@ -98,8 +96,11 @@ static void parse_battery_level(struct batt *batt,
 	if (batt->percentage != percentage) {
 		batt->percentage = percentage;
 		DBG("Battery Level updated: %d%%", percentage);
-		g_dbus_emit_property_changed(btd_get_dbus_connection(), batt->path,
-						BATTERY_INTERFACE, "Percentage");
+		if (!batt->battery) {
+			warn("Trying to update an unregistered battery");
+			return;
+		}
+		btd_battery_update(batt->battery, batt->percentage);
 	}
 }
 
@@ -115,22 +116,6 @@ static void batt_io_value_cb(uint16_t value_handle, const uint8_t *value,
 	}
 }
 
-static gboolean property_get_percentage(
-					const GDBusPropertyTable *property,
-					DBusMessageIter *iter, void *data)
-{
-	struct batt *batt = data;
-
-	dbus_message_iter_append_basic(iter, DBUS_TYPE_BYTE, &batt->percentage);
-
-	return TRUE;
-}
-
-static const GDBusPropertyTable battery_properties[] = {
-	{ "Percentage", "y", property_get_percentage },
-	{ }
-};
-
 static void batt_io_ccc_written_cb(uint16_t att_ecode, void *user_data)
 {
 	struct batt *batt = user_data;
@@ -141,13 +126,9 @@ static void batt_io_ccc_written_cb(uint16_t att_ecode, void *user_data)
 		return;
 	}
 
-	if (g_dbus_register_interface(btd_get_dbus_connection(),
-					batt->path, BATTERY_INTERFACE,
-					NULL, NULL,
-					battery_properties, batt,
-					NULL) == FALSE) {
-		error("Unable to register %s interface for %s",
-			BATTERY_INTERFACE, batt->path);
+	batt->battery = btd_battery_register(device_get_path(batt->device));
+
+	if (!batt->battery) {
 		batt_reset(batt);
 		return;
 	}
@@ -321,8 +302,6 @@ static int batt_accept(struct btd_service *service)
 		return -1;
 	}
 
-	batt->path = g_strdup (device_get_path(device));
-
 	btd_service_connecting_complete(service, 0);
 
 	return 0;
-- 
2.26.2

[PATCH BlueZ v2 3/7] battery: Add Source property to Battery API

[Sonny Sasaka]

As Battery API will be generalized for other battery reporting
protocols, the Source property is useful for diagnostics purposes.

Reviewed-by: Miao-chen Chou <mcchou@chromium.org>

---
 profiles/battery/battery.c |  3 ++-
 src/battery.c              | 35 +++++++++++++++++++++++++++++++----
 src/battery.h              |  2 +-
 3 files changed, 34 insertions(+), 6 deletions(-)

diff --git a/profiles/battery/battery.c b/profiles/battery/battery.c
index e1a61dd0b..2478816a4 100644
--- a/profiles/battery/battery.c
+++ b/profiles/battery/battery.c
@@ -126,7 +126,8 @@ static void batt_io_ccc_written_cb(uint16_t att_ecode, void *user_data)
 		return;
 	}
 
-	batt->battery = btd_battery_register(device_get_path(batt->device));
+	batt->battery = btd_battery_register(device_get_path(batt->device),
+					     "GATT Battery Service");
 
 	if (!batt->battery) {
 		batt_reset(batt);
diff --git a/src/battery.c b/src/battery.c
index 87a6b91fb..8613d6e23 100644
--- a/src/battery.c
+++ b/src/battery.c
@@ -31,8 +31,9 @@
 #define BATTERY_MAX_PERCENTAGE 100
 
 struct btd_battery {
-	char *path; /* D-Bus object path, owns pointer */
+	char *path; /* D-Bus object path */
 	uint8_t percentage; /* valid between 0 to 100 inclusively */
+	char *source; /* Descriptive source of the battery info */
 };
 
 static struct queue *batteries = NULL;
@@ -62,13 +63,15 @@ static bool match_path(const void *data, const void *user_data)
 	return g_strcmp0(battery->path, path) == 0;
 }
 
-static struct btd_battery *battery_new(const char *path)
+static struct btd_battery *battery_new(const char *path, const char *source)
 {
 	struct btd_battery *battery;
 
 	battery = new0(struct btd_battery, 1);
 	battery->path = g_strdup(path);
 	battery->percentage = UINT8_MAX;
+	if (source)
+		battery->source = g_strdup(source);
 
 	return battery;
 }
@@ -78,6 +81,9 @@ static void battery_free(struct btd_battery *battery)
 	if (battery->path)
 		g_free(battery->path);
 
+	if (battery->source)
+		g_free(battery->source);
+
 	free(battery);
 }
 
@@ -100,13 +106,34 @@ static gboolean property_percentage_exists(const GDBusPropertyTable *property,
 	return battery->percentage <= BATTERY_MAX_PERCENTAGE;
 }
 
+static gboolean property_source_get(const GDBusPropertyTable *property,
+				    DBusMessageIter *iter, void *data)
+{
+	struct btd_battery *battery = data;
+
+	dbus_message_iter_append_basic(iter, DBUS_TYPE_STRING,
+				       &battery->source);
+
+	return TRUE;
+}
+
+static gboolean property_source_exists(const GDBusPropertyTable *property,
+				       void *data)
+{
+	struct btd_battery *battery = data;
+
+	return battery->source != NULL;
+}
+
 static const GDBusPropertyTable battery_properties[] = {
 	{ "Percentage", "y", property_percentage_get, NULL,
 	  property_percentage_exists },
+	{ "Source", "s", property_source_get, NULL, property_source_exists,
+	  G_DBUS_PROPERTY_FLAG_EXPERIMENTAL },
 	{}
 };
 
-struct btd_battery *btd_battery_register(const char *path)
+struct btd_battery *btd_battery_register(const char *path, const char *source)
 {
 	struct btd_battery *battery;
 
@@ -122,7 +149,7 @@ struct btd_battery *btd_battery_register(const char *path)
 		return NULL;
 	}
 
-	battery = battery_new(path);
+	battery = battery_new(path, source);
 	battery_add(battery);
 
 	if (!g_dbus_register_interface(btd_get_dbus_connection(), battery->path,
diff --git a/src/battery.h b/src/battery.h
index 9c69b7afa..ff63454cd 100644
--- a/src/battery.h
+++ b/src/battery.h
@@ -10,6 +10,6 @@
 
 struct btd_battery;
 
-struct btd_battery *btd_battery_register(const char *path);
+struct btd_battery *btd_battery_register(const char *path, const char *source);
 bool btd_battery_unregister(struct btd_battery *battery);
 bool btd_battery_update(struct btd_battery *battery, uint8_t percentage);
-- 
2.26.2

[PATCH BlueZ v2 4/7] doc: Add Battery Provider API doc

[Sonny Sasaka]

This patch add the documentation of the Battery Provider which lets
external clients feed battery information to BlueZ if they are able to
decode battery reporting via any profile. BlueZ UI clients can then use
the org.bluez.Battery1 API as a single source of battery information
coming from many different profiles.

Reviewed-by: Miao-chen Chou <mcchou@chromium.org>

---
 doc/battery-api.txt | 55 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 55 insertions(+)

diff --git a/doc/battery-api.txt b/doc/battery-api.txt
index dc7dbeda2..058bf0c6e 100644
--- a/doc/battery-api.txt
+++ b/doc/battery-api.txt
@@ -12,3 +12,58 @@ Object path	[variable prefix]/{hci0,hci1,...}/dev_XX_XX_XX_XX_XX_XX
 Properties	byte Percentage [readonly]
 
 			The percentage of battery left as an unsigned 8-bit integer.
+
+		string Source [readonly, optional, experimental]
+
+			Describes where the battery information comes from
+			(e.g. "HFP 1.7", "HID").
+			This property is informational only and may be useful
+			for debugging purposes.
+
+
+Battery Provider Manager hierarchy
+==================================
+A battery provider starts by registering itself as a battery provider with the
+RegisterBatteryProvider method passing an object path as the provider ID. Then,
+it can start exposing org.bluez.BatteryProvider1 objects having the path
+starting with the given provider ID. It can also remove objects at any time.
+BlueZ will stop monitoring these exposed and removed objects after
+UnregisterBatteryProvider is called for that provider ID.
+
+Service		org.bluez
+Interface	org.bluez.BatteryProviderManager1 [experimental]
+Object path	/org/bluez/{hci0,hci1,...}
+
+Methods		void RegisterBatteryProvider(object provider)
+
+			This registers a battery provider. A registered
+			battery provider can then expose objects with
+			org.bluez.BatteryProvider1 interface described below.
+
+		void UnregisterBatteryProvider(object provider)
+
+			This unregisters a battery provider. After
+			unregistration, the BatteryProvider1 objects provided
+			by this client are ignored by BlueZ.
+
+
+Battery Provider hierarchy
+==========================
+
+Service		<client D-Bus address>
+Interface	org.bluez.BatteryProvider1 [experimental]
+Object path	{provider_root}/org/bluez/{hci0,hci1,...}/dev_XX_XX_XX_XX_XX_XX
+
+Properties	byte Percentage [readonly]
+
+			The percentage of battery left as an unsigned 8-bit
+			integer.
+
+		string Source [readonly, optional]
+
+			Describes where the battery information comes from
+			(e.g. "HFP 1.7", "HID").
+			This property is informational only and may be useful
+			for debugging purposes. The content of this property is
+			a free string, but it is recommended to include the
+			profile name and version to be useful.
-- 
2.26.2

[PATCH BlueZ v2 5/7] test: Add test app for Battery Provider API

[Sonny Sasaka]

The python test app simulates an application registering to BlueZ as a
Battery Provider providing three fake batteries drained periodically.

Reviewed-by: Miao-chen Chou <mcchou@chromium.org>

---
 test/example-battery-provider | 230 ++++++++++++++++++++++++++++++++++
 1 file changed, 230 insertions(+)
 create mode 100755 test/example-battery-provider

diff --git a/test/example-battery-provider b/test/example-battery-provider
new file mode 100755
index 000000000..3dbb08563
--- /dev/null
+++ b/test/example-battery-provider
@@ -0,0 +1,230 @@
+#!/usr/bin/env python3
+# SPDX-License-Identifier: LGPL-2.1-or-later
+
+import dbus
+import dbus.exceptions
+import dbus.mainloop.glib
+import dbus.service
+
+try:
+  from gi.repository import GObject
+except ImportError:
+  import gobject as GObject
+import sys
+
+mainloop = None
+app = None
+bus = None
+
+BLUEZ_SERVICE_NAME = 'org.bluez'
+DBUS_OM_IFACE =      'org.freedesktop.DBus.ObjectManager'
+DBUS_PROP_IFACE =    'org.freedesktop.DBus.Properties'
+
+BATTERY_PROVIDER_MANAGER_IFACE = 'org.bluez.BatteryProviderManager1'
+BATTERY_PROVIDER_IFACE = 'org.bluez.BatteryProvider1'
+BATTERY_PROVIDER_PATH = '/path/to/provider'
+
+BATTERY_PATH1 = '11_11_11_11_11_11'
+BATTERY_PATH2 = '22_22_22_22_22_22'
+BATTERY_PATH3 = '33_33_33_33_33_33'
+
+class InvalidArgsException(dbus.exceptions.DBusException):
+    _dbus_error_name = 'org.freedesktop.DBus.Error.InvalidArgs'
+
+
+class Application(dbus.service.Object):
+    def __init__(self, bus):
+        self.path = BATTERY_PROVIDER_PATH
+        self.services = []
+        self.batteries = []
+        dbus.service.Object.__init__(self, bus, self.path)
+
+    def get_path(self):
+        return dbus.ObjectPath(self.path)
+
+    def add_battery(self, battery):
+        self.batteries.append(battery)
+        self.InterfacesAdded(battery.get_path(), battery.get_properties())
+        GObject.timeout_add(1000, drain_battery, battery)
+
+    def remove_battery(self, battery):
+        self.batteries.remove(battery)
+        self.InterfacesRemoved(battery.get_path(), [BATTERY_PROVIDER_IFACE])
+
+    @dbus.service.method(DBUS_OM_IFACE, out_signature='a{oa{sa{sv}}}')
+    def GetManagedObjects(self):
+        response = {}
+        print('GetManagedObjects called')
+
+        for battery in self.batteries:
+            response[battery.get_path()] = battery.get_properties()
+
+        return response
+
+    @dbus.service.signal(DBUS_OM_IFACE, signature='oa{sa{sv}}')
+    def InterfacesAdded(self, object_path, interfaces_and_properties):
+        return
+
+    @dbus.service.signal(DBUS_OM_IFACE, signature='oas')
+    def InterfacesRemoved(self, object_path, interfaces):
+        return
+
+
+class Battery(dbus.service.Object):
+    """
+    org.bluez.BatteryProvider1 interface implementation
+    """
+    def __init__(self, bus, dev, percentage, source = None):
+        self.path = BATTERY_PROVIDER_PATH + '/org/bluez/hci0/dev_' + dev
+        self.bus = bus
+        self.percentage = percentage
+        self.source = source
+        dbus.service.Object.__init__(self, bus, self.path)
+
+    def get_battery_properties(self):
+        properties = {}
+        if self.percentage != None:
+            properties['Percentage'] = dbus.Byte(self.percentage)
+        if self.source != None:
+            properties['Source'] = self.source
+        return properties
+
+    def get_properties(self):
+        return { BATTERY_PROVIDER_IFACE: self.get_battery_properties() }
+
+    def get_path(self):
+        return dbus.ObjectPath(self.path)
+
+    def set_percentage(self, percentage):
+        if percentage < 0 or percentage > 100:
+            print('percentage not valid')
+            return
+
+        self.percentage = percentage
+        print('battery %s percentage %d' % (self.path, self.percentage))
+        self.PropertiesChanged(
+                BATTERY_PROVIDER_IFACE, self.get_battery_properties())
+
+    @dbus.service.method(DBUS_PROP_IFACE,
+                         in_signature='s',
+                         out_signature='a{sv}')
+    def GetAll(self, interface):
+        if interface != BATTERY_PROVIDER_IFACE:
+            raise InvalidArgsException()
+
+        return self.get_properties()[BATTERY_PROVIDER_IFACE]
+
+    @dbus.service.signal(DBUS_PROP_IFACE, signature='sa{sv}')
+    def PropertiesChanged(self, interface, properties):
+        return
+
+
+def add_late_battery():
+    app.add_battery(Battery(bus, BATTERY_PATH3, 70, 'Protocol 2'))
+
+
+def drain_battery(battery):
+    new_percentage = 100
+    if battery.percentage != None:
+        new_percentage = battery.percentage - 5
+        if new_percentage < 0:
+            new_percentage = 0
+
+    battery.set_percentage(new_percentage)
+
+    if new_percentage <= 0:
+        return False
+
+    return True
+
+def register_provider_cb():
+    print('Battery Provider registered')
+
+    # Battery added early right after RegisterBatteryProvider succeeds
+    app.add_battery(Battery(bus, BATTERY_PATH2, None))
+    # Battery added later
+    GObject.timeout_add(5000, add_late_battery)
+
+
+def register_provider_error_cb(error):
+    print('Failed to register Battery Provider: ' + str(error))
+    mainloop.quit()
+
+
+def find_manager(bus):
+    remote_om = dbus.Interface(bus.get_object(BLUEZ_SERVICE_NAME, '/'),
+                               DBUS_OM_IFACE)
+    objects = remote_om.GetManagedObjects()
+
+    for o, props in objects.items():
+        if BATTERY_PROVIDER_MANAGER_IFACE in props.keys():
+            return o
+
+    return None
+
+
+def unregister_provider_cb():
+    print('Battery Provider unregistered')
+
+
+def unregister_provider_error_cb(error):
+    print('Failed to unregister Battery Provider: ' + str(error))
+
+
+def unregister_battery_provider(battery_provider_manager):
+    battery_provider_manager.UnregisterBatteryProvider(BATTERY_PROVIDER_PATH,
+                                    reply_handler=unregister_provider_cb,
+                                    error_handler=unregister_provider_error_cb)
+
+
+def remove_battery(app, battery):
+    app.remove_battery(battery)
+
+
+"""
+Simulates an application registering to BlueZ as a Battery Provider providing
+fake batteries drained periodically.
+"""
+def main():
+    global mainloop, bus, app
+
+    dbus.mainloop.glib.DBusGMainLoop(set_as_default=True)
+
+    bus = dbus.SystemBus()
+
+    manager_path = find_manager(bus)
+    if not manager_path:
+        print('BatteryProviderManager1 interface not found')
+        return
+
+    print('BatteryProviderManager1 path = ', manager_path)
+
+    battery_provider_manager = dbus.Interface(
+            bus.get_object(BLUEZ_SERVICE_NAME, manager_path),
+            BATTERY_PROVIDER_MANAGER_IFACE)
+
+    app = Application(bus)
+
+    # Battery pre-added before RegisterBatteryProvider
+    battery1 = Battery(bus, BATTERY_PATH1, 87, 'Protocol 1')
+    app.add_battery(battery1)
+
+    mainloop = GObject.MainLoop()
+
+    print('Registering Battery Provider...')
+
+    battery_provider_manager.RegisterBatteryProvider(BATTERY_PROVIDER_PATH,
+                                    reply_handler=register_provider_cb,
+                                    error_handler=register_provider_error_cb)
+
+    # Unregister the Battery Provider after an arbitrary amount of time
+    GObject.timeout_add(
+            12000, unregister_battery_provider, battery_provider_manager)
+    # Simulate battery removal by a provider
+    GObject.timeout_add(8000, remove_battery, app, battery1)
+
+    mainloop.run()
+
+
+if __name__ == '__main__':
+    main()
-- 
2.26.2

[PATCH BlueZ v2 6/7] adapter: Add a public function to find a device by path

[Sonny Sasaka]

The public function is motivated by the Battery Provider API code which
needs to probe whether a device exists.

Reviewed-by: Miao-chen Chou <mcchou@chromium.org>

---
 src/adapter.c | 33 ++++++++++++++++++++++++---------
 src/adapter.h |  2 ++
 2 files changed, 26 insertions(+), 9 deletions(-)

diff --git a/src/adapter.c b/src/adapter.c
index c0053000a..d27faaaa3 100644
--- a/src/adapter.c
+++ b/src/adapter.c
@@ -872,6 +872,30 @@ struct btd_device *btd_adapter_find_device(struct btd_adapter *adapter,
 	return device;
 }
 
+static int device_path_cmp(gconstpointer a, gconstpointer b)
+{
+	const struct btd_device *device = a;
+	const char *path = b;
+	const char *dev_path = device_get_path(device);
+
+	return strcasecmp(dev_path, path);
+}
+
+struct btd_device *btd_adapter_find_device_by_path(struct btd_adapter *adapter,
+						   const char *path)
+{
+	GSList *list;
+
+	if (!adapter)
+		return NULL;
+
+	list = g_slist_find_custom(adapter->devices, path, device_path_cmp);
+	if (!list)
+		return NULL;
+
+	return list->data;
+}
+
 static void uuid_to_uuid128(uuid_t *uuid128, const uuid_t *uuid)
 {
 	if (uuid->type == SDP_UUID16)
@@ -3184,15 +3208,6 @@ static gboolean property_get_roles(const GDBusPropertyTable *property,
 	return TRUE;
 }
 
-static int device_path_cmp(gconstpointer a, gconstpointer b)
-{
-	const struct btd_device *device = a;
-	const char *path = b;
-	const char *dev_path = device_get_path(device);
-
-	return strcasecmp(dev_path, path);
-}
-
 static DBusMessage *remove_device(DBusConnection *conn,
 					DBusMessage *msg, void *user_data)
 {
diff --git a/src/adapter.h b/src/adapter.h
index dcc574857..a77c7a61c 100644
--- a/src/adapter.h
+++ b/src/adapter.h
@@ -83,6 +83,8 @@ sdp_list_t *btd_adapter_get_services(struct btd_adapter *adapter);
 struct btd_device *btd_adapter_find_device(struct btd_adapter *adapter,
 							const bdaddr_t *dst,
 							uint8_t dst_type);
+struct btd_device *btd_adapter_find_device_by_path(struct btd_adapter *adapter,
+						   const char *path);
 
 const char *adapter_get_path(struct btd_adapter *adapter);
 const bdaddr_t *btd_adapter_get_address(struct btd_adapter *adapter);
-- 
2.26.2

[PATCH BlueZ v2 7/7] battery: Implement Battery Provider API

[Sonny Sasaka]

This patch implements the BatteryProvider1 and BatteryProviderManager1
API. This is a means for external clients to feed battery information to
BlueZ if they handle some profile and can decode battery reporting.

The battery information is then exposed externally via the existing
Battery1 interface. UI components can consume this API to display
Bluetooth peripherals' battery via a unified BlueZ API.

Reviewed-by: Miao-chen Chou <mcchou@chromium.org>

---
 profiles/battery/battery.c |   2 +-
 src/adapter.c              |  11 ++
 src/battery.c              | 371 ++++++++++++++++++++++++++++++++++++-
 src/battery.h              |  10 +-
 4 files changed, 389 insertions(+), 5 deletions(-)

diff --git a/profiles/battery/battery.c b/profiles/battery/battery.c
index 2478816a4..81f849d57 100644
--- a/profiles/battery/battery.c
+++ b/profiles/battery/battery.c
@@ -127,7 +127,7 @@ static void batt_io_ccc_written_cb(uint16_t att_ecode, void *user_data)
 	}
 
 	batt->battery = btd_battery_register(device_get_path(batt->device),
-					     "GATT Battery Service");
+					     "GATT Battery Service", NULL);
 
 	if (!batt->battery) {
 		batt_reset(batt);
diff --git a/src/adapter.c b/src/adapter.c
index d27faaaa3..b791cdad2 100644
--- a/src/adapter.c
+++ b/src/adapter.c
@@ -66,6 +66,7 @@
 #include "advertising.h"
 #include "adv_monitor.h"
 #include "eir.h"
+#include "battery.h"
 
 #define MODE_OFF		0x00
 #define MODE_CONNECTABLE	0x01
@@ -254,6 +255,8 @@ struct btd_adapter {
 
 	struct btd_adv_monitor_manager *adv_monitor_manager;
 
+	struct btd_battery_provider_manager *battery_provider_manager;
+
 	gboolean initialized;
 
 	GSList *pin_callbacks;
@@ -6361,6 +6364,9 @@ static void adapter_remove(struct btd_adapter *adapter)
 	btd_adv_monitor_manager_destroy(adapter->adv_monitor_manager);
 	adapter->adv_monitor_manager = NULL;
 
+	btd_battery_provider_manager_destroy(adapter->battery_provider_manager);
+	adapter->battery_provider_manager = NULL;
+
 	g_slist_free(adapter->pin_callbacks);
 	adapter->pin_callbacks = NULL;
 
@@ -8651,6 +8657,11 @@ static int adapter_register(struct btd_adapter *adapter)
 		}
 	}
 
+	if (g_dbus_get_flags() & G_DBUS_FLAG_ENABLE_EXPERIMENTAL) {
+		adapter->battery_provider_manager =
+			btd_battery_provider_manager_create(adapter);
+	}
+
 	db = btd_gatt_database_get_db(adapter->database);
 	adapter->db_id = gatt_db_register(db, services_modified,
 							services_modified,
diff --git a/src/battery.c b/src/battery.c
index 8613d6e23..8594c8d77 100644
--- a/src/battery.c
+++ b/src/battery.c
@@ -25,8 +25,11 @@
 #include "dbus-common.h"
 #include "adapter.h"
 #include "log.h"
+#include "error.h"
 
 #define BATTERY_INTERFACE "org.bluez.Battery1"
+#define BATTERY_PROVIDER_INTERFACE "org.bluez.BatteryProvider1"
+#define BATTERY_PROVIDER_MANAGER_INTERFACE "org.bluez.BatteryProviderManager1"
 
 #define BATTERY_MAX_PERCENTAGE 100
 
@@ -34,10 +37,27 @@ struct btd_battery {
 	char *path; /* D-Bus object path */
 	uint8_t percentage; /* valid between 0 to 100 inclusively */
 	char *source; /* Descriptive source of the battery info */
+	char *provider_path; /* The provider root path, if any */
+};
+
+struct btd_battery_provider_manager {
+	struct btd_adapter *adapter; /* Does not own pointer */
+	struct queue *battery_providers;
+};
+
+struct battery_provider {
+	struct btd_battery_provider_manager *manager; /* Does not own pointer */
+
+	char *owner; /* Owner D-Bus address */
+	char *path; /* D-Bus object path */
+
+	GDBusClient *client;
 };
 
 static struct queue *batteries = NULL;
 
+static void provider_disconnect_cb(DBusConnection *conn, void *user_data);
+
 static void battery_add(struct btd_battery *battery)
 {
 	if (!batteries)
@@ -63,7 +83,8 @@ static bool match_path(const void *data, const void *user_data)
 	return g_strcmp0(battery->path, path) == 0;
 }
 
-static struct btd_battery *battery_new(const char *path, const char *source)
+static struct btd_battery *battery_new(const char *path, const char *source,
+				       const char *provider_path)
 {
 	struct btd_battery *battery;
 
@@ -72,6 +93,8 @@ static struct btd_battery *battery_new(const char *path, const char *source)
 	battery->percentage = UINT8_MAX;
 	if (source)
 		battery->source = g_strdup(source);
+	if (provider_path)
+		battery->provider_path = g_strdup(provider_path);
 
 	return battery;
 }
@@ -133,7 +156,8 @@ static const GDBusPropertyTable battery_properties[] = {
 	{}
 };
 
-struct btd_battery *btd_battery_register(const char *path, const char *source)
+struct btd_battery *btd_battery_register(const char *path, const char *source,
+					 const char *provider_path)
 {
 	struct btd_battery *battery;
 
@@ -149,7 +173,7 @@ struct btd_battery *btd_battery_register(const char *path, const char *source)
 		return NULL;
 	}
 
-	battery = battery_new(path, source);
+	battery = battery_new(path, source, provider_path);
 	battery_add(battery);
 
 	if (!g_dbus_register_interface(btd_get_dbus_connection(), battery->path,
@@ -216,3 +240,344 @@ bool btd_battery_update(struct btd_battery *battery, uint8_t percentage)
 
 	return true;
 }
+
+static struct btd_battery *find_battery_by_path(const char *path)
+{
+	return queue_find(batteries, match_path, path);
+}
+
+static void provided_battery_property_changed_cb(GDBusProxy *proxy,
+						 const char *name,
+						 DBusMessageIter *iter,
+						 void *user_data)
+{
+	uint8_t percentage;
+	struct battery_provider *provider = user_data;
+	const char *path = g_dbus_proxy_get_path(proxy);
+	const char *export_path;
+
+	export_path = &path[strlen(provider->path)];
+
+	if (strcmp(name, "Percentage") != 0)
+		return;
+
+	if (dbus_message_iter_get_arg_type(iter) != DBUS_TYPE_BYTE)
+		return;
+
+	dbus_message_iter_get_basic(iter, &percentage);
+
+	DBG("battery percentage changed on %s, percentage = %d",
+	    g_dbus_proxy_get_path(proxy), percentage);
+
+	btd_battery_update(find_battery_by_path(export_path), percentage);
+}
+
+static void provided_battery_added_cb(GDBusProxy *proxy, void *user_data)
+{
+	struct battery_provider *provider = user_data;
+	struct btd_battery *battery;
+	const char *path = g_dbus_proxy_get_path(proxy);
+	const char *export_path;
+	const char *source = NULL;
+	uint8_t percentage;
+	DBusMessageIter iter;
+
+	export_path = &path[strlen(provider->path)];
+
+	if (strcmp(g_dbus_proxy_get_interface(proxy),
+		   BATTERY_PROVIDER_INTERFACE) != 0)
+		return;
+
+	if (!btd_adapter_find_device_by_path(provider->manager->adapter,
+					     export_path)) {
+		warn("Ignoring non-existent device path for battery %s",
+		     export_path);
+		return;
+	}
+
+	if (find_battery_by_path(export_path))
+		return;
+
+	g_dbus_proxy_set_property_watch(
+		proxy, provided_battery_property_changed_cb, provider);
+
+	if (g_dbus_proxy_get_property(proxy, "Source", &iter) == TRUE)
+		dbus_message_iter_get_basic(&iter, &source);
+
+	battery = btd_battery_register(export_path, source, provider->path);
+
+	DBG("provided battery added %s", path);
+
+	/* Percentage property may not be immediately available, that's okay
+	 * since we monitor changes to this property.
+	 */
+	if (g_dbus_proxy_get_property(proxy, "Percentage", &iter) == FALSE)
+		return;
+
+	dbus_message_iter_get_basic(&iter, &percentage);
+
+	btd_battery_update(battery, percentage);
+}
+
+static void provided_battery_removed_cb(GDBusProxy *proxy, void *user_data)
+{
+	struct battery_provider *provider = user_data;
+	struct btd_battery *battery;
+	const char *path = g_dbus_proxy_get_path(proxy);
+	const char *export_path;
+
+	export_path = &path[strlen(provider->path)];
+
+	if (strcmp(g_dbus_proxy_get_interface(proxy),
+		   BATTERY_PROVIDER_INTERFACE) != 0)
+		return;
+
+	DBG("provided battery removed %s", g_dbus_proxy_get_path(proxy));
+
+	battery = find_battery_by_path(export_path);
+	if (!battery)
+		return;
+
+	if (g_strcmp0(battery->provider_path, provider->path) != 0)
+		return;
+
+	g_dbus_proxy_set_property_watch(proxy, NULL, NULL);
+
+	btd_battery_unregister(battery);
+}
+
+static bool match_provider_path(const void *data, const void *user_data)
+{
+	const struct battery_provider *provider = data;
+	const char *path = user_data;
+
+	return strcmp(provider->path, path) == 0;
+}
+
+static void unregister_if_path_has_prefix(void *data, void *user_data)
+{
+	struct btd_battery *battery = data;
+	struct battery_provider *provider = user_data;
+
+	if (g_strcmp0(battery->provider_path, provider->path) == 0)
+		btd_battery_unregister(battery);
+}
+
+static void battery_provider_free(gpointer data)
+{
+	struct battery_provider *provider = data;
+
+	/* Unregister batteries under the root path of provider->path */
+	queue_foreach(batteries, unregister_if_path_has_prefix, provider);
+
+	if (provider->owner)
+		g_free(provider->owner);
+
+	if (provider->path)
+		g_free(provider->path);
+
+	if (provider->client) {
+		g_dbus_client_set_disconnect_watch(provider->client, NULL,
+						   NULL);
+		g_dbus_client_set_proxy_handlers(provider->client, NULL, NULL,
+						 NULL, NULL);
+		g_dbus_client_unref(provider->client);
+	}
+
+	free(provider);
+}
+
+static struct battery_provider *
+battery_provider_new(DBusConnection *conn,
+		     struct btd_battery_provider_manager *manager,
+		     const char *path, const char *sender)
+{
+	struct battery_provider *provider;
+
+	provider = new0(struct battery_provider, 1);
+	provider->manager = manager;
+	provider->owner = g_strdup(sender);
+	provider->path = g_strdup(path);
+
+	provider->client = g_dbus_client_new_full(conn, sender, path, path);
+
+	if (!provider->client) {
+		error("error creating D-Bus client %s", path);
+		battery_provider_free(provider);
+		return NULL;
+	}
+
+	g_dbus_client_set_disconnect_watch(provider->client,
+					   provider_disconnect_cb, provider);
+
+	g_dbus_client_set_proxy_handlers(provider->client,
+					 provided_battery_added_cb,
+					 provided_battery_removed_cb, NULL,
+					 provider);
+
+	return provider;
+}
+
+static void provider_disconnect_cb(DBusConnection *conn, void *user_data)
+{
+	struct battery_provider *provider = user_data;
+	struct btd_battery_provider_manager *manager = provider->manager;
+
+	DBG("battery provider client disconnected %s root path %s",
+	    provider->owner, provider->path);
+
+	if (!queue_find(manager->battery_providers, NULL, provider)) {
+		warn("Disconnection on a non-existing provider %s",
+		     provider->path);
+		return;
+	}
+
+	queue_remove(manager->battery_providers, provider);
+	battery_provider_free(provider);
+}
+
+static DBusMessage *register_battery_provider(DBusConnection *conn,
+					      DBusMessage *msg, void *user_data)
+{
+	struct btd_battery_provider_manager *manager = user_data;
+	const char *sender = dbus_message_get_sender(msg);
+	DBusMessageIter args;
+	const char *path;
+	struct battery_provider *provider;
+
+	if (!dbus_message_iter_init(msg, &args))
+		return btd_error_invalid_args(msg);
+
+	if (dbus_message_iter_get_arg_type(&args) != DBUS_TYPE_OBJECT_PATH)
+		return btd_error_invalid_args(msg);
+
+	dbus_message_iter_get_basic(&args, &path);
+
+	DBG("register battery provider path = %s", path);
+
+	if (!g_str_has_prefix(path, "/"))
+		return btd_error_invalid_args(msg);
+
+	if (queue_find(manager->battery_providers, match_provider_path, path)) {
+		return dbus_message_new_error(msg,
+					      ERROR_INTERFACE ".AlreadyExists",
+					      "Provider already exists");
+	}
+
+	provider = battery_provider_new(conn, manager, path, sender);
+	queue_push_head(manager->battery_providers, provider);
+
+	return dbus_message_new_method_return(msg);
+}
+
+static DBusMessage *unregister_battery_provider(DBusConnection *conn,
+						DBusMessage *msg,
+						void *user_data)
+{
+	struct btd_battery_provider_manager *manager = user_data;
+	const char *sender = dbus_message_get_sender(msg);
+	DBusMessageIter args;
+	const char *path;
+	struct battery_provider *provider;
+
+	if (!dbus_message_iter_init(msg, &args))
+		return btd_error_invalid_args(msg);
+
+	if (dbus_message_iter_get_arg_type(&args) != DBUS_TYPE_OBJECT_PATH)
+		return btd_error_invalid_args(msg);
+
+	dbus_message_iter_get_basic(&args, &path);
+
+	DBG("unregister battery provider path = %s", path);
+
+	provider = queue_find(manager->battery_providers, match_provider_path,
+			      path);
+	if (!provider || strcmp(provider->owner, sender) != 0) {
+		return dbus_message_new_error(msg,
+					      ERROR_INTERFACE ".DoesNotExist",
+					      "Provider does not exist");
+	}
+
+	queue_remove(manager->battery_providers, provider);
+	battery_provider_free(provider);
+
+	return dbus_message_new_method_return(msg);
+}
+
+static const GDBusMethodTable methods[] = {
+	{ GDBUS_EXPERIMENTAL_METHOD("RegisterBatteryProvider",
+				    GDBUS_ARGS({ "provider", "o" }), NULL,
+				    register_battery_provider) },
+	{ GDBUS_EXPERIMENTAL_METHOD("UnregisterBatteryProvider",
+				    GDBUS_ARGS({ "provider", "o" }), NULL,
+				    unregister_battery_provider) },
+	{}
+};
+
+static struct btd_battery_provider_manager *
+manager_new(struct btd_adapter *adapter)
+{
+	struct btd_battery_provider_manager *manager;
+
+	DBG("");
+
+	manager = new0(struct btd_battery_provider_manager, 1);
+	manager->adapter = adapter;
+	manager->battery_providers = queue_new();
+
+	return manager;
+}
+
+static void manager_free(struct btd_battery_provider_manager *manager)
+{
+	if (!manager)
+		return;
+
+	DBG("");
+
+	queue_destroy(manager->battery_providers, battery_provider_free);
+
+	free(manager);
+}
+
+struct btd_battery_provider_manager *
+btd_battery_provider_manager_create(struct btd_adapter *adapter)
+{
+	struct btd_battery_provider_manager *manager;
+
+	if (!adapter)
+		return NULL;
+
+	manager = manager_new(adapter);
+	if (!manager)
+		return NULL;
+
+	if (!g_dbus_register_interface(btd_get_dbus_connection(),
+				       adapter_get_path(manager->adapter),
+				       BATTERY_PROVIDER_MANAGER_INTERFACE,
+				       methods, NULL, NULL, manager, NULL)) {
+		error("error registering " BATTERY_PROVIDER_MANAGER_INTERFACE
+		      " interface");
+		manager_free(manager);
+		return NULL;
+	}
+
+	info("Battery Provider Manager created");
+
+	return manager;
+}
+
+void btd_battery_provider_manager_destroy(
+	struct btd_battery_provider_manager *manager)
+{
+	if (!manager)
+		return;
+
+	g_dbus_unregister_interface(btd_get_dbus_connection(),
+				    adapter_get_path(manager->adapter),
+				    BATTERY_PROVIDER_MANAGER_INTERFACE);
+
+	info("Battery Provider Manager destroyed");
+
+	manager_free(manager);
+}
diff --git a/src/battery.h b/src/battery.h
index ff63454cd..271659474 100644
--- a/src/battery.h
+++ b/src/battery.h
@@ -8,8 +8,16 @@
  *
  */
 
+struct btd_adapter;
 struct btd_battery;
+struct btd_battery_provider_manager;
 
-struct btd_battery *btd_battery_register(const char *path, const char *source);
+struct btd_battery *btd_battery_register(const char *path, const char *source,
+					 const char *provider_path);
 bool btd_battery_unregister(struct btd_battery *battery);
 bool btd_battery_update(struct btd_battery *battery, uint8_t percentage);
+
+struct btd_battery_provider_manager *
+btd_battery_provider_manager_create(struct btd_adapter *adapter);
+void btd_battery_provider_manager_destroy(
+	struct btd_battery_provider_manager *manager);
-- 
2.26.2

RE: [BlueZ,v2,1/7] battery: Add the internal Battery API

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 2008 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=381631

---Test result---

##############################
Test: CheckPatch - FAIL
Output:
battery: Add the internal Battery API
ERROR:INITIALISED_STATIC: do not initialise statics to NULL
#71: FILE: src/battery.c:38:
+static struct queue *batteries = NULL;

- total: 1 errors, 0 warnings, 215 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

"[PATCH] battery: Add the internal Battery API" has style problems, please review.

NOTE: Ignored message types: COMMIT_MESSAGE COMPLEX_MACRO CONST_STRUCT FILE_PATH_CHANGES MISSING_SIGN_OFF PREFER_PACKED SPLIT_STRING SSCANF_TO_KSTRTO

NOTE: If any of the errors are false positives, please report
      them to the maintainer, see CHECKPATCH in MAINTAINERS.

test: Add test app for Battery Provider API
ERROR:EXECUTE_PERMISSIONS: do not set execute permissions for source files
#12: FILE: test/example-battery-provider

- total: 1 errors, 0 warnings, 230 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

"[PATCH] test: Add test app for Battery Provider API" has style problems, please review.

NOTE: Ignored message types: COMMIT_MESSAGE COMPLEX_MACRO CONST_STRUCT FILE_PATH_CHANGES MISSING_SIGN_OFF PREFER_PACKED SPLIT_STRING SSCANF_TO_KSTRTO

NOTE: If any of the errors are false positives, please report
      them to the maintainer, see CHECKPATCH in MAINTAINERS.


##############################
Test: CheckGitLint - PASS

##############################
Test: CheckBuild - PASS

##############################
Test: MakeCheck - PASS



---
Regards,
Linux Bluetooth

Re: [PATCH BlueZ v2 4/7] doc: Add Battery Provider API doc

[Luiz Augusto von Dentz]

Hi Sonny,

On Tue, Nov 10, 2020 at 5:22 PM Sonny Sasaka <sonnysasaka@chromium.org> wrote:
>
> This patch add the documentation of the Battery Provider which lets
> external clients feed battery information to BlueZ if they are able to
> decode battery reporting via any profile. BlueZ UI clients can then use
> the org.bluez.Battery1 API as a single source of battery information
> coming from many different profiles.
>
> Reviewed-by: Miao-chen Chou <mcchou@chromium.org>
>
> ---
>  doc/battery-api.txt | 55 +++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 55 insertions(+)
>
> diff --git a/doc/battery-api.txt b/doc/battery-api.txt
> index dc7dbeda2..058bf0c6e 100644
> --- a/doc/battery-api.txt
> +++ b/doc/battery-api.txt
> @@ -12,3 +12,58 @@ Object path  [variable prefix]/{hci0,hci1,...}/dev_XX_XX_XX_XX_XX_XX
>  Properties     byte Percentage [readonly]
>
>                         The percentage of battery left as an unsigned 8-bit integer.
> +
> +               string Source [readonly, optional, experimental]
> +
> +                       Describes where the battery information comes from
> +                       (e.g. "HFP 1.7", "HID").
> +                       This property is informational only and may be useful
> +                       for debugging purposes.

We should probably tight this to UUID instead.

> +
> +
> +Battery Provider Manager hierarchy
> +==================================
> +A battery provider starts by registering itself as a battery provider with the
> +RegisterBatteryProvider method passing an object path as the provider ID. Then,
> +it can start exposing org.bluez.BatteryProvider1 objects having the path
> +starting with the given provider ID. It can also remove objects at any time.
> +BlueZ will stop monitoring these exposed and removed objects after
> +UnregisterBatteryProvider is called for that provider ID.
> +
> +Service                org.bluez
> +Interface      org.bluez.BatteryProviderManager1 [experimental]
> +Object path    /org/bluez/{hci0,hci1,...}
> +
> +Methods                void RegisterBatteryProvider(object provider)
> +
> +                       This registers a battery provider. A registered
> +                       battery provider can then expose objects with
> +                       org.bluez.BatteryProvider1 interface described below.
> +
> +               void UnregisterBatteryProvider(object provider)
> +
> +                       This unregisters a battery provider. After
> +                       unregistration, the BatteryProvider1 objects provided
> +                       by this client are ignored by BlueZ.

Not sure if you were followinging the monitor patches, for registering
objects we do prefer the ObjectManager style so multiple provider can
be registered at once, also we may want to tight the control of
battery provider with Profile API so we guarantee that the same entity
that handles the profile connection is the one providing the battery
status.

> +
> +
> +Battery Provider hierarchy
> +==========================
> +
> +Service                <client D-Bus address>
> +Interface      org.bluez.BatteryProvider1 [experimental]
> +Object path    {provider_root}/org/bluez/{hci0,hci1,...}/dev_XX_XX_XX_XX_XX_XX
> +
> +Properties     byte Percentage [readonly]
> +
> +                       The percentage of battery left as an unsigned 8-bit
> +                       integer.
> +
> +               string Source [readonly, optional]
> +
> +                       Describes where the battery information comes from
> +                       (e.g. "HFP 1.7", "HID").
> +                       This property is informational only and may be useful
> +                       for debugging purposes. The content of this property is
> +                       a free string, but it is recommended to include the
> +                       profile name and version to be useful.
> --
> 2.26.2

Perhaps we should make this use the same interface as we use for the
daemon itself (Battery1) and the Source there as well. Last but not
least, have you explored the idea of exporting the battery status over
uHID? If I got this right this would aggregate the status of different
sources and then make the daemon expose them, which while it works for
now it means that upper layer have different interfaces for handling a
battery status of something plugged over USB and over Bluetooth.


-- 
Luiz Augusto von Dentz

Re: [PATCH BlueZ v2 7/7] battery: Implement Battery Provider API

[Bastien Nocera]

Hey Sonny,

On Tue, 2020-11-10 at 17:17 -0800, Sonny Sasaka wrote:
> This patch implements the BatteryProvider1 and
> BatteryProviderManager1
> API. This is a means for external clients to feed battery information
> to
> BlueZ if they handle some profile and can decode battery reporting.
> 
> The battery information is then exposed externally via the existing
> Battery1 interface. UI components can consume this API to display
> Bluetooth peripherals' battery via a unified BlueZ API.

Was this patch reviewed for potential security problems? From the top
of my head, the possible problems would be:
- I don't see any filters on which user could register battery
providers, so on a multi user system, you could have a user logged in
via SSH squatting all the battery providers, while the user "at the
console" can't have their own providers. Also, what happens if the user
at the console changes (fast user switching)?
- It looks like battery providers don't check for paired, trusted or
even connected devices, so I would be able to create nearly unbound
number of battery providers depending on how big the cache for "seen"
devices is.

Given that the interface between upower and bluez is supposedly
trusted, it might be good to ensure that there are no fuzzing problems
on the bluez API side that could translate to causing problems in
upower itself.

I didn't review the code in depth, but, having written this mechanism
for Bluetooth battery reporting, I think that this is the right way to
go to allow daemons like pulseaudio to report battery status.

Cheers

Re: [PATCH BlueZ v2 7/7] battery: Implement Battery Provider API

[Bastien Nocera]

On Tue, 2020-11-17 at 11:51 +0100, Bastien Nocera wrote:
> <
> <snip> didn't review the code in depth, but, having written this
> mechanism
> for Bluetooth battery reporting, I think that this is the right way
> to
> go to allow daemons like pulseaudio to report battery status.

It would also be useful to know what external components, or internal
plugins you expect to feed this API.

Apparently HSP/HFP, for example, provide more information that what can
be expressed with the Battery1 API, whether it is charging or not, or
whether a battery level is unknown, etc.

So I would say that, even if the current battery API is extended, we
need to make sure that the D-Bus API to feed new data is extensible
enough to allow new properties, and they don't need to be added
separately to org.bluez.BatteryProvider1 or org.bluez.Battery1.

Re: [PATCH BlueZ v2 7/7] battery: Implement Battery Provider API

[Sonny Sasaka]

Hi Bastien,

Thank you for the feedback. Please find my answers below.

On Tue, Nov 17, 2020 at 2:51 AM Bastien Nocera <hadess@hadess.net> wrote:
>
> Hey Sonny,
>
> On Tue, 2020-11-10 at 17:17 -0800, Sonny Sasaka wrote:
> > This patch implements the BatteryProvider1 and
> > BatteryProviderManager1
> > API. This is a means for external clients to feed battery information
> > to
> > BlueZ if they handle some profile and can decode battery reporting.
> >
> > The battery information is then exposed externally via the existing
> > Battery1 interface. UI components can consume this API to display
> > Bluetooth peripherals' battery via a unified BlueZ API.
>
> Was this patch reviewed for potential security problems? From the top
> of my head, the possible problems would be:
> - I don't see any filters on which user could register battery
> providers, so on a multi user system, you could have a user logged in
> via SSH squatting all the battery providers, while the user "at the
> console" can't have their own providers. Also, what happens if the user
> at the console changes (fast user switching)?
> - It looks like battery providers don't check for paired, trusted or
> even connected devices, so I would be able to create nearly unbound
> number of battery providers depending on how big the cache for "seen"
> devices is.
For security, the API can be access-limited at D-Bus level using D-Bus
configuration files. For example, we can let only trusted UNIX users
as the callers for this API. This D-Bus config file would be
distribution-specific. In Chrome OS, for example, only the "audio" and
"power" users are allowed to call this API. This way we can make sure
that the callers do not abuse the API for denial-of-service kind of
attack.

>
> Given that the interface between upower and bluez is supposedly
> trusted, it might be good to ensure that there are no fuzzing problems
> on the bluez API side that could translate to causing problems in
> upower itself.
Could you give an example of what potential problems of upower can be
caused by communicating with BlueZ through this API?

>
> I didn't review the code in depth, but, having written this mechanism
> for Bluetooth battery reporting, I think that this is the right way to
> go to allow daemons like pulseaudio to report battery status.
>
> Cheers
>

Re: [PATCH BlueZ v2 7/7] battery: Implement Battery Provider API

[Sonny Sasaka]

Hi Bastien,

More responses below.

On Tue, Nov 17, 2020 at 10:01 AM Bastien Nocera <hadess@hadess.net> wrote:
>
> On Tue, 2020-11-17 at 11:51 +0100, Bastien Nocera wrote:
> > <
> > <snip> didn't review the code in depth, but, having written this
> > mechanism
> > for Bluetooth battery reporting, I think that this is the right way
> > to
> > go to allow daemons like pulseaudio to report battery status.
>
> It would also be useful to know what external components, or internal
> plugins you expect to feed this API.
BlueZ could have internal plugins to read proprietary battery
reporting, e.g. JBL speakers and Bose QC35.

>
> Apparently HSP/HFP, for example, provide more information that what can
> be expressed with the Battery1 API, whether it is charging or not, or
> whether a battery level is unknown, etc.
>
> So I would say that, even if the current battery API is extended, we
> need to make sure that the D-Bus API to feed new data is extensible
> enough to allow new properties, and they don't need to be added
> separately to org.bluez.BatteryProvider1 or org.bluez.Battery1.
I proposed the API to start simple, but I believe that it can always
be extended as we need in the future so I don't think the additional
features need to be coded now. I documented how this API could evolve
in the future by extending other features as well in this design
document that I shared with Luiz and Marcel:
https://docs.google.com/document/d/1OV4sjHLhTzB91D7LyTVT6R0SX2vXwSG1IA3q5yWQWps.

>

Re: [PATCH BlueZ v2 7/7] battery: Implement Battery Provider API

[Sonny Sasaka]

Hi Bastien,

On Tue, Nov 17, 2020 at 2:22 PM Sonny Sasaka <sonnysasaka@chromium.org> wrote:
>
> Hi Bastien,
>
> More responses below.
>
> On Tue, Nov 17, 2020 at 10:01 AM Bastien Nocera <hadess@hadess.net> wrote:
> >
> > On Tue, 2020-11-17 at 11:51 +0100, Bastien Nocera wrote:
> > > <
> > > <snip> didn't review the code in depth, but, having written this
> > > mechanism
> > > for Bluetooth battery reporting, I think that this is the right way
> > > to
> > > go to allow daemons like pulseaudio to report battery status.
> >
> > It would also be useful to know what external components, or internal
> > plugins you expect to feed this API.
> BlueZ could have internal plugins to read proprietary battery
> reporting, e.g. JBL speakers and Bose QC35.
Adding to mention that in Chrome OS we also plan to have a plugin to
decode Pixel Buds 2 multiple battery values (left, right, and case).
We have not yet decided whether this is going to be internal plugin or
external client though. But generally the API should allow this kind
of feature as well.


>
> >
> > Apparently HSP/HFP, for example, provide more information that what can
> > be expressed with the Battery1 API, whether it is charging or not, or
> > whether a battery level is unknown, etc.
> >
> > So I would say that, even if the current battery API is extended, we
> > need to make sure that the D-Bus API to feed new data is extensible
> > enough to allow new properties, and they don't need to be added
> > separately to org.bluez.BatteryProvider1 or org.bluez.Battery1.
> I proposed the API to start simple, but I believe that it can always
> be extended as we need in the future so I don't think the additional
> features need to be coded now. I documented how this API could evolve
> in the future by extending other features as well in this design
> document that I shared with Luiz and Marcel:
> https://docs.google.com/document/d/1OV4sjHLhTzB91D7LyTVT6R0SX2vXwSG1IA3q5yWQWps.
>
> >

Re: [PATCH BlueZ v2 7/7] battery: Implement Battery Provider API

[Luiz Augusto von Dentz]

Hi Sonny,

On Tue, Nov 17, 2020 at 2:20 PM Sonny Sasaka <sonnysasaka@chromium.org> wrote:
>
> Hi Bastien,
>
> Thank you for the feedback. Please find my answers below.
>
> On Tue, Nov 17, 2020 at 2:51 AM Bastien Nocera <hadess@hadess.net> wrote:
> >
> > Hey Sonny,
> >
> > On Tue, 2020-11-10 at 17:17 -0800, Sonny Sasaka wrote:
> > > This patch implements the BatteryProvider1 and
> > > BatteryProviderManager1
> > > API. This is a means for external clients to feed battery information
> > > to
> > > BlueZ if they handle some profile and can decode battery reporting.
> > >
> > > The battery information is then exposed externally via the existing
> > > Battery1 interface. UI components can consume this API to display
> > > Bluetooth peripherals' battery via a unified BlueZ API.
> >
> > Was this patch reviewed for potential security problems? From the top
> > of my head, the possible problems would be:
> > - I don't see any filters on which user could register battery
> > providers, so on a multi user system, you could have a user logged in
> > via SSH squatting all the battery providers, while the user "at the
> > console" can't have their own providers. Also, what happens if the user
> > at the console changes (fast user switching)?
> > - It looks like battery providers don't check for paired, trusted or
> > even connected devices, so I would be able to create nearly unbound
> > number of battery providers depending on how big the cache for "seen"
> > devices is.
> For security, the API can be access-limited at D-Bus level using D-Bus
> configuration files. For example, we can let only trusted UNIX users
> as the callers for this API. This D-Bus config file would be
> distribution-specific. In Chrome OS, for example, only the "audio" and
> "power" users are allowed to call this API. This way we can make sure
> that the callers do not abuse the API for denial-of-service kind of
> attack.

I guess there is still some the risk of conflicts even with the use of
D-Bus policy blocking roge applications, there could still be multiple
entities providing the battery status from the same source, which is
why I suggested we couple the Battery1 with the Profile1, so only the
entity that has registered to handle let say HFP can provide a battery
status and we automatically deduce the source is from HFP.

> >
> > Given that the interface between upower and bluez is supposedly
> > trusted, it might be good to ensure that there are no fuzzing problems
> > on the bluez API side that could translate to causing problems in
> > upower itself.
> Could you give an example of what potential problems of upower can be
> caused by communicating with BlueZ through this API?
>
> >
> > I didn't review the code in depth, but, having written this mechanism
> > for Bluetooth battery reporting, I think that this is the right way to
> > go to allow daemons like pulseaudio to report battery status.
> >
> > Cheers
> >



-- 
Luiz Augusto von Dentz

Re: [PATCH BlueZ v2 4/7] doc: Add Battery Provider API doc

[Sonny Sasaka]

Hi Luiz,

Thanks for the feedback. Please find my responses below.

On Mon, Nov 16, 2020 at 4:17 PM Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
>
> Hi Sonny,
>
> On Tue, Nov 10, 2020 at 5:22 PM Sonny Sasaka <sonnysasaka@chromium.org> wrote:
> >
> > This patch add the documentation of the Battery Provider which lets
> > external clients feed battery information to BlueZ if they are able to
> > decode battery reporting via any profile. BlueZ UI clients can then use
> > the org.bluez.Battery1 API as a single source of battery information
> > coming from many different profiles.
> >
> > Reviewed-by: Miao-chen Chou <mcchou@chromium.org>
> >
> > ---
> >  doc/battery-api.txt | 55 +++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 55 insertions(+)
> >
> > diff --git a/doc/battery-api.txt b/doc/battery-api.txt
> > index dc7dbeda2..058bf0c6e 100644
> > --- a/doc/battery-api.txt
> > +++ b/doc/battery-api.txt
> > @@ -12,3 +12,58 @@ Object path  [variable prefix]/{hci0,hci1,...}/dev_XX_XX_XX_XX_XX_XX
> >  Properties     byte Percentage [readonly]
> >
> >                         The percentage of battery left as an unsigned 8-bit integer.
> > +
> > +               string Source [readonly, optional, experimental]
> > +
> > +                       Describes where the battery information comes from
> > +                       (e.g. "HFP 1.7", "HID").
> > +                       This property is informational only and may be useful
> > +                       for debugging purposes.
>
> We should probably tight this to UUID instead.
Ack. Will update the doc to suggest UUID in this source field.

>
> > +
> > +
> > +Battery Provider Manager hierarchy
> > +==================================
> > +A battery provider starts by registering itself as a battery provider with the
> > +RegisterBatteryProvider method passing an object path as the provider ID. Then,
> > +it can start exposing org.bluez.BatteryProvider1 objects having the path
> > +starting with the given provider ID. It can also remove objects at any time.
> > +BlueZ will stop monitoring these exposed and removed objects after
> > +UnregisterBatteryProvider is called for that provider ID.
> > +
> > +Service                org.bluez
> > +Interface      org.bluez.BatteryProviderManager1 [experimental]
> > +Object path    /org/bluez/{hci0,hci1,...}
> > +
> > +Methods                void RegisterBatteryProvider(object provider)
> > +
> > +                       This registers a battery provider. A registered
> > +                       battery provider can then expose objects with
> > +                       org.bluez.BatteryProvider1 interface described below.
> > +
> > +               void UnregisterBatteryProvider(object provider)
> > +
> > +                       This unregisters a battery provider. After
> > +                       unregistration, the BatteryProvider1 objects provided
> > +                       by this client are ignored by BlueZ.
>
> Not sure if you were followinging the monitor patches, for registering
> objects we do prefer the ObjectManager style so multiple provider can
> be registered at once, also we may want to tight the control of
> battery provider with Profile API so we guarantee that the same entity
> that handles the profile connection is the one providing the battery
> status.
It is actually already in ObjectManager style. After the "root path"
is registered, the provider can expose many D-Bus objects at once and
bluetoothd can detect it.
About tying it with the Profile API, I will take a look at how it could be done.

>
> > +
> > +
> > +Battery Provider hierarchy
> > +==========================
> > +
> > +Service                <client D-Bus address>
> > +Interface      org.bluez.BatteryProvider1 [experimental]
> > +Object path    {provider_root}/org/bluez/{hci0,hci1,...}/dev_XX_XX_XX_XX_XX_XX
> > +
> > +Properties     byte Percentage [readonly]
> > +
> > +                       The percentage of battery left as an unsigned 8-bit
> > +                       integer.
> > +
> > +               string Source [readonly, optional]
> > +
> > +                       Describes where the battery information comes from
> > +                       (e.g. "HFP 1.7", "HID").
> > +                       This property is informational only and may be useful
> > +                       for debugging purposes. The content of this property is
> > +                       a free string, but it is recommended to include the
> > +                       profile name and version to be useful.
> > --
> > 2.26.2
>
> Perhaps we should make this use the same interface as we use for the
> daemon itself (Battery1) and the Source there as well. Last but not
> least, have you explored the idea of exporting the battery status over
> uHID? If I got this right this would aggregate the status of different
> sources and then make the daemon expose them, which while it works for
> now it means that upper layer have different interfaces for handling a
> battery status of something plugged over USB and over Bluetooth.
I am okay with naming the interface Battery1, the same as the daemon.
Will make an update.
About the exporting battery status via UHID, it is possible to be
done, but it is an orthogonal problem that we plan to tackle
separately. Right now, this API is general enough for Chrome OS to
allow both HFP and HID batteries to be consolidated in BlueZ. Chrome
OS's powerd feeds only Bluetooth battery levels from
/sys/class/power_supply and filters out USB devices so the UI layer
does not need to worry about it: everything from BlueZ is tied to a
Bluetooth device.

>
>
> --
> Luiz Augusto von Dentz

Re: [PATCH BlueZ v2 4/7] doc: Add Battery Provider API doc

[Luiz Augusto von Dentz]

Hi Sonny,

On Tue, Nov 17, 2020 at 2:37 PM Sonny Sasaka <sonnysasaka@chromium.org> wrote:
>
> Hi Luiz,
>
> Thanks for the feedback. Please find my responses below.
>
> On Mon, Nov 16, 2020 at 4:17 PM Luiz Augusto von Dentz
> <luiz.dentz@gmail.com> wrote:
> >
> > Hi Sonny,
> >
> > On Tue, Nov 10, 2020 at 5:22 PM Sonny Sasaka <sonnysasaka@chromium.org> wrote:
> > >
> > > This patch add the documentation of the Battery Provider which lets
> > > external clients feed battery information to BlueZ if they are able to
> > > decode battery reporting via any profile. BlueZ UI clients can then use
> > > the org.bluez.Battery1 API as a single source of battery information
> > > coming from many different profiles.
> > >
> > > Reviewed-by: Miao-chen Chou <mcchou@chromium.org>
> > >
> > > ---
> > >  doc/battery-api.txt | 55 +++++++++++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 55 insertions(+)
> > >
> > > diff --git a/doc/battery-api.txt b/doc/battery-api.txt
> > > index dc7dbeda2..058bf0c6e 100644
> > > --- a/doc/battery-api.txt
> > > +++ b/doc/battery-api.txt
> > > @@ -12,3 +12,58 @@ Object path  [variable prefix]/{hci0,hci1,...}/dev_XX_XX_XX_XX_XX_XX
> > >  Properties     byte Percentage [readonly]
> > >
> > >                         The percentage of battery left as an unsigned 8-bit integer.
> > > +
> > > +               string Source [readonly, optional, experimental]
> > > +
> > > +                       Describes where the battery information comes from
> > > +                       (e.g. "HFP 1.7", "HID").
> > > +                       This property is informational only and may be useful
> > > +                       for debugging purposes.
> >
> > We should probably tight this to UUID instead.
> Ack. Will update the doc to suggest UUID in this source field.
>
> >
> > > +
> > > +
> > > +Battery Provider Manager hierarchy
> > > +==================================
> > > +A battery provider starts by registering itself as a battery provider with the
> > > +RegisterBatteryProvider method passing an object path as the provider ID. Then,
> > > +it can start exposing org.bluez.BatteryProvider1 objects having the path
> > > +starting with the given provider ID. It can also remove objects at any time.
> > > +BlueZ will stop monitoring these exposed and removed objects after
> > > +UnregisterBatteryProvider is called for that provider ID.
> > > +
> > > +Service                org.bluez
> > > +Interface      org.bluez.BatteryProviderManager1 [experimental]
> > > +Object path    /org/bluez/{hci0,hci1,...}
> > > +
> > > +Methods                void RegisterBatteryProvider(object provider)
> > > +
> > > +                       This registers a battery provider. A registered
> > > +                       battery provider can then expose objects with
> > > +                       org.bluez.BatteryProvider1 interface described below.
> > > +
> > > +               void UnregisterBatteryProvider(object provider)
> > > +
> > > +                       This unregisters a battery provider. After
> > > +                       unregistration, the BatteryProvider1 objects provided
> > > +                       by this client are ignored by BlueZ.
> >
> > Not sure if you were followinging the monitor patches, for registering
> > objects we do prefer the ObjectManager style so multiple provider can
> > be registered at once, also we may want to tight the control of
> > battery provider with Profile API so we guarantee that the same entity
> > that handles the profile connection is the one providing the battery
> > status.
> It is actually already in ObjectManager style. After the "root path"
> is registered, the provider can expose many D-Bus objects at once and
> bluetoothd can detect it.
> About tying it with the Profile API, I will take a look at how it could be done.
>
> >
> > > +
> > > +
> > > +Battery Provider hierarchy
> > > +==========================
> > > +
> > > +Service                <client D-Bus address>
> > > +Interface      org.bluez.BatteryProvider1 [experimental]
> > > +Object path    {provider_root}/org/bluez/{hci0,hci1,...}/dev_XX_XX_XX_XX_XX_XX
> > > +
> > > +Properties     byte Percentage [readonly]
> > > +
> > > +                       The percentage of battery left as an unsigned 8-bit
> > > +                       integer.
> > > +
> > > +               string Source [readonly, optional]
> > > +
> > > +                       Describes where the battery information comes from
> > > +                       (e.g. "HFP 1.7", "HID").
> > > +                       This property is informational only and may be useful
> > > +                       for debugging purposes. The content of this property is
> > > +                       a free string, but it is recommended to include the
> > > +                       profile name and version to be useful.
> > > --
> > > 2.26.2
> >
> > Perhaps we should make this use the same interface as we use for the
> > daemon itself (Battery1) and the Source there as well. Last but not
> > least, have you explored the idea of exporting the battery status over
> > uHID? If I got this right this would aggregate the status of different
> > sources and then make the daemon expose them, which while it works for
> > now it means that upper layer have different interfaces for handling a
> > battery status of something plugged over USB and over Bluetooth.
> I am okay with naming the interface Battery1, the same as the daemon.
> Will make an update.
> About the exporting battery status via UHID, it is possible to be
> done, but it is an orthogonal problem that we plan to tackle
> separately. Right now, this API is general enough for Chrome OS to
> allow both HFP and HID batteries to be consolidated in BlueZ. Chrome
> OS's powerd feeds only Bluetooth battery levels from
> /sys/class/power_supply and filters out USB devices so the UI layer
> does not need to worry about it: everything from BlueZ is tied to a
> Bluetooth device.

But how about devices pushing their battery status over HID reports
instead of GATT? Afaik this is possible since the HID device may have
support for USB (wired) as well where it exposes its battery status
over HID it may just choose to continue doing so while connected over
Bluetooth.

> >
> >
> > --
> > Luiz Augusto von Dentz



-- 
Luiz Augusto von Dentz

Re: [PATCH BlueZ v2 4/7] doc: Add Battery Provider API doc

[Sonny Sasaka]

Hi Luiz,

On Tue, Nov 17, 2020 at 3:01 PM Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
>
> Hi Sonny,
>
> On Tue, Nov 17, 2020 at 2:37 PM Sonny Sasaka <sonnysasaka@chromium.org> wrote:
> >
> > Hi Luiz,
> >
> > Thanks for the feedback. Please find my responses below.
> >
> > On Mon, Nov 16, 2020 at 4:17 PM Luiz Augusto von Dentz
> > <luiz.dentz@gmail.com> wrote:
> > >
> > > Hi Sonny,
> > >
> > > On Tue, Nov 10, 2020 at 5:22 PM Sonny Sasaka <sonnysasaka@chromium.org> wrote:
> > > >
> > > > This patch add the documentation of the Battery Provider which lets
> > > > external clients feed battery information to BlueZ if they are able to
> > > > decode battery reporting via any profile. BlueZ UI clients can then use
> > > > the org.bluez.Battery1 API as a single source of battery information
> > > > coming from many different profiles.
> > > >
> > > > Reviewed-by: Miao-chen Chou <mcchou@chromium.org>
> > > >
> > > > ---
> > > >  doc/battery-api.txt | 55 +++++++++++++++++++++++++++++++++++++++++++++
> > > >  1 file changed, 55 insertions(+)
> > > >
> > > > diff --git a/doc/battery-api.txt b/doc/battery-api.txt
> > > > index dc7dbeda2..058bf0c6e 100644
> > > > --- a/doc/battery-api.txt
> > > > +++ b/doc/battery-api.txt
> > > > @@ -12,3 +12,58 @@ Object path  [variable prefix]/{hci0,hci1,...}/dev_XX_XX_XX_XX_XX_XX
> > > >  Properties     byte Percentage [readonly]
> > > >
> > > >                         The percentage of battery left as an unsigned 8-bit integer.
> > > > +
> > > > +               string Source [readonly, optional, experimental]
> > > > +
> > > > +                       Describes where the battery information comes from
> > > > +                       (e.g. "HFP 1.7", "HID").
> > > > +                       This property is informational only and may be useful
> > > > +                       for debugging purposes.
> > >
> > > We should probably tight this to UUID instead.
> > Ack. Will update the doc to suggest UUID in this source field.
> >
> > >
> > > > +
> > > > +
> > > > +Battery Provider Manager hierarchy
> > > > +==================================
> > > > +A battery provider starts by registering itself as a battery provider with the
> > > > +RegisterBatteryProvider method passing an object path as the provider ID. Then,
> > > > +it can start exposing org.bluez.BatteryProvider1 objects having the path
> > > > +starting with the given provider ID. It can also remove objects at any time.
> > > > +BlueZ will stop monitoring these exposed and removed objects after
> > > > +UnregisterBatteryProvider is called for that provider ID.
> > > > +
> > > > +Service                org.bluez
> > > > +Interface      org.bluez.BatteryProviderManager1 [experimental]
> > > > +Object path    /org/bluez/{hci0,hci1,...}
> > > > +
> > > > +Methods                void RegisterBatteryProvider(object provider)
> > > > +
> > > > +                       This registers a battery provider. A registered
> > > > +                       battery provider can then expose objects with
> > > > +                       org.bluez.BatteryProvider1 interface described below.
> > > > +
> > > > +               void UnregisterBatteryProvider(object provider)
> > > > +
> > > > +                       This unregisters a battery provider. After
> > > > +                       unregistration, the BatteryProvider1 objects provided
> > > > +                       by this client are ignored by BlueZ.
> > >
> > > Not sure if you were followinging the monitor patches, for registering
> > > objects we do prefer the ObjectManager style so multiple provider can
> > > be registered at once, also we may want to tight the control of
> > > battery provider with Profile API so we guarantee that the same entity
> > > that handles the profile connection is the one providing the battery
> > > status.
> > It is actually already in ObjectManager style. After the "root path"
> > is registered, the provider can expose many D-Bus objects at once and
> > bluetoothd can detect it.
> > About tying it with the Profile API, I will take a look at how it could be done.
> >
> > >
> > > > +
> > > > +
> > > > +Battery Provider hierarchy
> > > > +==========================
> > > > +
> > > > +Service                <client D-Bus address>
> > > > +Interface      org.bluez.BatteryProvider1 [experimental]
> > > > +Object path    {provider_root}/org/bluez/{hci0,hci1,...}/dev_XX_XX_XX_XX_XX_XX
> > > > +
> > > > +Properties     byte Percentage [readonly]
> > > > +
> > > > +                       The percentage of battery left as an unsigned 8-bit
> > > > +                       integer.
> > > > +
> > > > +               string Source [readonly, optional]
> > > > +
> > > > +                       Describes where the battery information comes from
> > > > +                       (e.g. "HFP 1.7", "HID").
> > > > +                       This property is informational only and may be useful
> > > > +                       for debugging purposes. The content of this property is
> > > > +                       a free string, but it is recommended to include the
> > > > +                       profile name and version to be useful.
> > > > --
> > > > 2.26.2
> > >
> > > Perhaps we should make this use the same interface as we use for the
> > > daemon itself (Battery1) and the Source there as well. Last but not
> > > least, have you explored the idea of exporting the battery status over
> > > uHID? If I got this right this would aggregate the status of different
> > > sources and then make the daemon expose them, which while it works for
> > > now it means that upper layer have different interfaces for handling a
> > > battery status of something plugged over USB and over Bluetooth.
> > I am okay with naming the interface Battery1, the same as the daemon.
> > Will make an update.
> > About the exporting battery status via UHID, it is possible to be
> > done, but it is an orthogonal problem that we plan to tackle
> > separately. Right now, this API is general enough for Chrome OS to
> > allow both HFP and HID batteries to be consolidated in BlueZ. Chrome
> > OS's powerd feeds only Bluetooth battery levels from
> > /sys/class/power_supply and filters out USB devices so the UI layer
> > does not need to worry about it: everything from BlueZ is tied to a
> > Bluetooth device.
>
> But how about devices pushing their battery status over HID reports
> instead of GATT? Afaik this is possible since the HID device may have
> support for USB (wired) as well where it exposes its battery status
> over HID it may just choose to continue doing so while connected over
> Bluetooth.
If the Bluetooth device reports battery status via HID, it will go
into sys/class/power_supply. In Chrome OS, powerd sends this back to
BlueZ because it knows the Bluetooth device address from
/sys/class/power_supply/ file name. This way the UI can get the
battery value from BlueZ since it's tied to a Bluetooth device. In
most Linux desktops, upower already exposes /sys/class/power_supply to
the UI directly so it's fine also.
>
> > >
> > >
> > > --
> > > Luiz Augusto von Dentz
>
>
>
> --
> Luiz Augusto von Dentz

Re: [PATCH BlueZ v2 4/7] doc: Add Battery Provider API doc

[Luiz Augusto von Dentz]

Hi Sonny,

On Tue, Nov 17, 2020 at 3:17 PM Sonny Sasaka <sonnysasaka@chromium.org> wrote:
>
> Hi Luiz,
>
> On Tue, Nov 17, 2020 at 3:01 PM Luiz Augusto von Dentz
> <luiz.dentz@gmail.com> wrote:
> >
> > Hi Sonny,
> >
> > On Tue, Nov 17, 2020 at 2:37 PM Sonny Sasaka <sonnysasaka@chromium.org> wrote:
> > >
> > > Hi Luiz,
> > >
> > > Thanks for the feedback. Please find my responses below.
> > >
> > > On Mon, Nov 16, 2020 at 4:17 PM Luiz Augusto von Dentz
> > > <luiz.dentz@gmail.com> wrote:
> > > >
> > > > Hi Sonny,
> > > >
> > > > On Tue, Nov 10, 2020 at 5:22 PM Sonny Sasaka <sonnysasaka@chromium.org> wrote:
> > > > >
> > > > > This patch add the documentation of the Battery Provider which lets
> > > > > external clients feed battery information to BlueZ if they are able to
> > > > > decode battery reporting via any profile. BlueZ UI clients can then use
> > > > > the org.bluez.Battery1 API as a single source of battery information
> > > > > coming from many different profiles.
> > > > >
> > > > > Reviewed-by: Miao-chen Chou <mcchou@chromium.org>
> > > > >
> > > > > ---
> > > > >  doc/battery-api.txt | 55 +++++++++++++++++++++++++++++++++++++++++++++
> > > > >  1 file changed, 55 insertions(+)
> > > > >
> > > > > diff --git a/doc/battery-api.txt b/doc/battery-api.txt
> > > > > index dc7dbeda2..058bf0c6e 100644
> > > > > --- a/doc/battery-api.txt
> > > > > +++ b/doc/battery-api.txt
> > > > > @@ -12,3 +12,58 @@ Object path  [variable prefix]/{hci0,hci1,...}/dev_XX_XX_XX_XX_XX_XX
> > > > >  Properties     byte Percentage [readonly]
> > > > >
> > > > >                         The percentage of battery left as an unsigned 8-bit integer.
> > > > > +
> > > > > +               string Source [readonly, optional, experimental]
> > > > > +
> > > > > +                       Describes where the battery information comes from
> > > > > +                       (e.g. "HFP 1.7", "HID").
> > > > > +                       This property is informational only and may be useful
> > > > > +                       for debugging purposes.
> > > >
> > > > We should probably tight this to UUID instead.
> > > Ack. Will update the doc to suggest UUID in this source field.
> > >
> > > >
> > > > > +
> > > > > +
> > > > > +Battery Provider Manager hierarchy
> > > > > +==================================
> > > > > +A battery provider starts by registering itself as a battery provider with the
> > > > > +RegisterBatteryProvider method passing an object path as the provider ID. Then,
> > > > > +it can start exposing org.bluez.BatteryProvider1 objects having the path
> > > > > +starting with the given provider ID. It can also remove objects at any time.
> > > > > +BlueZ will stop monitoring these exposed and removed objects after
> > > > > +UnregisterBatteryProvider is called for that provider ID.
> > > > > +
> > > > > +Service                org.bluez
> > > > > +Interface      org.bluez.BatteryProviderManager1 [experimental]
> > > > > +Object path    /org/bluez/{hci0,hci1,...}
> > > > > +
> > > > > +Methods                void RegisterBatteryProvider(object provider)
> > > > > +
> > > > > +                       This registers a battery provider. A registered
> > > > > +                       battery provider can then expose objects with
> > > > > +                       org.bluez.BatteryProvider1 interface described below.
> > > > > +
> > > > > +               void UnregisterBatteryProvider(object provider)
> > > > > +
> > > > > +                       This unregisters a battery provider. After
> > > > > +                       unregistration, the BatteryProvider1 objects provided
> > > > > +                       by this client are ignored by BlueZ.
> > > >
> > > > Not sure if you were followinging the monitor patches, for registering
> > > > objects we do prefer the ObjectManager style so multiple provider can
> > > > be registered at once, also we may want to tight the control of
> > > > battery provider with Profile API so we guarantee that the same entity
> > > > that handles the profile connection is the one providing the battery
> > > > status.
> > > It is actually already in ObjectManager style. After the "root path"
> > > is registered, the provider can expose many D-Bus objects at once and
> > > bluetoothd can detect it.
> > > About tying it with the Profile API, I will take a look at how it could be done.
> > >
> > > >
> > > > > +
> > > > > +
> > > > > +Battery Provider hierarchy
> > > > > +==========================
> > > > > +
> > > > > +Service                <client D-Bus address>
> > > > > +Interface      org.bluez.BatteryProvider1 [experimental]
> > > > > +Object path    {provider_root}/org/bluez/{hci0,hci1,...}/dev_XX_XX_XX_XX_XX_XX
> > > > > +
> > > > > +Properties     byte Percentage [readonly]
> > > > > +
> > > > > +                       The percentage of battery left as an unsigned 8-bit
> > > > > +                       integer.
> > > > > +
> > > > > +               string Source [readonly, optional]
> > > > > +
> > > > > +                       Describes where the battery information comes from
> > > > > +                       (e.g. "HFP 1.7", "HID").
> > > > > +                       This property is informational only and may be useful
> > > > > +                       for debugging purposes. The content of this property is
> > > > > +                       a free string, but it is recommended to include the
> > > > > +                       profile name and version to be useful.
> > > > > --
> > > > > 2.26.2
> > > >
> > > > Perhaps we should make this use the same interface as we use for the
> > > > daemon itself (Battery1) and the Source there as well. Last but not
> > > > least, have you explored the idea of exporting the battery status over
> > > > uHID? If I got this right this would aggregate the status of different
> > > > sources and then make the daemon expose them, which while it works for
> > > > now it means that upper layer have different interfaces for handling a
> > > > battery status of something plugged over USB and over Bluetooth.
> > > I am okay with naming the interface Battery1, the same as the daemon.
> > > Will make an update.
> > > About the exporting battery status via UHID, it is possible to be
> > > done, but it is an orthogonal problem that we plan to tackle
> > > separately. Right now, this API is general enough for Chrome OS to
> > > allow both HFP and HID batteries to be consolidated in BlueZ. Chrome
> > > OS's powerd feeds only Bluetooth battery levels from
> > > /sys/class/power_supply and filters out USB devices so the UI layer
> > > does not need to worry about it: everything from BlueZ is tied to a
> > > Bluetooth device.
> >
> > But how about devices pushing their battery status over HID reports
> > instead of GATT? Afaik this is possible since the HID device may have
> > support for USB (wired) as well where it exposes its battery status
> > over HID it may just choose to continue doing so while connected over
> > Bluetooth.
> If the Bluetooth device reports battery status via HID, it will go
> into sys/class/power_supply. In Chrome OS, powerd sends this back to
> BlueZ because it knows the Bluetooth device address from
> /sys/class/power_supply/ file name.

Well that is something Id like to avoid since if we do in the future
create an HID report this sort of logic could cause duplications as it
either the daemon or the kernel may start parsing these so we will
need some way to identify each source (perhaps by UUID) to avoid
duplications. Also having differences in upower and powerd really
doesn't help us unifying the handling here, I hope at least for
Bluetooth we end up with something similar when either daemons are
used, otherwise we may need plugins for each separately.

> >
> > > >
> > > >
> > > > --
> > > > Luiz Augusto von Dentz
> >
> >
> >
> > --
> > Luiz Augusto von Dentz



-- 
Luiz Augusto von Dentz

Re: [PATCH BlueZ v2 4/7] doc: Add Battery Provider API doc

[Sonny Sasaka]

Hi Luiz,


On Tue, Nov 17, 2020 at 3:33 PM Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
>
> Hi Sonny,
>
> On Tue, Nov 17, 2020 at 3:17 PM Sonny Sasaka <sonnysasaka@chromium.org> wrote:
> >
> > Hi Luiz,
> >
> > On Tue, Nov 17, 2020 at 3:01 PM Luiz Augusto von Dentz
> > <luiz.dentz@gmail.com> wrote:
> > >
> > > Hi Sonny,
> > >
> > > On Tue, Nov 17, 2020 at 2:37 PM Sonny Sasaka <sonnysasaka@chromium.org> wrote:
> > > >
> > > > Hi Luiz,
> > > >
> > > > Thanks for the feedback. Please find my responses below.
> > > >
> > > > On Mon, Nov 16, 2020 at 4:17 PM Luiz Augusto von Dentz
> > > > <luiz.dentz@gmail.com> wrote:
> > > > >
> > > > > Hi Sonny,
> > > > >
> > > > > On Tue, Nov 10, 2020 at 5:22 PM Sonny Sasaka <sonnysasaka@chromium.org> wrote:
> > > > > >
> > > > > > This patch add the documentation of the Battery Provider which lets
> > > > > > external clients feed battery information to BlueZ if they are able to
> > > > > > decode battery reporting via any profile. BlueZ UI clients can then use
> > > > > > the org.bluez.Battery1 API as a single source of battery information
> > > > > > coming from many different profiles.
> > > > > >
> > > > > > Reviewed-by: Miao-chen Chou <mcchou@chromium.org>
> > > > > >
> > > > > > ---
> > > > > >  doc/battery-api.txt | 55 +++++++++++++++++++++++++++++++++++++++++++++
> > > > > >  1 file changed, 55 insertions(+)
> > > > > >
> > > > > > diff --git a/doc/battery-api.txt b/doc/battery-api.txt
> > > > > > index dc7dbeda2..058bf0c6e 100644
> > > > > > --- a/doc/battery-api.txt
> > > > > > +++ b/doc/battery-api.txt
> > > > > > @@ -12,3 +12,58 @@ Object path  [variable prefix]/{hci0,hci1,...}/dev_XX_XX_XX_XX_XX_XX
> > > > > >  Properties     byte Percentage [readonly]
> > > > > >
> > > > > >                         The percentage of battery left as an unsigned 8-bit integer.
> > > > > > +
> > > > > > +               string Source [readonly, optional, experimental]
> > > > > > +
> > > > > > +                       Describes where the battery information comes from
> > > > > > +                       (e.g. "HFP 1.7", "HID").
> > > > > > +                       This property is informational only and may be useful
> > > > > > +                       for debugging purposes.
> > > > >
> > > > > We should probably tight this to UUID instead.
> > > > Ack. Will update the doc to suggest UUID in this source field.
> > > >
> > > > >
> > > > > > +
> > > > > > +
> > > > > > +Battery Provider Manager hierarchy
> > > > > > +==================================
> > > > > > +A battery provider starts by registering itself as a battery provider with the
> > > > > > +RegisterBatteryProvider method passing an object path as the provider ID. Then,
> > > > > > +it can start exposing org.bluez.BatteryProvider1 objects having the path
> > > > > > +starting with the given provider ID. It can also remove objects at any time.
> > > > > > +BlueZ will stop monitoring these exposed and removed objects after
> > > > > > +UnregisterBatteryProvider is called for that provider ID.
> > > > > > +
> > > > > > +Service                org.bluez
> > > > > > +Interface      org.bluez.BatteryProviderManager1 [experimental]
> > > > > > +Object path    /org/bluez/{hci0,hci1,...}
> > > > > > +
> > > > > > +Methods                void RegisterBatteryProvider(object provider)
> > > > > > +
> > > > > > +                       This registers a battery provider. A registered
> > > > > > +                       battery provider can then expose objects with
> > > > > > +                       org.bluez.BatteryProvider1 interface described below.
> > > > > > +
> > > > > > +               void UnregisterBatteryProvider(object provider)
> > > > > > +
> > > > > > +                       This unregisters a battery provider. After
> > > > > > +                       unregistration, the BatteryProvider1 objects provided
> > > > > > +                       by this client are ignored by BlueZ.
> > > > >
> > > > > Not sure if you were followinging the monitor patches, for registering
> > > > > objects we do prefer the ObjectManager style so multiple provider can
> > > > > be registered at once, also we may want to tight the control of
> > > > > battery provider with Profile API so we guarantee that the same entity
> > > > > that handles the profile connection is the one providing the battery
> > > > > status.
> > > > It is actually already in ObjectManager style. After the "root path"
> > > > is registered, the provider can expose many D-Bus objects at once and
> > > > bluetoothd can detect it.
> > > > About tying it with the Profile API, I will take a look at how it could be done.
> > > >
> > > > >
> > > > > > +
> > > > > > +
> > > > > > +Battery Provider hierarchy
> > > > > > +==========================
> > > > > > +
> > > > > > +Service                <client D-Bus address>
> > > > > > +Interface      org.bluez.BatteryProvider1 [experimental]
> > > > > > +Object path    {provider_root}/org/bluez/{hci0,hci1,...}/dev_XX_XX_XX_XX_XX_XX
> > > > > > +
> > > > > > +Properties     byte Percentage [readonly]
> > > > > > +
> > > > > > +                       The percentage of battery left as an unsigned 8-bit
> > > > > > +                       integer.
> > > > > > +
> > > > > > +               string Source [readonly, optional]
> > > > > > +
> > > > > > +                       Describes where the battery information comes from
> > > > > > +                       (e.g. "HFP 1.7", "HID").
> > > > > > +                       This property is informational only and may be useful
> > > > > > +                       for debugging purposes. The content of this property is
> > > > > > +                       a free string, but it is recommended to include the
> > > > > > +                       profile name and version to be useful.
> > > > > > --
> > > > > > 2.26.2
> > > > >
> > > > > Perhaps we should make this use the same interface as we use for the
> > > > > daemon itself (Battery1) and the Source there as well. Last but not
> > > > > least, have you explored the idea of exporting the battery status over
> > > > > uHID? If I got this right this would aggregate the status of different
> > > > > sources and then make the daemon expose them, which while it works for
> > > > > now it means that upper layer have different interfaces for handling a
> > > > > battery status of something plugged over USB and over Bluetooth.
> > > > I am okay with naming the interface Battery1, the same as the daemon.
> > > > Will make an update.
> > > > About the exporting battery status via UHID, it is possible to be
> > > > done, but it is an orthogonal problem that we plan to tackle
> > > > separately. Right now, this API is general enough for Chrome OS to
> > > > allow both HFP and HID batteries to be consolidated in BlueZ. Chrome
> > > > OS's powerd feeds only Bluetooth battery levels from
> > > > /sys/class/power_supply and filters out USB devices so the UI layer
> > > > does not need to worry about it: everything from BlueZ is tied to a
> > > > Bluetooth device.
> > >
> > > But how about devices pushing their battery status over HID reports
> > > instead of GATT? Afaik this is possible since the HID device may have
> > > support for USB (wired) as well where it exposes its battery status
> > > over HID it may just choose to continue doing so while connected over
> > > Bluetooth.
> > If the Bluetooth device reports battery status via HID, it will go
> > into sys/class/power_supply. In Chrome OS, powerd sends this back to
> > BlueZ because it knows the Bluetooth device address from
> > /sys/class/power_supply/ file name.
>
> Well that is something Id like to avoid since if we do in the future
> create an HID report this sort of logic could cause duplications as it
> either the daemon or the kernel may start parsing these so we will
> need some way to identify each source (perhaps by UUID) to avoid
> duplications. Also having differences in upower and powerd really
> doesn't help us unifying the handling here, I hope at least for
> Bluetooth we end up with something similar when either daemons are
> used, otherwise we may need plugins for each separately.
Duplication is something that we have to deal with, regardless whether
it's about HID or anything else. As I originally proposed and
reflected in the code, we start with the simplest duplicate
resolution: accepting the first one who registered and ignores the
rest. From BlueZ point of view, clients can feed battery information
and it handles the duplication. From external clients point of view,
they do not know whether there is already another source (provider
from HFP does not know that there is already another provider from
FastPair, for example) so what they do is feed what they have and let
BlueZ handle the duplication. With this behavior defined, I don't
understand why it becomes necessary for BlueZ to extract HID battery
information directly? I *do* think it is a good idea, but I don't
understand why it (extracting HID battery directly by BlueZ) is a
prerequisite to have the battery provider API in place.

>
> > >
> > > > >
> > > > >
> > > > > --
> > > > > Luiz Augusto von Dentz
> > >
> > >
> > >
> > > --
> > > Luiz Augusto von Dentz
>
>
>
> --
> Luiz Augusto von Dentz

Re: [PATCH BlueZ v2 7/7] battery: Implement Battery Provider API

[Bastien Nocera]

On Tue, 2020-11-17 at 14:16 -0800, Sonny Sasaka wrote:
> Hi Bastien,
> 
> Thank you for the feedback. Please find my answers below.
> 
> On Tue, Nov 17, 2020 at 2:51 AM Bastien Nocera <hadess@hadess.net>
> wrote:
> > 
> > Hey Sonny,
> > 
> > On Tue, 2020-11-10 at 17:17 -0800, Sonny Sasaka wrote:
> > > This patch implements the BatteryProvider1 and
> > > BatteryProviderManager1
> > > API. This is a means for external clients to feed battery
> > > information
> > > to
> > > BlueZ if they handle some profile and can decode battery
> > > reporting.
> > > 
> > > The battery information is then exposed externally via the
> > > existing
> > > Battery1 interface. UI components can consume this API to display
> > > Bluetooth peripherals' battery via a unified BlueZ API.
> > 
> > Was this patch reviewed for potential security problems? From the
> > top
> > of my head, the possible problems would be:
> > - I don't see any filters on which user could register battery
> > providers, so on a multi user system, you could have a user logged
> > in
> > via SSH squatting all the battery providers, while the user "at the
> > console" can't have their own providers. Also, what happens if the
> > user
> > at the console changes (fast user switching)?
> > - It looks like battery providers don't check for paired, trusted
> > or
> > even connected devices, so I would be able to create nearly unbound
> > number of battery providers depending on how big the cache for
> > "seen"
> > devices is.
> For security, the API can be access-limited at D-Bus level using D-
> Bus
> configuration files. For example, we can let only trusted UNIX users
> as the callers for this API. This D-Bus config file would be
> distribution-specific. In Chrome OS, for example, only the "audio"
> and
> "power" users are allowed to call this API. This way we can make sure
> that the callers do not abuse the API for denial-of-service kind of
> attack.

That wouldn't solve it, the point is to avoid one user causing problems
for another logged in user. If both users are in the audio group, which
they'd likely be to be able to use the computer, they'd be able to
cause problems to each other.

> 
> > 
> > Given that the interface between upower and bluez is supposedly
> > trusted, it might be good to ensure that there are no fuzzing
> > problems
> > on the bluez API side that could translate to causing problems in
> > upower itself.
> Could you give an example of what potential problems of upower can be
> caused by communicating with BlueZ through this API?

I haven't looked at the code in depth, but I would expect property
types to be checked before being exported, rather than relying on the
original dbus type matching the expected export type, this sort of
thing.

> 
> > 
> > I didn't review the code in depth, but, having written this
> > mechanism
> > for Bluetooth battery reporting, I think that this is the right way
> > to
> > go to allow daemons like pulseaudio to report battery status.
> > 
> > Cheers
> > 

Re: [PATCH BlueZ v2 7/7] battery: Implement Battery Provider API

[Bastien Nocera]

On Tue, 2020-11-17 at 14:22 -0800, Sonny Sasaka wrote:
> Hi Bastien,
> 
> More responses below.
> 
> On Tue, Nov 17, 2020 at 10:01 AM Bastien Nocera <hadess@hadess.net>
> wrote:
> > 
> > On Tue, 2020-11-17 at 11:51 +0100, Bastien Nocera wrote:
> > > <
> > > <snip> didn't review the code in depth, but, having written this
> > > mechanism
> > > for Bluetooth battery reporting, I think that this is the right
> > > way
> > > to
> > > go to allow daemons like pulseaudio to report battery status.
> > 
> > It would also be useful to know what external components, or
> > internal
> > plugins you expect to feed this API.
> BlueZ could have internal plugins to read proprietary battery
> reporting, e.g. JBL speakers and Bose QC35.

But you don't need to go over D-Bus to implement this.

> 
> > 
> > Apparently HSP/HFP, for example, provide more information that what
> > can
> > be expressed with the Battery1 API, whether it is charging or not,
> > or
> > whether a battery level is unknown, etc.
> > 
> > So I would say that, even if the current battery API is extended,
> > we
> > need to make sure that the D-Bus API to feed new data is extensible
> > enough to allow new properties, and they don't need to be added
> > separately to org.bluez.BatteryProvider1 or org.bluez.Battery1.
> I proposed the API to start simple, but I believe that it can always
> be extended as we need in the future so I don't think the additional
> features need to be coded now. I documented how this API could evolve
> in the future by extending other features as well in this design
> document that I shared with Luiz and Marcel:
>  
> https://docs.google.com/document/d/1OV4sjHLhTzB91D7LyTVT6R0SX2vXwSG1IA3q5yWQWps
> .

Right. My advice would have been to say "the properties exported by
BatteryProvider1 API match the properties exported by the Battery1
API", so you don't need to update 2 APIs separately when the API is
extended.

> 
> > 

Re: [PATCH BlueZ v2 7/7] battery: Implement Battery Provider API

[Sonny Sasaka]

Hi Luiz,


On Tue, Nov 17, 2020 at 2:26 PM Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
>
> Hi Sonny,
>
> On Tue, Nov 17, 2020 at 2:20 PM Sonny Sasaka <sonnysasaka@chromium.org> wrote:
> >
> > Hi Bastien,
> >
> > Thank you for the feedback. Please find my answers below.
> >
> > On Tue, Nov 17, 2020 at 2:51 AM Bastien Nocera <hadess@hadess.net> wrote:
> > >
> > > Hey Sonny,
> > >
> > > On Tue, 2020-11-10 at 17:17 -0800, Sonny Sasaka wrote:
> > > > This patch implements the BatteryProvider1 and
> > > > BatteryProviderManager1
> > > > API. This is a means for external clients to feed battery information
> > > > to
> > > > BlueZ if they handle some profile and can decode battery reporting.
> > > >
> > > > The battery information is then exposed externally via the existing
> > > > Battery1 interface. UI components can consume this API to display
> > > > Bluetooth peripherals' battery via a unified BlueZ API.
> > >
> > > Was this patch reviewed for potential security problems? From the top
> > > of my head, the possible problems would be:
> > > - I don't see any filters on which user could register battery
> > > providers, so on a multi user system, you could have a user logged in
> > > via SSH squatting all the battery providers, while the user "at the
> > > console" can't have their own providers. Also, what happens if the user
> > > at the console changes (fast user switching)?
> > > - It looks like battery providers don't check for paired, trusted or
> > > even connected devices, so I would be able to create nearly unbound
> > > number of battery providers depending on how big the cache for "seen"
> > > devices is.
> > For security, the API can be access-limited at D-Bus level using D-Bus
> > configuration files. For example, we can let only trusted UNIX users
> > as the callers for this API. This D-Bus config file would be
> > distribution-specific. In Chrome OS, for example, only the "audio" and
> > "power" users are allowed to call this API. This way we can make sure
> > that the callers do not abuse the API for denial-of-service kind of
> > attack.
>
> I guess there is still some the risk of conflicts even with the use of
> D-Bus policy blocking roge applications, there could still be multiple
> entities providing the battery status from the same source, which is
> why I suggested we couple the Battery1 with the Profile1, so only the
> entity that has registered to handle let say HFP can provide a battery
> status and we automatically deduce the source is from HFP.

These are two different issues. The issue with bad applications can be
overcome with D-Bus policy. The issue with multiple providers is
inherent: we have to have a duplicate resolution because one device
may report battery from different sources, e.g. via HFP and A2DP at
the same time. The latter case is rare in practice but still possible,
so I propose the simplest duplication resolution which is accept the
first provider registered (of that specific device) and ignore the
rest.

Coupling Battery1 with Profile1 will limit the flexibility of this
feature. For example, some speakers report battery status via a
separate LE channel (GATT). If we require Battery provider to be also
a registered Profile, we won't be able to support these cases since
GATT clients do not register any profile.


>
> > >
> > > Given that the interface between upower and bluez is supposedly
> > > trusted, it might be good to ensure that there are no fuzzing problems
> > > on the bluez API side that could translate to causing problems in
> > > upower itself.
> > Could you give an example of what potential problems of upower can be
> > caused by communicating with BlueZ through this API?
> >
> > >
> > > I didn't review the code in depth, but, having written this mechanism
> > > for Bluetooth battery reporting, I think that this is the right way to
> > > go to allow daemons like pulseaudio to report battery status.
> > >
> > > Cheers
> > >
>
>
>
> --
> Luiz Augusto von Dentz

Re: [PATCH BlueZ v2 7/7] battery: Implement Battery Provider API

[Sonny Sasaka]

Hi Bastien,

On Thu, Nov 19, 2020 at 2:44 AM Bastien Nocera <hadess@hadess.net> wrote:
>
> On Tue, 2020-11-17 at 14:16 -0800, Sonny Sasaka wrote:
> > Hi Bastien,
> >
> > Thank you for the feedback. Please find my answers below.
> >
> > On Tue, Nov 17, 2020 at 2:51 AM Bastien Nocera <hadess@hadess.net>
> > wrote:
> > >
> > > Hey Sonny,
> > >
> > > On Tue, 2020-11-10 at 17:17 -0800, Sonny Sasaka wrote:
> > > > This patch implements the BatteryProvider1 and
> > > > BatteryProviderManager1
> > > > API. This is a means for external clients to feed battery
> > > > information
> > > > to
> > > > BlueZ if they handle some profile and can decode battery
> > > > reporting.
> > > >
> > > > The battery information is then exposed externally via the
> > > > existing
> > > > Battery1 interface. UI components can consume this API to display
> > > > Bluetooth peripherals' battery via a unified BlueZ API.
> > >
> > > Was this patch reviewed for potential security problems? From the
> > > top
> > > of my head, the possible problems would be:
> > > - I don't see any filters on which user could register battery
> > > providers, so on a multi user system, you could have a user logged
> > > in
> > > via SSH squatting all the battery providers, while the user "at the
> > > console" can't have their own providers. Also, what happens if the
> > > user
> > > at the console changes (fast user switching)?
> > > - It looks like battery providers don't check for paired, trusted
> > > or
> > > even connected devices, so I would be able to create nearly unbound
> > > number of battery providers depending on how big the cache for
> > > "seen"
> > > devices is.
> > For security, the API can be access-limited at D-Bus level using D-
> > Bus
> > configuration files. For example, we can let only trusted UNIX users
> > as the callers for this API. This D-Bus config file would be
> > distribution-specific. In Chrome OS, for example, only the "audio"
> > and
> > "power" users are allowed to call this API. This way we can make sure
> > that the callers do not abuse the API for denial-of-service kind of
> > attack.
>
> That wouldn't solve it, the point is to avoid one user causing problems
> for another logged in user. If both users are in the audio group, which
> they'd likely be to be able to use the computer, they'd be able to
> cause problems to each other.

If I understand your case correctly, both users being in "audio" group
still won't allow them both to become battery providers because the
D-Bus policy only allows "audio" user and not "audio" group.


>
> >
> > >
> > > Given that the interface between upower and bluez is supposedly
> > > trusted, it might be good to ensure that there are no fuzzing
> > > problems
> > > on the bluez API side that could translate to causing problems in
> > > upower itself.
> > Could you give an example of what potential problems of upower can be
> > caused by communicating with BlueZ through this API?
>
> I haven't looked at the code in depth, but I would expect property
> types to be checked before being exported, rather than relying on the
> original dbus type matching the expected export type, this sort of
> thing.
Yes, the code does not just forward information. The code processes
the input and exports it as a clearly defined output type.
>
> >
> > >
> > > I didn't review the code in depth, but, having written this
> > > mechanism
> > > for Bluetooth battery reporting, I think that this is the right way
> > > to
> > > go to allow daemons like pulseaudio to report battery status.
> > >
> > > Cheers
> > >
>
>

Re: [PATCH BlueZ v2 7/7] battery: Implement Battery Provider API

[Sonny Sasaka]

Hi Bastien,

On Thu, Nov 19, 2020 at 2:53 AM Bastien Nocera <hadess@hadess.net> wrote:
>
> On Tue, 2020-11-17 at 14:22 -0800, Sonny Sasaka wrote:
> > Hi Bastien,
> >
> > More responses below.
> >
> > On Tue, Nov 17, 2020 at 10:01 AM Bastien Nocera <hadess@hadess.net>
> > wrote:
> > >
> > > On Tue, 2020-11-17 at 11:51 +0100, Bastien Nocera wrote:
> > > > <
> > > > <snip> didn't review the code in depth, but, having written this
> > > > mechanism
> > > > for Bluetooth battery reporting, I think that this is the right
> > > > way
> > > > to
> > > > go to allow daemons like pulseaudio to report battery status.
> > >
> > > It would also be useful to know what external components, or
> > > internal
> > > plugins you expect to feed this API.
> > BlueZ could have internal plugins to read proprietary battery
> > reporting, e.g. JBL speakers and Bose QC35.
>
> But you don't need to go over D-Bus to implement this.
Some proprietary protocols may not want to become an internal BlueZ
plugin, for example because it is developed closed source. D-Bus API
is useful to support these cases.
>
> >
> > >
> > > Apparently HSP/HFP, for example, provide more information that what
> > > can
> > > be expressed with the Battery1 API, whether it is charging or not,
> > > or
> > > whether a battery level is unknown, etc.
> > >
> > > So I would say that, even if the current battery API is extended,
> > > we
> > > need to make sure that the D-Bus API to feed new data is extensible
> > > enough to allow new properties, and they don't need to be added
> > > separately to org.bluez.BatteryProvider1 or org.bluez.Battery1.
> > I proposed the API to start simple, but I believe that it can always
> > be extended as we need in the future so I don't think the additional
> > features need to be coded now. I documented how this API could evolve
> > in the future by extending other features as well in this design
> > document that I shared with Luiz and Marcel:
> >
> > https://docs.google.com/document/d/1OV4sjHLhTzB91D7LyTVT6R0SX2vXwSG1IA3q5yWQWps
> > .
>
> Right. My advice would have been to say "the properties exported by
> BatteryProvider1 API match the properties exported by the Battery1
> API", so you don't need to update 2 APIs separately when the API is
> extended.
I am considering doing this. Let me think it through to make sure we
don't stumble on anything in the future if we do it this way.

>
> >
> > >
>
>

Re: [PATCH BlueZ v2 7/7] battery: Implement Battery Provider API

[Luiz Augusto von Dentz]

Hi Sonny,

On Thu, Nov 19, 2020 at 12:15 PM Sonny Sasaka <sonnysasaka@chromium.org> wrote:
>
> Hi Luiz,
>
>
> On Tue, Nov 17, 2020 at 2:26 PM Luiz Augusto von Dentz
> <luiz.dentz@gmail.com> wrote:
> >
> > Hi Sonny,
> >
> > On Tue, Nov 17, 2020 at 2:20 PM Sonny Sasaka <sonnysasaka@chromium.org> wrote:
> > >
> > > Hi Bastien,
> > >
> > > Thank you for the feedback. Please find my answers below.
> > >
> > > On Tue, Nov 17, 2020 at 2:51 AM Bastien Nocera <hadess@hadess.net> wrote:
> > > >
> > > > Hey Sonny,
> > > >
> > > > On Tue, 2020-11-10 at 17:17 -0800, Sonny Sasaka wrote:
> > > > > This patch implements the BatteryProvider1 and
> > > > > BatteryProviderManager1
> > > > > API. This is a means for external clients to feed battery information
> > > > > to
> > > > > BlueZ if they handle some profile and can decode battery reporting.
> > > > >
> > > > > The battery information is then exposed externally via the existing
> > > > > Battery1 interface. UI components can consume this API to display
> > > > > Bluetooth peripherals' battery via a unified BlueZ API.
> > > >
> > > > Was this patch reviewed for potential security problems? From the top
> > > > of my head, the possible problems would be:
> > > > - I don't see any filters on which user could register battery
> > > > providers, so on a multi user system, you could have a user logged in
> > > > via SSH squatting all the battery providers, while the user "at the
> > > > console" can't have their own providers. Also, what happens if the user
> > > > at the console changes (fast user switching)?
> > > > - It looks like battery providers don't check for paired, trusted or
> > > > even connected devices, so I would be able to create nearly unbound
> > > > number of battery providers depending on how big the cache for "seen"
> > > > devices is.
> > > For security, the API can be access-limited at D-Bus level using D-Bus
> > > configuration files. For example, we can let only trusted UNIX users
> > > as the callers for this API. This D-Bus config file would be
> > > distribution-specific. In Chrome OS, for example, only the "audio" and
> > > "power" users are allowed to call this API. This way we can make sure
> > > that the callers do not abuse the API for denial-of-service kind of
> > > attack.
> >
> > I guess there is still some the risk of conflicts even with the use of
> > D-Bus policy blocking roge applications, there could still be multiple
> > entities providing the battery status from the same source, which is
> > why I suggested we couple the Battery1 with the Profile1, so only the
> > entity that has registered to handle let say HFP can provide a battery
> > status and we automatically deduce the source is from HFP.
>
> These are two different issues. The issue with bad applications can be
> overcome with D-Bus policy. The issue with multiple providers is
> inherent: we have to have a duplicate resolution because one device
> may report battery from different sources, e.g. via HFP and A2DP at
> the same time. The latter case is rare in practice but still possible,
> so I propose the simplest duplication resolution which is accept the
> first provider registered (of that specific device) and ignore the
> rest.
>
> Coupling Battery1 with Profile1 will limit the flexibility of this
> feature. For example, some speakers report battery status via a
> separate LE channel (GATT). If we require Battery provider to be also
> a registered Profile, we won't be able to support these cases since
> GATT clients do not register any profile.

Actually it is a good policy to provide GattProfile1 if you are
interested in enabling auto-connect, which I suppose most third-party
services would like to enable:

https://git.kernel.org/pub/scm/bluetooth/bluez.git/tree/doc/gatt-api.txt#n367

Note that we are doing to avoid complicate conflict resolution since
profiles by design can only be registered once that means Battery
sources will also be limited to one per profile, Im fine if you choose
not to have this integration in the first version .

>
> >
> > > >
> > > > Given that the interface between upower and bluez is supposedly
> > > > trusted, it might be good to ensure that there are no fuzzing problems
> > > > on the bluez API side that could translate to causing problems in
> > > > upower itself.
> > > Could you give an example of what potential problems of upower can be
> > > caused by communicating with BlueZ through this API?
> > >
> > > >
> > > > I didn't review the code in depth, but, having written this mechanism
> > > > for Bluetooth battery reporting, I think that this is the right way to
> > > > go to allow daemons like pulseaudio to report battery status.
> > > >
> > > > Cheers
> > > >
> >
> >
> >
> > --
> > Luiz Augusto von Dentz



-- 
Luiz Augusto von Dentz

Re: [PATCH BlueZ v2 7/7] battery: Implement Battery Provider API

[Bastien Nocera]

On Thu, 2020-11-19 at 12:20 -0800, Sonny Sasaka wrote:
> Hi Bastien,
> 
> On Thu, Nov 19, 2020 at 2:44 AM Bastien Nocera <hadess@hadess.net>
> wrote:
> > 
> > On Tue, 2020-11-17 at 14:16 -0800, Sonny Sasaka wrote:
> > > Hi Bastien,
> > > 
> > > Thank you for the feedback. Please find my answers below.
> > > 
> > > On Tue, Nov 17, 2020 at 2:51 AM Bastien Nocera
> > > <hadess@hadess.net>
> > > wrote:
> > > > 
> > > > Hey Sonny,
> > > > 
> > > > On Tue, 2020-11-10 at 17:17 -0800, Sonny Sasaka wrote:
> > > > > This patch implements the BatteryProvider1 and
> > > > > BatteryProviderManager1
> > > > > API. This is a means for external clients to feed battery
> > > > > information
> > > > > to
> > > > > BlueZ if they handle some profile and can decode battery
> > > > > reporting.
> > > > > 
> > > > > The battery information is then exposed externally via the
> > > > > existing
> > > > > Battery1 interface. UI components can consume this API to
> > > > > display
> > > > > Bluetooth peripherals' battery via a unified BlueZ API.
> > > > 
> > > > Was this patch reviewed for potential security problems? From
> > > > the
> > > > top
> > > > of my head, the possible problems would be:
> > > > - I don't see any filters on which user could register battery
> > > > providers, so on a multi user system, you could have a user
> > > > logged
> > > > in
> > > > via SSH squatting all the battery providers, while the user "at
> > > > the
> > > > console" can't have their own providers. Also, what happens if
> > > > the
> > > > user
> > > > at the console changes (fast user switching)?
> > > > - It looks like battery providers don't check for paired,
> > > > trusted
> > > > or
> > > > even connected devices, so I would be able to create nearly
> > > > unbound
> > > > number of battery providers depending on how big the cache for
> > > > "seen"
> > > > devices is.
> > > For security, the API can be access-limited at D-Bus level using
> > > D-
> > > Bus
> > > configuration files. For example, we can let only trusted UNIX
> > > users
> > > as the callers for this API. This D-Bus config file would be
> > > distribution-specific. In Chrome OS, for example, only the
> > > "audio"
> > > and
> > > "power" users are allowed to call this API. This way we can make
> > > sure
> > > that the callers do not abuse the API for denial-of-service kind
> > > of
> > > attack.
> > 
> > That wouldn't solve it, the point is to avoid one user causing
> > problems
> > for another logged in user. If both users are in the audio group,
> > which
> > they'd likely be to be able to use the computer, they'd be able to
> > cause problems to each other.
> 
> If I understand your case correctly, both users being in "audio"
> group
> still won't allow them both to become battery providers because the
> D-Bus policy only allows "audio" user and not "audio" group.

OK, I guess that means that this is a separate daemon running as a
different user, not, say, PulseAudio running in the user's session
feeding information. Is that right?

Either way, I guess we'll need to test this out once the feature is
merged.

Apart from the concern about having to duplicate the exported
properties, the rest looks good. I've made some additional comments
about the architecture in the design document you shared, but those
should not have any impact on the implementation.

Good job :)

Re: [PATCH BlueZ v2 7/7] battery: Implement Battery Provider API

[Sonny Sasaka]

Hi Bastien,

On Fri, Nov 20, 2020 at 2:33 AM Bastien Nocera <hadess@hadess.net> wrote:
>
> On Thu, 2020-11-19 at 12:20 -0800, Sonny Sasaka wrote:
> > Hi Bastien,
> >
> > On Thu, Nov 19, 2020 at 2:44 AM Bastien Nocera <hadess@hadess.net>
> > wrote:
> > >
> > > On Tue, 2020-11-17 at 14:16 -0800, Sonny Sasaka wrote:
> > > > Hi Bastien,
> > > >
> > > > Thank you for the feedback. Please find my answers below.
> > > >
> > > > On Tue, Nov 17, 2020 at 2:51 AM Bastien Nocera
> > > > <hadess@hadess.net>
> > > > wrote:
> > > > >
> > > > > Hey Sonny,
> > > > >
> > > > > On Tue, 2020-11-10 at 17:17 -0800, Sonny Sasaka wrote:
> > > > > > This patch implements the BatteryProvider1 and
> > > > > > BatteryProviderManager1
> > > > > > API. This is a means for external clients to feed battery
> > > > > > information
> > > > > > to
> > > > > > BlueZ if they handle some profile and can decode battery
> > > > > > reporting.
> > > > > >
> > > > > > The battery information is then exposed externally via the
> > > > > > existing
> > > > > > Battery1 interface. UI components can consume this API to
> > > > > > display
> > > > > > Bluetooth peripherals' battery via a unified BlueZ API.
> > > > >
> > > > > Was this patch reviewed for potential security problems? From
> > > > > the
> > > > > top
> > > > > of my head, the possible problems would be:
> > > > > - I don't see any filters on which user could register battery
> > > > > providers, so on a multi user system, you could have a user
> > > > > logged
> > > > > in
> > > > > via SSH squatting all the battery providers, while the user "at
> > > > > the
> > > > > console" can't have their own providers. Also, what happens if
> > > > > the
> > > > > user
> > > > > at the console changes (fast user switching)?
> > > > > - It looks like battery providers don't check for paired,
> > > > > trusted
> > > > > or
> > > > > even connected devices, so I would be able to create nearly
> > > > > unbound
> > > > > number of battery providers depending on how big the cache for
> > > > > "seen"
> > > > > devices is.
> > > > For security, the API can be access-limited at D-Bus level using
> > > > D-
> > > > Bus
> > > > configuration files. For example, we can let only trusted UNIX
> > > > users
> > > > as the callers for this API. This D-Bus config file would be
> > > > distribution-specific. In Chrome OS, for example, only the
> > > > "audio"
> > > > and
> > > > "power" users are allowed to call this API. This way we can make
> > > > sure
> > > > that the callers do not abuse the API for denial-of-service kind
> > > > of
> > > > attack.
> > >
> > > That wouldn't solve it, the point is to avoid one user causing
> > > problems
> > > for another logged in user. If both users are in the audio group,
> > > which
> > > they'd likely be to be able to use the computer, they'd be able to
> > > cause problems to each other.
> >
> > If I understand your case correctly, both users being in "audio"
> > group
> > still won't allow them both to become battery providers because the
> > D-Bus policy only allows "audio" user and not "audio" group.
>
> OK, I guess that means that this is a separate daemon running as a
> different user, not, say, PulseAudio running in the user's session
> feeding information. Is that right?
Yes, that's right.

>
> Either way, I guess we'll need to test this out once the feature is
> merged.
It will first be tested in Chrome OS with CRAS as the battery provider
from HFP (I am working on it too). I guess PulseAudio can then follow
along so Linux desktops can get headphones batteries in the UI. Then,
as Luiz suggested, batteries from HID will be parsed directly from
within bluetoothd (a TODO, not blocking the progress of the API).

>
> Apart from the concern about having to duplicate the exported
> properties, the rest looks good. I've made some additional comments
> about the architecture in the design document you shared, but those
> should not have any impact on the implementation.

Thanks for the review. I will update the patches based on your and
Luiz's feedback.
>
> Good job :)
>

Re: [PATCH BlueZ v2 7/7] battery: Implement Battery Provider API

[Sonny Sasaka]

Hi Luiz,

On Thu, Nov 19, 2020 at 3:56 PM Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
>
> Hi Sonny,
>
> On Thu, Nov 19, 2020 at 12:15 PM Sonny Sasaka <sonnysasaka@chromium.org> wrote:
> >
> > Hi Luiz,
> >
> >
> > On Tue, Nov 17, 2020 at 2:26 PM Luiz Augusto von Dentz
> > <luiz.dentz@gmail.com> wrote:
> > >
> > > Hi Sonny,
> > >
> > > On Tue, Nov 17, 2020 at 2:20 PM Sonny Sasaka <sonnysasaka@chromium.org> wrote:
> > > >
> > > > Hi Bastien,
> > > >
> > > > Thank you for the feedback. Please find my answers below.
> > > >
> > > > On Tue, Nov 17, 2020 at 2:51 AM Bastien Nocera <hadess@hadess.net> wrote:
> > > > >
> > > > > Hey Sonny,
> > > > >
> > > > > On Tue, 2020-11-10 at 17:17 -0800, Sonny Sasaka wrote:
> > > > > > This patch implements the BatteryProvider1 and
> > > > > > BatteryProviderManager1
> > > > > > API. This is a means for external clients to feed battery information
> > > > > > to
> > > > > > BlueZ if they handle some profile and can decode battery reporting.
> > > > > >
> > > > > > The battery information is then exposed externally via the existing
> > > > > > Battery1 interface. UI components can consume this API to display
> > > > > > Bluetooth peripherals' battery via a unified BlueZ API.
> > > > >
> > > > > Was this patch reviewed for potential security problems? From the top
> > > > > of my head, the possible problems would be:
> > > > > - I don't see any filters on which user could register battery
> > > > > providers, so on a multi user system, you could have a user logged in
> > > > > via SSH squatting all the battery providers, while the user "at the
> > > > > console" can't have their own providers. Also, what happens if the user
> > > > > at the console changes (fast user switching)?
> > > > > - It looks like battery providers don't check for paired, trusted or
> > > > > even connected devices, so I would be able to create nearly unbound
> > > > > number of battery providers depending on how big the cache for "seen"
> > > > > devices is.
> > > > For security, the API can be access-limited at D-Bus level using D-Bus
> > > > configuration files. For example, we can let only trusted UNIX users
> > > > as the callers for this API. This D-Bus config file would be
> > > > distribution-specific. In Chrome OS, for example, only the "audio" and
> > > > "power" users are allowed to call this API. This way we can make sure
> > > > that the callers do not abuse the API for denial-of-service kind of
> > > > attack.
> > >
> > > I guess there is still some the risk of conflicts even with the use of
> > > D-Bus policy blocking roge applications, there could still be multiple
> > > entities providing the battery status from the same source, which is
> > > why I suggested we couple the Battery1 with the Profile1, so only the
> > > entity that has registered to handle let say HFP can provide a battery
> > > status and we automatically deduce the source is from HFP.
> >
> > These are two different issues. The issue with bad applications can be
> > overcome with D-Bus policy. The issue with multiple providers is
> > inherent: we have to have a duplicate resolution because one device
> > may report battery from different sources, e.g. via HFP and A2DP at
> > the same time. The latter case is rare in practice but still possible,
> > so I propose the simplest duplication resolution which is accept the
> > first provider registered (of that specific device) and ignore the
> > rest.
> >
> > Coupling Battery1 with Profile1 will limit the flexibility of this
> > feature. For example, some speakers report battery status via a
> > separate LE channel (GATT). If we require Battery provider to be also
> > a registered Profile, we won't be able to support these cases since
> > GATT clients do not register any profile.
>
> Actually it is a good policy to provide GattProfile1 if you are
> interested in enabling auto-connect, which I suppose most third-party
> services would like to enable:
>
> https://git.kernel.org/pub/scm/bluetooth/bluez.git/tree/doc/gatt-api.txt#n367
>
> Note that we are doing to avoid complicate conflict resolution since
> profiles by design can only be registered once that means Battery
> sources will also be limited to one per profile, Im fine if you choose
> not to have this integration in the first version .
Thanks, Luiz. I will proceed without profile integration in the first
iteration, since battery sources will be limited to one per profile
anyway.

>
> >
> > >
> > > > >
> > > > > Given that the interface between upower and bluez is supposedly
> > > > > trusted, it might be good to ensure that there are no fuzzing problems
> > > > > on the bluez API side that could translate to causing problems in
> > > > > upower itself.
> > > > Could you give an example of what potential problems of upower can be
> > > > caused by communicating with BlueZ through this API?
> > > >
> > > > >
> > > > > I didn't review the code in depth, but, having written this mechanism
> > > > > for Bluetooth battery reporting, I think that this is the right way to
> > > > > go to allow daemons like pulseaudio to report battery status.
> > > > >
> > > > > Cheers
> > > > >
> > >
> > >
> > >
> > > --
> > > Luiz Augusto von Dentz
>
>
>
> --
> Luiz Augusto von Dentz

Re: [PATCH BlueZ v2 4/7] doc: Add Battery Provider API doc

[Sonny Sasaka]

Hi Luiz,

As agreed off this thread, we will proceed with the first iteration of
the Battery Provider API without profile integration and HID internal
battery decoder. Please take another look at v3 patches I just sent.
Thanks for reviewing!

On Tue, Nov 17, 2020 at 3:55 PM Sonny Sasaka <sonnysasaka@chromium.org> wrote:
>
> Hi Luiz,
>
>
> On Tue, Nov 17, 2020 at 3:33 PM Luiz Augusto von Dentz
> <luiz.dentz@gmail.com> wrote:
> >
> > Hi Sonny,
> >
> > On Tue, Nov 17, 2020 at 3:17 PM Sonny Sasaka <sonnysasaka@chromium.org> wrote:
> > >
> > > Hi Luiz,
> > >
> > > On Tue, Nov 17, 2020 at 3:01 PM Luiz Augusto von Dentz
> > > <luiz.dentz@gmail.com> wrote:
> > > >
> > > > Hi Sonny,
> > > >
> > > > On Tue, Nov 17, 2020 at 2:37 PM Sonny Sasaka <sonnysasaka@chromium.org> wrote:
> > > > >
> > > > > Hi Luiz,
> > > > >
> > > > > Thanks for the feedback. Please find my responses below.
> > > > >
> > > > > On Mon, Nov 16, 2020 at 4:17 PM Luiz Augusto von Dentz
> > > > > <luiz.dentz@gmail.com> wrote:
> > > > > >
> > > > > > Hi Sonny,
> > > > > >
> > > > > > On Tue, Nov 10, 2020 at 5:22 PM Sonny Sasaka <sonnysasaka@chromium.org> wrote:
> > > > > > >
> > > > > > > This patch add the documentation of the Battery Provider which lets
> > > > > > > external clients feed battery information to BlueZ if they are able to
> > > > > > > decode battery reporting via any profile. BlueZ UI clients can then use
> > > > > > > the org.bluez.Battery1 API as a single source of battery information
> > > > > > > coming from many different profiles.
> > > > > > >
> > > > > > > Reviewed-by: Miao-chen Chou <mcchou@chromium.org>
> > > > > > >
> > > > > > > ---
> > > > > > >  doc/battery-api.txt | 55 +++++++++++++++++++++++++++++++++++++++++++++
> > > > > > >  1 file changed, 55 insertions(+)
> > > > > > >
> > > > > > > diff --git a/doc/battery-api.txt b/doc/battery-api.txt
> > > > > > > index dc7dbeda2..058bf0c6e 100644
> > > > > > > --- a/doc/battery-api.txt
> > > > > > > +++ b/doc/battery-api.txt
> > > > > > > @@ -12,3 +12,58 @@ Object path  [variable prefix]/{hci0,hci1,...}/dev_XX_XX_XX_XX_XX_XX
> > > > > > >  Properties     byte Percentage [readonly]
> > > > > > >
> > > > > > >                         The percentage of battery left as an unsigned 8-bit integer.
> > > > > > > +
> > > > > > > +               string Source [readonly, optional, experimental]
> > > > > > > +
> > > > > > > +                       Describes where the battery information comes from
> > > > > > > +                       (e.g. "HFP 1.7", "HID").
> > > > > > > +                       This property is informational only and may be useful
> > > > > > > +                       for debugging purposes.
> > > > > >
> > > > > > We should probably tight this to UUID instead.
> > > > > Ack. Will update the doc to suggest UUID in this source field.
> > > > >
> > > > > >
> > > > > > > +
> > > > > > > +
> > > > > > > +Battery Provider Manager hierarchy
> > > > > > > +==================================
> > > > > > > +A battery provider starts by registering itself as a battery provider with the
> > > > > > > +RegisterBatteryProvider method passing an object path as the provider ID. Then,
> > > > > > > +it can start exposing org.bluez.BatteryProvider1 objects having the path
> > > > > > > +starting with the given provider ID. It can also remove objects at any time.
> > > > > > > +BlueZ will stop monitoring these exposed and removed objects after
> > > > > > > +UnregisterBatteryProvider is called for that provider ID.
> > > > > > > +
> > > > > > > +Service                org.bluez
> > > > > > > +Interface      org.bluez.BatteryProviderManager1 [experimental]
> > > > > > > +Object path    /org/bluez/{hci0,hci1,...}
> > > > > > > +
> > > > > > > +Methods                void RegisterBatteryProvider(object provider)
> > > > > > > +
> > > > > > > +                       This registers a battery provider. A registered
> > > > > > > +                       battery provider can then expose objects with
> > > > > > > +                       org.bluez.BatteryProvider1 interface described below.
> > > > > > > +
> > > > > > > +               void UnregisterBatteryProvider(object provider)
> > > > > > > +
> > > > > > > +                       This unregisters a battery provider. After
> > > > > > > +                       unregistration, the BatteryProvider1 objects provided
> > > > > > > +                       by this client are ignored by BlueZ.
> > > > > >
> > > > > > Not sure if you were followinging the monitor patches, for registering
> > > > > > objects we do prefer the ObjectManager style so multiple provider can
> > > > > > be registered at once, also we may want to tight the control of
> > > > > > battery provider with Profile API so we guarantee that the same entity
> > > > > > that handles the profile connection is the one providing the battery
> > > > > > status.
> > > > > It is actually already in ObjectManager style. After the "root path"
> > > > > is registered, the provider can expose many D-Bus objects at once and
> > > > > bluetoothd can detect it.
> > > > > About tying it with the Profile API, I will take a look at how it could be done.
> > > > >
> > > > > >
> > > > > > > +
> > > > > > > +
> > > > > > > +Battery Provider hierarchy
> > > > > > > +==========================
> > > > > > > +
> > > > > > > +Service                <client D-Bus address>
> > > > > > > +Interface      org.bluez.BatteryProvider1 [experimental]
> > > > > > > +Object path    {provider_root}/org/bluez/{hci0,hci1,...}/dev_XX_XX_XX_XX_XX_XX
> > > > > > > +
> > > > > > > +Properties     byte Percentage [readonly]
> > > > > > > +
> > > > > > > +                       The percentage of battery left as an unsigned 8-bit
> > > > > > > +                       integer.
> > > > > > > +
> > > > > > > +               string Source [readonly, optional]
> > > > > > > +
> > > > > > > +                       Describes where the battery information comes from
> > > > > > > +                       (e.g. "HFP 1.7", "HID").
> > > > > > > +                       This property is informational only and may be useful
> > > > > > > +                       for debugging purposes. The content of this property is
> > > > > > > +                       a free string, but it is recommended to include the
> > > > > > > +                       profile name and version to be useful.
> > > > > > > --
> > > > > > > 2.26.2
> > > > > >
> > > > > > Perhaps we should make this use the same interface as we use for the
> > > > > > daemon itself (Battery1) and the Source there as well. Last but not
> > > > > > least, have you explored the idea of exporting the battery status over
> > > > > > uHID? If I got this right this would aggregate the status of different
> > > > > > sources and then make the daemon expose them, which while it works for
> > > > > > now it means that upper layer have different interfaces for handling a
> > > > > > battery status of something plugged over USB and over Bluetooth.
> > > > > I am okay with naming the interface Battery1, the same as the daemon.
> > > > > Will make an update.
> > > > > About the exporting battery status via UHID, it is possible to be
> > > > > done, but it is an orthogonal problem that we plan to tackle
> > > > > separately. Right now, this API is general enough for Chrome OS to
> > > > > allow both HFP and HID batteries to be consolidated in BlueZ. Chrome
> > > > > OS's powerd feeds only Bluetooth battery levels from
> > > > > /sys/class/power_supply and filters out USB devices so the UI layer
> > > > > does not need to worry about it: everything from BlueZ is tied to a
> > > > > Bluetooth device.
> > > >
> > > > But how about devices pushing their battery status over HID reports
> > > > instead of GATT? Afaik this is possible since the HID device may have
> > > > support for USB (wired) as well where it exposes its battery status
> > > > over HID it may just choose to continue doing so while connected over
> > > > Bluetooth.
> > > If the Bluetooth device reports battery status via HID, it will go
> > > into sys/class/power_supply. In Chrome OS, powerd sends this back to
> > > BlueZ because it knows the Bluetooth device address from
> > > /sys/class/power_supply/ file name.
> >
> > Well that is something Id like to avoid since if we do in the future
> > create an HID report this sort of logic could cause duplications as it
> > either the daemon or the kernel may start parsing these so we will
> > need some way to identify each source (perhaps by UUID) to avoid
> > duplications. Also having differences in upower and powerd really
> > doesn't help us unifying the handling here, I hope at least for
> > Bluetooth we end up with something similar when either daemons are
> > used, otherwise we may need plugins for each separately.
> Duplication is something that we have to deal with, regardless whether
> it's about HID or anything else. As I originally proposed and
> reflected in the code, we start with the simplest duplicate
> resolution: accepting the first one who registered and ignores the
> rest. From BlueZ point of view, clients can feed battery information
> and it handles the duplication. From external clients point of view,
> they do not know whether there is already another source (provider
> from HFP does not know that there is already another provider from
> FastPair, for example) so what they do is feed what they have and let
> BlueZ handle the duplication. With this behavior defined, I don't
> understand why it becomes necessary for BlueZ to extract HID battery
> information directly? I *do* think it is a good idea, but I don't
> understand why it (extracting HID battery directly by BlueZ) is a
> prerequisite to have the battery provider API in place.
>
> >
> > > >
> > > > > >
> > > > > >
> > > > > > --
> > > > > > Luiz Augusto von Dentz
> > > >
> > > >
> > > >
> > > > --
> > > > Luiz Augusto von Dentz
> >
> >
> >
> > --
> > Luiz Augusto von Dentz