[PATCH v4 0/5] Enhanced support for extended inquiry response

[PATCH v4 0/5] Enhanced support for extended inquiry response

[Inga Stotland]

EIR is updated whenever local SDP record database changes. 
Added support for UUID128 service descriptors in EIR to allow peek at available
services offered by a remote device without establishing SDP connection.  


-- 
Inga Stotland
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

[PATCH 1/5] Spec constants for Extended Inquiry Response field types

[Inga Stotland]

---
 src/sdpd.h |   13 +++++++++++++
 1 files changed, 13 insertions(+), 0 deletions(-)

diff --git a/src/sdpd.h b/src/sdpd.h
index e93b0b6..c1d69a9 100644
--- a/src/sdpd.h
+++ b/src/sdpd.h
@@ -34,6 +34,19 @@
 #define SDPDBG(fmt...)
 #endif
 
+#define EIR_DATA_LENGTH  240
+
+#define EIR_UUID16_SOME             0x02  /* 16-bit UUID, more available */
+#define EIR_UUID16_ALL              0x03  /* 16-bit UUID, all listed */
+#define EIR_UUID32_SOME             0x04  /* 32-bit UUID, more available */
+#define EIR_UUID32_ALL              0x05  /* 32-bit UUID, all listed */
+#define EIR_UUID128_SOME            0x06  /* 128-bit UUID, more available */
+#define EIR_UUID128_ALL             0x07  /* 128-bit UUID, all listed */
+#define EIR_NAME_SHORT              0x08  /* shortened local name */
+#define EIR_NAME_COMPLETE           0x09  /* complete local name */
+#define EIR_TX_POWER                0x0A  /* Transmit power level */
+#define EIR_DEVICE_ID               0x10  /* device ID */
+
 typedef struct request {
 	bdaddr_t device;
 	bdaddr_t bdaddr;
-- 
1.7.1

-- 
Inga Stotland
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

[PATCH 2/5] Support for adding UUID128 to extended inquiry response

[Inga Stotland]

---
 src/sdpd-service.c |  108 ++++++++++++++++++++++++++++++++++++++++++++--------
 1 files changed, 92 insertions(+), 16 deletions(-)

diff --git a/src/sdpd-service.c b/src/sdpd-service.c
index cdbb4f4..1314ada 100644
--- a/src/sdpd-service.c
+++ b/src/sdpd-service.c
@@ -49,6 +49,8 @@
 #include "manager.h"
 #include "adapter.h"
 
+#define SIZEOF_UUID128 16
+
 static sdp_record_t *server = NULL;
 
 static uint16_t did_vendor = 0x0000;
@@ -174,41 +176,103 @@ static void update_svclass_list(const bdaddr_t *src)
 
 }
 
+static void eir_generate_uuid128(sdp_list_t *list,
+					uint8_t *ptr, uint16_t *eir_len)
+{
+	int i, k, index = 0;
+	uint16_t len = *eir_len;
+	uint8_t *uuid128;
+	gboolean truncated = FALSE;
+
+	/* Store UUID128 in place, skip 2 bytes to insert type and length later */
+	uuid128 = ptr + 2;
+
+	for (; list; list = list->next) {
+		sdp_record_t *rec = (sdp_record_t *) list->data;
+
+		if (rec->svclass.type != SDP_UUID128)
+			continue;
+
+		/* Stop if not enough space to put next UUID128 */
+		if ((len + 2 + SIZEOF_UUID128) > EIR_DATA_LENGTH) {
+			truncated = TRUE;
+			break;
+		}
+
+		/* Check for duplicates, EIR data is Little Endian */
+		for (i = 0; i < index; i++) {
+			for (k = 0; k < SIZEOF_UUID128; k++) {
+				if (uuid128[i*SIZEOF_UUID128 + k] !=
+					rec->svclass.value.uuid128.data[SIZEOF_UUID128 - k])
+					break;
+			}
+			if (k == SIZEOF_UUID128)
+				break;
+		}
+
+		if (i < index)
+			continue;
+
+		/* EIR data is Little Endian */
+		for (k = 0; k < SIZEOF_UUID128; k++)
+			uuid128[index*SIZEOF_UUID128 + k] =
+				rec->svclass.value.uuid128.data[SIZEOF_UUID128 - 1 - k];
+
+		len += SIZEOF_UUID128;
+		index++;
+	}
+
+	if (index > 0 || truncated) {
+		/* EIR Data length */
+		ptr[0] = (index * SIZEOF_UUID128) + 1;
+		/* EIR Data type */
+		ptr[1] = (truncated) ? EIR_UUID128_SOME : EIR_UUID128_ALL;
+		len += 2;
+		*eir_len = len;
+	}
+}
+
 void create_ext_inquiry_response(const char *name,
 					int8_t tx_power, sdp_list_t *services,
 					uint8_t *data)
 {
 	sdp_list_t *list = services;
 	uint8_t *ptr = data;
-	uint16_t uuid[24];
+	uint16_t eir_len = 0;
+	uint16_t uuid16[EIR_DATA_LENGTH/2];
 	int i, index = 0;
+	gboolean truncated = FALSE;
 
 	if (name) {
 		int len = strlen(name);
 
+		/* EIR Data type */
 		if (len > 48) {
 			len = 48;
-			ptr[1] = 0x08;
+			ptr[1] = EIR_NAME_SHORT;
 		} else
-			ptr[1] = 0x09;
+			ptr[1] = EIR_NAME_COMPLETE;
 
+		/* EIR Data length */
 		ptr[0] = len + 1;
 
 		memcpy(ptr + 2, name, len);
 
-		ptr += len + 2;
+		eir_len += (len + 2);
+		ptr += (len + 2);
 	}
 
 	if (tx_power != 0) {
 		*ptr++ = 2;
-		*ptr++ = 0x0a;
+		*ptr++ = EIR_TX_POWER;
 		*ptr++ = (uint8_t) tx_power;
+		eir_len += 3;
 	}
 
 	if (did_vendor != 0x0000) {
 		uint16_t source = 0x0002;
 		*ptr++ = 9;
-		*ptr++ = 0x10;
+		*ptr++ = EIR_DEVICE_ID;
 		*ptr++ = (source & 0x00ff);
 		*ptr++ = (source & 0xff00) >> 8;
 		*ptr++ = (did_vendor & 0x00ff);
@@ -217,10 +281,10 @@ void create_ext_inquiry_response(const char *name,
 		*ptr++ = (did_product & 0xff00) >> 8;
 		*ptr++ = (did_version & 0x00ff);
 		*ptr++ = (did_version & 0xff00) >> 8;
+		eir_len += 10;
 	}
 
-	ptr[1] = 0x03;
-
+	/* Group all UUID16 types */
 	for (; list; list = list->next) {
 		sdp_record_t *rec = (sdp_record_t *) list->data;
 
@@ -233,30 +297,42 @@ void create_ext_inquiry_response(const char *name,
 		if (rec->svclass.value.uuid16 == PNP_INFO_SVCLASS_ID)
 			continue;
 
-		if (index > 23) {
-			ptr[1] = 0x02;
+		/* Stop if not enough space to put next UUID16 */
+		if ((eir_len + 2 + sizeof(uint16_t)) > EIR_DATA_LENGTH) {
+			truncated = TRUE;
 			break;
 		}
 
+		/* Check for duplicates */
 		for (i = 0; i < index; i++)
-			if (uuid[i] == rec->svclass.value.uuid16)
+			if (uuid16[i] == rec->svclass.value.uuid16)
 				break;
 
-		if (i == index - 1)
+		if (i < index)
 			continue;
 
-		uuid[index++] = rec->svclass.value.uuid16;
+		uuid16[index++] = rec->svclass.value.uuid16;
+		eir_len += sizeof(uint16_t);
 	}
 
 	if (index > 0) {
-		ptr[0] = (index * 2) + 1;
+		/* EIR Data length */
+		ptr[0] = (index * sizeof(uint16_t)) + 1;
+		/* EIR Data type */
+		ptr[1] = (truncated) ? EIR_UUID16_SOME : EIR_UUID16_ALL;
+
 		ptr += 2;
+		eir_len += 2;
 
 		for (i = 0; i < index; i++) {
-			*ptr++ = (uuid[i] & 0x00ff);
-			*ptr++ = (uuid[i] & 0xff00) >> 8;
+			*ptr++ = (uuid16[i] & 0x00ff);
+			*ptr++ = (uuid16[i] & 0xff00) >> 8;
 		}
 	}
+
+	/* Group all UUID128 types */
+	if (eir_len <= EIR_DATA_LENGTH - 2)
+		eir_generate_uuid128(services, ptr, &eir_len);
 }
 
 void register_public_browse_group(void)
-- 
1.7.1

-- 
Inga Stotland
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

Re: [PATCH 2/5] Support for adding UUID128 to extended inquiry response

[Luiz Augusto von Dentz]

Hi,

On Thu, Jul 22, 2010 at 7:06 PM, Inga Stotland <ingas@codeaurora.org> wrote=
:
> ---
> =A0src/sdpd-service.c | =A0108 ++++++++++++++++++++++++++++++++++++++++++=
++--------
> =A01 files changed, 92 insertions(+), 16 deletions(-)
>
> diff --git a/src/sdpd-service.c b/src/sdpd-service.c
> index cdbb4f4..1314ada 100644
> --- a/src/sdpd-service.c
> +++ b/src/sdpd-service.c
> @@ -49,6 +49,8 @@
> =A0#include "manager.h"
> =A0#include "adapter.h"
>
> +#define SIZEOF_UUID128 16
> +
> =A0static sdp_record_t *server =3D NULL;
>
> =A0static uint16_t did_vendor =3D 0x0000;
> @@ -174,41 +176,103 @@ static void update_svclass_list(const bdaddr_t *sr=
c)
>
> =A0}
>
> +static void eir_generate_uuid128(sdp_list_t *list,
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=
 =A0 uint8_t *ptr, uint16_t *eir_len)
> +{
> + =A0 =A0 =A0 int i, k, index =3D 0;
> + =A0 =A0 =A0 uint16_t len =3D *eir_len;
> + =A0 =A0 =A0 uint8_t *uuid128;
> + =A0 =A0 =A0 gboolean truncated =3D FALSE;
> +
> + =A0 =A0 =A0 /* Store UUID128 in place, skip 2 bytes to insert type and =
length later */
> + =A0 =A0 =A0 uuid128 =3D ptr + 2;
> +
> + =A0 =A0 =A0 for (; list; list =3D list->next) {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 sdp_record_t *rec =3D (sdp_record_t *) list=
->data;
> +
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (rec->svclass.type !=3D SDP_UUID128)
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 continue;
> +
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 /* Stop if not enough space to put next UUI=
D128 */
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 if ((len + 2 + SIZEOF_UUID128) > EIR_DATA_L=
ENGTH) {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 truncated =3D TRUE;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 break;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 }
> +
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 /* Check for duplicates, EIR data is Little=
 Endian */
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 for (i =3D 0; i < index; i++) {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 for (k =3D 0; k < SIZEOF_UU=
ID128; k++) {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (uuid128=
[i*SIZEOF_UUID128 + k] !=3D
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=
 =A0 rec->svclass.value.uuid128.data[SIZEOF_UUID128 - k])
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=
 =A0 break;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 }
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (k =3D=3D SIZEOF_UUID128=
)
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 break;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 }
> +
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (i < index)
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 continue;
> +
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 /* EIR data is Little Endian */
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 for (k =3D 0; k < SIZEOF_UUID128; k++)
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 uuid128[index*SIZEOF_UUID12=
8 + k] =3D
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 rec->svclas=
s.value.uuid128.data[SIZEOF_UUID128 - 1 - k];
> +
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 len +=3D SIZEOF_UUID128;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 index++;
> + =A0 =A0 =A0 }
> +
> + =A0 =A0 =A0 if (index > 0 || truncated) {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 /* EIR Data length */
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 ptr[0] =3D (index * SIZEOF_UUID128) + 1;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 /* EIR Data type */
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 ptr[1] =3D (truncated) ? EIR_UUID128_SOME :=
 EIR_UUID128_ALL;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 len +=3D 2;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 *eir_len =3D len;
> + =A0 =A0 =A0 }
> +}
> +

I think you should probably split this in two parts, first update with
the cleanups and then add the code to handle uuid 128.

> =A0void create_ext_inquiry_response(const char *name,
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
=A0 =A0int8_t tx_power, sdp_list_t *services,
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
=A0 =A0uint8_t *data)
> =A0{
> =A0 =A0 =A0 =A0sdp_list_t *list =3D services;
> =A0 =A0 =A0 =A0uint8_t *ptr =3D data;
> - =A0 =A0 =A0 uint16_t uuid[24];
> + =A0 =A0 =A0 uint16_t eir_len =3D 0;
> + =A0 =A0 =A0 uint16_t uuid16[EIR_DATA_LENGTH/2];
> =A0 =A0 =A0 =A0int i, index =3D 0;
> + =A0 =A0 =A0 gboolean truncated =3D FALSE;
>
> =A0 =A0 =A0 =A0if (name) {
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0int len =3D strlen(name);
>
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 /* EIR Data type */
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0if (len > 48) {
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0len =3D 48;
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 ptr[1] =3D 0x08;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 ptr[1] =3D EIR_NAME_SHORT;
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0} else
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 ptr[1] =3D 0x09;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 ptr[1] =3D EIR_NAME_COMPLET=
E;
>
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 /* EIR Data length */
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0ptr[0] =3D len + 1;
>
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0memcpy(ptr + 2, name, len);
>
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 ptr +=3D len + 2;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 eir_len +=3D (len + 2);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 ptr +=3D (len + 2);
> =A0 =A0 =A0 =A0}
>
> =A0 =A0 =A0 =A0if (tx_power !=3D 0) {
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0*ptr++ =3D 2;
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 *ptr++ =3D 0x0a;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 *ptr++ =3D EIR_TX_POWER;
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0*ptr++ =3D (uint8_t) tx_power;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 eir_len +=3D 3;
> =A0 =A0 =A0 =A0}
>
> =A0 =A0 =A0 =A0if (did_vendor !=3D 0x0000) {
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0uint16_t source =3D 0x0002;
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0*ptr++ =3D 9;
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 *ptr++ =3D 0x10;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 *ptr++ =3D EIR_DEVICE_ID;
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0*ptr++ =3D (source & 0x00ff);
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0*ptr++ =3D (source & 0xff00) >> 8;
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0*ptr++ =3D (did_vendor & 0x00ff);
> @@ -217,10 +281,10 @@ void create_ext_inquiry_response(const char *name,
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0*ptr++ =3D (did_product & 0xff00) >> 8;
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0*ptr++ =3D (did_version & 0x00ff);
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0*ptr++ =3D (did_version & 0xff00) >> 8;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 eir_len +=3D 10;
> =A0 =A0 =A0 =A0}
>
> - =A0 =A0 =A0 ptr[1] =3D 0x03;
> -
> + =A0 =A0 =A0 /* Group all UUID16 types */
> =A0 =A0 =A0 =A0for (; list; list =3D list->next) {
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0sdp_record_t *rec =3D (sdp_record_t *) lis=
t->data;
>
> @@ -233,30 +297,42 @@ void create_ext_inquiry_response(const char *name,
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0if (rec->svclass.value.uuid16 =3D=3D PNP_I=
NFO_SVCLASS_ID)
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0continue;
>
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (index > 23) {
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 ptr[1] =3D 0x02;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 /* Stop if not enough space to put next UUI=
D16 */
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 if ((eir_len + 2 + sizeof(uint16_t)) > EIR_=
DATA_LENGTH) {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 truncated =3D TRUE;
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0break;
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0}
>
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 /* Check for duplicates */
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0for (i =3D 0; i < index; i++)
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (uuid[i] =3D=3D rec->svc=
lass.value.uuid16)
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (uuid16[i] =3D=3D rec->s=
vclass.value.uuid16)
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0break;
>
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (i =3D=3D index - 1)
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (i < index)
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0continue;
>
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 uuid[index++] =3D rec->svclass.value.uuid16=
;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 uuid16[index++] =3D rec->svclass.value.uuid=
16;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 eir_len +=3D sizeof(uint16_t);
> =A0 =A0 =A0 =A0}
>
> =A0 =A0 =A0 =A0if (index > 0) {
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 ptr[0] =3D (index * 2) + 1;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 /* EIR Data length */
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 ptr[0] =3D (index * sizeof(uint16_t)) + 1;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 /* EIR Data type */
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 ptr[1] =3D (truncated) ? EIR_UUID16_SOME : =
EIR_UUID16_ALL;
> +
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0ptr +=3D 2;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 eir_len +=3D 2;
>
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0for (i =3D 0; i < index; i++) {
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 *ptr++ =3D (uuid[i] & 0x00f=
f);
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 *ptr++ =3D (uuid[i] & 0xff0=
0) >> 8;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 *ptr++ =3D (uuid16[i] & 0x0=
0ff);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 *ptr++ =3D (uuid16[i] & 0xf=
f00) >> 8;
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0}
> =A0 =A0 =A0 =A0}
> +
> + =A0 =A0 =A0 /* Group all UUID128 types */
> + =A0 =A0 =A0 if (eir_len <=3D EIR_DATA_LENGTH - 2)
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 eir_generate_uuid128(services, ptr, &eir_le=
n);
> =A0}
>
> =A0void register_public_browse_group(void)
> --
> 1.7.1

Also this checks for duplicates looks suspicious, does it really need
to check that in place and every time, I think it might be more
efficient if we do that before generating the EIR data avoiding
comparing data with different endianess or maybe it just make more
sense to not allow duplicates in the database then we don't have to do
this at all.

--=20
Luiz Augusto von Dentz
Computer Engineer

Re: [PATCH 2/5] Support for adding UUID128 to extended inquiry response

[ingas]

Hi Luiz,

> Hi,
>
> On Thu, Jul 22, 2010 at 7:06 PM, Inga Stotland <ingas@codeaurora.org>
> wrote:
>> ---
>>  src/sdpd-service.c |  108
>> ++++++++++++++++++++++++++++++++++++++++++++--------
>>  1 files changed, 92 insertions(+), 16 deletions(-)
>>
>> diff --git a/src/sdpd-service.c b/src/sdpd-service.c
>> index cdbb4f4..1314ada 100644
>> --- a/src/sdpd-service.c
>> +++ b/src/sdpd-service.c
>> @@ -49,6 +49,8 @@
>>  #include "manager.h"
>>  #include "adapter.h"
>
>> +
>
> I think you should probably split this in two parts, first update with
> the cleanups and then add the code to handle uuid 128.
>

Sure, I can split this in more patches.

>>  void create_ext_inquiry_response(const char *name,
>>                                        int8_t tx_power, sdp_list_t
>> *services,
>>                                        uint8_t *data)
>>
>> +               /* Check for duplicates */
>>                for (i = 0; i < index; i++)
>> -                       if (uuid[i] == rec->svclass.value.uuid16)
>> +                       if (uuid16[i] == rec->svclass.value.uuid16)
>>                                break;
>>
>> -               if (i == index - 1)
>> +               if (i < index)
>>                        continue;
>>

>
> Also this checks for duplicates looks suspicious, does it really need
> to check that in place and every time, I think it might be more
> efficient if we do that before generating the EIR data avoiding
> comparing data with different endianess or maybe it just make more
> sense to not allow duplicates in the database then we don't have to do
> this at all.
>

This check for duplicates was in place prior to my modifications. I merely
added a comment and fixed an existing problem.

If you examine the patch closely, you might notice that the previous
"check for duplicates" implementation was doing the following:
- cycling through the list of so far accumulated UUIDs in EIR and
comparing them to the current one. If duplicate is found, break out of
this cycling loop. Notice, that in the case of a duplicate, (i <
index)condition will be true, where index is the number of accumulated
UUIDs.
- the current implementation performs check for (i == index -1). Well, we
did not need to cycle through the whole array to cmpare to the last item
:) But if my reading of the original intent is correct, we don't want to
put a duplicate UUID in EIR, and, therefore, should go to next iteration
if a duplicate is found. Hence, the check should be (i < index).

Maybe it's a better idea to submit this fix separately.

I do not disagree that it would be better to check for duplicate records
elsewhere, but this  is outside the scope of this patch.

Regards,

Inga

Re: [PATCH 2/5] Support for adding UUID128 to extended inquiry response

[Luiz Augusto von Dentz]

Hi,

On Tue, Jul 27, 2010 at 11:08 PM,  <ingas@codeaurora.org> wrote:
> If you examine the patch closely, you might notice that the previous
> "check for duplicates" implementation was doing the following:
> - cycling through the list of so far accumulated UUIDs in EIR and
> comparing them to the current one. If duplicate is found, break out of
> this cycling loop. Notice, that in the case of a duplicate, (i <
> index)condition will be true, where index is the number of accumulated
> UUIDs.
> - the current implementation performs check for (i =3D=3D index -1). Well=
, we
> did not need to cycle through the whole array to cmpare to the last item
> :) But if my reading of the original intent is correct, we don't want to
> put a duplicate UUID in EIR, and, therefore, should go to next iteration
> if a duplicate is found. Hence, the check should be (i < index).

Yep, the check was already there before you touch it.

> Maybe it's a better idea to submit this fix separately.
>
> I do not disagree that it would be better to check for duplicate records
> elsewhere, but this =A0is outside the scope of this patch.

No problem, the other patches seems to be in good shape to be push
upstream, so lets try to do it this week.

--=20
Luiz Augusto von Dentz
Computer Engineer

[PATCH 3/5] Update EIR whenever record is added or removed

[Inga Stotland]

---
 plugins/service.c |   18 ++++++++++++++++++
 src/adapter.c     |   23 ++++++++++++++++++++++-
 src/adapter.h     |    1 +
 3 files changed, 41 insertions(+), 1 deletions(-)

diff --git a/plugins/service.c b/plugins/service.c
index 96280bd..ee9176b 100644
--- a/plugins/service.c
+++ b/plugins/service.c
@@ -445,6 +445,8 @@ static DBusMessage *update_record(DBusConnection *conn, DBusMessage *msg,
 				strerror(EIO));
 	}
 
+	adapter_update_eir_data(&src);
+
 	return dbus_message_new_method_return(msg);
 }
 
@@ -516,6 +518,7 @@ static DBusMessage *add_service_record(DBusConnection *conn,
 	const char *sender, *record;
 	dbus_uint32_t handle;
 	int err;
+	bdaddr_t bdaddr;
 
 	if (dbus_message_get_args(msg, NULL,
 			DBUS_TYPE_STRING, &record, DBUS_TYPE_INVALID) == FALSE)
@@ -526,6 +529,13 @@ static DBusMessage *add_service_record(DBusConnection *conn,
 	if (err < 0)
 		return failed_strerror(msg, err);
 
+	if (serv_adapter->adapter)
+		adapter_get_address(serv_adapter->adapter, &bdaddr);
+	else
+		bacpy(&bdaddr, BDADDR_ANY);
+
+	adapter_update_eir_data(&bdaddr);
+
 	reply = dbus_message_new_method_return(msg);
 	if (!reply)
 		return NULL;
@@ -550,6 +560,7 @@ static DBusMessage *remove_service_record(DBusConnection *conn,
 	struct service_adapter *serv_adapter = data;
 	dbus_uint32_t handle;
 	const char *sender;
+	bdaddr_t bdaddr;
 
 	if (dbus_message_get_args(msg, NULL, DBUS_TYPE_UINT32, &handle,
 						DBUS_TYPE_INVALID) == FALSE)
@@ -560,6 +571,13 @@ static DBusMessage *remove_service_record(DBusConnection *conn,
 	if (remove_record(conn, sender, serv_adapter, handle) < 0)
 		return not_available(msg);
 
+	if (serv_adapter->adapter)
+		adapter_get_address(serv_adapter->adapter, &bdaddr);
+	else
+		bacpy(&bdaddr, BDADDR_ANY);
+
+	adapter_update_eir_data(&bdaddr);
+
 	return dbus_message_new_method_return(msg);
 }
 
diff --git a/src/adapter.c b/src/adapter.c
index 789a196..c1e1768 100644
--- a/src/adapter.c
+++ b/src/adapter.c
@@ -820,7 +820,7 @@ static DBusMessage *set_pairable_timeout(DBusConnection *conn,
 
 static void update_ext_inquiry_response(struct btd_adapter *adapter)
 {
-	uint8_t fec = 0, data[240];
+	uint8_t fec = 0, data[EIR_DATA_LENGTH];
 	struct hci_dev *dev = &adapter->dev;
 	int dd;
 
@@ -846,6 +846,27 @@ static void update_ext_inquiry_response(struct btd_adapter *adapter)
 	hci_close_dev(dd);
 }
 
+void adapter_update_eir_data(const bdaddr_t *src)
+{
+	struct btd_adapter *adapter;
+	GSList *adapters;
+
+	if (bacmp(src, BDADDR_ANY) != 0) {
+		adapter = manager_find_adapter(src);
+		if (adapter)
+			update_ext_inquiry_response(adapter);
+		else
+			error("Updating EIR failed: device not found");
+	} else {
+
+		/* Update extended inquiry reponse for ANY adapter */
+		for (adapters = manager_get_adapters(); adapters; adapters = adapters->next) {
+			adapter = adapters->data;
+			update_ext_inquiry_response(adapter);
+		}
+	}
+}
+
 void adapter_set_class_complete(bdaddr_t *bdaddr, uint8_t status)
 {
 	uint8_t class[3];
diff --git a/src/adapter.h b/src/adapter.h
index 8226514..f72eb0b 100644
--- a/src/adapter.h
+++ b/src/adapter.h
@@ -126,6 +126,7 @@ void adapter_service_insert(const bdaddr_t *bdaddr, void *rec);
 void adapter_service_remove(const bdaddr_t *bdaddr, void *rec);
 sdp_list_t *adapter_get_services(struct btd_adapter *adapter);
 void adapter_set_class_complete(bdaddr_t *bdaddr, uint8_t status);
+void adapter_update_eir_data(const bdaddr_t *src);
 
 struct agent *adapter_get_agent(struct btd_adapter *adapter);
 void adapter_add_connection(struct btd_adapter *adapter,
-- 
1.7.1

-- 
Inga Stotland
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

[PATCH 4/5] Handle arrays in device properties dictionary

[Inga Stotland]

---
 src/adapter.c |    8 +++++++-
 1 files changed, 7 insertions(+), 1 deletions(-)

diff --git a/src/adapter.c b/src/adapter.c
index c1e1768..b22b086 100644
--- a/src/adapter.c
+++ b/src/adapter.c
@@ -2715,6 +2715,7 @@ static void append_dict_valist(DBusMessageIter *iter,
 	DBusMessageIter dict;
 	const char *key;
 	int type;
+	int n_elements;
 	void *val;
 
 	dbus_message_iter_open_container(iter, DBUS_TYPE_ARRAY,
@@ -2726,7 +2727,12 @@ static void append_dict_valist(DBusMessageIter *iter,
 	while (key) {
 		type = va_arg(var_args, int);
 		val = va_arg(var_args, void *);
-		dict_append_entry(&dict, key, type, val);
+		if (type == DBUS_TYPE_ARRAY) {
+			n_elements = va_arg(var_args, int);
+			if (n_elements > 0)
+				dict_append_array(&dict, key, DBUS_TYPE_STRING, val, n_elements);
+		} else
+			dict_append_entry(&dict, key, type, val);
 		key = va_arg(var_args, char *);
 	}
 
-- 
1.7.1

-- 
Inga Stotland
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

[PATCH 5/5] Extended support for generating dictionary value of service UUIDs

[Inga Stotland]

---
 src/adapter.c  |   95 ++++++++++++++++++++++++++++++++++++++++++++++++++++++--
 src/adapter.h  |    4 +-
 src/dbus-hci.c |    6 ++--
 3 files changed, 97 insertions(+), 8 deletions(-)

diff --git a/src/adapter.c b/src/adapter.c
index b22b086..2438c29 100644
--- a/src/adapter.c
+++ b/src/adapter.c
@@ -2763,8 +2763,84 @@ static void emit_device_found(const char *path, const char *address,
 	g_dbus_send_message(connection, signal);
 }
 
+static int get_uuid_count_eir(uint8_t *eir_data)
+{
+	uint8_t len = 0;
+	int count = 0;
+
+	while (len < EIR_DATA_LENGTH) {
+		uint8_t type = eir_data[1];
+		uint8_t field_len = eir_data[0];
+		if (type == EIR_UUID16_SOME || type == EIR_UUID16_ALL)
+			count += field_len/2;
+		else if (type == EIR_UUID32_SOME || type == EIR_UUID32_ALL)
+			count += field_len/4;
+		else if (type == EIR_UUID128_SOME || type == EIR_UUID128_ALL)
+			count += field_len/16;
+		len += field_len + 1;
+		eir_data += field_len + 1;
+	}
+
+	return count;
+}
+
+static void get_uuids_eir(char **uuids, uint8_t *eir_data)
+{
+	uint8_t len = 0;
+
+	/* Count UUID16, UUID32 and UUID128 */
+	while (len < EIR_DATA_LENGTH) {
+		uint8_t field_len = eir_data[0];
+		uint8_t type = eir_data[1];
+		int count;
+		uuid_t service;
+		int size;
+		uint8_t *data = &eir_data[2];
+		int i, k;
+
+		/* Generate uuids in SDP format (EIR data is Little Endian) */
+		if (type == EIR_UUID16_SOME || type == EIR_UUID16_ALL) {
+			size = 2;
+			count = field_len/size;
+			service.type = SDP_UUID16;
+			for (i = 0; i < count; i++) {
+				uint16_t val16 = data[1];
+				val16 = (val16<<8) + data[0];
+				service.value.uuid16 = val16;
+				*uuids++ = bt_uuid2string(&service);
+				data += size;
+			}
+		} else if (type == EIR_UUID32_SOME || type == EIR_UUID32_ALL) {
+			size = 4;
+			count = field_len/size;
+			service.type = SDP_UUID32;
+			for (i = 0; i < count; i++) {
+				uint32_t val32 = data[3];
+				for (k = size-2; k >= 0; k--)
+					val32 = (val32<<8) + data[k];
+				service.value.uuid32 = val32;
+				*uuids++ = bt_uuid2string(&service);
+				data += size;
+			}
+		} else if (type == EIR_UUID128_SOME || type == EIR_UUID128_ALL) {
+			size = 16;
+			count = field_len/size;
+			service.type = SDP_UUID128;
+			for (i = 0; i < count; i++) {
+				for (k = 0; k < size; k++)
+					service.value.uuid128.data[k] = data[size-k-1];
+				*uuids++ = bt_uuid2string(&service);
+				data += size;
+			}
+		}
+
+		len += field_len + 1;
+		eir_data += field_len + 1;
+	}
+}
+
 void adapter_emit_device_found(struct btd_adapter *adapter,
-				struct remote_dev_info *dev)
+				struct remote_dev_info *dev, uint8_t *eir_data)
 {
 	struct btd_device *device;
 	char peer_addr[18], local_addr[18];
@@ -2772,6 +2848,8 @@ void adapter_emit_device_found(struct btd_adapter *adapter,
 	dbus_bool_t paired = FALSE;
 	dbus_int16_t rssi = dev->rssi;
 	char *alias;
+	char **uuids = NULL;
+	int uuid_count = 0;
 
 	ba2str(&dev->bdaddr, peer_addr);
 	ba2str(&adapter->bdaddr, local_addr);
@@ -2791,6 +2869,15 @@ void adapter_emit_device_found(struct btd_adapter *adapter,
 	} else
 		alias = g_strdup(dev->alias);
 
+	/* Extract UUIDs from extended inquiry response if any*/
+	if (eir_data != NULL)
+		uuid_count = get_uuid_count_eir(eir_data);
+
+	if (uuid_count > 0) {
+		uuids = g_new0(char *, uuid_count + 1);
+		get_uuids_eir(uuids, eir_data);
+	}
+
 	emit_device_found(adapter->path, paddr,
 			"Address", DBUS_TYPE_STRING, &paddr,
 			"Class", DBUS_TYPE_UINT32, &dev->class,
@@ -2800,15 +2887,17 @@ void adapter_emit_device_found(struct btd_adapter *adapter,
 			"Alias", DBUS_TYPE_STRING, &alias,
 			"LegacyPairing", DBUS_TYPE_BOOLEAN, &dev->legacy,
 			"Paired", DBUS_TYPE_BOOLEAN, &paired,
+			"UUIDs", DBUS_TYPE_ARRAY, &uuids, uuid_count,
 			NULL);
 
 	g_free(alias);
+	g_strfreev(uuids);
 }
 
 void adapter_update_found_devices(struct btd_adapter *adapter, bdaddr_t *bdaddr,
 				int8_t rssi, uint32_t class, const char *name,
 				const char *alias, gboolean legacy,
-				name_status_t name_status)
+				name_status_t name_status, uint8_t *eir_data)
 {
 	struct remote_dev_info *dev, match;
 
@@ -2847,7 +2936,7 @@ done:
 	adapter->found_devices = g_slist_sort(adapter->found_devices,
 						(GCompareFunc) dev_rssi_cmp);
 
-	adapter_emit_device_found(adapter, dev);
+	adapter_emit_device_found(adapter, dev, eir_data);
 }
 
 int adapter_remove_found_device(struct btd_adapter *adapter, bdaddr_t *bdaddr)
diff --git a/src/adapter.h b/src/adapter.h
index f72eb0b..231d2c9 100644
--- a/src/adapter.h
+++ b/src/adapter.h
@@ -113,10 +113,10 @@ struct remote_dev_info *adapter_search_found_devices(struct btd_adapter *adapter
 void adapter_update_found_devices(struct btd_adapter *adapter, bdaddr_t *bdaddr,
 				int8_t rssi, uint32_t class, const char *name,
 				const char *alias, gboolean legacy,
-				name_status_t name_status);
+				name_status_t name_status, uint8_t *eir_data);
 int adapter_remove_found_device(struct btd_adapter *adapter, bdaddr_t *bdaddr);
 void adapter_emit_device_found(struct btd_adapter *adapter,
-				struct remote_dev_info *dev);
+				struct remote_dev_info *dev, uint8_t *eir_data);
 void adapter_update_oor_devices(struct btd_adapter *adapter);
 void adapter_mode_changed(struct btd_adapter *adapter, uint8_t scan_mode);
 void adapter_setname_complete(bdaddr_t *local, uint8_t status);
diff --git a/src/dbus-hci.c b/src/dbus-hci.c
index b83506f..6d27caa 100644
--- a/src/dbus-hci.c
+++ b/src/dbus-hci.c
@@ -515,7 +515,7 @@ void hcid_dbus_inquiry_result(bdaddr_t *local, bdaddr_t *peer, uint32_t class,
 	if (dev) {
 		adapter_update_found_devices(adapter, peer, rssi, class,
 						NULL, NULL, dev->legacy,
-						NAME_NOT_REQUIRED);
+						NAME_NOT_REQUIRED, data);
 		return;
 	}
 
@@ -566,7 +566,7 @@ void hcid_dbus_inquiry_result(bdaddr_t *local, bdaddr_t *peer, uint32_t class,
 
 	/* add in the list to track name sent/pending */
 	adapter_update_found_devices(adapter, peer, rssi, class, name, alias,
-					legacy, name_status);
+					legacy, name_status, data);
 
 	g_free(name);
 	g_free(alias);
@@ -642,7 +642,7 @@ void hcid_dbus_remote_name(bdaddr_t *local, bdaddr_t *peer, uint8_t status,
 	if (dev_info) {
 		g_free(dev_info->name);
 		dev_info->name = g_strdup(name);
-		adapter_emit_device_found(adapter, dev_info);
+		adapter_emit_device_found(adapter, dev_info, NULL);
 	}
 
 	if (device)
-- 
1.7.1

-- 
Inga Stotland
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

Re: [PATCH v4 0/5] Enhanced support for extended inquiry response

[Johan Hedberg]

Hi Inga,

On Thu, Jul 22, 2010, Inga Stotland wrote:
> EIR is updated whenever local SDP record database changes.

Could you explain to me why this is needed. With the current git
(without your patches) the local EIR data seems to be correctly updated
whenever the SDP database changes. Are there some scenarios when this
doesn't happen? I did a quick test with sdptool add/del while bluetoothd
was running and I could see the appropriate "Write Extended Inquiry
Response" commands with hcidump.

Johan

Re: [PATCH v4 0/5] Enhanced support for extended inquiry response

[Johan Hedberg]

On Fri, Jul 23, 2010, Johan Hedberg wrote:
> Could you explain to me why this is needed. With the current git
> (without your patches) the local EIR data seems to be correctly updated
> whenever the SDP database changes. Are there some scenarios when this
> doesn't happen? I did a quick test with sdptool add/del while bluetoothd
> was running and I could see the appropriate "Write Extended Inquiry
> Response" commands with hcidump.

It seems currently this only happens when a local record change causes
the service class bits of the class of device to be updated too. If the
record change doesn't affect the class of device then it seems the EIR
data isn't modified accordingly. Is this correct? If so, could you
please add that information to your commit message so that it's clear
why the patch is needed.

Johan

Re: [PATCH v4 0/5] Enhanced support for extended inquiry response

[Johan Hedberg]

Hi,

On Fri, Jul 23, 2010, Johan Hedberg wrote:
> On Fri, Jul 23, 2010, Johan Hedberg wrote:
> > Could you explain to me why this is needed. With the current git
> > (without your patches) the local EIR data seems to be correctly updated
> > whenever the SDP database changes. Are there some scenarios when this
> > doesn't happen? I did a quick test with sdptool add/del while bluetoothd
> > was running and I could see the appropriate "Write Extended Inquiry
> > Response" commands with hcidump.
> 
> It seems currently this only happens when a local record change causes
> the service class bits of the class of device to be updated too. If the
> record change doesn't affect the class of device then it seems the EIR
> data isn't modified accordingly. Is this correct? If so, could you
> please add that information to your commit message so that it's clear
> why the patch is needed.

Even with your patch it seems this still doesn't work. If I I don't have
any OBEX servers running and I do "sdptool add OPUSH" I get the
apropriate EIR update with the OPP UUID (0x1105) added to the list, but
if I do a consecutive "sdptool add FTP" 0x1106 doesn't get added even
though it should. Then when I do sdptool del for the OPP record the UUID
doesn't get removed from the EIR data. So it seems some things still
need fixing.

Also, could you somehow try to avoid the multiple "Write Extended
Inquiry Response" commands while bluetoothd is probing drivers for an
adapter that entered the UP state? With the class of device this is done
through adapter->cache_enable, so maybe some more generic system could
be used for all HCI trafic related to probing drivers.

Johan

Re: [PATCH v4 0/5] Enhanced support for extended inquiry response

[ingas]

Hi Johan,

> Hi,
>
> On Fri, Jul 23, 2010, Johan Hedberg wrote:
>> On Fri, Jul 23, 2010, Johan Hedberg wrote:
>> > Could you explain to me why this is needed. With the current git
>> > (without your patches) the local EIR data seems to be correctly
>> updated
>> > whenever the SDP database changes. Are there some scenarios when this
>> > doesn't happen? I did a quick test with sdptool add/del while
>> bluetoothd
>> > was running and I could see the appropriate "Write Extended Inquiry
>> > Response" commands with hcidump.
>>
>> It seems currently this only happens when a local record change causes
>> the service class bits of the class of device to be updated too. If the
>> record change doesn't affect the class of device then it seems the EIR
>> data isn't modified accordingly. Is this correct? If so, could you
>> please add that information to your commit message so that it's clear
>> why the patch is needed.
>
> Even with your patch it seems this still doesn't work. If I I don't have
> any OBEX servers running and I do "sdptool add OPUSH" I get the
> apropriate EIR update with the OPP UUID (0x1105) added to the list, but
> if I do a consecutive "sdptool add FTP" 0x1106 doesn't get added even
> though it should. Then when I do sdptool del for the OPP record the UUID
> doesn't get removed from the EIR data. So it seems some things still
> need fixing.
>
> Also, could you somehow try to avoid the multiple "Write Extended
> Inquiry Response" commands while bluetoothd is probing drivers for an
> adapter that entered the UP state? With the class of device this is done
> through adapter->cache_enable, so maybe some more generic system could
> be used for all HCI trafic related to probing drivers.
>
> Johan
>

Thank you for your comments.

It seems my comment for this patch submisiion was not specific enough (my
apologies). The extended support for EIR update was meant for the cases
when d-bus methods "AddRecord", "RemoveRecord" and "Update Record" were
called. I did verify the problems that you indicated in EIR update when
using sdptool. However, this is a pre-existing condition not related to
the extensions that I submitted. sdptool uses different mechanism for EIR
update that was not affected by my changes. That needs to be fixed in a
separate patch set, IMO.

Testing of EIR being updated when calling the above mentioned d-bus
methods can be done using d-feet interface to call the methods with
subsequent call to hcitool to execute "Read EIR" command ($hcitool -i
hci<n> cmd 0x03 0x0051).

I do need to change the comments on the patches to expalin better what has
been added. Will that be sufficient?

Regards,

Inga

Re: [PATCH v4 0/5] Enhanced support for extended inquiry response

[Johan Hedberg]

Hi Inga,

On Sun, Jul 25, 2010, ingas@codeaurora.org wrote:
> It seems my comment for this patch submisiion was not specific enough (my
> apologies). The extended support for EIR update was meant for the cases
> when d-bus methods "AddRecord", "RemoveRecord" and "Update Record" were
> called. I did verify the problems that you indicated in EIR update when
> using sdptool. However, this is a pre-existing condition not related to
> the extensions that I submitted. sdptool uses different mechanism for EIR
> update that was not affected by my changes. That needs to be fixed in a
> separate patch set, IMO.

Both the D-Bus interface as well as sdptool trigger the same code paths
in src/sdpd-service.c. I.e. it doesn't make sense to solve this twice
when it can be solved once by targetting the common lower layer. We
already do this for the service class bits with update_svclass_list() in
sdpd-service.c. The need to update the EIR data is very similar to the
need to update the service class bits so the existing code is probably a
good place to take example of.

And yes, please make your commit messages more verbose. It's far less of
a crime to have too verbose messages than not having them descriptive
enough: it doesn't just help the reviewers right now in understanding
the purpose of the patches but will also help anybody looking through
the commit history a few years from now when the details of the matter
have been forgotten.

Johan

Re: [PATCH v4 0/5] Enhanced support for extended inquiry response

[ingas]

Hi Johan,

>
> Both the D-Bus interface as well as sdptool trigger the same code paths
> in src/sdpd-service.c. I.e. it doesn't make sense to solve this twice
> when it can be solved once by targetting the common lower layer. We
> already do this for the service class bits with update_svclass_list() in
> sdpd-service.c. The need to update the EIR data is very similar to the
> need to update the service class bits so the existing code is probably a
> good place to take example of.
>
>

My original intent when creating this patch set was no to perturb the code
that is already  in place too much.  Hence the changes were done only to
D-Bus methods. Sure, I can change the EIR write bot happen deeper down the
pipeline. However, before I invest your and my time in generating and
reviewing more patches , let’s agree on the solution first.

In the current implementation,  EIR write is keyed off HCI Command
Complete events for a set of commands (in security.c).
 - When first registering an OPP service with sdptool, the
update_svcclass_list()  (sdpd-service.c) calls manager_update_svc()
(manager.c) which in turn calls adapter_update()  (adapter.c).  BTW, why
this level of indirection, since manager_update_svc() is called only from
one place in the code and calling adapter_update()  is all it does? 
Probably some earlier architectural decision I am not aware of…

- Anyway, as a result, HCI command to write service class is done, returns
Command Complete event (processed in , calls back into adapter layer
adapter_set_class_complete() which invokes  invokes
update_ext_inquiry_response() and (voilà!) EIR is written. So far so good.

- When consequently another OBEX based service like FTP is added using
sdptool, service class of the device does not change, adapter_update()
does nothing, and ultimately EIR is not updated with FTP uuid. This is the
problem that you observed when testing with sdptool.  While showing either
only  OPP or only FTP might be considered not a big deal, skipping UUID in
EIR for  another OBEX-based service like PBAP in EIR might not be so
great, since those services are very different…

The current implementation may be extended as following:
Since update_svcclass_list() is being called throughout sdpd-service.c in
all the places where EIR needs to be updated as well, I propose to modify
the corresponding function  adapter_set_service_classes() (adapter.c) :

**********************
Before:
/* If we already have the CoD we want or the cache is enabled or an
* existing CoD write is in progress just bail out */
if (adapter->current_cod == adapter->wanted_cod ||
	adapter->cache_enable || adapter->pending_cod)
return 0;


***********************

After:
/* If we already have the CoD we want or the cache is enabled or an
* existing CoD write is in progress just bail out */
if (adapter->cache_enable || adapter->pending_cod)
return 0;

/* If we already have the CoD we want, update EIR and return */
if (adapter->current_cod == adapter->wanted_cod ) {
	update_ext_inquiry_response(adapter);
	return 0;
}

**********************
Will that be acceptable? I tested this modification and it’s working with
both d-bus methods and sdptool.

Of course, it might be possible to cache the EIR currently written down,
compare the update, and decide whether the EIR needs to be updated. But
this is more involved, not entirely safe, as we might run into coherency
issues.

As a side note, in future it might be a good idea to expose EIR writes to
an application layer providing external API. The intent of EIR is to allow
quick scan of the surroundings to find a particular service that a device
is interested at the moment (and in turn, exposing the services that the
device choses to expose at the moment) without actually going into full
blown connection mode.   EIR space is at a premium for sophisticated
devices with multiple services. Not every single service needs to be
exposed in EIR. It would be nice to be able to pick which one to add. For
example, if a device supports both headset and handsfree profiles (as all
of the phones do), it might not be necessary to advertise headset uuid in
EIR since the headsets  out there are mostly legacy devices that cannot
perform EIR anyway… Also, as more new services that use uuid128 are
introduced, the EIR buffer pretty quickly if every single one of them is
added without discretion. Just something to throw out there :)


Regards,

Inga

Re: [PATCH v4 0/5] Enhanced support for extended inquiry response

[Johan Hedberg]

Hi Inga,

First of all, sorry for the slight delay in my reply. I've been on
holiday last week with quite limted email processing capability.

On Wed, Jul 28, 2010, ingas@codeaurora.org wrote:
> In the current implementation,  EIR write is keyed off HCI Command
> Complete events for a set of commands (in security.c).
>  - When first registering an OPP service with sdptool, the
> update_svcclass_list()  (sdpd-service.c) calls manager_update_svc()
> (manager.c) which in turn calls adapter_update()  (adapter.c).  BTW, why
> this level of indirection, since manager_update_svc() is called only from
> one place in the code and calling adapter_update()  is all it does? 
> Probably some earlier architectural decision I am not aware of…

That looks pretty stupid to me too. Might be some historical artifact.
Anyway, feel free to send a patch to remove this indirection (i.e. call
adapter_update directly).

> The current implementation may be extended as following:
> Since update_svcclass_list() is being called throughout sdpd-service.c in
> all the places where EIR needs to be updated as well, I propose to modify
> the corresponding function  adapter_set_service_classes() (adapter.c) :
> 
> **********************
> Before:
> /* If we already have the CoD we want or the cache is enabled or an
> * existing CoD write is in progress just bail out */
> if (adapter->current_cod == adapter->wanted_cod ||
> 	adapter->cache_enable || adapter->pending_cod)
> return 0;
> 
> 
> ***********************
> 
> After:
> /* If we already have the CoD we want or the cache is enabled or an
> * existing CoD write is in progress just bail out */
> if (adapter->cache_enable || adapter->pending_cod)
> return 0;
> 
> /* If we already have the CoD we want, update EIR and return */
> if (adapter->current_cod == adapter->wanted_cod ) {
> 	update_ext_inquiry_response(adapter);
> 	return 0;
> }
> 
> **********************
> Will that be acceptable? I tested this modification and it’s working with
> both d-bus methods and sdptool.

Seems good to me.

> As a side note, in future it might be a good idea to expose EIR writes to
> an application layer providing external API. The intent of EIR is to allow
> quick scan of the surroundings to find a particular service that a device
> is interested at the moment (and in turn, exposing the services that the
> device choses to expose at the moment) without actually going into full
> blown connection mode.   EIR space is at a premium for sophisticated
> devices with multiple services. Not every single service needs to be
> exposed in EIR. It would be nice to be able to pick which one to add. For
> example, if a device supports both headset and handsfree profiles (as all
> of the phones do), it might not be necessary to advertise headset uuid in
> EIR since the headsets  out there are mostly legacy devices that cannot
> perform EIR anyway… Also, as more new services that use uuid128 are
> introduced, the EIR buffer pretty quickly if every single one of them is
> added without discretion. Just something to throw out there :)

Good point (not that any concrete examples of such a device/situation
would have come accross yet). One possibility to solve the issue without
directly exposing this to applications would be to have a (rather
static) setting in main.conf of UUIDs to be prioritized over others in
the EIR data.

Johan

Re: [PATCH v4 0/5] Enhanced support for extended inquiry response

[ingas]

Hi Johan,

Thanks for your reply.

> Hi Inga,
>
> First of all, sorry for the slight delay in my reply. I've been on
> holiday last week with quite limted email processing capability.
>
> On Wed, Jul 28, 2010, ingas@codeaurora.org wrote:
>> In the current implementation,  EIR write is keyed off HCI Command
>> Complete events for a set of commands (in security.c).
>>  - When first registering an OPP service with sdptool, the
>> update_svcclass_list()  (sdpd-service.c) calls manager_update_svc()
>> (manager.c) which in turn calls adapter_update()  (adapter.c).  BTW, why
>> this level of indirection, since manager_update_svc() is called only
>> from
>> one place in the code and calling adapter_update()  is all it does?
>> Probably some earlier architectural decision I am not aware of…
>
> That looks pretty stupid to me too. Might be some historical artifact.
> Anyway, feel free to send a patch to remove this indirection (i.e. call
> adapter_update directly).
>

Will do once i get through the initial submission.


>> The current implementation may be extended as following:
>> Since update_svcclass_list() is being called throughout sdpd-service.c
>> in
>> all the places where EIR needs to be updated as well, I propose to
>> modify
>> the corresponding function  adapter_set_service_classes() (adapter.c) :
>>
>> **********************
>> Before:
>> /* If we already have the CoD we want or the cache is enabled or an
>> * existing CoD write is in progress just bail out */
>> if (adapter->current_cod == adapter->wanted_cod ||
>> 	adapter->cache_enable || adapter->pending_cod)
>> return 0;
>>
>>
>> ***********************
>>
>> After:
>> /* If we already have the CoD we want or the cache is enabled or an
>> * existing CoD write is in progress just bail out */
>> if (adapter->cache_enable || adapter->pending_cod)
>> return 0;
>>
>> /* If we already have the CoD we want, update EIR and return */
>> if (adapter->current_cod == adapter->wanted_cod ) {
>> 	update_ext_inquiry_response(adapter);
>> 	return 0;
>> }
>>
>> **********************
>> Will that be acceptable? I tested this modification and it’s working
>> with
>> both d-bus methods and sdptool.
>
> Seems good to me.
>


I just submitted an updated patch set for EIR (hopefully, not messing up
my tabs an spaces in the process). This is certainly a learning process
and your patience is appreciated :)

Regards,

Inga