[PATCH 1/3] android/gatt: Fix scan handling

[PATCH 1/3] android/gatt: Fix scan handling

[Lukasz Rymanowski]

Android has its own ScanQueue which is used for tracking scanning
clients which means we do not have to have this logic internally.

Android will call Scan Off only when his scanQueue is empty.
---
 android/gatt.c | 62 ++++++++++++++++++----------------------------------------
 1 file changed, 19 insertions(+), 43 deletions(-)

diff --git a/android/gatt.c b/android/gatt.c
index 0dfb90e..775adec 100644
--- a/android/gatt.c
+++ b/android/gatt.c
@@ -71,9 +71,9 @@ struct gatt_device {
 
 static struct ipc *hal_ipc = NULL;
 static bdaddr_t adapter_addr;
+static bool scanning = false;
 
 static struct queue *gatt_clients = NULL;
-static struct queue *scan_clients = NULL;
 static struct queue *conn_list	= NULL;		/* Connected devices */
 static struct queue *conn_wait_queue = NULL;	/* Devs waiting to connect */
 
@@ -192,13 +192,6 @@ static void handle_client_unregister(const void *buf, uint16_t len)
 		goto failed;
 	}
 
-	queue_remove_if(scan_clients, match_by_value,
-						INT_TO_PTR(cmd->client_if));
-
-	/* If there is no client interesting in scan, just stop it */
-	if (queue_isempty(scan_clients))
-		bt_le_discovery_stop(NULL);
-
 	free(cl);
 	status = HAL_STATUS_SUCCESS;
 
@@ -350,7 +343,7 @@ static void le_device_found_handler(bdaddr_t *addr, uint8_t addr_type,
 	struct hal_ev_gatt_client_scan_result *ev = (void *) buf;
 	char bda[18];
 
-	if (queue_isempty(scan_clients))
+	if (!scanning)
 		goto connect;
 
 	ba2str(addr, bda);
@@ -541,51 +534,43 @@ static void handle_client_scan(const void *buf, uint16_t len)
 	const struct hal_cmd_gatt_client_scan *cmd = buf;
 	uint8_t status;
 	void *registered;
-	void *l;
 
 	DBG("new state %d", cmd->start);
 
 	registered = queue_find(gatt_clients, match_client_by_id,
 						INT_TO_PTR(cmd->client_if));
+	if (!registered) {
+		error("gatt: Client not registered");
+		status = HAL_STATUS_FAILED;
+		goto reply;
+	}
+
 	/* Turn off scan */
 	if (!cmd->start) {
-		if (registered)
-			queue_remove_if(scan_clients, match_by_value,
-						INT_TO_PTR(cmd->client_if));
+		DBG("Stopping LE SCAN");
 
-		if (queue_isempty(scan_clients)) {
-			DBG("Stopping LE SCAN");
+		if (scanning) {
 			bt_le_discovery_stop(NULL);
+			scanning = false;
 		}
 
 		status = HAL_STATUS_SUCCESS;
 		goto reply;
 	}
 
-	/* If device already do scan, reply with success and avoid to add it
-	 * again to the list
-	 */
-	l = queue_find(scan_clients, match_by_value,
-						INT_TO_PTR(cmd->client_if));
-	if (l) {
+	/* Reply success if we already do scan */
+	if (scanning) {
 		status = HAL_STATUS_SUCCESS;
 		goto reply;
 	}
 
+	/* Turn on scan */
 	if (!bt_le_discovery_start(le_device_found_handler)) {
 		error("gatt: LE scan switch failed");
 		status = HAL_STATUS_FAILED;
 		goto reply;
 	}
-
-	/* Add scan client to the list if its registered */
-	if (registered && !queue_push_tail(scan_clients,
-						INT_TO_PTR(cmd->client_if))) {
-		error("gatt: Cannot push scan client");
-		status = HAL_STATUS_FAILED;
-		goto reply;
-	}
-
+	scanning = true;
 	status = HAL_STATUS_SUCCESS;
 
 reply:
@@ -724,12 +709,10 @@ static void handle_client_connect(const void *buf, uint16_t len)
 	}
 
 	/* Start le scan if not started */
-	if (queue_isempty(scan_clients)) {
-		if (!bt_le_discovery_start(le_device_found_handler)) {
-			error("gatt: Could not start scan");
-			status = HAL_STATUS_FAILED;
-			goto reply;
-		}
+	if (!scanning && !bt_le_discovery_start(le_device_found_handler)) {
+		error("gatt: Could not start scan");
+		status = HAL_STATUS_FAILED;
+		goto reply;
 	}
 
 	if (!queue_push_tail(conn_wait_queue, dev)) {
@@ -1226,13 +1209,6 @@ bool bt_gatt_register(struct ipc *ipc, const bdaddr_t *addr)
 		return false;
 	}
 
-	scan_clients = queue_new();
-	if (!scan_clients) {
-		error("gatt: Cannot allocate scan_clients");
-		queue_destroy(gatt_clients, NULL);
-		return false;
-	}
-
 	return true;
 }
 
-- 
1.8.4

[PATCH 2/3] android/gatt: Restart scan after connection

[Lukasz Rymanowski]

With this patch we make sure that scan is restarted if it was holded for
connection purpose.
---
 android/gatt.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/android/gatt.c b/android/gatt.c
index 775adec..5384a72 100644
--- a/android/gatt.c
+++ b/android/gatt.c
@@ -481,6 +481,12 @@ reply:
 	/* If connection did not succeed, destroy device */
 	if (status)
 		destroy_device(dev);
+
+	/* Check if we should restart scan */
+	if (scanning)
+		bt_le_discovery_start(le_device_found_handler);
+
+	/*FIXME: What to do if discovery won't start here. */
 }
 
 static int connect_le(struct gatt_device *dev)
-- 
1.8.4

[PATCH 3/3] android/gatt: Add info debug to connect function

[Lukasz Rymanowski]

Because we handle connect always in "auto connect" mode we want this
info log to give us idea how BLE applications use that.

Also add comment about we handle is_direct in this function.
---
 android/gatt.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/android/gatt.c b/android/gatt.c
index 5384a72..705a441 100644
--- a/android/gatt.c
+++ b/android/gatt.c
@@ -636,8 +636,15 @@ static void handle_client_connect(const void *buf, uint16_t len)
 	bdaddr_t addr;
 	uint8_t status;
 	bool send_notify = false;
+	char a[18];
 
-	DBG("");
+	/* For now we handle direct connect in the same way as auto.
+	 * connect. This is to avoid issues with broken applications which
+	 * might block HCI by calling connect to device not in range. However
+	 * we can consider later to change that.
+	*/
+	ba2str((bdaddr_t *)&cmd->bdaddr, a);
+	info("gatt: Connect to: %s(is_direct=%d)", a, cmd->is_direct);
 
 	/* Check if client is registered */
 	l = queue_find(gatt_clients, match_client_by_id,
-- 
1.8.4

Re: [PATCH 1/3] android/gatt: Fix scan handling

[Szymon Janc]

Hi Łukasz,

On Sunday 23 of March 2014 21:20:08 Lukasz Rymanowski wrote:
> Android has its own ScanQueue which is used for tracking scanning
> clients which means we do not have to have this logic internally.
> 
> Android will call Scan Off only when his scanQueue is empty.
> ---
>  android/gatt.c | 62 ++++++++++++++++++----------------------------------------
>  1 file changed, 19 insertions(+), 43 deletions(-)
> 
> diff --git a/android/gatt.c b/android/gatt.c
> index 0dfb90e..775adec 100644
> --- a/android/gatt.c
> +++ b/android/gatt.c
> @@ -71,9 +71,9 @@ struct gatt_device {
>  
>  static struct ipc *hal_ipc = NULL;
>  static bdaddr_t adapter_addr;
> +static bool scanning = false;
>  
>  static struct queue *gatt_clients = NULL;
> -static struct queue *scan_clients = NULL;
>  static struct queue *conn_list	= NULL;		/* Connected devices */
>  static struct queue *conn_wait_queue = NULL;	/* Devs waiting to connect */
>  
> @@ -192,13 +192,6 @@ static void handle_client_unregister(const void *buf, uint16_t len)
>  		goto failed;
>  	}
>  
> -	queue_remove_if(scan_clients, match_by_value,
> -						INT_TO_PTR(cmd->client_if));
> -
> -	/* If there is no client interesting in scan, just stop it */
> -	if (queue_isempty(scan_clients))
> -		bt_le_discovery_stop(NULL);
> -
>  	free(cl);
>  	status = HAL_STATUS_SUCCESS;
>  
> @@ -350,7 +343,7 @@ static void le_device_found_handler(bdaddr_t *addr, uint8_t addr_type,
>  	struct hal_ev_gatt_client_scan_result *ev = (void *) buf;
>  	char bda[18];
>  
> -	if (queue_isempty(scan_clients))
> +	if (!scanning)
>  		goto connect;
>  
>  	ba2str(addr, bda);
> @@ -541,51 +534,43 @@ static void handle_client_scan(const void *buf, uint16_t len)
>  	const struct hal_cmd_gatt_client_scan *cmd = buf;
>  	uint8_t status;
>  	void *registered;
> -	void *l;
>  
>  	DBG("new state %d", cmd->start);
>  
>  	registered = queue_find(gatt_clients, match_client_by_id,
>  						INT_TO_PTR(cmd->client_if));
> +	if (!registered) {
> +		error("gatt: Client not registered");
> +		status = HAL_STATUS_FAILED;
> +		goto reply;
> +	}
> +
>  	/* Turn off scan */
>  	if (!cmd->start) {
> -		if (registered)
> -			queue_remove_if(scan_clients, match_by_value,
> -						INT_TO_PTR(cmd->client_if));
> +		DBG("Stopping LE SCAN");
>  
> -		if (queue_isempty(scan_clients)) {
> -			DBG("Stopping LE SCAN");
> +		if (scanning) {
>  			bt_le_discovery_stop(NULL);
> +			scanning = false;
>  		}
>  
>  		status = HAL_STATUS_SUCCESS;
>  		goto reply;
>  	}
>  
> -	/* If device already do scan, reply with success and avoid to add it
> -	 * again to the list
> -	 */
> -	l = queue_find(scan_clients, match_by_value,
> -						INT_TO_PTR(cmd->client_if));
> -	if (l) {
> +	/* Reply success if we already do scan */
> +	if (scanning) {
>  		status = HAL_STATUS_SUCCESS;
>  		goto reply;
>  	}
>  
> +	/* Turn on scan */
>  	if (!bt_le_discovery_start(le_device_found_handler)) {
>  		error("gatt: LE scan switch failed");
>  		status = HAL_STATUS_FAILED;
>  		goto reply;
>  	}
> -
> -	/* Add scan client to the list if its registered */
> -	if (registered && !queue_push_tail(scan_clients,
> -						INT_TO_PTR(cmd->client_if))) {
> -		error("gatt: Cannot push scan client");
> -		status = HAL_STATUS_FAILED;
> -		goto reply;
> -	}
> -
> +	scanning = true;
>  	status = HAL_STATUS_SUCCESS;
>  
>  reply:
> @@ -724,12 +709,10 @@ static void handle_client_connect(const void *buf, uint16_t len)
>  	}
>  
>  	/* Start le scan if not started */
> -	if (queue_isempty(scan_clients)) {
> -		if (!bt_le_discovery_start(le_device_found_handler)) {
> -			error("gatt: Could not start scan");
> -			status = HAL_STATUS_FAILED;
> -			goto reply;
> -		}
> +	if (!scanning && !bt_le_discovery_start(le_device_found_handler)) {
> +		error("gatt: Could not start scan");
> +		status = HAL_STATUS_FAILED;
> +		goto reply;
>  	}
>  
>  	if (!queue_push_tail(conn_wait_queue, dev)) {
> @@ -1226,13 +1209,6 @@ bool bt_gatt_register(struct ipc *ipc, const bdaddr_t *addr)
>  		return false;
>  	}
>  
> -	scan_clients = queue_new();
> -	if (!scan_clients) {
> -		error("gatt: Cannot allocate scan_clients");
> -		queue_destroy(gatt_clients, NULL);
> -		return false;
> -	}
> -
>  	return true;
>  }
>  
> 

As discussed offline, patches 1 and 2 are now pushed. Thanks.

Also please remember to put that info into document describing gatt HAL
when one will be created.

-- 
Best regards, 
Szymon Janc