* [PATCH 0/4] shared/gatt: Couple fixes and improvements
@ 2015-11-16 21:08 Łukasz Rymanowski
2015-11-16 21:08 ` [PATCH 1/4] shared/gatt-server: Fix exit on error during execute write Łukasz Rymanowski
` (3 more replies)
0 siblings, 4 replies; 19+ messages in thread
From: Łukasz Rymanowski @ 2015-11-16 21:08 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Łukasz Rymanowski
I've back to work on making Android gatt to make use of shared/gatt.
Here is couple of fixes I've done to shared code when doing this work.
Android gatt still needs testing but will be available sooner or later :)
Łukasz Rymanowski (4):
shared/gatt-server: Fix exit on error during execute write
shared/gatt-client: Add notify type to notification callback
shared/gatt-server: Improve long write session handling
shared/gatt-server: Cancel remaining write exec on error in the middle
profiles/scanparam/scan.c | 5 ++-
src/gatt-client.c | 4 +-
src/shared/gatt-client.c | 13 ++++--
src/shared/gatt-client.h | 13 ++++--
src/shared/gatt-server.c | 101 +++++++++++++++++++++++++++++++++++++++++++---
tools/btgatt-client.c | 2 +-
unit/test-gatt.c | 10 +++--
7 files changed, 127 insertions(+), 21 deletions(-)
--
2.5.0
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 1/4] shared/gatt-server: Fix exit on error during execute write
2015-11-16 21:08 [PATCH 0/4] shared/gatt: Couple fixes and improvements Łukasz Rymanowski
@ 2015-11-16 21:08 ` Łukasz Rymanowski
2015-11-16 21:08 ` [PATCH 2/4] shared/gatt-client: Add notify type to notification callback Łukasz Rymanowski
` (2 subsequent siblings)
3 siblings, 0 replies; 19+ messages in thread
From: Łukasz Rymanowski @ 2015-11-16 21:08 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Łukasz Rymanowski
If there is an error during execute write we should drop all
outstanding data
---
src/shared/gatt-server.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/src/shared/gatt-server.c b/src/shared/gatt-server.c
index ba668e3..329f53f 100644
--- a/src/shared/gatt-server.c
+++ b/src/shared/gatt-server.c
@@ -1204,6 +1204,9 @@ static void exec_next_prep_write(struct bt_gatt_server *server,
err = BT_ATT_ERROR_UNLIKELY;
error:
+ queue_remove_all(server->prep_queue, NULL, NULL,
+ prep_write_data_destroy);
+
bt_att_send_error_rsp(server->att, BT_ATT_OP_EXEC_WRITE_REQ,
ehandle, err);
}
--
2.5.0
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH 2/4] shared/gatt-client: Add notify type to notification callback
2015-11-16 21:08 [PATCH 0/4] shared/gatt: Couple fixes and improvements Łukasz Rymanowski
2015-11-16 21:08 ` [PATCH 1/4] shared/gatt-server: Fix exit on error during execute write Łukasz Rymanowski
@ 2015-11-16 21:08 ` Łukasz Rymanowski
2015-11-17 14:10 ` Luiz Augusto von Dentz
2015-11-16 21:08 ` [PATCH 3/4] shared/gatt-server: Improve long write session handling Łukasz Rymanowski
2015-11-16 21:08 ` [PATCH 4/4] shared/gatt-server: Cancel remaining write exec on error in the middle Łukasz Rymanowski
3 siblings, 1 reply; 19+ messages in thread
From: Łukasz Rymanowski @ 2015-11-16 21:08 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Łukasz Rymanowski
Android expect information either its indication or notification.
This patch adds that.
---
profiles/scanparam/scan.c | 5 +++--
src/gatt-client.c | 4 ++--
src/shared/gatt-client.c | 13 +++++++++----
src/shared/gatt-client.h | 13 ++++++++++---
tools/btgatt-client.c | 2 +-
unit/test-gatt.c | 10 ++++++----
6 files changed, 31 insertions(+), 16 deletions(-)
diff --git a/profiles/scanparam/scan.c b/profiles/scanparam/scan.c
index 4015b3f..fd01135 100644
--- a/profiles/scanparam/scan.c
+++ b/profiles/scanparam/scan.c
@@ -92,8 +92,9 @@ static void write_scan_params(struct scan *scan)
false, value, sizeof(value));
}
-static void refresh_value_cb(uint16_t value_handle, const uint8_t *value,
- uint16_t length, void *user_data)
+static void refresh_value_cb(notify_t type, uint16_t value_handle,
+ const uint8_t *value, uint16_t length,
+ void *user_data)
{
struct scan *scan = user_data;
diff --git a/src/gatt-client.c b/src/gatt-client.c
index 39f6646..e74803c 100644
--- a/src/gatt-client.c
+++ b/src/gatt-client.c
@@ -1057,8 +1057,8 @@ static bool match_notify_sender(const void *a, const void *b)
return strcmp(client->owner, sender) == 0;
}
-static void notify_cb(uint16_t value_handle, const uint8_t *value,
- uint16_t length, void *user_data)
+static void notify_cb(notify_t type, uint16_t value_handle,
+ const uint8_t *value, uint16_t length, void *user_data)
{
struct async_dbus_op *op = user_data;
struct notify_client *client = op->data;
diff --git a/src/shared/gatt-client.c b/src/shared/gatt-client.c
index 06ac763..76e4a9a 100644
--- a/src/shared/gatt-client.c
+++ b/src/shared/gatt-client.c
@@ -1109,7 +1109,7 @@ struct service_changed_op {
static void process_service_changed(struct bt_gatt_client *client,
uint16_t start_handle,
uint16_t end_handle);
-static void service_changed_cb(uint16_t value_handle, const uint8_t *value,
+static void service_changed_cb(notify_t type, uint16_t value_handle, const uint8_t *value,
uint16_t length, void *user_data);
static void complete_notify_request(void *data)
@@ -1446,8 +1446,9 @@ fail:
" after Service Changed");
}
-static void service_changed_cb(uint16_t value_handle, const uint8_t *value,
- uint16_t length, void *user_data)
+static void service_changed_cb(notify_t type, uint16_t value_handle,
+ const uint8_t *value, uint16_t length,
+ void *user_data)
{
struct bt_gatt_client *client = user_data;
struct service_changed_op *op;
@@ -1545,6 +1546,7 @@ static bool gatt_client_init(struct bt_gatt_client *client, uint16_t mtu)
}
struct pdu_data {
+ notify_t type;
const void *pdu;
uint16_t length;
};
@@ -1614,7 +1616,8 @@ static void notify_handler(void *data, void *user_data)
* CCC, there is really no reason not to notify the handlers.
*/
if (notify_data->notify)
- notify_data->notify(value_handle, value, pdu_data->length - 2,
+ notify_data->notify(pdu_data->type, value_handle, value,
+ pdu_data->length - 2,
notify_data->user_data);
}
@@ -1629,6 +1632,8 @@ static void notify_cb(uint8_t opcode, const void *pdu, uint16_t length,
memset(&pdu_data, 0, sizeof(pdu_data));
pdu_data.pdu = pdu;
pdu_data.length = length;
+ pdu_data.type = opcode == BT_ATT_OP_HANDLE_VAL_IND ?
+ BT_GATT_INDICATION : BT_GATT_NOTIFICATION;
queue_foreach(client->notify_list, notify_handler, &pdu_data);
diff --git a/src/shared/gatt-client.h b/src/shared/gatt-client.h
index befa43f..3018a99 100644
--- a/src/shared/gatt-client.h
+++ b/src/shared/gatt-client.h
@@ -27,6 +27,11 @@
#define BT_GATT_UUID_SIZE 16
+typedef enum {
+ BT_GATT_NOTIFICATION,
+ BT_GATT_INDICATION,
+} notify_t;
+
struct bt_gatt_client;
struct bt_gatt_client *bt_gatt_client_new(struct gatt_db *db,
@@ -46,9 +51,11 @@ typedef void (*bt_gatt_client_read_callback_t)(bool success, uint8_t att_ecode,
typedef void (*bt_gatt_client_write_long_callback_t)(bool success,
bool reliable_error, uint8_t att_ecode,
void *user_data);
-typedef void (*bt_gatt_client_notify_callback_t)(uint16_t value_handle,
- const uint8_t *value, uint16_t length,
- void *user_data);
+typedef void (*bt_gatt_client_notify_callback_t)(notify_t type,
+ uint16_t value_handle,
+ const uint8_t *value,
+ uint16_t length,
+ void *user_data);
typedef void (*bt_gatt_client_register_callback_t)(uint16_t att_ecode,
void *user_data);
typedef void (*bt_gatt_client_service_changed_callback_t)(uint16_t start_handle,
diff --git a/tools/btgatt-client.c b/tools/btgatt-client.c
index 0f6a1bd..76fd6d0 100644
--- a/tools/btgatt-client.c
+++ b/tools/btgatt-client.c
@@ -1099,7 +1099,7 @@ static void register_notify_usage(void)
printf("Usage: register-notify <chrc value handle>\n");
}
-static void notify_cb(uint16_t value_handle, const uint8_t *value,
+static void notify_cb(notify_t type, uint16_t value_handle, const uint8_t *value,
uint16_t length, void *user_data)
{
int i;
diff --git a/unit/test-gatt.c b/unit/test-gatt.c
index 326a32c..b6d4ab6 100644
--- a/unit/test-gatt.c
+++ b/unit/test-gatt.c
@@ -2202,8 +2202,9 @@ static const struct test_step test_long_read_15 = {
.expected_att_ecode = 0x0c
};
-static void notification_cb(uint16_t value_handle, const uint8_t *value,
- uint16_t length, void *user_data)
+static void notification_cb(notify_t type, uint16_t value_handle,
+ const uint8_t *value, uint16_t length,
+ void *user_data)
{
struct context *context = user_data;
const struct test_step *step = context->data->step;
@@ -2270,8 +2271,9 @@ static void test_server_indication_confirm(struct context *context)
g_assert(indication_received == 1);
}
-static void indication_cb(uint16_t value_handle, const uint8_t *value,
- uint16_t length, void *user_data)
+static void indication_cb(notify_t type, uint16_t value_handle,
+ const uint8_t *value, uint16_t length,
+ void *user_data)
{
struct context *context = user_data;
const struct test_step *step = context->data->step;
--
2.5.0
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH 3/4] shared/gatt-server: Improve long write session handling
2015-11-16 21:08 [PATCH 0/4] shared/gatt: Couple fixes and improvements Łukasz Rymanowski
2015-11-16 21:08 ` [PATCH 1/4] shared/gatt-server: Fix exit on error during execute write Łukasz Rymanowski
2015-11-16 21:08 ` [PATCH 2/4] shared/gatt-client: Add notify type to notification callback Łukasz Rymanowski
@ 2015-11-16 21:08 ` Łukasz Rymanowski
2015-11-17 9:45 ` Grzegorz Kolodziejczyk
2015-11-17 12:12 ` Marcin Kraglak
2015-11-16 21:08 ` [PATCH 4/4] shared/gatt-server: Cancel remaining write exec on error in the middle Łukasz Rymanowski
3 siblings, 2 replies; 19+ messages in thread
From: Łukasz Rymanowski @ 2015-11-16 21:08 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Łukasz Rymanowski
With current implementation application layer has no idea when
long write session ends as it gets write callback with execute
write for each prepare write data.
With this patch, after data are completly received by gatt-server it
starts to call write callback in proper order with opcode
BT_ATT_OP_PREP_WRITE_REQ. After that, each characteristic gets
BT_ATT_OP_EXEC_WRITE_REQ so it knows that complete characteristic value
has been received and data can be marked as valid.
---
src/shared/gatt-server.c | 78 ++++++++++++++++++++++++++++++++++++++++++++----
1 file changed, 73 insertions(+), 5 deletions(-)
diff --git a/src/shared/gatt-server.c b/src/shared/gatt-server.c
index 329f53f..931d619 100644
--- a/src/shared/gatt-server.c
+++ b/src/shared/gatt-server.c
@@ -104,6 +104,8 @@ struct bt_gatt_server {
struct queue *prep_queue;
unsigned int max_prep_queue_len;
+ struct queue *exec_queue;
+
struct async_read_op *pending_read_op;
struct async_write_op *pending_write_op;
@@ -137,6 +139,7 @@ static void bt_gatt_server_free(struct bt_gatt_server *server)
server->pending_write_op->server = NULL;
queue_destroy(server->prep_queue, prep_write_data_destroy);
+ queue_destroy(server->exec_queue, NULL);
gatt_db_unref(server->db);
bt_att_unref(server->att);
@@ -1155,10 +1158,61 @@ error:
}
+static void exec_next_write(struct bt_gatt_server *server);
+
+static void exec_write_complete_cb(struct gatt_db_attribute *attr, int err,
+ void *user_data)
+{
+ struct bt_gatt_server *server = user_data;
+ uint16_t handle = gatt_db_attribute_get_handle(attr);
+
+ if (err) {
+ queue_remove_all(server->exec_queue, NULL, NULL, NULL);
+ bt_att_send_error_rsp(server->att, BT_ATT_OP_EXEC_WRITE_REQ,
+ handle, err);
+ }
+
+ exec_next_write(server);
+}
+
+static void exec_next_write(struct bt_gatt_server *server)
+{
+ uint16_t *handle;
+ struct gatt_db_attribute *attr;
+ bool status;
+ int err;
+
+ handle = queue_pop_head(server->exec_queue);
+ if (!handle) {
+ bt_att_send(server->att, BT_ATT_OP_EXEC_WRITE_RSP, NULL, 0,
+ NULL, NULL, NULL);
+ }
+
+ attr = gatt_db_get_attribute(server->db, PTR_TO_UINT(handle));
+ if (!attr) {
+ err = BT_ATT_ERROR_UNLIKELY;
+ goto error;
+ }
+
+ status = gatt_db_attribute_write(attr, 0, NULL , 0,
+ BT_ATT_OP_EXEC_WRITE_REQ,
+ server->att,
+ exec_write_complete_cb, server);
+ if (status)
+ return;
+
+ err = BT_ATT_ERROR_UNLIKELY;
+
+error:
+ queue_remove_all(server->exec_queue, NULL, NULL, NULL);
+ bt_att_send_error_rsp(server->att, BT_ATT_OP_EXEC_WRITE_REQ,
+ PTR_TO_UINT(handle), err);
+}
+
static void exec_next_prep_write(struct bt_gatt_server *server,
uint16_t ehandle, int err);
-static void exec_write_complete_cb(struct gatt_db_attribute *attr, int err,
+static void prep_write_complete_cb(struct gatt_db_attribute *attr, int err,
void *user_data)
{
struct bt_gatt_server *server = user_data;
@@ -1167,6 +1221,11 @@ static void exec_write_complete_cb(struct gatt_db_attribute *attr, int err,
exec_next_prep_write(server, handle, err);
}
+static bool match_attribute_handle(const void *data, const void *match_data)
+{
+ return PTR_TO_UINT(data) == PTR_TO_UINT(match_data);
+}
+
static void exec_next_prep_write(struct bt_gatt_server *server,
uint16_t ehandle, int err)
{
@@ -1177,10 +1236,17 @@ static void exec_next_prep_write(struct bt_gatt_server *server,
if (err)
goto error;
+ /*
+ * Remember to which handles we need to send execute after reliable
+ * or long write session is done
+ */
+ if (ehandle && !queue_find(server->exec_queue, match_attribute_handle,
+ UINT_TO_PTR(ehandle)))
+ queue_push_tail(server->exec_queue, UINT_TO_PTR(ehandle));
+
next = queue_pop_head(server->prep_queue);
if (!next) {
- bt_att_send(server->att, BT_ATT_OP_EXEC_WRITE_RSP, NULL, 0,
- NULL, NULL, NULL);
+ exec_next_write(server);
return;
}
@@ -1192,9 +1258,9 @@ static void exec_next_prep_write(struct bt_gatt_server *server,
status = gatt_db_attribute_write(attr, next->offset,
next->value, next->length,
- BT_ATT_OP_EXEC_WRITE_REQ,
+ BT_ATT_OP_PREP_WRITE_REQ,
server->att,
- exec_write_complete_cb, server);
+ prep_write_complete_cb, server);
prep_write_data_destroy(next);
@@ -1396,6 +1462,8 @@ struct bt_gatt_server *bt_gatt_server_new(struct gatt_db *db,
server->max_prep_queue_len = DEFAULT_MAX_PREP_QUEUE_LEN;
server->prep_queue = queue_new();
+ server->exec_queue = queue_new();
+
if (!gatt_server_register_att_handlers(server)) {
bt_gatt_server_free(server);
return NULL;
--
2.5.0
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH 4/4] shared/gatt-server: Cancel remaining write exec on error in the middle
2015-11-16 21:08 [PATCH 0/4] shared/gatt: Couple fixes and improvements Łukasz Rymanowski
` (2 preceding siblings ...)
2015-11-16 21:08 ` [PATCH 3/4] shared/gatt-server: Improve long write session handling Łukasz Rymanowski
@ 2015-11-16 21:08 ` Łukasz Rymanowski
2015-11-17 9:57 ` Grzegorz Kolodziejczyk
2015-11-17 15:22 ` Marcin Kraglak
3 siblings, 2 replies; 19+ messages in thread
From: Łukasz Rymanowski @ 2015-11-16 21:08 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Łukasz Rymanowski
If one execute write fail, server sends error response to the client.
All characteristics before the failing one has been written and we cannot
do anything about that. All the following characteristics will be
canceled with this patch.
According to the spec BT Core 4.2, Vol.3, Part F, chapter 3.4.6.3.
in case of error, state of attributes is not defined, which makes
this behavior ok.
---
src/shared/gatt-server.c | 22 +++++++++++++++++++++-
1 file changed, 21 insertions(+), 1 deletion(-)
diff --git a/src/shared/gatt-server.c b/src/shared/gatt-server.c
index 931d619..3cd5190 100644
--- a/src/shared/gatt-server.c
+++ b/src/shared/gatt-server.c
@@ -1158,6 +1158,22 @@ error:
}
+static void exec_write_cancel(void *data, void *user_data)
+{
+ uint16_t handle = PTR_TO_UINT(data);
+ struct bt_gatt_server *server = user_data;
+ struct gatt_db_attribute *attr;
+ uint8_t cancel = 0;
+
+ attr = gatt_db_get_attribute(server->db, PTR_TO_UINT(handle));
+ if (!attr) {
+ return;
+ }
+
+ gatt_db_attribute_write(attr, 0, &cancel , 1, BT_ATT_OP_EXEC_WRITE_REQ,
+ server->att, NULL, NULL);
+}
+
static void exec_next_write(struct bt_gatt_server *server);
static void exec_write_complete_cb(struct gatt_db_attribute *attr, int err,
@@ -1167,6 +1183,7 @@ static void exec_write_complete_cb(struct gatt_db_attribute *attr, int err,
uint16_t handle = gatt_db_attribute_get_handle(attr);
if (err) {
+ queue_foreach(server->exec_queue, exec_write_cancel, server);
queue_remove_all(server->exec_queue, NULL, NULL, NULL);
bt_att_send_error_rsp(server->att, BT_ATT_OP_EXEC_WRITE_REQ,
handle, err);
@@ -1178,6 +1195,7 @@ static void exec_write_complete_cb(struct gatt_db_attribute *attr, int err,
static void exec_next_write(struct bt_gatt_server *server)
{
uint16_t *handle;
+ uint8_t execute = 1;
struct gatt_db_attribute *attr;
bool status;
int err;
@@ -1194,7 +1212,7 @@ static void exec_next_write(struct bt_gatt_server *server)
goto error;
}
- status = gatt_db_attribute_write(attr, 0, NULL , 0,
+ status = gatt_db_attribute_write(attr, 0, &execute , 1,
BT_ATT_OP_EXEC_WRITE_REQ,
server->att,
exec_write_complete_cb, server);
@@ -1204,6 +1222,8 @@ static void exec_next_write(struct bt_gatt_server *server)
err = BT_ATT_ERROR_UNLIKELY;
error:
+
+ queue_foreach(server->exec_queue, exec_write_cancel, server);
queue_remove_all(server->exec_queue, NULL, NULL, NULL);
bt_att_send_error_rsp(server->att, BT_ATT_OP_EXEC_WRITE_REQ,
PTR_TO_UINT(handle), err);
--
2.5.0
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH 3/4] shared/gatt-server: Improve long write session handling
2015-11-16 21:08 ` [PATCH 3/4] shared/gatt-server: Improve long write session handling Łukasz Rymanowski
@ 2015-11-17 9:45 ` Grzegorz Kolodziejczyk
2015-11-17 12:12 ` Marcin Kraglak
1 sibling, 0 replies; 19+ messages in thread
From: Grzegorz Kolodziejczyk @ 2015-11-17 9:45 UTC (permalink / raw)
To: Łukasz Rymanowski; +Cc: linux-bluetooth
Hi Lukasz,
On 16 November 2015 at 22:08, Łukasz Rymanowski
<lukasz.rymanowski@codecoup.pl> wrote:
>
> With current implementation application layer has no idea when
> long write session ends as it gets write callback with execute
> write for each prepare write data.
> With this patch, after data are completly received by gatt-server it
> starts to call write callback in proper order with opcode
> BT_ATT_OP_PREP_WRITE_REQ. After that, each characteristic gets
> BT_ATT_OP_EXEC_WRITE_REQ so it knows that complete characteristic value
> has been received and data can be marked as valid.
> ---
> src/shared/gatt-server.c | 78 ++++++++++++++++++++++++++++++++++++++++++++----
> 1 file changed, 73 insertions(+), 5 deletions(-)
>
> diff --git a/src/shared/gatt-server.c b/src/shared/gatt-server.c
> index 329f53f..931d619 100644
> --- a/src/shared/gatt-server.c
> +++ b/src/shared/gatt-server.c
> @@ -104,6 +104,8 @@ struct bt_gatt_server {
> struct queue *prep_queue;
> unsigned int max_prep_queue_len;
>
> + struct queue *exec_queue;
> +
> struct async_read_op *pending_read_op;
> struct async_write_op *pending_write_op;
>
> @@ -137,6 +139,7 @@ static void bt_gatt_server_free(struct bt_gatt_server *server)
> server->pending_write_op->server = NULL;
>
> queue_destroy(server->prep_queue, prep_write_data_destroy);
> + queue_destroy(server->exec_queue, NULL);
>
> gatt_db_unref(server->db);
> bt_att_unref(server->att);
> @@ -1155,10 +1158,61 @@ error:
>
> }
>
> +static void exec_next_write(struct bt_gatt_server *server);
> +
> +static void exec_write_complete_cb(struct gatt_db_attribute *attr, int err,
> + void *user_data)
> +{
> + struct bt_gatt_server *server = user_data;
> + uint16_t handle = gatt_db_attribute_get_handle(attr);
> +
> + if (err) {
> + queue_remove_all(server->exec_queue, NULL, NULL, NULL);
> + bt_att_send_error_rsp(server->att, BT_ATT_OP_EXEC_WRITE_REQ,
> + handle, err);
> + }
> +
> + exec_next_write(server);
> +}
> +
> +static void exec_next_write(struct bt_gatt_server *server)
> +{
> + uint16_t *handle;
> + struct gatt_db_attribute *attr;
> + bool status;
> + int err;
> +
> + handle = queue_pop_head(server->exec_queue);
> + if (!handle) {
> + bt_att_send(server->att, BT_ATT_OP_EXEC_WRITE_RSP, NULL, 0,
> + NULL, NULL, NULL);
> + }
Single command "ifs" don't require brackets.
>
> +
> + attr = gatt_db_get_attribute(server->db, PTR_TO_UINT(handle));
> + if (!attr) {
> + err = BT_ATT_ERROR_UNLIKELY;
> + goto error;
> + }
> +
> + status = gatt_db_attribute_write(attr, 0, NULL , 0,
> + BT_ATT_OP_EXEC_WRITE_REQ,
> + server->att,
> + exec_write_complete_cb, server);
> + if (status)
> + return;
> +
> + err = BT_ATT_ERROR_UNLIKELY;
> +
> +error:
> + queue_remove_all(server->exec_queue, NULL, NULL, NULL);
> + bt_att_send_error_rsp(server->att, BT_ATT_OP_EXEC_WRITE_REQ,
> + PTR_TO_UINT(handle), err);
> +}
> +
> static void exec_next_prep_write(struct bt_gatt_server *server,
> uint16_t ehandle, int err);
>
> -static void exec_write_complete_cb(struct gatt_db_attribute *attr, int err,
> +static void prep_write_complete_cb(struct gatt_db_attribute *attr, int err,
> void *user_data)
> {
> struct bt_gatt_server *server = user_data;
> @@ -1167,6 +1221,11 @@ static void exec_write_complete_cb(struct gatt_db_attribute *attr, int err,
> exec_next_prep_write(server, handle, err);
> }
>
> +static bool match_attribute_handle(const void *data, const void *match_data)
> +{
> + return PTR_TO_UINT(data) == PTR_TO_UINT(match_data);
> +}
> +
> static void exec_next_prep_write(struct bt_gatt_server *server,
> uint16_t ehandle, int err)
> {
> @@ -1177,10 +1236,17 @@ static void exec_next_prep_write(struct bt_gatt_server *server,
> if (err)
> goto error;
>
> + /*
> + * Remember to which handles we need to send execute after reliable
> + * or long write session is done
> + */
> + if (ehandle && !queue_find(server->exec_queue, match_attribute_handle,
> + UINT_TO_PTR(ehandle)))
> + queue_push_tail(server->exec_queue, UINT_TO_PTR(ehandle));
> +
> next = queue_pop_head(server->prep_queue);
> if (!next) {
> - bt_att_send(server->att, BT_ATT_OP_EXEC_WRITE_RSP, NULL, 0,
> - NULL, NULL, NULL);
> + exec_next_write(server);
> return;
> }
>
> @@ -1192,9 +1258,9 @@ static void exec_next_prep_write(struct bt_gatt_server *server,
>
> status = gatt_db_attribute_write(attr, next->offset,
> next->value, next->length,
> - BT_ATT_OP_EXEC_WRITE_REQ,
> + BT_ATT_OP_PREP_WRITE_REQ,
> server->att,
> - exec_write_complete_cb, server);
> + prep_write_complete_cb, server);
>
> prep_write_data_destroy(next);
>
> @@ -1396,6 +1462,8 @@ struct bt_gatt_server *bt_gatt_server_new(struct gatt_db *db,
> server->max_prep_queue_len = DEFAULT_MAX_PREP_QUEUE_LEN;
> server->prep_queue = queue_new();
>
> + server->exec_queue = queue_new();
> +
> if (!gatt_server_register_att_handlers(server)) {
> bt_gatt_server_free(server);
> return NULL;
> --
> 2.5.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
BR,
Grzegorz
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 4/4] shared/gatt-server: Cancel remaining write exec on error in the middle
2015-11-16 21:08 ` [PATCH 4/4] shared/gatt-server: Cancel remaining write exec on error in the middle Łukasz Rymanowski
@ 2015-11-17 9:57 ` Grzegorz Kolodziejczyk
2015-11-17 21:05 ` Łukasz Rymanowski
2015-11-17 15:22 ` Marcin Kraglak
1 sibling, 1 reply; 19+ messages in thread
From: Grzegorz Kolodziejczyk @ 2015-11-17 9:57 UTC (permalink / raw)
To: Łukasz Rymanowski; +Cc: linux-bluetooth
Hi Lukasz,
On 16 November 2015 at 22:08, Łukasz Rymanowski
<lukasz.rymanowski@codecoup.pl> wrote:
> If one execute write fail, server sends error response to the client.
> All characteristics before the failing one has been written and we cannot
> do anything about that. All the following characteristics will be
> canceled with this patch.
>
> According to the spec BT Core 4.2, Vol.3, Part F, chapter 3.4.6.3.
> in case of error, state of attributes is not defined, which makes
> this behavior ok.
> ---
> src/shared/gatt-server.c | 22 +++++++++++++++++++++-
> 1 file changed, 21 insertions(+), 1 deletion(-)
>
> diff --git a/src/shared/gatt-server.c b/src/shared/gatt-server.c
> index 931d619..3cd5190 100644
> --- a/src/shared/gatt-server.c
> +++ b/src/shared/gatt-server.c
> @@ -1158,6 +1158,22 @@ error:
>
> }
>
> +static void exec_write_cancel(void *data, void *user_data)
> +{
> + uint16_t handle = PTR_TO_UINT(data);
> + struct bt_gatt_server *server = user_data;
> + struct gatt_db_attribute *attr;
> + uint8_t cancel = 0;
> +
> + attr = gatt_db_get_attribute(server->db, PTR_TO_UINT(handle));
> + if (!attr) {
> + return;
> + }
No brackets.
> +
> + gatt_db_attribute_write(attr, 0, &cancel , 1, BT_ATT_OP_EXEC_WRITE_REQ,
Shouldn't be len - sizeof value ? instead of "1" ?
> + server->att, NULL, NULL);
> +}
> +
> static void exec_next_write(struct bt_gatt_server *server);
>
> static void exec_write_complete_cb(struct gatt_db_attribute *attr, int err,
> @@ -1167,6 +1183,7 @@ static void exec_write_complete_cb(struct gatt_db_attribute *attr, int err,
> uint16_t handle = gatt_db_attribute_get_handle(attr);
>
> if (err) {
> + queue_foreach(server->exec_queue, exec_write_cancel, server);
> queue_remove_all(server->exec_queue, NULL, NULL, NULL);
> bt_att_send_error_rsp(server->att, BT_ATT_OP_EXEC_WRITE_REQ,
> handle, err);
> @@ -1178,6 +1195,7 @@ static void exec_write_complete_cb(struct gatt_db_attribute *attr, int err,
> static void exec_next_write(struct bt_gatt_server *server)
> {
> uint16_t *handle;
> + uint8_t execute = 1;
> struct gatt_db_attribute *attr;
> bool status;
> int err;
> @@ -1194,7 +1212,7 @@ static void exec_next_write(struct bt_gatt_server *server)
> goto error;
> }
>
> - status = gatt_db_attribute_write(attr, 0, NULL , 0,
> + status = gatt_db_attribute_write(attr, 0, &execute , 1,
ditto (sizeof)
> BT_ATT_OP_EXEC_WRITE_REQ,
> server->att,
> exec_write_complete_cb, server);
> @@ -1204,6 +1222,8 @@ static void exec_next_write(struct bt_gatt_server *server)
> err = BT_ATT_ERROR_UNLIKELY;
>
> error:
> +
> + queue_foreach(server->exec_queue, exec_write_cancel, server);
> queue_remove_all(server->exec_queue, NULL, NULL, NULL);
> bt_att_send_error_rsp(server->att, BT_ATT_OP_EXEC_WRITE_REQ,
> PTR_TO_UINT(handle), err);
> --
> 2.5.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
BR,
Grzegorz
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 3/4] shared/gatt-server: Improve long write session handling
2015-11-16 21:08 ` [PATCH 3/4] shared/gatt-server: Improve long write session handling Łukasz Rymanowski
2015-11-17 9:45 ` Grzegorz Kolodziejczyk
@ 2015-11-17 12:12 ` Marcin Kraglak
2015-11-17 12:49 ` Luiz Augusto von Dentz
2015-11-17 21:22 ` Łukasz Rymanowski
1 sibling, 2 replies; 19+ messages in thread
From: Marcin Kraglak @ 2015-11-17 12:12 UTC (permalink / raw)
To: Łukasz Rymanowski; +Cc: linux-bluetooth@vger.kernel.org development
Hi Lukasz,
On 16 November 2015 at 22:08, Łukasz Rymanowski
<lukasz.rymanowski@codecoup.pl> wrote:
>
> With current implementation application layer has no idea when
> long write session ends as it gets write callback with execute
> write for each prepare write data.
> With this patch, after data are completly received by gatt-server it
> starts to call write callback in proper order with opcode
> BT_ATT_OP_PREP_WRITE_REQ. After that, each characteristic gets
> BT_ATT_OP_EXEC_WRITE_REQ so it knows that complete characteristic value
> has been received and data can be marked as valid.
> ---
> src/shared/gatt-server.c | 78 ++++++++++++++++++++++++++++++++++++++++++++----
> 1 file changed, 73 insertions(+), 5 deletions(-)
>
> diff --git a/src/shared/gatt-server.c b/src/shared/gatt-server.c
> index 329f53f..931d619 100644
> --- a/src/shared/gatt-server.c
> +++ b/src/shared/gatt-server.c
> @@ -104,6 +104,8 @@ struct bt_gatt_server {
> struct queue *prep_queue;
> unsigned int max_prep_queue_len;
>
> + struct queue *exec_queue;
> +
> struct async_read_op *pending_read_op;
> struct async_write_op *pending_write_op;
>
> @@ -137,6 +139,7 @@ static void bt_gatt_server_free(struct bt_gatt_server *server)
> server->pending_write_op->server = NULL;
>
> queue_destroy(server->prep_queue, prep_write_data_destroy);
> + queue_destroy(server->exec_queue, NULL);
>
> gatt_db_unref(server->db);
> bt_att_unref(server->att);
> @@ -1155,10 +1158,61 @@ error:
>
> }
>
> +static void exec_next_write(struct bt_gatt_server *server);
> +
> +static void exec_write_complete_cb(struct gatt_db_attribute *attr, int err,
> + void *user_data)
> +{
> + struct bt_gatt_server *server = user_data;
> + uint16_t handle = gatt_db_attribute_get_handle(attr);
> +
> + if (err) {
> + queue_remove_all(server->exec_queue, NULL, NULL, NULL);
> + bt_att_send_error_rsp(server->att, BT_ATT_OP_EXEC_WRITE_REQ,
> + handle, err);
> + }
> +
> + exec_next_write(server);
> +}
> +
> +static void exec_next_write(struct bt_gatt_server *server)
> +{
> + uint16_t *handle;
> + struct gatt_db_attribute *attr;
> + bool status;
> + int err;
> +
> + handle = queue_pop_head(server->exec_queue);
> + if (!handle) {
> + bt_att_send(server->att, BT_ATT_OP_EXEC_WRITE_RSP, NULL, 0,
> + NULL, NULL, NULL);
> + }
> +
> + attr = gatt_db_get_attribute(server->db, PTR_TO_UINT(handle));
> + if (!attr) {
> + err = BT_ATT_ERROR_UNLIKELY;
> + goto error;
> + }
> +
> + status = gatt_db_attribute_write(attr, 0, NULL , 0,
> + BT_ATT_OP_EXEC_WRITE_REQ,
> + server->att,
> + exec_write_complete_cb, server);
> + if (status)
> + return;
> +
> + err = BT_ATT_ERROR_UNLIKELY;
> +
> +error:
> + queue_remove_all(server->exec_queue, NULL, NULL, NULL);
> + bt_att_send_error_rsp(server->att, BT_ATT_OP_EXEC_WRITE_REQ,
> + PTR_TO_UINT(handle), err);
> +}
> +
> static void exec_next_prep_write(struct bt_gatt_server *server,
> uint16_t ehandle, int err);
>
> -static void exec_write_complete_cb(struct gatt_db_attribute *attr, int err,
> +static void prep_write_complete_cb(struct gatt_db_attribute *attr, int err,
> void *user_data)
> {
> struct bt_gatt_server *server = user_data;
> @@ -1167,6 +1221,11 @@ static void exec_write_complete_cb(struct gatt_db_attribute *attr, int err,
> exec_next_prep_write(server, handle, err);
> }
>
> +static bool match_attribute_handle(const void *data, const void *match_data)
> +{
> + return PTR_TO_UINT(data) == PTR_TO_UINT(match_data);
> +}
> +
> static void exec_next_prep_write(struct bt_gatt_server *server,
> uint16_t ehandle, int err)
> {
> @@ -1177,10 +1236,17 @@ static void exec_next_prep_write(struct bt_gatt_server *server,
> if (err)
Shouldn't you remove all elements of server->exec_queue here? Looking
at the next patch, all attributes in exec_queue shall be notified that
exec write is canceled too.
And just open question: with introducing two types of write requests
in reliable write procedure: BT_ATT_OP_EXEC_WRITE_REQ and
BT_ATT_OP_PREP_WRITE_REQ, we could avoid using two queues and pass
prepare writes immediatelly to specific service implementation instead
of storing all values in prepare writes in server.
> goto error;
>
>
> + /*
> + * Remember to which handles we need to send execute after reliable
> + * or long write session is done
> + */
> + if (ehandle && !queue_find(server->exec_queue, match_attribute_handle,
> + UINT_TO_PTR(ehandle)))
> + queue_push_tail(server->exec_queue, UINT_TO_PTR(ehandle));
> +
> next = queue_pop_head(server->prep_queue);
> if (!next) {
> - bt_att_send(server->att, BT_ATT_OP_EXEC_WRITE_RSP, NULL, 0,
> - NULL, NULL, NULL);
> + exec_next_write(server);
> return;
> }
>
> @@ -1192,9 +1258,9 @@ static void exec_next_prep_write(struct bt_gatt_server *server,
>
> status = gatt_db_attribute_write(attr, next->offset,
> next->value, next->length,
> - BT_ATT_OP_EXEC_WRITE_REQ,
> + BT_ATT_OP_PREP_WRITE_REQ,
> server->att,
> - exec_write_complete_cb, server);
> + prep_write_complete_cb, server);
>
> prep_write_data_destroy(next);
>
> @@ -1396,6 +1462,8 @@ struct bt_gatt_server *bt_gatt_server_new(struct gatt_db *db,
> server->max_prep_queue_len = DEFAULT_MAX_PREP_QUEUE_LEN;
> server->prep_queue = queue_new();
>
> + server->exec_queue = queue_new();
> +
> if (!gatt_server_register_att_handlers(server)) {
> bt_gatt_server_free(server);
> return NULL;
> --
> 2.5.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
BR
Marcin Kraglak
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 3/4] shared/gatt-server: Improve long write session handling
2015-11-17 12:12 ` Marcin Kraglak
@ 2015-11-17 12:49 ` Luiz Augusto von Dentz
2015-11-17 21:32 ` Łukasz Rymanowski
2015-11-17 21:22 ` Łukasz Rymanowski
1 sibling, 1 reply; 19+ messages in thread
From: Luiz Augusto von Dentz @ 2015-11-17 12:49 UTC (permalink / raw)
To: Marcin Kraglak
Cc: Łukasz Rymanowski, linux-bluetooth@vger.kernel.org development
Hi Marcin,
On Tue, Nov 17, 2015 at 2:12 PM, Marcin Kraglak
<marcin.kraglak@tieto.com> wrote:
> Hi Lukasz,
>
> On 16 November 2015 at 22:08, Łukasz Rymanowski
> <lukasz.rymanowski@codecoup.pl> wrote:
>>
>> With current implementation application layer has no idea when
>> long write session ends as it gets write callback with execute
>> write for each prepare write data.
>> With this patch, after data are completly received by gatt-server it
>> starts to call write callback in proper order with opcode
>> BT_ATT_OP_PREP_WRITE_REQ. After that, each characteristic gets
>> BT_ATT_OP_EXEC_WRITE_REQ so it knows that complete characteristic value
>> has been received and data can be marked as valid.
>> ---
>> src/shared/gatt-server.c | 78 ++++++++++++++++++++++++++++++++++++++++++++----
>> 1 file changed, 73 insertions(+), 5 deletions(-)
>>
>> diff --git a/src/shared/gatt-server.c b/src/shared/gatt-server.c
>> index 329f53f..931d619 100644
>> --- a/src/shared/gatt-server.c
>> +++ b/src/shared/gatt-server.c
>> @@ -104,6 +104,8 @@ struct bt_gatt_server {
>> struct queue *prep_queue;
>> unsigned int max_prep_queue_len;
>>
>> + struct queue *exec_queue;
>> +
>> struct async_read_op *pending_read_op;
>> struct async_write_op *pending_write_op;
>>
>> @@ -137,6 +139,7 @@ static void bt_gatt_server_free(struct bt_gatt_server *server)
>> server->pending_write_op->server = NULL;
>>
>> queue_destroy(server->prep_queue, prep_write_data_destroy);
>> + queue_destroy(server->exec_queue, NULL);
>>
>> gatt_db_unref(server->db);
>> bt_att_unref(server->att);
>> @@ -1155,10 +1158,61 @@ error:
>>
>> }
>>
>> +static void exec_next_write(struct bt_gatt_server *server);
>> +
>> +static void exec_write_complete_cb(struct gatt_db_attribute *attr, int err,
>> + void *user_data)
>> +{
>> + struct bt_gatt_server *server = user_data;
>> + uint16_t handle = gatt_db_attribute_get_handle(attr);
>> +
>> + if (err) {
>> + queue_remove_all(server->exec_queue, NULL, NULL, NULL);
>> + bt_att_send_error_rsp(server->att, BT_ATT_OP_EXEC_WRITE_REQ,
>> + handle, err);
>> + }
>> +
>> + exec_next_write(server);
>> +}
>> +
>> +static void exec_next_write(struct bt_gatt_server *server)
>> +{
>> + uint16_t *handle;
>> + struct gatt_db_attribute *attr;
>> + bool status;
>> + int err;
>> +
>> + handle = queue_pop_head(server->exec_queue);
>> + if (!handle) {
>> + bt_att_send(server->att, BT_ATT_OP_EXEC_WRITE_RSP, NULL, 0,
>> + NULL, NULL, NULL);
>> + }
>> +
>> + attr = gatt_db_get_attribute(server->db, PTR_TO_UINT(handle));
>> + if (!attr) {
>> + err = BT_ATT_ERROR_UNLIKELY;
>> + goto error;
>> + }
>> +
>> + status = gatt_db_attribute_write(attr, 0, NULL , 0,
>> + BT_ATT_OP_EXEC_WRITE_REQ,
>> + server->att,
>> + exec_write_complete_cb, server);
>> + if (status)
>> + return;
>> +
>> + err = BT_ATT_ERROR_UNLIKELY;
>> +
>> +error:
>> + queue_remove_all(server->exec_queue, NULL, NULL, NULL);
>> + bt_att_send_error_rsp(server->att, BT_ATT_OP_EXEC_WRITE_REQ,
>> + PTR_TO_UINT(handle), err);
>> +}
>> +
>> static void exec_next_prep_write(struct bt_gatt_server *server,
>> uint16_t ehandle, int err);
>>
>> -static void exec_write_complete_cb(struct gatt_db_attribute *attr, int err,
>> +static void prep_write_complete_cb(struct gatt_db_attribute *attr, int err,
>> void *user_data)
>> {
>> struct bt_gatt_server *server = user_data;
>> @@ -1167,6 +1221,11 @@ static void exec_write_complete_cb(struct gatt_db_attribute *attr, int err,
>> exec_next_prep_write(server, handle, err);
>> }
>>
>> +static bool match_attribute_handle(const void *data, const void *match_data)
>> +{
>> + return PTR_TO_UINT(data) == PTR_TO_UINT(match_data);
>> +}
>> +
>> static void exec_next_prep_write(struct bt_gatt_server *server,
>> uint16_t ehandle, int err)
>> {
>> @@ -1177,10 +1236,17 @@ static void exec_next_prep_write(struct bt_gatt_server *server,
>> if (err)
> Shouldn't you remove all elements of server->exec_queue here? Looking
> at the next patch, all attributes in exec_queue shall be notified that
> exec write is canceled too.
> And just open question: with introducing two types of write requests
> in reliable write procedure: BT_ATT_OP_EXEC_WRITE_REQ and
> BT_ATT_OP_PREP_WRITE_REQ, we could avoid using two queues and pass
> prepare writes immediatelly to specific service implementation instead
> of storing all values in prepare writes in server.
Even if we have some flush logic trigger by execute we cannot really
dispatch writes immediately to the services because it can fail with
some errors that are not allowed by the testing spec, yes there is a
specific test checking the e.g. invalid offset is only receive as a
response to execute. It looks like this is because the value could be
changed in between prepare and execute, which IMO is something
horribly broken with prepare + execute since it is intended for
reliable writes but it can produce all sort of errors which is
impossible to guess without reading the value written after it has
been executed in case anything has change in between the commands.
>> goto error;
>>
>>
>> + /*
>> + * Remember to which handles we need to send execute after reliable
>> + * or long write session is done
>> + */
>> + if (ehandle && !queue_find(server->exec_queue, match_attribute_handle,
>> + UINT_TO_PTR(ehandle)))
>> + queue_push_tail(server->exec_queue, UINT_TO_PTR(ehandle));
>> +
>> next = queue_pop_head(server->prep_queue);
>> if (!next) {
>> - bt_att_send(server->att, BT_ATT_OP_EXEC_WRITE_RSP, NULL, 0,
>> - NULL, NULL, NULL);
>> + exec_next_write(server);
>> return;
>> }
>>
>> @@ -1192,9 +1258,9 @@ static void exec_next_prep_write(struct bt_gatt_server *server,
>>
>> status = gatt_db_attribute_write(attr, next->offset,
>> next->value, next->length,
>> - BT_ATT_OP_EXEC_WRITE_REQ,
>> + BT_ATT_OP_PREP_WRITE_REQ,
>> server->att,
>> - exec_write_complete_cb, server);
>> + prep_write_complete_cb, server);
>>
>> prep_write_data_destroy(next);
>>
>> @@ -1396,6 +1462,8 @@ struct bt_gatt_server *bt_gatt_server_new(struct gatt_db *db,
>> server->max_prep_queue_len = DEFAULT_MAX_PREP_QUEUE_LEN;
>> server->prep_queue = queue_new();
>>
>> + server->exec_queue = queue_new();
>> +
>> if (!gatt_server_register_att_handlers(server)) {
>> bt_gatt_server_free(server);
>> return NULL;
>> --
>> 2.5.0
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
>
>
>
> --
> BR
> Marcin Kraglak
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Luiz Augusto von Dentz
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/4] shared/gatt-client: Add notify type to notification callback
2015-11-16 21:08 ` [PATCH 2/4] shared/gatt-client: Add notify type to notification callback Łukasz Rymanowski
@ 2015-11-17 14:10 ` Luiz Augusto von Dentz
2015-11-17 22:03 ` Łukasz Rymanowski
0 siblings, 1 reply; 19+ messages in thread
From: Luiz Augusto von Dentz @ 2015-11-17 14:10 UTC (permalink / raw)
To: Łukasz Rymanowski; +Cc: linux-bluetooth
Hi Lukasz,
On Mon, Nov 16, 2015 at 11:08 PM, Łukasz Rymanowski
<lukasz.rymanowski@codecoup.pl> wrote:
> Android expect information either its indication or notification.
> This patch adds that.
> ---
> profiles/scanparam/scan.c | 5 +++--
> src/gatt-client.c | 4 ++--
> src/shared/gatt-client.c | 13 +++++++++----
> src/shared/gatt-client.h | 13 ++++++++++---
> tools/btgatt-client.c | 2 +-
> unit/test-gatt.c | 10 ++++++----
> 6 files changed, 31 insertions(+), 16 deletions(-)
>
> diff --git a/profiles/scanparam/scan.c b/profiles/scanparam/scan.c
> index 4015b3f..fd01135 100644
> --- a/profiles/scanparam/scan.c
> +++ b/profiles/scanparam/scan.c
> @@ -92,8 +92,9 @@ static void write_scan_params(struct scan *scan)
> false, value, sizeof(value));
> }
>
> -static void refresh_value_cb(uint16_t value_handle, const uint8_t *value,
> - uint16_t length, void *user_data)
> +static void refresh_value_cb(notify_t type, uint16_t value_handle,
> + const uint8_t *value, uint16_t length,
> + void *user_data)
> {
> struct scan *scan = user_data;
>
> diff --git a/src/gatt-client.c b/src/gatt-client.c
> index 39f6646..e74803c 100644
> --- a/src/gatt-client.c
> +++ b/src/gatt-client.c
> @@ -1057,8 +1057,8 @@ static bool match_notify_sender(const void *a, const void *b)
> return strcmp(client->owner, sender) == 0;
> }
>
> -static void notify_cb(uint16_t value_handle, const uint8_t *value,
> - uint16_t length, void *user_data)
> +static void notify_cb(notify_t type, uint16_t value_handle,
> + const uint8_t *value, uint16_t length, void *user_data)
> {
> struct async_dbus_op *op = user_data;
> struct notify_client *client = op->data;
> diff --git a/src/shared/gatt-client.c b/src/shared/gatt-client.c
> index 06ac763..76e4a9a 100644
> --- a/src/shared/gatt-client.c
> +++ b/src/shared/gatt-client.c
> @@ -1109,7 +1109,7 @@ struct service_changed_op {
> static void process_service_changed(struct bt_gatt_client *client,
> uint16_t start_handle,
> uint16_t end_handle);
> -static void service_changed_cb(uint16_t value_handle, const uint8_t *value,
> +static void service_changed_cb(notify_t type, uint16_t value_handle, const uint8_t *value,
> uint16_t length, void *user_data);
>
> static void complete_notify_request(void *data)
> @@ -1446,8 +1446,9 @@ fail:
> " after Service Changed");
> }
>
> -static void service_changed_cb(uint16_t value_handle, const uint8_t *value,
> - uint16_t length, void *user_data)
> +static void service_changed_cb(notify_t type, uint16_t value_handle,
> + const uint8_t *value, uint16_t length,
> + void *user_data)
> {
> struct bt_gatt_client *client = user_data;
> struct service_changed_op *op;
> @@ -1545,6 +1546,7 @@ static bool gatt_client_init(struct bt_gatt_client *client, uint16_t mtu)
> }
>
> struct pdu_data {
> + notify_t type;
> const void *pdu;
> uint16_t length;
> };
> @@ -1614,7 +1616,8 @@ static void notify_handler(void *data, void *user_data)
> * CCC, there is really no reason not to notify the handlers.
> */
> if (notify_data->notify)
> - notify_data->notify(value_handle, value, pdu_data->length - 2,
> + notify_data->notify(pdu_data->type, value_handle, value,
> + pdu_data->length - 2,
> notify_data->user_data);
> }
>
> @@ -1629,6 +1632,8 @@ static void notify_cb(uint8_t opcode, const void *pdu, uint16_t length,
> memset(&pdu_data, 0, sizeof(pdu_data));
> pdu_data.pdu = pdu;
> pdu_data.length = length;
> + pdu_data.type = opcode == BT_ATT_OP_HANDLE_VAL_IND ?
> + BT_GATT_INDICATION : BT_GATT_NOTIFICATION;
>
> queue_foreach(client->notify_list, notify_handler, &pdu_data);
>
> diff --git a/src/shared/gatt-client.h b/src/shared/gatt-client.h
> index befa43f..3018a99 100644
> --- a/src/shared/gatt-client.h
> +++ b/src/shared/gatt-client.h
> @@ -27,6 +27,11 @@
>
> #define BT_GATT_UUID_SIZE 16
>
> +typedef enum {
> + BT_GATT_NOTIFICATION,
> + BT_GATT_INDICATION,
> +} notify_t;
> +
> struct bt_gatt_client;
>
> struct bt_gatt_client *bt_gatt_client_new(struct gatt_db *db,
> @@ -46,9 +51,11 @@ typedef void (*bt_gatt_client_read_callback_t)(bool success, uint8_t att_ecode,
> typedef void (*bt_gatt_client_write_long_callback_t)(bool success,
> bool reliable_error, uint8_t att_ecode,
> void *user_data);
> -typedef void (*bt_gatt_client_notify_callback_t)(uint16_t value_handle,
> - const uint8_t *value, uint16_t length,
> - void *user_data);
> +typedef void (*bt_gatt_client_notify_callback_t)(notify_t type,
> + uint16_t value_handle,
> + const uint8_t *value,
> + uint16_t length,
> + void *user_data);
> typedef void (*bt_gatt_client_register_callback_t)(uint16_t att_ecode,
> void *user_data);
> typedef void (*bt_gatt_client_service_changed_callback_t)(uint16_t start_handle,
> diff --git a/tools/btgatt-client.c b/tools/btgatt-client.c
> index 0f6a1bd..76fd6d0 100644
> --- a/tools/btgatt-client.c
> +++ b/tools/btgatt-client.c
> @@ -1099,7 +1099,7 @@ static void register_notify_usage(void)
> printf("Usage: register-notify <chrc value handle>\n");
> }
>
> -static void notify_cb(uint16_t value_handle, const uint8_t *value,
> +static void notify_cb(notify_t type, uint16_t value_handle, const uint8_t *value,
> uint16_t length, void *user_data)
> {
> int i;
> diff --git a/unit/test-gatt.c b/unit/test-gatt.c
> index 326a32c..b6d4ab6 100644
> --- a/unit/test-gatt.c
> +++ b/unit/test-gatt.c
> @@ -2202,8 +2202,9 @@ static const struct test_step test_long_read_15 = {
> .expected_att_ecode = 0x0c
> };
>
> -static void notification_cb(uint16_t value_handle, const uint8_t *value,
> - uint16_t length, void *user_data)
> +static void notification_cb(notify_t type, uint16_t value_handle,
> + const uint8_t *value, uint16_t length,
> + void *user_data)
> {
> struct context *context = user_data;
> const struct test_step *step = context->data->step;
> @@ -2270,8 +2271,9 @@ static void test_server_indication_confirm(struct context *context)
> g_assert(indication_received == 1);
> }
>
> -static void indication_cb(uint16_t value_handle, const uint8_t *value,
> - uint16_t length, void *user_data)
> +static void indication_cb(notify_t type, uint16_t value_handle,
> + const uint8_t *value, uint16_t length,
> + void *user_data)
Can't you figure out from the callback type? I the code already have
dedicated callbacks for both so you can hardcode their types there.
--
Luiz Augusto von Dentz
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 4/4] shared/gatt-server: Cancel remaining write exec on error in the middle
2015-11-16 21:08 ` [PATCH 4/4] shared/gatt-server: Cancel remaining write exec on error in the middle Łukasz Rymanowski
2015-11-17 9:57 ` Grzegorz Kolodziejczyk
@ 2015-11-17 15:22 ` Marcin Kraglak
2015-11-17 21:04 ` Łukasz Rymanowski
1 sibling, 1 reply; 19+ messages in thread
From: Marcin Kraglak @ 2015-11-17 15:22 UTC (permalink / raw)
To: Łukasz Rymanowski; +Cc: linux-bluetooth@vger.kernel.org development
Hi Lukasz,
On 16 November 2015 at 22:08, Łukasz Rymanowski
<lukasz.rymanowski@codecoup.pl> wrote:
> If one execute write fail, server sends error response to the client.
> All characteristics before the failing one has been written and we cannot
> do anything about that. All the following characteristics will be
> canceled with this patch.
>
> According to the spec BT Core 4.2, Vol.3, Part F, chapter 3.4.6.3.
> in case of error, state of attributes is not defined, which makes
> this behavior ok.
> ---
> src/shared/gatt-server.c | 22 +++++++++++++++++++++-
> 1 file changed, 21 insertions(+), 1 deletion(-)
>
> diff --git a/src/shared/gatt-server.c b/src/shared/gatt-server.c
> index 931d619..3cd5190 100644
> --- a/src/shared/gatt-server.c
> +++ b/src/shared/gatt-server.c
> @@ -1158,6 +1158,22 @@ error:
>
> }
>
> +static void exec_write_cancel(void *data, void *user_data)
> +{
> + uint16_t handle = PTR_TO_UINT(data);
> + struct bt_gatt_server *server = user_data;
> + struct gatt_db_attribute *attr;
> + uint8_t cancel = 0;
> +
> + attr = gatt_db_get_attribute(server->db, PTR_TO_UINT(handle));
> + if (!attr) {
> + return;
> + }
> +
> + gatt_db_attribute_write(attr, 0, &cancel , 1, BT_ATT_OP_EXEC_WRITE_REQ,
> + server->att, NULL, NULL);
gatt_db_attribute_write() returns if callback is not provided. I
assume exec write req is mainly to inform app that exec write has
finished or has been canceled, so maybe you should just ignore exec
write responses as application already checked if value is valid in
prepare writes?
> +}
> +
> static void exec_next_write(struct bt_gatt_server *server);
>
> static void exec_write_complete_cb(struct gatt_db_attribute *attr, int err,
> @@ -1167,6 +1183,7 @@ static void exec_write_complete_cb(struct gatt_db_attribute *attr, int err,
> uint16_t handle = gatt_db_attribute_get_handle(attr);
>
> if (err) {
> + queue_foreach(server->exec_queue, exec_write_cancel, server);
> queue_remove_all(server->exec_queue, NULL, NULL, NULL);
> bt_att_send_error_rsp(server->att, BT_ATT_OP_EXEC_WRITE_REQ,
> handle, err);
> @@ -1178,6 +1195,7 @@ static void exec_write_complete_cb(struct gatt_db_attribute *attr, int err,
> static void exec_next_write(struct bt_gatt_server *server)
> {
> uint16_t *handle;
> + uint8_t execute = 1;
> struct gatt_db_attribute *attr;
> bool status;
> int err;
> @@ -1194,7 +1212,7 @@ static void exec_next_write(struct bt_gatt_server *server)
> goto error;
> }
>
> - status = gatt_db_attribute_write(attr, 0, NULL , 0,
> + status = gatt_db_attribute_write(attr, 0, &execute , 1,
> BT_ATT_OP_EXEC_WRITE_REQ,
> server->att,
> exec_write_complete_cb, server);
> @@ -1204,6 +1222,8 @@ static void exec_next_write(struct bt_gatt_server *server)
> err = BT_ATT_ERROR_UNLIKELY;
>
> error:
> +
> + queue_foreach(server->exec_queue, exec_write_cancel, server);
> queue_remove_all(server->exec_queue, NULL, NULL, NULL);
> bt_att_send_error_rsp(server->att, BT_ATT_OP_EXEC_WRITE_REQ,
> PTR_TO_UINT(handle), err);
> --
> 2.5.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
BR
Marcin Kraglak
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 4/4] shared/gatt-server: Cancel remaining write exec on error in the middle
2015-11-17 15:22 ` Marcin Kraglak
@ 2015-11-17 21:04 ` Łukasz Rymanowski
0 siblings, 0 replies; 19+ messages in thread
From: Łukasz Rymanowski @ 2015-11-17 21:04 UTC (permalink / raw)
To: Marcin Kraglak; +Cc: linux-bluetooth@vger.kernel.org development
Hi Marcin
On Tue, Nov 17, 2015 at 4:22 PM, Marcin Kraglak
<marcin.kraglak@tieto.com> wrote:
> Hi Lukasz,
>
> On 16 November 2015 at 22:08, Łukasz Rymanowski
> <lukasz.rymanowski@codecoup.pl> wrote:
>> If one execute write fail, server sends error response to the client.
>> All characteristics before the failing one has been written and we cannot
>> do anything about that. All the following characteristics will be
>> canceled with this patch.
>>
>> According to the spec BT Core 4.2, Vol.3, Part F, chapter 3.4.6.3.
>> in case of error, state of attributes is not defined, which makes
>> this behavior ok.
>> ---
>> src/shared/gatt-server.c | 22 +++++++++++++++++++++-
>> 1 file changed, 21 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/shared/gatt-server.c b/src/shared/gatt-server.c
>> index 931d619..3cd5190 100644
>> --- a/src/shared/gatt-server.c
>> +++ b/src/shared/gatt-server.c
>> @@ -1158,6 +1158,22 @@ error:
>>
>> }
>>
>> +static void exec_write_cancel(void *data, void *user_data)
>> +{
>> + uint16_t handle = PTR_TO_UINT(data);
>> + struct bt_gatt_server *server = user_data;
>> + struct gatt_db_attribute *attr;
>> + uint8_t cancel = 0;
>> +
>> + attr = gatt_db_get_attribute(server->db, PTR_TO_UINT(handle));
>> + if (!attr) {
>> + return;
>> + }
>> +
>> + gatt_db_attribute_write(attr, 0, &cancel , 1, BT_ATT_OP_EXEC_WRITE_REQ,
>> + server->att, NULL, NULL);
> gatt_db_attribute_write() returns if callback is not provided.
Good point, I missed that. Will fix.
I
> assume exec write req is mainly to inform app that exec write has
> finished or has been canceled, so maybe you should just ignore exec
> write responses as application already checked if value is valid in
> prepare writes?
Note sure if I follow here. Note that in this exec_queue we have now
handles which got some prepare writes. Now app is waiting for exec
write.
We are going to deliver it and ignore what is the response because it
does not matter now, since some previous exec write already fail.
>> +}
>> +
>> static void exec_next_write(struct bt_gatt_server *server);
>>
>> static void exec_write_complete_cb(struct gatt_db_attribute *attr, int err,
>> @@ -1167,6 +1183,7 @@ static void exec_write_complete_cb(struct gatt_db_attribute *attr, int err,
>> uint16_t handle = gatt_db_attribute_get_handle(attr);
>>
>> if (err) {
>> + queue_foreach(server->exec_queue, exec_write_cancel, server);
>> queue_remove_all(server->exec_queue, NULL, NULL, NULL);
>> bt_att_send_error_rsp(server->att, BT_ATT_OP_EXEC_WRITE_REQ,
>> handle, err);
>> @@ -1178,6 +1195,7 @@ static void exec_write_complete_cb(struct gatt_db_attribute *attr, int err,
>> static void exec_next_write(struct bt_gatt_server *server)
>> {
>> uint16_t *handle;
>> + uint8_t execute = 1;
>> struct gatt_db_attribute *attr;
>> bool status;
>> int err;
>> @@ -1194,7 +1212,7 @@ static void exec_next_write(struct bt_gatt_server *server)
>> goto error;
>> }
>>
>> - status = gatt_db_attribute_write(attr, 0, NULL , 0,
>> + status = gatt_db_attribute_write(attr, 0, &execute , 1,
>> BT_ATT_OP_EXEC_WRITE_REQ,
>> server->att,
>> exec_write_complete_cb, server);
>> @@ -1204,6 +1222,8 @@ static void exec_next_write(struct bt_gatt_server *server)
>> err = BT_ATT_ERROR_UNLIKELY;
>>
>> error:
>> +
>> + queue_foreach(server->exec_queue, exec_write_cancel, server);
>> queue_remove_all(server->exec_queue, NULL, NULL, NULL);
>> bt_att_send_error_rsp(server->att, BT_ATT_OP_EXEC_WRITE_REQ,
>> PTR_TO_UINT(handle), err);
>> --
>> 2.5.0
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
>
>
> --
> BR
> Marcin Kraglak
--
BR / Pozdrawiam
Łukasz
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 4/4] shared/gatt-server: Cancel remaining write exec on error in the middle
2015-11-17 9:57 ` Grzegorz Kolodziejczyk
@ 2015-11-17 21:05 ` Łukasz Rymanowski
0 siblings, 0 replies; 19+ messages in thread
From: Łukasz Rymanowski @ 2015-11-17 21:05 UTC (permalink / raw)
To: Grzegorz Kolodziejczyk; +Cc: linux-bluetooth
Hi Grzegorz,
On Tue, Nov 17, 2015 at 10:57 AM, Grzegorz Kolodziejczyk
<grzegorz.kolodziejczyk@tieto.com> wrote:
> Hi Lukasz,
>
> On 16 November 2015 at 22:08, Łukasz Rymanowski
> <lukasz.rymanowski@codecoup.pl> wrote:
>> If one execute write fail, server sends error response to the client.
>> All characteristics before the failing one has been written and we cannot
>> do anything about that. All the following characteristics will be
>> canceled with this patch.
>>
>> According to the spec BT Core 4.2, Vol.3, Part F, chapter 3.4.6.3.
>> in case of error, state of attributes is not defined, which makes
>> this behavior ok.
>> ---
>> src/shared/gatt-server.c | 22 +++++++++++++++++++++-
>> 1 file changed, 21 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/shared/gatt-server.c b/src/shared/gatt-server.c
>> index 931d619..3cd5190 100644
>> --- a/src/shared/gatt-server.c
>> +++ b/src/shared/gatt-server.c
>> @@ -1158,6 +1158,22 @@ error:
>>
>> }
>>
>> +static void exec_write_cancel(void *data, void *user_data)
>> +{
>> + uint16_t handle = PTR_TO_UINT(data);
>> + struct bt_gatt_server *server = user_data;
>> + struct gatt_db_attribute *attr;
>> + uint8_t cancel = 0;
>> +
>> + attr = gatt_db_get_attribute(server->db, PTR_TO_UINT(handle));
>> + if (!attr) {
>> + return;
>> + }
> No brackets.
I knew that I will forgot about that after my last project :) thanks
>> +
>> + gatt_db_attribute_write(attr, 0, &cancel , 1, BT_ATT_OP_EXEC_WRITE_REQ,
> Shouldn't be len - sizeof value ? instead of "1" ?
Ok, will fix
>> + server->att, NULL, NULL);
>> +}
>> +
>> static void exec_next_write(struct bt_gatt_server *server);
>>
>> static void exec_write_complete_cb(struct gatt_db_attribute *attr, int err,
>> @@ -1167,6 +1183,7 @@ static void exec_write_complete_cb(struct gatt_db_attribute *attr, int err,
>> uint16_t handle = gatt_db_attribute_get_handle(attr);
>>
>> if (err) {
>> + queue_foreach(server->exec_queue, exec_write_cancel, server);
>> queue_remove_all(server->exec_queue, NULL, NULL, NULL);
>> bt_att_send_error_rsp(server->att, BT_ATT_OP_EXEC_WRITE_REQ,
>> handle, err);
>> @@ -1178,6 +1195,7 @@ static void exec_write_complete_cb(struct gatt_db_attribute *attr, int err,
>> static void exec_next_write(struct bt_gatt_server *server)
>> {
>> uint16_t *handle;
>> + uint8_t execute = 1;
>> struct gatt_db_attribute *attr;
>> bool status;
>> int err;
>> @@ -1194,7 +1212,7 @@ static void exec_next_write(struct bt_gatt_server *server)
>> goto error;
>> }
>>
>> - status = gatt_db_attribute_write(attr, 0, NULL , 0,
>> + status = gatt_db_attribute_write(attr, 0, &execute , 1,
> ditto (sizeof)
OK
>> BT_ATT_OP_EXEC_WRITE_REQ,
>> server->att,
>> exec_write_complete_cb, server);
>> @@ -1204,6 +1222,8 @@ static void exec_next_write(struct bt_gatt_server *server)
>> err = BT_ATT_ERROR_UNLIKELY;
>>
>> error:
>> +
>> + queue_foreach(server->exec_queue, exec_write_cancel, server);
>> queue_remove_all(server->exec_queue, NULL, NULL, NULL);
>> bt_att_send_error_rsp(server->att, BT_ATT_OP_EXEC_WRITE_REQ,
>> PTR_TO_UINT(handle), err);
>> --
>> 2.5.0
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
> BR,
> Grzegorz
--
BR / Pozdrawiam
Łukasz
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 3/4] shared/gatt-server: Improve long write session handling
2015-11-17 12:12 ` Marcin Kraglak
2015-11-17 12:49 ` Luiz Augusto von Dentz
@ 2015-11-17 21:22 ` Łukasz Rymanowski
1 sibling, 0 replies; 19+ messages in thread
From: Łukasz Rymanowski @ 2015-11-17 21:22 UTC (permalink / raw)
To: Marcin Kraglak; +Cc: linux-bluetooth@vger.kernel.org development
Hi Marcin,
On Tue, Nov 17, 2015 at 1:12 PM, Marcin Kraglak
<marcin.kraglak@tieto.com> wrote:
> Hi Lukasz,
>
> On 16 November 2015 at 22:08, Łukasz Rymanowski
> <lukasz.rymanowski@codecoup.pl> wrote:
>>
>> With current implementation application layer has no idea when
>> long write session ends as it gets write callback with execute
>> write for each prepare write data.
>> With this patch, after data are completly received by gatt-server it
>> starts to call write callback in proper order with opcode
>> BT_ATT_OP_PREP_WRITE_REQ. After that, each characteristic gets
>> BT_ATT_OP_EXEC_WRITE_REQ so it knows that complete characteristic value
>> has been received and data can be marked as valid.
>> ---
>> src/shared/gatt-server.c | 78 ++++++++++++++++++++++++++++++++++++++++++++----
>> 1 file changed, 73 insertions(+), 5 deletions(-)
>>
>> diff --git a/src/shared/gatt-server.c b/src/shared/gatt-server.c
>> index 329f53f..931d619 100644
>> --- a/src/shared/gatt-server.c
>> +++ b/src/shared/gatt-server.c
>> @@ -104,6 +104,8 @@ struct bt_gatt_server {
>> struct queue *prep_queue;
>> unsigned int max_prep_queue_len;
>>
>> + struct queue *exec_queue;
>> +
>> struct async_read_op *pending_read_op;
>> struct async_write_op *pending_write_op;
>>
>> @@ -137,6 +139,7 @@ static void bt_gatt_server_free(struct bt_gatt_server *server)
>> server->pending_write_op->server = NULL;
>>
>> queue_destroy(server->prep_queue, prep_write_data_destroy);
>> + queue_destroy(server->exec_queue, NULL);
>>
>> gatt_db_unref(server->db);
>> bt_att_unref(server->att);
>> @@ -1155,10 +1158,61 @@ error:
>>
>> }
>>
>> +static void exec_next_write(struct bt_gatt_server *server);
>> +
>> +static void exec_write_complete_cb(struct gatt_db_attribute *attr, int err,
>> + void *user_data)
>> +{
>> + struct bt_gatt_server *server = user_data;
>> + uint16_t handle = gatt_db_attribute_get_handle(attr);
>> +
>> + if (err) {
>> + queue_remove_all(server->exec_queue, NULL, NULL, NULL);
>> + bt_att_send_error_rsp(server->att, BT_ATT_OP_EXEC_WRITE_REQ,
>> + handle, err);
>> + }
>> +
>> + exec_next_write(server);
>> +}
>> +
>> +static void exec_next_write(struct bt_gatt_server *server)
>> +{
>> + uint16_t *handle;
>> + struct gatt_db_attribute *attr;
>> + bool status;
>> + int err;
>> +
>> + handle = queue_pop_head(server->exec_queue);
>> + if (!handle) {
>> + bt_att_send(server->att, BT_ATT_OP_EXEC_WRITE_RSP, NULL, 0,
>> + NULL, NULL, NULL);
>> + }
>> +
>> + attr = gatt_db_get_attribute(server->db, PTR_TO_UINT(handle));
>> + if (!attr) {
>> + err = BT_ATT_ERROR_UNLIKELY;
>> + goto error;
>> + }
>> +
>> + status = gatt_db_attribute_write(attr, 0, NULL , 0,
>> + BT_ATT_OP_EXEC_WRITE_REQ,
>> + server->att,
>> + exec_write_complete_cb, server);
>> + if (status)
>> + return;
>> +
>> + err = BT_ATT_ERROR_UNLIKELY;
>> +
>> +error:
>> + queue_remove_all(server->exec_queue, NULL, NULL, NULL);
>> + bt_att_send_error_rsp(server->att, BT_ATT_OP_EXEC_WRITE_REQ,
>> + PTR_TO_UINT(handle), err);
>> +}
>> +
>> static void exec_next_prep_write(struct bt_gatt_server *server,
>> uint16_t ehandle, int err);
>>
>> -static void exec_write_complete_cb(struct gatt_db_attribute *attr, int err,
>> +static void prep_write_complete_cb(struct gatt_db_attribute *attr, int err,
>> void *user_data)
>> {
>> struct bt_gatt_server *server = user_data;
>> @@ -1167,6 +1221,11 @@ static void exec_write_complete_cb(struct gatt_db_attribute *attr, int err,
>> exec_next_prep_write(server, handle, err);
>> }
>>
>> +static bool match_attribute_handle(const void *data, const void *match_data)
>> +{
>> + return PTR_TO_UINT(data) == PTR_TO_UINT(match_data);
>> +}
>> +
>> static void exec_next_prep_write(struct bt_gatt_server *server,
>> uint16_t ehandle, int err)
>> {
>> @@ -1177,10 +1236,17 @@ static void exec_next_prep_write(struct bt_gatt_server *server,
>> if (err)
> Shouldn't you remove all elements of server->exec_queue here?Looking
> at the next patch, all attributes in exec_queue shall be notified that
> exec write is canceled too.
You are right, will fix, thanks
> And just open question: with introducing two types of write requests
> in reliable write procedure: BT_ATT_OP_EXEC_WRITE_REQ and
> BT_ATT_OP_PREP_WRITE_REQ, we could avoid using two queues and pass
> prepare writes immediatelly to specific service implementation instead
> of storing all values in prepare writes in server.
>
>> goto error;
>>
>>
>> + /*
>> + * Remember to which handles we need to send execute after reliable
>> + * or long write session is done
>> + */
>> + if (ehandle && !queue_find(server->exec_queue, match_attribute_handle,
>> + UINT_TO_PTR(ehandle)))
>> + queue_push_tail(server->exec_queue, UINT_TO_PTR(ehandle));
>> +
>> next = queue_pop_head(server->prep_queue);
>> if (!next) {
>> - bt_att_send(server->att, BT_ATT_OP_EXEC_WRITE_RSP, NULL, 0,
>> - NULL, NULL, NULL);
>> + exec_next_write(server);
>> return;
>> }
>>
>> @@ -1192,9 +1258,9 @@ static void exec_next_prep_write(struct bt_gatt_server *server,
>>
>> status = gatt_db_attribute_write(attr, next->offset,
>> next->value, next->length,
>> - BT_ATT_OP_EXEC_WRITE_REQ,
>> + BT_ATT_OP_PREP_WRITE_REQ,
>> server->att,
>> - exec_write_complete_cb, server);
>> + prep_write_complete_cb, server);
>>
>> prep_write_data_destroy(next);
>>
>> @@ -1396,6 +1462,8 @@ struct bt_gatt_server *bt_gatt_server_new(struct gatt_db *db,
>> server->max_prep_queue_len = DEFAULT_MAX_PREP_QUEUE_LEN;
>> server->prep_queue = queue_new();
>>
>> + server->exec_queue = queue_new();
>> +
>> if (!gatt_server_register_att_handlers(server)) {
>> bt_gatt_server_free(server);
>> return NULL;
>> --
>> 2.5.0
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
>
>
>
> --
> BR
> Marcin Kraglak
--
BR / Pozdrawiam
Łukasz
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 3/4] shared/gatt-server: Improve long write session handling
2015-11-17 12:49 ` Luiz Augusto von Dentz
@ 2015-11-17 21:32 ` Łukasz Rymanowski
2015-11-18 23:01 ` Luiz Augusto von Dentz
0 siblings, 1 reply; 19+ messages in thread
From: Łukasz Rymanowski @ 2015-11-17 21:32 UTC (permalink / raw)
To: Luiz Augusto von Dentz
Cc: Marcin Kraglak, linux-bluetooth@vger.kernel.org development
Hi Luiz, Marcin,
On Tue, Nov 17, 2015 at 1:49 PM, Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
> Hi Marcin,
>
> On Tue, Nov 17, 2015 at 2:12 PM, Marcin Kraglak
> <marcin.kraglak@tieto.com> wrote:
>> Hi Lukasz,
>>
>> On 16 November 2015 at 22:08, Łukasz Rymanowski
>> <lukasz.rymanowski@codecoup.pl> wrote:
>>>
>>> With current implementation application layer has no idea when
>>> long write session ends as it gets write callback with execute
>>> write for each prepare write data.
>>> With this patch, after data are completly received by gatt-server it
>>> starts to call write callback in proper order with opcode
>>> BT_ATT_OP_PREP_WRITE_REQ. After that, each characteristic gets
>>> BT_ATT_OP_EXEC_WRITE_REQ so it knows that complete characteristic value
>>> has been received and data can be marked as valid.
>>> ---
>>> src/shared/gatt-server.c | 78 ++++++++++++++++++++++++++++++++++++++++++++----
>>> 1 file changed, 73 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/src/shared/gatt-server.c b/src/shared/gatt-server.c
>>> index 329f53f..931d619 100644
>>> --- a/src/shared/gatt-server.c
>>> +++ b/src/shared/gatt-server.c
>>> @@ -104,6 +104,8 @@ struct bt_gatt_server {
>>> struct queue *prep_queue;
>>> unsigned int max_prep_queue_len;
>>>
>>> + struct queue *exec_queue;
>>> +
>>> struct async_read_op *pending_read_op;
>>> struct async_write_op *pending_write_op;
>>>
>>> @@ -137,6 +139,7 @@ static void bt_gatt_server_free(struct bt_gatt_server *server)
>>> server->pending_write_op->server = NULL;
>>>
>>> queue_destroy(server->prep_queue, prep_write_data_destroy);
>>> + queue_destroy(server->exec_queue, NULL);
>>>
>>> gatt_db_unref(server->db);
>>> bt_att_unref(server->att);
>>> @@ -1155,10 +1158,61 @@ error:
>>>
>>> }
>>>
>>> +static void exec_next_write(struct bt_gatt_server *server);
>>> +
>>> +static void exec_write_complete_cb(struct gatt_db_attribute *attr, int err,
>>> + void *user_data)
>>> +{
>>> + struct bt_gatt_server *server = user_data;
>>> + uint16_t handle = gatt_db_attribute_get_handle(attr);
>>> +
>>> + if (err) {
>>> + queue_remove_all(server->exec_queue, NULL, NULL, NULL);
>>> + bt_att_send_error_rsp(server->att, BT_ATT_OP_EXEC_WRITE_REQ,
>>> + handle, err);
>>> + }
>>> +
>>> + exec_next_write(server);
>>> +}
>>> +
>>> +static void exec_next_write(struct bt_gatt_server *server)
>>> +{
>>> + uint16_t *handle;
>>> + struct gatt_db_attribute *attr;
>>> + bool status;
>>> + int err;
>>> +
>>> + handle = queue_pop_head(server->exec_queue);
>>> + if (!handle) {
>>> + bt_att_send(server->att, BT_ATT_OP_EXEC_WRITE_RSP, NULL, 0,
>>> + NULL, NULL, NULL);
>>> + }
>>> +
>>> + attr = gatt_db_get_attribute(server->db, PTR_TO_UINT(handle));
>>> + if (!attr) {
>>> + err = BT_ATT_ERROR_UNLIKELY;
>>> + goto error;
>>> + }
>>> +
>>> + status = gatt_db_attribute_write(attr, 0, NULL , 0,
>>> + BT_ATT_OP_EXEC_WRITE_REQ,
>>> + server->att,
>>> + exec_write_complete_cb, server);
>>> + if (status)
>>> + return;
>>> +
>>> + err = BT_ATT_ERROR_UNLIKELY;
>>> +
>>> +error:
>>> + queue_remove_all(server->exec_queue, NULL, NULL, NULL);
>>> + bt_att_send_error_rsp(server->att, BT_ATT_OP_EXEC_WRITE_REQ,
>>> + PTR_TO_UINT(handle), err);
>>> +}
>>> +
>>> static void exec_next_prep_write(struct bt_gatt_server *server,
>>> uint16_t ehandle, int err);
>>>
>>> -static void exec_write_complete_cb(struct gatt_db_attribute *attr, int err,
>>> +static void prep_write_complete_cb(struct gatt_db_attribute *attr, int err,
>>> void *user_data)
>>> {
>>> struct bt_gatt_server *server = user_data;
>>> @@ -1167,6 +1221,11 @@ static void exec_write_complete_cb(struct gatt_db_attribute *attr, int err,
>>> exec_next_prep_write(server, handle, err);
>>> }
>>>
>>> +static bool match_attribute_handle(const void *data, const void *match_data)
>>> +{
>>> + return PTR_TO_UINT(data) == PTR_TO_UINT(match_data);
>>> +}
>>> +
>>> static void exec_next_prep_write(struct bt_gatt_server *server,
>>> uint16_t ehandle, int err)
>>> {
>>> @@ -1177,10 +1236,17 @@ static void exec_next_prep_write(struct bt_gatt_server *server,
>>> if (err)
>> Shouldn't you remove all elements of server->exec_queue here? Looking
>> at the next patch, all attributes in exec_queue shall be notified that
>> exec write is canceled too.
>> And just open question: with introducing two types of write requests
>> in reliable write procedure: BT_ATT_OP_EXEC_WRITE_REQ and
>> BT_ATT_OP_PREP_WRITE_REQ, we could avoid using two queues and pass
>> prepare writes immediatelly to specific service implementation instead
>> of storing all values in prepare writes in server.
>
> Even if we have some flush logic trigger by execute we cannot really
> dispatch writes immediately to the services because it can fail with
> some errors that are not allowed by the testing spec, yes there is a
> specific test checking the e.g. invalid offset is only receive as a
> response to execute. It looks like this is because the value could be
> changed in between prepare and execute, which IMO is something
> horribly broken with prepare + execute since it is intended for
> reliable writes but it can produce all sort of errors which is
> impossible to guess without reading the value written after it has
> been executed in case anything has change in between the commands.
>
I see Marcins point and agree. According to spec if handle is valid,
permisions are ok and there
is enought place in the queue, Prepare Write Response shall be sent
back. No matter if application
likes data or not. Error on value validation we will get during
execute writes. So in teory we could indeed
avoid prep_queue and send data directly to application when handle is
ok and permisions are fine.
Actually this is how we do it now in Android part.
But on the other side, that is not big deal, so if you Luiz saw any
issues with that, we can stay with server
handling queuing prep writes.
>>> next = queue_pop_head(server->prep_queue);
>>> if (!next) {
>>> - bt_att_send(server->att, BT_ATT_OP_EXEC_WRITE_RSP, NULL, 0,
>>> - NULL, NULL, NULL);
>>> + exec_next_write(server);
>>> return;
>>> }
>>>
>>> @@ -1192,9 +1258,9 @@ static void exec_next_prep_write(struct bt_gatt_server *server,
>>>
>>> status = gatt_db_attribute_write(attr, next->offset,
>>> next->value, next->length,
>>> - BT_ATT_OP_EXEC_WRITE_REQ,
>>> + BT_ATT_OP_PREP_WRITE_REQ,
>>> server->att,
>>> - exec_write_complete_cb, server);
>>> + prep_write_complete_cb, server);
>>>
>>> prep_write_data_destroy(next);
>>>
>>> @@ -1396,6 +1462,8 @@ struct bt_gatt_server *bt_gatt_server_new(struct gatt_db *db,
>>> server->max_prep_queue_len = DEFAULT_MAX_PREP_QUEUE_LEN;
>>> server->prep_queue = queue_new();
>>>
>>> + server->exec_queue = queue_new();
>>> +
>>> if (!gatt_server_register_att_handlers(server)) {
>>> bt_gatt_server_free(server);
>>> return NULL;
>>> --
>>> 2.5.0
>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>
>>
>>
>>
>> --
>> BR
>> Marcin Kraglak
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
>
>
> --
> Luiz Augusto von Dentz
--
BR / Pozdrawiam
Łukasz
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/4] shared/gatt-client: Add notify type to notification callback
2015-11-17 14:10 ` Luiz Augusto von Dentz
@ 2015-11-17 22:03 ` Łukasz Rymanowski
0 siblings, 0 replies; 19+ messages in thread
From: Łukasz Rymanowski @ 2015-11-17 22:03 UTC (permalink / raw)
To: Luiz Augusto von Dentz; +Cc: linux-bluetooth
Hi Luiz,
On Tue, Nov 17, 2015 at 3:10 PM, Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
> Hi Lukasz,
>
> On Mon, Nov 16, 2015 at 11:08 PM, Łukasz Rymanowski
> <lukasz.rymanowski@codecoup.pl> wrote:
>> Android expect information either its indication or notification.
>> This patch adds that.
>> ---
>> profiles/scanparam/scan.c | 5 +++--
>> src/gatt-client.c | 4 ++--
>> src/shared/gatt-client.c | 13 +++++++++----
>> src/shared/gatt-client.h | 13 ++++++++++---
>> tools/btgatt-client.c | 2 +-
>> unit/test-gatt.c | 10 ++++++----
>> 6 files changed, 31 insertions(+), 16 deletions(-)
>>
>> diff --git a/profiles/scanparam/scan.c b/profiles/scanparam/scan.c
>> index 4015b3f..fd01135 100644
>> --- a/profiles/scanparam/scan.c
>> +++ b/profiles/scanparam/scan.c
>> @@ -92,8 +92,9 @@ static void write_scan_params(struct scan *scan)
>> false, value, sizeof(value));
>> }
>>
>> -static void refresh_value_cb(uint16_t value_handle, const uint8_t *value,
>> - uint16_t length, void *user_data)
>> +static void refresh_value_cb(notify_t type, uint16_t value_handle,
>> + const uint8_t *value, uint16_t length,
>> + void *user_data)
>> {
>> struct scan *scan = user_data;
>>
>> diff --git a/src/gatt-client.c b/src/gatt-client.c
>> index 39f6646..e74803c 100644
>> --- a/src/gatt-client.c
>> +++ b/src/gatt-client.c
>> @@ -1057,8 +1057,8 @@ static bool match_notify_sender(const void *a, const void *b)
>> return strcmp(client->owner, sender) == 0;
>> }
>>
>> -static void notify_cb(uint16_t value_handle, const uint8_t *value,
>> - uint16_t length, void *user_data)
>> +static void notify_cb(notify_t type, uint16_t value_handle,
>> + const uint8_t *value, uint16_t length, void *user_data)
>> {
>> struct async_dbus_op *op = user_data;
>> struct notify_client *client = op->data;
>> diff --git a/src/shared/gatt-client.c b/src/shared/gatt-client.c
>> index 06ac763..76e4a9a 100644
>> --- a/src/shared/gatt-client.c
>> +++ b/src/shared/gatt-client.c
>> @@ -1109,7 +1109,7 @@ struct service_changed_op {
>> static void process_service_changed(struct bt_gatt_client *client,
>> uint16_t start_handle,
>> uint16_t end_handle);
>> -static void service_changed_cb(uint16_t value_handle, const uint8_t *value,
>> +static void service_changed_cb(notify_t type, uint16_t value_handle, const uint8_t *value,
>> uint16_t length, void *user_data);
>>
>> static void complete_notify_request(void *data)
>> @@ -1446,8 +1446,9 @@ fail:
>> " after Service Changed");
>> }
>>
>> -static void service_changed_cb(uint16_t value_handle, const uint8_t *value,
>> - uint16_t length, void *user_data)
>> +static void service_changed_cb(notify_t type, uint16_t value_handle,
>> + const uint8_t *value, uint16_t length,
>> + void *user_data)
>> {
>> struct bt_gatt_client *client = user_data;
>> struct service_changed_op *op;
>> @@ -1545,6 +1546,7 @@ static bool gatt_client_init(struct bt_gatt_client *client, uint16_t mtu)
>> }
>>
>> struct pdu_data {
>> + notify_t type;
>> const void *pdu;
>> uint16_t length;
>> };
>> @@ -1614,7 +1616,8 @@ static void notify_handler(void *data, void *user_data)
>> * CCC, there is really no reason not to notify the handlers.
>> */
>> if (notify_data->notify)
>> - notify_data->notify(value_handle, value, pdu_data->length - 2,
>> + notify_data->notify(pdu_data->type, value_handle, value,
>> + pdu_data->length - 2,
>> notify_data->user_data);
>> }
>>
>> @@ -1629,6 +1632,8 @@ static void notify_cb(uint8_t opcode, const void *pdu, uint16_t length,
>> memset(&pdu_data, 0, sizeof(pdu_data));
>> pdu_data.pdu = pdu;
>> pdu_data.length = length;
>> + pdu_data.type = opcode == BT_ATT_OP_HANDLE_VAL_IND ?
>> + BT_GATT_INDICATION : BT_GATT_NOTIFICATION;
>>
>> queue_foreach(client->notify_list, notify_handler, &pdu_data);
>>
>> diff --git a/src/shared/gatt-client.h b/src/shared/gatt-client.h
>> index befa43f..3018a99 100644
>> --- a/src/shared/gatt-client.h
>> +++ b/src/shared/gatt-client.h
>> @@ -27,6 +27,11 @@
>>
>> #define BT_GATT_UUID_SIZE 16
>>
>> +typedef enum {
>> + BT_GATT_NOTIFICATION,
>> + BT_GATT_INDICATION,
>> +} notify_t;
>> +
>> struct bt_gatt_client;
>>
>> struct bt_gatt_client *bt_gatt_client_new(struct gatt_db *db,
>> @@ -46,9 +51,11 @@ typedef void (*bt_gatt_client_read_callback_t)(bool success, uint8_t att_ecode,
>> typedef void (*bt_gatt_client_write_long_callback_t)(bool success,
>> bool reliable_error, uint8_t att_ecode,
>> void *user_data);
>> -typedef void (*bt_gatt_client_notify_callback_t)(uint16_t value_handle,
>> - const uint8_t *value, uint16_t length,
>> - void *user_data);
>> +typedef void (*bt_gatt_client_notify_callback_t)(notify_t type,
>> + uint16_t value_handle,
>> + const uint8_t *value,
>> + uint16_t length,
>> + void *user_data);
>> typedef void (*bt_gatt_client_register_callback_t)(uint16_t att_ecode,
>> void *user_data);
>> typedef void (*bt_gatt_client_service_changed_callback_t)(uint16_t start_handle,
>> diff --git a/tools/btgatt-client.c b/tools/btgatt-client.c
>> index 0f6a1bd..76fd6d0 100644
>> --- a/tools/btgatt-client.c
>> +++ b/tools/btgatt-client.c
>> @@ -1099,7 +1099,7 @@ static void register_notify_usage(void)
>> printf("Usage: register-notify <chrc value handle>\n");
>> }
>>
>> -static void notify_cb(uint16_t value_handle, const uint8_t *value,
>> +static void notify_cb(notify_t type, uint16_t value_handle, const uint8_t *value,
>> uint16_t length, void *user_data)
>> {
>> int i;
>> diff --git a/unit/test-gatt.c b/unit/test-gatt.c
>> index 326a32c..b6d4ab6 100644
>> --- a/unit/test-gatt.c
>> +++ b/unit/test-gatt.c
>> @@ -2202,8 +2202,9 @@ static const struct test_step test_long_read_15 = {
>> .expected_att_ecode = 0x0c
>> };
>>
>> -static void notification_cb(uint16_t value_handle, const uint8_t *value,
>> - uint16_t length, void *user_data)
>> +static void notification_cb(notify_t type, uint16_t value_handle,
>> + const uint8_t *value, uint16_t length,
>> + void *user_data)
>> {
>> struct context *context = user_data;
>> const struct test_step *step = context->data->step;
>> @@ -2270,8 +2271,9 @@ static void test_server_indication_confirm(struct context *context)
>> g_assert(indication_received == 1);
>> }
>>
>> -static void indication_cb(uint16_t value_handle, const uint8_t *value,
>> - uint16_t length, void *user_data)
>> +static void indication_cb(notify_t type, uint16_t value_handle,
>> + const uint8_t *value, uint16_t length,
>> + void *user_data)
>
> Can't you figure out from the callback type? I the code already have
> dedicated callbacks for both so you can hardcode their types there.
This is only test-gatt where I fixed compile issues after notify_t has
been added to notify_cb.
What I could do here is to add g_assert(type == XX) to confirm that
indication_cb handles indication and notification is notification.
Actually in this way I will have test if type is
set correctly. That is what you meant here?
>
>
> --
> Luiz Augusto von Dentz
--
BR / Pozdrawiam
Łukasz
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 3/4] shared/gatt-server: Improve long write session handling
2015-11-17 21:32 ` Łukasz Rymanowski
@ 2015-11-18 23:01 ` Luiz Augusto von Dentz
2015-11-20 8:19 ` Łukasz Rymanowski
[not found] ` <CAExxLLCJ5QCQg+48WBPe-ss3TDrpDZj9rO5x3zzPAi0B=WQtPA@mail.gmail.com>
0 siblings, 2 replies; 19+ messages in thread
From: Luiz Augusto von Dentz @ 2015-11-18 23:01 UTC (permalink / raw)
To: Łukasz Rymanowski
Cc: Marcin Kraglak, linux-bluetooth@vger.kernel.org development
Hi Lukasz,
On Tue, Nov 17, 2015 at 11:32 PM, Łukasz Rymanowski
<lukasz.rymanowski@codecoup.pl> wrote:
> Hi Luiz, Marcin,
>
> On Tue, Nov 17, 2015 at 1:49 PM, Luiz Augusto von Dentz
> <luiz.dentz@gmail.com> wrote:
>> Hi Marcin,
>>
>> On Tue, Nov 17, 2015 at 2:12 PM, Marcin Kraglak
>> <marcin.kraglak@tieto.com> wrote:
>>> Hi Lukasz,
>>>
>>> On 16 November 2015 at 22:08, Łukasz Rymanowski
>>> <lukasz.rymanowski@codecoup.pl> wrote:
>>>>
>>>> With current implementation application layer has no idea when
>>>> long write session ends as it gets write callback with execute
>>>> write for each prepare write data.
>>>> With this patch, after data are completly received by gatt-server it
>>>> starts to call write callback in proper order with opcode
>>>> BT_ATT_OP_PREP_WRITE_REQ. After that, each characteristic gets
>>>> BT_ATT_OP_EXEC_WRITE_REQ so it knows that complete characteristic value
>>>> has been received and data can be marked as valid.
>>>> ---
>>>> src/shared/gatt-server.c | 78 ++++++++++++++++++++++++++++++++++++++++++++----
>>>> 1 file changed, 73 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/src/shared/gatt-server.c b/src/shared/gatt-server.c
>>>> index 329f53f..931d619 100644
>>>> --- a/src/shared/gatt-server.c
>>>> +++ b/src/shared/gatt-server.c
>>>> @@ -104,6 +104,8 @@ struct bt_gatt_server {
>>>> struct queue *prep_queue;
>>>> unsigned int max_prep_queue_len;
>>>>
>>>> + struct queue *exec_queue;
>>>> +
>>>> struct async_read_op *pending_read_op;
>>>> struct async_write_op *pending_write_op;
>>>>
>>>> @@ -137,6 +139,7 @@ static void bt_gatt_server_free(struct bt_gatt_server *server)
>>>> server->pending_write_op->server = NULL;
>>>>
>>>> queue_destroy(server->prep_queue, prep_write_data_destroy);
>>>> + queue_destroy(server->exec_queue, NULL);
>>>>
>>>> gatt_db_unref(server->db);
>>>> bt_att_unref(server->att);
>>>> @@ -1155,10 +1158,61 @@ error:
>>>>
>>>> }
>>>>
>>>> +static void exec_next_write(struct bt_gatt_server *server);
>>>> +
>>>> +static void exec_write_complete_cb(struct gatt_db_attribute *attr, int err,
>>>> + void *user_data)
>>>> +{
>>>> + struct bt_gatt_server *server = user_data;
>>>> + uint16_t handle = gatt_db_attribute_get_handle(attr);
>>>> +
>>>> + if (err) {
>>>> + queue_remove_all(server->exec_queue, NULL, NULL, NULL);
>>>> + bt_att_send_error_rsp(server->att, BT_ATT_OP_EXEC_WRITE_REQ,
>>>> + handle, err);
>>>> + }
>>>> +
>>>> + exec_next_write(server);
>>>> +}
>>>> +
>>>> +static void exec_next_write(struct bt_gatt_server *server)
>>>> +{
>>>> + uint16_t *handle;
>>>> + struct gatt_db_attribute *attr;
>>>> + bool status;
>>>> + int err;
>>>> +
>>>> + handle = queue_pop_head(server->exec_queue);
>>>> + if (!handle) {
>>>> + bt_att_send(server->att, BT_ATT_OP_EXEC_WRITE_RSP, NULL, 0,
>>>> + NULL, NULL, NULL);
>>>> + }
>>>> +
>>>> + attr = gatt_db_get_attribute(server->db, PTR_TO_UINT(handle));
>>>> + if (!attr) {
>>>> + err = BT_ATT_ERROR_UNLIKELY;
>>>> + goto error;
>>>> + }
>>>> +
>>>> + status = gatt_db_attribute_write(attr, 0, NULL , 0,
>>>> + BT_ATT_OP_EXEC_WRITE_REQ,
>>>> + server->att,
>>>> + exec_write_complete_cb, server);
>>>> + if (status)
>>>> + return;
>>>> +
>>>> + err = BT_ATT_ERROR_UNLIKELY;
>>>> +
>>>> +error:
>>>> + queue_remove_all(server->exec_queue, NULL, NULL, NULL);
>>>> + bt_att_send_error_rsp(server->att, BT_ATT_OP_EXEC_WRITE_REQ,
>>>> + PTR_TO_UINT(handle), err);
>>>> +}
>>>> +
>>>> static void exec_next_prep_write(struct bt_gatt_server *server,
>>>> uint16_t ehandle, int err);
>>>>
>>>> -static void exec_write_complete_cb(struct gatt_db_attribute *attr, int err,
>>>> +static void prep_write_complete_cb(struct gatt_db_attribute *attr, int err,
>>>> void *user_data)
>>>> {
>>>> struct bt_gatt_server *server = user_data;
>>>> @@ -1167,6 +1221,11 @@ static void exec_write_complete_cb(struct gatt_db_attribute *attr, int err,
>>>> exec_next_prep_write(server, handle, err);
>>>> }
>>>>
>>>> +static bool match_attribute_handle(const void *data, const void *match_data)
>>>> +{
>>>> + return PTR_TO_UINT(data) == PTR_TO_UINT(match_data);
>>>> +}
>>>> +
>>>> static void exec_next_prep_write(struct bt_gatt_server *server,
>>>> uint16_t ehandle, int err)
>>>> {
>>>> @@ -1177,10 +1236,17 @@ static void exec_next_prep_write(struct bt_gatt_server *server,
>>>> if (err)
>>> Shouldn't you remove all elements of server->exec_queue here? Looking
>>> at the next patch, all attributes in exec_queue shall be notified that
>>> exec write is canceled too.
>>> And just open question: with introducing two types of write requests
>>> in reliable write procedure: BT_ATT_OP_EXEC_WRITE_REQ and
>>> BT_ATT_OP_PREP_WRITE_REQ, we could avoid using two queues and pass
>>> prepare writes immediatelly to specific service implementation instead
>>> of storing all values in prepare writes in server.
>>
>> Even if we have some flush logic trigger by execute we cannot really
>> dispatch writes immediately to the services because it can fail with
>> some errors that are not allowed by the testing spec, yes there is a
>> specific test checking the e.g. invalid offset is only receive as a
>> response to execute. It looks like this is because the value could be
>> changed in between prepare and execute, which IMO is something
>> horribly broken with prepare + execute since it is intended for
>> reliable writes but it can produce all sort of errors which is
>> impossible to guess without reading the value written after it has
>> been executed in case anything has change in between the commands.
>>
>
> I see Marcins point and agree. According to spec if handle is valid,
> permisions are ok and there
> is enought place in the queue, Prepare Write Response shall be sent
> back. No matter if application
> likes data or not. Error on value validation we will get during
> execute writes. So in teory we could indeed
> avoid prep_queue and send data directly to application when handle is
> ok and permisions are fine.
I thought the data reaching the application would mean it shall now
apply, but perhaps Android interface does not care what the
application will do with it, anyway this is something I would not like
to have in D-Bus since we don't expose prepare + execute write there.
> Actually this is how we do it now in Android part.
> But on the other side, that is not big deal, so if you Luiz saw any
> issues with that, we can stay with server
> handling queuing prep writes.
Im afraid Android got this wrong, if do dispatch to the application it
means they have to be queued there so you no longer can guarantee the
sequence of execute e.g.
prep handle1 -> app1
prep handle2 -> app2
prep handle2 -> app2
prep handle1 -> app1
exec -> app1, app2, app1 ? (opps, it already execute the first and the
second write)
It is no doubt a completely bogus scenario, but still I think it is
better be safe than sorry if some profile turns out to depend in a
strict sequence.
>>>> next = queue_pop_head(server->prep_queue);
>>>> if (!next) {
>>>> - bt_att_send(server->att, BT_ATT_OP_EXEC_WRITE_RSP, NULL, 0,
>>>> - NULL, NULL, NULL);
>>>> + exec_next_write(server);
>>>> return;
>>>> }
>>>>
>>>> @@ -1192,9 +1258,9 @@ static void exec_next_prep_write(struct bt_gatt_server *server,
>>>>
>>>> status = gatt_db_attribute_write(attr, next->offset,
>>>> next->value, next->length,
>>>> - BT_ATT_OP_EXEC_WRITE_REQ,
>>>> + BT_ATT_OP_PREP_WRITE_REQ,
>>>> server->att,
>>>> - exec_write_complete_cb, server);
>>>> + prep_write_complete_cb, server);
>>>>
>>>> prep_write_data_destroy(next);
>>>>
>>>> @@ -1396,6 +1462,8 @@ struct bt_gatt_server *bt_gatt_server_new(struct gatt_db *db,
>>>> server->max_prep_queue_len = DEFAULT_MAX_PREP_QUEUE_LEN;
>>>> server->prep_queue = queue_new();
>>>>
>>>> + server->exec_queue = queue_new();
>>>> +
>>>> if (!gatt_server_register_att_handlers(server)) {
>>>> bt_gatt_server_free(server);
>>>> return NULL;
>>>> --
>>>> 2.5.0
>>>>
>>>> --
>>>> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
>>>> the body of a message to majordomo@vger.kernel.org
>>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>>
>>>
>>>
>>>
>>> --
>>> BR
>>> Marcin Kraglak
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>
>>
>>
>> --
>> Luiz Augusto von Dentz
>
>
>
> --
> BR / Pozdrawiam
> Łukasz
--
Luiz Augusto von Dentz
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 3/4] shared/gatt-server: Improve long write session handling
2015-11-18 23:01 ` Luiz Augusto von Dentz
@ 2015-11-20 8:19 ` Łukasz Rymanowski
[not found] ` <CAExxLLCJ5QCQg+48WBPe-ss3TDrpDZj9rO5x3zzPAi0B=WQtPA@mail.gmail.com>
1 sibling, 0 replies; 19+ messages in thread
From: Łukasz Rymanowski @ 2015-11-20 8:19 UTC (permalink / raw)
To: Luiz Augusto von Dentz
Cc: Marcin Kraglak, linux-bluetooth@vger.kernel.org development
Hi Luiz,
On Thu, Nov 19, 2015 at 12:01 AM, Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
> Hi Lukasz,
>
> On Tue, Nov 17, 2015 at 11:32 PM, Łukasz Rymanowski
> <lukasz.rymanowski@codecoup.pl> wrote:
>> Hi Luiz, Marcin,
>>
>> On Tue, Nov 17, 2015 at 1:49 PM, Luiz Augusto von Dentz
>> <luiz.dentz@gmail.com> wrote:
>>> Hi Marcin,
>>>
>>> On Tue, Nov 17, 2015 at 2:12 PM, Marcin Kraglak
>>> <marcin.kraglak@tieto.com> wrote:
>>>> Hi Lukasz,
>>>>
>>>> On 16 November 2015 at 22:08, Łukasz Rymanowski
>>>> <lukasz.rymanowski@codecoup.pl> wrote:
>>>>>
>>>>> With current implementation application layer has no idea when
>>>>> long write session ends as it gets write callback with execute
>>>>> write for each prepare write data.
>>>>> With this patch, after data are completly received by gatt-server it
>>>>> starts to call write callback in proper order with opcode
>>>>> BT_ATT_OP_PREP_WRITE_REQ. After that, each characteristic gets
>>>>> BT_ATT_OP_EXEC_WRITE_REQ so it knows that complete characteristic value
>>>>> has been received and data can be marked as valid.
>>>>> ---
>>>>> src/shared/gatt-server.c | 78 ++++++++++++++++++++++++++++++++++++++++++++----
>>>>> 1 file changed, 73 insertions(+), 5 deletions(-)
>>>>>
>>>>> diff --git a/src/shared/gatt-server.c b/src/shared/gatt-server.c
>>>>> index 329f53f..931d619 100644
>>>>> --- a/src/shared/gatt-server.c
>>>>> +++ b/src/shared/gatt-server.c
>>>>> @@ -104,6 +104,8 @@ struct bt_gatt_server {
>>>>> struct queue *prep_queue;
>>>>> unsigned int max_prep_queue_len;
>>>>>
>>>>> + struct queue *exec_queue;
>>>>> +
>>>>> struct async_read_op *pending_read_op;
>>>>> struct async_write_op *pending_write_op;
>>>>>
>>>>> @@ -137,6 +139,7 @@ static void bt_gatt_server_free(struct bt_gatt_server *server)
>>>>> server->pending_write_op->server = NULL;
>>>>>
>>>>> queue_destroy(server->prep_queue, prep_write_data_destroy);
>>>>> + queue_destroy(server->exec_queue, NULL);
>>>>>
>>>>> gatt_db_unref(server->db);
>>>>> bt_att_unref(server->att);
>>>>> @@ -1155,10 +1158,61 @@ error:
>>>>>
>>>>> }
>>>>>
>>>>> +static void exec_next_write(struct bt_gatt_server *server);
>>>>> +
>>>>> +static void exec_write_complete_cb(struct gatt_db_attribute *attr, int err,
>>>>> + void *user_data)
>>>>> +{
>>>>> + struct bt_gatt_server *server = user_data;
>>>>> + uint16_t handle = gatt_db_attribute_get_handle(attr);
>>>>> +
>>>>> + if (err) {
>>>>> + queue_remove_all(server->exec_queue, NULL, NULL, NULL);
>>>>> + bt_att_send_error_rsp(server->att, BT_ATT_OP_EXEC_WRITE_REQ,
>>>>> + handle, err);
>>>>> + }
>>>>> +
>>>>> + exec_next_write(server);
>>>>> +}
>>>>> +
>>>>> +static void exec_next_write(struct bt_gatt_server *server)
>>>>> +{
>>>>> + uint16_t *handle;
>>>>> + struct gatt_db_attribute *attr;
>>>>> + bool status;
>>>>> + int err;
>>>>> +
>>>>> + handle = queue_pop_head(server->exec_queue);
>>>>> + if (!handle) {
>>>>> + bt_att_send(server->att, BT_ATT_OP_EXEC_WRITE_RSP, NULL, 0,
>>>>> + NULL, NULL, NULL);
>>>>> + }
>>>>> +
>>>>> + attr = gatt_db_get_attribute(server->db, PTR_TO_UINT(handle));
>>>>> + if (!attr) {
>>>>> + err = BT_ATT_ERROR_UNLIKELY;
>>>>> + goto error;
>>>>> + }
>>>>> +
>>>>> + status = gatt_db_attribute_write(attr, 0, NULL , 0,
>>>>> + BT_ATT_OP_EXEC_WRITE_REQ,
>>>>> + server->att,
>>>>> + exec_write_complete_cb, server);
>>>>> + if (status)
>>>>> + return;
>>>>> +
>>>>> + err = BT_ATT_ERROR_UNLIKELY;
>>>>> +
>>>>> +error:
>>>>> + queue_remove_all(server->exec_queue, NULL, NULL, NULL);
>>>>> + bt_att_send_error_rsp(server->att, BT_ATT_OP_EXEC_WRITE_REQ,
>>>>> + PTR_TO_UINT(handle), err);
>>>>> +}
>>>>> +
>>>>> static void exec_next_prep_write(struct bt_gatt_server *server,
>>>>> uint16_t ehandle, int err);
>>>>>
>>>>> -static void exec_write_complete_cb(struct gatt_db_attribute *attr, int err,
>>>>> +static void prep_write_complete_cb(struct gatt_db_attribute *attr, int err,
>>>>> void *user_data)
>>>>> {
>>>>> struct bt_gatt_server *server = user_data;
>>>>> @@ -1167,6 +1221,11 @@ static void exec_write_complete_cb(struct gatt_db_attribute *attr, int err,
>>>>> exec_next_prep_write(server, handle, err);
>>>>> }
>>>>>
>>>>> +static bool match_attribute_handle(const void *data, const void *match_data)
>>>>> +{
>>>>> + return PTR_TO_UINT(data) == PTR_TO_UINT(match_data);
>>>>> +}
>>>>> +
>>>>> static void exec_next_prep_write(struct bt_gatt_server *server,
>>>>> uint16_t ehandle, int err)
>>>>> {
>>>>> @@ -1177,10 +1236,17 @@ static void exec_next_prep_write(struct bt_gatt_server *server,
>>>>> if (err)
>>>> Shouldn't you remove all elements of server->exec_queue here? Looking
>>>> at the next patch, all attributes in exec_queue shall be notified that
>>>> exec write is canceled too.
>>>> And just open question: with introducing two types of write requests
>>>> in reliable write procedure: BT_ATT_OP_EXEC_WRITE_REQ and
>>>> BT_ATT_OP_PREP_WRITE_REQ, we could avoid using two queues and pass
>>>> prepare writes immediatelly to specific service implementation instead
>>>> of storing all values in prepare writes in server.
>>>
>>> Even if we have some flush logic trigger by execute we cannot really
>>> dispatch writes immediately to the services because it can fail with
>>> some errors that are not allowed by the testing spec, yes there is a
>>> specific test checking the e.g. invalid offset is only receive as a
>>> response to execute. It looks like this is because the value could be
>>> changed in between prepare and execute, which IMO is something
>>> horribly broken with prepare + execute since it is intended for
>>> reliable writes but it can produce all sort of errors which is
>>> impossible to guess without reading the value written after it has
>>> been executed in case anything has change in between the commands.
>>>
>>
>> I see Marcins point and agree. According to spec if handle is valid,
>> permisions are ok and there
>> is enought place in the queue, Prepare Write Response shall be sent
>> back. No matter if application
>> likes data or not. Error on value validation we will get during
>> execute writes. So in teory we could indeed
>> avoid prep_queue and send data directly to application when handle is
>> ok and permisions are fine.
>
> I thought the data reaching the application would mean it shall now
> apply, but perhaps Android interface does not care what the
> application will do with it, anyway this is something I would not like
> to have in D-Bus since we don't expose prepare + execute write there.
>
>> Actually this is how we do it now in Android part.
>> But on the other side, that is not big deal, so if you Luiz saw any
>> issues with that, we can stay with server
>> handling queuing prep writes.
>
> Im afraid Android got this wrong, if do dispatch to the application it
> means they have to be queued there so you no longer can guarantee the
> sequence of execute e.g.
>
> prep handle1 -> app1
> prep handle2 -> app2
> prep handle2 -> app2
> prep handle1 -> app1
>
> exec -> app1, app2, app1 ? (opps, it already execute the first and the
> second write)
>
Indeed that is the issue.
Now, lets take other example.
prep handle1 -> app1 (offset 0)
prep handle2 -> app1 (offset 20 - means that first prepare was only
part of the value)
Exec->app1 ?( opps, what to do with only part of the value)
To summarize, current solution in BlueZ works only for reliable write,
Android solution works for both but have
the one issue you mentioned (when there is more then one prepare for
same characteristic)
The whole problem is that gatt server needs to figure out if client is
doing reliable or long write and actually
since we queue all the prep writes we should be able to do that.
In case of reliable session we could keep current behavior, and in
case of long session we probably
should aggregate complete value and do one exec write to application,
does it makes sense?
For Android I will just skip prep and execute write but just use
regular write request. Basically we
will make use of logic already in gatt-server and do not bother app
with prep/execute. That should work.
> It is no doubt a completely bogus scenario, but still I think it is
> better be safe than sorry if some profile turns out to depend in a
> strict sequence.
>
>>>>> next = queue_pop_head(server->prep_queue);
>>>>> if (!next) {
>>>>> - bt_att_send(server->att, BT_ATT_OP_EXEC_WRITE_RSP, NULL, 0,
>>>>> - NULL, NULL, NULL);
>>>>> + exec_next_write(server);
>>>>> return;
>>>>> }
>>>>>
>>>>> @@ -1192,9 +1258,9 @@ static void exec_next_prep_write(struct bt_gatt_server *server,
>>>>>
>>>>> status = gatt_db_attribute_write(attr, next->offset,
>>>>> next->value, next->length,
>>>>> - BT_ATT_OP_EXEC_WRITE_REQ,
>>>>> + BT_ATT_OP_PREP_WRITE_REQ,
>>>>> server->att,
>>>>> - exec_write_complete_cb, server);
>>>>> + prep_write_complete_cb, server);
>>>>>
>>>>> prep_write_data_destroy(next);
>>>>>
>>>>> @@ -1396,6 +1462,8 @@ struct bt_gatt_server *bt_gatt_server_new(struct gatt_db *db,
>>>>> server->max_prep_queue_len = DEFAULT_MAX_PREP_QUEUE_LEN;
>>>>> server->prep_queue = queue_new();
>>>>>
>>>>> + server->exec_queue = queue_new();
>>>>> +
>>>>> if (!gatt_server_register_att_handlers(server)) {
>>>>> bt_gatt_server_free(server);
>>>>> return NULL;
>>>>> --
>>>>> 2.5.0
>>>>>
>>>>> --
>>>>> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
>>>>> the body of a message to majordomo@vger.kernel.org
>>>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>>>
>>>>
>>>>
>>>>
>>>> --
>>>> BR
>>>> Marcin Kraglak
>>>> --
>>>> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
>>>> the body of a message to majordomo@vger.kernel.org
>>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>>
>>>
>>>
>>> --
>>> Luiz Augusto von Dentz
>>
>>
>>
>> --
>> BR / Pozdrawiam
>> Łukasz
>
>
>
> --
> Luiz Augusto von Dentz
--
BR / Pozdrawiam
Łukasz
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 3/4] shared/gatt-server: Improve long write session handling
[not found] ` <CAExxLLCJ5QCQg+48WBPe-ss3TDrpDZj9rO5x3zzPAi0B=WQtPA@mail.gmail.com>
@ 2015-11-20 9:03 ` Luiz Augusto von Dentz
0 siblings, 0 replies; 19+ messages in thread
From: Luiz Augusto von Dentz @ 2015-11-20 9:03 UTC (permalink / raw)
To: Łukasz Rymanowski
Cc: Marcin Kraglak, linux-bluetooth@vger.kernel.org development
Hi Lukasz,
On Fri, Nov 20, 2015 at 10:15 AM, Łukasz Rymanowski
<lukasz.rymanowski@codecoup.pl> wrote:
> Hi Luiz,
>
> On Thu, Nov 19, 2015 at 12:01 AM, Luiz Augusto von Dentz
> <luiz.dentz@gmail.com> wrote:
>>
>> Hi Lukasz,
>>
>> On Tue, Nov 17, 2015 at 11:32 PM, Łukasz Rymanowski
>> <lukasz.rymanowski@codecoup.pl> wrote:
>> > Hi Luiz, Marcin,
>> >
>> > On Tue, Nov 17, 2015 at 1:49 PM, Luiz Augusto von Dentz
>> > <luiz.dentz@gmail.com> wrote:
>> >> Hi Marcin,
>> >>
>> >> On Tue, Nov 17, 2015 at 2:12 PM, Marcin Kraglak
>> >> <marcin.kraglak@tieto.com> wrote:
>> >>> Hi Lukasz,
>> >>>
>> >>> On 16 November 2015 at 22:08, Łukasz Rymanowski
>> >>> <lukasz.rymanowski@codecoup.pl> wrote:
>> >>>>
>> >>>> With current implementation application layer has no idea when
>> >>>> long write session ends as it gets write callback with execute
>> >>>> write for each prepare write data.
>> >>>> With this patch, after data are completly received by gatt-server it
>> >>>> starts to call write callback in proper order with opcode
>> >>>> BT_ATT_OP_PREP_WRITE_REQ. After that, each characteristic gets
>> >>>> BT_ATT_OP_EXEC_WRITE_REQ so it knows that complete characteristic
>> >>>> value
>> >>>> has been received and data can be marked as valid.
>> >>>> ---
>> >>>> src/shared/gatt-server.c | 78
>> >>>> ++++++++++++++++++++++++++++++++++++++++++++----
>> >>>> 1 file changed, 73 insertions(+), 5 deletions(-)
>> >>>>
>> >>>> diff --git a/src/shared/gatt-server.c b/src/shared/gatt-server.c
>> >>>> index 329f53f..931d619 100644
>> >>>> --- a/src/shared/gatt-server.c
>> >>>> +++ b/src/shared/gatt-server.c
>> >>>> @@ -104,6 +104,8 @@ struct bt_gatt_server {
>> >>>> struct queue *prep_queue;
>> >>>> unsigned int max_prep_queue_len;
>> >>>>
>> >>>> + struct queue *exec_queue;
>> >>>> +
>> >>>> struct async_read_op *pending_read_op;
>> >>>> struct async_write_op *pending_write_op;
>> >>>>
>> >>>> @@ -137,6 +139,7 @@ static void bt_gatt_server_free(struct
>> >>>> bt_gatt_server *server)
>> >>>> server->pending_write_op->server = NULL;
>> >>>>
>> >>>> queue_destroy(server->prep_queue, prep_write_data_destroy);
>> >>>> + queue_destroy(server->exec_queue, NULL);
>> >>>>
>> >>>> gatt_db_unref(server->db);
>> >>>> bt_att_unref(server->att);
>> >>>> @@ -1155,10 +1158,61 @@ error:
>> >>>>
>> >>>> }
>> >>>>
>> >>>> +static void exec_next_write(struct bt_gatt_server *server);
>> >>>> +
>> >>>> +static void exec_write_complete_cb(struct gatt_db_attribute *attr,
>> >>>> int err,
>> >>>> + void
>> >>>> *user_data)
>> >>>> +{
>> >>>> + struct bt_gatt_server *server = user_data;
>> >>>> + uint16_t handle = gatt_db_attribute_get_handle(attr);
>> >>>> +
>> >>>> + if (err) {
>> >>>> + queue_remove_all(server->exec_queue, NULL, NULL,
>> >>>> NULL);
>> >>>> + bt_att_send_error_rsp(server->att,
>> >>>> BT_ATT_OP_EXEC_WRITE_REQ,
>> >>>> +
>> >>>> handle, err);
>> >>>> + }
>> >>>> +
>> >>>> + exec_next_write(server);
>> >>>> +}
>> >>>> +
>> >>>> +static void exec_next_write(struct bt_gatt_server *server)
>> >>>> +{
>> >>>> + uint16_t *handle;
>> >>>> + struct gatt_db_attribute *attr;
>> >>>> + bool status;
>> >>>> + int err;
>> >>>> +
>> >>>> + handle = queue_pop_head(server->exec_queue);
>> >>>> + if (!handle) {
>> >>>> + bt_att_send(server->att, BT_ATT_OP_EXEC_WRITE_RSP,
>> >>>> NULL, 0,
>> >>>> + NULL, NULL,
>> >>>> NULL);
>> >>>> + }
>> >>>> +
>> >>>> + attr = gatt_db_get_attribute(server->db,
>> >>>> PTR_TO_UINT(handle));
>> >>>> + if (!attr) {
>> >>>> + err = BT_ATT_ERROR_UNLIKELY;
>> >>>> + goto error;
>> >>>> + }
>> >>>> +
>> >>>> + status = gatt_db_attribute_write(attr, 0, NULL , 0,
>> >>>> +
>> >>>> BT_ATT_OP_EXEC_WRITE_REQ,
>> >>>> + server->att,
>> >>>> +
>> >>>> exec_write_complete_cb, server);
>> >>>> + if (status)
>> >>>> + return;
>> >>>> +
>> >>>> + err = BT_ATT_ERROR_UNLIKELY;
>> >>>> +
>> >>>> +error:
>> >>>> + queue_remove_all(server->exec_queue, NULL, NULL, NULL);
>> >>>> + bt_att_send_error_rsp(server->att, BT_ATT_OP_EXEC_WRITE_REQ,
>> >>>> + PTR_TO_UINT(handle),
>> >>>> err);
>> >>>> +}
>> >>>> +
>> >>>> static void exec_next_prep_write(struct bt_gatt_server *server,
>> >>>> uint16_t ehandle, int
>> >>>> err);
>> >>>>
>> >>>> -static void exec_write_complete_cb(struct gatt_db_attribute *attr,
>> >>>> int err,
>> >>>> +static void prep_write_complete_cb(struct gatt_db_attribute *attr,
>> >>>> int err,
>> >>>> void
>> >>>> *user_data)
>> >>>> {
>> >>>> struct bt_gatt_server *server = user_data;
>> >>>> @@ -1167,6 +1221,11 @@ static void exec_write_complete_cb(struct
>> >>>> gatt_db_attribute *attr, int err,
>> >>>> exec_next_prep_write(server, handle, err);
>> >>>> }
>> >>>>
>> >>>> +static bool match_attribute_handle(const void *data, const void
>> >>>> *match_data)
>> >>>> +{
>> >>>> + return PTR_TO_UINT(data) == PTR_TO_UINT(match_data);
>> >>>> +}
>> >>>> +
>> >>>> static void exec_next_prep_write(struct bt_gatt_server *server,
>> >>>> uint16_t ehandle, int
>> >>>> err)
>> >>>> {
>> >>>> @@ -1177,10 +1236,17 @@ static void exec_next_prep_write(struct
>> >>>> bt_gatt_server *server,
>> >>>> if (err)
>> >>> Shouldn't you remove all elements of server->exec_queue here? Looking
>> >>> at the next patch, all attributes in exec_queue shall be notified that
>> >>> exec write is canceled too.
>> >>> And just open question: with introducing two types of write requests
>> >>> in reliable write procedure: BT_ATT_OP_EXEC_WRITE_REQ and
>> >>> BT_ATT_OP_PREP_WRITE_REQ, we could avoid using two queues and pass
>> >>> prepare writes immediatelly to specific service implementation instead
>> >>> of storing all values in prepare writes in server.
>> >>
>> >> Even if we have some flush logic trigger by execute we cannot really
>> >> dispatch writes immediately to the services because it can fail with
>> >> some errors that are not allowed by the testing spec, yes there is a
>> >> specific test checking the e.g. invalid offset is only receive as a
>> >> response to execute. It looks like this is because the value could be
>> >> changed in between prepare and execute, which IMO is something
>> >> horribly broken with prepare + execute since it is intended for
>> >> reliable writes but it can produce all sort of errors which is
>> >> impossible to guess without reading the value written after it has
>> >> been executed in case anything has change in between the commands.
>> >>
>> >
>> > I see Marcins point and agree. According to spec if handle is valid,
>> > permisions are ok and there
>> > is enought place in the queue, Prepare Write Response shall be sent
>> > back. No matter if application
>> > likes data or not. Error on value validation we will get during
>> > execute writes. So in teory we could indeed
>> > avoid prep_queue and send data directly to application when handle is
>> > ok and permisions are fine.
>>
>> I thought the data reaching the application would mean it shall now
>> apply, but perhaps Android interface does not care what the
>> application will do with it, anyway this is something I would not like
>> to have in D-Bus since we don't expose prepare + execute write there.
>>
>>
>> > Actually this is how we do it now in Android part.
>> > But on the other side, that is not big deal, so if you Luiz saw any
>> > issues with that, we can stay with server
>> > handling queuing prep writes.
>>
>> Im afraid Android got this wrong, if do dispatch to the application it
>> means they have to be queued there so you no longer can guarantee the
>> sequence of execute e.g.
>>
>> prep handle1 -> app1
>> prep handle2 -> app2
>> prep handle2 -> app2
>> prep handle1 -> app1
>>
>> exec -> app1, app2, app1 ? (opps, it already execute the first and the
>> second write)
>>
>
> Indeed that is the issue.
>
> Now, lets take other example.
>
> prep handle1 -> app1 (offset 0)
> prep handle2 -> app1 (offset 20 - means that first prepare was only part of
> the value)
>
> Exec->app1 ?( opps, what to do with only part of the value)
If you are talking about D-Bus experimental interface, yes that does
not support offset for WriteValue yet but I think we might need to
introduce for long writes, actually Web Bluetooth also don't care
about offset:
https://webbluetoothcg.github.io/web-bluetooth/#dom-bluetoothgattcharacteristic-writevalue
The problem was that for client it won't be that useful since you
almost always start from offset 0 which would probably mean we could
just aggregate the writes and send on idle, but if we want we can add
the offset as well.
> To summarize, current solution in BlueZ works only for reliable write,
> Android solution works for both but have
> the one issue you mentioned (when there is more then one prepare for same
> characteristic)
Well Android doesn't seems to do anything except pass the request up
to the application, then it is up to the application to queue and
execute, perhaps you even need an app that behave well enough to pass
qualification.
> The whole problem is that gatt server needs to figure out if client is doing
> reliable or long write and actually
> since we queue all the prep writes we should be able to do that.
> In case of reliable session we could keep current behavior, and in case of
> long session we probably
> should aggregate complete value and do one exec write to application, does
> it makes sense?
It could in theory detect if the previous item on the queue is a
contiguous area then just append but Im not sure it is worth it since
we could do this in gatt-database with an idle timeout.
> For Android I will just skip prep and execute write but just use regular
> write request. Basically we
> will make use of logic already in gatt-server and do not bother app with
> prep/execute. That should work.
Yep, and if it already has an offset then it could handle any sequence.
>
>
>> It is no doubt a completely bogus scenario, but still I think it is
>> better be safe than sorry if some profile turns out to depend in a
>> strict sequence.
>>
>> >>>> next = queue_pop_head(server->prep_queue);
>> >>>> if (!next) {
>> >>>> - bt_att_send(server->att, BT_ATT_OP_EXEC_WRITE_RSP,
>> >>>> NULL, 0,
>> >>>> - NULL, NULL,
>> >>>> NULL);
>> >>>> + exec_next_write(server);
>> >>>> return;
>> >>>> }
>> >>>>
>> >>>> @@ -1192,9 +1258,9 @@ static void exec_next_prep_write(struct
>> >>>> bt_gatt_server *server,
>> >>>>
>> >>>> status = gatt_db_attribute_write(attr, next->offset,
>> >>>> next->value,
>> >>>> next->length,
>> >>>> -
>> >>>> BT_ATT_OP_EXEC_WRITE_REQ,
>> >>>> +
>> >>>> BT_ATT_OP_PREP_WRITE_REQ,
>> >>>> server->att,
>> >>>> -
>> >>>> exec_write_complete_cb, server);
>> >>>> +
>> >>>> prep_write_complete_cb, server);
>> >>>>
>> >>>> prep_write_data_destroy(next);
>> >>>>
>> >>>> @@ -1396,6 +1462,8 @@ struct bt_gatt_server
>> >>>> *bt_gatt_server_new(struct gatt_db *db,
>> >>>> server->max_prep_queue_len = DEFAULT_MAX_PREP_QUEUE_LEN;
>> >>>> server->prep_queue = queue_new();
>> >>>>
>> >>>> + server->exec_queue = queue_new();
>> >>>> +
>> >>>> if (!gatt_server_register_att_handlers(server)) {
>> >>>> bt_gatt_server_free(server);
>> >>>> return NULL;
>> >>>> --
>> >>>> 2.5.0
>> >>>>
>> >>>> --
>> >>>> To unsubscribe from this list: send the line "unsubscribe
>> >>>> linux-bluetooth" in
>> >>>> the body of a message to majordomo@vger.kernel.org
>> >>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>> >>>
>> >>>
>> >>>
>> >>>
>> >>> --
>> >>> BR
>> >>> Marcin Kraglak
>> >>> --
>> >>> To unsubscribe from this list: send the line "unsubscribe
>> >>> linux-bluetooth" in
>> >>> the body of a message to majordomo@vger.kernel.org
>> >>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>> >>
>> >>
>> >>
>> >> --
>> >> Luiz Augusto von Dentz
>> >
>> >
>> >
>> > --
>> > BR / Pozdrawiam
>> > Łukasz
>>
>>
>>
>> --
>> Luiz Augusto von Dentz
>
>
>
>
> --
> BR / Pozdrawiam
> Łukasz
--
Luiz Augusto von Dentz
^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2015-11-20 9:03 UTC | newest]
Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-11-16 21:08 [PATCH 0/4] shared/gatt: Couple fixes and improvements Łukasz Rymanowski
2015-11-16 21:08 ` [PATCH 1/4] shared/gatt-server: Fix exit on error during execute write Łukasz Rymanowski
2015-11-16 21:08 ` [PATCH 2/4] shared/gatt-client: Add notify type to notification callback Łukasz Rymanowski
2015-11-17 14:10 ` Luiz Augusto von Dentz
2015-11-17 22:03 ` Łukasz Rymanowski
2015-11-16 21:08 ` [PATCH 3/4] shared/gatt-server: Improve long write session handling Łukasz Rymanowski
2015-11-17 9:45 ` Grzegorz Kolodziejczyk
2015-11-17 12:12 ` Marcin Kraglak
2015-11-17 12:49 ` Luiz Augusto von Dentz
2015-11-17 21:32 ` Łukasz Rymanowski
2015-11-18 23:01 ` Luiz Augusto von Dentz
2015-11-20 8:19 ` Łukasz Rymanowski
[not found] ` <CAExxLLCJ5QCQg+48WBPe-ss3TDrpDZj9rO5x3zzPAi0B=WQtPA@mail.gmail.com>
2015-11-20 9:03 ` Luiz Augusto von Dentz
2015-11-17 21:22 ` Łukasz Rymanowski
2015-11-16 21:08 ` [PATCH 4/4] shared/gatt-server: Cancel remaining write exec on error in the middle Łukasz Rymanowski
2015-11-17 9:57 ` Grzegorz Kolodziejczyk
2015-11-17 21:05 ` Łukasz Rymanowski
2015-11-17 15:22 ` Marcin Kraglak
2015-11-17 21:04 ` Łukasz Rymanowski
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.