[RFC 0/2] shared/gatt-client: ATT invalidation handler interface

[RFC 0/2] shared/gatt-client: ATT invalidation handler interface

[Jakub Tyszkowski]

Purpose of this RFC is to discuss the need for implementing the following item
  from the TODO list:

  "
  ATT/GATT (new shared stack)
  ===========================

  - Introduce a handler interface to shared/gatt-client which can be used by the
    upper layer to determine when the link has been disconnected or an ATT
    protocol request times out.

    Priority: Medium
    Complexity: C2
  "

Even though the following patches propose the solution for this TODO entry if I
understood it correctly, I'm not sure if it is really needed to have this
in shared/gatt-client, as this would only be wrapping the shared/att code.
As shared/gatt-client is not directly handling the connection, nor is
initialising the shared/att, its up to user, which does that
(tools/btgatt-client for now) to handle this events, as it is using the
shared/att API directly anyway. And btw. 'bt_att_register_disconnect()' was
designed to be 'user ready/user friendly' as it can handle multiple
users/callbacks. If we realy need this multiple user notifications we could
extend the 'bt_att_set_timeout_cb()' to work in similar way.

Is there a good reason to have this functionality on shared/gatt-client level?
If we really want this, then what about the gatt-server? We duplicate the
wrapping we have in gatt-client?

Regards,

Jakub Tyszkowski (2):
  shared/gatt-client: Add callback for reporting att state
  tools/btgatt-client: Use shared/gatt att state callback mechanism

 src/shared/gatt-client.c | 62 +++++++++++++++++++++++++++++++++++++++++++++++-
 src/shared/gatt-client.h |  5 ++++
 tools/btgatt-client.c    | 12 +++-------
 3 files changed, 69 insertions(+), 10 deletions(-)

--
1.9.1

[RFC 1/2] shared/gatt-client: Add callback for reporting att state

[Jakub Tyszkowski]

This will be used to notify upper layers about the link disconnection
or ATT procedure timeout.
---
 src/shared/gatt-client.c | 62 +++++++++++++++++++++++++++++++++++++++++++++++-
 src/shared/gatt-client.h |  5 ++++
 2 files changed, 66 insertions(+), 1 deletion(-)

diff --git a/src/shared/gatt-client.c b/src/shared/gatt-client.c
index 6dc8e95..5c72466 100644
--- a/src/shared/gatt-client.c
+++ b/src/shared/gatt-client.c
@@ -88,6 +88,10 @@ struct bt_gatt_client {
 	bt_gatt_client_destroy_func_t debug_destroy;
 	void *debug_data;
 
+	bt_gatt_client_stale_callback_t att_stale_callback;
+	bt_gatt_client_destroy_func_t att_stale_destroy;
+	void *att_stale_data;
+
 	struct service_list *svc_head, *svc_tail;
 	bool in_init;
 	bool ready;
@@ -104,7 +108,7 @@ struct bt_gatt_client {
 	/* List of registered notification/indication callbacks */
 	struct queue *notify_list;
 	int next_reg_id;
-	unsigned int notify_id, ind_id;
+	unsigned int notify_id, ind_id, disc_id;
 	bool in_notify;
 	bool need_notify_cleanup;
 
@@ -1211,6 +1215,29 @@ static void notify_cb(uint8_t opcode, const void *pdu, uint16_t length,
 	bt_gatt_client_unref(client);
 }
 
+static void disconnect_cb(void *user_data)
+{
+	struct bt_gatt_client *client = user_data;
+
+	util_debug(client->debug_callback, client->debug_data, "Disconnect_cb");
+
+	if (client->att_stale_callback)
+		client->att_stale_callback(client->att_stale_data);
+}
+
+static void att_timeout_cb(unsigned int id, uint8_t opcode,
+							void *user_data)
+{
+	struct bt_gatt_client *client = user_data;
+
+	util_debug(client->debug_callback, client->debug_data,
+			"ID: %d transaction timeout, opcode: 0x%02x", id,
+			opcode);
+
+	if (client->att_stale_callback)
+		client->att_stale_callback(client->att_stale_data);
+}
+
 struct bt_gatt_client *bt_gatt_client_new(struct bt_att *att, uint16_t mtu)
 {
 	struct bt_gatt_client *client;
@@ -1264,6 +1291,20 @@ struct bt_gatt_client *bt_gatt_client_new(struct bt_att *att, uint16_t mtu)
 		return NULL;
 	}
 
+	client->disc_id = bt_att_register_disconnect(att, disconnect_cb,
+								client,	NULL);
+	if (!client->disc_id) {
+		bt_att_unregister(att, client->ind_id);
+		bt_att_unregister(att, client->notify_id);
+		queue_destroy(client->notify_list, NULL);
+		queue_destroy(client->svc_chngd_queue, NULL);
+		queue_destroy(client->long_write_queue, NULL);
+		free(client);
+		return NULL;
+	}
+
+	bt_att_set_timeout_cb(att, att_timeout_cb, client, NULL);
+
 	client->att = bt_att_ref(att);
 
 	gatt_client_init(client, mtu);
@@ -1299,6 +1340,7 @@ void bt_gatt_client_unref(struct bt_gatt_client *client)
 
 	bt_att_unregister(client->att, client->notify_id);
 	bt_att_unregister(client->att, client->ind_id);
+	bt_att_unregister_disconnect(client->att, client->disc_id);
 
 	queue_destroy(client->svc_chngd_queue, free);
 	queue_destroy(client->long_write_queue, long_write_op_unref);
@@ -1368,6 +1410,24 @@ bool bt_gatt_client_set_debug(struct bt_gatt_client *client,
 	return true;
 }
 
+bool bt_gatt_client_set_stale_handler(struct bt_gatt_client *client,
+				bt_gatt_client_stale_callback_t callback,
+				void *user_data,
+				bt_gatt_client_destroy_func_t destroy)
+{
+	if (!client)
+		return false;
+
+	if (client->att_stale_destroy)
+		client->att_stale_destroy(client->att_stale_data);
+
+	client->att_stale_callback = callback;
+	client->att_stale_destroy = destroy;
+	client->att_stale_data = user_data;
+
+	return true;
+}
+
 bool bt_gatt_service_iter_init(struct bt_gatt_service_iter *iter,
 						struct bt_gatt_client *client)
 {
diff --git a/src/shared/gatt-client.h b/src/shared/gatt-client.h
index 6807f6b..73a1002 100644
--- a/src/shared/gatt-client.h
+++ b/src/shared/gatt-client.h
@@ -59,6 +59,7 @@ typedef void (*bt_gatt_client_notify_id_callback_t)(unsigned int id,
 typedef void (*bt_gatt_client_service_changed_callback_t)(uint16_t start_handle,
 							uint16_t end_handle,
 							void *user_data);
+typedef void (*bt_gatt_client_stale_callback_t)(void *user_data);
 
 bool bt_gatt_client_is_ready(struct bt_gatt_client *client);
 bool bt_gatt_client_set_ready_handler(struct bt_gatt_client *client,
@@ -73,6 +74,10 @@ bool bt_gatt_client_set_debug(struct bt_gatt_client *client,
 					bt_gatt_client_debug_func_t callback,
 					void *user_data,
 					bt_gatt_client_destroy_func_t destroy);
+bool bt_gatt_client_set_stale_handler(struct bt_gatt_client *client,
+				bt_gatt_client_stale_callback_t callback,
+				void *user_data,
+				bt_gatt_client_destroy_func_t destroy);
 
 typedef struct {
 	uint16_t start_handle;
-- 
1.9.1

[RFC 2/2] tools/btgatt-client: Use shared/gatt att state callback mechanism

[Jakub Tyszkowski]

With this btgatt-tester can detect if att connection is invalid due
to link disconnection or att protocol procedure timeout. There is no
point in diffirentiating these two reasons as result is the same - att
connection is not valid.
---
 tools/btgatt-client.c | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/tools/btgatt-client.c b/tools/btgatt-client.c
index d900e08..9b9224e 100644
--- a/tools/btgatt-client.c
+++ b/tools/btgatt-client.c
@@ -71,9 +71,9 @@ static void print_prompt(void)
 	fflush(stdout);
 }
 
-static void att_disconnect_cb(void *user_data)
+static void att_stale_cb(void *user_data)
 {
-	printf("Device disconnected\n");
+	printf("Att is stale. Terminating.\n");
 
 	mainloop_quit();
 }
@@ -122,13 +122,6 @@ static struct client *client_create(int fd, uint16_t mtu)
 		return NULL;
 	}
 
-	if (!bt_att_register_disconnect(att, att_disconnect_cb, NULL, NULL)) {
-		fprintf(stderr, "Failed to set ATT disconnect handler\n");
-		bt_att_unref(att);
-		free(cli);
-		return NULL;
-	}
-
 	cli->fd = fd;
 	cli->gatt = bt_gatt_client_new(att, mtu);
 	if (!cli->gatt) {
@@ -147,6 +140,7 @@ static struct client *client_create(int fd, uint16_t mtu)
 	bt_gatt_client_set_ready_handler(cli->gatt, ready_cb, cli, NULL);
 	bt_gatt_client_set_service_changed(cli->gatt, service_changed_cb, cli,
 									NULL);
+	bt_gatt_client_set_stale_handler(cli->gatt, att_stale_cb, cli, NULL);
 
 	/* bt_gatt_client already holds a reference */
 	bt_att_unref(att);
-- 
1.9.1

Re: [RFC 0/2] shared/gatt-client: ATT invalidation handler interface

[Arman Uguray]

Hi Jakub,


> Even though the following patches propose the solution for this TODO entry if I
> understood it correctly, I'm not sure if it is really needed to have this
> in shared/gatt-client, as this would only be wrapping the shared/att code.
> As shared/gatt-client is not directly handling the connection, nor is
> initialising the shared/att, its up to user, which does that
> (tools/btgatt-client for now) to handle this events, as it is using the
> shared/att API directly anyway. And btw. 'bt_att_register_disconnect()' was
> designed to be 'user ready/user friendly' as it can handle multiple
> users/callbacks. If we realy need this multiple user notifications we could
> extend the 'bt_att_set_timeout_cb()' to work in similar way.
>
> Is there a good reason to have this functionality on shared/gatt-client level?
> If we really want this, then what about the gatt-server? We duplicate the
> wrapping we have in gatt-client?

I guess you're right in that it might be unnecessary to add an extra
layer of abstraction for something that is already pretty simple to
use. So let's not add a callback mechanism into shared/gatt-client.

That being said, shared/gatt-client still needs to properly handle
disconnections. So, at least internally, gatt-client should probably
clear its service cache and make sure that all registered notify
handlers have been unregistered. This is assuming that the same
instance of gatt-client will be used across multiple connections. I
guess we should discuss whether we expect users to create a new
gatt-client for each connection or whether to extend gatt-client to
allow a new bt_att structure to be assigned to it.

As for timeouts, bt_att automatically destroys the underlying io,
which should close the connection. So it maybe enough for the
upper-layer to just call bt_att_register_disconnect.

Cheers,
Arman

Re: [RFC 0/2] shared/gatt-client: ATT invalidation handler interface

[Tyszkowski Jakub]

Hi Arman,

On 09/29/2014 09:48 PM, Arman Uguray wrote:
> Hi Jakub,
>
>
>> Even though the following patches propose the solution for this TODO entry if I
>> understood it correctly, I'm not sure if it is really needed to have this
>> in shared/gatt-client, as this would only be wrapping the shared/att code.
>> As shared/gatt-client is not directly handling the connection, nor is
>> initialising the shared/att, its up to user, which does that
>> (tools/btgatt-client for now) to handle this events, as it is using the
>> shared/att API directly anyway. And btw. 'bt_att_register_disconnect()' was
>> designed to be 'user ready/user friendly' as it can handle multiple
>> users/callbacks. If we realy need this multiple user notifications we could
>> extend the 'bt_att_set_timeout_cb()' to work in similar way.
>>
>> Is there a good reason to have this functionality on shared/gatt-client level?
>> If we really want this, then what about the gatt-server? We duplicate the
>> wrapping we have in gatt-client?
>
> I guess you're right in that it might be unnecessary to add an extra
> layer of abstraction for something that is already pretty simple to
> use. So let's not add a callback mechanism into shared/gatt-client.

It really depends on how gatt-client should be used by multiple users.

I thought about some 'master entity' like bluetooth profile owning the 
link and creating bt_att and gatt_client/gatt_server instances, while 
exposing function to get their references for the given bdaddr. 
Something like:

	bt_gatt_connection_state_cb_t gatt_connection_state_cb
			(gatt_connection_state_t, void *user_data) {}
	bt_connect_gatt(&bdaddr, role, gatt_connection_state_cb);

Where gatt_client reference is passed as user_data argument for the 
gatt_connection_state_cb when connecting and NULL is passed on 
disconnection. This makes tracking gatt_client's life cycle quite 
simple, as gatt-client is created on the firs connect call and other 
users calling connect just receives the reference in callback when it's 
already connected.

With such solution the 'master entity' deals with bt_att callback API 
and calls the registered gatt_connection_state_cb so no need to expose 
callback mechanism on gatt-client level. As we don't want to expose 
bt_att API to users, bt_att callback mechanism would be wrapped anyway, 
but on a different level.

Probably we could also make 'master entity' have just something like:

	gatt_client *bt_get_gatt_client(bdaddr);

and let the users do the connect_cb registration using gatt-client API 
but I'm not sure if this is a better solution.

Just thinking out loud. :)

>
> That being said, shared/gatt-client still needs to properly handle
> disconnections. So, at least internally, gatt-client should probably
> clear its service cache and make sure that all registered notify
> handlers have been unregistered. This is assuming that the same
> instance of gatt-client will be used across multiple connections. I
> guess we should discuss whether we expect users to create a new
> gatt-client for each connection or whether to extend gatt-client to
> allow a new bt_att structure to be assigned to it.

This would be irrelevant with the first solution proposed above, as 
users would not care about it as they are getting the reference on each 
connection. I think we should go with the simplest solution.

Should we think here also about the caching?

>
> As for timeouts, bt_att automatically destroys the underlying io,
> which should close the connection. So it maybe enough for the
> upper-layer to just call bt_att_register_disconnect.

Right. We don't care about the disconnect reason anyway.

>
> Cheers,
> Arman
>

Regards,
Jakub

Re: [RFC 0/2] shared/gatt-client: ATT invalidation handler interface

[Arman Uguray]

Hi Jakub,


> It really depends on how gatt-client should be used by multiple users.
>
> I thought about some 'master entity' like bluetooth profile owning the link
> and creating bt_att and gatt_client/gatt_server instances, while exposing
> function to get their references for the given bdaddr. Something like:
>
>         bt_gatt_connection_state_cb_t gatt_connection_state_cb
>                         (gatt_connection_state_t, void *user_data) {}
>         bt_connect_gatt(&bdaddr, role, gatt_connection_state_cb);
>
> Where gatt_client reference is passed as user_data argument for the
> gatt_connection_state_cb when connecting and NULL is passed on
> disconnection. This makes tracking gatt_client's life cycle quite simple, as
> gatt-client is created on the firs connect call and other users calling
> connect just receives the reference in callback when it's already connected.
>
> With such solution the 'master entity' deals with bt_att callback API and
> calls the registered gatt_connection_state_cb so no need to expose callback
> mechanism on gatt-client level. As we don't want to expose bt_att API to
> users, bt_att callback mechanism would be wrapped anyway, but on a different
> level.
>
> Probably we could also make 'master entity' have just something like:
>
>         gatt_client *bt_get_gatt_client(bdaddr);
>
> and let the users do the connect_cb registration using gatt-client API but
> I'm not sure if this is a better solution.
>
> Just thinking out loud. :)

Yeah, this overall makes sense, since in both the desktop and android
implementations, somebody will need to put together the different
pieces from shared. In bluetoothd, src/device.c will probably own
these, via something like:

int fd = socket(...).
connect(fd);

att = bt_att_new(att);
bt_att_set_close_on_unref(att);
bt_att_register_disconnect(att, disconnect_cb);

client = bt_gatt_client_new(att, mtu);

This would be internal to bluetoothd. Currently the D-Bus API for the
desktop daemon doesn't have a "per-application GATT connect"
interface. It just has a generic UI connect. So this is something that
will need to change and your proposal makes sense I think.

Now when the link gets disconnected (either via bt_att_unref(att) or
it's terminated unexpectedly), the bt_att instance becomes no longer
valid and a new bt_att needs to be created. At this point we need to
decide what happens to the bt_gatt_client. Obviously none of the
discovery/read/write operations will work since the bt_att is invalid
and the attribute cache stored in bt_gatt_client is also technically
invalid unless we are bonded. So maybe, as you said, we simply
invalidate the existing bt_gatt_client (mark it as invalid
internally), and expect users to obtain a new instance by requesting
to connect.


> This would be irrelevant with the first solution proposed above, as users
> would not care about it as they are getting the reference on each
> connection. I think we should go with the simplest solution.
>
> Should we think here also about the caching?
>

Yeah, so it seems like we're limiting the lifetime/validity of a
bt_gatt_client object to the lifetime of the physical link itself.
Hence the cache is valid as long the connection is up and a new
connection would warrant service discovery from scratch. A feature
that we can add later is long-term caching in the bonding case. We
would basically persist the attribute cache in a file and next time
there is a new connection we would instantiate a bt_gatt_client from
the file:

client = bt_gatt_client_create_from_storage(att, mtu, keyfile);

Though this is something we can worry about later. For now, let's have
bt_gatt_client register a disconnect handler and mark itself as
invalid. I guess we can keep the service cache alive there but the
gatt_client would be unusable at this point and creating a new one
would be necessary for the next connection.

I hope this makes sense :)

Cheers,
Arman