[PATCH v2 0/5]android/gatt: GATT client improvements

[PATCH v2 0/5]android/gatt: GATT client improvements

[Lukasz Rymanowski]

This patch sets adds:
* preparation for client listen handling
* refactor send_client_connect_notify function
* fix scenario when application unregister client without proper cleaning

v2:
* removed listen from this patch set. Will send it later.
* cleaning of other patches


Lukasz Rymanowski (5):
  android/gatt: Refactor send_client_connect_notify
  android/gatt: Fix handling client unregister
  android/gatt: Move functions up in the file
  android/gatt: Cleanup device lists
  android/gatt: Fix type of status parameter

 android/gatt.c | 209 ++++++++++++++++++++++++++++++++++++++++-----------------
 1 file changed, 146 insertions(+), 63 deletions(-)

-- 
1.8.4

[PATCH v2 1/5] android/gatt: Refactor send_client_connect_notify

[Lukasz Rymanowski]

Create helper function to send connect notification, similar to the
send_client_disconnect_notify
---
 android/gatt.c | 55 ++++++++++++++++++++++++++++++++++---------------------
 1 file changed, 34 insertions(+), 21 deletions(-)

diff --git a/android/gatt.c b/android/gatt.c
index a33ce25..80fa139 100644
--- a/android/gatt.c
+++ b/android/gatt.c
@@ -480,6 +480,24 @@ failed:
 					HAL_OP_GATT_CLIENT_REGISTER, status);
 }
 
+static void send_client_connect_notify(int32_t id, struct gatt_device *dev,
+								int32_t status)
+{
+	struct hal_ev_gatt_client_connect ev;
+
+	ev.client_if = id;
+	ev.status = status;
+
+	if (status == GATT_SUCCESS) {
+		/* Set address and client id in the event */
+		bdaddr2android(&dev->bdaddr, &ev.bda);
+		ev.conn_id = dev->conn_id;
+	}
+
+	ipc_send_notif(hal_ipc, HAL_SERVICE_ID_GATT,
+				HAL_EV_GATT_CLIENT_CONNECT, sizeof(ev), &ev);
+}
+
 static void handle_client_unregister(const void *buf, uint16_t len)
 {
 	const struct hal_cmd_gatt_client_unregister *cmd = buf;
@@ -785,25 +803,26 @@ done:
 	return FALSE;
 }
 
-static void send_client_connect_notify(void *data, void *user_data)
+struct connect_data {
+	struct gatt_device *dev;
+	int32_t status;
+};
+
+static void send_client_connect_notifications(void *data, void *user_data)
 {
-	struct hal_ev_gatt_client_connect *ev = user_data;
 	int32_t id = PTR_TO_INT(data);
+	struct connect_data *c = user_data;
 
-	ev->client_if = id;
-
-	ipc_send_notif(hal_ipc, HAL_SERVICE_ID_GATT,
-				HAL_EV_GATT_CLIENT_CONNECT, sizeof(*ev), ev);
+	send_client_connect_notify(id, c->dev, c->status);
 }
 
 static void connect_cb(GIOChannel *io, GError *gerr, gpointer user_data)
 {
 	bdaddr_t *addr = user_data;
 	struct gatt_device *dev;
-	struct hal_ev_gatt_client_connect ev;
+	struct connect_data data;
 	GAttrib *attrib;
 	static uint32_t conn_id = 0;
-	int32_t status;
 
 	/* Take device from conn waiting queue */
 	dev = queue_remove_if(conn_wait_queue, match_dev_by_bdaddr, addr);
@@ -816,19 +835,16 @@ static void connect_cb(GIOChannel *io, GError *gerr, gpointer user_data)
 	g_io_channel_unref(dev->att_io);
 	dev->att_io = NULL;
 
-	/* Set address and client id in the event */
-	bdaddr2android(&dev->bdaddr, &ev.bda);
-
 	if (gerr) {
 		error("gatt: connection failed %s", gerr->message);
-		status = GATT_FAILURE;
+		data.status = GATT_FAILURE;
 		goto reply;
 	}
 
 	attrib = g_attrib_new(io);
 	if (!attrib) {
 		error("gatt: unable to create new GAttrib instance");
-		status = GATT_FAILURE;
+		data.status = GATT_FAILURE;
 		goto reply;
 	}
 
@@ -841,21 +857,18 @@ static void connect_cb(GIOChannel *io, GError *gerr, gpointer user_data)
 	if (!queue_push_tail(conn_list, dev)) {
 		error("gatt: Cannot push dev on conn_list");
 		connection_cleanup(dev);
-		status = GATT_FAILURE;
+		data.status = GATT_FAILURE;
 		goto reply;
 	}
 
-	status = GATT_SUCCESS;
-	goto reply;
+	data.status = GATT_SUCCESS;
 
 reply:
-	ev.conn_id = dev ? dev->conn_id : 0;
-	ev.status = status;
-
-	queue_foreach(dev->clients, send_client_connect_notify, &ev);
+	data.dev = dev;
+	queue_foreach(dev->clients, send_client_connect_notifications, &data);
 
 	/* If connection did not succeed, destroy device */
-	if (status)
+	if (data.status)
 		destroy_device(dev);
 
 	/* Check if we should restart scan */
-- 
1.8.4

[PATCH v2 2/5] android/gatt: Fix handling client unregister

[Lukasz Rymanowski]

When client do unregister we need to make sure that there is no
connected device or outstanding connection request for this client.
---
 android/gatt.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/android/gatt.c b/android/gatt.c
index 80fa139..3483f5d 100644
--- a/android/gatt.c
+++ b/android/gatt.c
@@ -498,6 +498,28 @@ static void send_client_connect_notify(int32_t id, struct gatt_device *dev,
 				HAL_EV_GATT_CLIENT_CONNECT, sizeof(ev), &ev);
 }
 
+static void remove_client_from_device_list(void *data, void *user_data)
+{
+	struct gatt_device *dev = data;
+
+	queue_remove_if(dev->clients, match_by_value, user_data);
+}
+
+static void remove_client_from_devices(int32_t id)
+{
+	DBG("");
+
+	queue_foreach(conn_list, remove_client_from_device_list,
+							INT_TO_PTR(id));
+
+	/*TODO: Check if there is any zombie device (connected no client)*/
+
+	queue_foreach(conn_wait_queue, remove_client_from_device_list,
+							INT_TO_PTR(id));
+
+	/*TODO: Check if there is not zombie device plus stop scan */
+}
+
 static void handle_client_unregister(const void *buf, uint16_t len)
 {
 	const struct hal_cmd_gatt_client_unregister *cmd = buf;
@@ -514,6 +536,11 @@ static void handle_client_unregister(const void *buf, uint16_t len)
 		goto failed;
 	}
 
+	/* Check if there is any connect request or connected device for this
+	 * client. If so, remove this client from those lists.
+	 */
+	remove_client_from_devices(cl->id);
+
 	destroy_gatt_client(cl);
 	status = HAL_STATUS_SUCCESS;
 
-- 
1.8.4

[PATCH v2 3/5] android/gatt: Move functions up in the file

[Lukasz Rymanowski]

Move send_client_disconnect_notify connection_cleanup and
put_device_on_disc_list up in the file

This is needed by following patch
---
 android/gatt.c | 84 +++++++++++++++++++++++++++++-----------------------------
 1 file changed, 42 insertions(+), 42 deletions(-)

diff --git a/android/gatt.c b/android/gatt.c
index 3483f5d..6368317 100644
--- a/android/gatt.c
+++ b/android/gatt.c
@@ -480,6 +480,41 @@ failed:
 					HAL_OP_GATT_CLIENT_REGISTER, status);
 }
 
+static void connection_cleanup(struct gatt_device *device)
+{
+	if (device->watch_id) {
+		g_source_remove(device->watch_id);
+		device->watch_id = 0;
+	}
+
+	if (device->att_io) {
+		g_io_channel_shutdown(device->att_io, FALSE, NULL);
+		g_io_channel_unref(device->att_io);
+		device->att_io = NULL;
+	}
+
+	if (device->attrib) {
+		GAttrib *attrib = device->attrib;
+		device->attrib = NULL;
+		g_attrib_cancel_all(attrib);
+		g_attrib_unref(attrib);
+	}
+}
+
+static void send_client_disconnect_notify(int32_t id, struct gatt_device *dev,
+								uint8_t status)
+{
+	struct hal_ev_gatt_client_disconnect ev;
+
+	ev.client_if = id;
+	ev.conn_id = dev->conn_id;
+	ev.status = status;
+	bdaddr2android(&dev->bdaddr, &ev.bda);
+
+	ipc_send_notif(hal_ipc, HAL_SERVICE_ID_GATT,
+			HAL_EV_GATT_CLIENT_DISCONNECT, sizeof(ev), &ev);
+}
+
 static void send_client_connect_notify(int32_t id, struct gatt_device *dev,
 								int32_t status)
 {
@@ -505,6 +540,13 @@ static void remove_client_from_device_list(void *data, void *user_data)
 	queue_remove_if(dev->clients, match_by_value, user_data);
 }
 
+static void put_device_on_disc_list(struct gatt_device *dev)
+{
+	dev->conn_id = 0;
+	queue_remove_all(dev->clients, NULL, NULL, NULL);
+	queue_push_tail(disc_dev_list, dev);
+}
+
 static void remove_client_from_devices(int32_t id)
 {
 	DBG("");
@@ -684,41 +726,6 @@ done:
 	send_client_all_primary(gatt_status, dev->services, dev->conn_id);
 }
 
-static void connection_cleanup(struct gatt_device *device)
-{
-	if (device->watch_id) {
-		g_source_remove(device->watch_id);
-		device->watch_id = 0;
-	}
-
-	if (device->att_io) {
-		g_io_channel_shutdown(device->att_io, FALSE, NULL);
-		g_io_channel_unref(device->att_io);
-		device->att_io = NULL;
-	}
-
-	if (device->attrib) {
-		GAttrib *attrib = device->attrib;
-		device->attrib = NULL;
-		g_attrib_cancel_all(attrib);
-		g_attrib_unref(attrib);
-	}
-}
-
-static void send_client_disconnect_notify(int32_t id, struct gatt_device *dev,
-								uint8_t status)
-{
-	struct hal_ev_gatt_client_disconnect ev;
-
-	ev.client_if = id;
-	ev.conn_id = dev->conn_id;
-	ev.status = status;
-	bdaddr2android(&dev->bdaddr, &ev.bda);
-
-	ipc_send_notif(hal_ipc, HAL_SERVICE_ID_GATT,
-			HAL_EV_GATT_CLIENT_DISCONNECT, sizeof(ev), &ev);
-}
-
 static void client_disconnect_notify(void *data, void *user_data)
 {
 	struct gatt_device *dev = user_data;
@@ -782,13 +789,6 @@ connect:
 	bt_le_discovery_stop(bt_le_discovery_stop_cb);
 }
 
-static void put_device_on_disc_list(struct gatt_device *dev)
-{
-	dev->conn_id = 0;
-	queue_remove_all(dev->clients, NULL, NULL, NULL);
-	queue_push_tail(disc_dev_list, dev);
-}
-
 static gboolean disconnected_cb(GIOChannel *io, GIOCondition cond,
 							gpointer user_data)
 {
-- 
1.8.4

[PATCH v2 4/5] android/gatt: Cleanup device lists

[Lukasz Rymanowski]

With this patch, devices from conn_list and conn_wait_queue which are
without a client e.g. because client has unregister without any
cleaning, are move to the disconnected device queue.

Also connected device without clients are disconnected.
And if there is no dev waiting for connect we do stop scan.
---
 android/gatt.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 47 insertions(+), 4 deletions(-)

diff --git a/android/gatt.c b/android/gatt.c
index 6368317..235e9e4 100644
--- a/android/gatt.c
+++ b/android/gatt.c
@@ -284,6 +284,13 @@ static bool match_dev_by_conn_id(const void *data, const void *user_data)
 	return dev->conn_id == conn_id;
 }
 
+static bool match_dev_without_client(const void *data, const void *user_data)
+{
+	const struct gatt_device *dev = data;
+
+	return queue_isempty(dev->clients);
+}
+
 static bool match_srvc_by_element_id(const void *data, const void *user_data)
 {
 	const struct element_id *exp_id = user_data;
@@ -547,19 +554,55 @@ static void put_device_on_disc_list(struct gatt_device *dev)
 	queue_push_tail(disc_dev_list, dev);
 }
 
+static void cleanup_conn_list(int32_t id)
+{
+	struct gatt_device *dev;
+
+	/* Find device without client */
+	dev = queue_remove_if(conn_list, match_dev_without_client, NULL);
+	while (dev) {
+		/* Device is connected, lets disconnect and notify client */
+		connection_cleanup(dev);
+		send_client_disconnect_notify(id, dev, GATT_SUCCESS);
+
+		/* Put device on disconnected device list */
+		put_device_on_disc_list(dev);
+		dev = queue_remove_if(conn_list, match_dev_without_client,
+									NULL);
+	};
+}
+
+static void cleanup_conn_wait_queue(int32_t id)
+{
+	struct gatt_device *dev;
+
+	/* Find device without client */
+	dev = queue_remove_if(conn_wait_queue, match_dev_without_client, NULL);
+	while (dev) {
+		send_client_connect_notify(id, dev, GATT_FAILURE);
+
+		/* Put device on disconnected device list */
+		put_device_on_disc_list(dev);
+		dev = queue_remove_if(conn_wait_queue,
+						match_dev_without_client, NULL);
+	};
+
+	/* Stop scan we had for connecting devices */
+	if (queue_isempty(conn_wait_queue) && !scanning)
+		bt_le_discovery_stop(NULL);
+}
+
 static void remove_client_from_devices(int32_t id)
 {
 	DBG("");
 
 	queue_foreach(conn_list, remove_client_from_device_list,
 							INT_TO_PTR(id));
-
-	/*TODO: Check if there is any zombie device (connected no client)*/
+	cleanup_conn_list(id);
 
 	queue_foreach(conn_wait_queue, remove_client_from_device_list,
 							INT_TO_PTR(id));
-
-	/*TODO: Check if there is not zombie device plus stop scan */
+	cleanup_conn_wait_queue(id);
 }
 
 static void handle_client_unregister(const void *buf, uint16_t len)
-- 
1.8.4

[PATCH v2 5/5] android/gatt: Fix type of status parameter

[Lukasz Rymanowski]

---
 android/gatt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/android/gatt.c b/android/gatt.c
index 235e9e4..cdca424 100644
--- a/android/gatt.c
+++ b/android/gatt.c
@@ -509,7 +509,7 @@ static void connection_cleanup(struct gatt_device *device)
 }
 
 static void send_client_disconnect_notify(int32_t id, struct gatt_device *dev,
-								uint8_t status)
+								int32_t status)
 {
 	struct hal_ev_gatt_client_disconnect ev;
 
-- 
1.8.4

Re: [PATCH v2 1/5] android/gatt: Refactor send_client_connect_notify

[Szymon Janc]

Hi Łukasz,

On Thursday 10 of April 2014 17:30:01 Lukasz Rymanowski wrote:
> Create helper function to send connect notification, similar to the
> send_client_disconnect_notify
> ---
>  android/gatt.c | 55 ++++++++++++++++++++++++++++++++++---------------------
>  1 file changed, 34 insertions(+), 21 deletions(-)
> 
> diff --git a/android/gatt.c b/android/gatt.c
> index a33ce25..80fa139 100644
> --- a/android/gatt.c
> +++ b/android/gatt.c
> @@ -480,6 +480,24 @@ failed:
>  					HAL_OP_GATT_CLIENT_REGISTER, status);
>  }
>  
> +static void send_client_connect_notify(int32_t id, struct gatt_device *dev,
> +								int32_t status)
> +{
> +	struct hal_ev_gatt_client_connect ev;
> +
> +	ev.client_if = id;
> +	ev.status = status;
> +
> +	if (status == GATT_SUCCESS) {
> +		/* Set address and client id in the event */
> +		bdaddr2android(&dev->bdaddr, &ev.bda);
> +		ev.conn_id = dev->conn_id;
> +	}

This will send garbage if status != GATT_SUCCESS.

> +
> +	ipc_send_notif(hal_ipc, HAL_SERVICE_ID_GATT,
> +				HAL_EV_GATT_CLIENT_CONNECT, sizeof(ev), &ev);
> +}
> +
>  static void handle_client_unregister(const void *buf, uint16_t len)
>  {
>  	const struct hal_cmd_gatt_client_unregister *cmd = buf;
> @@ -785,25 +803,26 @@ done:
>  	return FALSE;
>  }
>  
> -static void send_client_connect_notify(void *data, void *user_data)
> +struct connect_data {
> +	struct gatt_device *dev;
> +	int32_t status;
> +};
> +
> +static void send_client_connect_notifications(void *data, void *user_data)
>  {
> -	struct hal_ev_gatt_client_connect *ev = user_data;
>  	int32_t id = PTR_TO_INT(data);
> +	struct connect_data *c = user_data;
>  
> -	ev->client_if = id;
> -
> -	ipc_send_notif(hal_ipc, HAL_SERVICE_ID_GATT,
> -				HAL_EV_GATT_CLIENT_CONNECT, sizeof(*ev), ev);
> +	send_client_connect_notify(id, c->dev, c->status);
>  }
>  
>  static void connect_cb(GIOChannel *io, GError *gerr, gpointer user_data)
>  {
>  	bdaddr_t *addr = user_data;
>  	struct gatt_device *dev;
> -	struct hal_ev_gatt_client_connect ev;
> +	struct connect_data data;
>  	GAttrib *attrib;
>  	static uint32_t conn_id = 0;
> -	int32_t status;
>  
>  	/* Take device from conn waiting queue */
>  	dev = queue_remove_if(conn_wait_queue, match_dev_by_bdaddr, addr);
> @@ -816,19 +835,16 @@ static void connect_cb(GIOChannel *io, GError *gerr, gpointer user_data)
>  	g_io_channel_unref(dev->att_io);
>  	dev->att_io = NULL;
>  
> -	/* Set address and client id in the event */
> -	bdaddr2android(&dev->bdaddr, &ev.bda);
> -
>  	if (gerr) {
>  		error("gatt: connection failed %s", gerr->message);
> -		status = GATT_FAILURE;
> +		data.status = GATT_FAILURE;
>  		goto reply;
>  	}
>  
>  	attrib = g_attrib_new(io);
>  	if (!attrib) {
>  		error("gatt: unable to create new GAttrib instance");
> -		status = GATT_FAILURE;
> +		data.status = GATT_FAILURE;
>  		goto reply;
>  	}
>  
> @@ -841,21 +857,18 @@ static void connect_cb(GIOChannel *io, GError *gerr, gpointer user_data)
>  	if (!queue_push_tail(conn_list, dev)) {
>  		error("gatt: Cannot push dev on conn_list");
>  		connection_cleanup(dev);
> -		status = GATT_FAILURE;
> +		data.status = GATT_FAILURE;
>  		goto reply;
>  	}
>  
> -	status = GATT_SUCCESS;
> -	goto reply;
> +	data.status = GATT_SUCCESS;
>  
>  reply:
> -	ev.conn_id = dev ? dev->conn_id : 0;
> -	ev.status = status;
> -
> -	queue_foreach(dev->clients, send_client_connect_notify, &ev);
> +	data.dev = dev;
> +	queue_foreach(dev->clients, send_client_connect_notifications, &data);
>  
>  	/* If connection did not succeed, destroy device */
> -	if (status)
> +	if (data.status)
>  		destroy_device(dev);
>  
>  	/* Check if we should restart scan */
> 

-- 
Best regards, 
Szymon Janc

Re: [PATCH v2 1/5] android/gatt: Refactor send_client_connect_notify

[Lukasz Rymanowski]

Hi Szymon,

On 11 April 2014 15:27, Szymon Janc <szymon.janc@tieto.com> wrote:
> Hi Łukasz,
>
> On Thursday 10 of April 2014 17:30:01 Lukasz Rymanowski wrote:
>> Create helper function to send connect notification, similar to the
>> send_client_disconnect_notify
>> ---
>>  android/gatt.c | 55 ++++++++++++++++++++++++++++++++++---------------------
>>  1 file changed, 34 insertions(+), 21 deletions(-)
>>
>> diff --git a/android/gatt.c b/android/gatt.c
>> index a33ce25..80fa139 100644
>> --- a/android/gatt.c
>> +++ b/android/gatt.c
>> @@ -480,6 +480,24 @@ failed:
>>                                       HAL_OP_GATT_CLIENT_REGISTER, status);
>>  }
>>
>> +static void send_client_connect_notify(int32_t id, struct gatt_device *dev,
>> +                                                             int32_t status)
>> +{
>> +     struct hal_ev_gatt_client_connect ev;
>> +
>> +     ev.client_if = id;
>> +     ev.status = status;
>> +
>> +     if (status == GATT_SUCCESS) {
>> +             /* Set address and client id in the event */
>> +             bdaddr2android(&dev->bdaddr, &ev.bda);
>> +             ev.conn_id = dev->conn_id;
>> +     }
>
> This will send garbage if status != GATT_SUCCESS.
>
In error case status and client_if is important, other stuff can be a garbage.

>> +
>> +     ipc_send_notif(hal_ipc, HAL_SERVICE_ID_GATT,
>> +                             HAL_EV_GATT_CLIENT_CONNECT, sizeof(ev), &ev);
>> +}
>> +
>>  static void handle_client_unregister(const void *buf, uint16_t len)
>>  {
>>       const struct hal_cmd_gatt_client_unregister *cmd = buf;
>> @@ -785,25 +803,26 @@ done:
>>       return FALSE;
>>  }
>>
>> -static void send_client_connect_notify(void *data, void *user_data)
>> +struct connect_data {
>> +     struct gatt_device *dev;
>> +     int32_t status;
>> +};
>> +
>> +static void send_client_connect_notifications(void *data, void *user_data)
>>  {
>> -     struct hal_ev_gatt_client_connect *ev = user_data;
>>       int32_t id = PTR_TO_INT(data);
>> +     struct connect_data *c = user_data;
>>
>> -     ev->client_if = id;
>> -
>> -     ipc_send_notif(hal_ipc, HAL_SERVICE_ID_GATT,
>> -                             HAL_EV_GATT_CLIENT_CONNECT, sizeof(*ev), ev);
>> +     send_client_connect_notify(id, c->dev, c->status);
>>  }
>>
>>  static void connect_cb(GIOChannel *io, GError *gerr, gpointer user_data)
>>  {
>>       bdaddr_t *addr = user_data;
>>       struct gatt_device *dev;
>> -     struct hal_ev_gatt_client_connect ev;
>> +     struct connect_data data;
>>       GAttrib *attrib;
>>       static uint32_t conn_id = 0;
>> -     int32_t status;
>>
>>       /* Take device from conn waiting queue */
>>       dev = queue_remove_if(conn_wait_queue, match_dev_by_bdaddr, addr);
>> @@ -816,19 +835,16 @@ static void connect_cb(GIOChannel *io, GError *gerr, gpointer user_data)
>>       g_io_channel_unref(dev->att_io);
>>       dev->att_io = NULL;
>>
>> -     /* Set address and client id in the event */
>> -     bdaddr2android(&dev->bdaddr, &ev.bda);
>> -
>>       if (gerr) {
>>               error("gatt: connection failed %s", gerr->message);
>> -             status = GATT_FAILURE;
>> +             data.status = GATT_FAILURE;
>>               goto reply;
>>       }
>>
>>       attrib = g_attrib_new(io);
>>       if (!attrib) {
>>               error("gatt: unable to create new GAttrib instance");
>> -             status = GATT_FAILURE;
>> +             data.status = GATT_FAILURE;
>>               goto reply;
>>       }
>>
>> @@ -841,21 +857,18 @@ static void connect_cb(GIOChannel *io, GError *gerr, gpointer user_data)
>>       if (!queue_push_tail(conn_list, dev)) {
>>               error("gatt: Cannot push dev on conn_list");
>>               connection_cleanup(dev);
>> -             status = GATT_FAILURE;
>> +             data.status = GATT_FAILURE;
>>               goto reply;
>>       }
>>
>> -     status = GATT_SUCCESS;
>> -     goto reply;
>> +     data.status = GATT_SUCCESS;
>>
>>  reply:
>> -     ev.conn_id = dev ? dev->conn_id : 0;
>> -     ev.status = status;
>> -
>> -     queue_foreach(dev->clients, send_client_connect_notify, &ev);
>> +     data.dev = dev;
>> +     queue_foreach(dev->clients, send_client_connect_notifications, &data);
>>
>>       /* If connection did not succeed, destroy device */
>> -     if (status)
>> +     if (data.status)
>>               destroy_device(dev);
>>
>>       /* Check if we should restart scan */
>>
>
> --
> Best regards,
> Szymon Janc

\Lukasz

Re: [PATCH v2 5/5] android/gatt: Fix type of status parameter

[Szymon Janc]

Hi Łukasz,

On Thursday 10 of April 2014 17:30:05 Lukasz Rymanowski wrote:
> ---
>  android/gatt.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/android/gatt.c b/android/gatt.c
> index 235e9e4..cdca424 100644
> --- a/android/gatt.c
> +++ b/android/gatt.c
> @@ -509,7 +509,7 @@ static void connection_cleanup(struct gatt_device *device)
>  }
>  
>  static void send_client_disconnect_notify(int32_t id, struct gatt_device *dev,
> -								uint8_t status)
> +								int32_t status)
>  {
>  	struct hal_ev_gatt_client_disconnect ev;
>  
> 

I've applied this patch, but please check why this function is always called
success status. Shouldn't we call this callback with fail status if
handle_client_disconnect() failed?

-- 
Best regards, 
Szymon Janc

Re: [PATCH v2 1/5] android/gatt: Refactor send_client_connect_notify

[Szymon Janc]

Hi Łukasz,

On Friday 11 of April 2014 15:32:20 Lukasz Rymanowski wrote:
> Hi Szymon,
> 
> On 11 April 2014 15:27, Szymon Janc <szymon.janc@tieto.com> wrote:
> > Hi Łukasz,
> >
> > On Thursday 10 of April 2014 17:30:01 Lukasz Rymanowski wrote:
> >> Create helper function to send connect notification, similar to the
> >> send_client_disconnect_notify
> >> ---
> >>  android/gatt.c | 55 ++++++++++++++++++++++++++++++++++---------------------
> >>  1 file changed, 34 insertions(+), 21 deletions(-)
> >>
> >> diff --git a/android/gatt.c b/android/gatt.c
> >> index a33ce25..80fa139 100644
> >> --- a/android/gatt.c
> >> +++ b/android/gatt.c
> >> @@ -480,6 +480,24 @@ failed:
> >>                                       HAL_OP_GATT_CLIENT_REGISTER, status);
> >>  }
> >>
> >> +static void send_client_connect_notify(int32_t id, struct gatt_device *dev,
> >> +                                                             int32_t status)
> >> +{
> >> +     struct hal_ev_gatt_client_connect ev;
> >> +
> >> +     ev.client_if = id;
> >> +     ev.status = status;
> >> +
> >> +     if (status == GATT_SUCCESS) {
> >> +             /* Set address and client id in the event */
> >> +             bdaddr2android(&dev->bdaddr, &ev.bda);
> >> +             ev.conn_id = dev->conn_id;
> >> +     }
> >
> > This will send garbage if status != GATT_SUCCESS.
> >
> In error case status and client_if is important, other stuff can be a garbage.

I don't think we should be sending random stack data over IPC.

> >> +
> >> +     ipc_send_notif(hal_ipc, HAL_SERVICE_ID_GATT,
> >> +                             HAL_EV_GATT_CLIENT_CONNECT, sizeof(ev), &ev);
> >> +}
> >> +
> >>  static void handle_client_unregister(const void *buf, uint16_t len)
> >>  {
> >>       const struct hal_cmd_gatt_client_unregister *cmd = buf;
> >> @@ -785,25 +803,26 @@ done:
> >>       return FALSE;
> >>  }
> >>
> >> -static void send_client_connect_notify(void *data, void *user_data)
> >> +struct connect_data {
> >> +     struct gatt_device *dev;
> >> +     int32_t status;
> >> +};
> >> +
> >> +static void send_client_connect_notifications(void *data, void *user_data)
> >>  {
> >> -     struct hal_ev_gatt_client_connect *ev = user_data;
> >>       int32_t id = PTR_TO_INT(data);
> >> +     struct connect_data *c = user_data;
> >>
> >> -     ev->client_if = id;
> >> -
> >> -     ipc_send_notif(hal_ipc, HAL_SERVICE_ID_GATT,
> >> -                             HAL_EV_GATT_CLIENT_CONNECT, sizeof(*ev), ev);
> >> +     send_client_connect_notify(id, c->dev, c->status);
> >>  }
> >>
> >>  static void connect_cb(GIOChannel *io, GError *gerr, gpointer user_data)
> >>  {
> >>       bdaddr_t *addr = user_data;
> >>       struct gatt_device *dev;
> >> -     struct hal_ev_gatt_client_connect ev;
> >> +     struct connect_data data;
> >>       GAttrib *attrib;
> >>       static uint32_t conn_id = 0;
> >> -     int32_t status;
> >>
> >>       /* Take device from conn waiting queue */
> >>       dev = queue_remove_if(conn_wait_queue, match_dev_by_bdaddr, addr);
> >> @@ -816,19 +835,16 @@ static void connect_cb(GIOChannel *io, GError *gerr, gpointer user_data)
> >>       g_io_channel_unref(dev->att_io);
> >>       dev->att_io = NULL;
> >>
> >> -     /* Set address and client id in the event */
> >> -     bdaddr2android(&dev->bdaddr, &ev.bda);
> >> -
> >>       if (gerr) {
> >>               error("gatt: connection failed %s", gerr->message);
> >> -             status = GATT_FAILURE;
> >> +             data.status = GATT_FAILURE;
> >>               goto reply;
> >>       }
> >>
> >>       attrib = g_attrib_new(io);
> >>       if (!attrib) {
> >>               error("gatt: unable to create new GAttrib instance");
> >> -             status = GATT_FAILURE;
> >> +             data.status = GATT_FAILURE;
> >>               goto reply;
> >>       }
> >>
> >> @@ -841,21 +857,18 @@ static void connect_cb(GIOChannel *io, GError *gerr, gpointer user_data)
> >>       if (!queue_push_tail(conn_list, dev)) {
> >>               error("gatt: Cannot push dev on conn_list");
> >>               connection_cleanup(dev);
> >> -             status = GATT_FAILURE;
> >> +             data.status = GATT_FAILURE;
> >>               goto reply;
> >>       }
> >>
> >> -     status = GATT_SUCCESS;
> >> -     goto reply;
> >> +     data.status = GATT_SUCCESS;
> >>
> >>  reply:
> >> -     ev.conn_id = dev ? dev->conn_id : 0;
> >> -     ev.status = status;
> >> -
> >> -     queue_foreach(dev->clients, send_client_connect_notify, &ev);
> >> +     data.dev = dev;
> >> +     queue_foreach(dev->clients, send_client_connect_notifications, &data);
> >>
> >>       /* If connection did not succeed, destroy device */
> >> -     if (status)
> >> +     if (data.status)
> >>               destroy_device(dev);
> >>
> >>       /* Check if we should restart scan */
> >>
> >
> > --
> > Best regards,
> > Szymon Janc
> 
> \Lukasz

-- 
Best regards, 
Szymon Janc