[PATCH v3 0/5] Add DeviceInformationService plugin

[PATCH v3 0/5] Add DeviceInformationService plugin

[chen.ganir]

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

Add Device Information Service GATT Client for reading peer
device PNP_ID and store it.

This is v3 of the patch set, modifying the doc/device-api.txt.

Chen Ganir (5):
  Add DeviceInformation GATT Client
  DeviceInfo: Add connection logic
  DeviceInfo: Discover Characteristics
  Fix device version in GetProperties
  DeviceInfo: Read PNP ID

 Makefile.am             |    9 ++-
 deviceinfo/deviceinfo.c |  205 +++++++++++++++++++++++++++++++++++++++++++++++
 deviceinfo/deviceinfo.h |   24 ++++++
 deviceinfo/main.c       |   55 +++++++++++++
 deviceinfo/manager.c    |   82 +++++++++++++++++++
 deviceinfo/manager.h    |   24 ++++++
 doc/device-api.txt      |    4 +
 src/device.c            |   35 ++++++++-
 src/device.h            |    3 +
 9 files changed, 437 insertions(+), 4 deletions(-)
 create mode 100644 deviceinfo/deviceinfo.c
 create mode 100644 deviceinfo/deviceinfo.h
 create mode 100644 deviceinfo/main.c
 create mode 100644 deviceinfo/manager.c
 create mode 100644 deviceinfo/manager.h

-- 
1.7.4.1

[PATCH v3 1/5] Add DeviceInformation GATT Client

[chen.ganir]

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

Add the DeviceInformation GATT Client plugin skeleton.
---
 Makefile.am             |    9 +++--
 deviceinfo/deviceinfo.c |   87 +++++++++++++++++++++++++++++++++++++++++++++++
 deviceinfo/deviceinfo.h |   24 +++++++++++++
 deviceinfo/main.c       |   55 +++++++++++++++++++++++++++++
 deviceinfo/manager.c    |   79 ++++++++++++++++++++++++++++++++++++++++++
 deviceinfo/manager.h    |   24 +++++++++++++
 6 files changed, 275 insertions(+), 3 deletions(-)
 create mode 100644 deviceinfo/deviceinfo.c
 create mode 100644 deviceinfo/deviceinfo.h
 create mode 100644 deviceinfo/main.c
 create mode 100644 deviceinfo/manager.c
 create mode 100644 deviceinfo/manager.h

diff --git a/Makefile.am b/Makefile.am
index ddc28d2..0a2a38d 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -213,7 +213,8 @@ builtin_sources += health/hdp_main.c health/hdp_types.h \
 endif
 
 if GATTMODULES
-builtin_modules += thermometer alert time gatt_example proximity
+builtin_modules += thermometer alert time gatt_example proximity \
+			deviceinfo
 builtin_sources += thermometer/main.c \
 			thermometer/manager.h thermometer/manager.c \
 			thermometer/thermometer.h thermometer/thermometer.c \
@@ -222,10 +223,12 @@ builtin_sources += thermometer/main.c \
             plugins/gatt-example.c \
             proximity/main.c proximity/manager.h proximity/manager.c \
 			proximity/monitor.h proximity/monitor.c \
-			proximity/reporter.h proximity/reporter.c
+			proximity/reporter.h proximity/reporter.c \
+			deviceinfo/main.c \
+			deviceinfo/manager.h deviceinfo/manager.c \
+			deviceinfo/deviceinfo.h deviceinfo/deviceinfo.c
 endif
 
-
 builtin_modules += hciops mgmtops
 builtin_sources += plugins/hciops.c plugins/mgmtops.c
 
diff --git a/deviceinfo/deviceinfo.c b/deviceinfo/deviceinfo.c
new file mode 100644
index 0000000..95e7f1d
--- /dev/null
+++ b/deviceinfo/deviceinfo.c
@@ -0,0 +1,87 @@
+/*
+ *
+ *  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 "deviceinfo.h"
+
+struct deviceinfo {
+	struct btd_device	*dev;		/* Device reference */
+};
+
+static GSList *deviceinfoservers = NULL;
+
+static void deviceinfo_free(gpointer user_data)
+{
+	struct deviceinfo *d = user_data;
+
+	btd_device_unref(d->dev);
+	g_free(d);
+}
+
+static gint cmp_device(gconstpointer a, gconstpointer b)
+{
+	const struct deviceinfo *d = a;
+	const struct btd_device *dev = b;
+
+	if (dev == d->dev)
+		return 0;
+
+	return -1;
+}
+
+int deviceinfo_register(struct btd_device *device)
+{
+	struct deviceinfo *d;
+
+	d = g_new0(struct deviceinfo, 1);
+	d->dev = btd_device_ref(device);
+
+	deviceinfoservers = g_slist_prepend(deviceinfoservers, d);
+
+	return 0;
+}
+
+void deviceinfo_unregister(struct btd_device *device)
+{
+	struct deviceinfo *d;
+	GSList *l;
+
+	l = g_slist_find_custom(deviceinfoservers, device, cmp_device);
+	if (l == NULL)
+		return;
+
+	d = l->data;
+	deviceinfoservers = g_slist_remove(deviceinfoservers, d);
+
+	deviceinfo_free(d);
+}
diff --git a/deviceinfo/deviceinfo.h b/deviceinfo/deviceinfo.h
new file mode 100644
index 0000000..0ea6ff5
--- /dev/null
+++ b/deviceinfo/deviceinfo.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 deviceinfo_register(struct btd_device *device);
+void deviceinfo_unregister(struct btd_device *device);
diff --git a/deviceinfo/main.c b/deviceinfo/main.c
new file mode 100644
index 0000000..3acf04b
--- /dev/null
+++ b/deviceinfo/main.c
@@ -0,0 +1,55 @@
+/*
+ *
+ *  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 <errno.h>
+#include <stdint.h>
+
+#include "plugin.h"
+#include "manager.h"
+#include "hcid.h"
+#include "log.h"
+
+static int deviceinfo_init(void)
+{
+	if (!main_opts.gatt_enabled) {
+		DBG("GATT is disabled");
+		return -ENOTSUP;
+	}
+
+	return deviceinfo_manager_init();
+}
+
+static void deviceinfo_exit(void)
+{
+	if (!main_opts.gatt_enabled)
+		return;
+
+	deviceinfo_manager_exit();
+}
+
+BLUETOOTH_PLUGIN_DEFINE(deviceinfo, VERSION, BLUETOOTH_PLUGIN_PRIORITY_DEFAULT,
+					deviceinfo_init, deviceinfo_exit)
diff --git a/deviceinfo/manager.c b/deviceinfo/manager.c
new file mode 100644
index 0000000..025a076
--- /dev/null
+++ b/deviceinfo/manager.c
@@ -0,0 +1,79 @@
+/*
+ *
+ *  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 "deviceinfo.h"
+#include "manager.h"
+
+#define DEVICE_INFORMATION_UUID		"0000180a-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 deviceinfo_driver_probe(struct btd_device *device, GSList *uuids)
+{
+	GSList *primaries, *l;
+
+	primaries = btd_device_get_primaries(device);
+
+	l = g_slist_find_custom(primaries, DEVICE_INFORMATION_UUID,
+							primary_uuid_cmp);
+	if (l == NULL)
+		return -EINVAL;
+
+	return deviceinfo_register(device);
+}
+
+static void deviceinfo_driver_remove(struct btd_device *device)
+{
+	deviceinfo_unregister(device);
+}
+
+static struct btd_device_driver deviceinfo_device_driver = {
+	.name	= "deviceinfo-driver",
+	.uuids	= BTD_UUIDS(DEVICE_INFORMATION_UUID),
+	.probe	= deviceinfo_driver_probe,
+	.remove	= deviceinfo_driver_remove
+};
+
+int deviceinfo_manager_init(void)
+{
+	return btd_register_device_driver(&deviceinfo_device_driver);
+}
+
+void deviceinfo_manager_exit(void)
+{
+	btd_unregister_device_driver(&deviceinfo_device_driver);
+}
diff --git a/deviceinfo/manager.h b/deviceinfo/manager.h
new file mode 100644
index 0000000..0f742ca
--- /dev/null
+++ b/deviceinfo/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 deviceinfo_manager_init(void);
+void deviceinfo_manager_exit(void);
-- 
1.7.4.1

Re: [PATCH v3 1/5] Add DeviceInformation GATT Client

[Johan Hedberg]

Hi Chen,

On Wed, Mar 28, 2012, chen.ganir@ti.com wrote:
> +struct deviceinfo {
> +	struct btd_device	*dev;		/* Device reference */
> +};

Would it make sense to use device_info instead of deviceinfo? Or maybe
just di? In general I'm not so happy with this long word though I'm not
sure what the best replacement for it would be. Same goes for the
subdirectory and the filenames.

> +static GSList *deviceinfoservers = NULL;

You could call this just "servers" since it's static and already inside
a file called deviceinfo.c.

> +static void deviceinfo_exit(void)
> +{
> +	if (!main_opts.gatt_enabled)
> +		return;
> +
> +	deviceinfo_manager_exit();
> +}

I think it's a bug if *_exit() gets called even if *_init() failed. So
probably the check for gatt_enabled is unnecessary here.

Johan

[PATCH v3 2/5] DeviceInfo: Add connection logic

[chen.ganir]

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

Add connection logic to the Device Information Plugin. When the
driver is loaded, it will request a connection to the remote device
and release the connection request when destroyed.
---
 deviceinfo/deviceinfo.c |   28 ++++++++++++++++++++++++++++
 1 files changed, 28 insertions(+), 0 deletions(-)

diff --git a/deviceinfo/deviceinfo.c b/deviceinfo/deviceinfo.c
index 95e7f1d..8624a31 100644
--- a/deviceinfo/deviceinfo.c
+++ b/deviceinfo/deviceinfo.c
@@ -29,13 +29,18 @@
 #include <bluetooth/uuid.h>
 #include "adapter.h"
 #include "device.h"
+#include "gattrib.h"
+#include "attio.h"
 #include "att.h"
 #include "gattrib.h"
 #include "gatt.h"
 #include "deviceinfo.h"
 
+
 struct deviceinfo {
 	struct btd_device	*dev;		/* Device reference */
+	GAttrib			*attrib;		/* GATT connection */
+	guint			attioid;		/* Att watcher id */
 };
 
 static GSList *deviceinfoservers = NULL;
@@ -44,6 +49,12 @@ static void deviceinfo_free(gpointer user_data)
 {
 	struct deviceinfo *d = user_data;
 
+	if (d->attioid > 0)
+		btd_device_remove_attio_callback(d->dev, d->attioid);
+
+	if (d->attrib != NULL)
+		g_attrib_unref(d->attrib);
+
 	btd_device_unref(d->dev);
 	g_free(d);
 }
@@ -59,6 +70,21 @@ static gint cmp_device(gconstpointer a, gconstpointer b)
 	return -1;
 }
 
+static void attio_connected_cb(GAttrib *attrib, gpointer user_data)
+{
+	struct deviceinfo *d = user_data;
+
+	d->attrib = g_attrib_ref(attrib);
+}
+
+static void attio_disconnected_cb(gpointer user_data)
+{
+	struct deviceinfo *d = user_data;
+
+	g_attrib_unref(d->attrib);
+	d->attrib = NULL;
+}
+
 int deviceinfo_register(struct btd_device *device)
 {
 	struct deviceinfo *d;
@@ -68,6 +94,8 @@ int deviceinfo_register(struct btd_device *device)
 
 	deviceinfoservers = g_slist_prepend(deviceinfoservers, d);
 
+	d->attioid = btd_device_add_attio_callback(device, attio_connected_cb,
+						attio_disconnected_cb, d);
 	return 0;
 }
 
-- 
1.7.4.1

[PATCH v3 3/5] DeviceInfo: Discover Characteristics

[chen.ganir]

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

Add logic to discover all characteristics and build a
characteristic list.
---
 deviceinfo/deviceinfo.c |   54 ++++++++++++++++++++++++++++++++++++++++++++++-
 deviceinfo/deviceinfo.h |    2 +-
 deviceinfo/manager.c    |    5 +++-
 3 files changed, 58 insertions(+), 3 deletions(-)

diff --git a/deviceinfo/deviceinfo.c b/deviceinfo/deviceinfo.c
index 8624a31..cc2b061 100644
--- a/deviceinfo/deviceinfo.c
+++ b/deviceinfo/deviceinfo.c
@@ -34,17 +34,33 @@
 #include "att.h"
 #include "gattrib.h"
 #include "gatt.h"
+#include "log.h"
 #include "deviceinfo.h"
 
+#define PNPID_UUID		                "00002a50-0000-1000-8000-00805f9b34fb"
 
 struct deviceinfo {
 	struct btd_device	*dev;		/* Device reference */
 	GAttrib			*attrib;		/* GATT connection */
+	struct att_range	*svc_range;	/* DeviceInfo range */
 	guint			attioid;		/* Att watcher id */
+	GSList			*chars;		/* Characteristics */
 };
 
 static GSList *deviceinfoservers = NULL;
 
+struct characteristic {
+	struct gatt_char		attr;	/* Characteristic */
+	struct deviceinfo	*d;	/* deviceinfo where the char belongs */
+};
+
+static void char_free(gpointer user_data)
+{
+	struct characteristic *c = user_data;
+
+	g_free(c);
+}
+
 static void deviceinfo_free(gpointer user_data)
 {
 	struct deviceinfo *d = user_data;
@@ -55,7 +71,11 @@ static void deviceinfo_free(gpointer user_data)
 	if (d->attrib != NULL)
 		g_attrib_unref(d->attrib);
 
+	if (d->chars != NULL)
+		g_slist_free_full(d->chars, char_free);
+
 	btd_device_unref(d->dev);
+	g_free(d->svc_range);
 	g_free(d);
 }
 
@@ -70,11 +90,40 @@ static gint cmp_device(gconstpointer a, gconstpointer b)
 	return -1;
 }
 
+static void configure_deviceinfo_cb(GSList *characteristics, guint8 status,
+							gpointer user_data)
+{
+	struct deviceinfo *d = user_data;
+	GSList *l;
+
+	if (status != 0) {
+		error("Discover deviceinfo characteristics: %s",
+							att_ecode2str(status));
+		return;
+	}
+
+	for (l = characteristics; l; l = l->next) {
+		struct gatt_char *c = l->data;
+		struct characteristic *ch;
+
+		ch = g_new0(struct characteristic, 1);
+		ch->attr.handle = c->handle;
+		ch->attr.properties = c->properties;
+		ch->attr.value_handle = c->value_handle;
+		memcpy(ch->attr.uuid, c->uuid, MAX_LEN_UUID_STR + 1);
+		ch->d = d;
+
+		d->chars = g_slist_append(d->chars, ch);
+	}
+}
 static void attio_connected_cb(GAttrib *attrib, gpointer user_data)
 {
 	struct deviceinfo *d = user_data;
 
 	d->attrib = g_attrib_ref(attrib);
+
+	gatt_discover_char(d->attrib, d->svc_range->start, d->svc_range->end,
+					NULL, configure_deviceinfo_cb, d);
 }
 
 static void attio_disconnected_cb(gpointer user_data)
@@ -85,12 +134,15 @@ static void attio_disconnected_cb(gpointer user_data)
 	d->attrib = NULL;
 }
 
-int deviceinfo_register(struct btd_device *device)
+int deviceinfo_register(struct btd_device *device, struct gatt_primary *prim)
 {
 	struct deviceinfo *d;
 
 	d = g_new0(struct deviceinfo, 1);
 	d->dev = btd_device_ref(device);
+	d->svc_range = g_new0(struct att_range, 1);
+	d->svc_range->start = prim->range.start;
+	d->svc_range->end = prim->range.end;
 
 	deviceinfoservers = g_slist_prepend(deviceinfoservers, d);
 
diff --git a/deviceinfo/deviceinfo.h b/deviceinfo/deviceinfo.h
index 0ea6ff5..7a804a5 100644
--- a/deviceinfo/deviceinfo.h
+++ b/deviceinfo/deviceinfo.h
@@ -20,5 +20,5 @@
  *
  */
 
-int deviceinfo_register(struct btd_device *device);
+int deviceinfo_register(struct btd_device *device, struct gatt_primary *prim);
 void deviceinfo_unregister(struct btd_device *device);
diff --git a/deviceinfo/manager.c b/deviceinfo/manager.c
index 025a076..c2378c2 100644
--- a/deviceinfo/manager.c
+++ b/deviceinfo/manager.c
@@ -44,6 +44,7 @@ static gint primary_uuid_cmp(gconstpointer a, gconstpointer b)
 
 static int deviceinfo_driver_probe(struct btd_device *device, GSList *uuids)
 {
+	struct gatt_primary *prim;
 	GSList *primaries, *l;
 
 	primaries = btd_device_get_primaries(device);
@@ -53,7 +54,9 @@ static int deviceinfo_driver_probe(struct btd_device *device, GSList *uuids)
 	if (l == NULL)
 		return -EINVAL;
 
-	return deviceinfo_register(device);
+	prim = l->data;
+
+	return deviceinfo_register(device, prim);
 }
 
 static void deviceinfo_driver_remove(struct btd_device *device)
-- 
1.7.4.1

Re: [PATCH v3 3/5] DeviceInfo: Discover Characteristics

[Johan Hedberg]

Hi Chen,

On Wed, Mar 28, 2012, chen.ganir@ti.com wrote:
> +static void char_free(gpointer user_data)
> +{
> +	struct characteristic *c = user_data;
> +
> +	g_free(c);
> +}
> +
>  static void deviceinfo_free(gpointer user_data)
>  {
>  	struct deviceinfo *d = user_data;
> @@ -55,7 +71,11 @@ static void deviceinfo_free(gpointer user_data)
>  	if (d->attrib != NULL)
>  		g_attrib_unref(d->attrib);
>  
> +	if (d->chars != NULL)
> +		g_slist_free_full(d->chars, char_free);

You don't need to check for NULL before calling g_slist_free_full since
NULL is a valid GSList (signifying an empty list). You also don't really
need the char_free function but could simply do:

	g_slist_free_full(d->chars, g_free);

Johan

[PATCH v3 4/5] Fix device version in GetProperties

[chen.ganir]

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

Fix how device version is added to the property dictionay.
---
 src/device.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/src/device.c b/src/device.c
index eab7e4c..3b772ee 100644
--- a/src/device.c
+++ b/src/device.c
@@ -383,7 +383,7 @@ static DBusMessage *get_properties(DBusConnection *conn,
 							&device->product);
 
 	/* Version */
-	if (device->product)
+	if (device->version)
 		dict_append_entry(&dict, "Version", DBUS_TYPE_UINT16,
 							&device->version);
 
-- 
1.7.4.1

Re: [PATCH v3 4/5] Fix device version in GetProperties

[Johan Hedberg]

Hi Chen,

On Wed, Mar 28, 2012, chen.ganir@ti.com wrote:
> From: Chen Ganir <chen.ganir@ti.com>
> 
> Fix how device version is added to the property dictionay.
> ---
>  src/device.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/src/device.c b/src/device.c
> index eab7e4c..3b772ee 100644
> --- a/src/device.c
> +++ b/src/device.c
> @@ -383,7 +383,7 @@ static DBusMessage *get_properties(DBusConnection *conn,
>  							&device->product);
>  
>  	/* Version */
> -	if (device->product)
> +	if (device->version)
>  		dict_append_entry(&dict, "Version", DBUS_TYPE_UINT16,
>  							&device->version);
>  

This one has been pushed upstream so no need to resend it for v4 of this
set.

Johan

[PATCH v3 5/5] DeviceInfo: Read PNP ID

[chen.ganir]

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

Read the PNP ID characteristic of the DeviceInfo Service, and
store it inside the btd_device, for use by other profiles.
---
 deviceinfo/deviceinfo.c |   38 ++++++++++++++++++++++++++++++++++++++
 doc/device-api.txt      |    4 ++++
 src/device.c            |   33 +++++++++++++++++++++++++++++++++
 src/device.h            |    3 +++
 4 files changed, 78 insertions(+), 0 deletions(-)

diff --git a/deviceinfo/deviceinfo.c b/deviceinfo/deviceinfo.c
index cc2b061..dffff60 100644
--- a/deviceinfo/deviceinfo.c
+++ b/deviceinfo/deviceinfo.c
@@ -90,6 +90,42 @@ static gint cmp_device(gconstpointer a, gconstpointer b)
 	return -1;
 }
 
+static void read_pnpid_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) {
+		DBG("value read failed: %s",
+							att_ecode2str(status));
+		return;
+	}
+
+	if (!dec_read_resp(pdu, len, value, &vlen)) {
+		DBG("Protocol error\n");
+		return;
+	}
+
+	if (vlen < 7) {
+		DBG("Invalid deviceinfo received");
+		return;
+	}
+
+	device_set_pnpid(ch->d->dev,value[0],att_get_u16(&value[1]), att_get_u16(&value[3]),
+				att_get_u16(&value[5]));
+}
+
+static void process_deviceinfo_char(struct characteristic *ch)
+{
+	if (g_strcmp0(ch->attr.uuid, PNPID_UUID) == 0) {
+		gatt_read_char(ch->d->attrib, ch->attr.value_handle, 0,
+							read_pnpid_cb, ch);
+		return;
+	}
+}
+
 static void configure_deviceinfo_cb(GSList *characteristics, guint8 status,
 							gpointer user_data)
 {
@@ -114,6 +150,8 @@ static void configure_deviceinfo_cb(GSList *characteristics, guint8 status,
 		ch->d = d;
 
 		d->chars = g_slist_append(d->chars, ch);
+
+		process_deviceinfo_char(ch);
 	}
 }
 static void attio_connected_cb(GAttrib *attrib, gpointer user_data)
diff --git a/doc/device-api.txt b/doc/device-api.txt
index e8fc314..6c1a3c7 100644
--- a/doc/device-api.txt
+++ b/doc/device-api.txt
@@ -129,6 +129,10 @@ Properties	string Address [readonly]
 
 			Vendor unique numeric identifier.
 
+	    uint16 VendorSource [readonly]
+
+	        Vendor source numeric identifier.
+
 		uint16 Product [readonly]
 
 			Product unique numeric identifier.
diff --git a/src/device.c b/src/device.c
index 3b772ee..dad8c26 100644
--- a/src/device.c
+++ b/src/device.c
@@ -131,6 +131,7 @@ struct btd_device {
 	gchar		*path;
 	char		name[MAX_NAME_LENGTH + 1];
 	char		*alias;
+	uint16_t	vendor_src;
 	uint16_t	vendor;
 	uint16_t	product;
 	uint16_t	version;
@@ -377,6 +378,11 @@ static DBusMessage *get_properties(DBusConnection *conn,
 		dict_append_entry(&dict, "Vendor", DBUS_TYPE_UINT16,
 							&device->vendor);
 
+	/* Vendor Source*/
+	if (device->vendor_src)
+		dict_append_entry(&dict, "VendorSource", DBUS_TYPE_UINT16,
+							&device->vendor_src);
+
 	/* Product */
 	if (device->product)
 		dict_append_entry(&dict, "Product", DBUS_TYPE_UINT16,
@@ -983,6 +989,19 @@ static void device_set_vendor(struct btd_device *device, uint16_t value)
 				DBUS_TYPE_UINT16, &value);
 }
 
+static void device_set_vendor_src(struct btd_device *device, uint16_t value)
+{
+	DBusConnection *conn = get_dbus_connection();
+
+	if (device->vendor_src == value)
+		return;
+
+	device->vendor_src = value;
+
+	emit_property_changed(conn, device->path, DEVICE_INTERFACE, "VendorSource",
+				DBUS_TYPE_UINT16, &value);
+}
+
 static void device_set_product(struct btd_device *device, uint16_t value)
 {
 	DBusConnection *conn = get_dbus_connection();
@@ -1102,6 +1121,11 @@ uint16_t btd_device_get_vendor(struct btd_device *device)
 	return device->vendor;
 }
 
+uint16_t btd_device_get_vendor_src(struct btd_device *device)
+{
+	return device->vendor_src;
+}
+
 uint16_t btd_device_get_product(struct btd_device *device)
 {
 	return device->product;
@@ -2992,3 +3016,12 @@ gboolean btd_device_remove_attio_callback(struct btd_device *device, guint id)
 
 	return TRUE;
 }
+
+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)
+{
+	device_set_vendor(device, vendor_id);
+	device_set_vendor_src(device, vendor_id_src);
+	device_set_product(device, product_id);
+	device_set_version(device, product_ver);
+}
diff --git a/src/device.h b/src/device.h
index 7cb9bb2..c7973c5 100644
--- a/src/device.h
+++ b/src/device.h
@@ -39,6 +39,7 @@ struct btd_device *device_create(DBusConnection *conn,
 void device_set_name(struct btd_device *device, const char *name);
 void device_get_name(struct btd_device *device, char *name, size_t len);
 uint16_t btd_device_get_vendor(struct btd_device *device);
+uint16_t btd_device_get_vendor_src(struct btd_device *device);
 uint16_t btd_device_get_product(struct btd_device *device);
 uint16_t btd_device_get_version(struct btd_device *device);
 void device_remove(struct btd_device *device, gboolean remove_stored);
@@ -120,3 +121,5 @@ int device_block(DBusConnection *conn, struct btd_device *device,
 						gboolean update_only);
 int device_unblock(DBusConnection *conn, struct btd_device *device,
 					gboolean silent, gboolean update_only);
+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);
-- 
1.7.4.1

Re: [PATCH v3 5/5] DeviceInfo: Read PNP ID

[Johan Hedberg]

Hi Chen,

On Wed, Mar 28, 2012, chen.ganir@ti.com wrote:
> +	if (status != 0) {
> +		DBG("value read failed: %s",
> +							att_ecode2str(status));

Why is this split into two lines. The call can easily fit within 80
chars with all of its parameters. Also, this should be error() and not
DBG().

> +	if (!dec_read_resp(pdu, len, value, &vlen)) {
> +		DBG("Protocol error\n");

This should be error() and the \n at the end should be removed.

> +	device_set_pnpid(ch->d->dev,value[0],att_get_u16(&value[1]), att_get_u16(&value[3]),
> +				att_get_u16(&value[5]));

Over 80 character line and the continuation looks like it's incorrectly
indented (it should be indented with tabs as much as possible as long as
you don't hit the 80 character boundary.

> +static void process_deviceinfo_char(struct characteristic *ch)
> +{
> +	if (g_strcmp0(ch->attr.uuid, PNPID_UUID) == 0) {
> +		gatt_read_char(ch->d->attrib, ch->attr.value_handle, 0,
> +							read_pnpid_cb, ch);
> +		return;
> +	}
> +}

The explicit return statement seems redundant since that's what the
function does anyway even without the if-branch. You can then also
remove the {}.

Johan

Re: [PATCH v3 5/5] DeviceInfo: Read PNP ID

[Anderson Lizardo]

Hi Chen/Johan,

On Thu, Mar 29, 2012 at 6:22 AM, Johan Hedberg <johan.hedberg@gmail.com> wrote:
> Hi Chen,
>
> On Wed, Mar 28, 2012, chen.ganir@ti.com wrote:
>> +     if (status != 0) {
>> +             DBG("value read failed: %s",
>> +                                                     att_ecode2str(status));
>
> Why is this split into two lines. The call can easily fit within 80
> chars with all of its parameters. Also, this should be error() and not
> DBG().
>
>> +     if (!dec_read_resp(pdu, len, value, &vlen)) {
>> +             DBG("Protocol error\n");
>
> This should be error() and the \n at the end should be removed.

Just to complement: if using error(), be sure to use a more meaningful
error message, because the source file and function name will not
appear on the log in this case.

E.g.
"Could not read PNP ID value: %s"
"ATT protocol error while reading PNP ID"

Also, there is no need to add "\n" to DBG() or error(), they are added
automatically.

Regards,
-- 
Anderson Lizardo
Instituto Nokia de Tecnologia - INdT
Manaus - Brazil