Skip to contentSkip to conversation

Enhanced support for extended inquiry response

PATCH Superseded Inga Stotland 3 participants 17 messages Jul 22, 2010 – Aug 2, 2010 Bluetooth drivers

v4 of 8 Superseded Latest is v8 →

lore
Share & tools
lore

Inga Stotland

author
#0 lore raw

v4 cover Enhanced support for extended inquiry response

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.

Signature · 3 lines
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.
author
#1 lore raw

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

---
 src/sdpd.h |   13 +++++++++++++
 1 files changed, 13 insertions(+), 0 deletions(-)
src/sdpd.h +13 -0
index e93b0b6..c1d69a9 100644
@@ -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;
Signature · 6 lines
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.
author
#2 lore raw

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

In reply to Inga Stotland
---
 src/sdpd-service.c |  108 ++++++++++++++++++++++++++++++++++++++++++++--------
 1 files changed, 92 insertions(+), 16 deletions(-)
src/sdpd-service.c +92 -16
index cdbb4f4..1314ada 100644
@@ -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)
Signature · 6 lines
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.
maintainer
#3 lore raw

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

Hi,

On Thu, Jul 22, 2010 at 7:06 PM, Inga Stotland <[email protected]> wrote= :

Quoted 2 lines

---
=A0src/sdpd-service.c | =A0108 ++++++++++++++++++++++++++++++++++++++++++=

++--------

src/sdpd-service.c static void update_svclass_list(const bdaddr_t *sr=

Quoted 16 lines

=A01 files changed, 92 insertions(+), 16 deletions(-)

src/sdpd-service.c +0 -0
index cdbb4f4..1314ada 100644
@@ -49,6 +49,8 @@

=A0#include "manager.h"
=A0#include "adapter.h"

src/sdpd-service.c +2 -0
+#define SIZEOF_UUID128 16
+

=A0static sdp_record_t *server =3D NULL;

=A0static uint16_t did_vendor =3D 0x0000;

src/sdpd-service.c +0 -0
@@ -174,41 +176,103 @@ static void update_svclass_list(const bdaddr_t *sr=

c)

src/sdpd-service.c static void update_svclass_list(const bdaddr_t *src)

Quoted 5 lines
src/sdpd-service.c +2 -0
 =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)

src/sdpd-service.c static void update_svclass_list(const bdaddr_t *sr= · line 186

Quoted 7 lines
src/sdpd-service.c +7 -0
+{
+ =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 */

src/sdpd-service.c static void update_svclass_list(const bdaddr_t *sr= · line 190

Quoted 4 lines
src/sdpd-service.c +4 -0
+ =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;

src/sdpd-service.c static void update_svclass_list(const bdaddr_t *sr= · line 195

Quoted 5 lines
src/sdpd-service.c +5 -0
+
+ =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 */

src/sdpd-service.c static void update_svclass_list(const bdaddr_t *sr= · line 196

Quoted 1 line
src/sdpd-service.c +1 -0
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 if ((len + 2 + SIZEOF_UUID128) > EIR_DATA_L=

ENGTH) {

src/sdpd-service.c static void update_svclass_list(const bdaddr_t *sr= · line 201

Quoted 5 lines
src/sdpd-service.c +5 -0
+ =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 */

src/sdpd-service.c static void update_svclass_list(const bdaddr_t *sr= · line 203

Quoted 2 lines
src/sdpd-service.c +2 -0
+ =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++) {

src/sdpd-service.c static void update_svclass_list(const bdaddr_t *sr= · line 204

Quoted 1 line
src/sdpd-service.c +1 -0
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (uuid128=

[i*SIZEOF_UUID128 + k] !=3D

src/sdpd-service.c static void update_svclass_list(const bdaddr_t *sr= · line 205

Quoted 1 line
src/sdpd-service.c +1 -0
+ =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])

src/sdpd-service.c static void update_svclass_list(const bdaddr_t *sr= · line 206

Quoted 1 line
src/sdpd-service.c +1 -0
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=

=A0 break;

src/sdpd-service.c static void update_svclass_list(const bdaddr_t *sr= · line 208

Quoted 2 lines
src/sdpd-service.c +2 -0
+ =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=

)

src/sdpd-service.c static void update_svclass_list(const bdaddr_t *sr= · line 217

Quoted 9 lines
src/sdpd-service.c +9 -0
+ =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

src/sdpd-service.c static void update_svclass_list(const bdaddr_t *sr= · line 218

Quoted 1 line
src/sdpd-service.c +1 -0
+ =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];

src/sdpd-service.c static void update_svclass_list(const bdaddr_t *sr= · line 228

Quoted 10 lines
src/sdpd-service.c +10 -0
+
+ =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;

src/sdpd-service.c static void update_svclass_list(const bdaddr_t *sr= · line 233

Quoted 5 lines
src/sdpd-service.c +5 -0
+ =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.

Quoted 2 lines
src/sdpd-service.c +0 -0
 =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,

Quoted 1 line

=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =

=A0 =A0uint8_t *data)

Quoted 12 lines

=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);

Quoted 8 lines

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

Quoted 38 lines

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

src/sdpd-service.c +2 -2
@@ -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) {
Quoted 1 line

=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0sdp_record_t *rec =3D (sdp_record_t *) lis=

t->data;

Quoted 3 lines
src/sdpd-service.c +0 -0
@@ -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)

Quoted 5 lines

=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 */

Quoted 1 line

+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 if ((eir_len + 2 + sizeof(uint16_t)) > EIR_=

DATA_LENGTH) {

Quoted 7 lines

+ =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)

Quoted 1 line

+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (uuid16[i] =3D=3D rec->s=

vclass.value.uuid16)

Quoted 7 lines

=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=

;

Quoted 1 line

+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 uuid16[index++] =3D rec->svclass.value.uuid=

16;

Quoted 9 lines

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

Quoted 6 lines

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

Quoted 1 line

- =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 *ptr++ =3D (uuid[i] & 0xff0=

0) >> 8;

Quoted 1 line

+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 *ptr++ =3D (uuid16[i] & 0x0=

0ff);

Quoted 1 line

+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 *ptr++ =3D (uuid16[i] & 0xf=

f00) >> 8;

Quoted 6 lines

=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);

Quoted 5 lines

=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

author
#4 lore raw

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

Hi Luiz,

Quoted 17 lines

Hi,

Quoted 12 lines · Inga Stotland

---
 src/sdpd-service.c |  108
++++++++++++++++++++++++++++++++++++++++++++--------

 1 files changed, 92 insertions(+), 16 deletions(-)
src/sdpd-service.c +0 -0
index cdbb4f4..1314ada 100644
@@ -49,6 +49,8 @@
  #include "manager.h"
  #include "adapter.h"
Quoted 5 lines

src/sdpd-service.c line 51

Quoted 1 line
src/sdpd-service.c +1 -0
+

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.

Quoted 17 lines
Quoted 11 lines
src/sdpd-service.c +0 -0
  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;

Quoted 4 lines

-               if (i == index - 1)
+               if (i < index)
                       continue;

Quoted 7 lines

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

maintainer
#5 lore raw

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

Hi,

Quoted 8 lines · On Tue, Jul 27, 2010 at 11:08 PM, <[email protected]> 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

Quoted 4 lines

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.

Quoted 4 lines

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

author
#6 lore raw

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

In reply to Inga Stotland
---
 plugins/service.c |   18 ++++++++++++++++++
 src/adapter.c     |   23 ++++++++++++++++++++++-
 src/adapter.h     |    1 +
 3 files changed, 41 insertions(+), 1 deletions(-)
plugins/service.c +18 -0
index 96280bd..ee9176b 100644
@@ -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);
 }
src/adapter.c +22 -1
index 789a196..c1e1768 100644
@@ -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];
src/adapter.h +1 -0
index 8226514..f72eb0b 100644
@@ -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,
Signature · 6 lines
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.
author
#7 lore raw

PATCH v4 4/5 Handle arrays in device properties dictionary

In reply to Inga Stotland
---
 src/adapter.c |    8 +++++++-
 1 files changed, 7 insertions(+), 1 deletions(-)
src/adapter.c +7 -1
index c1e1768..b22b086 100644
@@ -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 *);
 	}
Signature · 6 lines
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.
author
#8 lore raw

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

In reply to Inga Stotland
---
 src/adapter.c  |   95 ++++++++++++++++++++++++++++++++++++++++++++++++++++++--
 src/adapter.h  |    4 +-
 src/dbus-hci.c |    6 ++--
 3 files changed, 97 insertions(+), 8 deletions(-)
src/adapter.c +92 -3
index b22b086..2438c29 100644
@@ -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)
src/adapter.h +2 -2
index f72eb0b..231d2c9 100644
@@ -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);
src/dbus-hci.c +3 -3
index b83506f..6d27caa 100644
@@ -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)
Signature · 6 lines
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.
#9 lore raw
In reply to Inga Stotland

Hi Inga,

Quoted 1 line · Inga Stotland

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

#10 lore raw
Quoted 6 lines · Johan Hedberg

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

#11 lore raw

Hi,

Quoted 8 lines · Johan Hedberg
Quoted 6 lines · Johan Hedberg

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.

Quoted 6 lines

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

author
#12 lore raw

Hi Johan,

Quoted 28 lines

Hi,

Quoted 10 lines · Johan Hedberg
Quoted 2 lines · Johan Hedberg

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

Quoted 2 lines

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

Quoted 2 lines

was running and I could see the appropriate "Write Extended Inquiry Response" commands with hcidump.

Quoted 6 lines

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.

Quoted 8 lines

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

#13 lore raw

Hi Inga,

Quoted 8 lines · On Sun, Jul 25, 2010, [email protected] 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

author
#14 lore raw

Hi Johan,

Quoted 10 lines

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

#15 lore raw

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.

Quoted 8 lines · On Wed, Jul 28, 2010, [email protected] 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).

Quoted 23 lines

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 */

Quoted 8 lines
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.

Quoted 6 lines

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

Quoted 8 lines

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

author
#16 lore raw

Hi Johan,

Thanks for your reply.

Quoted 16 lines

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.

Quoted 9 lines · On Wed, Jul 28, 2010, [email protected] 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…

Quoted 4 lines

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.

Quoted 30 lines
Quoted 30 lines

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;
}
Quoted 7 lines
Quoted 4 lines

**********************
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