[PATCH v2 0/9] Add GATT Client Battery Service

[PATCH v2 0/9] Add GATT Client Battery Service

[Chen Ganir]

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 2 of this patch set, rebased on top of the latest sources and 
fixes some issues found during testing and in the ML comments.

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

 Makefile.am                          |   10 +-
 doc/battery-api.txt                  |   38 +++
 doc/device-api.txt                   |    5 +
 profiles/batterystate/batterystate.c |  518 ++++++++++++++++++++++++++++++++++
 profiles/batterystate/batterystate.h |   24 ++
 profiles/batterystate/main.c         |   67 +++++
 profiles/batterystate/manager.c      |   93 ++++++
 profiles/batterystate/manager.h      |   24 ++
 src/device.c                         |   66 +++++
 src/device.h                         |    3 +
 10 files changed, 846 insertions(+), 2 deletions(-)
 create mode 100644 doc/battery-api.txt
 create mode 100644 profiles/batterystate/batterystate.c
 create mode 100644 profiles/batterystate/batterystate.h
 create mode 100644 profiles/batterystate/main.c
 create mode 100644 profiles/batterystate/manager.c
 create mode 100644 profiles/batterystate/manager.h

-- 
1.7.9.5

[PATCH v2 1/9] Battery: Add Battery Service GATT Client

[Chen Ganir]

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

diff --git a/Makefile.am b/Makefile.am
index 45a811c..e698718 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 \
+            batterystate
 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/batterystate/main.c \
+			profiles/batterystate/manager.c \
+			profiles/batterystate/manager.h \
+			profiles/batterystate/batterystate.c \
+			profiles/batterystate/batterystate.h
 endif
 
 builtin_modules += formfactor
diff --git a/profiles/batterystate/batterystate.c b/profiles/batterystate/batterystate.c
new file mode 100644
index 0000000..04c2e5e
--- /dev/null
+++ b/profiles/batterystate/batterystate.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 "batterystate.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 batterystate_free(gpointer user_data)
+{
+	struct battery *batt = user_data;
+
+	btd_device_unref(batt->dev);
+	g_free(batt);
+}
+
+
+int batterystate_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 batterystate_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);
+
+	batterystate_free(batt);
+}
diff --git a/profiles/batterystate/batterystate.h b/profiles/batterystate/batterystate.h
new file mode 100644
index 0000000..9aedae7
--- /dev/null
+++ b/profiles/batterystate/batterystate.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 batterystate_register(struct btd_device *device);
+void batterystate_unregister(struct btd_device *device);
diff --git a/profiles/batterystate/main.c b/profiles/batterystate/main.c
new file mode 100644
index 0000000..360254b
--- /dev/null
+++ b/profiles/batterystate/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 batterystate_init(void)
+{
+	if (!main_opts.gatt_enabled) {
+		DBG("GATT is disabled");
+		return -ENOTSUP;
+	}
+
+	return batterystate_manager_init();
+}
+
+static void batterystate_exit(void)
+{
+	batterystate_manager_exit();
+}
+
+BLUETOOTH_PLUGIN_DEFINE(batterystate, VERSION,
+			BLUETOOTH_PLUGIN_PRIORITY_DEFAULT, batterystate_init,
+			batterystate_exit)
diff --git a/profiles/batterystate/manager.c b/profiles/batterystate/manager.c
new file mode 100644
index 0000000..6718acf
--- /dev/null
+++ b/profiles/batterystate/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 "batterystate.h"
+#include "manager.h"
+
+#define BATTERY_SERVICE_UUID		"0000180f-0000-1000-8000-00805f9b34fb"
+
+static int batterystate_driver_probe(struct btd_device *device, GSList *uuids)
+{
+	return batterystate_register(device);
+}
+
+static void batterystate_driver_remove(struct btd_device *device)
+{
+	batterystate_unregister(device);
+}
+
+static struct btd_device_driver battery_device_driver = {
+	.name	= "battery-driver",
+	.uuids	= BTD_UUIDS(BATTERY_SERVICE_UUID),
+	.probe	= batterystate_driver_probe,
+	.remove	= batterystate_driver_remove
+};
+
+int batterystate_manager_init(void)
+{
+	return btd_register_device_driver(&battery_device_driver);
+}
+
+void batterystate_manager_exit(void)
+{
+	btd_unregister_device_driver(&battery_device_driver);
+}
diff --git a/profiles/batterystate/manager.h b/profiles/batterystate/manager.h
new file mode 100644
index 0000000..7f0bf15
--- /dev/null
+++ b/profiles/batterystate/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 batterystate_manager_init(void);
+void batterystate_manager_exit(void);
-- 
1.7.9.5

[PATCH v2 2/9] Battery: Add connection logic

[Chen Ganir]

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/batterystate/batterystate.c |   78 +++++++++++++++++++++++++++++++++-
 profiles/batterystate/batterystate.h |    3 +-
 profiles/batterystate/manager.c      |   22 +++++++++-
 3 files changed, 99 insertions(+), 4 deletions(-)

diff --git a/profiles/batterystate/batterystate.c b/profiles/batterystate/batterystate.c
index 04c2e5e..40663f6 100644
--- a/profiles/batterystate/batterystate.c
+++ b/profiles/batterystate/batterystate.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 "batterystate.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,84 @@ static void batterystate_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_batterystate_cb(GSList *characteristics, guint8 status,
+							gpointer user_data)
+{
+	struct battery *batt = user_data;
+	GSList *l;
+
+	if (status != 0) {
+		error("Discover batterystate characteristics: %s",
+							att_ecode2str(status));
+		return;
+	}
 
-int batterystate_register(struct btd_device *device)
+	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_batterystate_cb, batt);
+	}
+}
+
+static void attio_disconnected_cb(gpointer user_data)
+{
+	struct battery *batt = user_data;
+
+	g_attrib_unref(batt->attrib);
+	batt->attrib = NULL;
+}
+
+int batterystate_register(struct btd_device *device,
+				struct gatt_primary *prim)
 {
 	struct battery *batt;
 
 	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/batterystate/batterystate.h b/profiles/batterystate/batterystate.h
index 9aedae7..2d30028 100644
--- a/profiles/batterystate/batterystate.h
+++ b/profiles/batterystate/batterystate.h
@@ -19,6 +19,5 @@
  *  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
  *
  */
-
-int batterystate_register(struct btd_device *device);
+int batterystate_register(struct btd_device *device, struct gatt_primary *prim);
 void batterystate_unregister(struct btd_device *device);
diff --git a/profiles/batterystate/manager.c b/profiles/batterystate/manager.c
index 6718acf..62076ac 100644
--- a/profiles/batterystate/manager.c
+++ b/profiles/batterystate/manager.c
@@ -34,9 +34,29 @@
 
 #define BATTERY_SERVICE_UUID		"0000180f-0000-1000-8000-00805f9b34fb"
 
+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);
+}
+
 static int batterystate_driver_probe(struct btd_device *device, GSList *uuids)
 {
-	return batterystate_register(device);
+	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);
+	if (l == NULL)
+		return -EINVAL;
+
+	prim = l->data;
+
+	return batterystate_register(device, prim);
 }
 
 static void batterystate_driver_remove(struct btd_device *device)
-- 
1.7.9.5

[PATCH v2 3/9] Battery: Discover Characteristic Descriptors

[Chen Ganir]

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

diff --git a/profiles/batterystate/batterystate.c b/profiles/batterystate/batterystate.c
index 40663f6..a7d2f6e 100644
--- a/profiles/batterystate/batterystate.c
+++ b/profiles/batterystate/batterystate.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,6 +87,45 @@ static void batterystate_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_batterystate_cb(GSList *characteristics, guint8 status,
 							gpointer user_data)
 {
@@ -95,6 +141,7 @@ static void configure_batterystate_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 +151,20 @@ static void configure_batterystate_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 v2 4/9] Battery: Get Battery ID

[Chen Ganir]

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

diff --git a/profiles/batterystate/batterystate.c b/profiles/batterystate/batterystate.c
index a7d2f6e..d3a1974 100644
--- a/profiles/batterystate/batterystate.c
+++ b/profiles/batterystate/batterystate.c
@@ -37,6 +37,8 @@
 #include "batterystate.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,18 @@ 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 */
+	uint8_t			level;		/* Battery last known level */
 };
 
 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 +92,55 @@ static void batterystate_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 +174,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);
@@ -140,31 +195,35 @@ static void configure_batterystate_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 v2 5/9] Battery: Add Battery list to btd_device

[Chen Ganir]

Add peer battery list to the btd_device. New property added to Device
called Batteries.
---
 doc/device-api.txt                   |    5 +++
 profiles/batterystate/batterystate.c |    6 ++++
 src/device.c                         |   66 ++++++++++++++++++++++++++++++++++
 src/device.h                         |    3 ++
 4 files changed, 80 insertions(+)

diff --git a/doc/device-api.txt b/doc/device-api.txt
index 3b84033..3d19a53 100644
--- a/doc/device-api.txt
+++ b/doc/device-api.txt
@@ -175,3 +175,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.
diff --git a/profiles/batterystate/batterystate.c b/profiles/batterystate/batterystate.c
index d3a1974..23dfab5 100644
--- a/profiles/batterystate/batterystate.c
+++ b/profiles/batterystate/batterystate.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 */
@@ -206,6 +207,11 @@ static void configure_batterystate_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;
+			ch->path = g_strdup_printf("%s/BATT%04X",
+						device_get_path(batt->dev),
+						c->handle);
+
+			device_add_battery(batt->dev, ch->path);
 
 			batt->chars = g_slist_append(batt->chars, ch);
 
diff --git a/src/device.c b/src/device.c
index cd571f7..98e431a 100644
--- a/src/device.c
+++ b/src/device.c
@@ -127,6 +127,10 @@ struct att_callbacks {
 	gpointer user_data;
 };
 
+struct btd_battery {
+	char *path;
+};
+
 struct btd_device {
 	bdaddr_t	bdaddr;
 	uint8_t		bdaddr_type;
@@ -172,6 +176,7 @@ struct btd_device {
 
 	GIOChannel      *att_io;
 	guint		cleanup_id;
+	GSList		*batteries;
 };
 
 static uint16_t uuid_list[] = {
@@ -262,6 +267,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;
@@ -1219,6 +1234,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);
@@ -3141,3 +3159,51 @@ 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);
+			return;
+		}
+	}
+
+	batteries_changed(device);
+}
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 v2 6/9] Battery: Add Battery D-BUS API

[Chen Ganir]

Add Battery level specific API
---
 doc/battery-api.txt                  |   33 +++++++++++++
 profiles/batterystate/batterystate.c |   89 ++++++++++++++++++++++++++++++++--
 profiles/batterystate/batterystate.h |    3 +-
 profiles/batterystate/main.c         |   18 ++++++-
 profiles/batterystate/manager.c      |   19 ++++++--
 profiles/batterystate/manager.h      |    2 +-
 6 files changed, 151 insertions(+), 13 deletions(-)
 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..5d7510d
--- /dev/null
+++ b/doc/battery-api.txt
@@ -0,0 +1,33 @@
+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,hci1,...}/dev_XX_XX_XX_XX_XX_XX/BATTYYYY
+
+
+Methods	dict GetProperties()
+
+			Returns all properties for the interface. See the
+			Properties section for the available properties.
+
+Properties	byte Namespace [readonly]
+
+			Namespace value from the battery format characteristic
+			descriptor.Combined with Description provides a unique
+			battery identifyer if multiple batteries are supported.
+
+		uint16 Description [readonly]
+
+			Description value from the battery format characteristic
+			descriptor. Combined with Namespace provides a unique
+			battery identifyer if multiple batteries are supported.
+
+		byte Level [readonly]
+
+			Battery level (0-100).
diff --git a/profiles/batterystate/batterystate.c b/profiles/batterystate/batterystate.c
index 23dfab5..a9b2414 100644
--- a/profiles/batterystate/batterystate.c
+++ b/profiles/batterystate/batterystate.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 "batterystate.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 batterystate_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 batterystate_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);
 }
 
@@ -181,6 +212,44 @@ static void discover_desc_cb(guint8 status, const guint8 *pdu, guint16 len,
 	att_data_list_free(list);
 }
 
+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, "Namespace", DBUS_TYPE_BYTE, &c->ns);
+
+	dict_append_entry(&dict, "Description", DBUS_TYPE_UINT16,
+							&c->description);
+
+	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 configure_batterystate_cb(GSList *characteristics, guint8 status,
 							gpointer user_data)
@@ -213,6 +282,15 @@ static void configure_batterystate_cb(GSList *characteristics, guint8 status,
 
 			device_add_battery(batt->dev, ch->path);
 
+			if (!g_dbus_register_interface(batt->conn, ch->path,
+						BATTERY_INTERFACE,
+						battery_methods, NULL, NULL,
+						ch, NULL)) {
+				error("D-Bus register interface %s failed",
+							BATTERY_INTERFACE);
+				continue;
+			}
+
 			batt->chars = g_slist_append(batt->chars, ch);
 
 			start = c->value_handle + 1;
@@ -254,14 +332,14 @@ static void attio_disconnected_cb(gpointer user_data)
 	batt->attrib = NULL;
 }
 
-int batterystate_register(struct btd_device *device,
-				struct gatt_primary *prim)
+int batterystate_register(DBusConnection *connection, struct btd_device *device,
+						struct gatt_primary *prim)
 {
 	struct battery *batt;
 
 	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;
@@ -271,6 +349,7 @@ int batterystate_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/batterystate/batterystate.h b/profiles/batterystate/batterystate.h
index 2d30028..63b55bc 100644
--- a/profiles/batterystate/batterystate.h
+++ b/profiles/batterystate/batterystate.h
@@ -19,5 +19,6 @@
  *  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
  *
  */
-int batterystate_register(struct btd_device *device, struct gatt_primary *prim);
+int batterystate_register(DBusConnection *conn, struct btd_device *device,
+						struct gatt_primary *prim);
 void batterystate_unregister(struct btd_device *device);
diff --git a/profiles/batterystate/main.c b/profiles/batterystate/main.c
index 360254b..a41952d 100644
--- a/profiles/batterystate/main.c
+++ b/profiles/batterystate/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 batterystate_init(void)
 {
 	if (!main_opts.gatt_enabled) {
@@ -40,12 +42,24 @@ static int batterystate_init(void)
 		return -ENOTSUP;
 	}
 
-	return batterystate_manager_init();
+	connection = dbus_bus_get(DBUS_BUS_SYSTEM, NULL);
+	if (connection == NULL)
+		return -EIO;
+
+	if (batterystate_manager_init(connection) < 0) {
+		dbus_connection_unref(connection);
+		return -EIO;
+	}
+
+	return 0;
 }
 
 static void batterystate_exit(void)
 {
 	batterystate_manager_exit();
+
+	dbus_connection_unref(connection);
+	connection = NULL;
 }
 
 BLUETOOTH_PLUGIN_DEFINE(batterystate, VERSION,
diff --git a/profiles/batterystate/manager.c b/profiles/batterystate/manager.c
index 62076ac..5e52b16 100644
--- a/profiles/batterystate/manager.c
+++ b/profiles/batterystate/manager.c
@@ -20,7 +20,7 @@
  *
  */
 
-#include <glib.h>
+#include <gdbus.h>
 #include <errno.h>
 #include <bluetooth/uuid.h>
 
@@ -34,6 +34,8 @@
 
 #define BATTERY_SERVICE_UUID		"0000180f-0000-1000-8000-00805f9b34fb"
 
+static DBusConnection *connection;
+
 static gint primary_uuid_cmp(gconstpointer a, gconstpointer b)
 {
 	const struct gatt_primary *prim = a;
@@ -56,7 +58,7 @@ static int batterystate_driver_probe(struct btd_device *device, GSList *uuids)
 
 	prim = l->data;
 
-	return batterystate_register(device, prim);
+	return batterystate_register(connection, device, prim);
 }
 
 static void batterystate_driver_remove(struct btd_device *device)
@@ -71,12 +73,21 @@ static struct btd_device_driver battery_device_driver = {
 	.remove	= batterystate_driver_remove
 };
 
-int batterystate_manager_init(void)
+int batterystate_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 batterystate_manager_exit(void)
 {
 	btd_unregister_device_driver(&battery_device_driver);
+
+	dbus_connection_unref(connection);
+	connection = NULL;
 }
diff --git a/profiles/batterystate/manager.h b/profiles/batterystate/manager.h
index 7f0bf15..9f99e70 100644
--- a/profiles/batterystate/manager.h
+++ b/profiles/batterystate/manager.h
@@ -20,5 +20,5 @@
  *
  */
 
-int batterystate_manager_init(void);
+int batterystate_manager_init(DBusConnection *conn);
 void batterystate_manager_exit(void);
-- 
1.7.9.5

[PATCH v2 7/9] Battery: Read Battery level characteristic

[Chen Ganir]

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

diff --git a/profiles/batterystate/batterystate.c b/profiles/batterystate/batterystate.c
index a9b2414..e5b68a9 100644
--- a/profiles/batterystate/batterystate.c
+++ b/profiles/batterystate/batterystate.c
@@ -124,6 +124,40 @@ static void batterystate_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)
@@ -291,6 +325,7 @@ static void configure_batterystate_cb(GSList *characteristics, guint8 status,
 				continue;
 			}
 
+			process_batteryservice_char(ch);
 			batt->chars = g_slist_append(batt->chars, ch);
 
 			start = c->value_handle + 1;
@@ -321,6 +356,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_batterystate_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 v2 8/9] Battery: Add support for notifications

[Chen Ganir]

Add support for emitting PropertyChanged when a battery level
characteristic notification is sent from the peer device.
---
 doc/battery-api.txt                  |    5 ++
 profiles/batterystate/batterystate.c |  107 +++++++++++++++++++++++++++++++++-
 2 files changed, 111 insertions(+), 1 deletion(-)

diff --git a/doc/battery-api.txt b/doc/battery-api.txt
index 5d7510d..f31e7e5 100644
--- a/doc/battery-api.txt
+++ b/doc/battery-api.txt
@@ -16,6 +16,11 @@ 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 Namespace [readonly]
 
 			Namespace value from the battery format characteristic
diff --git a/profiles/batterystate/batterystate.c b/profiles/batterystate/batterystate.c
index e5b68a9..718863b 100644
--- a/profiles/batterystate/batterystate.c
+++ b/profiles/batterystate/batterystate.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 */
 };
 
@@ -104,6 +105,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 batterystate_free(gpointer user_data)
 {
 	struct battery *batt = user_data;
@@ -117,6 +126,10 @@ static void batterystate_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 +171,17 @@ static void process_batteryservice_char(struct characteristic *ch)
 	}
 }
 
+static void batterylevel_enable_notify_cb(guint8 status, const guint8 *pdu,
+						guint16 len, gpointer user_data)
+{
+	char *msg = user_data;
+
+	if (status != 0)
+		error("Could not enable battery level notification: %s", msg);
+
+	g_free(msg);
+}
+
 static void batterylevel_presentation_format_desc_cb(guint8 status,
 						const guint8 *pdu, guint16 len,
 						gpointer user_data)
@@ -194,6 +218,23 @@ 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;
+		char *msg;
+
+		val = GATT_CLIENT_CHARAC_CFG_NOTIF_BIT;
+		msg = g_strdup("Enable BatteryLevel notification");
+
+		att_put_u16(val, atval);
+		gatt_write_char(ch->batt->attrib, desc->handle, atval, 2,
+					batterylevel_enable_notify_cb, msg);
+		return;
+	}
+
 	bt_uuid16_create(&btuuid, GATT_CHARAC_FMT_UUID);
 
 	if (bt_uuid_cmp(&desc->uuid, &btuuid) == 0) {
@@ -277,6 +318,12 @@ static DBusMessage *get_properties(DBusConnection *conn, DBusMessage *msg,
 	return reply;
 }
 
+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 GDBusMethodTable battery_methods[] = {
 	{ GDBUS_METHOD("GetProperties",
 				NULL, GDBUS_ARGS({ "properties", "a{sv}" }),
@@ -284,6 +331,11 @@ static GDBusMethodTable battery_methods[] = {
 	{ }
 };
 
+static GDBusSignalTable battery_signals[] = {
+	{ GDBUS_SIGNAL("PropertyChanged",
+		GDBUS_ARGS({ "name", "s" }, { "value", "v" })) },
+	{ }
+};
 
 static void configure_batterystate_cb(GSList *characteristics, guint8 status,
 							gpointer user_data)
@@ -318,7 +370,7 @@ static void configure_batterystate_cb(GSList *characteristics, guint8 status,
 
 			if (!g_dbus_register_interface(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);
@@ -346,12 +398,63 @@ static void configure_batterystate_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,
@@ -369,6 +472,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

[PATCH v2 9/9] Battery: Emit property changed on first read

[Chen Ganir]

Emit battery level property changed upon connection, on first read.
---
 profiles/batterystate/batterystate.c |    3 +++
 1 file changed, 3 insertions(+)

diff --git a/profiles/batterystate/batterystate.c b/profiles/batterystate/batterystate.c
index 718863b..4300f85 100644
--- a/profiles/batterystate/batterystate.c
+++ b/profiles/batterystate/batterystate.c
@@ -72,6 +72,8 @@ struct descriptor {
 	bt_uuid_t		uuid;		/* UUID */
 };
 
+static void emit_battery_level_changed(struct characteristic *c);
+
 static void char_free(gpointer user_data)
 {
 	struct characteristic *c = user_data;
@@ -161,6 +163,7 @@ static void read_batterylevel_cb(guint8 status, const guint8 *pdu, guint16 len,
 	}
 
 	ch->level = value[0];
+	emit_battery_level_changed(ch);
 }
 
 static void process_batteryservice_char(struct characteristic *ch)
-- 
1.7.9.5

Re: [PATCH v2 0/9] Add GATT Client Battery Service

[Chen Ganir]

On 07/25/2012 08:42 AM, Chen Ganir wrote:
> 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 2 of this patch set, rebased on top of the latest sources and
> fixes some issues found during testing and in the ML comments.
>
> Chen Ganir (9):
>    Battery: Add Battery Service GATT Client
>    Battery: Add connection logic
>    Battery: Discover Characteristic Descriptors
>    Battery: Get Battery ID
>    Battery: Add Battery list to btd_device
>    Battery: Add Battery D-BUS API
>    Battery: Read Battery level characteristic
>    Battery: Add support for notifications
>    Battery: Emit property changed on first read
>
>   Makefile.am                          |   10 +-
>   doc/battery-api.txt                  |   38 +++
>   doc/device-api.txt                   |    5 +
>   profiles/batterystate/batterystate.c |  518 ++++++++++++++++++++++++++++++++++
>   profiles/batterystate/batterystate.h |   24 ++
>   profiles/batterystate/main.c         |   67 +++++
>   profiles/batterystate/manager.c      |   93 ++++++
>   profiles/batterystate/manager.h      |   24 ++
>   src/device.c                         |   66 +++++
>   src/device.h                         |    3 +
>   10 files changed, 846 insertions(+), 2 deletions(-)
>   create mode 100644 doc/battery-api.txt
>   create mode 100644 profiles/batterystate/batterystate.c
>   create mode 100644 profiles/batterystate/batterystate.h
>   create mode 100644 profiles/batterystate/main.c
>   create mode 100644 profiles/batterystate/manager.c
>   create mode 100644 profiles/batterystate/manager.h
>

Ping anyone ? Did anyone get to review this ?

Thanks,
Chen Ganir

Re: [PATCH v2 0/9] Add GATT Client Battery Service

[Claudio Takahasi]

Hi Chen Ganir:

On Wed, Aug 1, 2012 at 3:40 AM, Chen Ganir <chen.ganir@ti.com> wrote:
> On 07/25/2012 08:42 AM, Chen Ganir wrote:
>>
>> 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 2 of this patch set, rebased on top of the latest sources
>> and
>> fixes some issues found during testing and in the ML comments.
>>
>> Chen Ganir (9):
>>    Battery: Add Battery Service GATT Client
>>    Battery: Add connection logic
>>    Battery: Discover Characteristic Descriptors
>>    Battery: Get Battery ID
>>    Battery: Add Battery list to btd_device
>>    Battery: Add Battery D-BUS API
>>    Battery: Read Battery level characteristic
>>    Battery: Add support for notifications
>>    Battery: Emit property changed on first read
>>
>>   Makefile.am                          |   10 +-
>>   doc/battery-api.txt                  |   38 +++
>>   doc/device-api.txt                   |    5 +
>>   profiles/batterystate/batterystate.c |  518
>> ++++++++++++++++++++++++++++++++++
>>   profiles/batterystate/batterystate.h |   24 ++
>>   profiles/batterystate/main.c         |   67 +++++
>>   profiles/batterystate/manager.c      |   93 ++++++
>>   profiles/batterystate/manager.h      |   24 ++
>>   src/device.c                         |   66 +++++
>>   src/device.h                         |    3 +
>>   10 files changed, 846 insertions(+), 2 deletions(-)
>>   create mode 100644 doc/battery-api.txt
>>   create mode 100644 profiles/batterystate/batterystate.c
>>   create mode 100644 profiles/batterystate/batterystate.h
>>   create mode 100644 profiles/batterystate/main.c
>>   create mode 100644 profiles/batterystate/manager.c
>>   create mode 100644 profiles/batterystate/manager.h
>>
>
> Ping anyone ? Did anyone get to review this ?
>
> Thanks,
> Chen Ganir


not yet.
We have an INTERNAL release in two weeks, next week we will send
comments in the ML.

BR,
Claudio

Re: [PATCH v2 0/9] Add GATT Client Battery Service

[Chen Ganir]

Claudio,

On 08/03/2012 03:57 PM, Claudio Takahasi wrote:
> Hi Chen Ganir:
>
> On Wed, Aug 1, 2012 at 3:40 AM, Chen Ganir <chen.ganir@ti.com> wrote:
>> On 07/25/2012 08:42 AM, Chen Ganir wrote:
>>>
>>> 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 2 of this patch set, rebased on top of the latest sources
>>> and
>>> fixes some issues found during testing and in the ML comments.
>>>
>>> Chen Ganir (9):
>>>     Battery: Add Battery Service GATT Client
>>>     Battery: Add connection logic
>>>     Battery: Discover Characteristic Descriptors
>>>     Battery: Get Battery ID
>>>     Battery: Add Battery list to btd_device
>>>     Battery: Add Battery D-BUS API
>>>     Battery: Read Battery level characteristic
>>>     Battery: Add support for notifications
>>>     Battery: Emit property changed on first read
>>>
>>>    Makefile.am                          |   10 +-
>>>    doc/battery-api.txt                  |   38 +++
>>>    doc/device-api.txt                   |    5 +
>>>    profiles/batterystate/batterystate.c |  518
>>> ++++++++++++++++++++++++++++++++++
>>>    profiles/batterystate/batterystate.h |   24 ++
>>>    profiles/batterystate/main.c         |   67 +++++
>>>    profiles/batterystate/manager.c      |   93 ++++++
>>>    profiles/batterystate/manager.h      |   24 ++
>>>    src/device.c                         |   66 +++++
>>>    src/device.h                         |    3 +
>>>    10 files changed, 846 insertions(+), 2 deletions(-)
>>>    create mode 100644 doc/battery-api.txt
>>>    create mode 100644 profiles/batterystate/batterystate.c
>>>    create mode 100644 profiles/batterystate/batterystate.h
>>>    create mode 100644 profiles/batterystate/main.c
>>>    create mode 100644 profiles/batterystate/manager.c
>>>    create mode 100644 profiles/batterystate/manager.h
>>>
>>
>> Ping anyone ? Did anyone get to review this ?
>>
>> Thanks,
>> Chen Ganir
>
>
> not yet.
> We have an INTERNAL release in two weeks, next week we will send
> comments in the ML.
>
Thanks. I'll be waiting.


> BR,
> Claudio
>

Chen Ganir.

Re: [PATCH v2 0/9] Add GATT Client Battery Service

[Joao Paulo Rechi Vita]

Hello Chen,

On Mon, Aug 6, 2012 at 2:52 AM, Chen Ganir <chen.ganir@ti.com> wrote:
> Claudio,
>
>
> On 08/03/2012 03:57 PM, Claudio Takahasi wrote:
>>
>> Hi Chen Ganir:
>>
>> On Wed, Aug 1, 2012 at 3:40 AM, Chen Ganir <chen.ganir@ti.com> wrote:
>>>
>>> On 07/25/2012 08:42 AM, Chen Ganir wrote:
>>>>
>>>>
>>>> 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 2 of this patch set, rebased on top of the latest
>>>> sources
>>>> and
>>>> fixes some issues found during testing and in the ML comments.
>>>>
>>>> Chen Ganir (9):
>>>>     Battery: Add Battery Service GATT Client
>>>>     Battery: Add connection logic
>>>>     Battery: Discover Characteristic Descriptors
>>>>     Battery: Get Battery ID
>>>>     Battery: Add Battery list to btd_device
>>>>     Battery: Add Battery D-BUS API
>>>>     Battery: Read Battery level characteristic
>>>>     Battery: Add support for notifications
>>>>     Battery: Emit property changed on first read
>>>>
>>>>    Makefile.am                          |   10 +-
>>>>    doc/battery-api.txt                  |   38 +++
>>>>    doc/device-api.txt                   |    5 +
>>>>    profiles/batterystate/batterystate.c |  518
>>>> ++++++++++++++++++++++++++++++++++
>>>>    profiles/batterystate/batterystate.h |   24 ++
>>>>    profiles/batterystate/main.c         |   67 +++++
>>>>    profiles/batterystate/manager.c      |   93 ++++++
>>>>    profiles/batterystate/manager.h      |   24 ++
>>>>    src/device.c                         |   66 +++++
>>>>    src/device.h                         |    3 +
>>>>    10 files changed, 846 insertions(+), 2 deletions(-)
>>>>    create mode 100644 doc/battery-api.txt
>>>>    create mode 100644 profiles/batterystate/batterystate.c
>>>>    create mode 100644 profiles/batterystate/batterystate.h
>>>>    create mode 100644 profiles/batterystate/main.c
>>>>    create mode 100644 profiles/batterystate/manager.c
>>>>    create mode 100644 profiles/batterystate/manager.h
>>>>
>>>
>>> Ping anyone ? Did anyone get to review this ?
>>>
>>> Thanks,
>>> Chen Ganir
>>
>>
>>
>> not yet.
>> We have an INTERNAL release in two weeks, next week we will send
>> comments in the ML.
>>
> Thanks. I'll be waiting.
>
>

I've been reviewing and did some quick tests on your code. It's
working with some minor issues, and I'll comment them on each commit.
But first I have a few more general questions:

1. Any specific reason for calling the plugin directory and files
'batterystate'? I don't see any reference to this name on the
documentation.

2. The spec recommends reading the battery level value with very
little frequency. Quoting the section 3.1.1:

"For example, if the expected battery life is in the order of years,
reading the battery level value more frequently than once a week is
not recommended."

And on the same section:

"The value of the Client Characteristic Configuration descriptor is
persistent for bonded devices when not in a connection."

At the moment the plugin is reading it every time it is probed, which
is a lot more than recommended. What do you think about only enabling
notifications after paring, and not reading the value at all and just
wait for the notifications. If the device doesn't support
notifications (which I *think* should be uncommon) we can read the
value after pairing and refresh it every week or so. For this to work
well we'll need characteristics value storage support, but that will
improve other things as well. While we don't support that, I don't
have other ideas to improve this.


>> BR,
>> Claudio
>>
>
> Chen Ganir.
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth"
> in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
João Paulo Rechi Vita
Openbossa Labs - INdT

Re: [PATCH v2 2/9] Battery: Add connection logic

[Joao Paulo Rechi Vita]

On Wed, Jul 25, 2012 at 2:42 AM, Chen Ganir <chen.ganir@ti.com> wrote:
> 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/batterystate/batterystate.c |   78 +++++++++++++++++++++++++++++++++-
>  profiles/batterystate/batterystate.h |    3 +-
>  profiles/batterystate/manager.c      |   22 +++++++++-
>  3 files changed, 99 insertions(+), 4 deletions(-)
>
> diff --git a/profiles/batterystate/batterystate.c b/profiles/batterystate/batterystate.c
> index 04c2e5e..40663f6 100644
> --- a/profiles/batterystate/batterystate.c
> +++ b/profiles/batterystate/batterystate.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 "batterystate.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,84 @@ static void batterystate_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_batterystate_cb(GSList *characteristics, guint8 status,
> +                                                       gpointer user_data)
> +{
> +       struct battery *batt = user_data;
> +       GSList *l;
> +
> +       if (status != 0) {
> +               error("Discover batterystate characteristics: %s",
> +                                                       att_ecode2str(status));
> +               return;
> +       }
>
> -int batterystate_register(struct btd_device *device)
> +       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_batterystate_cb, batt);
> +       }
> +}
> +
> +static void attio_disconnected_cb(gpointer user_data)
> +{
> +       struct battery *batt = user_data;
> +
> +       g_attrib_unref(batt->attrib);
> +       batt->attrib = NULL;
> +}
> +
> +int batterystate_register(struct btd_device *device,
> +                               struct gatt_primary *prim)
>  {
>         struct battery *batt;
>
>         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/batterystate/batterystate.h b/profiles/batterystate/batterystate.h
> index 9aedae7..2d30028 100644
> --- a/profiles/batterystate/batterystate.h
> +++ b/profiles/batterystate/batterystate.h
> @@ -19,6 +19,5 @@
>   *  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
>   *
>   */
> -
> -int batterystate_register(struct btd_device *device);
> +int batterystate_register(struct btd_device *device, struct gatt_primary *prim);
>  void batterystate_unregister(struct btd_device *device);
> diff --git a/profiles/batterystate/manager.c b/profiles/batterystate/manager.c
> index 6718acf..62076ac 100644
> --- a/profiles/batterystate/manager.c
> +++ b/profiles/batterystate/manager.c
> @@ -34,9 +34,29 @@
>
>  #define BATTERY_SERVICE_UUID           "0000180f-0000-1000-8000-00805f9b34fb"
>
> +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);
> +}
> +
>  static int batterystate_driver_probe(struct btd_device *device, GSList *uuids)
>  {
> -       return batterystate_register(device);
> +       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);

No need to check for the BATTERY_SERVICE_UUID. If driver has been
probed its because the remote implements this service, since it's
declared on the .uuids field of the plugin struct.

> +       if (l == NULL)
> +               return -EINVAL;
> +
> +       prim = l->data;
> +
> +       return batterystate_register(device, prim);

Getting the primary service pointer (manly used for handle range
information could be done from inside batterystate_register() itself
on batterystate.c instead of doing so on the manager code. This way
the plugin keeps more self-contained.

>  }
>
>  static void batterystate_driver_remove(struct btd_device *device)
> --
> 1.7.9.5
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
João Paulo Rechi Vita
Openbossa Labs - INdT

Re: [PATCH v2 4/9] Battery: Get Battery ID

[Joao Paulo Rechi Vita]

On Wed, Jul 25, 2012 at 2:42 AM, Chen Ganir <chen.ganir@ti.com> wrote:
> Read the battery level format characteristic descriptor to get the
> unique namespace and description values.
> ---
>  profiles/batterystate/batterystate.c |  113 ++++++++++++++++++++++++++--------
>  1 file changed, 86 insertions(+), 27 deletions(-)
>
> diff --git a/profiles/batterystate/batterystate.c b/profiles/batterystate/batterystate.c
> index a7d2f6e..d3a1974 100644
> --- a/profiles/batterystate/batterystate.c
> +++ b/profiles/batterystate/batterystate.c
> @@ -37,6 +37,8 @@
>  #include "batterystate.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,18 @@ 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 */
> +       uint8_t                 level;          /* Battery last known level */

The 'level' field it's not being used here. It should be added by the
same commit that uses it.

>  };
>
>  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 +92,55 @@ static void batterystate_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 +174,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);
> @@ -140,31 +195,35 @@ static void configure_batterystate_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
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
João Paulo Rechi Vita
Openbossa Labs - INdT

Re: [PATCH v2 5/9] Battery: Add Battery list to btd_device

[Joao Paulo Rechi Vita]

On Wed, Jul 25, 2012 at 2:42 AM, Chen Ganir <chen.ganir@ti.com> wrote:
> Add peer battery list to the btd_device. New property added to Device
> called Batteries.
> ---
>  doc/device-api.txt                   |    5 +++
>  profiles/batterystate/batterystate.c |    6 ++++
>  src/device.c                         |   66 ++++++++++++++++++++++++++++++++++
>  src/device.h                         |    3 ++
>  4 files changed, 80 insertions(+)
>
> diff --git a/doc/device-api.txt b/doc/device-api.txt
> index 3b84033..3d19a53 100644
> --- a/doc/device-api.txt
> +++ b/doc/device-api.txt
> @@ -175,3 +175,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.

This property should be deprecated by the introduction of the D-Bus
object manager (scheduled for the 5.0 release).

> diff --git a/profiles/batterystate/batterystate.c b/profiles/batterystate/batterystate.c
> index d3a1974..23dfab5 100644
> --- a/profiles/batterystate/batterystate.c
> +++ b/profiles/batterystate/batterystate.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 */
> @@ -206,6 +207,11 @@ static void configure_batterystate_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;
> +                       ch->path = g_strdup_printf("%s/BATT%04X",
> +                                               device_get_path(batt->dev),
> +                                               c->handle);
> +

What's the reationale behind using the characteristic handle as an
identifier of the battery on the object path? Can't we do anything
better than that? I understand this avoids clashing on a device with
multiple battery services, but I don't really like the idea of
exposing this internal details as an identifier on the API. Maybe
using the battery namespace + description? It is mandatory when the
device implements more than one instance of the battery service. If
there is only one instance we don't need an identifier at all, only
BATT would be sufficient.

> +                       device_add_battery(batt->dev, ch->path);
>
>                         batt->chars = g_slist_append(batt->chars, ch);
>
> diff --git a/src/device.c b/src/device.c
> index cd571f7..98e431a 100644
> --- a/src/device.c
> +++ b/src/device.c
> @@ -127,6 +127,10 @@ struct att_callbacks {
>         gpointer user_data;
>  };
>
> +struct btd_battery {
> +       char *path;
> +};
> +
>  struct btd_device {
>         bdaddr_t        bdaddr;
>         uint8_t         bdaddr_type;
> @@ -172,6 +176,7 @@ struct btd_device {
>
>         GIOChannel      *att_io;
>         guint           cleanup_id;
> +       GSList          *batteries;
>  };
>
>  static uint16_t uuid_list[] = {
> @@ -262,6 +267,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;
> @@ -1219,6 +1234,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);
> @@ -3141,3 +3159,51 @@ 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);
> +                       return;

If you return here the property changed signal will not be emitted.

> +               }
> +       }
> +
> +       batteries_changed(device);
> +}
> 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
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
João Paulo Rechi Vita
Openbossa Labs - INdT

Re: [PATCH v2 8/9] Battery: Add support for notifications

[Joao Paulo Rechi Vita]

On Wed, Jul 25, 2012 at 2:42 AM, Chen Ganir <chen.ganir@ti.com> wrote:
> Add support for emitting PropertyChanged when a battery level
> characteristic notification is sent from the peer device.
> ---
>  doc/battery-api.txt                  |    5 ++
>  profiles/batterystate/batterystate.c |  107 +++++++++++++++++++++++++++++++++-
>  2 files changed, 111 insertions(+), 1 deletion(-)
>
> diff --git a/doc/battery-api.txt b/doc/battery-api.txt
> index 5d7510d..f31e7e5 100644
> --- a/doc/battery-api.txt
> +++ b/doc/battery-api.txt
> @@ -16,6 +16,11 @@ 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 Namespace [readonly]
>
>                         Namespace value from the battery format characteristic
> diff --git a/profiles/batterystate/batterystate.c b/profiles/batterystate/batterystate.c
> index e5b68a9..718863b 100644
> --- a/profiles/batterystate/batterystate.c
> +++ b/profiles/batterystate/batterystate.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 */
>  };
>
> @@ -104,6 +105,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 batterystate_free(gpointer user_data)
>  {
>         struct battery *batt = user_data;
> @@ -117,6 +126,10 @@ static void batterystate_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 +171,17 @@ static void process_batteryservice_char(struct characteristic *ch)
>         }
>  }
>
> +static void batterylevel_enable_notify_cb(guint8 status, const guint8 *pdu,
> +                                               guint16 len, gpointer user_data)
> +{
> +       char *msg = user_data;
> +
> +       if (status != 0)
> +               error("Could not enable battery level notification: %s", msg);
> +
> +       g_free(msg);
> +}
> +
>  static void batterylevel_presentation_format_desc_cb(guint8 status,
>                                                 const guint8 *pdu, guint16 len,
>                                                 gpointer user_data)
> @@ -194,6 +218,23 @@ 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;
> +               char *msg;
> +
> +               val = GATT_CLIENT_CHARAC_CFG_NOTIF_BIT;
> +               msg = g_strdup("Enable BatteryLevel notification");
> +

What's the point of passing "Enable BatteryLevel notification" as
user_data for the batterylevel_enable_notify_cb() ?

> +               att_put_u16(val, atval);
> +               gatt_write_char(ch->batt->attrib, desc->handle, atval, 2,
> +                                       batterylevel_enable_notify_cb, msg);
> +               return;
> +       }
> +
>         bt_uuid16_create(&btuuid, GATT_CHARAC_FMT_UUID);
>
>         if (bt_uuid_cmp(&desc->uuid, &btuuid) == 0) {
> @@ -277,6 +318,12 @@ static DBusMessage *get_properties(DBusConnection *conn, DBusMessage *msg,
>         return reply;
>  }
>
> +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 GDBusMethodTable battery_methods[] = {
>         { GDBUS_METHOD("GetProperties",
>                                 NULL, GDBUS_ARGS({ "properties", "a{sv}" }),
> @@ -284,6 +331,11 @@ static GDBusMethodTable battery_methods[] = {
>         { }
>  };
>
> +static GDBusSignalTable battery_signals[] = {
> +       { GDBUS_SIGNAL("PropertyChanged",
> +               GDBUS_ARGS({ "name", "s" }, { "value", "v" })) },
> +       { }
> +};
>
>  static void configure_batterystate_cb(GSList *characteristics, guint8 status,
>                                                         gpointer user_data)
> @@ -318,7 +370,7 @@ static void configure_batterystate_cb(GSList *characteristics, guint8 status,
>
>                         if (!g_dbus_register_interface(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);
> @@ -346,12 +398,63 @@ static void configure_batterystate_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;
> +       }
> +

If pdu can be NULL here, than bluetoothd would have broken when
derreferencing it at "handle = att_get_u16(&pdu[1]);" in
notif_handler(). If pdu can be NULL the check needs to be done there.

> +       new_batt_level = pdu[1];
> +
> +       if (new_batt_level != c->level)
> +               changed = TRUE;
> +
> +       c->level = new_batt_level;
> +
> +done:

There is no need for the done label, since 'changed' is always false
when you 'goto' here. You could simply return instead of using goto,
although the whole if statement is likely to be dropped from this
function.

> +       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;
> +

ch->attr.uuid == BATTERY_LEVEL_UUID should be tested before anything
else, since other profiles may have enabled notifications for other
characteristics (hog does that, for example). When testing with hog
devices I got "notif_handler: Unexpected handle 0x0017" for every
input report received.

> +       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,
> @@ -369,6 +472,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
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
João Paulo Rechi Vita
Openbossa Labs - INdT

Re: [PATCH v2 9/9] Battery: Emit property changed on first read

[Joao Paulo Rechi Vita]

On Wed, Jul 25, 2012 at 2:42 AM, Chen Ganir <chen.ganir@ti.com> wrote:
> Emit battery level property changed upon connection, on first read.
> ---
>  profiles/batterystate/batterystate.c |    3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/profiles/batterystate/batterystate.c b/profiles/batterystate/batterystate.c
> index 718863b..4300f85 100644
> --- a/profiles/batterystate/batterystate.c
> +++ b/profiles/batterystate/batterystate.c
> @@ -72,6 +72,8 @@ struct descriptor {
>         bt_uuid_t               uuid;           /* UUID */
>  };
>
> +static void emit_battery_level_changed(struct characteristic *c);
> +

No need of this forward declaration, simply move the function
implementation before the first function that references it
(read_batterylevel_cb).

>  static void char_free(gpointer user_data)
>  {
>         struct characteristic *c = user_data;
> @@ -161,6 +163,7 @@ static void read_batterylevel_cb(guint8 status, const guint8 *pdu, guint16 len,
>         }
>
>         ch->level = value[0];
> +       emit_battery_level_changed(ch);
>  }
>
>  static void process_batteryservice_char(struct characteristic *ch)
> --
> 1.7.9.5
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
João Paulo Rechi Vita
Openbossa Labs - INdT

Re: [PATCH v2 0/9] Add GATT Client Battery Service

[Chen Ganir]

Joao,

On 08/07/2012 07:29 PM, Joao Paulo Rechi Vita wrote:
> Hello Chen,
>
> On Mon, Aug 6, 2012 at 2:52 AM, Chen Ganir <chen.ganir@ti.com> wrote:
>> Claudio,
>>
>>
>> On 08/03/2012 03:57 PM, Claudio Takahasi wrote:
>>>
>>> Hi Chen Ganir:
>>>
>>> On Wed, Aug 1, 2012 at 3:40 AM, Chen Ganir <chen.ganir@ti.com> wrote:
>>>>
>>>> On 07/25/2012 08:42 AM, Chen Ganir wrote:
>>>>>
>>>>>
>>>>> 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 2 of this patch set, rebased on top of the latest
>>>>> sources
>>>>> and
>>>>> fixes some issues found during testing and in the ML comments.
>>>>>
>>>>> Chen Ganir (9):
>>>>>      Battery: Add Battery Service GATT Client
>>>>>      Battery: Add connection logic
>>>>>      Battery: Discover Characteristic Descriptors
>>>>>      Battery: Get Battery ID
>>>>>      Battery: Add Battery list to btd_device
>>>>>      Battery: Add Battery D-BUS API
>>>>>      Battery: Read Battery level characteristic
>>>>>      Battery: Add support for notifications
>>>>>      Battery: Emit property changed on first read
>>>>>
>>>>>     Makefile.am                          |   10 +-
>>>>>     doc/battery-api.txt                  |   38 +++
>>>>>     doc/device-api.txt                   |    5 +
>>>>>     profiles/batterystate/batterystate.c |  518
>>>>> ++++++++++++++++++++++++++++++++++
>>>>>     profiles/batterystate/batterystate.h |   24 ++
>>>>>     profiles/batterystate/main.c         |   67 +++++
>>>>>     profiles/batterystate/manager.c      |   93 ++++++
>>>>>     profiles/batterystate/manager.h      |   24 ++
>>>>>     src/device.c                         |   66 +++++
>>>>>     src/device.h                         |    3 +
>>>>>     10 files changed, 846 insertions(+), 2 deletions(-)
>>>>>     create mode 100644 doc/battery-api.txt
>>>>>     create mode 100644 profiles/batterystate/batterystate.c
>>>>>     create mode 100644 profiles/batterystate/batterystate.h
>>>>>     create mode 100644 profiles/batterystate/main.c
>>>>>     create mode 100644 profiles/batterystate/manager.c
>>>>>     create mode 100644 profiles/batterystate/manager.h
>>>>>
>>>>
>>>> Ping anyone ? Did anyone get to review this ?
>>>>
>>>> Thanks,
>>>> Chen Ganir
>>>
>>>
>>>
>>> not yet.
>>> We have an INTERNAL release in two weeks, next week we will send
>>> comments in the ML.
>>>
>> Thanks. I'll be waiting.
>>
>>
>
> I've been reviewing and did some quick tests on your code. It's
> working with some minor issues, and I'll comment them on each commit.
> But first I have a few more general questions:
>
> 1. Any specific reason for calling the plugin directory and files
> 'batterystate'? I don't see any reference to this name on the
> documentation.
No specific reason. I will rename it to Battery, to correspond with the 
BAS spec.

>
> 2. The spec recommends reading the battery level value with very
> little frequency. Quoting the section 3.1.1:
>
> "For example, if the expected battery life is in the order of years,
> reading the battery level value more frequently than once a week is
> not recommended."
>
> And on the same section:
>
> "The value of the Client Characteristic Configuration descriptor is
> persistent for bonded devices when not in a connection."
>
> At the moment the plugin is reading it every time it is probed, which
> is a lot more than recommended. What do you think about only enabling
> notifications after paring, and not reading the value at all and just
> wait for the notifications. If the device doesn't support
> notifications (which I *think* should be uncommon) we can read the
> value after pairing and refresh it every week or so. For this to work
> well we'll need characteristics value storage support, but that will
> improve other things as well. While we don't support that, I don't
> have other ideas to improve this.
I agree with you that we will need some storage to allow following the 
specs correctly. I could just read the battery level on pairing only, 
and then rely on notifications (if supported) or read battery level on 
each connect. However, what happens if the device remains connected for 
a long period of time (more than a week) ? I will need to add some kind 
of mechanism to check how much time has passed since last reading, and 
invoke battery level readout for each battery level characteristic. Do 
you have any suggestions for improvement here ?

>
>
>>> BR,
>>> Claudio
>>>
>>
>> Chen Ganir.
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth"
>> in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
>
>
Thanks for reviewing this patch set. I will try to send revised and 
fixed set today or tomorrow with the fixes mentioned in your other patch 
review.

Chen Ganir.

Re: [PATCH v2 2/9] Battery: Add connection logic

[Chen Ganir]

On 08/07/2012 07:29 PM, Joao Paulo Rechi Vita wrote:
> On Wed, Jul 25, 2012 at 2:42 AM, Chen Ganir <chen.ganir@ti.com> wrote:
>> 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/batterystate/batterystate.c |   78 +++++++++++++++++++++++++++++++++-
>>   profiles/batterystate/batterystate.h |    3 +-
>>   profiles/batterystate/manager.c      |   22 +++++++++-
>>   3 files changed, 99 insertions(+), 4 deletions(-)
>>
>> diff --git a/profiles/batterystate/batterystate.c b/profiles/batterystate/batterystate.c
>> index 04c2e5e..40663f6 100644
>> --- a/profiles/batterystate/batterystate.c
>> +++ b/profiles/batterystate/batterystate.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 "batterystate.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,84 @@ static void batterystate_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_batterystate_cb(GSList *characteristics, guint8 status,
>> +                                                       gpointer user_data)
>> +{
>> +       struct battery *batt = user_data;
>> +       GSList *l;
>> +
>> +       if (status != 0) {
>> +               error("Discover batterystate characteristics: %s",
>> +                                                       att_ecode2str(status));
>> +               return;
>> +       }
>>
>> -int batterystate_register(struct btd_device *device)
>> +       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_batterystate_cb, batt);
>> +       }
>> +}
>> +
>> +static void attio_disconnected_cb(gpointer user_data)
>> +{
>> +       struct battery *batt = user_data;
>> +
>> +       g_attrib_unref(batt->attrib);
>> +       batt->attrib = NULL;
>> +}
>> +
>> +int batterystate_register(struct btd_device *device,
>> +                               struct gatt_primary *prim)
>>   {
>>          struct battery *batt;
>>
>>          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/batterystate/batterystate.h b/profiles/batterystate/batterystate.h
>> index 9aedae7..2d30028 100644
>> --- a/profiles/batterystate/batterystate.h
>> +++ b/profiles/batterystate/batterystate.h
>> @@ -19,6 +19,5 @@
>>    *  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
>>    *
>>    */
>> -
>> -int batterystate_register(struct btd_device *device);
>> +int batterystate_register(struct btd_device *device, struct gatt_primary *prim);
>>   void batterystate_unregister(struct btd_device *device);
>> diff --git a/profiles/batterystate/manager.c b/profiles/batterystate/manager.c
>> index 6718acf..62076ac 100644
>> --- a/profiles/batterystate/manager.c
>> +++ b/profiles/batterystate/manager.c
>> @@ -34,9 +34,29 @@
>>
>>   #define BATTERY_SERVICE_UUID           "0000180f-0000-1000-8000-00805f9b34fb"
>>
>> +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);
>> +}
>> +
>>   static int batterystate_driver_probe(struct btd_device *device, GSList *uuids)
>>   {
>> -       return batterystate_register(device);
>> +       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);
>
> No need to check for the BATTERY_SERVICE_UUID. If driver has been
> probed its because the remote implements this service, since it's
> declared on the .uuids field of the plugin struct.
I will remove the check.

>
>> +       if (l == NULL)
>> +               return -EINVAL;
>> +
>> +       prim = l->data;
>> +
>> +       return batterystate_register(device, prim);
>
> Getting the primary service pointer (manly used for handle range
> information could be done from inside batterystate_register() itself
> on batterystate.c instead of doing so on the manager code. This way
> the plugin keeps more self-contained.
I'll move that into the manager code as you recommended. Most of this 
code was taken from other plugins, just to make sure i conform with the 
current convnetions.

>
>>   }
>>
>>   static void batterystate_driver_remove(struct btd_device *device)
>> --
>> 1.7.9.5
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
>
>

Thanks,
Chen Ganir.

Re: [PATCH v2 4/9] Battery: Get Battery ID

[Chen Ganir]

Joao,

On 08/07/2012 07:29 PM, Joao Paulo Rechi Vita wrote:
> On Wed, Jul 25, 2012 at 2:42 AM, Chen Ganir <chen.ganir@ti.com> wrote:
>> Read the battery level format characteristic descriptor to get the
>> unique namespace and description values.
>> ---
>>   profiles/batterystate/batterystate.c |  113 ++++++++++++++++++++++++++--------
>>   1 file changed, 86 insertions(+), 27 deletions(-)
>>
>> diff --git a/profiles/batterystate/batterystate.c b/profiles/batterystate/batterystate.c
>> index a7d2f6e..d3a1974 100644
>> --- a/profiles/batterystate/batterystate.c
>> +++ b/profiles/batterystate/batterystate.c
>> @@ -37,6 +37,8 @@
>>   #include "batterystate.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,18 @@ 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 */
>> +       uint8_t                 level;          /* Battery last known level */
>
> The 'level' field it's not being used here. It should be added by the
> same commit that uses it.
>
I'll check it's usage and move it to the relevant patch.

>>   };
>>
>>   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 +92,55 @@ static void batterystate_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 +174,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);
>> @@ -140,31 +195,35 @@ static void configure_batterystate_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
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
>
>


-- 
*Chen Ganir***| Texas Instruments | *SW Engineer* Mobile Connectivity
Solutions| Direct: (972)-9-7906080 | Mobile: (972)-52-2438-406|Zarhin
26,Ra'anana, Israel |http://www.ti.com <http://www.ti.com/>
| chen.ganir@ti.com <mailto:chen.ganir@ti.com>

Re: [PATCH v2 5/9] Battery: Add Battery list to btd_device

[Chen Ganir]

Joao,

On 08/07/2012 07:29 PM, Joao Paulo Rechi Vita wrote:
> On Wed, Jul 25, 2012 at 2:42 AM, Chen Ganir <chen.ganir@ti.com> wrote:
>> Add peer battery list to the btd_device. New property added to Device
>> called Batteries.
>> ---
>>   doc/device-api.txt                   |    5 +++
>>   profiles/batterystate/batterystate.c |    6 ++++
>>   src/device.c                         |   66 ++++++++++++++++++++++++++++++++++
>>   src/device.h                         |    3 ++
>>   4 files changed, 80 insertions(+)
>>
>> diff --git a/doc/device-api.txt b/doc/device-api.txt
>> index 3b84033..3d19a53 100644
>> --- a/doc/device-api.txt
>> +++ b/doc/device-api.txt
>> @@ -175,3 +175,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.
>
> This property should be deprecated by the introduction of the D-Bus
> object manager (scheduled for the 5.0 release).
>
>> diff --git a/profiles/batterystate/batterystate.c b/profiles/batterystate/batterystate.c
>> index d3a1974..23dfab5 100644
>> --- a/profiles/batterystate/batterystate.c
>> +++ b/profiles/batterystate/batterystate.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 */
>> @@ -206,6 +207,11 @@ static void configure_batterystate_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;
>> +                       ch->path = g_strdup_printf("%s/BATT%04X",
>> +                                               device_get_path(batt->dev),
>> +                                               c->handle);
>> +
>
> What's the reationale behind using the characteristic handle as an
> identifier of the battery on the object path? Can't we do anything
> better than that? I understand this avoids clashing on a device with
> multiple battery services, but I don't really like the idea of
> exposing this internal details as an identifier on the API. Maybe
> using the battery namespace + description? It is mandatory when the
> device implements more than one instance of the battery service. If
> there is only one instance we don't need an identifier at all, only
> BATT would be sufficient.
Well, I did try to use the battery namespace and description, but i had 
to register the battery at a later stage, since discovering the 
characteristics format descriptors and reading them was done at a later 
stage. I also wanted to keep the naming convention, and avoid giving 
different names according to availability of other batteries or format 
descriptors. naming the batteries with the handle makes the most sense, 
since it is same as naming services and characteristics obj paths. I 
dont like the idea of having a single battery called BATT, multiple 
batteries called BATTXXXXXX for battery with namespace and BATTYYYY for 
batteries without it. This will be very confusing. Battery naming 
convention should remain the same regardless of those things.

>
>> +                       device_add_battery(batt->dev, ch->path);
>>
>>                          batt->chars = g_slist_append(batt->chars, ch);
>>
>> diff --git a/src/device.c b/src/device.c
>> index cd571f7..98e431a 100644
>> --- a/src/device.c
>> +++ b/src/device.c
>> @@ -127,6 +127,10 @@ struct att_callbacks {
>>          gpointer user_data;
>>   };
>>
>> +struct btd_battery {
>> +       char *path;
>> +};
>> +
>>   struct btd_device {
>>          bdaddr_t        bdaddr;
>>          uint8_t         bdaddr_type;
>> @@ -172,6 +176,7 @@ struct btd_device {
>>
>>          GIOChannel      *att_io;
>>          guint           cleanup_id;
>> +       GSList          *batteries;
>>   };
>>
>>   static uint16_t uuid_list[] = {
>> @@ -262,6 +267,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;
>> @@ -1219,6 +1234,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);
>> @@ -3141,3 +3159,51 @@ 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);
>> +                       return;
>
> If you return here the property changed signal will not be emitted.
>
True. I will fix this.

>> +               }
>> +       }
>> +
>> +       batteries_changed(device);
>> +}
>> 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
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
>
>

Thanks,
Chen Ganir.

Re: [PATCH v2 8/9] Battery: Add support for notifications

[Chen Ganir]

Joao,

On 08/07/2012 07:29 PM, Joao Paulo Rechi Vita wrote:
> On Wed, Jul 25, 2012 at 2:42 AM, Chen Ganir <chen.ganir@ti.com> wrote:
>> Add support for emitting PropertyChanged when a battery level
>> characteristic notification is sent from the peer device.
>> ---
>>   doc/battery-api.txt                  |    5 ++
>>   profiles/batterystate/batterystate.c |  107 +++++++++++++++++++++++++++++++++-
>>   2 files changed, 111 insertions(+), 1 deletion(-)
>>
>> diff --git a/doc/battery-api.txt b/doc/battery-api.txt
>> index 5d7510d..f31e7e5 100644
>> --- a/doc/battery-api.txt
>> +++ b/doc/battery-api.txt
>> @@ -16,6 +16,11 @@ 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 Namespace [readonly]
>>
>>                          Namespace value from the battery format characteristic
>> diff --git a/profiles/batterystate/batterystate.c b/profiles/batterystate/batterystate.c
>> index e5b68a9..718863b 100644
>> --- a/profiles/batterystate/batterystate.c
>> +++ b/profiles/batterystate/batterystate.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 */
>>   };
>>
>> @@ -104,6 +105,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 batterystate_free(gpointer user_data)
>>   {
>>          struct battery *batt = user_data;
>> @@ -117,6 +126,10 @@ static void batterystate_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 +171,17 @@ static void process_batteryservice_char(struct characteristic *ch)
>>          }
>>   }
>>
>> +static void batterylevel_enable_notify_cb(guint8 status, const guint8 *pdu,
>> +                                               guint16 len, gpointer user_data)
>> +{
>> +       char *msg = user_data;
>> +
>> +       if (status != 0)
>> +               error("Could not enable battery level notification: %s", msg);
>> +
>> +       g_free(msg);
>> +}
>> +
>>   static void batterylevel_presentation_format_desc_cb(guint8 status,
>>                                                  const guint8 *pdu, guint16 len,
>>                                                  gpointer user_data)
>> @@ -194,6 +218,23 @@ 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;
>> +               char *msg;
>> +
>> +               val = GATT_CLIENT_CHARAC_CFG_NOTIF_BIT;
>> +               msg = g_strdup("Enable BatteryLevel notification");
>> +
>
> What's the point of passing "Enable BatteryLevel notification" as
> user_data for the batterylevel_enable_notify_cb() ?
>
No point. just a leftover i need to remove.

>> +               att_put_u16(val, atval);
>> +               gatt_write_char(ch->batt->attrib, desc->handle, atval, 2,
>> +                                       batterylevel_enable_notify_cb, msg);
>> +               return;
>> +       }
>> +
>>          bt_uuid16_create(&btuuid, GATT_CHARAC_FMT_UUID);
>>
>>          if (bt_uuid_cmp(&desc->uuid, &btuuid) == 0) {
>> @@ -277,6 +318,12 @@ static DBusMessage *get_properties(DBusConnection *conn, DBusMessage *msg,
>>          return reply;
>>   }
>>
>> +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 GDBusMethodTable battery_methods[] = {
>>          { GDBUS_METHOD("GetProperties",
>>                                  NULL, GDBUS_ARGS({ "properties", "a{sv}" }),
>> @@ -284,6 +331,11 @@ static GDBusMethodTable battery_methods[] = {
>>          { }
>>   };
>>
>> +static GDBusSignalTable battery_signals[] = {
>> +       { GDBUS_SIGNAL("PropertyChanged",
>> +               GDBUS_ARGS({ "name", "s" }, { "value", "v" })) },
>> +       { }
>> +};
>>
>>   static void configure_batterystate_cb(GSList *characteristics, guint8 status,
>>                                                          gpointer user_data)
>> @@ -318,7 +370,7 @@ static void configure_batterystate_cb(GSList *characteristics, guint8 status,
>>
>>                          if (!g_dbus_register_interface(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);
>> @@ -346,12 +398,63 @@ static void configure_batterystate_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;
>> +       }
>> +
>
> If pdu can be NULL here, than bluetoothd would have broken when
> derreferencing it at "handle = att_get_u16(&pdu[1]);" in
> notif_handler(). If pdu can be NULL the check needs to be done there.
>
I will modify that.

>> +       new_batt_level = pdu[1];
>> +
>> +       if (new_batt_level != c->level)
>> +               changed = TRUE;
>> +
>> +       c->level = new_batt_level;
>> +
>> +done:
>
> There is no need for the done label, since 'changed' is always false
> when you 'goto' here. You could simply return instead of using goto,
> although the whole if statement is likely to be dropped from this
> function.
>
This is assuming notifications arrive only when battery level has 
changed. I agree, and i will change that.

>> +       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;
>> +
>
> ch->attr.uuid == BATTERY_LEVEL_UUID should be tested before anything
> else, since other profiles may have enabled notifications for other
> characteristics (hog does that, for example). When testing with hog
> devices I got "notif_handler: Unexpected handle 0x0017" for every
> input report received.
>
ch->attr.uuid is only valid later in the code, after i get the handle 
out of the PDU (need to make sure the PDU is in the correct length) , 
find the characteristic in the list (make sure it is not NULL), and set 
ch to l->data. I believe removing the error code will remove the 
annoyance, but i see no better way to extract the characteristic without 
making sure everything is valid.

>> +       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,
>> @@ -369,6 +472,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
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
>
>

Thanks,
Chen Ganir.

Re: [PATCH v2 9/9] Battery: Emit property changed on first read

[Chen Ganir]

Joao,

On 08/07/2012 07:29 PM, Joao Paulo Rechi Vita wrote:
> On Wed, Jul 25, 2012 at 2:42 AM, Chen Ganir <chen.ganir@ti.com> wrote:
>> Emit battery level property changed upon connection, on first read.
>> ---
>>   profiles/batterystate/batterystate.c |    3 +++
>>   1 file changed, 3 insertions(+)
>>
>> diff --git a/profiles/batterystate/batterystate.c b/profiles/batterystate/batterystate.c
>> index 718863b..4300f85 100644
>> --- a/profiles/batterystate/batterystate.c
>> +++ b/profiles/batterystate/batterystate.c
>> @@ -72,6 +72,8 @@ struct descriptor {
>>          bt_uuid_t               uuid;           /* UUID */
>>   };
>>
>> +static void emit_battery_level_changed(struct characteristic *c);
>> +
>
> No need of this forward declaration, simply move the function
> implementation before the first function that references it
> (read_batterylevel_cb).
>
I will move the function as suggested.

>>   static void char_free(gpointer user_data)
>>   {
>>          struct characteristic *c = user_data;
>> @@ -161,6 +163,7 @@ static void read_batterylevel_cb(guint8 status, const guint8 *pdu, guint16 len,
>>          }
>>
>>          ch->level = value[0];
>> +       emit_battery_level_changed(ch);
>>   }
>>
>>   static void process_batteryservice_char(struct characteristic *ch)
>> --
>> 1.7.9.5
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
>
>

Thanks,
Chen Ganir.

Re: [PATCH v2 5/9] Battery: Add Battery list to btd_device

[Chen Ganir]

Joao,

On 08/07/2012 07:29 PM, Joao Paulo Rechi Vita wrote:
> On Wed, Jul 25, 2012 at 2:42 AM, Chen Ganir <chen.ganir@ti.com> wrote:
>> Add peer battery list to the btd_device. New property added to Device
>> called Batteries.
>> ---
>>   doc/device-api.txt                   |    5 +++
>>   profiles/batterystate/batterystate.c |    6 ++++
>>   src/device.c                         |   66 ++++++++++++++++++++++++++++++++++
>>   src/device.h                         |    3 ++
>>   4 files changed, 80 insertions(+)
>>
>> diff --git a/doc/device-api.txt b/doc/device-api.txt
>> index 3b84033..3d19a53 100644
>> --- a/doc/device-api.txt
>> +++ b/doc/device-api.txt
>> @@ -175,3 +175,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.
>
> This property should be deprecated by the introduction of the D-Bus
> object manager (scheduled for the 5.0 release).
>
Can you please explain ? What do you mean deprecated ? What is the 
proposed mechanism to replace this property ?

Thanks,
Chen Ganir.