This patch set includes some small bug fixes in shared/gatt-client,
tools/btgatt-client, and shared/gatt-db. It also updates the TODO list with two
new items.
Arman Uguray (7):
shared/gatt-client: Fix bug in service changed queue.
shared/gatt-client: Fix bug in service changed handling.
tools/btgatt-client: Fix read-long-value offset parsing.
tools/btgatt-client: Fix off-by-one error.
tools/btgatt-client: Print prompt on service changed.
shared/gatt-db: Fix range handling in read by grp type.
TODO: Add new shared/gatt TODO items.
TODO | 15 +++++++++++++++
src/shared/gatt-client.c | 10 +++++++---
src/shared/gatt-db.c | 11 ++++++++++-
tools/btgatt-client.c | 11 +++++++----
4 files changed, 39 insertions(+), 8 deletions(-)
--
2.1.0.rc2.206.gedb03e5
Hi Marcin,
On Sun, Oct 26, 2014 at 11:54 PM, Marcin Kraglak
<[email protected]> wrote:
> Hi Arman,
>
> On 26 October 2014 22:12, Arman Uguray <[email protected]> wrote:
>> This patch fixes bug in gatt_db_read_by_grp_type in which the range end handle
>> was being ignored. This caused incorrect results to be sent for a database
>> where services exists beyond the end handle provided in the request.
>> ---
>> src/shared/gatt-db.c | 11 ++++++++++-
>> 1 file changed, 10 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/shared/gatt-db.c b/src/shared/gatt-db.c
>> index b3f95d2..4e74030 100644
>> --- a/src/shared/gatt-db.c
>> +++ b/src/shared/gatt-db.c
>> @@ -428,6 +428,7 @@ static void read_by_group_type(void *data, void *user_data)
>> {
>> struct read_by_group_type_data *search_data = user_data;
>> struct gatt_db_service *service = data;
>> + uint16_t grp_start, grp_end;
>>
>> if (!service->active)
>> return;
>> @@ -439,7 +440,15 @@ static void read_by_group_type(void *data, void *user_data)
>> if (bt_uuid_cmp(&search_data->uuid, &service->attributes[0]->uuid))
>> return;
>>
>> - if (service->attributes[0]->handle < search_data->start_handle)
>> + grp_start = service->attributes[0]->handle;
>> + grp_end = service->attributes[service->num_handles - 1]->handle;
>
> I suggest
>
> grp_end = grp_start + num_handles - 1
>
> because not all attributes must be in use. First attribute must be
> filled, because it's service decl.
Thanks, fixed in v1.
Hi Arman,
On 26 October 2014 22:12, Arman Uguray <[email protected]> wrote:
> This patch fixes bug in gatt_db_read_by_grp_type in which the range end handle
> was being ignored. This caused incorrect results to be sent for a database
> where services exists beyond the end handle provided in the request.
> ---
> src/shared/gatt-db.c | 11 ++++++++++-
> 1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/src/shared/gatt-db.c b/src/shared/gatt-db.c
> index b3f95d2..4e74030 100644
> --- a/src/shared/gatt-db.c
> +++ b/src/shared/gatt-db.c
> @@ -428,6 +428,7 @@ static void read_by_group_type(void *data, void *user_data)
> {
> struct read_by_group_type_data *search_data = user_data;
> struct gatt_db_service *service = data;
> + uint16_t grp_start, grp_end;
>
> if (!service->active)
> return;
> @@ -439,7 +440,15 @@ static void read_by_group_type(void *data, void *user_data)
> if (bt_uuid_cmp(&search_data->uuid, &service->attributes[0]->uuid))
> return;
>
> - if (service->attributes[0]->handle < search_data->start_handle)
> + grp_start = service->attributes[0]->handle;
> + grp_end = service->attributes[service->num_handles - 1]->handle;
I suggest
grp_end = grp_start + num_handles - 1
because not all attributes must be in use. First attribute must be
filled, because it's service decl.
> +
> + if (grp_end < search_data->start_handle ||
> + grp_start > search_data->end_handle)
> + return;
> +
> + if (service->attributes[0]->handle < search_data->start_handle ||
> + service->attributes[0]->handle > search_data->end_handle)
> return;
>
> /* Remember size of uuid */
> --
> 2.1.0.rc2.206.gedb03e5
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
BR
Marcin
Added items for adding packed structs for ATT PDUs and improving unit test
coverage.
---
TODO | 15 +++++++++++++++
1 file changed, 15 insertions(+)
diff --git a/TODO b/TODO
index fe5ebe5..b5a9612 100644
--- a/TODO
+++ b/TODO
@@ -105,6 +105,21 @@ ATT/GATT (new shared stack)
Priority: Medium
Complexity: C1
+- Add complete GATT test coverage in unit/test-gatt following the GATT test
+ spec. This could use shared/gatt-client and shared/gatt-server at the same
+ time to test both against eachother. We should definitely have tests for
+ gatt-server and gatt-client simultaneously on one side of the connection.
+
+ Priority: High
+ Complexity: C4
+
+- Define packed structs for ATT protocol PDUs in shared/att-types to improve
+ readability. We should probably do this once there are extensive unit tests
+ for gatt-client/gatt-server so that we don't accidentally break working code.
+
+ Priority: Medium
+ Complexity: C2
+
- Introduce a handler interface to shared/gatt-client which can be used by the
upper layer to determine when the link has been disconnected or an ATT
protocol request times out.
--
2.1.0.rc2.206.gedb03e5
This patch fixes bug in gatt_db_read_by_grp_type in which the range end handle
was being ignored. This caused incorrect results to be sent for a database
where services exists beyond the end handle provided in the request.
---
src/shared/gatt-db.c | 11 ++++++++++-
1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/src/shared/gatt-db.c b/src/shared/gatt-db.c
index b3f95d2..4e74030 100644
--- a/src/shared/gatt-db.c
+++ b/src/shared/gatt-db.c
@@ -428,6 +428,7 @@ static void read_by_group_type(void *data, void *user_data)
{
struct read_by_group_type_data *search_data = user_data;
struct gatt_db_service *service = data;
+ uint16_t grp_start, grp_end;
if (!service->active)
return;
@@ -439,7 +440,15 @@ static void read_by_group_type(void *data, void *user_data)
if (bt_uuid_cmp(&search_data->uuid, &service->attributes[0]->uuid))
return;
- if (service->attributes[0]->handle < search_data->start_handle)
+ grp_start = service->attributes[0]->handle;
+ grp_end = service->attributes[service->num_handles - 1]->handle;
+
+ if (grp_end < search_data->start_handle ||
+ grp_start > search_data->end_handle)
+ return;
+
+ if (service->attributes[0]->handle < search_data->start_handle ||
+ service->attributes[0]->handle > search_data->end_handle)
return;
/* Remember size of uuid */
--
2.1.0.rc2.206.gedb03e5
The service changed handler now prints the command line prompt if the
changed service list empty.
---
tools/btgatt-client.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/tools/btgatt-client.c b/tools/btgatt-client.c
index ea9acb7..5eedfd0 100644
--- a/tools/btgatt-client.c
+++ b/tools/btgatt-client.c
@@ -304,8 +304,11 @@ static void service_changed_cb(uint16_t start_handle, uint16_t end_handle,
printf("\nService Changed handled - start: 0x%04x end: 0x%04x\n",
start_handle, end_handle);
- if (!bt_gatt_service_iter_next_by_handle(&iter, start_handle, &service))
+ if (!bt_gatt_service_iter_next_by_handle(&iter, start_handle,
+ &service)) {
+ print_prompt();
return;
+ }
print_service(service);
--
2.1.0.rc2.206.gedb03e5
This patch fixes a bug where the upper layer never got notified of a
service changed event if no new services are added within a changed
service range. This also fixes a bug where an empty changed service list
or ATT error caused pending service changed events to not get processed.
---
src/shared/gatt-client.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/src/shared/gatt-client.c b/src/shared/gatt-client.c
index faae71c..a3bb86d 100644
--- a/src/shared/gatt-client.c
+++ b/src/shared/gatt-client.c
@@ -965,16 +965,16 @@ static void service_changed_complete(struct discovery_op *op, bool success,
client->in_svc_chngd = false;
- if (!success) {
+ if (!success && att_ecode != BT_ATT_ERROR_ATTRIBUTE_NOT_FOUND) {
util_debug(client->debug_callback, client->debug_data,
"Failed to discover services within changed range - "
"error: 0x%02x", att_ecode);
- return;
+ goto next;
}
/* No new services in the modified range */
if (!op->result_head || !op->result_tail)
- return;
+ goto next;
start_handle = op->result_head->service.start_handle;
end_handle = op->result_tail->service.end_handle;
@@ -984,6 +984,7 @@ static void service_changed_complete(struct discovery_op *op, bool success,
service_list_insert_services(&client->svc_head, &client->svc_tail,
op->result_head, op->result_tail);
+next:
/* Notify the upper layer of changed services */
if (client->svc_chngd_callback)
client->svc_chngd_callback(start_handle, end_handle,
--
2.1.0.rc2.206.gedb03e5
This patch fixes an error in write-long-value command that caused an
extra invalid byte to get sent at the end of the long write value.
---
tools/btgatt-client.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/btgatt-client.c b/tools/btgatt-client.c
index e7cd31c..ea9acb7 100644
--- a/tools/btgatt-client.c
+++ b/tools/btgatt-client.c
@@ -709,7 +709,7 @@ static void cmd_write_long_value(struct client *cli, char *cmd_str)
return;
}
- length = argc - 1;
+ length = argc - 2;
if (length > 0) {
if (length > UINT16_MAX) {
--
2.1.0.rc2.206.gedb03e5
This patch fixes the way read-long-value command parses the "offset"
argument by interpreting it as base 10 instead of 16, which is more
intuitive.
---
tools/btgatt-client.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/tools/btgatt-client.c b/tools/btgatt-client.c
index b7bfa24..e7cd31c 100644
--- a/tools/btgatt-client.c
+++ b/tools/btgatt-client.c
@@ -486,8 +486,8 @@ static void cmd_read_long_value(struct client *cli, char *cmd_str)
}
endptr = NULL;
- offset = strtol(argv[1], &endptr, 16);
- if (!endptr || *endptr != '\0' || !handle) {
+ offset = strtol(argv[1], &endptr, 10);
+ if (!endptr || *endptr != '\0') {
printf("Invalid offset: %s\n", argv[1]);
return;
}
--
2.1.0.rc2.206.gedb03e5
This patch fixes a bug where the handle range for queued service changed
events weren't getting stored. This caused service discovery to be
initiated for the 0x0000-0x0000 range, which caused an error response.
---
src/shared/gatt-client.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/src/shared/gatt-client.c b/src/shared/gatt-client.c
index 57d3e1f..faae71c 100644
--- a/src/shared/gatt-client.c
+++ b/src/shared/gatt-client.c
@@ -1095,6 +1095,9 @@ static void service_changed_cb(uint16_t value_handle, const uint8_t *value,
if (!op)
return;
+ op->start_handle = start;
+ op->end_handle = end;
+
queue_push_tail(client->svc_chngd_queue, op);
}
--
2.1.0.rc2.206.gedb03e5