[PATCH 1/3] Update gatt_service_add size assert for dealing with 128-bit uuids

Linux bluetooth development
 help / color / mirror / Atom feed

* [PATCH 1/3] Update gatt_service_add size assert for dealing with 128-bit uuids
@ 2013-07-29 18:42 Dirk-Jan C. Binnema
  2013-07-29 18:42 ` [PATCH 2/3] Rename GATT_OPT_CHR_UUID into GATT_OPT_CHR_UUID16 Dirk-Jan C. Binnema
                   ` (2 more replies)
  0 siblings, 3 replies; 4+ messages in thread
From: Dirk-Jan C. Binnema @ 2013-07-29 18:42 UTC (permalink / raw)
  To: linux-bluetooth; +Cc: Dirk-Jan C. Binnema

From: "Dirk-Jan C. Binnema" <djcb@djcbsoftware.nl>

To paraphrase Anderson Lizardo: "h" was overflowing for 128-bit services
because the handle is incremented after each attribute is added so at the
end we had: 0xffff + 1 == 0 (h is uint16_t).

As it seems more complex to try to remove the overflow without
affecting the logic too much, we update the assertion.
---
 attrib/gatt-service.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/attrib/gatt-service.c b/attrib/gatt-service.c
index 4b02d39..1b84c89 100644
--- a/attrib/gatt-service.c
+++ b/attrib/gatt-service.c
@@ -341,7 +341,7 @@ gboolean gatt_service_add(struct btd_adapter *adapter, uint16_t uuid,
 	}
 
 	g_assert(size < USHRT_MAX);
-	g_assert(h - start_handle == (uint16_t) size);
+	g_assert(h == 0 || (h - start_handle == (uint16_t) size));
 	g_slist_free_full(chrs, free_gatt_info);
 
 	return TRUE;
-- 
1.8.1.4


^ permalink raw reply related	[flat|nested] 4+ messages in thread

* [PATCH 2/3] Rename GATT_OPT_CHR_UUID into GATT_OPT_CHR_UUID16
  2013-07-29 18:42 [PATCH 1/3] Update gatt_service_add size assert for dealing with 128-bit uuids Dirk-Jan C. Binnema
@ 2013-07-29 18:42 ` Dirk-Jan C. Binnema
  2013-07-29 18:42 ` [PATCH 3/3] Add support for 128-bit characteristic uuids in gatt_service_add Dirk-Jan C. Binnema
  2013-07-30 11:28 ` [PATCH 1/3] Update gatt_service_add size assert for dealing with 128-bit uuids Johan Hedberg
  2 siblings, 0 replies; 4+ messages in thread
From: Dirk-Jan C. Binnema @ 2013-07-29 18:42 UTC (permalink / raw)
  To: linux-bluetooth; +Cc: Dirk-Jan C. Binnema

From: "Dirk-Jan C. Binnema" <djcb@djcbsoftware.nl>

Rename the option for all in-tree users. This frees up GATT_OPT_CHR_UUID to be used
for UUIDs of type bt_uuid_t* (in a next patch).
---
 attrib/gatt-service.c         |  4 ++--
 attrib/gatt-service.h         |  2 +-
 plugins/gatt-example.c        |  2 +-
 profiles/alert/server.c       | 16 ++++++++--------
 profiles/proximity/immalert.c |  2 +-
 profiles/proximity/linkloss.c |  2 +-
 profiles/time/server.c        |  8 ++++----
 7 files changed, 18 insertions(+), 18 deletions(-)

diff --git a/attrib/gatt-service.c b/attrib/gatt-service.c
index 1b84c89..6ae3902 100644
--- a/attrib/gatt-service.c
+++ b/attrib/gatt-service.c
@@ -68,7 +68,7 @@ static GSList *parse_opts(gatt_option opt1, va_list args)
 
 	while (opt != GATT_OPT_INVALID) {
 		switch (opt) {
-		case GATT_OPT_CHR_UUID:
+		case GATT_OPT_CHR_UUID16:
 			bt_uuid16_create(&info->uuid, va_arg(args, int));
 			/* characteristic declaration and value */
 			info->num_attrs += 2;
@@ -108,7 +108,7 @@ static GSList *parse_opts(gatt_option opt1, va_list args)
 		}
 
 		opt = va_arg(args, gatt_option);
-		if (opt == GATT_OPT_CHR_UUID) {
+		if (opt == GATT_OPT_CHR_UUID16) {
 			info = g_new0(struct gatt_info, 1);
 			l = g_slist_append(l, info);
 		}
diff --git a/attrib/gatt-service.h b/attrib/gatt-service.h
index b810e2e..3964c2b 100644
--- a/attrib/gatt-service.h
+++ b/attrib/gatt-service.h
@@ -24,7 +24,7 @@
 
 typedef enum {
 	GATT_OPT_INVALID = 0,
-	GATT_OPT_CHR_UUID,
+	GATT_OPT_CHR_UUID16,
 	GATT_OPT_CHR_PROPS,
 	GATT_OPT_CHR_VALUE_CB,
 	GATT_OPT_CHR_AUTHENTICATION,
diff --git a/plugins/gatt-example.c b/plugins/gatt-example.c
index 37972e8..b926947 100644
--- a/plugins/gatt-example.c
+++ b/plugins/gatt-example.c
@@ -114,7 +114,7 @@ static gboolean register_battery_service(struct btd_adapter *adapter)
 
 	return gatt_service_add(adapter, GATT_PRIM_SVC_UUID, &uuid,
 			/* battery state characteristic */
-			GATT_OPT_CHR_UUID, BATTERY_STATE_UUID,
+			GATT_OPT_CHR_UUID16, BATTERY_STATE_UUID,
 			GATT_OPT_CHR_PROPS, ATT_CHAR_PROPER_READ |
 							ATT_CHAR_PROPER_NOTIFY,
 			GATT_OPT_CHR_VALUE_CB, ATTRIB_READ,
diff --git a/profiles/alert/server.c b/profiles/alert/server.c
index 8da1928..4565470 100644
--- a/profiles/alert/server.c
+++ b/profiles/alert/server.c
@@ -789,7 +789,7 @@ static void register_phone_alert_service(struct alert_adapter *al_adapter)
 	/* Phone Alert Status Service */
 	gatt_service_add(al_adapter->adapter, GATT_PRIM_SVC_UUID, &uuid,
 			/* Alert Status characteristic */
-			GATT_OPT_CHR_UUID, ALERT_STATUS_CHR_UUID,
+			GATT_OPT_CHR_UUID16, ALERT_STATUS_CHR_UUID,
 			GATT_OPT_CHR_PROPS, ATT_CHAR_PROPER_READ |
 							ATT_CHAR_PROPER_NOTIFY,
 			GATT_OPT_CHR_VALUE_CB, ATTRIB_READ,
@@ -799,12 +799,12 @@ static void register_phone_alert_service(struct alert_adapter *al_adapter)
 			GATT_OPT_CHR_VALUE_GET_HANDLE,
 			&al_adapter->hnd_value[NOTIFY_ALERT_STATUS],
 			/* Ringer Control Point characteristic */
-			GATT_OPT_CHR_UUID, RINGER_CP_CHR_UUID,
+			GATT_OPT_CHR_UUID16, RINGER_CP_CHR_UUID,
 			GATT_OPT_CHR_PROPS, ATT_CHAR_PROPER_WRITE_WITHOUT_RESP,
 			GATT_OPT_CHR_VALUE_CB, ATTRIB_WRITE,
 			ringer_cp_write, NULL,
 			/* Ringer Setting characteristic */
-			GATT_OPT_CHR_UUID, RINGER_SETTING_CHR_UUID,
+			GATT_OPT_CHR_UUID16, RINGER_SETTING_CHR_UUID,
 			GATT_OPT_CHR_PROPS, ATT_CHAR_PROPER_READ |
 							ATT_CHAR_PROPER_NOTIFY,
 			GATT_OPT_CHR_VALUE_CB, ATTRIB_READ,
@@ -892,35 +892,35 @@ static void register_alert_notif_service(struct alert_adapter *al_adapter)
 	/* Alert Notification Service */
 	gatt_service_add(al_adapter->adapter, GATT_PRIM_SVC_UUID, &uuid,
 			/* Supported New Alert Category */
-			GATT_OPT_CHR_UUID, SUPP_NEW_ALERT_CAT_CHR_UUID,
+			GATT_OPT_CHR_UUID16, SUPP_NEW_ALERT_CAT_CHR_UUID,
 			GATT_OPT_CHR_PROPS, ATT_CHAR_PROPER_READ,
 			GATT_OPT_CHR_VALUE_CB, ATTRIB_READ,
 			supp_new_alert_cat_read, al_adapter->adapter,
 			GATT_OPT_CHR_VALUE_GET_HANDLE,
 			&al_adapter->supp_new_alert_cat_handle,
 			/* New Alert */
-			GATT_OPT_CHR_UUID, NEW_ALERT_CHR_UUID,
+			GATT_OPT_CHR_UUID16, NEW_ALERT_CHR_UUID,
 			GATT_OPT_CHR_PROPS, ATT_CHAR_PROPER_NOTIFY,
 			GATT_OPT_CCC_GET_HANDLE,
 			&al_adapter->hnd_ccc[NOTIFY_NEW_ALERT],
 			GATT_OPT_CHR_VALUE_GET_HANDLE,
 			&al_adapter->hnd_value[NOTIFY_NEW_ALERT],
 			/* Supported Unread Alert Category */
-			GATT_OPT_CHR_UUID, SUPP_UNREAD_ALERT_CAT_CHR_UUID,
+			GATT_OPT_CHR_UUID16, SUPP_UNREAD_ALERT_CAT_CHR_UUID,
 			GATT_OPT_CHR_PROPS, ATT_CHAR_PROPER_READ,
 			GATT_OPT_CHR_VALUE_CB, ATTRIB_READ,
 			supp_unread_alert_cat_read, al_adapter->adapter,
 			GATT_OPT_CHR_VALUE_GET_HANDLE,
 			&al_adapter->supp_unread_alert_cat_handle,
 			/* Unread Alert Status */
-			GATT_OPT_CHR_UUID, UNREAD_ALERT_CHR_UUID,
+			GATT_OPT_CHR_UUID16, UNREAD_ALERT_CHR_UUID,
 			GATT_OPT_CHR_PROPS, ATT_CHAR_PROPER_NOTIFY,
 			GATT_OPT_CCC_GET_HANDLE,
 			&al_adapter->hnd_ccc[NOTIFY_UNREAD_ALERT],
 			GATT_OPT_CHR_VALUE_GET_HANDLE,
 			&al_adapter->hnd_value[NOTIFY_UNREAD_ALERT],
 			/* Alert Notification Control Point */
-			GATT_OPT_CHR_UUID, ALERT_NOTIF_CP_CHR_UUID,
+			GATT_OPT_CHR_UUID16, ALERT_NOTIF_CP_CHR_UUID,
 			GATT_OPT_CHR_PROPS, ATT_CHAR_PROPER_WRITE,
 			GATT_OPT_CHR_VALUE_CB, ATTRIB_WRITE,
 			alert_notif_cp_write, NULL,
diff --git a/profiles/proximity/immalert.c b/profiles/proximity/immalert.c
index 06e8eb8..2f8d4e7 100644
--- a/profiles/proximity/immalert.c
+++ b/profiles/proximity/immalert.c
@@ -249,7 +249,7 @@ void imm_alert_register(struct btd_adapter *adapter)
 	svc_added = gatt_service_add(adapter,
 				GATT_PRIM_SVC_UUID, &uuid,
 				/* Alert level characteristic */
-				GATT_OPT_CHR_UUID, ALERT_LEVEL_CHR_UUID,
+				GATT_OPT_CHR_UUID16, ALERT_LEVEL_CHR_UUID,
 				GATT_OPT_CHR_PROPS,
 					ATT_CHAR_PROPER_WRITE_WITHOUT_RESP,
 				GATT_OPT_CHR_VALUE_CB, ATTRIB_WRITE,
diff --git a/profiles/proximity/linkloss.c b/profiles/proximity/linkloss.c
index cb87b12..a7ed96c 100644
--- a/profiles/proximity/linkloss.c
+++ b/profiles/proximity/linkloss.c
@@ -290,7 +290,7 @@ void link_loss_register(struct btd_adapter *adapter)
 	svc_added = gatt_service_add(adapter,
 			GATT_PRIM_SVC_UUID, &uuid,
 			/* Alert level characteristic */
-			GATT_OPT_CHR_UUID, ALERT_LEVEL_CHR_UUID,
+			GATT_OPT_CHR_UUID16, ALERT_LEVEL_CHR_UUID,
 			GATT_OPT_CHR_PROPS,
 				ATT_CHAR_PROPER_READ | ATT_CHAR_PROPER_WRITE,
 			GATT_OPT_CHR_VALUE_CB, ATTRIB_READ,
diff --git a/profiles/time/server.c b/profiles/time/server.c
index 518a29f..178d4d2 100644
--- a/profiles/time/server.c
+++ b/profiles/time/server.c
@@ -152,14 +152,14 @@ static gboolean register_current_time_service(struct btd_adapter *adapter)
 	/* Current Time service */
 	return gatt_service_add(adapter, GATT_PRIM_SVC_UUID, &uuid,
 				/* CT Time characteristic */
-				GATT_OPT_CHR_UUID, CT_TIME_CHR_UUID,
+				GATT_OPT_CHR_UUID16, CT_TIME_CHR_UUID,
 				GATT_OPT_CHR_PROPS, ATT_CHAR_PROPER_READ |
 							ATT_CHAR_PROPER_NOTIFY,
 				GATT_OPT_CHR_VALUE_CB, ATTRIB_READ,
 						current_time_read, adapter,
 
 				/* Local Time Information characteristic */
-				GATT_OPT_CHR_UUID, LOCAL_TIME_INFO_CHR_UUID,
+				GATT_OPT_CHR_UUID16, LOCAL_TIME_INFO_CHR_UUID,
 				GATT_OPT_CHR_PROPS, ATT_CHAR_PROPER_READ,
 				GATT_OPT_CHR_VALUE_CB, ATTRIB_READ,
 						local_time_info_read, adapter,
@@ -215,14 +215,14 @@ static gboolean register_ref_time_update_service(struct btd_adapter *adapter)
 	/* Reference Time Update service */
 	return gatt_service_add(adapter, GATT_PRIM_SVC_UUID, &uuid,
 				/* Time Update control point */
-				GATT_OPT_CHR_UUID, TIME_UPDATE_CTRL_CHR_UUID,
+				GATT_OPT_CHR_UUID16, TIME_UPDATE_CTRL_CHR_UUID,
 				GATT_OPT_CHR_PROPS,
 					ATT_CHAR_PROPER_WRITE_WITHOUT_RESP,
 				GATT_OPT_CHR_VALUE_CB, ATTRIB_WRITE,
 						time_update_control, adapter,
 
 				/* Time Update status */
-				GATT_OPT_CHR_UUID, TIME_UPDATE_STAT_CHR_UUID,
+				GATT_OPT_CHR_UUID16, TIME_UPDATE_STAT_CHR_UUID,
 				GATT_OPT_CHR_PROPS, ATT_CHAR_PROPER_READ,
 				GATT_OPT_CHR_VALUE_CB, ATTRIB_READ,
 						time_update_status, adapter,
-- 
1.8.1.4


^ permalink raw reply related	[flat|nested] 4+ messages in thread

* [PATCH 3/3] Add support for 128-bit characteristic uuids in gatt_service_add
  2013-07-29 18:42 [PATCH 1/3] Update gatt_service_add size assert for dealing with 128-bit uuids Dirk-Jan C. Binnema
  2013-07-29 18:42 ` [PATCH 2/3] Rename GATT_OPT_CHR_UUID into GATT_OPT_CHR_UUID16 Dirk-Jan C. Binnema
@ 2013-07-29 18:42 ` Dirk-Jan C. Binnema
  2013-07-30 11:28 ` [PATCH 1/3] Update gatt_service_add size assert for dealing with 128-bit uuids Johan Hedberg
  2 siblings, 0 replies; 4+ messages in thread
From: Dirk-Jan C. Binnema @ 2013-07-29 18:42 UTC (permalink / raw)
  To: linux-bluetooth; +Cc: Dirk-Jan C. Binnema

From: "Dirk-Jan C. Binnema" <djcb@djcbsoftware.nl>

Custom GATT services typically use 128-bits UUIDs; this patch allows for using
gatt_service_add to define such services by adding a new option GATT_OPT_CHR_UUID,
which allows for passing bt_uuid_t* values. Note, a previous patch renamed the
old GATT_OPT_CHR_UUID into GATT_OPT_CHR_UUID16.
---
 attrib/gatt-service.c | 17 ++++++++++++-----
 attrib/gatt-service.h |  6 ++++++
 2 files changed, 18 insertions(+), 5 deletions(-)

diff --git a/attrib/gatt-service.c b/attrib/gatt-service.c
index 6ae3902..4cd300f 100644
--- a/attrib/gatt-service.c
+++ b/attrib/gatt-service.c
@@ -73,6 +73,12 @@ static GSList *parse_opts(gatt_option opt1, va_list args)
 			/* characteristic declaration and value */
 			info->num_attrs += 2;
 			break;
+		case GATT_OPT_CHR_UUID:
+			memcpy(&info->uuid, va_arg(args, bt_uuid_t *),
+				sizeof(bt_uuid_t));
+			/* characteristic declaration and value */
+			info->num_attrs += 2;
+			break;
 		case GATT_OPT_CHR_PROPS:
 			info->props = va_arg(args, int);
 
@@ -108,7 +114,7 @@ static GSList *parse_opts(gatt_option opt1, va_list args)
 		}
 
 		opt = va_arg(args, gatt_option);
-		if (opt == GATT_OPT_CHR_UUID16) {
+		if (opt == GATT_OPT_CHR_UUID16 || opt == GATT_OPT_CHR_UUID) {
 			info = g_new0(struct gatt_info, 1);
 			l = g_slist_append(l, info);
 		}
@@ -183,10 +189,11 @@ static gboolean add_characteristic(struct btd_adapter *adapter,
 	uint16_t h = *handle;
 	struct attribute *a;
 	bt_uuid_t bt_uuid;
-	uint8_t atval[5];
+	uint8_t atval[ATT_MAX_VALUE_LEN];
 	GSList *l;
 
-	if (!info->uuid.value.u16 || !info->props) {
+	if ((info->uuid.type != BT_UUID16 && info->uuid.type != BT_UUID128) ||
+		!info->props) {
 		error("Characteristic UUID or properties are missing");
 		return FALSE;
 	}
@@ -222,9 +229,9 @@ static gboolean add_characteristic(struct btd_adapter *adapter,
 	bt_uuid16_create(&bt_uuid, GATT_CHARAC_UUID);
 	atval[0] = info->props;
 	att_put_u16(h + 1, &atval[1]);
-	att_put_u16(info->uuid.value.u16, &atval[3]);
+	att_put_uuid(info->uuid, &atval[3]);
 	if (attrib_db_add(adapter, h++, &bt_uuid, ATT_NONE, ATT_NOT_PERMITTED,
-						atval, sizeof(atval)) == NULL)
+				atval, 3 + info->uuid.type / 8) == NULL)
 		return FALSE;
 
 	/* characteristic value */
diff --git a/attrib/gatt-service.h b/attrib/gatt-service.h
index 3964c2b..728d3a8 100644
--- a/attrib/gatt-service.h
+++ b/attrib/gatt-service.h
@@ -24,7 +24,13 @@
 
 typedef enum {
 	GATT_OPT_INVALID = 0,
+
+	/* bt_uuid_t* value */
+	GATT_OPT_CHR_UUID,
+
+	/* a uint16 value */
 	GATT_OPT_CHR_UUID16,
+
 	GATT_OPT_CHR_PROPS,
 	GATT_OPT_CHR_VALUE_CB,
 	GATT_OPT_CHR_AUTHENTICATION,
-- 
1.8.1.4


^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [PATCH 1/3] Update gatt_service_add size assert for dealing with 128-bit uuids
  2013-07-29 18:42 [PATCH 1/3] Update gatt_service_add size assert for dealing with 128-bit uuids Dirk-Jan C. Binnema
  2013-07-29 18:42 ` [PATCH 2/3] Rename GATT_OPT_CHR_UUID into GATT_OPT_CHR_UUID16 Dirk-Jan C. Binnema
  2013-07-29 18:42 ` [PATCH 3/3] Add support for 128-bit characteristic uuids in gatt_service_add Dirk-Jan C. Binnema
@ 2013-07-30 11:28 ` Johan Hedberg
  2 siblings, 0 replies; 4+ messages in thread
From: Johan Hedberg @ 2013-07-30 11:28 UTC (permalink / raw)
  To: Dirk-Jan C. Binnema; +Cc: linux-bluetooth, Dirk-Jan C. Binnema

Hi Dirk,

On Mon, Jul 29, 2013, Dirk-Jan C. Binnema wrote:
> To paraphrase Anderson Lizardo: "h" was overflowing for 128-bit services
> because the handle is incremented after each attribute is added so at the
> end we had: 0xffff + 1 == 0 (h is uint16_t).
> 
> As it seems more complex to try to remove the overflow without
> affecting the logic too much, we update the assertion.
> ---
>  attrib/gatt-service.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

I made a couple of more cosmetic cleanups and after that pushed these
patches upstream. Thanks.

Some things to keep in mind for future patches:

- Keep your commit message line lengths max 72-74 (both git log and git
  shortlog indent them and we don't want to run past a 80 char terminal)
- Prefix the summary lines appropriately (in this case "attrib: ...")
- Split lines are indented as much as possible while keeping them under
  80 characters (i.e. not just past the opening parenthesis)

Johan

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2013-07-30 11:28 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-07-29 18:42 [PATCH 1/3] Update gatt_service_add size assert for dealing with 128-bit uuids Dirk-Jan C. Binnema
2013-07-29 18:42 ` [PATCH 2/3] Rename GATT_OPT_CHR_UUID into GATT_OPT_CHR_UUID16 Dirk-Jan C. Binnema
2013-07-29 18:42 ` [PATCH 3/3] Add support for 128-bit characteristic uuids in gatt_service_add Dirk-Jan C. Binnema
2013-07-30 11:28 ` [PATCH 1/3] Update gatt_service_add size assert for dealing with 128-bit uuids Johan Hedberg

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox