[PATCH 1/6] android/gatt: Remove redundant search

[PATCH 1/6] android/gatt: Remove redundant search

[Jakub Tyszkowski]

Searching for device is not needed since we have reference counting and
device is there as long as app connection exist.
---
 android/gatt.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/android/gatt.c b/android/gatt.c
index 4398194..425d3e8 100644
--- a/android/gatt.c
+++ b/android/gatt.c
@@ -914,14 +914,10 @@ static void destroy_connection(void *data)
 	if (conn->timeout_id > 0)
 		g_source_remove(conn->timeout_id);
 
-	if (!queue_find(gatt_devices, match_by_value, conn->device))
-		goto cleanup;
-
 	conn->device->conn_cnt--;
 	if (conn->device->conn_cnt == 0)
 		connection_cleanup(conn->device);
 
-cleanup:
 	queue_destroy(conn->transactions, free);
 	device_unref(conn->device);
 	free(conn);
-- 
1.9.1

[PATCH 2/6] android/gatt: Remove REQUEST_INIT state

[Jakub Tyszkowski]

This state is redundant as it is just yet another pending state for which
we are not even testing anywhere.
---
 android/gatt.c | 9 ---------
 1 file changed, 9 deletions(-)

diff --git a/android/gatt.c b/android/gatt.c
index 425d3e8..210a877 100644
--- a/android/gatt.c
+++ b/android/gatt.c
@@ -692,7 +692,6 @@ static void destroy_gatt_app(void *data)
 }
 
 enum pend_req_state {
-	REQUEST_INIT,
 	REQUEST_PENDING,
 	REQUEST_DONE,
 };
@@ -4881,8 +4880,6 @@ static void read_requested_attributes(void *data, void *user_data)
 		return;
 	}
 
-	resp_data->state = REQUEST_PENDING;
-
 	gatt_db_attribute_read(attrib, resp_data->offset, process_data->opcode,
 						&process_data->device->bdaddr,
 						attribute_read_cb, resp_data);
@@ -6097,7 +6094,6 @@ static uint8_t read_by_type(const uint8_t *cmd, uint16_t cmd_len,
 			return ATT_ECODE_INSUFF_RESOURCES;
 		}
 
-		data->state = REQUEST_INIT;
 		data->attrib = attrib;
 		if (!queue_push_tail(device->pending_requests, data)) {
 			free(data);
@@ -6150,7 +6146,6 @@ static uint8_t read_request(const uint8_t *cmd, uint16_t cmd_len,
 
 	data->offset = offset;
 	data->attrib = attrib;
-	data->state = REQUEST_INIT;
 	if (!queue_push_tail(dev->pending_requests, data)) {
 		free(data);
 		return ATT_ECODE_INSUFF_RESOURCES;
@@ -6325,7 +6320,6 @@ static void find_by_type_request_cb(struct gatt_db_attribute *attrib,
 		return;
 	}
 
-	request_data->state = REQUEST_INIT;
 	request_data->attrib = attrib;
 	request_data->filter_vlen = find_data->search_vlen;
 	memcpy(request_data->filter_value, find_data->search_value,
@@ -6538,7 +6532,6 @@ static uint8_t write_req_request(const uint8_t *cmd, uint16_t cmd_len,
 		return ATT_ECODE_INSUFF_RESOURCES;
 
 	data->attrib = attrib;
-	data->state = REQUEST_PENDING;
 
 	if (!queue_push_tail(dev->pending_requests, data)) {
 		free(data);
@@ -6595,7 +6588,6 @@ static uint8_t write_prep_request(const uint8_t *cmd, uint16_t cmd_len,
 
 	data->attrib = attrib;
 	data->offset = offset;
-	data->state = REQUEST_PENDING;
 
 	if (!queue_push_tail(dev->pending_requests, data)) {
 		free(data);
@@ -6668,7 +6660,6 @@ static uint8_t write_execute_request(const uint8_t *cmd, uint16_t cmd_len,
 	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;
-- 
1.9.1

[PATCH 3/6] android/gatt: Minor debug message change

[Jakub Tyszkowski]

It's easier to have opcode in hex.
---
 android/gatt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/android/gatt.c b/android/gatt.c
index 210a877..0a6e6e9 100644
--- a/android/gatt.c
+++ b/android/gatt.c
@@ -4748,7 +4748,7 @@ static uint8_t check_device_permissions(struct gatt_device *device,
 							BT_IO_OPT_INVALID))
 		return ATT_ECODE_UNLIKELY;
 
-	DBG("opcode %u permissions %u sec_level %u", opcode, permissions,
+	DBG("opcode 0x%02x permissions %u sec_level %u", opcode, permissions,
 								sec_level);
 
 	switch (opcode) {
-- 
1.9.1

[PATCH 4/6] android/gatt: Minor function renames

[Jakub Tyszkowski]

App connection notification functions names were not very descriptive
and it wasn't clear why for server and client notification sending
'status' function parameter was modified. With this patch, 'state' valid
for server and 'connection/disconnection status' valid for client are
distinguished.

Some functions also had 'send' prefix despite not calling 'ipc_send_'
directly like others do.

Declared type in 'struct app_connection *connection' function parameter
tells enough, thus parameter name was shortened to 'conn' in favor of
consistency and more descriptive function naming.
---
 android/gatt.c | 114 ++++++++++++++++++++++++++++-----------------------------
 1 file changed, 57 insertions(+), 57 deletions(-)

diff --git a/android/gatt.c b/android/gatt.c
index 0a6e6e9..7b924af 100644
--- a/android/gatt.c
+++ b/android/gatt.c
@@ -795,97 +795,97 @@ static struct gatt_device *create_device(const bdaddr_t *addr)
 	return device_ref(dev);
 }
 
-static void send_client_connection_notify(struct app_connection *connection,
+static void send_client_connect_status_notify(struct app_connection *conn,
 								int32_t status)
 {
 	struct hal_ev_gatt_client_connect ev;
 
-	if (connection->app->func) {
-		connection->app->func(&connection->device->bdaddr,
+	if (conn->app->func) {
+		conn->app->func(&conn->device->bdaddr,
 					status == GATT_SUCCESS ? 0 : -ENOTCONN,
-					connection->device->attrib);
+					conn->device->attrib);
 		return;
 	}
 
-	ev.client_if = connection->app->id;
-	ev.conn_id = connection->id;
+	ev.client_if = conn->app->id;
+	ev.conn_id = conn->id;
 	ev.status = status;
 
-	bdaddr2android(&connection->device->bdaddr, &ev.bda);
+	bdaddr2android(&conn->device->bdaddr, &ev.bda);
 
 	ipc_send_notif(hal_ipc, HAL_SERVICE_ID_GATT, HAL_EV_GATT_CLIENT_CONNECT,
 							sizeof(ev), &ev);
 }
 
-static void send_server_connection_notify(struct app_connection *connection,
+static void send_server_connection_state_notify(struct app_connection *conn,
 								bool connected)
 {
 	struct hal_ev_gatt_server_connection ev;
 
-	if (connection->app->func) {
-		connection->app->func(&connection->device->bdaddr,
+	if (conn->app->func) {
+		conn->app->func(&conn->device->bdaddr,
 					connected ? 0 : -ENOTCONN,
-					connection->device->attrib);
+					conn->device->attrib);
 		return;
 	}
 
-	ev.server_if = connection->app->id;
-	ev.conn_id = connection->id;
+	ev.server_if = conn->app->id;
+	ev.conn_id = conn->id;
 	ev.connected = connected;
 
-	bdaddr2android(&connection->device->bdaddr, &ev.bdaddr);
+	bdaddr2android(&conn->device->bdaddr, &ev.bdaddr);
 
 	ipc_send_notif(hal_ipc, HAL_SERVICE_ID_GATT,
 				HAL_EV_GATT_SERVER_CONNECTION, sizeof(ev), &ev);
 }
 
-static void send_client_disconnection_notify(struct app_connection *connection,
+static void send_client_disconnect_status_notify(struct app_connection *conn,
 								int32_t status)
 {
 	struct hal_ev_gatt_client_disconnect ev;
 
-	if (connection->app->func) {
-		connection->app->func(&connection->device->bdaddr, -ENOTCONN,
-						connection->device->attrib);
+	if (conn->app->func) {
+		conn->app->func(&conn->device->bdaddr, -ENOTCONN,
+						conn->device->attrib);
 		return;
 	}
 
-	ev.client_if = connection->app->id;
-	ev.conn_id = connection->id;
+	ev.client_if = conn->app->id;
+	ev.conn_id = conn->id;
 	ev.status = status;
 
-	bdaddr2android(&connection->device->bdaddr, &ev.bda);
+	bdaddr2android(&conn->device->bdaddr, &ev.bda);
 
 	ipc_send_notif(hal_ipc, HAL_SERVICE_ID_GATT,
 				HAL_EV_GATT_CLIENT_DISCONNECT, sizeof(ev), &ev);
 
 }
 
-static void send_app_disconnect_notify(struct app_connection *connection,
+static void notify_app_disconnect_status(struct app_connection *conn,
 								int32_t status)
 {
-	if (!connection->app)
+	if (!conn->app)
 		return;
 
-	if (connection->app->type == GATT_CLIENT)
-		send_client_disconnection_notify(connection, status);
+	if (conn->app->type == GATT_CLIENT)
+		send_client_disconnect_status_notify(conn, status);
 	else
-		send_server_connection_notify(connection, !!status);
+		send_server_connection_state_notify(conn, !!status);
 }
 
-static void send_app_connect_notify(struct app_connection *connection,
+static void notify_app_connect_status(struct app_connection *conn,
 								int32_t status)
 {
-	if (!connection->app)
+	if (!conn->app)
 		return;
 
-	if (connection->app->type == GATT_CLIENT)
-		send_client_connection_notify(connection, status);
-	else if (connection->app->type == GATT_SERVER)
-		send_server_connection_notify(connection, !status);
+	if (conn->app->type == GATT_CLIENT)
+		send_client_connect_status_notify(conn, status);
+	else if (conn->app->type == GATT_SERVER)
+		send_server_connection_state_notify(conn, !status);
 }
 
-static void disconnect_notify_by_device(void *data, void *user_data)
+static void notify_app_disconnect_status_by_device(void *data, void *user_data)
 {
 	struct app_connection *conn = data;
 	struct gatt_device *dev = user_data;
@@ -895,11 +895,11 @@ static void disconnect_notify_by_device(void *data, void *user_data)
 
 	switch (dev->state) {
 	case DEVICE_CONNECTED:
-		send_app_disconnect_notify(conn, GATT_SUCCESS);
+		notify_app_disconnect_status(conn, GATT_SUCCESS);
 		break;
 	case DEVICE_CONNECT_INIT:
 	case DEVICE_CONNECT_READY:
-		send_app_connect_notify(conn, GATT_FAILURE);
+		notify_app_connect_status(conn, GATT_FAILURE);
 		break;
 	case DEVICE_DISCONNECTED:
 		break;
@@ -925,7 +925,8 @@ static void destroy_connection(void *data)
 static void device_disconnect_clients(struct gatt_device *dev)
 {
 	/* Notify disconnection to all clients */
-	queue_foreach(app_connections, disconnect_notify_by_device, dev);
+	queue_foreach(app_connections, notify_app_disconnect_status_by_device,
+									dev);
 
 	/* Remove all clients by given device's */
 	queue_remove_all(app_connections, match_connection_by_device, dev,
@@ -1416,13 +1417,13 @@ struct connect_data {
 	int32_t status;
 };
 
-static void send_app_connect_notifications(void *data, void *user_data)
+static void notify_app_connect_status_by_device(void *data, void *user_data)
 {
 	struct app_connection *conn = data;
 	struct connect_data *con_data = user_data;
 
 	if (conn->device == con_data->dev)
-		send_app_connect_notify(conn, con_data->status);
+		notify_app_connect_status(conn, con_data->status);
 }
 
 static struct app_connection *find_conn_without_app(struct gatt_device *dev)
@@ -1634,7 +1635,8 @@ reply:
 
 	data.dev = dev;
 	data.status = status;
-	queue_foreach(app_connections, send_app_connect_notifications, &data);
+	queue_foreach(app_connections, notify_app_connect_status_by_device,
+									&data);
 	device_unref(dev);
 
 	/* Check if we should restart scan */
@@ -1909,7 +1911,7 @@ static void trigger_disconnection(struct app_connection *connection)
 {
 	/* Notify client */
 	if (queue_remove(app_connections, connection))
-			send_app_disconnect_notify(connection, GATT_SUCCESS);
+			notify_app_disconnect_status(connection, GATT_SUCCESS);
 
 	destroy_connection(connection);
 }
@@ -1972,28 +1974,27 @@ static int connect_bredr(struct gatt_device *dev)
 	return 0;
 }
 
-static bool trigger_connection(struct app_connection *connection)
+static bool trigger_connection(struct app_connection *conn)
 {
 	bool ret;
 
-	switch (connection->device->state) {
+	switch (conn->device->state) {
 	case DEVICE_DISCONNECTED:
 		/*
 		 *  If device was last seen over BR/EDR connect over it.
 		 *  Note: Connection state is handled in connect_bredr() func
 		 */
-		if (bt_device_last_seen_bearer(&connection->device->bdaddr) ==
+		if (bt_device_last_seen_bearer(&conn->device->bdaddr) ==
 								BDADDR_BREDR)
-			return connect_bredr(connection->device) == 0;
+			return connect_bredr(conn->device) == 0;
 
 		/* For LE use auto connect feature */
-		ret = auto_connect_le(connection->device);
+		ret = auto_connect_le(conn->device);
 		if (ret)
-			device_set_state(connection->device,
-							DEVICE_CONNECT_INIT);
+			device_set_state(conn->device, DEVICE_CONNECT_INIT);
 		break;
 	case DEVICE_CONNECTED:
-		send_app_connect_notify(connection, GATT_SUCCESS);
+		notify_app_connect_status(conn, GATT_SUCCESS);
 		ret = true;
 		break;
 	case DEVICE_CONNECT_READY:
@@ -2544,7 +2545,7 @@ failed:
 	send_client_incl_service_notify(&service->id, incl, conn->id);
 }
 
-static bool search_included_services(struct app_connection *connection,
+static bool search_included_services(struct app_connection *conn,
 							struct service *service)
 {
 	struct get_included_data *data;
@@ -2557,7 +2558,7 @@ static bool search_included_services(struct app_connection *connection,
 	}
 
 	data->prim = service;
-	data->conn = connection;
+	data->conn = conn;
 
 	if (service->primary) {
 		start = service->prim.range.start;
@@ -2567,8 +2568,8 @@ static bool search_included_services(struct app_connection *connection,
 		end = service->incl.range.end;
 	}
 
-	gatt_find_included(connection->device->attrib, start, end,
-							get_included_cb, data);
+	gatt_find_included(conn->device->attrib, start, end, get_included_cb,
+									data);
 
 	return true;
 }
@@ -2925,9 +2926,8 @@ reply:
 	free(data);
 }
 
-static bool build_descr_cache(struct app_connection *connection,
-					struct service *srvc,
-					struct characteristic *ch)
+static bool build_descr_cache(struct app_connection *conn, struct service *srvc,
+						struct characteristic *ch)
 {
 	struct discover_desc_data *cb_data;
 	uint16_t start, end;
@@ -2944,11 +2944,11 @@ static bool build_descr_cache(struct app_connection *connection,
 	if (!cb_data)
 		return false;
 
-	cb_data->conn = connection;
+	cb_data->conn = conn;
 	cb_data->srvc = srvc;
 	cb_data->ch = ch;
 
-	if (!gatt_discover_desc(connection->device->attrib, start, end, NULL,
+	if (!gatt_discover_desc(conn->device->attrib, start, end, NULL,
 					gatt_discover_desc_cb, cb_data)) {
 		free(cb_data);
 		return false;
-- 
1.9.1

[PATCH 5/6] android/gatt: Remove reduntant comparison

[Jakub Tyszkowski]

This is not needed as we only have one type or the other.
---
 android/gatt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/android/gatt.c b/android/gatt.c
index 7b924af..9be750a 100644
--- a/android/gatt.c
+++ b/android/gatt.c
@@ -881,7 +881,7 @@ static void notify_app_connect_status(struct app_connection *conn,
 
 	if (conn->app->type == GATT_CLIENT)
 		send_client_connect_status_notify(conn, status);
-	else if (conn->app->type == GATT_SERVER)
+	else
 		send_server_connection_state_notify(conn, !status);
 }
 
-- 
1.9.1

[PATCH 6/6] android/gatt: Remove device's connection counter

[Jakub Tyszkowski]

Originally we used connection counter to determine if device object
is still needed or should be destroyed but we switched to reference
counting quite some time ago and this counter is not needed anymore.
---
 android/gatt.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/android/gatt.c b/android/gatt.c
index 9be750a..cda38ea 100644
--- a/android/gatt.c
+++ b/android/gatt.c
@@ -166,7 +166,6 @@ struct gatt_device {
 	guint ind_id;
 
 	int ref;
-	int conn_cnt;
 
 	struct queue *autoconnect_apps;
 
@@ -913,8 +912,8 @@ static void destroy_connection(void *data)
 	if (conn->timeout_id > 0)
 		g_source_remove(conn->timeout_id);
 
-	conn->device->conn_cnt--;
-	if (conn->device->conn_cnt == 0)
+	if (!queue_find(app_connections, match_connection_by_device,
+							conn->device))
 		connection_cleanup(conn->device);
 
 	queue_destroy(conn->transactions, free);
@@ -1115,7 +1114,6 @@ static struct app_connection *create_connection(struct gatt_device *device,
 	}
 
 	new_conn->device = device_ref(device);
-	new_conn->device->conn_cnt++;
 
 	return new_conn;
 }
@@ -1605,7 +1603,7 @@ reply:
 	 */
 	queue_foreach(dev->autoconnect_apps, create_app_connection, dev);
 
-	if (!dev->conn_cnt) {
+	if (!queue_find(app_connections, match_connection_by_device, dev)) {
 		struct app_connection *conn;
 
 		if (!dev->attrib)
-- 
1.9.1

Re: [PATCH 1/6] android/gatt: Remove redundant search

[Szymon Janc]

Hi Jakub,

On Tuesday 10 of February 2015 13:48:09 Jakub Tyszkowski wrote:
> Searching for device is not needed since we have reference counting and
> device is there as long as app connection exist.
> ---
>  android/gatt.c | 4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/android/gatt.c b/android/gatt.c
> index 4398194..425d3e8 100644
> --- a/android/gatt.c
> +++ b/android/gatt.c
> @@ -914,14 +914,10 @@ static void destroy_connection(void *data)
>  	if (conn->timeout_id > 0)
>  		g_source_remove(conn->timeout_id);
>  
> -	if (!queue_find(gatt_devices, match_by_value, conn->device))
> -		goto cleanup;
> -
>  	conn->device->conn_cnt--;
>  	if (conn->device->conn_cnt == 0)
>  		connection_cleanup(conn->device);
>  
> -cleanup:
>  	queue_destroy(conn->transactions, free);
>  	device_unref(conn->device);
>  	free(conn);
> 

All patches applied, thanks.

-- 
Best regards, 
Szymon Janc