[PATCH v4 0/10] Add GATT Client Battery Service

[PATCH v4 0/10] Add GATT Client Battery Service

[chen.ganir]

From: Chen Ganir <chen.ganir@ti.com>

Add suupport for LE GATT Client Battery Service.

This plugin adds battery list to the btd_device, exposes DBUS API to list the
device batteries, and allows querying for battery information. In addition this
patch allows getting notifications for battery level changes.

Look at doc/device-api.txt and doc/battery-api.txt for more information.

This is version 4 of this patch set, rebased on top of the latest sources and 
fixes:

  Remove namespace and descriptor battery properties 
  Documentation changes are now in one single patch
  reorder some patches

Chen Ganir (10):
  Battery: Add Battery Service Gatt Client
  Battery: Add Battery Service plugin skeleton
  Battery: Add connection logic
  Battery: Discover Characteristic Descriptors
  Battery: Get Battery ID
  Battery: Add Battery list to btd_device
  Battery: Read Battery level characteristic
  Battery: Add Battery D-BUS API
  Battery: Add support for notifications
  Battery: Emit property changed on first read

 Makefile.am                |   10 +-
 doc/battery-api.txt        |   26 +++
 doc/device-api.txt         |    5 +
 profiles/battery/battery.c |  529 ++++++++++++++++++++++++++++++++++++++++++++
 profiles/battery/battery.h |   26 +++
 profiles/battery/main.c    |   67 ++++++
 profiles/battery/manager.c |   71 ++++++
 profiles/battery/manager.h |   24 ++
 src/device.c               |   65 ++++++
 src/device.h               |    3 +
 10 files changed, 824 insertions(+), 2 deletions(-)
 create mode 100644 doc/battery-api.txt
 create mode 100644 profiles/battery/battery.c
 create mode 100644 profiles/battery/battery.h
 create mode 100644 profiles/battery/main.c
 create mode 100644 profiles/battery/manager.c
 create mode 100644 profiles/battery/manager.h

-- 
1.7.9.5

[PATCH v4 01/10] Battery: Add Battery Service Gatt Client

[chen.ganir]

From: Chen Ganir <chen.ganir@ti.com>

Add documentation for new GATT Client Battery service plugin
---
 doc/battery-api.txt |   26 ++++++++++++++++++++++++++
 doc/device-api.txt  |    5 +++++
 2 files changed, 31 insertions(+)
 create mode 100644 doc/battery-api.txt

diff --git a/doc/battery-api.txt b/doc/battery-api.txt
new file mode 100644
index 0000000..6e20a6a
--- /dev/null
+++ b/doc/battery-api.txt
@@ -0,0 +1,26 @@
+BlueZ D-Bus Battery API description
+****************************************
+
+	Texas Instruments, Inc. <chen.ganir@ti.com>
+
+Battery Service hierarchy
+=====================================
+
+Service		org.bluez
+Interface	org.bluez.Battery
+Object path	[variable prefix]/{hci0,..}/dev_XX_XX_XX_XX_XX_XX/BATT-NN-DDDD
+
+
+Methods	dict GetProperties()
+
+			Returns all properties for the interface. See the
+			Properties section for the available properties.
+
+Signals		PropertyChanged(string name, variant value)
+
+		This signal indicates a changed value of the given
+		property.
+
+Properties	byte Level [readonly]
+
+			Battery level (0-100).
diff --git a/doc/device-api.txt b/doc/device-api.txt
index 1f0dc96..5d760b1 100644
--- a/doc/device-api.txt
+++ b/doc/device-api.txt
@@ -179,3 +179,8 @@ Properties	string Address [readonly]
 			Note that this property can exhibit false-positives
 			in the case of Bluetooth 2.1 (or newer) devices that
 			have disabled Extended Inquiry Response support.
+
+		array{string} Batteries [readonly]
+
+			List of device battery object paths that represents the available
+			batteries on the remote device.
-- 
1.7.9.5

[PATCH v4 02/10] Battery: Add Battery Service plugin skeleton

[chen.ganir]

From: Chen Ganir <chen.ganir@ti.com>

Add support for the Battery Service Gatt Client side.
---
 Makefile.am                |   10 ++++-
 profiles/battery/battery.c |   88 ++++++++++++++++++++++++++++++++++++++++++++
 profiles/battery/battery.h |   24 ++++++++++++
 profiles/battery/main.c    |   53 ++++++++++++++++++++++++++
 profiles/battery/manager.c |   62 +++++++++++++++++++++++++++++++
 profiles/battery/manager.h |   24 ++++++++++++
 6 files changed, 259 insertions(+), 2 deletions(-)
 create mode 100644 profiles/battery/battery.c
 create mode 100644 profiles/battery/battery.h
 create mode 100644 profiles/battery/main.c
 create mode 100644 profiles/battery/manager.c
 create mode 100644 profiles/battery/manager.h

diff --git a/Makefile.am b/Makefile.am
index a74709d..87e2c1c 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -211,7 +211,8 @@ builtin_sources += profiles/health/hdp_main.c profiles/health/hdp_types.h \
 endif
 
 if GATTMODULES
-builtin_modules += thermometer alert time gatt_example proximity deviceinfo
+builtin_modules += thermometer alert time gatt_example proximity deviceinfo \
+            battery
 builtin_sources += profiles/thermometer/main.c \
 			profiles/thermometer/manager.h \
 			profiles/thermometer/manager.c \
@@ -237,7 +238,12 @@ builtin_sources += profiles/thermometer/main.c \
 			profiles/deviceinfo/manager.h \
 			profiles/deviceinfo/manager.c \
 			profiles/deviceinfo/deviceinfo.h \
-			profiles/deviceinfo/deviceinfo.c
+			profiles/deviceinfo/deviceinfo.c \
+			profiles/battery/main.c \
+			profiles/battery/manager.c \
+			profiles/battery/manager.h \
+			profiles/battery/battery.c \
+			profiles/battery/battery.h
 endif
 
 builtin_modules += formfactor
diff --git a/profiles/battery/battery.c b/profiles/battery/battery.c
new file mode 100644
index 0000000..7ed5707
--- /dev/null
+++ b/profiles/battery/battery.c
@@ -0,0 +1,88 @@
+/*
+ *
+ *  BlueZ - Bluetooth protocol stack for Linux
+ *
+ *  Copyright (C) 2012 Texas Instruments, Inc.
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; either version 2 of the License, or
+ *  (at your option) any later version.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with this program; if not, write to the Free Software
+ *  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+ *
+ */
+
+#ifdef HAVE_CONFIG_H
+#include <config.h>
+#endif
+
+#include <glib.h>
+#include <bluetooth/uuid.h>
+
+#include "adapter.h"
+#include "device.h"
+#include "att.h"
+#include "gattrib.h"
+#include "gatt.h"
+#include "battery.h"
+
+struct battery {
+	struct btd_device	*dev;		/* Device reference */
+};
+
+static GSList *servers;
+
+static gint cmp_device(gconstpointer a, gconstpointer b)
+{
+	const struct battery *batt = a;
+	const struct btd_device *dev = b;
+
+	if (dev == batt->dev)
+		return 0;
+
+	return -1;
+}
+
+static void battery_free(gpointer user_data)
+{
+	struct battery *batt = user_data;
+
+	btd_device_unref(batt->dev);
+	g_free(batt);
+}
+
+
+int battery_register(struct btd_device *device)
+{
+	struct battery *batt;
+
+	batt = g_new0(struct battery, 1);
+	batt->dev = btd_device_ref(device);
+
+	servers = g_slist_prepend(servers, batt);
+
+	return 0;
+}
+
+void battery_unregister(struct btd_device *device)
+{
+	struct battery *batt;
+	GSList *l;
+
+	l = g_slist_find_custom(servers, device, cmp_device);
+	if (l == NULL)
+		return;
+
+	batt = l->data;
+	servers = g_slist_remove(servers, batt);
+
+	battery_free(batt);
+}
diff --git a/profiles/battery/battery.h b/profiles/battery/battery.h
new file mode 100644
index 0000000..9933343
--- /dev/null
+++ b/profiles/battery/battery.h
@@ -0,0 +1,24 @@
+/*
+ *
+ *  BlueZ - Bluetooth protocol stack for Linux
+ *
+ *  Copyright (C) 2012 Texas Instruments, Inc.
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; either version 2 of the License, or
+ *  (at your option) any later version.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with this program; if not, write to the Free Software
+ *  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+ *
+ */
+
+int battery_register(struct btd_device *device);
+void battery_unregister(struct btd_device *device);
diff --git a/profiles/battery/main.c b/profiles/battery/main.c
new file mode 100644
index 0000000..47f4249
--- /dev/null
+++ b/profiles/battery/main.c
@@ -0,0 +1,53 @@
+/*
+ *
+ *  BlueZ - Bluetooth protocol stack for Linux
+ *
+ *  Copyright (C) 2012 Texas Instruments, Inc.
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; either version 2 of the License, or
+ *  (at your option) any later version.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with this program; if not, write to the Free Software
+ *  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+ *
+ */
+
+#ifdef HAVE_CONFIG_H
+#include <config.h>
+#endif
+
+#include <stdint.h>
+#include <glib.h>
+#include <errno.h>
+
+#include "hcid.h"
+#include "plugin.h"
+#include "manager.h"
+#include "log.h"
+
+static int battery_init(void)
+{
+	if (!main_opts.gatt_enabled) {
+		DBG("GATT is disabled");
+		return -ENOTSUP;
+	}
+
+	return battery_manager_init();
+}
+
+static void battery_exit(void)
+{
+	battery_manager_exit();
+}
+
+BLUETOOTH_PLUGIN_DEFINE(battery, VERSION,
+			BLUETOOTH_PLUGIN_PRIORITY_DEFAULT, battery_init,
+			battery_exit)
diff --git a/profiles/battery/manager.c b/profiles/battery/manager.c
new file mode 100644
index 0000000..84b85a3
--- /dev/null
+++ b/profiles/battery/manager.c
@@ -0,0 +1,62 @@
+/*
+ *
+ *  BlueZ - Bluetooth protocol stack for Linux
+ *
+ *  Copyright (C) 2012 Texas Instruments, Inc.
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; either version 2 of the License, or
+ *  (at your option) any later version.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with this program; if not, write to the Free Software
+ *  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+ *
+ */
+
+#include <glib.h>
+#include <errno.h>
+#include <bluetooth/uuid.h>
+
+#include "adapter.h"
+#include "device.h"
+#include "att.h"
+#include "gattrib.h"
+#include "gatt.h"
+#include "battery.h"
+#include "manager.h"
+
+#define BATTERY_SERVICE_UUID		"0000180f-0000-1000-8000-00805f9b34fb"
+
+static int battery_driver_probe(struct btd_device *device, GSList *uuids)
+{
+	return battery_register(device);
+}
+
+static void battery_driver_remove(struct btd_device *device)
+{
+	battery_unregister(device);
+}
+
+static struct btd_device_driver battery_device_driver = {
+	.name	= "battery-driver",
+	.uuids	= BTD_UUIDS(BATTERY_SERVICE_UUID),
+	.probe	= battery_driver_probe,
+	.remove	= battery_driver_remove
+};
+
+int battery_manager_init(void)
+{
+	return btd_register_device_driver(&battery_device_driver);
+}
+
+void battery_manager_exit(void)
+{
+	btd_unregister_device_driver(&battery_device_driver);
+}
diff --git a/profiles/battery/manager.h b/profiles/battery/manager.h
new file mode 100644
index 0000000..b2c849f
--- /dev/null
+++ b/profiles/battery/manager.h
@@ -0,0 +1,24 @@
+/*
+ *
+ *  BlueZ - Bluetooth protocol stack for Linux
+ *
+ *  Copyright (C) 2012 Texas Instruments, Inc.
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; either version 2 of the License, or
+ *  (at your option) any later version.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with this program; if not, write to the Free Software
+ *  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+ *
+ */
+
+int battery_manager_init(void);
+void battery_manager_exit(void);
-- 
1.7.9.5

[PATCH v4 03/10] Battery: Add connection logic

[chen.ganir]

From: Chen Ganir <chen.ganir@ti.com>

Add connection logic to the Battery Plugin. When the driver is
loaded, it will request a connection to the remote device and
release the connection request when destroyed.
---
 profiles/battery/battery.c |   91 ++++++++++++++++++++++++++++++++++++++++++++
 profiles/battery/battery.h |    2 +
 profiles/battery/manager.c |    2 -
 3 files changed, 93 insertions(+), 2 deletions(-)

diff --git a/profiles/battery/battery.c b/profiles/battery/battery.c
index 7ed5707..f9ef73d 100644
--- a/profiles/battery/battery.c
+++ b/profiles/battery/battery.c
@@ -29,17 +29,29 @@
 
 #include "adapter.h"
 #include "device.h"
+#include "gattrib.h"
+#include "attio.h"
 #include "att.h"
 #include "gattrib.h"
 #include "gatt.h"
 #include "battery.h"
+#include "log.h"
 
 struct battery {
 	struct btd_device	*dev;		/* Device reference */
+	GAttrib			*attrib;	/* GATT connection */
+	guint			attioid;	/* Att watcher id */
+	struct att_range	*svc_range;	/* Battery range */
+	GSList			*chars;		/* Characteristics */
 };
 
 static GSList *servers;
 
+struct characteristic {
+	struct gatt_char	attr;	/* Characteristic */
+	struct battery		*batt;	/* Parent Battery Service */
+};
+
 static gint cmp_device(gconstpointer a, gconstpointer b)
 {
 	const struct battery *batt = a;
@@ -55,20 +67,99 @@ static void battery_free(gpointer user_data)
 {
 	struct battery *batt = user_data;
 
+	if (batt->chars != NULL)
+		g_slist_free_full(batt->chars, g_free);
+
+	if (batt->attioid > 0)
+		btd_device_remove_attio_callback(batt->dev, batt->attioid);
+
+	if (batt->attrib != NULL)
+		g_attrib_unref(batt->attrib);
+
 	btd_device_unref(batt->dev);
 	g_free(batt);
 }
 
+static void configure_battery_cb(GSList *characteristics, guint8 status,
+							gpointer user_data)
+{
+	struct battery *batt = user_data;
+	GSList *l;
+
+	if (status != 0) {
+		error("Discover Battery characteristics: %s",
+							att_ecode2str(status));
+		return;
+	}
+
+	for (l = characteristics; l; l = l->next) {
+		struct gatt_char *c = l->data;
+		struct characteristic *ch;
+
+		ch = g_new0(struct characteristic, 1);
+		ch->attr.handle = c->handle;
+		ch->attr.properties = c->properties;
+		ch->attr.value_handle = c->value_handle;
+		memcpy(ch->attr.uuid, c->uuid, MAX_LEN_UUID_STR + 1);
+		ch->batt = batt;
+
+		batt->chars = g_slist_append(batt->chars, ch);
+	}
+}
+
+static void attio_connected_cb(GAttrib *attrib, gpointer user_data)
+{
+	struct battery *batt = user_data;
+
+	batt->attrib = g_attrib_ref(attrib);
+
+	if (batt->chars == NULL) {
+		gatt_discover_char(batt->attrib, batt->svc_range->start,
+					batt->svc_range->end, NULL,
+					configure_battery_cb, batt);
+	}
+}
+
+static void attio_disconnected_cb(gpointer user_data)
+{
+	struct battery *batt = user_data;
+
+	g_attrib_unref(batt->attrib);
+	batt->attrib = NULL;
+}
+
+static gint primary_uuid_cmp(gconstpointer a, gconstpointer b)
+{
+	const struct gatt_primary *prim = a;
+	const char *uuid = b;
+
+	return g_strcmp0(prim->uuid, uuid);
+}
 
 int battery_register(struct btd_device *device)
 {
 	struct battery *batt;
+	struct gatt_primary *prim;
+	GSList *primaries, *l;
+
+	primaries = btd_device_get_primaries(device);
+
+	l = g_slist_find_custom(primaries, BATTERY_SERVICE_UUID,
+							primary_uuid_cmp);
+	prim = l->data;
 
 	batt = g_new0(struct battery, 1);
 	batt->dev = btd_device_ref(device);
 
+	batt->svc_range = g_new0(struct att_range, 1);
+	batt->svc_range->start = prim->range.start;
+	batt->svc_range->end = prim->range.end;
+
 	servers = g_slist_prepend(servers, batt);
 
+	batt->attioid = btd_device_add_attio_callback(device,
+				attio_connected_cb, attio_disconnected_cb,
+				batt);
 	return 0;
 }
 
diff --git a/profiles/battery/battery.h b/profiles/battery/battery.h
index 9933343..801186d 100644
--- a/profiles/battery/battery.h
+++ b/profiles/battery/battery.h
@@ -20,5 +20,7 @@
  *
  */
 
+#define BATTERY_SERVICE_UUID		"0000180f-0000-1000-8000-00805f9b34fb"
+
 int battery_register(struct btd_device *device);
 void battery_unregister(struct btd_device *device);
diff --git a/profiles/battery/manager.c b/profiles/battery/manager.c
index 84b85a3..22b8b20 100644
--- a/profiles/battery/manager.c
+++ b/profiles/battery/manager.c
@@ -32,8 +32,6 @@
 #include "battery.h"
 #include "manager.h"
 
-#define BATTERY_SERVICE_UUID		"0000180f-0000-1000-8000-00805f9b34fb"
-
 static int battery_driver_probe(struct btd_device *device, GSList *uuids)
 {
 	return battery_register(device);
-- 
1.7.9.5

[PATCH v4 04/10] Battery: Discover Characteristic Descriptors

[chen.ganir]

From: Chen Ganir <chen.ganir@ti.com>

Discover all characteristic descriptors, and build a descriptor
list
---
 profiles/battery/battery.c |   62 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 62 insertions(+)

diff --git a/profiles/battery/battery.c b/profiles/battery/battery.c
index f9ef73d..f93fdbc 100644
--- a/profiles/battery/battery.c
+++ b/profiles/battery/battery.c
@@ -50,6 +50,13 @@ static GSList *servers;
 struct characteristic {
 	struct gatt_char	attr;	/* Characteristic */
 	struct battery		*batt;	/* Parent Battery Service */
+	GSList				*desc;	/* Descriptors */
+};
+
+struct descriptor {
+	struct characteristic	*ch;	/* Parent Characteristic */
+	uint16_t		handle;	/* Descriptor Handle */
+	bt_uuid_t		uuid;	/* UUID */
 };
 
 static gint cmp_device(gconstpointer a, gconstpointer b)
@@ -80,7 +87,47 @@ static void battery_free(gpointer user_data)
 	g_free(batt);
 }
 
+static void discover_desc_cb(guint8 status, const guint8 *pdu, guint16 len,
+							gpointer user_data)
+{
+	struct characteristic *ch = user_data;
+	struct att_data_list *list;
+	uint8_t format;
+	int i;
+
+	if (status != 0) {
+		error("Discover all characteristic descriptors failed [%s]: %s",
+					ch->attr.uuid, att_ecode2str(status));
+		return;
+	}
+
+	list = dec_find_info_resp(pdu, len, &format);
+	if (list == NULL)
+		return;
+
+	for (i = 0; i < list->num; i++) {
+		struct descriptor *desc;
+		uint8_t *value;
+
+		value = list->data[i];
+		desc = g_new0(struct descriptor, 1);
+		desc->handle = att_get_u16(value);
+		desc->ch = ch;
+
+		if (format == 0x01)
+			desc->uuid = att_get_uuid16(&value[2]);
+		else
+			desc->uuid = att_get_uuid128(&value[2]);
+
+		ch->desc = g_slist_append(ch->desc, desc);
+	}
+
+	att_data_list_free(list);
+}
+
+
 static void configure_battery_cb(GSList *characteristics, guint8 status,
+
 							gpointer user_data)
 {
 	struct battery *batt = user_data;
@@ -95,6 +142,7 @@ static void configure_battery_cb(GSList *characteristics, guint8 status,
 	for (l = characteristics; l; l = l->next) {
 		struct gatt_char *c = l->data;
 		struct characteristic *ch;
+		uint16_t start, end;
 
 		ch = g_new0(struct characteristic, 1);
 		ch->attr.handle = c->handle;
@@ -104,6 +152,20 @@ static void configure_battery_cb(GSList *characteristics, guint8 status,
 		ch->batt = batt;
 
 		batt->chars = g_slist_append(batt->chars, ch);
+
+		start = c->value_handle + 1;
+
+		if (l->next != NULL) {
+			struct gatt_char *c = l->next->data;
+			if (start == c->handle)
+				continue;
+			end = c->handle - 1;
+		} else if (c->value_handle != batt->svc_range->end)
+			end = batt->svc_range->end;
+		else
+			continue;
+
+		gatt_find_info(batt->attrib, start, end, discover_desc_cb, ch);
 	}
 }
 
-- 
1.7.9.5

[PATCH v4 05/10] Battery: Get Battery ID

[chen.ganir]

From: Chen Ganir <chen.ganir@ti.com>

Read the battery level format characteristic descriptor to get the
unique namespace and description values.
---
 profiles/battery/battery.c |  112 +++++++++++++++++++++++++++++++++-----------
 1 file changed, 85 insertions(+), 27 deletions(-)

diff --git a/profiles/battery/battery.c b/profiles/battery/battery.c
index f93fdbc..a33ac8c 100644
--- a/profiles/battery/battery.c
+++ b/profiles/battery/battery.c
@@ -37,6 +37,8 @@
 #include "battery.h"
 #include "log.h"
 
+#define BATTERY_LEVEL_UUID	"00002a19-0000-1000-8000-00805f9b34fb"
+
 struct battery {
 	struct btd_device	*dev;		/* Device reference */
 	GAttrib			*attrib;	/* GATT connection */
@@ -48,15 +50,17 @@ struct battery {
 static GSList *servers;
 
 struct characteristic {
-	struct gatt_char	attr;	/* Characteristic */
-	struct battery		*batt;	/* Parent Battery Service */
+	struct gatt_char	attr;		/* Characteristic */
+	struct battery		*batt;		/* Parent Battery Service */
 	GSList				*desc;	/* Descriptors */
+	uint8_t			ns;		/* Battery Namespace */
+	uint16_t		description;	/* Battery description */
 };
 
 struct descriptor {
-	struct characteristic	*ch;	/* Parent Characteristic */
-	uint16_t		handle;	/* Descriptor Handle */
-	bt_uuid_t		uuid;	/* UUID */
+	struct characteristic	*ch;		/* Parent Characteristic */
+	uint16_t		handle;		/* Descriptor Handle */
+	bt_uuid_t		uuid;		/* UUID */
 };
 
 static gint cmp_device(gconstpointer a, gconstpointer b)
@@ -87,6 +91,55 @@ static void battery_free(gpointer user_data)
 	g_free(batt);
 }
 
+static void batterylevel_presentation_format_desc_cb(guint8 status,
+						const guint8 *pdu, guint16 len,
+						gpointer user_data)
+{
+	struct descriptor *desc = user_data;
+	uint8_t value[ATT_MAX_MTU];
+	int vlen;
+
+	if (status != 0) {
+		error("Presentation Format desc read failed: %s",
+							att_ecode2str(status));
+		return;
+	}
+
+	vlen = dec_read_resp(pdu, len, value, sizeof(value));
+	if (!vlen) {
+		error("Presentation Format desc read failed: Protocol error\n");
+		return;
+	}
+
+	if (vlen < 7) {
+		error("Presentation Format desc read failed: Invalid range");
+		return;
+	}
+
+	desc->ch->ns = value[4];
+	desc->ch->description = att_get_u16(&value[5]);
+}
+
+
+static void process_batterylevel_desc(struct descriptor *desc)
+{
+	struct characteristic *ch = desc->ch;
+	char uuidstr[MAX_LEN_UUID_STR];
+	bt_uuid_t btuuid;
+
+	bt_uuid16_create(&btuuid, GATT_CHARAC_FMT_UUID);
+
+	if (bt_uuid_cmp(&desc->uuid, &btuuid) == 0) {
+		gatt_read_char(ch->batt->attrib, desc->handle, 0,
+				batterylevel_presentation_format_desc_cb, desc);
+		return;
+	}
+
+	bt_uuid_to_string(&desc->uuid, uuidstr, MAX_LEN_UUID_STR);
+	DBG("Ignored descriptor %s characteristic %s", uuidstr,	ch->attr.uuid);
+}
+
+
 static void discover_desc_cb(guint8 status, const guint8 *pdu, guint16 len,
 							gpointer user_data)
 {
@@ -120,6 +173,7 @@ static void discover_desc_cb(guint8 status, const guint8 *pdu, guint16 len,
 			desc->uuid = att_get_uuid128(&value[2]);
 
 		ch->desc = g_slist_append(ch->desc, desc);
+		process_batterylevel_desc(desc);
 	}
 
 	att_data_list_free(list);
@@ -141,31 +195,35 @@ static void configure_battery_cb(GSList *characteristics, guint8 status,
 
 	for (l = characteristics; l; l = l->next) {
 		struct gatt_char *c = l->data;
-		struct characteristic *ch;
-		uint16_t start, end;
-
-		ch = g_new0(struct characteristic, 1);
-		ch->attr.handle = c->handle;
-		ch->attr.properties = c->properties;
-		ch->attr.value_handle = c->value_handle;
-		memcpy(ch->attr.uuid, c->uuid, MAX_LEN_UUID_STR + 1);
-		ch->batt = batt;
 
-		batt->chars = g_slist_append(batt->chars, ch);
-
-		start = c->value_handle + 1;
-
-		if (l->next != NULL) {
-			struct gatt_char *c = l->next->data;
-			if (start == c->handle)
+		if (g_strcmp0(c->uuid, BATTERY_LEVEL_UUID) == 0) {
+			struct characteristic *ch;
+			uint16_t start, end;
+
+			ch = g_new0(struct characteristic, 1);
+			ch->attr.handle = c->handle;
+			ch->attr.properties = c->properties;
+			ch->attr.value_handle = c->value_handle;
+			memcpy(ch->attr.uuid, c->uuid, MAX_LEN_UUID_STR + 1);
+			ch->batt = batt;
+
+			batt->chars = g_slist_append(batt->chars, ch);
+
+			start = c->value_handle + 1;
+
+			if (l->next != NULL) {
+				struct gatt_char *c = l->next->data;
+				if (start == c->handle)
+					continue;
+				end = c->handle - 1;
+			} else if (c->value_handle != batt->svc_range->end)
+				end = batt->svc_range->end;
+			else
 				continue;
-			end = c->handle - 1;
-		} else if (c->value_handle != batt->svc_range->end)
-			end = batt->svc_range->end;
-		else
-			continue;
 
-		gatt_find_info(batt->attrib, start, end, discover_desc_cb, ch);
+			gatt_find_info(batt->attrib, start, end,
+							discover_desc_cb, ch);
+		}
 	}
 }
 
-- 
1.7.9.5

[PATCH v4 06/10] Battery: Add Battery list to btd_device

[chen.ganir]

From: Chen Ganir <chen.ganir@ti.com>

Add peer battery list to the btd_device. New property added to Device
called Batteries.
---
 profiles/battery/battery.c |   14 ++++++++--
 src/device.c               |   65 ++++++++++++++++++++++++++++++++++++++++++++
 src/device.h               |    3 ++
 3 files changed, 79 insertions(+), 3 deletions(-)

diff --git a/profiles/battery/battery.c b/profiles/battery/battery.c
index a33ac8c..3f79c1f 100644
--- a/profiles/battery/battery.c
+++ b/profiles/battery/battery.c
@@ -50,6 +50,7 @@ struct battery {
 static GSList *servers;
 
 struct characteristic {
+	char			*path;          /* object path */
 	struct gatt_char	attr;		/* Characteristic */
 	struct battery		*batt;		/* Parent Battery Service */
 	GSList				*desc;	/* Descriptors */
@@ -151,12 +152,12 @@ static void discover_desc_cb(guint8 status, const guint8 *pdu, guint16 len,
 	if (status != 0) {
 		error("Discover all characteristic descriptors failed [%s]: %s",
 					ch->attr.uuid, att_ecode2str(status));
-		return;
+		goto update_char;
 	}
 
 	list = dec_find_info_resp(pdu, len, &format);
 	if (list == NULL)
-		return;
+		goto update_char;
 
 	for (i = 0; i < list->num; i++) {
 		struct descriptor *desc;
@@ -177,6 +178,14 @@ static void discover_desc_cb(guint8 status, const guint8 *pdu, guint16 len,
 	}
 
 	att_data_list_free(list);
+
+update_char:
+	ch->path = g_strdup_printf("%s/BATT-%02X-%04X",
+				device_get_path(ch->batt->dev),
+				ch->ns,
+				ch->description);
+
+	device_add_battery(ch->batt->dev, ch->path);
 }
 
 
@@ -206,7 +215,6 @@ static void configure_battery_cb(GSList *characteristics, guint8 status,
 			ch->attr.value_handle = c->value_handle;
 			memcpy(ch->attr.uuid, c->uuid, MAX_LEN_UUID_STR + 1);
 			ch->batt = batt;
-
 			batt->chars = g_slist_append(batt->chars, ch);
 
 			start = c->value_handle + 1;
diff --git a/src/device.c b/src/device.c
index f781298..c7c6c7d 100644
--- a/src/device.c
+++ b/src/device.c
@@ -124,6 +124,10 @@ struct att_callbacks {
 	gpointer user_data;
 };
 
+struct btd_battery {
+	char *path;
+};
+
 struct btd_device {
 	bdaddr_t	bdaddr;
 	uint8_t		bdaddr_type;
@@ -169,6 +173,7 @@ struct btd_device {
 
 	GIOChannel      *att_io;
 	guint		cleanup_id;
+	GSList		*batteries;
 };
 
 static uint16_t uuid_list[] = {
@@ -259,6 +264,7 @@ static void device_free(gpointer user_data)
 	g_slist_free_full(device->primaries, g_free);
 	g_slist_free_full(device->attios, g_free);
 	g_slist_free_full(device->attios_offline, g_free);
+	g_slist_free_full(device->batteries, g_free);
 
 	attio_cleanup(device);
 
@@ -433,6 +439,15 @@ static DBusMessage *get_properties(DBusConnection *conn,
 	ptr = adapter_get_path(adapter);
 	dict_append_entry(&dict, "Adapter", DBUS_TYPE_OBJECT_PATH, &ptr);
 
+	/* Batteries */
+	str = g_new0(char *, g_slist_length(device->batteries) + 1);
+	for (i = 0, l = device->batteries; l; l = l->next, i++) {
+		struct btd_battery *b = l->data;
+		str[i] = b->path;
+	}
+	dict_append_array(&dict, "Batteries", DBUS_TYPE_OBJECT_PATH, &str, i);
+	g_free(str);
+
 	dbus_message_iter_close_container(&iter, &dict);
 
 	return reply;
@@ -1215,6 +1230,9 @@ void device_remove(struct btd_device *device, gboolean remove_stored)
 	g_slist_free(device->drivers);
 	device->drivers = NULL;
 
+	g_slist_free(device->batteries);
+	device->batteries = NULL;
+
 	attrib_client_unregister(device->services);
 
 	btd_device_unref(device);
@@ -3147,3 +3165,50 @@ void device_set_pnpid(struct btd_device *device, uint8_t vendor_id_src,
 	device_set_product(device, product_id);
 	device_set_version(device, product_ver);
 }
+
+static void batteries_changed(struct btd_device *device)
+{
+	DBusConnection *conn = get_dbus_connection();
+	char **batteries;
+	GSList *l;
+	int i;
+
+	batteries = g_new0(char *, g_slist_length(device->batteries) + 1);
+	for (i = 0, l = device->batteries; l; l = l->next, i++) {
+		struct btd_battery *batt = l->data;
+		batteries[i] = batt->path;
+	}
+
+	emit_array_property_changed(conn, device->path, DEVICE_INTERFACE,
+				    "Batteries", DBUS_TYPE_STRING, &batteries,
+				    i);
+
+	g_free(batteries);
+}
+
+void device_add_battery(struct btd_device *device, char *path)
+{
+	struct btd_battery *batt;
+
+	batt = g_new0(struct btd_battery, 1);
+	batt->path = g_strdup(path);
+	device->batteries = g_slist_append(device->batteries, batt);
+	batteries_changed(device);
+}
+
+void device_remove_battery(struct btd_device *device, char *path)
+{
+	GSList *l;
+
+	for (l = device->batteries; l; l = l->next) {
+		struct btd_battery *b = l->data;
+
+		if (g_strcmp0(path, b->path) == 0) {
+			device->batteries = g_slist_remove(device->batteries, b);
+			g_free(b->path);
+			g_free(b);
+			batteries_changed(device);
+			return;
+		}
+	}
+}
diff --git a/src/device.h b/src/device.h
index 26e17f7..db71a8a 100644
--- a/src/device.h
+++ b/src/device.h
@@ -126,3 +126,6 @@ int device_unblock(DBusConnection *conn, struct btd_device *device,
 void device_set_pnpid(struct btd_device *device, uint8_t vendor_id_src,
 			uint16_t vendor_id, uint16_t product_id,
 			uint16_t product_ver);
+
+void device_add_battery(struct btd_device *device, char *path);
+void device_remove_battery(struct btd_device *device, char *path);
-- 
1.7.9.5

[PATCH v4 07/10] Battery: Read Battery level characteristic

[chen.ganir]

From: Chen Ganir <chen.ganir@ti.com>

Add support for reading the battery level characteristic on
connection establishment.
---
 profiles/battery/battery.c |   44 ++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 42 insertions(+), 2 deletions(-)

diff --git a/profiles/battery/battery.c b/profiles/battery/battery.c
index 3f79c1f..6baf2db 100644
--- a/profiles/battery/battery.c
+++ b/profiles/battery/battery.c
@@ -56,6 +56,7 @@ struct characteristic {
 	GSList				*desc;	/* Descriptors */
 	uint8_t			ns;		/* Battery Namespace */
 	uint16_t		description;	/* Battery description */
+	uint8_t        level;
 };
 
 struct descriptor {
@@ -92,6 +93,40 @@ static void battery_free(gpointer user_data)
 	g_free(batt);
 }
 
+static void read_batterylevel_cb(guint8 status, const guint8 *pdu, guint16 len,
+							gpointer user_data)
+{
+	struct characteristic *ch = user_data;
+	uint8_t value[ATT_MAX_MTU];
+	int vlen;
+
+	if (status != 0) {
+		error("Failed to read Battery Level:%s", att_ecode2str(status));
+		return;
+	}
+
+	vlen = dec_read_resp(pdu, len, value, sizeof(value));
+	if (!vlen) {
+		error("Failed to read Battery Level: Protocol error\n");
+		return;
+	}
+
+	if (vlen < 1) {
+		error("Failed to read Battery Level: Wrong pdu len");
+		return;
+	}
+
+	ch->level = value[0];
+}
+
+static void process_batteryservice_char(struct characteristic *ch)
+{
+	if (g_strcmp0(ch->attr.uuid, BATTERY_LEVEL_UUID) == 0) {
+		gatt_read_char(ch->batt->attrib, ch->attr.value_handle, 0,
+						read_batterylevel_cb, ch);
+	}
+}
+
 static void batterylevel_presentation_format_desc_cb(guint8 status,
 						const guint8 *pdu, guint16 len,
 						gpointer user_data)
@@ -140,7 +175,6 @@ static void process_batterylevel_desc(struct descriptor *desc)
 	DBG("Ignored descriptor %s characteristic %s", uuidstr,	ch->attr.uuid);
 }
 
-
 static void discover_desc_cb(guint8 status, const guint8 *pdu, guint16 len,
 							gpointer user_data)
 {
@@ -186,9 +220,9 @@ update_char:
 				ch->description);
 
 	device_add_battery(ch->batt->dev, ch->path);
+	process_batteryservice_char(ch);
 }
 
-
 static void configure_battery_cb(GSList *characteristics, guint8 status,
 
 							gpointer user_data)
@@ -245,6 +279,12 @@ static void attio_connected_cb(GAttrib *attrib, gpointer user_data)
 		gatt_discover_char(batt->attrib, batt->svc_range->start,
 					batt->svc_range->end, NULL,
 					configure_battery_cb, batt);
+	} else {
+		GSList *l;
+		for (l = batt->chars; l; l = l->next) {
+			struct characteristic *c = l->data;
+			process_batteryservice_char(c);
+		}
 	}
 }
 
-- 
1.7.9.5

[PATCH v4 08/10] Battery: Add Battery D-BUS API

[chen.ganir]

From: Chen Ganir <chen.ganir@ti.com>

Add Battery level specific API
---
 profiles/battery/battery.c |   84 ++++++++++++++++++++++++++++++++++++++++----
 profiles/battery/battery.h |    2 +-
 profiles/battery/main.c    |   18 ++++++++--
 profiles/battery/manager.c |   19 +++++++---
 profiles/battery/manager.h |    2 +-
 5 files changed, 111 insertions(+), 14 deletions(-)

diff --git a/profiles/battery/battery.c b/profiles/battery/battery.c
index 6baf2db..108813c 100644
--- a/profiles/battery/battery.c
+++ b/profiles/battery/battery.c
@@ -24,7 +24,9 @@
 #include <config.h>
 #endif
 
-#include <glib.h>
+#include <gdbus.h>
+#include <errno.h>
+#include <dbus/dbus.h>
 #include <bluetooth/uuid.h>
 
 #include "adapter.h"
@@ -34,12 +36,16 @@
 #include "att.h"
 #include "gattrib.h"
 #include "gatt.h"
+#include "dbus-common.h"
 #include "battery.h"
 #include "log.h"
 
+#define BATTERY_INTERFACE	"org.bluez.Battery"
+
 #define BATTERY_LEVEL_UUID	"00002a19-0000-1000-8000-00805f9b34fb"
 
 struct battery {
+	DBusConnection		*conn;		/* The connection to the bus */
 	struct btd_device	*dev;		/* Device reference */
 	GAttrib			*attrib;	/* GATT connection */
 	guint			attioid;	/* Att watcher id */
@@ -65,6 +71,28 @@ struct descriptor {
 	bt_uuid_t		uuid;		/* UUID */
 };
 
+static void char_free(gpointer user_data)
+{
+	struct characteristic *c = user_data;
+
+	g_slist_free_full(c->desc, g_free);
+
+	g_free(c);
+}
+
+static void char_interface_free(gpointer user_data)
+{
+	struct characteristic *c = user_data;
+	device_remove_battery(c->batt->dev, c->path);
+
+	g_dbus_unregister_interface(c->batt->conn,
+			c->path, BATTERY_INTERFACE);
+
+	g_free(c->path);
+
+	char_free(c);
+}
+
 static gint cmp_device(gconstpointer a, gconstpointer b)
 {
 	const struct battery *batt = a;
@@ -81,7 +109,7 @@ static void battery_free(gpointer user_data)
 	struct battery *batt = user_data;
 
 	if (batt->chars != NULL)
-		g_slist_free_full(batt->chars, g_free);
+		g_slist_free_full(batt->chars, char_interface_free);
 
 	if (batt->attioid > 0)
 		btd_device_remove_attio_callback(batt->dev, batt->attioid);
@@ -89,7 +117,10 @@ static void battery_free(gpointer user_data)
 	if (batt->attrib != NULL)
 		g_attrib_unref(batt->attrib);
 
+
+	dbus_connection_unref(batt->conn);
 	btd_device_unref(batt->dev);
+	g_free(batt->svc_range);
 	g_free(batt);
 }
 
@@ -175,6 +206,38 @@ static void process_batterylevel_desc(struct descriptor *desc)
 	DBG("Ignored descriptor %s characteristic %s", uuidstr,	ch->attr.uuid);
 }
 
+static DBusMessage *get_properties(DBusConnection *conn, DBusMessage *msg,
+								void *data)
+{
+	struct characteristic *c = data;
+	DBusMessageIter iter;
+	DBusMessageIter dict;
+	DBusMessage *reply;
+
+	reply = dbus_message_new_method_return(msg);
+	if (reply == NULL)
+		return NULL;
+
+	dbus_message_iter_init_append(reply, &iter);
+
+	dbus_message_iter_open_container(&iter, DBUS_TYPE_ARRAY,
+			DBUS_DICT_ENTRY_BEGIN_CHAR_AS_STRING
+			DBUS_TYPE_STRING_AS_STRING DBUS_TYPE_VARIANT_AS_STRING
+			DBUS_DICT_ENTRY_END_CHAR_AS_STRING, &dict);
+
+	dict_append_entry(&dict, "Level", DBUS_TYPE_BYTE, &c->level);
+
+	dbus_message_iter_close_container(&iter, &dict);
+
+	return reply;
+}
+
+static GDBusMethodTable battery_methods[] = {
+	{ GDBUS_METHOD("GetProperties",
+				NULL, GDBUS_ARGS({ "properties", "a{sv}" }),
+				get_properties) },
+	{ }
+};
 static void discover_desc_cb(guint8 status, const guint8 *pdu, guint16 len,
 							gpointer user_data)
 {
@@ -219,10 +282,17 @@ update_char:
 				ch->ns,
 				ch->description);
 
-	device_add_battery(ch->batt->dev, ch->path);
+	if (!g_dbus_register_interface(ch->batt->conn, ch->path,
+				BATTERY_INTERFACE,
+				battery_methods, NULL, NULL,
+				ch, NULL)) {
+		error("D-Bus register interface %s failed",
+		      BATTERY_INTERFACE);
+	} else {
+		device_add_battery(ch->batt->dev, ch->path);
 	process_batteryservice_char(ch);
+	}
 }
-
 static void configure_battery_cb(GSList *characteristics, guint8 status,
 
 							gpointer user_data)
@@ -249,6 +319,7 @@ static void configure_battery_cb(GSList *characteristics, guint8 status,
 			ch->attr.value_handle = c->value_handle;
 			memcpy(ch->attr.uuid, c->uuid, MAX_LEN_UUID_STR + 1);
 			ch->batt = batt;
+
 			batt->chars = g_slist_append(batt->chars, ch);
 
 			start = c->value_handle + 1;
@@ -304,7 +375,7 @@ static gint primary_uuid_cmp(gconstpointer a, gconstpointer b)
 	return g_strcmp0(prim->uuid, uuid);
 }
 
-int battery_register(struct btd_device *device)
+int battery_register(DBusConnection *connection, struct btd_device *device)
 {
 	struct battery *batt;
 	struct gatt_primary *prim;
@@ -318,7 +389,7 @@ int battery_register(struct btd_device *device)
 
 	batt = g_new0(struct battery, 1);
 	batt->dev = btd_device_ref(device);
-
+	batt->conn = dbus_connection_ref(connection);
 	batt->svc_range = g_new0(struct att_range, 1);
 	batt->svc_range->start = prim->range.start;
 	batt->svc_range->end = prim->range.end;
@@ -328,6 +399,7 @@ int battery_register(struct btd_device *device)
 	batt->attioid = btd_device_add_attio_callback(device,
 				attio_connected_cb, attio_disconnected_cb,
 				batt);
+
 	return 0;
 }
 
diff --git a/profiles/battery/battery.h b/profiles/battery/battery.h
index 801186d..8231949 100644
--- a/profiles/battery/battery.h
+++ b/profiles/battery/battery.h
@@ -21,6 +21,6 @@
  */
 
 #define BATTERY_SERVICE_UUID		"0000180f-0000-1000-8000-00805f9b34fb"
+int battery_register(DBusConnection *conn, struct btd_device *device);
 
-int battery_register(struct btd_device *device);
 void battery_unregister(struct btd_device *device);
diff --git a/profiles/battery/main.c b/profiles/battery/main.c
index 47f4249..49c7249 100644
--- a/profiles/battery/main.c
+++ b/profiles/battery/main.c
@@ -25,7 +25,7 @@
 #endif
 
 #include <stdint.h>
-#include <glib.h>
+#include <gdbus.h>
 #include <errno.h>
 
 #include "hcid.h"
@@ -33,6 +33,8 @@
 #include "manager.h"
 #include "log.h"
 
+static DBusConnection *connection;
+
 static int battery_init(void)
 {
 	if (!main_opts.gatt_enabled) {
@@ -40,12 +42,24 @@ static int battery_init(void)
 		return -ENOTSUP;
 	}
 
-	return battery_manager_init();
+	connection = dbus_bus_get(DBUS_BUS_SYSTEM, NULL);
+	if (connection == NULL)
+		return -EIO;
+
+	if (battery_manager_init(connection) < 0) {
+		dbus_connection_unref(connection);
+		return -EIO;
+	}
+
+	return 0;
 }
 
 static void battery_exit(void)
 {
 	battery_manager_exit();
+
+	dbus_connection_unref(connection);
+	connection = NULL;
 }
 
 BLUETOOTH_PLUGIN_DEFINE(battery, VERSION,
diff --git a/profiles/battery/manager.c b/profiles/battery/manager.c
index 22b8b20..13bc806 100644
--- a/profiles/battery/manager.c
+++ b/profiles/battery/manager.c
@@ -20,7 +20,7 @@
  *
  */
 
-#include <glib.h>
+#include <gdbus.h>
 #include <errno.h>
 #include <bluetooth/uuid.h>
 
@@ -32,9 +32,11 @@
 #include "battery.h"
 #include "manager.h"
 
+static DBusConnection *connection;
+
 static int battery_driver_probe(struct btd_device *device, GSList *uuids)
 {
-	return battery_register(device);
+	return battery_register(connection, device);
 }
 
 static void battery_driver_remove(struct btd_device *device)
@@ -49,12 +51,21 @@ static struct btd_device_driver battery_device_driver = {
 	.remove	= battery_driver_remove
 };
 
-int battery_manager_init(void)
+int battery_manager_init(DBusConnection *conn)
 {
-	return btd_register_device_driver(&battery_device_driver);
+	int ret;
+
+	ret = btd_register_device_driver(&battery_device_driver);
+	if (!ret)
+		connection = dbus_connection_ref(conn);
+
+	return ret;
 }
 
 void battery_manager_exit(void)
 {
 	btd_unregister_device_driver(&battery_device_driver);
+
+	dbus_connection_unref(connection);
+	connection = NULL;
 }
diff --git a/profiles/battery/manager.h b/profiles/battery/manager.h
index b2c849f..60acb1d 100644
--- a/profiles/battery/manager.h
+++ b/profiles/battery/manager.h
@@ -20,5 +20,5 @@
  *
  */
 
-int battery_manager_init(void);
+int battery_manager_init(DBusConnection *conn);
 void battery_manager_exit(void);
-- 
1.7.9.5

[PATCH v4 09/10] Battery: Add support for notifications

[chen.ganir]

From: Chen Ganir <chen.ganir@ti.com>

Add support for emitting PropertyChanged when a battery level
characteristic notification is sent from the peer device.
---
 profiles/battery/battery.c |  112 +++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 110 insertions(+), 2 deletions(-)

diff --git a/profiles/battery/battery.c b/profiles/battery/battery.c
index 108813c..3d3fb6b 100644
--- a/profiles/battery/battery.c
+++ b/profiles/battery/battery.c
@@ -50,6 +50,7 @@ struct battery {
 	GAttrib			*attrib;	/* GATT connection */
 	guint			attioid;	/* Att watcher id */
 	struct att_range	*svc_range;	/* Battery range */
+	guint                   attnotid;       /* Att notifications id */
 	GSList			*chars;		/* Characteristics */
 };
 
@@ -63,6 +64,7 @@ struct characteristic {
 	uint8_t			ns;		/* Battery Namespace */
 	uint16_t		description;	/* Battery description */
 	uint8_t        level;
+	gboolean		canNotify;
 };
 
 struct descriptor {
@@ -104,6 +106,14 @@ static gint cmp_device(gconstpointer a, gconstpointer b)
 	return -1;
 }
 
+static gint cmp_char_val_handle(gconstpointer a, gconstpointer b)
+{
+	const struct characteristic *ch = a;
+	const uint16_t *handle = b;
+
+	return ch->attr.value_handle - *handle;
+}
+
 static void battery_free(gpointer user_data)
 {
 	struct battery *batt = user_data;
@@ -117,6 +127,10 @@ static void battery_free(gpointer user_data)
 	if (batt->attrib != NULL)
 		g_attrib_unref(batt->attrib);
 
+	if (batt->attrib != NULL) {
+		g_attrib_unregister(batt->attrib, batt->attnotid);
+		g_attrib_unref(batt->attrib);
+	}
 
 	dbus_connection_unref(batt->conn);
 	btd_device_unref(batt->dev);
@@ -158,6 +172,18 @@ static void process_batteryservice_char(struct characteristic *ch)
 	}
 }
 
+static void batterylevel_enable_notify_cb(guint8 status, const guint8 *pdu,
+						guint16 len, gpointer user_data)
+{
+	struct characteristic *ch = (struct characteristic *)user_data;
+
+	if (status != 0) {
+		error("Could not enable batt level notification.");
+		ch->canNotify = FALSE;
+		process_batteryservice_char(ch);
+	}
+}
+
 static void batterylevel_presentation_format_desc_cb(guint8 status,
 						const guint8 *pdu, guint16 len,
 						gpointer user_data)
@@ -194,6 +220,21 @@ static void process_batterylevel_desc(struct descriptor *desc)
 	char uuidstr[MAX_LEN_UUID_STR];
 	bt_uuid_t btuuid;
 
+	bt_uuid16_create(&btuuid, GATT_CLIENT_CHARAC_CFG_UUID);
+
+	if (bt_uuid_cmp(&desc->uuid, &btuuid) == 0 && g_strcmp0(ch->attr.uuid,
+						BATTERY_LEVEL_UUID) == 0) {
+		uint8_t atval[2];
+		uint16_t val;
+
+		val = GATT_CLIENT_CHARAC_CFG_NOTIF_BIT;
+
+		att_put_u16(val, atval);
+		gatt_write_char(ch->batt->attrib, desc->handle, atval, 2,
+					batterylevel_enable_notify_cb, ch);
+		return;
+	}
+
 	bt_uuid16_create(&btuuid, GATT_CHARAC_FMT_UUID);
 
 	if (bt_uuid_cmp(&desc->uuid, &btuuid) == 0) {
@@ -238,6 +279,13 @@ static GDBusMethodTable battery_methods[] = {
 				get_properties) },
 	{ }
 };
+static GDBusSignalTable battery_signals[] = {
+	{ GDBUS_SIGNAL("PropertyChanged",
+		GDBUS_ARGS({ "name", "s" }, { "value", "v" })) },
+	{ }
+};
+
+
 static void discover_desc_cb(guint8 status, const guint8 *pdu, guint16 len,
 							gpointer user_data)
 {
@@ -284,7 +332,7 @@ update_char:
 
 	if (!g_dbus_register_interface(ch->batt->conn, ch->path,
 				BATTERY_INTERFACE,
-				battery_methods, NULL, NULL,
+				battery_methods, battery_signals, NULL,
 				ch, NULL)) {
 		error("D-Bus register interface %s failed",
 		      BATTERY_INTERFACE);
@@ -293,6 +341,12 @@ update_char:
 	process_batteryservice_char(ch);
 	}
 }
+static void emit_battery_level_changed(struct characteristic *c)
+{
+	emit_property_changed(c->batt->conn, c->path, BATTERY_INTERFACE,
+					"Level", DBUS_TYPE_BYTE, &c->level);
+}
+
 static void configure_battery_cb(GSList *characteristics, guint8 status,
 
 							gpointer user_data)
@@ -340,12 +394,63 @@ static void configure_battery_cb(GSList *characteristics, guint8 status,
 	}
 }
 
+static void proc_batterylevel(struct characteristic *c, const uint8_t *pdu,
+						uint16_t len, gboolean final)
+{
+	uint8_t new_batt_level = 0;
+	gboolean changed = FALSE;
+
+	if (!pdu) {
+		error("Battery level notification: Invalid pdu length");
+		goto done;
+	}
+
+	new_batt_level = pdu[1];
+
+	if (new_batt_level != c->level)
+		changed = TRUE;
+
+	c->level = new_batt_level;
+
+done:
+	if (changed)
+		emit_battery_level_changed(c);
+}
+
+static void notif_handler(const uint8_t *pdu, uint16_t len, gpointer user_data)
+{
+	struct battery *batt = user_data;
+	struct characteristic *ch;
+	uint16_t handle;
+	GSList *l;
+
+	if (len < 3) {
+		error("notif_handler: Bad pdu received");
+		return;
+	}
+
+	handle = att_get_u16(&pdu[1]);
+	l = g_slist_find_custom(batt->chars, &handle, cmp_char_val_handle);
+	if (l == NULL) {
+		error("notif_handler: Unexpected handle 0x%04x", handle);
+		return;
+	}
+
+	ch = l->data;
+	if (g_strcmp0(ch->attr.uuid, BATTERY_LEVEL_UUID) == 0) {
+		proc_batterylevel(ch, pdu, len, FALSE);
+	}
+}
+
 static void attio_connected_cb(GAttrib *attrib, gpointer user_data)
 {
 	struct battery *batt = user_data;
 
 	batt->attrib = g_attrib_ref(attrib);
 
+	batt->attnotid = g_attrib_register(batt->attrib, ATT_OP_HANDLE_NOTIFY,
+						notif_handler, batt, NULL);
+
 	if (batt->chars == NULL) {
 		gatt_discover_char(batt->attrib, batt->svc_range->start,
 					batt->svc_range->end, NULL,
@@ -354,7 +459,8 @@ static void attio_connected_cb(GAttrib *attrib, gpointer user_data)
 		GSList *l;
 		for (l = batt->chars; l; l = l->next) {
 			struct characteristic *c = l->data;
-			process_batteryservice_char(c);
+			if (!c->canNotify)
+				process_batteryservice_char(c);
 		}
 	}
 }
@@ -363,6 +469,8 @@ static void attio_disconnected_cb(gpointer user_data)
 {
 	struct battery *batt = user_data;
 
+	g_attrib_unregister(batt->attrib, batt->attnotid);
+	batt->attnotid = 0;
 	g_attrib_unref(batt->attrib);
 	batt->attrib = NULL;
 }
-- 
1.7.9.5

Re: [PATCH v4 01/10] Battery: Add Battery Service Gatt Client

[Johan Hedberg]

Hi Chen,

On Thu, Aug 16, 2012, chen.ganir@ti.com wrote:
> --- a/doc/device-api.txt
> +++ b/doc/device-api.txt
> @@ -179,3 +179,8 @@ Properties	string Address [readonly]
>  			Note that this property can exhibit false-positives
>  			in the case of Bluetooth 2.1 (or newer) devices that
>  			have disabled Extended Inquiry Response support.
> +
> +		array{string} Batteries [readonly]
> +
> +			List of device battery object paths that represents the available
> +			batteries on the remote device.

I don't think it's ok to pollute the Device interface or src/device.c
with profile-specific details. That should happen in profile-specific
plugins and interfaces. Since we're switching over to object manager
maybe an interface/property like this isn't needed at all? (even if it
would be needed the type should be array{object} and not array{string}

Johan

Re: [PATCH v4 01/10] Battery: Add Battery Service Gatt Client

[Chen Ganir]

On 08/16/2012 02:39 PM, Johan Hedberg wrote:
> Hi Chen,
>
> On Thu, Aug 16, 2012, chen.ganir@ti.com wrote:
>> --- a/doc/device-api.txt
>> +++ b/doc/device-api.txt
>> @@ -179,3 +179,8 @@ Properties	string Address [readonly]
>>   			Note that this property can exhibit false-positives
>>   			in the case of Bluetooth 2.1 (or newer) devices that
>>   			have disabled Extended Inquiry Response support.
>> +
>> +		array{string} Batteries [readonly]
>> +
>> +			List of device battery object paths that represents the available
>> +			batteries on the remote device.
>
> I don't think it's ok to pollute the Device interface or src/device.c
> with profile-specific details. That should happen in profile-specific
> plugins and interfaces. Since we're switching over to object manager
> maybe an interface/property like this isn't needed at all? (even if it
> would be needed the type should be array{object} and not array{string}
You are correct, the array should be object instead of string. However, 
i fail to understand why the object manager prevents this from getting 
accepted - do you have a time estimation when the object manager should 
be active or available ? I do not think this patch set should be 
deferred until we have changed the dbus API.

I prefer this way of putting the batteries below the device, since it is 
obvious which battery belongs to each device. The other option i can 
think of is to have another interface registered on the device object path:

Service		org.bluez
Interface	org.bluez.Batteries
Object path	[variable prefix]/{hci0,..}/dev_XX_XX_XX_XX_XX_XX

Methods	dict GetProperties()

		Returns all properties for the interface. See the
		Properties section for the available properties.

Signals		ProperyChanged(string name, variant value)

		This signal indicates a changed value of the given
		property.

Properties	array{object} Batteries [readonly]

		List of device battery object paths that represents the available 
batteries on the remote devices.


Service		org.bluez
Interface	org.bluez.Battery
Object path	[variable prefix]/{hci0,..}/dev_XX_XX_XX_XX_XX_XX/BATT-NN-DDDD


Methods	dict GetProperties()

		Returns all properties for the interface. See the
		Properties section for the available properties.

Signals		PropertyChanged(string name, variant value)

		This signal indicates a changed value of the given
		property.

Properties	byte Level [readonly]

			Battery level (0-100).

Any other suggestion ?


>
> Johan
>


-- 
BR,
Chen Ganir
Texas Instruments

Re: [PATCH v4 01/10] Battery: Add Battery Service Gatt Client

[Johan Hedberg]

Hi Chen,

On Thu, Aug 16, 2012, Chen Ganir wrote:
> >I don't think it's ok to pollute the Device interface or src/device.c
> >with profile-specific details. That should happen in profile-specific
> >plugins and interfaces. Since we're switching over to object manager
> >maybe an interface/property like this isn't needed at all? (even if it
> >would be needed the type should be array{object} and not array{string}
> 
> You are correct, the array should be object instead of string.
> However, i fail to understand why the object manager prevents this
> from getting accepted - do you have a time estimation when the
> object manager should be active or available ?

By the time of our next release (as per the content under the BlueZ 5
section in our TODO file).

> I prefer this way of putting the batteries below the device, since it
> is obvious which battery belongs to each device.

I fully agree with this. It's not what I had an issue with. However, if
we really want to make this clear you could additionally have a "Device"
property for each battery object pointing back to the parent object.

> The other option i can think of is to have another interface
> registered on the device object path:
> 
> Service		org.bluez
> Interface	org.bluez.Batteries
> Object path	[variable prefix]/{hci0,..}/dev_XX_XX_XX_XX_XX_XX
> 
> Methods	dict GetProperties()
> 
> 		Returns all properties for the interface. See the
> 		Properties section for the available properties.
> 
> Signals		ProperyChanged(string name, variant value)
> 
> 		This signal indicates a changed value of the given
> 		property.
> 
> Properties	array{object} Batteries [readonly]
> 
> 		List of device battery object paths that represents the available
> batteries on the remote devices.
> 
> 
> Service		org.bluez
> Interface	org.bluez.Battery
> Object path	[variable prefix]/{hci0,..}/dev_XX_XX_XX_XX_XX_XX/BATT-NN-DDDD
> 
> 
> Methods	dict GetProperties()
> 
> 		Returns all properties for the interface. See the
> 		Properties section for the available properties.
> 
> Signals		PropertyChanged(string name, variant value)
> 
> 		This signal indicates a changed value of the given
> 		property.
> 
> Properties	byte Level [readonly]
> 
> 			Battery level (0-100).
> 
> Any other suggestion ?

That would work, although both of these interfaces are rather
light-weight in that they only contain a single property which begs the
question whether this is a bit over-kill.

Another option is to do away with the per-battery object somehow and
expose all batteries through a single interface. I'm not so sure that's
a good idea though since it could prevent future extensibility of the
API and doesn't reuse the common tools we have for object-like
abstractions (which a battery certainly is).

Johan

Re: [PATCH v4 01/10] Battery: Add Battery Service Gatt Client

[Chen Ganir]

Johan,
On 08/16/2012 03:44 PM, Johan Hedberg wrote:
> Hi Chen,
>
> On Thu, Aug 16, 2012, Chen Ganir wrote:
>>> I don't think it's ok to pollute the Device interface or src/device.c
>>> with profile-specific details. That should happen in profile-specific
>>> plugins and interfaces. Since we're switching over to object manager
>>> maybe an interface/property like this isn't needed at all? (even if it
>>> would be needed the type should be array{object} and not array{string}
>>
>> You are correct, the array should be object instead of string.
>> However, i fail to understand why the object manager prevents this
>> from getting accepted - do you have a time estimation when the
>> object manager should be active or available ?
>
> By the time of our next release (as per the content under the BlueZ 5
> section in our TODO file).
Is there anywhere some documentation or instructions how to adapt the 
API for the new object manager ? Is it backward compatible to what we 
have today ? Do we really hold back DBUS additions until we change the 
DBUS API ?

>
>> I prefer this way of putting the batteries below the device, since it
>> is obvious which battery belongs to each device.
>
> I fully agree with this. It's not what I had an issue with. However, if
> we really want to make this clear you could additionally have a "Device"
> property for each battery object pointing back to the parent object.
I can do that, if it helps. Although it is not necessary, since the 
object path for a battery already contains the device objec path in it :
[variable prefix]/{hci0,..}/dev_XX_XX_XX_XX_XX_XX/BATT-NN-DDDD
Adding a device object path as a property will be redundant.

>
>> The other option i can think of is to have another interface
>> registered on the device object path:
>>
>> Service		org.bluez
>> Interface	org.bluez.Batteries
>> Object path	[variable prefix]/{hci0,..}/dev_XX_XX_XX_XX_XX_XX
>>
>> Methods	dict GetProperties()
>>
>> 		Returns all properties for the interface. See the
>> 		Properties section for the available properties.
>>
>> Signals		ProperyChanged(string name, variant value)
>>
>> 		This signal indicates a changed value of the given
>> 		property.
>>
>> Properties	array{object} Batteries [readonly]
>>
>> 		List of device battery object paths that represents the available
>> batteries on the remote devices.
>>
>>
>> Service		org.bluez
>> Interface	org.bluez.Battery
>> Object path	[variable prefix]/{hci0,..}/dev_XX_XX_XX_XX_XX_XX/BATT-NN-DDDD
>>
>>
>> Methods	dict GetProperties()
>>
>> 		Returns all properties for the interface. See the
>> 		Properties section for the available properties.
>>
>> Signals		PropertyChanged(string name, variant value)
>>
>> 		This signal indicates a changed value of the given
>> 		property.
>>
>> Properties	byte Level [readonly]
>>
>> 			Battery level (0-100).
>>
>> Any other suggestion ?
>
> That would work, although both of these interfaces are rather
> light-weight in that they only contain a single property which begs the
> question whether this is a bit over-kill.
I know. I believe so as well, but if you want to separate the 
interfaces, it is a must.

>
> Another option is to do away with the per-battery object somehow and
> expose all batteries through a single interface. I'm not so sure that's
> a good idea though since it could prevent future extensibility of the
> API and doesn't reuse the common tools we have for object-like
> abstractions (which a battery certainly is).
If we do what you suggest, a battery name (namespace+descriptor) will be 
the property name, and the value should be the battery level. i'm not 
too comfortable with it, and it is different than what we used to do up 
until now.

So where do we go from here ?

>
> Johan
>


-- 
BR,
Chen Ganir
Texas Instruments

Re: [PATCH v4 01/10] Battery: Add Battery Service Gatt Client

[Johan Hedberg]

Hi Chen,

On Thu, Aug 16, 2012, Chen Ganir wrote:
> >>However, i fail to understand why the object manager prevents this
> >>from getting accepted - do you have a time estimation when the
> >>object manager should be active or available ?
> >
> >By the time of our next release (as per the content under the BlueZ 5
> >section in our TODO file).
> 
> Is there anywhere some documentation or instructions how to adapt
> the API for the new object manager ? Is it backward compatible to
> what we have today ? Do we really hold back DBUS additions until we
> change the DBUS API ?

As an unrelated thing (but since I saw it in your commit messages too):
please spell D-Bus as D-Bus, not DBUS, DBus, or anything else in natural
written language. This keeps it consistent with the rest of our
documentation as well as how the D-Bus project itself spells the name.

Regarding the Object Manager interface the implementation details you'd
get from Lucas and Luiz who are working on it whereas the API spec is
available here:

http://dbus.freedesktop.org/doc/dbus-specification.html#standard-interfaces-objectmanager

> >>I prefer this way of putting the batteries below the device, since it
> >>is obvious which battery belongs to each device.
> >
> >I fully agree with this. It's not what I had an issue with. However, if
> >we really want to make this clear you could additionally have a "Device"
> >property for each battery object pointing back to the parent object.
> I can do that, if it helps. Although it is not necessary, since the
> object path for a battery already contains the device objec path in
> it :
> [variable prefix]/{hci0,..}/dev_XX_XX_XX_XX_XX_XX/BATT-NN-DDDD
> Adding a device object path as a property will be redundant.

Even though we do have such a hierarchical construction of the paths I
don't think we should encourage this kind of magic tricks in trying look
into actual values of object paths to infer their relation to each
other. However, since the battery object paths would be discovered
through the device object there might not be any need for such a
backwards reference.

> >>The other option i can think of is to have another interface
> >>registered on the device object path:
> >>
> >>Service		org.bluez
> >>Interface	org.bluez.Batteries
> >>Object path	[variable prefix]/{hci0,..}/dev_XX_XX_XX_XX_XX_XX
> >>
> >>Methods	dict GetProperties()
> >>
> >>		Returns all properties for the interface. See the
> >>		Properties section for the available properties.
> >>
> >>Signals		ProperyChanged(string name, variant value)
> >>
> >>		This signal indicates a changed value of the given
> >>		property.
> >>
> >>Properties	array{object} Batteries [readonly]
> >>
> >>		List of device battery object paths that represents the available
> >>batteries on the remote devices.
> >>
> >>
> >>Service		org.bluez
> >>Interface	org.bluez.Battery
> >>Object path	[variable prefix]/{hci0,..}/dev_XX_XX_XX_XX_XX_XX/BATT-NN-DDDD
> >>
> >>
> >>Methods	dict GetProperties()
> >>
> >>		Returns all properties for the interface. See the
> >>		Properties section for the available properties.
> >>
> >>Signals		PropertyChanged(string name, variant value)
> >>
> >>		This signal indicates a changed value of the given
> >>		property.
> >>
> >>Properties	byte Level [readonly]
> >>
> >>			Battery level (0-100).
> >>
> >>Any other suggestion ?
> >
> >That would work, although both of these interfaces are rather
> >light-weight in that they only contain a single property which begs the
> >question whether this is a bit over-kill.
> I know. I believe so as well, but if you want to separate the
> interfaces, it is a must.
> 
> >
> >Another option is to do away with the per-battery object somehow and
> >expose all batteries through a single interface. I'm not so sure that's
> >a good idea though since it could prevent future extensibility of the
> >API and doesn't reuse the common tools we have for object-like
> >abstractions (which a battery certainly is).
> If we do what you suggest, a battery name (namespace+descriptor)
> will be the property name, and the value should be the battery
> level. i'm not too comfortable with it, and it is different than
> what we used to do up until now.
> 
> So where do we go from here ?

Some extra opinions/view points could help. Maybe Marcel or Luiz have
some preferences/ideas on how to do this.

Johan

Re: [PATCH v4 01/10] Battery: Add Battery Service Gatt Client

[Luiz Augusto von Dentz]

Hi Chen, Johan,

On Thu, Aug 16, 2012 at 10:24 PM, Johan Hedberg <johan.hedberg@gmail.com> wrote:
> Hi Chen,
>
> On Thu, Aug 16, 2012, Chen Ganir wrote:
>> >>However, i fail to understand why the object manager prevents this
>> >>from getting accepted - do you have a time estimation when the
>> >>object manager should be active or available ?
>> >
>> >By the time of our next release (as per the content under the BlueZ 5
>> >section in our TODO file).
>>
>> Is there anywhere some documentation or instructions how to adapt
>> the API for the new object manager ? Is it backward compatible to
>> what we have today ? Do we really hold back DBUS additions until we
>> change the DBUS API ?
>
> As an unrelated thing (but since I saw it in your commit messages too):
> please spell D-Bus as D-Bus, not DBUS, DBus, or anything else in natural
> written language. This keeps it consistent with the rest of our
> documentation as well as how the D-Bus project itself spells the name.
>
> Regarding the Object Manager interface the implementation details you'd
> get from Lucas and Luiz who are working on it whereas the API spec is
> available here:
>
> http://dbus.freedesktop.org/doc/dbus-specification.html#standard-interfaces-objectmanager

With object manager we have to keep 2 things in mind while designing
new interfaces:

1. In case of returning new objects, as in CreateDevice, always
include the interfaces and properties e.g. D-Bus signature oa{sa{sv}}
(we will probably have a gdbus helper to do that) which is the
equivalent to InterfacesAdded signal. This avoid having to call
GetManagedObjects.
2. No need to keep a list of objects or emit signals when one is
created/destroyed, gdbus will take care of this automatically.

Now if this is really relevant here Im not sure.

>> >>I prefer this way of putting the batteries below the device, since it
>> >>is obvious which battery belongs to each device.
>> >
>> >I fully agree with this. It's not what I had an issue with. However, if
>> >we really want to make this clear you could additionally have a "Device"
>> >property for each battery object pointing back to the parent object.
>> I can do that, if it helps. Although it is not necessary, since the
>> object path for a battery already contains the device objec path in
>> it :
>> [variable prefix]/{hci0,..}/dev_XX_XX_XX_XX_XX_XX/BATT-NN-DDDD
>> Adding a device object path as a property will be redundant.
>
> Even though we do have such a hierarchical construction of the paths I
> don't think we should encourage this kind of magic tricks in trying look
> into actual values of object paths to infer their relation to each
> other. However, since the battery object paths would be discovered
> through the device object there might not be any need for such a
> backwards reference.
>
>> >>The other option i can think of is to have another interface
>> >>registered on the device object path:
>> >>
>> >>Service             org.bluez
>> >>Interface   org.bluez.Batteries
>> >>Object path [variable prefix]/{hci0,..}/dev_XX_XX_XX_XX_XX_XX
>> >>
>> >>Methods     dict GetProperties()
>> >>
>> >>            Returns all properties for the interface. See the
>> >>            Properties section for the available properties.
>> >>
>> >>Signals             ProperyChanged(string name, variant value)
>> >>
>> >>            This signal indicates a changed value of the given
>> >>            property.
>> >>
>> >>Properties  array{object} Batteries [readonly]
>> >>
>> >>            List of device battery object paths that represents the available
>> >>batteries on the remote devices.
>> >>
>> >>
>> >>Service             org.bluez
>> >>Interface   org.bluez.Battery
>> >>Object path [variable prefix]/{hci0,..}/dev_XX_XX_XX_XX_XX_XX/BATT-NN-DDDD
>> >>
>> >>
>> >>Methods     dict GetProperties()
>> >>
>> >>            Returns all properties for the interface. See the
>> >>            Properties section for the available properties.
>> >>
>> >>Signals             PropertyChanged(string name, variant value)
>> >>
>> >>            This signal indicates a changed value of the given
>> >>            property.
>> >>
>> >>Properties  byte Level [readonly]
>> >>
>> >>                    Battery level (0-100).
>> >>
>> >>Any other suggestion ?
>> >
>> >That would work, although both of these interfaces are rather
>> >light-weight in that they only contain a single property which begs the
>> >question whether this is a bit over-kill.
>> I know. I believe so as well, but if you want to separate the
>> interfaces, it is a must.
>>
>> >
>> >Another option is to do away with the per-battery object somehow and
>> >expose all batteries through a single interface. I'm not so sure that's
>> >a good idea though since it could prevent future extensibility of the
>> >API and doesn't reuse the common tools we have for object-like
>> >abstractions (which a battery certainly is).
>> If we do what you suggest, a battery name (namespace+descriptor)
>> will be the property name, and the value should be the battery
>> level. i'm not too comfortable with it, and it is different than
>> what we used to do up until now.
>>
>> So where do we go from here ?
>
> Some extra opinions/view points could help. Maybe Marcel or Luiz have
> some preferences/ideas on how to do this.

Im trying to think how having multiple objects could be useful for the
UI, most likely it only gonna show a single meter aggregating all the
batteries.

Another thing that worth adding is that there are other profiles that
do support battery status such as HFP and AVRCP, so I think this
should be made generic enough so other sources could be supported.

-- 
Luiz Augusto von Dentz

Re: [PATCH v4 01/10] Battery: Add Battery Service Gatt Client

[Chen Ganir]

Luiz,

On 08/20/2012 04:03 PM, Luiz Augusto von Dentz wrote:
> Hi Chen, Johan,
>
> On Thu, Aug 16, 2012 at 10:24 PM, Johan Hedberg <johan.hedberg@gmail.com> wrote:
>> Hi Chen,
>>
>> On Thu, Aug 16, 2012, Chen Ganir wrote:
>>>>> However, i fail to understand why the object manager prevents this
>>>> >from getting accepted - do you have a time estimation when the
>>>>> object manager should be active or available ?
>>>>
>>>> By the time of our next release (as per the content under the BlueZ 5
>>>> section in our TODO file).
>>>
>>> Is there anywhere some documentation or instructions how to adapt
>>> the API for the new object manager ? Is it backward compatible to
>>> what we have today ? Do we really hold back DBUS additions until we
>>> change the DBUS API ?
>>
>> As an unrelated thing (but since I saw it in your commit messages too):
>> please spell D-Bus as D-Bus, not DBUS, DBus, or anything else in natural
>> written language. This keeps it consistent with the rest of our
>> documentation as well as how the D-Bus project itself spells the name.
>>
>> Regarding the Object Manager interface the implementation details you'd
>> get from Lucas and Luiz who are working on it whereas the API spec is
>> available here:
>>
>> http://dbus.freedesktop.org/doc/dbus-specification.html#standard-interfaces-objectmanager
>
> With object manager we have to keep 2 things in mind while designing
> new interfaces:
>
> 1. In case of returning new objects, as in CreateDevice, always
> include the interfaces and properties e.g. D-Bus signature oa{sa{sv}}
> (we will probably have a gdbus helper to do that) which is the
> equivalent to InterfacesAdded signal. This avoid having to call
> GetManagedObjects.
> 2. No need to keep a list of objects or emit signals when one is
> created/destroyed, gdbus will take care of this automatically.
>
> Now if this is really relevant here Im not sure.
>
Is there any current implementation of this object manager in the bluez 
tree ? Is it already implemented ?

>>>>> I prefer this way of putting the batteries below the device, since it
>>>>> is obvious which battery belongs to each device.
>>>>
>>>> I fully agree with this. It's not what I had an issue with. However, if
>>>> we really want to make this clear you could additionally have a "Device"
>>>> property for each battery object pointing back to the parent object.
>>> I can do that, if it helps. Although it is not necessary, since the
>>> object path for a battery already contains the device objec path in
>>> it :
>>> [variable prefix]/{hci0,..}/dev_XX_XX_XX_XX_XX_XX/BATT-NN-DDDD
>>> Adding a device object path as a property will be redundant.
>>
>> Even though we do have such a hierarchical construction of the paths I
>> don't think we should encourage this kind of magic tricks in trying look
>> into actual values of object paths to infer their relation to each
>> other. However, since the battery object paths would be discovered
>> through the device object there might not be any need for such a
>> backwards reference.
>>
>>>>> The other option i can think of is to have another interface
>>>>> registered on the device object path:
>>>>>
>>>>> Service             org.bluez
>>>>> Interface   org.bluez.Batteries
>>>>> Object path [variable prefix]/{hci0,..}/dev_XX_XX_XX_XX_XX_XX
>>>>>
>>>>> Methods     dict GetProperties()
>>>>>
>>>>>             Returns all properties for the interface. See the
>>>>>             Properties section for the available properties.
>>>>>
>>>>> Signals             ProperyChanged(string name, variant value)
>>>>>
>>>>>             This signal indicates a changed value of the given
>>>>>             property.
>>>>>
>>>>> Properties  array{object} Batteries [readonly]
>>>>>
>>>>>             List of device battery object paths that represents the available
>>>>> batteries on the remote devices.
>>>>>
>>>>>
>>>>> Service             org.bluez
>>>>> Interface   org.bluez.Battery
>>>>> Object path [variable prefix]/{hci0,..}/dev_XX_XX_XX_XX_XX_XX/BATT-NN-DDDD
>>>>>
>>>>>
>>>>> Methods     dict GetProperties()
>>>>>
>>>>>             Returns all properties for the interface. See the
>>>>>             Properties section for the available properties.
>>>>>
>>>>> Signals             PropertyChanged(string name, variant value)
>>>>>
>>>>>             This signal indicates a changed value of the given
>>>>>             property.
>>>>>
>>>>> Properties  byte Level [readonly]
>>>>>
>>>>>                     Battery level (0-100).
>>>>>
>>>>> Any other suggestion ?
>>>>
>>>> That would work, although both of these interfaces are rather
>>>> light-weight in that they only contain a single property which begs the
>>>> question whether this is a bit over-kill.
>>> I know. I believe so as well, but if you want to separate the
>>> interfaces, it is a must.
>>>
>>>>
>>>> Another option is to do away with the per-battery object somehow and
>>>> expose all batteries through a single interface. I'm not so sure that's
>>>> a good idea though since it could prevent future extensibility of the
>>>> API and doesn't reuse the common tools we have for object-like
>>>> abstractions (which a battery certainly is).
>>> If we do what you suggest, a battery name (namespace+descriptor)
>>> will be the property name, and the value should be the battery
>>> level. i'm not too comfortable with it, and it is different than
>>> what we used to do up until now.
>>>
>>> So where do we go from here ?
>>
>> Some extra opinions/view points could help. Maybe Marcel or Luiz have
>> some preferences/ideas on how to do this.
>
> Im trying to think how having multiple objects could be useful for the
> UI, most likely it only gonna show a single meter aggregating all the
> batteries.
>
So what do you suggest here ? Calculating an average ? How shoudl it be 
done ? If 2 batteries are available, first is 100% and the second is 
50%, we should simply set the value as 75%? I'm not so sure that we 
should make such decisions for the end user.

> Another thing that worth adding is that there are other profiles that
> do support battery status such as HFP and AVRCP, so I think this
> should be made generic enough so other sources could be supported.
>
The internal device API uses the device_add_battery(...) and 
device_remove_battery(...) to allow adding/removing batteries to the 
device battery list, but it is the responsibility of the profile to 
register a D-Bus interface, and update.

I could redesign this, to add a generic battery API, which will expose a 
new API, such as battery_add(battery_struct* batt), 
battery_remove(battery_struct* batt) and battery_update(battery_struct* 
batt) which will allow a more generic approach. This Battery module will 
be responsible for registering/unregistering the D-Bus API, and profiles 
which need to use it will simply use the exposed API to 
add/remove/update. The batt_structure will also include some callback 
functions to be called when a value is queried for example, or if a 
device is removed. The LE Battery plugin will use the GATT to 
read/write/receive notification, and use the Generic Battery interface 
to interface with the external world. What do you think about it ?


BR,
Chen Ganir

Re: [PATCH v4 01/10] Battery: Add Battery Service Gatt Client

[Johan Hedberg]

Hi Chen,

On Sun, Aug 26, 2012, Chen Ganir wrote:
> So what do you suggest here ? Calculating an average ? How shoudl it
> be done ? If 2 batteries are available, first is 100% and the second
> is 50%, we should simply set the value as 75%? I'm not so sure that we
> should make such decisions for the end user.
> 
> >Another thing that worth adding is that there are other profiles that
> >do support battery status such as HFP and AVRCP, so I think this
> >should be made generic enough so other sources could be supported.
> 
> The internal device API uses the device_add_battery(...) and
> device_remove_battery(...) to allow adding/removing batteries to the
> device battery list, but it is the responsibility of the profile to
> register a D-Bus interface, and update.
> 
> I could redesign this, to add a generic battery API, which will
> expose a new API, such as battery_add(battery_struct* batt),
> battery_remove(battery_struct* batt) and
> battery_update(battery_struct* batt) which will allow a more generic
> approach. This Battery module will be responsible for
> registering/unregistering the D-Bus API, and profiles which need to
> use it will simply use the exposed API to add/remove/update. The
> batt_structure will also include some callback functions to be
> called when a value is queried for example, or if a device is
> removed. The LE Battery plugin will use the GATT to
> read/write/receive notification, and use the Generic Battery
> interface to interface with the external world. What do you think
> about it ?

I had a brief chat with Marcel about this and the following were the
main conclusions:

1. It'd be nice to have support for this added to UPower so that it can
enumerate Bluetooth batteries.

2. The easiest way to do 1. would be to have this one D-Bus object per
battery.

3. Use uint16 instead of byte to keep our D-BUs API consistent wrt.
unsigned integers (byte is signed).

4. No Adapter property or interface needed once we've merged the object
manager patches.

5. (as you proposed) we'll probably want/need a src/device.c API for
device drivers to register batteries. Other profiles supporting battery
info (AVRCP, HFP, etc) could use the same API.

Johan