2014-12-11 12:06:59

by Mariusz Skamra

[permalink] [raw]
Subject: [PATCH 1/2] android/gatt: Fix find_info_handle function

Server can contain attributes with 128-bit attribute types
---
android/gatt.c | 44 ++++++++++++++++++++++++++++++++++----------
1 file changed, 34 insertions(+), 10 deletions(-)

diff --git a/android/gatt.c b/android/gatt.c
index 58bc22d..6d3bcd7 100644
--- a/android/gatt.c
+++ b/android/gatt.c
@@ -6101,11 +6101,12 @@ static uint8_t find_info_handle(const uint8_t *cmd, uint16_t cmd_len,
uint8_t *rsp, size_t rsp_size,
uint16_t *length)
{
- struct queue *q;
+ struct gatt_db_attribute *attrib;
+ struct queue *q, *temp;
struct att_data_list *adl;
int iterator = 0;
uint16_t start, end;
- uint16_t len;
+ uint16_t len, queue_len;

DBG("");

@@ -6127,17 +6128,39 @@ static uint8_t find_info_handle(const uint8_t *cmd, uint16_t cmd_len,
return ATT_ECODE_ATTR_NOT_FOUND;
}

- len = queue_length(q);
- adl = att_data_list_alloc(len, 2 * sizeof(uint16_t));
- if (!adl) {
+ temp = queue_new();
+ if (!temp) {
queue_destroy(q, NULL);
+ return ATT_ECODE_UNLIKELY;
+ }
+
+ attrib = queue_peek_head(q);
+ /* UUIDS can be only 128 bit and 16 bit */
+ len = bt_uuid_len(gatt_db_attribute_get_type(attrib));
+ if (len != 2 && len != 16) {
+ queue_destroy(q, NULL);
+ queue_destroy(temp, NULL);
+ return ATT_ECODE_UNLIKELY;
+ }
+
+ while (attrib && bt_uuid_len(gatt_db_attribute_get_type(attrib)) == len) {
+ queue_push_tail(temp, queue_pop_head(q));
+ attrib = queue_peek_head(q);
+ }
+
+ queue_destroy(q, NULL);
+
+ queue_len = queue_length(temp);
+ adl = att_data_list_alloc(queue_len, len + sizeof(uint16_t));
+ if (!adl) {
+ queue_destroy(temp, NULL);
return ATT_ECODE_INSUFF_RESOURCES;
}

- while (queue_peek_head(q)) {
+ while (queue_peek_head(temp)) {
uint8_t *value;
const bt_uuid_t *type;
- struct gatt_db_attribute *attrib = queue_pop_head(q);
+ struct gatt_db_attribute *attrib = queue_pop_head(temp);
uint16_t handle;

type = gatt_db_attribute_get_type(attrib);
@@ -6148,17 +6171,18 @@ static uint8_t find_info_handle(const uint8_t *cmd, uint16_t cmd_len,

handle = gatt_db_attribute_get_handle(attrib);
put_le16(handle, value);
- memcpy(&value[2], &type->value.u16, bt_uuid_len(type));
+ memcpy(&value[2], &type->value, len);
}

- len = enc_find_info_resp(ATT_FIND_INFO_RESP_FMT_16BIT, adl, rsp,
+ len = enc_find_info_resp(len == 2 ? ATT_FIND_INFO_RESP_FMT_16BIT :
+ ATT_FIND_INFO_RESP_FMT_128BIT, adl, rsp,
rsp_size);
if (!len)
return ATT_ECODE_UNLIKELY;

*length = len;
att_data_list_free(adl);
- queue_destroy(q, free);
+ queue_destroy(temp, NULL);

return 0;
}
--
1.9.1



2014-12-11 14:11:16

by Szymon Janc

[permalink] [raw]
Subject: Re: [PATCH 1/2] android/gatt: Fix find_info_handle function

On Thursday 11 of December 2014 13:06:59 Mariusz Skamra wrote:
> Server can contain attributes with 128-bit attribute types
> ---
> android/gatt.c | 44 ++++++++++++++++++++++++++++++++++----------
> 1 file changed, 34 insertions(+), 10 deletions(-)
>
> diff --git a/android/gatt.c b/android/gatt.c
> index 58bc22d..6d3bcd7 100644
> --- a/android/gatt.c
> +++ b/android/gatt.c
> @@ -6101,11 +6101,12 @@ static uint8_t find_info_handle(const uint8_t *cmd, uint16_t cmd_len,
> uint8_t *rsp, size_t rsp_size,
> uint16_t *length)
> {
> - struct queue *q;
> + struct gatt_db_attribute *attrib;
> + struct queue *q, *temp;
> struct att_data_list *adl;
> int iterator = 0;
> uint16_t start, end;
> - uint16_t len;
> + uint16_t len, queue_len;
>
> DBG("");
>
> @@ -6127,17 +6128,39 @@ static uint8_t find_info_handle(const uint8_t *cmd, uint16_t cmd_len,
> return ATT_ECODE_ATTR_NOT_FOUND;
> }
>
> - len = queue_length(q);
> - adl = att_data_list_alloc(len, 2 * sizeof(uint16_t));
> - if (!adl) {
> + temp = queue_new();
> + if (!temp) {
> queue_destroy(q, NULL);
> + return ATT_ECODE_UNLIKELY;
> + }
> +
> + attrib = queue_peek_head(q);
> + /* UUIDS can be only 128 bit and 16 bit */
> + len = bt_uuid_len(gatt_db_attribute_get_type(attrib));
> + if (len != 2 && len != 16) {
> + queue_destroy(q, NULL);
> + queue_destroy(temp, NULL);
> + return ATT_ECODE_UNLIKELY;
> + }
> +
> + while (attrib && bt_uuid_len(gatt_db_attribute_get_type(attrib)) == len) {

This is over 80 characters. This might be hard to split so how about something
like this instead?

while (attrib) {
const bt_uuid_t *type = gatt_db_attribute_get_type(attrib);

if (bt_uuid_len(type) != len)
break;

......

}


Also some comments would be nice about why we need another queue.

> + queue_push_tail(temp, queue_pop_head(q));
> + attrib = queue_peek_head(q);
> + }
> +
> + queue_destroy(q, NULL);
> +
> + queue_len = queue_length(temp);
> + adl = att_data_list_alloc(queue_len, len + sizeof(uint16_t));
> + if (!adl) {
> + queue_destroy(temp, NULL);
> return ATT_ECODE_INSUFF_RESOURCES;
> }
>
> - while (queue_peek_head(q)) {
> + while (queue_peek_head(temp)) {
> uint8_t *value;
> const bt_uuid_t *type;
> - struct gatt_db_attribute *attrib = queue_pop_head(q);
> + struct gatt_db_attribute *attrib = queue_pop_head(temp);
> uint16_t handle;
>
> type = gatt_db_attribute_get_type(attrib);
> @@ -6148,17 +6171,18 @@ static uint8_t find_info_handle(const uint8_t *cmd, uint16_t cmd_len,
>
> handle = gatt_db_attribute_get_handle(attrib);
> put_le16(handle, value);
> - memcpy(&value[2], &type->value.u16, bt_uuid_len(type));
> + memcpy(&value[2], &type->value, len);
> }
>
> - len = enc_find_info_resp(ATT_FIND_INFO_RESP_FMT_16BIT, adl, rsp,
> + len = enc_find_info_resp(len == 2 ? ATT_FIND_INFO_RESP_FMT_16BIT :
> + ATT_FIND_INFO_RESP_FMT_128BIT, adl, rsp,
> rsp_size);
> if (!len)
> return ATT_ECODE_UNLIKELY;
>
> *length = len;
> att_data_list_free(adl);
> - queue_destroy(q, free);
> + queue_destroy(temp, NULL);
>
> return 0;
> }
>

--
Best regards,
Szymon Janc

2014-12-11 14:04:25

by Szymon Janc

[permalink] [raw]
Subject: Re: [PATCH 2/2] android/gatt: Fix service changed indication

Hi Mariusz,

On Thursday 11 of December 2014 13:07:00 Mariusz Skamra wrote:
> This patch fixes the service changed indication issue.
> Without confirmation callback indications were not sent.
> Dummy ignore_confirmation_cb is used for this purpose.
> ---
> android/gatt.c | 16 +++++++++-------
> 1 file changed, 9 insertions(+), 7 deletions(-)
>
> diff --git a/android/gatt.c b/android/gatt.c
> index 6d3bcd7..8a81852 100644
> --- a/android/gatt.c
> +++ b/android/gatt.c
> @@ -1017,6 +1017,12 @@ static void send_exchange_mtu_request(struct gatt_device *device)
> device_unref(device);
> }
>
> +static void ignore_confirmation_cb(guint8 status, const guint8 *pdu,
> + guint16 len, gpointer user_data)
> +{
> + /* Ignored. */
> +}
> +
> static void notify_att_range_change(struct gatt_device *dev,
> struct att_range *range)
> {
> @@ -1025,6 +1031,7 @@ static void notify_att_range_change(struct gatt_device *dev,
> uint16_t ccc;
> uint8_t *pdu;
> size_t mtu;
> + GAttribResultFunc confirmation_cb = NULL;
>
> handle = gatt_db_attribute_get_handle(service_changed_attrib);
> if (!handle)
> @@ -1044,6 +1051,7 @@ static void notify_att_range_change(struct gatt_device *dev,
> case 0x0002:
> length = enc_indication(handle, (uint8_t *) range,
> sizeof(*range), pdu, mtu);
> + confirmation_cb = ignore_confirmation_cb;
> break;
> default:
> /* 0xfff4 reserved for future use */
> @@ -1051,7 +1059,7 @@ static void notify_att_range_change(struct gatt_device *dev,
> }
>
> if (length)
> - g_attrib_send(dev->attrib, 0, pdu, length, NULL, NULL, NULL);
> + g_attrib_send(dev->attrib, 0, pdu, length, confirmation_cb, NULL, NULL);

Line over 80 characters.

> }
>
> static struct app_connection *create_connection(struct gatt_device *device,
> @@ -5432,12 +5440,6 @@ failed:
> HAL_OP_GATT_SERVER_DELETE_SERVICE, status);
> }
>
> -static void ignore_confirmation_cb(guint8 status, const guint8 *pdu,
> - guint16 len, gpointer user_data)
> -{
> - /* Ignored. */
> -}
> -
> static void handle_server_send_indication(const void *buf, uint16_t len)
> {
> const struct hal_cmd_gatt_server_send_indication *cmd = buf;
>

--
Best regards,
Szymon Janc

2014-12-11 12:07:00

by Mariusz Skamra

[permalink] [raw]
Subject: [PATCH 2/2] android/gatt: Fix service changed indication

This patch fixes the service changed indication issue.
Without confirmation callback indications were not sent.
Dummy ignore_confirmation_cb is used for this purpose.
---
android/gatt.c | 16 +++++++++-------
1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/android/gatt.c b/android/gatt.c
index 6d3bcd7..8a81852 100644
--- a/android/gatt.c
+++ b/android/gatt.c
@@ -1017,6 +1017,12 @@ static void send_exchange_mtu_request(struct gatt_device *device)
device_unref(device);
}

+static void ignore_confirmation_cb(guint8 status, const guint8 *pdu,
+ guint16 len, gpointer user_data)
+{
+ /* Ignored. */
+}
+
static void notify_att_range_change(struct gatt_device *dev,
struct att_range *range)
{
@@ -1025,6 +1031,7 @@ static void notify_att_range_change(struct gatt_device *dev,
uint16_t ccc;
uint8_t *pdu;
size_t mtu;
+ GAttribResultFunc confirmation_cb = NULL;

handle = gatt_db_attribute_get_handle(service_changed_attrib);
if (!handle)
@@ -1044,6 +1051,7 @@ static void notify_att_range_change(struct gatt_device *dev,
case 0x0002:
length = enc_indication(handle, (uint8_t *) range,
sizeof(*range), pdu, mtu);
+ confirmation_cb = ignore_confirmation_cb;
break;
default:
/* 0xfff4 reserved for future use */
@@ -1051,7 +1059,7 @@ static void notify_att_range_change(struct gatt_device *dev,
}

if (length)
- g_attrib_send(dev->attrib, 0, pdu, length, NULL, NULL, NULL);
+ g_attrib_send(dev->attrib, 0, pdu, length, confirmation_cb, NULL, NULL);
}

static struct app_connection *create_connection(struct gatt_device *device,
@@ -5432,12 +5440,6 @@ failed:
HAL_OP_GATT_SERVER_DELETE_SERVICE, status);
}

-static void ignore_confirmation_cb(guint8 status, const guint8 *pdu,
- guint16 len, gpointer user_data)
-{
- /* Ignored. */
-}
-
static void handle_server_send_indication(const void *buf, uint16_t len)
{
const struct hal_cmd_gatt_server_send_indication *cmd = buf;
--
1.9.1