Skip to contentSkip to conversation

Modifications composed during UPF38

PATCH In review Brian Gix 3 participants 11 messages Feb 14, 2011 – Feb 16, 2011

Subsystems (2)
lore
Share & tools
lore

Brian Gix

author
#0 lore raw

cover Modifications composed during UPF38

The major change in these patches is to register Primary Services into the SDP databases as required when registering them into the attrib-server's database. The Core v4.0 specification indicates that the only valid/foolproof way for a BR/EDR client to query a server for Primary Services is to do so via an SDP query. This is from Core v4.0, Vol 3, Part G, Sec 4.4:

"GATT service discovery over BR/EDR transport will list services that only run over an LE transport and therefore only SDP service discovery shall be used on BR/EDR."

This also means that there can be "LE Only" primary services that do NOT need to be in the SDP database, so if a valid range is not specified when registering a Primary Service, it will be omitted from SDP, and only appear when performing a GATT based Primary Service Discover.

Also -- The ATT_UUID we were using was incorrect, and I extended the functionality of the gatttool to allow for listening without sending anything, and ability to perform both a char-write and char-cmd.

Signature · 4 lines
Brian Gix
[email protected]
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum 
author
#1 lore raw

PATCH 1/3 Fixed def of ATT_UUID per BT Assigned Numbers

---
 lib/sdp.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
lib/sdp.h +1 -1
index 16b59fc..e901b2a 100644
@@ -55,6 +55,7 @@ extern "C" {
 #define TCS_BIN_UUID	0x0005
 #define TCS_AT_UUID	0x0006
 #define OBEX_UUID	0x0008
+#define ATT_UUID	0x0007
 #define IP_UUID		0x0009
 #define FTP_UUID	0x000a
 #define HTTP_UUID	0x000c
@@ -72,7 +73,6 @@ extern "C" {
 #define MCAP_CTRL_UUID	0x001e
 #define MCAP_DATA_UUID	0x001f
 #define L2CAP_UUID	0x0100
-#define ATT_UUID	0x1801
 
 /*
  * Service class identifiers of standard services and service groups
Signature · 7 lines
1.7.1

--
Brian Gix
[email protected]
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum 
#2 lore raw

Re: [PATCH 1/3] Fixed def of ATT_UUID per BT Assigned Numbers

Hi Brian,

Quoted 3 lines · Brian Gix
---
 lib/sdp.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

I've pushed this patch upstream but will wait for the other ones since there were some comments regarding them.

Johan

author
#3 lore raw

PATCH 2/3 Add SDP registration of Primary GATT services

In reply to Brian Gix

SDP registration can be supressed by passing Zero as the end handle argument to attrib_db_add().

---
 attrib/example.c    |  119 +++++++++++++++++++++++++++++++------------
 src/attrib-server.c |  139 +++++++++++++++++++++++++++++++++++----------------
 src/attrib-server.h |    6 ++-
 3 files changed, 184 insertions(+), 80 deletions(-)
attrib/example.c +85 -34
index 1911912..eab3c0f 100644
@@ -31,6 +31,7 @@
 
 #include <bluetooth/sdp.h>
 #include <bluetooth/sdp_lib.h>
+#include <src/sdpd.h>
 
 #include <glib.h>
 
@@ -59,6 +60,9 @@
 #define FMT_KILOGRAM_UUID		0xA010
 #define FMT_HANGING_UUID		0xA011
 
+#define SDP_RECORD_COUNT 10
+sdp_record_t *sdp_records[SDP_RECORD_COUNT];
+
 static int register_attributes(void)
 {
 	const char *desc_out_temp = "Outside Temperature";
@@ -77,59 +81,73 @@ static int register_attributes(void)
 	uint8_t atval[256];
 	uuid_t uuid;
 	int len;
+	int i = 0;
 
 	/* Battery state service: primary service definition */
 	sdp_uuid16_create(&uuid, GATT_PRIM_SVC_UUID);
 	att_put_u16(BATTERY_STATE_SVC_UUID, &atval[0]);
-	attrib_db_add(0x0100, &uuid, ATT_NONE, ATT_NOT_PERMITTED, atval, 2);
+	sdp_records[i++] = attrib_db_add(0x0100, 0x0111, &uuid,
+						"Battery State Service",
+						ATT_NONE, ATT_NOT_PERMITTED,
+						atval, 2);
 
 	/* Battery: battery state characteristic */
 	sdp_uuid16_create(&uuid, GATT_CHARAC_UUID);
 	atval[0] = ATT_CHAR_PROPER_READ;
 	att_put_u16(0x0110, &atval[1]);
 	att_put_u16(BATTERY_STATE_UUID, &atval[3]);
-	attrib_db_add(0x0106, &uuid, ATT_NONE, ATT_NOT_PERMITTED, atval, 5);
+	attrib_db_add(0x0106, 0, &uuid, NULL, ATT_NONE, ATT_NOT_PERMITTED,
+			atval, 5);
 
 	/* Battery: battery state attribute */
 	sdp_uuid16_create(&uuid, BATTERY_STATE_UUID);
 	atval[0] = 0x04;
-	attrib_db_add(0x0110, &uuid, ATT_NONE, ATT_NOT_PERMITTED, atval, 1);
+	attrib_db_add(0x0110, 0, &uuid, NULL, ATT_NONE, ATT_NOT_PERMITTED,
+			atval, 1);
 
 	/* Battery: Client Characteristic Configuration */
 	sdp_uuid16_create(&uuid, GATT_CLIENT_CHARAC_CFG_UUID);
 	atval[0] = 0x00;
 	atval[1] = 0x00;
-	attrib_db_add(0x0111, &uuid, ATT_NONE, ATT_AUTHENTICATION, atval, 2);
+	attrib_db_add(0x0111, 0, &uuid, NULL, ATT_NONE, ATT_AUTHENTICATION,
+			atval, 2);
 
 	/* Thermometer: primary service definition */
 	sdp_uuid16_create(&uuid, GATT_PRIM_SVC_UUID);
 	att_put_u16(THERM_HUMIDITY_SVC_UUID, &atval[0]);
-	attrib_db_add(0x0200, &uuid, ATT_NONE, ATT_NOT_PERMITTED, atval, 2);
+	sdp_records[i++] = attrib_db_add(0x0200, 0x0214, &uuid, "Thermometer",
+						ATT_NONE, ATT_NOT_PERMITTED,
+						atval, 2);
 
 	/* Thermometer: Include */
 	sdp_uuid16_create(&uuid, GATT_INCLUDE_UUID);
 	att_put_u16(0x0500, &atval[0]);
 	att_put_u16(0x0504, &atval[2]);
 	att_put_u16(MANUFACTURER_SVC_UUID, &atval[4]);
-	attrib_db_add(0x0201, &uuid, ATT_NONE, ATT_NOT_PERMITTED, atval, 6);
+	attrib_db_add(0x0201, 0, &uuid, NULL, ATT_NONE, ATT_NOT_PERMITTED,
+			atval, 6);
 
 	/* Thermometer: Include */
 	att_put_u16(0x0550, &atval[0]);
 	att_put_u16(0x0568, &atval[2]);
-	attrib_db_add(0x0202, &uuid, ATT_NONE, ATT_NOT_PERMITTED, atval, 4);
+	att_put_u16(VENDOR_SPECIFIC_SVC_UUID, &atval[4]);
+	attrib_db_add(0x0202, 0, &uuid, NULL, ATT_NONE, ATT_NOT_PERMITTED,
+			atval, 6);
 
 	/* Thermometer: temperature characteristic */
 	sdp_uuid16_create(&uuid, GATT_CHARAC_UUID);
 	atval[0] = ATT_CHAR_PROPER_READ;
 	att_put_u16(0x0204, &atval[1]);
 	att_put_u16(TEMPERATURE_UUID, &atval[3]);
-	attrib_db_add(0x0203, &uuid, ATT_NONE, ATT_NOT_PERMITTED, atval, 5);
+	attrib_db_add(0x0203, 0, &uuid, NULL, ATT_NONE, ATT_NOT_PERMITTED,
+			atval, 5);
 
 	/* Thermometer: temperature characteristic value */
 	sdp_uuid16_create(&uuid, TEMPERATURE_UUID);
 	atval[0] = 0x8A;
 	atval[1] = 0x02;
-	attrib_db_add(0x0204, &uuid, ATT_NONE, ATT_NOT_PERMITTED, atval, 2);
+	attrib_db_add(0x0204, 0, &uuid, NULL, ATT_NONE, ATT_NOT_PERMITTED,
+			atval, 2);
 
 	/* Thermometer: temperature characteristic format */
 	sdp_uuid16_create(&uuid, GATT_CHARAC_FMT_UUID);
@@ -138,25 +156,29 @@ static int register_attributes(void)
 	att_put_u16(FMT_CELSIUS_UUID, &atval[2]);
 	atval[4] = 0x01;
 	att_put_u16(FMT_OUTSIDE_UUID, &atval[5]);
-	attrib_db_add(0x0205, &uuid, ATT_NONE, ATT_NOT_PERMITTED, atval, 7);
+	attrib_db_add(0x0205, 0, &uuid, NULL, ATT_NONE, ATT_NOT_PERMITTED,
+			atval, 7);
 
 	/* Thermometer: characteristic user description */
 	sdp_uuid16_create(&uuid, GATT_CHARAC_USER_DESC_UUID);
 	len = strlen(desc_out_temp);
 	strncpy((char *) atval, desc_out_temp, len);
-	attrib_db_add(0x0206, &uuid, ATT_NONE, ATT_NOT_PERMITTED, atval, len);
+	attrib_db_add(0x0206, 0, &uuid, NULL, ATT_NONE, ATT_NOT_PERMITTED,
+			atval, len);
 
 	/* Thermometer: relative humidity characteristic */
 	sdp_uuid16_create(&uuid, GATT_CHARAC_UUID);
 	atval[0] = ATT_CHAR_PROPER_READ;
 	att_put_u16(0x0212, &atval[1]);
 	att_put_u16(RELATIVE_HUMIDITY_UUID, &atval[3]);
-	attrib_db_add(0x0210, &uuid, ATT_NONE, ATT_NOT_PERMITTED, atval, 5);
+	attrib_db_add(0x0210, 0, &uuid, NULL, ATT_NONE, ATT_NOT_PERMITTED,
+			atval, 5);
 
 	/* Thermometer: relative humidity value */
 	sdp_uuid16_create(&uuid, RELATIVE_HUMIDITY_UUID);
 	atval[0] = 0x27;
-	attrib_db_add(0x0212, &uuid, ATT_NONE, ATT_NOT_PERMITTED, atval, 1);
+	attrib_db_add(0x0212, 0, &uuid, NULL, ATT_NONE, ATT_NOT_PERMITTED,
+			atval, 1);
 
 	/* Thermometer: relative humidity characteristic format */
 	sdp_uuid16_create(&uuid, GATT_CHARAC_FMT_UUID);
@@ -165,68 +187,79 @@ static int register_attributes(void)
 	att_put_u16(FMT_PERCENT_UUID, &atval[2]);
 	att_put_u16(BLUETOOTH_SIG_UUID, &atval[4]);
 	att_put_u16(FMT_OUTSIDE_UUID, &atval[6]);
-	attrib_db_add(0x0213, &uuid, ATT_NONE, ATT_NOT_PERMITTED, atval, 8);
+	attrib_db_add(0x0213, 0, &uuid, NULL, ATT_NONE, ATT_NOT_PERMITTED,
+			atval, 8);
 
 	/* Thermometer: characteristic user description */
 	sdp_uuid16_create(&uuid, GATT_CHARAC_USER_DESC_UUID);
 	len = strlen(desc_out_hum);
 	strncpy((char *) atval, desc_out_hum, len);
-	attrib_db_add(0x0214, &uuid, ATT_NONE, ATT_NOT_PERMITTED, atval, len);
+	attrib_db_add(0x0214, 0, &uuid, NULL, ATT_NONE, ATT_NOT_PERMITTED,
+			atval, len);
 
 	/* Secondary Service: Manufacturer Service */
 	sdp_uuid16_create(&uuid, GATT_SND_SVC_UUID);
 	att_put_u16(MANUFACTURER_SVC_UUID, &atval[0]);
-	attrib_db_add(0x0500, &uuid, ATT_NONE, ATT_NOT_PERMITTED, atval, 2);
+	attrib_db_add(0x0500, 0, &uuid, NULL, ATT_NONE, ATT_NOT_PERMITTED,
+			atval, 2);
 
 	/* Manufacturer name characteristic definition */
 	sdp_uuid16_create(&uuid, GATT_CHARAC_UUID);
 	atval[0] = ATT_CHAR_PROPER_READ;
 	att_put_u16(0x0502, &atval[1]);
 	att_put_u16(MANUFACTURER_NAME_UUID, &atval[3]);
-	attrib_db_add(0x0501, &uuid, ATT_NONE, ATT_NOT_PERMITTED, atval, 5);
+	attrib_db_add(0x0501, 0, &uuid, NULL, ATT_NONE, ATT_NOT_PERMITTED,
+			atval, 5);
 
 	/* Manufacturer name characteristic value */
 	sdp_uuid16_create(&uuid, MANUFACTURER_NAME_UUID);
 	len = strlen(manufacturer_name1);
 	strncpy((char *) atval, manufacturer_name1, len);
-	attrib_db_add(0x0502, &uuid, ATT_NONE, ATT_NOT_PERMITTED, atval, len);
+	attrib_db_add(0x0502, 0, &uuid, NULL, ATT_NONE, ATT_NOT_PERMITTED,
+			atval, len);
 
 	/* Manufacturer serial number characteristic */
 	sdp_uuid16_create(&uuid, GATT_CHARAC_UUID);
 	atval[0] = ATT_CHAR_PROPER_READ;
 	att_put_u16(0x0504, &atval[1]);
 	att_put_u16(MANUFACTURER_SERIAL_UUID, &atval[3]);
-	attrib_db_add(0x0503, &uuid, ATT_NONE, ATT_NOT_PERMITTED, atval, 5);
+	attrib_db_add(0x0503, 0, &uuid, NULL, ATT_NONE, ATT_NOT_PERMITTED,
+			atval, 5);
 
 	/* Manufacturer serial number characteristic value */
 	sdp_uuid16_create(&uuid, MANUFACTURER_SERIAL_UUID);
 	len = strlen(serial1);
 	strncpy((char *) atval, serial1, len);
-	attrib_db_add(0x0504, &uuid, ATT_NONE, ATT_NOT_PERMITTED, atval, len);
+	attrib_db_add(0x0504, 0, &uuid, NULL, ATT_NONE, ATT_NOT_PERMITTED,
+			atval, len);
 
 	/* Secondary Service: Manufacturer Service */
 	sdp_uuid16_create(&uuid, GATT_SND_SVC_UUID);
 	att_put_u16(MANUFACTURER_SVC_UUID, &atval[0]);
-	attrib_db_add(0x0505, &uuid, ATT_NONE, ATT_NOT_PERMITTED, atval, 2);
+	attrib_db_add(0x0505, 0, &uuid, NULL, ATT_NONE, ATT_NOT_PERMITTED,
+			atval, 2);
 
 	/* Manufacturer name characteristic definition */
 	sdp_uuid16_create(&uuid, GATT_CHARAC_UUID);
 	atval[0] = ATT_CHAR_PROPER_READ;
 	att_put_u16(0x0507, &atval[1]);
 	att_put_u16(MANUFACTURER_NAME_UUID, &atval[3]);
-	attrib_db_add(0x0506, &uuid, ATT_NONE, ATT_NOT_PERMITTED, atval, 5);
+	attrib_db_add(0x0506, 0, &uuid, NULL, ATT_NONE, ATT_NOT_PERMITTED,
+			atval, 5);
 
 	/* Secondary Service: Vendor Specific Service */
 	sdp_uuid16_create(&uuid, GATT_SND_SVC_UUID);
 	att_put_u16(VENDOR_SPECIFIC_SVC_UUID, &atval[0]);
-	attrib_db_add(0x0550, &uuid, ATT_NONE, ATT_NOT_PERMITTED, atval, 2);
+	attrib_db_add(0x0550, 0, &uuid, NULL, ATT_NONE, ATT_NOT_PERMITTED,
+			atval, 2);
 
 	/* Vendor Specific Type characteristic definition */
 	sdp_uuid16_create(&uuid, GATT_CHARAC_UUID);
 	atval[0] = ATT_CHAR_PROPER_READ;
 	att_put_u16(0x0568, &atval[1]);
 	att_put_u16(VENDOR_SPECIFIC_TYPE_UUID, &atval[3]);
-	attrib_db_add(0x0560, &uuid, ATT_NONE, ATT_NOT_PERMITTED, atval, 5);
+	attrib_db_add(0x0560, 0, &uuid, NULL, ATT_NONE, ATT_NOT_PERMITTED,
+			atval, 5);
 
 	/* Vendor Specific Type characteristic value */
 	sdp_uuid16_create(&uuid, VENDOR_SPECIFIC_TYPE_UUID);
@@ -236,45 +269,53 @@ static int register_attributes(void)
 	atval[3] = 0x64;
 	atval[4] = 0x6F;
 	atval[5] = 0x72;
-	attrib_db_add(0x0568, &uuid, ATT_NONE, ATT_NOT_PERMITTED, atval, 6);
+	attrib_db_add(0x0568, 0, &uuid, NULL, ATT_NONE, ATT_NOT_PERMITTED,
+			atval, 6);
 
 	/* Manufacturer name attribute */
 	sdp_uuid16_create(&uuid, MANUFACTURER_NAME_UUID);
 	len = strlen(manufacturer_name2);
 	strncpy((char *) atval, manufacturer_name2, len);
-	attrib_db_add(0x0507, &uuid, ATT_NONE, ATT_NOT_PERMITTED, atval, len);
+	attrib_db_add(0x0507, 0, &uuid, NULL, ATT_NONE, ATT_NOT_PERMITTED,
+			atval, len);
 
 	/* Characteristic: serial number */
 	sdp_uuid16_create(&uuid, GATT_CHARAC_UUID);
 	atval[0] = ATT_CHAR_PROPER_READ;
 	att_put_u16(0x0509, &atval[1]);
 	att_put_u16(MANUFACTURER_SERIAL_UUID, &atval[3]);
-	attrib_db_add(0x0508, &uuid, ATT_NONE, ATT_NOT_PERMITTED, atval, 5);
+	attrib_db_add(0x0508, 0, &uuid, NULL, ATT_NONE, ATT_NOT_PERMITTED,
+			atval, 5);
 
 	/* Serial number characteristic value */
 	sdp_uuid16_create(&uuid, MANUFACTURER_SERIAL_UUID);
 	len = strlen(serial2);
 	strncpy((char *) atval, serial2, len);
-	attrib_db_add(0x0509, &uuid, ATT_NONE, ATT_NOT_PERMITTED, atval, len);
+	attrib_db_add(0x0509, 0, &uuid, NULL, ATT_NONE, ATT_NOT_PERMITTED,
+			atval, len);
 
 	/* Weight service: primary service definition */
 	sdp_uuid16_create(&uuid, GATT_PRIM_SVC_UUID);
 	memcpy(atval, prim_weight_uuid, 16);
-	attrib_db_add(0x0680, &uuid, ATT_NONE, ATT_NOT_PERMITTED, atval, 16);
+	sdp_records[i++] = attrib_db_add(0x0680, 0x685, &uuid, "Weight Service",
+						ATT_NONE, ATT_NOT_PERMITTED,
+						atval, 16);
 
 	/* Weight: include */
 	sdp_uuid16_create(&uuid, GATT_INCLUDE_UUID);
 	att_put_u16(0x0505, &atval[0]);
 	att_put_u16(0x0509, &atval[2]);
 	att_put_u16(MANUFACTURER_SVC_UUID, &atval[4]);
-	attrib_db_add(0x0681, &uuid, ATT_NONE, ATT_NOT_PERMITTED, atval, 6);
+	attrib_db_add(0x0681, 0, &uuid, NULL, ATT_NONE, ATT_NOT_PERMITTED,
+			atval, 6);
 
 	/* Weight: characteristic */
 	sdp_uuid16_create(&uuid, GATT_CHARAC_UUID);
 	atval[0] = ATT_CHAR_PROPER_READ;
 	att_put_u16(0x0683, &atval[1]);
 	memcpy(&atval[3], char_weight_uuid, 16);
-	attrib_db_add(0x0682, &uuid, ATT_NONE, ATT_NOT_PERMITTED, atval, 19);
+	attrib_db_add(0x0682, 0, &uuid, NULL, ATT_NONE, ATT_NOT_PERMITTED,
+			atval, 19);
 
 	/* Weight: characteristic value */
 	sdp_uuid128_create(&uuid, char_weight_uuid);
@@ -282,7 +323,8 @@ static int register_attributes(void)
 	atval[1] = 0x55;
 	atval[2] = 0x00;
 	atval[3] = 0x00;
-	attrib_db_add(0x0683, &uuid, ATT_NONE, ATT_NOT_PERMITTED, atval, 4);
+	attrib_db_add(0x0683, 0, &uuid, NULL, ATT_NONE, ATT_NOT_PERMITTED,
+			atval, 4);
 
 	/* Weight: characteristic format */
 	sdp_uuid16_create(&uuid, GATT_CHARAC_FMT_UUID);
@@ -291,13 +333,15 @@ static int register_attributes(void)
 	att_put_u16(FMT_KILOGRAM_UUID, &atval[2]);
 	att_put_u16(BLUETOOTH_SIG_UUID, &atval[4]);
 	att_put_u16(FMT_HANGING_UUID, &atval[6]);
-	attrib_db_add(0x0684, &uuid, ATT_NONE, ATT_NOT_PERMITTED, atval, 8);
+	attrib_db_add(0x0684, 0, &uuid, NULL, ATT_NONE, ATT_NOT_PERMITTED,
+			atval, 8);
 
 	/* Weight: characteristic user description */
 	sdp_uuid16_create(&uuid, GATT_CHARAC_USER_DESC_UUID);
 	len = strlen(desc_weight);
 	strncpy((char *) atval, desc_weight, len);
-	attrib_db_add(0x0685, &uuid, ATT_NONE, ATT_NOT_PERMITTED, atval, len);
+	attrib_db_add(0x0685, 0, &uuid, NULL, ATT_NONE, ATT_NOT_PERMITTED,
+			atval, len);
 
 	return 0;
 }
@@ -309,4 +353,11 @@ int server_example_init(void)
 
 void server_example_exit(void)
 {
+	int i;
+
+	for (i = 0; i < SDP_RECORD_COUNT; i++)
+		if (sdp_records[i]) {
+			remove_record_from_server(sdp_records[i]->handle);
+			sdp_records[i] = NULL;
+		}
 }
src/attrib-server.c +95 -44

237 lines not on this page · raw message

src/attrib-server.h +4 -2
index ba90ff4..2601d0e 100644
@@ -25,8 +25,10 @@
 int attrib_server_init(void);
 void attrib_server_exit(void);
 
-int attrib_db_add(uint16_t handle, uuid_t *uuid, int read_reqs, int write_reqs,
-						const uint8_t *value, int len);
+sdp_record_t *attrib_db_add(uint16_t handle, uint16_t end, uuid_t *uuid,
+				const char *name,
+				int read_reqs, int write_reqs,
+				const uint8_t *value, int len);
 int attrib_db_update(uint16_t handle, uuid_t *uuid, const uint8_t *value,
 								int len);
 int attrib_db_del(uint16_t handle);
Signature · 7 lines
1.7.1

--
Brian Gix
[email protected]
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum 
#4 lore raw

Re: [PATCH 2/3] Add SDP registration of Primary GATT services

Quoted 34 lines · Brian Gix

SDP registration can be supressed by passing Zero as the end handle argument to attrib_db_add().
---
 attrib/example.c    |  119 +++++++++++++++++++++++++++++++------------ src/attrib-server.c |  139 +++++++++++++++++++++++++++++++++++---------------- src/attrib-server.h |    6 ++-

 3 files changed, 184 insertions(+), 80 deletions(-)
attrib/example.c +4 -0
index 1911912..eab3c0f 100644
@@ -31,6 +31,7 @@
 
  #include <bluetooth/sdp.h>
  #include <bluetooth/sdp_lib.h>
+#include <src/sdpd.h>
 
  #include <glib.h>
 
@@ -59,6 +60,9 @@
  #define FMT_KILOGRAM_UUID              0xA010
  #define FMT_HANGING_UUID               0xA011
 
+#define SDP_RECORD_COUNT 10
+sdp_record_t *sdp_records[SDP_RECORD_COUNT];
+
  static int register_attributes(void)
  {
        const char *desc_out_temp = "Outside Temperature";
@@ -77,59 +81,73 @@ static int register_attributes(void)
        uint8_t atval[256];
        uuid_t uuid;
        int len;

attrib/example.c static int register_attributes(void) · line 92

Quoted 10 lines
attrib/example.c +5 -1
+       int i = 0;
 
        /* Battery state service: primary service definition */
        sdp_uuid16_create(&uuid, GATT_PRIM_SVC_UUID);
        att_put_u16(BATTERY_STATE_SVC_UUID, &atval[0]);
-       attrib_db_add(0x0100, &uuid, ATT_NONE, ATT_NOT_PERMITTED, atval, 2);
+       sdp_records[i++] = attrib_db_add(0x0100, 0x0111, &uuid,
+                                               "Battery State Service",
+                                               ATT_NONE, ATT_NOT_PERMITTED,
+                                               atval, 2);

What if instead of changing attrib_db_add() signature to return a SDP record, you create a "sdp_record_from_attrib()" for this purpose? This function could get the struct attribute * pointers for start/end.

There are plans for attrib_db_add() to return the created struct attribute *a (as shown in a patch I sent sometime ago which I still need to resend), I can send an updated version of that patch if you agree to this approach.

attrib/example.c static uint16_t write_value(struct gatt_channel *channel, uint16_t handle, · line 609

Quoted 9 lines
attrib/example.c +1 -1
@@ -609,7 +603,7 @@ static uint16_t write_value(struct gatt_channel *channel, uint16_t handle,
  static uint16_t mtu_exchange(struct gatt_channel *channel, uint16_t mtu,
                uint8_t *pdu, int len)
  {
-       channel->mtu = MIN(mtu, ATT_MAX_MTU);
+       channel->mtu = MIN(mtu, channel->mtu);
 
        return enc_mtu_resp(channel->mtu, pdu, len);
  }

This change looks unrelated to the patch.

Regards,

Signature · 3 lines
Anderson Lizardo
OpenBossa Labs - INdT
Manaus - Brazil
author
#5 lore raw

Re: [PATCH 2/3] Add SDP registration of Primary GATT services

Hi Anderson,

Quoted 50 lines · Anderson Lizardo
Quoted 34 lines · Brian Gix

SDP registration can be supressed by passing Zero as the end handle argument to attrib_db_add().

---
  attrib/example.c    |  119 +++++++++++++++++++++++++++++++------------
  src/attrib-server.c |  139 +++++++++++++++++++++++++++++++++++----------------
  src/attrib-server.h |    6 ++-
  3 files changed, 184 insertions(+), 80 deletions(-)
attrib/example.c +4 -0
index 1911912..eab3c0f 100644
@@ -31,6 +31,7 @@
 
  #include<bluetooth/sdp.h>
  #include<bluetooth/sdp_lib.h>
+#include<src/sdpd.h>
 
  #include<glib.h>
 
@@ -59,6 +60,9 @@
  #define FMT_KILOGRAM_UUID              0xA010
  #define FMT_HANGING_UUID               0xA011
 
+#define SDP_RECORD_COUNT 10
+sdp_record_t *sdp_records[SDP_RECORD_COUNT];
+
  static int register_attributes(void)
  {
        const char *desc_out_temp = "Outside Temperature";
@@ -77,59 +81,73 @@ static int register_attributes(void)
        uint8_t atval[256];
        uuid_t uuid;
        int len;

attrib/example.c static int register_attributes(void) · line 92

Quoted 10 lines
attrib/example.c +5 -1
+       int i = 0;
 
        /* Battery state service: primary service definition */
        sdp_uuid16_create(&uuid, GATT_PRIM_SVC_UUID);
        att_put_u16(BATTERY_STATE_SVC_UUID,&atval[0]);
-       attrib_db_add(0x0100,&uuid, ATT_NONE, ATT_NOT_PERMITTED, atval, 2);
+       sdp_records[i++] = attrib_db_add(0x0100, 0x0111,&uuid,
+                                               "Battery State Service",
+                                               ATT_NONE, ATT_NOT_PERMITTED,
+                                               atval, 2);

What if instead of changing attrib_db_add() signature to return a SDP record, you create a "sdp_record_from_attrib()" for this purpose? This function could get the struct attribute * pointers for start/end.

Quoted 4 lines

There are plans for attrib_db_add() to return the created struct attribute *a (as shown in a patch I sent sometime ago which I still need to resend), I can send an updated version of that patch if you agree to this approach.

I was considering an approach like this during UPF but abandoned it for lack of time. There are few hours available to make hot fixes during these events, and I just learned of the requirement for SDP records in "real time".

However, here are the issues as I see them:

1. We currently register everything into the attribute database a single attribute at a time. The current attrib_db_add therefore has no inherent knowledge of entire "Service" objects, and lacks the start-end range that is required not only by the SDP record, but also the ATT driven GATT Service Discovery. Ideally I would like to see a "Register Service" API which internally would register all INCLUDES, CHARACTERISTICS, and SDP Records, but this would be a significant departure from the current code.

2. A less significant change would be to create an sdp_record_from_attrib() API, but it would need to be called after all attributes for the service have been registered, because it needs to calculate the "end" handle of the service, much like the current GATT procedures to Discover and Find Services. This would probably constitute the "least change necessary" to the current paradigm.

3. I'm not sure that the "struct attrib *a" would add anything here, except as a way to prevent having to explicitly specify the attribute handle value in the first place. We should eventually get away from specifying explicit attribute handle values, because that makes it hard to have mix-and-match services. A service app writer should not be expected to know the remaining 0-65535 "number space" that is available on every device that they want to target there service app to. In that respect, having attrib_db_add assign an available handle, and return a "struct attrib *" pointer could then be used instead of a handle when registering the SDP of a completely registered primary service.

I propose that I change this patch set to follow plan "2" above. If you want to subsequently change the return of attrib_db_add() as you have described, it should then be trivial to change the arguments to the new sdp_record_from_attrib() API to accept the attrib struct pointer rather than the explicit start handle.

Quoted 14 lines

attrib/example.c static uint16_t write_value(struct gatt_channel *channel, uint16_t handle, · line 609

Quoted 10 lines
attrib/example.c +1 -1
@@ -609,7 +603,7 @@ static uint16_t write_value(struct gatt_channel *channel, uint16_t handle,
  static uint16_t mtu_exchange(struct gatt_channel *channel, uint16_t mtu,
                uint8_t *pdu, int len)
  {
-       channel->mtu = MIN(mtu, ATT_MAX_MTU);
+       channel->mtu = MIN(mtu, channel->mtu);
 
        return enc_mtu_resp(channel->mtu, pdu, len);
  }

This change looks unrelated to the patch.

Regards,

Signature · 4 lines
Brian Gix
[email protected]
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum
#6 lore raw

Re: [PATCH 2/3] Add SDP registration of Primary GATT services

Hi Brian,

Quoted 7 lines · Brian Gix

1. We currently register everything into the attribute database a single attribute at a time.  The current attrib_db_add therefore has no inherent knowledge of entire "Service" objects, and lacks the start-end range that is required not only by the SDP record, but also the ATT driven GATT Service Discovery. Ideally I would like to see a "Register Service" API which internally would register all INCLUDES, CHARACTERISTICS, and SDP Records, but this would be a significant departure from the current code.

You are right. In fact, we have a proposal (currently not implemented) to create a server side API for registering GATT services. Please see my e-mail from 2010-12-30 entitled "[RFC] BlueZ server side API for registering GATT services". I was actually expecting comments from you :)

Suggestions/comments on the proposal are welcome. Please try to keep discussion on this topic here on the list because there are other people interested on contributing to the implementation as well.

ALso note the proposal is to create an API on top of the current one, with minimal changes to allow both to be used in parallel (it may be necessary in cases we want more control over the attributes).

Quoted 6 lines

2. A less significant change would be to create an sdp_record_from_attrib() API, but it would need to be called after all attributes for the service have been registered, because it needs to calculate the "end" handle of the service, much like the current GATT procedures to Discover and Find Services. This would probably constitute the "least change necessary" to the current paradigm.

This is my suggestion on this thread. After the attributes of a specific service have been inserted into the database, you can know the start-end attributes.

Quoted 10 lines

3. I'm not sure that the "struct attrib *a" would add anything here, except as a way to prevent having to explicitly specify the attribute handle value in the first place.  We should eventually get away from specifying explicit attribute handle values, because that makes it hard to have mix-and-match services.  A service app writer should not be expected to know the remaining 0-65535 "number space" that is available on every device that they want to target there service app to.  In that respect, having attrib_db_add assign an available handle, and return a "struct attrib *" pointer could then be used instead of a handle when registering the SDP of a completely registered primary service.

What I had in mind (still not discussed on the list), was to make the "automatic handle allocation" feature together with the high-level GATT Service oriented API. This would work by keeping track of used/unused start-end ranges, and allocating ranges for services as they are registered/unregistered. Example:

* "free range list" starts with: [0x1000, 0xFFFF]
* Register service A with 10 attributes: [0x1000, 0x1009] (free range:
[0x100A, 0xFFFF])
* Register service B with 5 attributes: [0x100A, 0x100E] (free range:
[0x100F, 0xFFFF])
* Register service C with 11 attributes: [0x100F, 0x1019] (free range:
[0x101A, 0xFFFF])
* Unregister service B (free ranges: [0x100A, 0x100E], [0x101A, 0xFFFF])
* Register service D with 4 attributes: [0x100A, 0x100D] (free ranges:
[0x100E, 0x100E], [0x101A, 0xFFFF])

Quoted 5 lines

I propose that I change this patch set to follow plan "2" above.  If you want to subsequently change the return of attrib_db_add() as you have described, it should then be trivial to change the arguments to the new sdp_record_from_attrib() API to accept the attrib struct pointer rather than the explicit start handle.

Agreed. sdp_record_from_attrib() may receive just start/end handles if other fields of struct attrubute are not necessary.

Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
src/attrib-server.c +1 -1
@@ -609,7 +603,7 @@ static uint16_t write_value(struct gatt_channel
 *channel, uint16_t handle,
  static uint16_t mtu_exchange(struct gatt_channel *channel, uint16_t mtu,
                uint8_t *pdu, int len)
  {
-       channel->mtu = MIN(mtu, ATT_MAX_MTU);
+       channel->mtu = MIN(mtu, channel->mtu);
 
Quoted 4 lines
Quoted 4 lines
Quoted 2 lines
src/attrib-server.c +0 -0
        return enc_mtu_resp(channel->mtu, pdu, len);

 }

This change looks unrelated to the patch.

Any comments on this?

Regards,

Signature · 3 lines
Anderson Lizardo
Instituto Nokia de Tecnologia - INdT
Manaus - Brazil
author
#7 lore raw

PATCH 3/3 Add gatttool enhancements for UPF

In reply to Brian Gix

Including seperating write-cmd from write-char, and enabling --listen to stand on it's own if needed. (PTS tester)

---
 attrib/gatttool.c |   57 ++++++++++++++++++++++++++++++++++++++++++++++++----
 1 files changed, 52 insertions(+), 5 deletions(-)
attrib/gatttool.c +52 -5
index 8e8ed8e..054adb1 100644
@@ -63,6 +63,7 @@ static gboolean opt_listen = FALSE;
 static gboolean opt_char_desc = FALSE;
 static gboolean opt_le = FALSE;
 static gboolean opt_char_write = FALSE;
+static gboolean opt_char_cmd = FALSE;
 static GMainLoop *event_loop;
 static gboolean got_error = FALSE;
 
@@ -90,9 +91,9 @@ static GIOChannel *do_connect(gboolean le)
 
 	/* This check is required because currently setsockopt() returns no
 	 * errors for MTU values smaller than the allowed minimum. */
-	if (opt_mtu != 0 && opt_mtu < ATT_MIN_MTU_L2CAP) {
+	if (opt_mtu != 0 && opt_mtu < 23) {
 		g_printerr("MTU cannot be smaller than %d\n",
-							ATT_MIN_MTU_L2CAP);
+							23);
 		return NULL;
 	}
 
@@ -277,6 +278,14 @@ static gboolean characteristics(gpointer user_data)
 	return FALSE;
 }
 
+static void char_write_cb(guint8 status, const guint8 *pdu, guint16 plen,
+							gpointer user_data)
+{
+	if (plen == 1)
+		g_print("Attrib Write Succeeded\n");
+	else
+		g_printerr("Attrib Write failed: %s\n", att_ecode2str(status));
+}
 static void char_read_cb(guint8 status, const guint8 *pdu, guint16 plen,
 							gpointer user_data)
 {
@@ -427,7 +436,40 @@ static gboolean characteristics_write(gpointer user_data)
 		goto error;
 	}
 
+	gatt_write_char(attrib, opt_handle, value, len, char_write_cb, value);
+	gatt_read_char(attrib, opt_handle, char_read_cb, attrib);
+
+	return FALSE;
+
+error:
+	g_main_loop_quit(event_loop);
+	return FALSE;
+}
+
+static gboolean characteristics_cmd(gpointer user_data)
+{
+	GAttrib *attrib = user_data;
+	uint8_t *value;
+	size_t len;
+
+	if (opt_handle <= 0) {
+		g_printerr("A valid handle is required\n");
+		goto error;
+	}
+
+	if (opt_value == NULL || opt_value[0] == '\0') {
+		g_printerr("A value is required\n");
+		goto error;
+	}
+
+	len = attr_data_from_string(opt_value, &value);
+	if (len == 0) {
+		g_printerr("Invalid value\n");
+		goto error;
+	}
+
 	gatt_write_cmd(attrib, opt_handle, value, len, mainloop_quit, value);
+	gatt_read_char(attrib, opt_handle, char_read_cb, attrib);
 
 	return FALSE;
 
@@ -531,6 +573,8 @@ static GOptionEntry gatt_options[] = {
 		"Characteristics Value/Descriptor Read", NULL },
 	{ "char-write", 0, 0, G_OPTION_ARG_NONE, &opt_char_write,
 		"Characteristics Value Write", NULL },
+	{ "char-cmd", 0, 0, G_OPTION_ARG_NONE, &opt_char_cmd,
+		"Characteristics Value Cmd", NULL },
 	{ "char-desc", 0, 0, G_OPTION_ARG_NONE, &opt_char_desc,
 		"Characteristics Descriptor Discovery", NULL },
 	{ "listen", 0, 0, G_OPTION_ARG_NONE, &opt_listen,
@@ -561,7 +605,7 @@ int main(int argc, char *argv[])
 	GError *gerr = NULL;
 	GAttrib *attrib;
 	GIOChannel *chan;
-	GSourceFunc callback;
+	GSourceFunc callback = NULL;
 
 	context = g_option_context_new(NULL);
 	g_option_context_add_main_entries(context, options, NULL);
@@ -602,9 +646,11 @@ int main(int argc, char *argv[])
 		callback = characteristics_read;
 	else if (opt_char_write)
 		callback = characteristics_write;
+	else if (opt_char_cmd)
+		callback = characteristics_cmd;
 	else if (opt_char_desc)
 		callback = characteristics_desc;
-	else {
+	else if (!opt_listen) {
 		gchar *help = g_option_context_get_help(context, TRUE, NULL);
 		g_print("%s\n", help);
 		g_free(help);
@@ -625,7 +671,8 @@ int main(int argc, char *argv[])
 	if (opt_listen)
 		g_idle_add(listen_start, attrib);
 
-	g_idle_add(callback, attrib);
+	if (callback)
+		g_idle_add(callback, attrib);
 
 	g_main_loop_run(event_loop);
 
Signature · 7 lines
1.7.1

--
Brian Gix
[email protected]
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum 
#8 lore raw

Re: [PATCH 3/3] Add gatttool enhancements for UPF

Hi Brian,

attrib/gatttool.c static GIOChannel *do_connect(gboolean le) · line 98

Quoted 11 lines · Brian Gix
attrib/gatttool.c +2 -2
@@ -90,9 +91,9 @@ static GIOChannel *do_connect(gboolean le)
 
        /* This check is required because currently setsockopt() returns no
         * errors for MTU values smaller than the allowed minimum. */
-       if (opt_mtu != 0 && opt_mtu < ATT_MIN_MTU_L2CAP) {
+       if (opt_mtu != 0 && opt_mtu < 23) {
                g_printerr("MTU cannot be smaller than %d\n",
-                                                       ATT_MIN_MTU_L2CAP);
+                                                       23);
                return NULL;
        }

The changes above seem unrelated to this patch.

attrib/gatttool.c static gboolean characteristics(gpointer user_data) · line 287

Quoted 12 lines
attrib/gatttool.c +7 -0
@@ -277,6 +278,14 @@ static gboolean characteristics(gpointer user_data)
        return FALSE;
  }
 
+static void char_write_cb(guint8 status, const guint8 *pdu, guint16 plen,
+                                                       gpointer user_data)
+{
+       if (plen == 1)
+               g_print("Attrib Write Succeeded\n");
+       else
+               g_printerr("Attrib Write failed: %s\n", att_ecode2str(status));

Why not check by the status instead of plen ?

Also, I'd suggest "Characteristic write" instead of "Attrib Write".

attrib/gatttool.c static gboolean characteristics_write(gpointer user_data) · line 440

Quoted 10 lines
attrib/gatttool.c +3 -0
+}
  static void char_read_cb(guint8 status, const guint8 *pdu, guint16 plen,
                                                        gpointer user_data)
  {
@@ -427,7 +436,40 @@ static gboolean characteristics_write(gpointer user_data)
                goto error;
        }
 
+       gatt_write_char(attrib, opt_handle, value, len, char_write_cb, value);
+       gatt_read_char(attrib, opt_handle, char_read_cb, attrib);

Why both read and write ?

Quoted 22 lines
attrib/gatttool.c +22 -0
+
+       return FALSE;
+
+error:
+       g_main_loop_quit(event_loop);
+       return FALSE;
+}
+
+static gboolean characteristics_cmd(gpointer user_data)
+{
+       GAttrib *attrib = user_data;
+       uint8_t *value;
+       size_t len;
+
+       if (opt_handle <= 0) {
+               g_printerr("A valid handle is required\n");
+               goto error;
+       }
+
+       if (opt_value == NULL || opt_value[0] == '\0') {
+               g_printerr("A value is required\n");
+               goto error;

attrib/gatttool.c static gboolean characteristics_write(gpointer user_data) · line 472

Quoted 10 lines
attrib/gatttool.c +9 -0
+       }
+
+       len = attr_data_from_string(opt_value, &value);
+       if (len == 0) {
+               g_printerr("Invalid value\n");
+               goto error;
+       }
+
        gatt_write_cmd(attrib, opt_handle, value, len, mainloop_quit, value);
+       gatt_read_char(attrib, opt_handle, char_read_cb, attrib);

Same question here.

attrib/gatttool.c static GOptionEntry gatt_options[] = { · line 577

Quoted 9 lines
attrib/gatttool.c +2 -0
        return FALSE;
 
@@ -531,6 +573,8 @@ static GOptionEntry gatt_options[] = {
                "Characteristics Value/Descriptor Read", NULL },
        { "char-write", 0, 0, G_OPTION_ARG_NONE, &opt_char_write,
                "Characteristics Value Write", NULL },
+       { "char-cmd", 0, 0, G_OPTION_ARG_NONE, &opt_char_cmd,
+               "Characteristics Value Cmd", NULL },

Suggestion: "Characteristic Value write using Write Command" (or something similar).

Quoted 26 lines
attrib/gatttool.c +4 -2
        { "char-desc", 0, 0, G_OPTION_ARG_NONE, &opt_char_desc,
                "Characteristics Descriptor Discovery", NULL },
        { "listen", 0, 0, G_OPTION_ARG_NONE, &opt_listen,
@@ -561,7 +605,7 @@ int main(int argc, char *argv[])
        GError *gerr = NULL;
        GAttrib *attrib;
        GIOChannel *chan;
-       GSourceFunc callback;
+       GSourceFunc callback = NULL;
 
        context = g_option_context_new(NULL);
        g_option_context_add_main_entries(context, options, NULL);
@@ -602,9 +646,11 @@ int main(int argc, char *argv[])
                callback = characteristics_read;
        else if (opt_char_write)
                callback = characteristics_write;
+       else if (opt_char_cmd)
+               callback = characteristics_cmd;
        else if (opt_char_desc)
                callback = characteristics_desc;
-       else {
+       else if (!opt_listen) {
                gchar *help = g_option_context_get_help(context, TRUE, NULL);
                g_print("%s\n", help);
                g_free(help);

attrib/gatttool.c int main(int argc, char *argv[]) · line 677

Quoted 8 lines
attrib/gatttool.c +2 -1
@@ -625,7 +671,8 @@ int main(int argc, char *argv[])
        if (opt_listen)
                g_idle_add(listen_start, attrib);
 
-       g_idle_add(callback, attrib);
+       if (callback)
+               g_idle_add(callback, attrib);
 
        g_main_loop_run(event_loop);

Regards,

Signature · 3 lines
Anderson Lizardo
OpenBossa Labs - INdT
Manaus - Brazil
author
#9 lore raw

Re: [PATCH 3/3] Add gatttool enhancements for UPF

Hi Anderson,

The modifications to gatttool were largely organic in nature and on the fly responses to the needs of my UPF testing. Also, it is of course just a test tool.

More inline.

Quoted 5 lines · Anderson Lizardo

Hi Brian,

Quoted 2 lines · Brian Gix
(quoted diff) +0 -0
@@ -90,9 +91,9 @@ static GIOChannel *do_connect(gboolean le)
 
Quoted 11 lines

attrib/gatttool.c static GIOChannel *do_connect(gboolean le) · line 98

Quoted 9 lines
attrib/gatttool.c +2 -2
        /* This check is required because currently setsockopt() returns no
         * errors for MTU values smaller than the allowed minimum. */
-       if (opt_mtu != 0&&  opt_mtu<  ATT_MIN_MTU_L2CAP) {
+       if (opt_mtu != 0&&  opt_mtu<  23) {
                g_printerr("MTU cannot be smaller than %d\n",
-                                                       ATT_MIN_MTU_L2CAP);
+                                                       23);
                return NULL;
        }

The changes above seem unrelated to this patch.

The code was incorrect in that it limited it to 48 instead of 23. However, I should have used the ATT_MIN_MTU_LE define. This will be fixed.

Quoted 6 lines
Quoted 6 lines
attrib/gatttool.c +0 -0
@@ -277,6 +278,14 @@ static gboolean characteristics(gpointer user_data)
        return FALSE;
  }
 
Quoted 9 lines

attrib/gatttool.c static gboolean characteristics(gpointer user_data) · line 287

Quoted 7 lines
attrib/gatttool.c +7 -0
+static void char_write_cb(guint8 status, const guint8 *pdu, guint16 plen,
+                                                       gpointer user_data)
+{
+       if (plen == 1)
+               g_print("Attrib Write Succeeded\n");
+       else
+               g_printerr("Attrib Write failed: %s\n", att_ecode2str(status));

Why not check by the status instead of plen ?

During testing, it was apparent that status did not contain Zero. This may be due to there being no status octet in the WRITE_RSP packet. I didn't have time to trace this back into the gatt code, but will do so now.

Quoted 11 lines

Also, I'd suggest "Characteristic write" instead of "Attrib Write".

Quoted 8 lines
attrib/gatttool.c +1 -0
+}
  static void char_read_cb(guint8 status, const guint8 *pdu, guint16 plen,
                                                        gpointer user_data)
  {
@@ -427,7 +436,40 @@ static gboolean characteristics_write(gpointer user_data)
                goto error;
        }
 
Quoted 4 lines

Review static gboolean characteristics_write(gpointer user_data) · line 440

Quoted 2 lines
(quoted diff) +2 -0
+       gatt_write_char(attrib, opt_handle, value, len, char_write_cb, value);
+       gatt_read_char(attrib, opt_handle, char_read_cb, attrib);

Why both read and write ?

I needed to do this to confirm that the write actually succeeded within the bounds of the testing, which included not dropping the connection between the two procedures. This will not be needed for the forthcoming interactive gatttool, but that was not available for UPF. As it is, I would argue that this is more useful in a test tool than superfluous.

Quoted 27 lines
Quoted 27 lines
attrib/gatttool.c +26 -0
+
+       return FALSE;
+
+error:
+       g_main_loop_quit(event_loop);
+       return FALSE;
+}
+
+static gboolean characteristics_cmd(gpointer user_data)
+{
+       GAttrib *attrib = user_data;
+       uint8_t *value;
+       size_t len;
+
+       if (opt_handle<= 0) {
+               g_printerr("A valid handle is required\n");
+               goto error;
+       }
+
+       if (opt_value == NULL || opt_value[0] == '\0') {
+               g_printerr("A value is required\n");
+               goto error;
+       }
+
+       len = attr_data_from_string(opt_value,&value);
+       if (len == 0) {
Quoted 8 lines

attrib/gatttool.c static gboolean characteristics_write(gpointer user_data) · line 472

Quoted 6 lines
attrib/gatttool.c +5 -0
+               g_printerr("Invalid value\n");
+               goto error;
+       }
+
        gatt_write_cmd(attrib, opt_handle, value, len, mainloop_quit, value);
+       gatt_read_char(attrib, opt_handle, char_read_cb, attrib);

Same question here.

Same response here.

Quoted 50 lines

attrib/gatttool.c static GOptionEntry gatt_options[] = { · line 577

Quoted 10 lines
attrib/gatttool.c +2 -0
        return FALSE;
 
@@ -531,6 +573,8 @@ static GOptionEntry gatt_options[] = {
                "Characteristics Value/Descriptor Read", NULL },
        { "char-write", 0, 0, G_OPTION_ARG_NONE,&opt_char_write,
                "Characteristics Value Write", NULL },
+       { "char-cmd", 0, 0, G_OPTION_ARG_NONE,&opt_char_cmd,
+               "Characteristics Value Cmd", NULL },

Suggestion: "Characteristic Value write using Write Command" (or something similar).

Quoted 26 lines
attrib/gatttool.c +4 -2
        { "char-desc", 0, 0, G_OPTION_ARG_NONE,&opt_char_desc,
                "Characteristics Descriptor Discovery", NULL },
        { "listen", 0, 0, G_OPTION_ARG_NONE,&opt_listen,
@@ -561,7 +605,7 @@ int main(int argc, char *argv[])
        GError *gerr = NULL;
        GAttrib *attrib;
        GIOChannel *chan;
-       GSourceFunc callback;
+       GSourceFunc callback = NULL;
 
        context = g_option_context_new(NULL);
        g_option_context_add_main_entries(context, options, NULL);
@@ -602,9 +646,11 @@ int main(int argc, char *argv[])
                callback = characteristics_read;
        else if (opt_char_write)
                callback = characteristics_write;
+       else if (opt_char_cmd)
+               callback = characteristics_cmd;
        else if (opt_char_desc)
                callback = characteristics_desc;
-       else {
+       else if (!opt_listen) {
                gchar *help = g_option_context_get_help(context, TRUE, NULL);
                g_print("%s\n", help);
                g_free(help);

attrib/gatttool.c int main(int argc, char *argv[]) · line 677

Quoted 8 lines
attrib/gatttool.c +2 -1
@@ -625,7 +671,8 @@ int main(int argc, char *argv[])
        if (opt_listen)
                g_idle_add(listen_start, attrib);
 
-       g_idle_add(callback, attrib);
+       if (callback)
+               g_idle_add(callback, attrib);
 
        g_main_loop_run(event_loop);

Regards,

Signature · 4 lines
Brian Gix
[email protected]
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum
#10 lore raw

Re: [PATCH 3/3] Add gatttool enhancements for UPF

Hi Brian,

Quoted 5 lines · Brian Gix

Hi Anderson,

The modifications to gatttool were largely organic in nature and on the fly responses to the needs of my UPF testing. Also, it is of course just a test tool.

Understood, but if you could split these changes into more "logic" pieces (separate commits), with commit messages which explain the reasoning of the change (e.g. the answers on this thread), it will be nice.

Regards,

Signature · 3 lines
Anderson Lizardo
Instituto Nokia de Tecnologia - INdT
Manaus - Brazil