[PATCH 1/3] android/gatt: Fix parallel reading/writing attributes values from applications

[PATCH 1/3] android/gatt: Fix parallel reading/writing attributes values from applications

[Marcin Kraglak]

It is needed because in some cases we send few read requests to applications.
Now all transactions data are overriden by last one, and if application wants
to respond to previous requests, transaction data is not found.
It happens when two devices will read attribute from one application in the same
time or if device will read few values in time (i.e. read by type request or find
by type value request).
---
 android/gatt.c | 77 ++++++++++++++++++++++++++++++++++++++--------------------
 1 file changed, 50 insertions(+), 27 deletions(-)

diff --git a/android/gatt.c b/android/gatt.c
index 429181f..f450eac 100644
--- a/android/gatt.c
+++ b/android/gatt.c
@@ -105,9 +105,6 @@ struct gatt_app {
 
 	/* Valid for client applications */
 	struct queue *notifications;
-
-	/* Transaction data valid for server application */
-	struct pending_trans_data trans_id;
 };
 
 struct element_id {
@@ -174,6 +171,7 @@ struct gatt_device {
 struct app_connection {
 	struct gatt_device *device;
 	struct gatt_app *app;
+	struct queue *transactions;
 	int32_t id;
 };
 
@@ -855,6 +853,7 @@ static void destroy_connection(void *data)
 		connection_cleanup(conn->device);
 
 cleanup:
+	queue_destroy(conn->transactions, free);
 	device_unref(conn->device);
 	free(conn);
 }
@@ -1304,9 +1303,15 @@ static struct app_connection *create_connection(struct gatt_device *device,
 	new_conn->app = app;
 	new_conn->id = last_conn_id++;
 
+	new_conn->transactions = queue_new();
+	if (!new_conn->transactions) {
+		free(new_conn);
+		return NULL;
+	}
+
 	if (!queue_push_head(app_connections, new_conn)) {
 		error("gatt: Cannot push client on the client queue!?");
-
+		queue_destroy(new_conn->transactions, free);
 		free(new_conn);
 		return NULL;
 	}
@@ -4167,26 +4172,35 @@ done:
 		send_dev_pending_response(dev, opcode);
 }
 
-static void set_trans_id(struct gatt_app *app, unsigned int id, int8_t opcode)
+static struct pending_trans_data *conn_add_transact(struct app_connection *conn,
+								uint8_t opcode)
 {
-	app->trans_id.id = id;
-	app->trans_id.opcode = opcode;
-}
+	struct pending_trans_data *transaction;
+	static int32_t trans_id = 1;
 
-static void clear_trans_id(struct gatt_app *app)
-{
-	app->trans_id.id = 0;
-	app->trans_id.opcode = 0;
+	transaction = new0(struct pending_trans_data, 1);
+	if (!transaction)
+		return NULL;
+
+	if (!queue_push_tail(conn->transactions, transaction)) {
+		free(transaction);
+		return NULL;
+	}
+
+	transaction->id = trans_id++;
+	transaction->opcode = opcode;
+
+	return transaction;
 }
 
 static void read_cb(uint16_t handle, uint16_t offset, uint8_t att_opcode,
 					bdaddr_t *bdaddr, void *user_data)
 {
+	struct pending_trans_data *transaction;
 	struct hal_ev_gatt_server_request_read ev;
 	struct gatt_app *app;
 	struct app_connection *conn;
 	int32_t id = PTR_TO_INT(user_data);
-	static int32_t trans_id = 1;
 
 	app = find_app_by_id(id);
 	if (!app) {
@@ -4203,14 +4217,16 @@ static void read_cb(uint16_t handle, uint16_t offset, uint8_t att_opcode,
 	memset(&ev, 0, sizeof(ev));
 
 	/* Store the request data, complete callback and transaction id */
-	set_trans_id(app, trans_id++, att_opcode);
+	transaction = conn_add_transact(conn, att_opcode);
+	if (!transaction)
+		goto failed;
 
 	bdaddr2android(bdaddr, ev.bdaddr);
 	ev.conn_id = conn->id;
 	ev.attr_handle = handle;
 	ev.offset = offset;
 	ev.is_long = att_opcode == ATT_OP_READ_BLOB_REQ;
-	ev.trans_id = app->trans_id.id;
+	ev.trans_id = transaction->id;
 
 	ipc_send_notif(hal_ipc, HAL_SERVICE_ID_GATT,
 					HAL_EV_GATT_SERVER_REQUEST_READ,
@@ -4230,9 +4246,9 @@ static void write_cb(uint16_t handle, uint16_t offset,
 {
 	uint8_t buf[IPC_MTU];
 	struct hal_ev_gatt_server_request_write *ev = (void *) buf;
+	struct pending_trans_data *transaction;
 	struct gatt_app *app;
 	int32_t id = PTR_TO_INT(user_data);
-	static int32_t trans_id = 1;
 	struct app_connection *conn;
 
 	app = find_app_by_id(id);
@@ -4248,7 +4264,9 @@ static void write_cb(uint16_t handle, uint16_t offset,
 	}
 
 	/* Store the request data, complete callback and transaction id */
-	set_trans_id(app, trans_id++, att_opcode);
+	transaction = conn_add_transact(conn, att_opcode);
+	if (!transaction)
+		goto failed;
 
 	/* TODO figure it out */
 	if (att_opcode == ATT_OP_EXEC_WRITE_REQ)
@@ -4261,7 +4279,7 @@ static void write_cb(uint16_t handle, uint16_t offset,
 	ev->offset = offset;
 
 	ev->conn_id = conn->id;
-	ev->trans_id = app->trans_id.id;
+	ev->trans_id = transaction->id;
 
 	ev->is_prep = att_opcode == ATT_OP_PREP_WRITE_REQ;
 	ev->need_rsp = att_opcode == ATT_OP_WRITE_REQ;
@@ -4555,11 +4573,18 @@ reply:
 				HAL_OP_GATT_SERVER_SEND_INDICATION, status);
 }
 
+static bool match_trans_id(const void *data, const void *user_data)
+{
+	const struct pending_trans_data *transaction = data;
+
+	return transaction->id == PTR_TO_UINT(user_data);
+}
+
 static void handle_server_send_response(const void *buf, uint16_t len)
 {
 	const struct hal_cmd_gatt_server_send_response *cmd = buf;
+	struct pending_trans_data *transaction;
 	struct app_connection *conn;
-	struct gatt_app *app;
 	uint8_t status;
 
 	DBG("");
@@ -4571,22 +4596,20 @@ static void handle_server_send_response(const void *buf, uint16_t len)
 		goto reply;
 	}
 
-	app = conn->app;
-
-	if ((unsigned int)cmd->trans_id != app->trans_id.id) {
-		error("gatt: transaction ID mismatch (%d!=%d)",
-					cmd->trans_id, app->trans_id.id);
-
+	transaction = queue_remove_if(conn->transactions, match_trans_id,
+						UINT_TO_PTR(cmd->trans_id));
+	if (!transaction) {
+		error("gatt: transaction ID = %d not found", cmd->trans_id);
 		status = HAL_STATUS_FAILED;
 		goto reply;
 	}
 
-	send_gatt_response(conn->app->trans_id.opcode, cmd->handle, cmd->offset,
+	send_gatt_response(transaction->opcode, cmd->handle, cmd->offset,
 					cmd->status, cmd->len, cmd->data,
 					&conn->device->bdaddr);
 
 	/* Clean request data */
-	clear_trans_id(app);
+	free(transaction);
 
 	status = HAL_STATUS_SUCCESS;
 
-- 
1.9.3

[PATCH 2/3] android/gatt: Fix state of pending request for prep write

[Marcin Kraglak]

From: Lukasz Rymanowski <lukasz.rymanowski@tieto.com>

State PENDING is used when we send request to upper layer and this is
how we do with prep write request
---
 android/gatt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/android/gatt.c b/android/gatt.c
index f450eac..ddba505 100644
--- a/android/gatt.c
+++ b/android/gatt.c
@@ -5219,7 +5219,7 @@ static uint8_t write_prep_request(const uint8_t *cmd, uint16_t cmd_len,
 
 	data->handle = handle;
 	data->offset = offset;
-	data->state = REQUEST_INIT;
+	data->state = REQUEST_PENDING;
 
 	if (!queue_push_tail(dev->pending_requests, data)) {
 		free(data);
-- 
1.9.3

[PATCH 3/3] android/gatt: Handle prepare and execute write

[Marcin Kraglak]

From: Lukasz Rymanowski <lukasz.rymanowski@tieto.com>

Once remote device sends write execute we need to notify all the
server applications with this. Application will send response on this and
once we collect all the responses we should send execute write response
to remote device

This is initial version of handling execute write. Still many corner cases
have to be handled. Some of them seems to be not easy to solve with
existing BT HAL API e.g. BlueZ sends execute write to couple of server
applications and one of them reply with error. According to spec execute
write shall be atomic. In scenario that one server out of many replies
with error makes this write not atomic. There is no way to handle it with current
BT HAL API.
---
 android/gatt.c | 112 +++++++++++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 106 insertions(+), 6 deletions(-)

diff --git a/android/gatt.c b/android/gatt.c
index ddba505..147c4d1 100644
--- a/android/gatt.c
+++ b/android/gatt.c
@@ -173,6 +173,8 @@ struct app_connection {
 	struct gatt_app *app;
 	struct queue *transactions;
 	int32_t id;
+
+	bool wait_execute_write;
 };
 
 static struct ipc *hal_ipc = NULL;
@@ -4263,15 +4265,18 @@ static void write_cb(uint16_t handle, uint16_t offset,
 		goto failed;
 	}
 
+	/*
+	 * Remember that this application has ongoing prep write
+	 * Need it later to find out where to send execute write
+	 */
+	if (att_opcode == ATT_OP_PREP_WRITE_REQ)
+		conn->wait_execute_write = true;
+
 	/* Store the request data, complete callback and transaction id */
 	transaction = conn_add_transact(conn, att_opcode);
 	if (!transaction)
 		goto failed;
 
-	/* TODO figure it out */
-	if (att_opcode == ATT_OP_EXEC_WRITE_REQ)
-		goto failed;
-
 	memset(ev, 0, sizeof(*ev));
 
 	bdaddr2android(bdaddr, &ev->bdaddr);
@@ -4580,10 +4585,25 @@ static bool match_trans_id(const void *data, const void *user_data)
 	return transaction->id == PTR_TO_UINT(user_data);
 }
 
+
+static bool find_conn_waiting_exec_write(const void *data,
+							const void *user_data)
+{
+	const struct app_connection *conn = data;
+
+	return conn->wait_execute_write;
+}
+
+static bool pending_execute_write(void)
+{
+	return queue_find(app_connections, find_conn_waiting_exec_write, NULL);
+}
+
 static void handle_server_send_response(const void *buf, uint16_t len)
 {
 	const struct hal_cmd_gatt_server_send_response *cmd = buf;
 	struct pending_trans_data *transaction;
+	uint16_t handle = cmd->handle;
 	struct app_connection *conn;
 	uint8_t status;
 
@@ -4604,10 +4624,27 @@ static void handle_server_send_response(const void *buf, uint16_t len)
 		goto reply;
 	}
 
-	send_gatt_response(transaction->opcode, cmd->handle, cmd->offset,
+	if (transaction->opcode == ATT_OP_EXEC_WRITE_REQ) {
+		conn->wait_execute_write = false;
+
+		/* Check for execute response from all server applications */
+		if (pending_execute_write())
+			goto done;
+
+		/* Make sure handle is 0. We need it to find pending request */
+		handle = 0;
+
+		/*
+		 * FIXME: Handle situation when not all server applications
+		 * respond with a success.
+		 */
+	}
+
+	send_gatt_response(transaction->opcode, handle, cmd->offset,
 					cmd->status, cmd->len, cmd->data,
 					&conn->device->bdaddr);
 
+done:
 	/* Clean request data */
 	free(transaction);
 
@@ -5233,6 +5270,66 @@ static uint8_t write_prep_request(const uint8_t *cmd, uint16_t cmd_len,
 	return 0;
 }
 
+static void send_server_write_execute_notify(void *data, void *user_data)
+{
+	struct hal_ev_gatt_server_request_exec_write *ev = user_data;
+	struct pending_trans_data *transaction;
+	struct app_connection *conn = data;
+
+	if (!conn->wait_execute_write)
+		return;
+
+	ev->conn_id = conn->id;
+
+	transaction = conn_add_transact(conn, ATT_OP_EXEC_WRITE_REQ);
+	if (!transaction) {
+		conn->wait_execute_write = false;
+		return;
+	}
+
+	ev->trans_id = transaction->id;
+
+	ipc_send_notif(hal_ipc, HAL_SERVICE_ID_GATT,
+				HAL_EV_GATT_SERVER_REQUEST_EXEC_WRITE,
+				sizeof(*ev), ev);
+}
+
+static uint8_t write_execute_request(const uint8_t *cmd, uint16_t cmd_len,
+						struct gatt_device *dev)
+{
+	struct hal_ev_gatt_server_request_exec_write ev;
+	uint8_t value;
+	struct pending_request *data;
+
+	/*
+	 * Check if there was any write prep before.
+	 * TODO: Try to find better error code if possible
+	 */
+	if (!pending_execute_write())
+		return ATT_ECODE_UNLIKELY;
+
+	if (!dec_exec_write_req(cmd, cmd_len, &value))
+		return ATT_ECODE_INVALID_PDU;
+
+	memset(&ev, 0, sizeof(ev));
+	bdaddr2android(&dev->bdaddr, &ev.bdaddr);
+	ev.exec_write = value;
+
+	data = new0(struct pending_request, 1);
+	if (!data)
+		return ATT_ECODE_INSUFF_RESOURCES;
+
+	data->state = REQUEST_PENDING;
+	if (!queue_push_tail(dev->pending_requests, data)) {
+		free(data);
+		return ATT_ECODE_INSUFF_RESOURCES;
+	}
+
+	queue_foreach(app_connections, send_server_write_execute_notify, &ev);
+
+	return 0;
+}
+
 static void att_handler(const uint8_t *ipdu, uint16_t len, gpointer user_data)
 {
 	struct gatt_device *dev = user_data;
@@ -5301,7 +5398,10 @@ static void att_handler(const uint8_t *ipdu, uint16_t len, gpointer user_data)
 		/* Client will handle this */
 		return;
 	case ATT_OP_EXEC_WRITE_REQ:
-		/* TODO */
+		status = write_execute_request(ipdu, len, dev);
+		if (!status)
+			return;
+		break;
 	case ATT_OP_HANDLE_CNF:
 	case ATT_OP_READ_MULTI_REQ:
 	default:
-- 
1.9.3

Re: [PATCH 1/3] android/gatt: Fix parallel reading/writing attributes values from applications

[Szymon Janc]

Hi Marcin,

On Thursday 05 of June 2014 11:38:50 Marcin Kraglak wrote:
> It is needed because in some cases we send few read requests to
> applications. Now all transactions data are overriden by last one, and if
> application wants to respond to previous requests, transaction data is not
> found.
> It happens when two devices will read attribute from one application in the
> same time or if device will read few values in time (i.e. read by type
> request or find by type value request).
> ---
>  android/gatt.c | 77
> ++++++++++++++++++++++++++++++++++++++-------------------- 1 file changed,
> 50 insertions(+), 27 deletions(-)
> 
> diff --git a/android/gatt.c b/android/gatt.c
> index 429181f..f450eac 100644
> --- a/android/gatt.c
> +++ b/android/gatt.c
> @@ -105,9 +105,6 @@ struct gatt_app {
> 
>  	/* Valid for client applications */
>  	struct queue *notifications;
> -
> -	/* Transaction data valid for server application */
> -	struct pending_trans_data trans_id;
>  };
> 
>  struct element_id {
> @@ -174,6 +171,7 @@ struct gatt_device {
>  struct app_connection {
>  	struct gatt_device *device;
>  	struct gatt_app *app;
> +	struct queue *transactions;
>  	int32_t id;
>  };
> 
> @@ -855,6 +853,7 @@ static void destroy_connection(void *data)
>  		connection_cleanup(conn->device);
> 
>  cleanup:
> +	queue_destroy(conn->transactions, free);
>  	device_unref(conn->device);
>  	free(conn);
>  }
> @@ -1304,9 +1303,15 @@ static struct app_connection
> *create_connection(struct gatt_device *device, new_conn->app = app;
>  	new_conn->id = last_conn_id++;
> 
> +	new_conn->transactions = queue_new();
> +	if (!new_conn->transactions) {
> +		free(new_conn);
> +		return NULL;
> +	}
> +
>  	if (!queue_push_head(app_connections, new_conn)) {
>  		error("gatt: Cannot push client on the client queue!?");
> -
> +		queue_destroy(new_conn->transactions, free);
>  		free(new_conn);
>  		return NULL;
>  	}
> @@ -4167,26 +4172,35 @@ done:
>  		send_dev_pending_response(dev, opcode);
>  }
> 
> -static void set_trans_id(struct gatt_app *app, unsigned int id, int8_t
> opcode) +static struct pending_trans_data *conn_add_transact(struct
> app_connection *conn, +								uint8_t opcode)
>  {
> -	app->trans_id.id = id;
> -	app->trans_id.opcode = opcode;
> -}
> +	struct pending_trans_data *transaction;
> +	static int32_t trans_id = 1;
> 
> -static void clear_trans_id(struct gatt_app *app)
> -{
> -	app->trans_id.id = 0;
> -	app->trans_id.opcode = 0;
> +	transaction = new0(struct pending_trans_data, 1);
> +	if (!transaction)
> +		return NULL;
> +
> +	if (!queue_push_tail(conn->transactions, transaction)) {
> +		free(transaction);
> +		return NULL;
> +	}
> +
> +	transaction->id = trans_id++;
> +	transaction->opcode = opcode;
> +
> +	return transaction;
>  }
> 
>  static void read_cb(uint16_t handle, uint16_t offset, uint8_t att_opcode,
>  					bdaddr_t *bdaddr, void *user_data)
>  {
> +	struct pending_trans_data *transaction;
>  	struct hal_ev_gatt_server_request_read ev;
>  	struct gatt_app *app;
>  	struct app_connection *conn;
>  	int32_t id = PTR_TO_INT(user_data);
> -	static int32_t trans_id = 1;
> 
>  	app = find_app_by_id(id);
>  	if (!app) {
> @@ -4203,14 +4217,16 @@ static void read_cb(uint16_t handle, uint16_t
> offset, uint8_t att_opcode, memset(&ev, 0, sizeof(ev));
> 
>  	/* Store the request data, complete callback and transaction id */
> -	set_trans_id(app, trans_id++, att_opcode);
> +	transaction = conn_add_transact(conn, att_opcode);
> +	if (!transaction)
> +		goto failed;
> 
>  	bdaddr2android(bdaddr, ev.bdaddr);
>  	ev.conn_id = conn->id;
>  	ev.attr_handle = handle;
>  	ev.offset = offset;
>  	ev.is_long = att_opcode == ATT_OP_READ_BLOB_REQ;
> -	ev.trans_id = app->trans_id.id;
> +	ev.trans_id = transaction->id;
> 
>  	ipc_send_notif(hal_ipc, HAL_SERVICE_ID_GATT,
>  					HAL_EV_GATT_SERVER_REQUEST_READ,
> @@ -4230,9 +4246,9 @@ static void write_cb(uint16_t handle, uint16_t offset,
> {
>  	uint8_t buf[IPC_MTU];
>  	struct hal_ev_gatt_server_request_write *ev = (void *) buf;
> +	struct pending_trans_data *transaction;
>  	struct gatt_app *app;
>  	int32_t id = PTR_TO_INT(user_data);
> -	static int32_t trans_id = 1;
>  	struct app_connection *conn;
> 
>  	app = find_app_by_id(id);
> @@ -4248,7 +4264,9 @@ static void write_cb(uint16_t handle, uint16_t offset,
> }
> 
>  	/* Store the request data, complete callback and transaction id */
> -	set_trans_id(app, trans_id++, att_opcode);
> +	transaction = conn_add_transact(conn, att_opcode);
> +	if (!transaction)
> +		goto failed;
> 
>  	/* TODO figure it out */
>  	if (att_opcode == ATT_OP_EXEC_WRITE_REQ)
> @@ -4261,7 +4279,7 @@ static void write_cb(uint16_t handle, uint16_t offset,
> ev->offset = offset;
> 
>  	ev->conn_id = conn->id;
> -	ev->trans_id = app->trans_id.id;
> +	ev->trans_id = transaction->id;
> 
>  	ev->is_prep = att_opcode == ATT_OP_PREP_WRITE_REQ;
>  	ev->need_rsp = att_opcode == ATT_OP_WRITE_REQ;
> @@ -4555,11 +4573,18 @@ reply:
>  				HAL_OP_GATT_SERVER_SEND_INDICATION, status);
>  }
> 
> +static bool match_trans_id(const void *data, const void *user_data)
> +{
> +	const struct pending_trans_data *transaction = data;
> +
> +	return transaction->id == PTR_TO_UINT(user_data);
> +}
> +
>  static void handle_server_send_response(const void *buf, uint16_t len)
>  {
>  	const struct hal_cmd_gatt_server_send_response *cmd = buf;
> +	struct pending_trans_data *transaction;
>  	struct app_connection *conn;
> -	struct gatt_app *app;
>  	uint8_t status;
> 
>  	DBG("");
> @@ -4571,22 +4596,20 @@ static void handle_server_send_response(const void
> *buf, uint16_t len) goto reply;
>  	}
> 
> -	app = conn->app;
> -
> -	if ((unsigned int)cmd->trans_id != app->trans_id.id) {
> -		error("gatt: transaction ID mismatch (%d!=%d)",
> -					cmd->trans_id, app->trans_id.id);
> -
> +	transaction = queue_remove_if(conn->transactions, match_trans_id,
> +						UINT_TO_PTR(cmd->trans_id));
> +	if (!transaction) {
> +		error("gatt: transaction ID = %d not found", cmd->trans_id);
>  		status = HAL_STATUS_FAILED;
>  		goto reply;
>  	}
> 
> -	send_gatt_response(conn->app->trans_id.opcode, cmd->handle, cmd->offset,
> +	send_gatt_response(transaction->opcode, cmd->handle, cmd->offset,
>  					cmd->status, cmd->len, cmd->data,
>  					&conn->device->bdaddr);
> 
>  	/* Clean request data */
> -	clear_trans_id(app);
> +	free(transaction);
> 
>  	status = HAL_STATUS_SUCCESS;

All patches applied, thanks.

-- 
BR
Szymon Janc