[RFC 0/1] Implement Compound (Multi-step) GATT Procedure Support

[RFC 0/1] Implement Compound (Multi-step) GATT Procedure Support

[Brian Gix]

The following two proposed patches implement a method for correctly
staging GATT procedures that require multiple ATT req/resp exchanges.

The first RFC / PATCH is gattrib.[ch], which allows the caller to specify
a gboolean indicating that the command being queued is Compound
(even if the procedure ends up being single/atomic) and a queue ID
number, which allows the caller to cancel the GATT procedure at any
stage of the transaction.

IF -
The ATT opcode being queued is specified as compound, the resulting response
will not cause the next item in the queue to be sent until *after* the
response has been forwarded to the caller via the response callback.

IF -
The ATT opcode being queued is *not* compound, the resulting response
will service the pkt queue immediately (legacy functionality).

IF -
The ID passed to g_attrib_send_seq is ZERO, the command pkt will
be added to the TAIL of the queue, and a new ID assigned to it.
(Legacy functionality)

IF -
The ID passed to g_attrib_send_seq is NON-ZERO, the command pkt
will be added to the HEAD of the queue, causing it to be the next
pkt sent to the remote device.  The NON-ZERO ID is also then able
to cancel the command.


The second RFC / PATCH is to gatt.c. It modifies the existing gatt_read_char()
function to recognize the need for a compound Read procedure, and implements
it as a READ_REQ + READ_BLOB_REQ + READ_BLOB_REQ ...<etc> as needed, using
the g_attrib_send_seq() logic from the first RFC / PATCH.

-- 
Brian Gix
bgix@codeaurora.org
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum

[RFC 1/2] Add g_attrib_send_seq()

[Brian Gix]

Add g_attrib_send_seq() as an extension to g_attrib_send().
 	g_attrib_send_seq() functionally queues an entire
 	"GATT Procedure" as defined in the BT Core v4.0.
 	The intention is that the full procedure is run
 	to completion before the next GATT Procedure is
 	started. Subsequent ATT requests to a continuing
 	procedure are added to the head of the Attrib queue.

Fix g_attrib_send() to be the degenerative case of g_attrib_send_seq()
	This function now chains to g_attrib_send_seq() with arguments
	indicating that it is *not* a compound (multi-step) GATT
	procedure, but rather that the entire procedure is performed
	with a single ATT opcode request/response.

Fix received_data() to recognize that incoming response is (or isn't)
	part of a compound Procedure, so that it waits for additional
	requests before servicing the Attrib queue.
---
 attrib/gattrib.c |   44 ++++++++++++++++++++++++++++++++++++--------
 attrib/gattrib.h |    4 ++++
 2 files changed, 40 insertions(+), 8 deletions(-)

diff --git a/attrib/gattrib.c b/attrib/gattrib.c
index eace01b..8ef5d92 100644
--- a/attrib/gattrib.c
+++ b/attrib/gattrib.c
@@ -58,6 +58,7 @@ struct command {
 	guint16 len;
 	guint8 expected;
 	gboolean sent;
+	gboolean compound;
 	GAttribResultFunc func;
 	gpointer user_data;
 	GDestroyNotify notify;
@@ -285,6 +286,7 @@ static gboolean received_data(GIOChannel *io, GIOCondition cond, gpointer data)
 	uint8_t buf[512], status;
 	gsize len;
 	GIOStatus iostat;
+	gboolean compound = FALSE;
 
 	if (cond & (G_IO_HUP | G_IO_ERR | G_IO_NVAL)) {
 		attrib->read_watch = 0;
@@ -319,6 +321,8 @@ static gboolean received_data(GIOChannel *io, GIOCondition cond, gpointer data)
 		return attrib->events != NULL;
 	}
 
+	compound = cmd->compound;
+
 	if (buf[0] == ATT_OP_ERROR) {
 		status = buf[4];
 		goto done;
@@ -332,7 +336,8 @@ static gboolean received_data(GIOChannel *io, GIOCondition cond, gpointer data)
 	status = 0;
 
 done:
-	if (attrib->queue && g_queue_is_empty(attrib->queue) == FALSE)
+	if (!compound && attrib->queue &&
+			g_queue_is_empty(attrib->queue) == FALSE)
 		wake_up_sender(attrib);
 
 	if (cmd) {
@@ -340,6 +345,11 @@ done:
 			cmd->func(status, buf, len, cmd->user_data);
 
 		command_destroy(cmd);
+
+		if (compound && attrib->queue &&
+			g_queue_is_empty(attrib->queue) == FALSE)
+			wake_up_sender(attrib);
+
 	}
 
 	return TRUE;
@@ -367,12 +377,16 @@ GAttrib *g_attrib_new(GIOChannel *io)
 	return g_attrib_ref(attrib);
 }
 
-guint g_attrib_send(GAttrib *attrib, guint8 opcode, const guint8 *pdu,
-				guint16 len, GAttribResultFunc func,
-				gpointer user_data, GDestroyNotify notify)
+guint g_attrib_send_seq(GAttrib *attrib, gboolean compound, guint id,
+				guint8 opcode, const guint8 *pdu, guint16 len,
+				GAttribResultFunc func, gpointer user_data,
+				GDestroyNotify notify)
 {
 	struct command *c;
 
+	if (attrib == NULL || attrib->queue == NULL)
+		return 0;
+
 	c = g_try_new0(struct command, 1);
 	if (c == NULL)
 		return 0;
@@ -385,16 +399,30 @@ guint g_attrib_send(GAttrib *attrib, guint8 opcode, const guint8 *pdu,
 	c->func = func;
 	c->user_data = user_data;
 	c->notify = notify;
-	c->id = ++attrib->next_cmd_id;
+	c->compound = compound;
 
-	g_queue_push_tail(attrib->queue, c);
+	if (id) {
+		c->id = id;
+		g_queue_push_head(attrib->queue, c);
+	} else {
+		c->id = ++attrib->next_cmd_id;
+		g_queue_push_tail(attrib->queue, c);
 
-	if (g_queue_get_length(attrib->queue) == 1)
-		wake_up_sender(attrib);
+		if (g_queue_get_length(attrib->queue) == 1)
+			wake_up_sender(attrib);
+	}
 
 	return c->id;
 }
 
+guint g_attrib_send(GAttrib *attrib, guint8 opcode, const guint8 *pdu,
+				guint16 len, GAttribResultFunc func,
+				gpointer user_data, GDestroyNotify notify)
+{
+	return g_attrib_send_seq(attrib, FALSE, 0, opcode,
+				pdu, len, func, user_data, notify);
+}
+
 static gint command_cmp_by_id(gconstpointer a, gconstpointer b)
 {
 	const struct command *cmd = a;
diff --git a/attrib/gattrib.h b/attrib/gattrib.h
index 0940289..ade21bc 100644
--- a/attrib/gattrib.h
+++ b/attrib/gattrib.h
@@ -53,6 +53,10 @@ gboolean g_attrib_set_destroy_function(GAttrib *attrib,
 guint g_attrib_send(GAttrib *attrib, guint8 opcode, const guint8 *pdu,
 				guint16 len, GAttribResultFunc func,
 				gpointer user_data, GDestroyNotify notify);
+guint g_attrib_send_seq(GAttrib *attrib, gboolean compound, guint id,
+				guint8 opcode, const guint8 *pdu,
+				guint16 len, GAttribResultFunc func,
+				gpointer user_data, GDestroyNotify notify);
 gboolean g_attrib_cancel(GAttrib *attrib, guint id);
 gboolean g_attrib_cancel_all(GAttrib *attrib);
 
-- 
1.7.2.2

Re: [RFC 1/2] Add g_attrib_send_seq()

[Claudio Takahasi]

Hi Brian,

On Fri, Dec 17, 2010 at 7:48 PM, Brian Gix <bgix@codeaurora.org> wrote:
> Add g_attrib_send_seq() as an extension to g_attrib_send().
>        g_attrib_send_seq() functionally queues an entire
>        "GATT Procedure" as defined in the BT Core v4.0.
>        The intention is that the full procedure is run
>        to completion before the next GATT Procedure is
>        started. Subsequent ATT requests to a continuing
>        procedure are added to the head of the Attrib queue.
>
> Fix g_attrib_send() to be the degenerative case of g_attrib_send_seq()
>        This function now chains to g_attrib_send_seq() with arguments
>        indicating that it is *not* a compound (multi-step) GATT
>        procedure, but rather that the entire procedure is performed
>        with a single ATT opcode request/response.
>
> Fix received_data() to recognize that incoming response is (or isn't)
>        part of a compound Procedure, so that it waits for additional
>        requests before servicing the Attrib queue.
> ---
>  attrib/gattrib.c |   44 ++++++++++++++++++++++++++++++++++++--------
>  attrib/gattrib.h |    4 ++++
>  2 files changed, 40 insertions(+), 8 deletions(-)
>
> diff --git a/attrib/gattrib.c b/attrib/gattrib.c
> index eace01b..8ef5d92 100644
> --- a/attrib/gattrib.c
> +++ b/attrib/gattrib.c
> @@ -58,6 +58,7 @@ struct command {
>        guint16 len;
>        guint8 expected;
>        gboolean sent;
> +       gboolean compound;
>        GAttribResultFunc func;
>        gpointer user_data;
>        GDestroyNotify notify;
> @@ -285,6 +286,7 @@ static gboolean received_data(GIOChannel *io, GIOCondition cond, gpointer data)
>        uint8_t buf[512], status;
>        gsize len;
>        GIOStatus iostat;
> +       gboolean compound = FALSE;
Intialization is not necessary.

>
>        if (cond & (G_IO_HUP | G_IO_ERR | G_IO_NVAL)) {
>                attrib->read_watch = 0;
> @@ -319,6 +321,8 @@ static gboolean received_data(GIOChannel *io, GIOCondition cond, gpointer data)
>                return attrib->events != NULL;
>        }
>
> +       compound = cmd->compound;
> +
>        if (buf[0] == ATT_OP_ERROR) {
>                status = buf[4];
>                goto done;
> @@ -332,7 +336,8 @@ static gboolean received_data(GIOChannel *io, GIOCondition cond, gpointer data)
>        status = 0;
>
>  done:
> -       if (attrib->queue && g_queue_is_empty(attrib->queue) == FALSE)
> +       if (!compound && attrib->queue &&
> +                       g_queue_is_empty(attrib->queue) == FALSE)
>                wake_up_sender(attrib);
>
>        if (cmd) {
> @@ -340,6 +345,11 @@ done:
>                        cmd->func(status, buf, len, cmd->user_data);
>
>                command_destroy(cmd);
> +
> +               if (compound && attrib->queue &&
> +                       g_queue_is_empty(attrib->queue) == FALSE)
> +                       wake_up_sender(attrib);
> +
Any chance to change the logic to avoid duplicated verification?
No mater the value of "compound" wake_up_sender() is always called. Is
the order important?

>        }
>
>        return TRUE;
> @@ -367,12 +377,16 @@ GAttrib *g_attrib_new(GIOChannel *io)
>        return g_attrib_ref(attrib);
>  }
>
> -guint g_attrib_send(GAttrib *attrib, guint8 opcode, const guint8 *pdu,
> -                               guint16 len, GAttribResultFunc func,
> -                               gpointer user_data, GDestroyNotify notify)
> +guint g_attrib_send_seq(GAttrib *attrib, gboolean compound, guint id,
> +                               guint8 opcode, const guint8 *pdu, guint16 len,
> +                               GAttribResultFunc func, gpointer user_data,
> +                               GDestroyNotify notify)
>  {
>        struct command *c;
>
> +       if (attrib == NULL || attrib->queue == NULL)
> +               return 0;
> +
Is it necessary to check if queue is NULL?

>        c = g_try_new0(struct command, 1);
>        if (c == NULL)
>                return 0;
> @@ -385,16 +399,30 @@ guint g_attrib_send(GAttrib *attrib, guint8 opcode, const guint8 *pdu,
>        c->func = func;
>        c->user_data = user_data;
>        c->notify = notify;
> -       c->id = ++attrib->next_cmd_id;
> +       c->compound = compound;
>
> -       g_queue_push_tail(attrib->queue, c);
> +       if (id) {
> +               c->id = id;
> +               g_queue_push_head(attrib->queue, c);
> +       } else {
> +               c->id = ++attrib->next_cmd_id;
> +               g_queue_push_tail(attrib->queue, c);
>
> -       if (g_queue_get_length(attrib->queue) == 1)
> -               wake_up_sender(attrib);
> +               if (g_queue_get_length(attrib->queue) == 1)
> +                       wake_up_sender(attrib);
> +       }
>
>        return c->id;
>  }
>
> +guint g_attrib_send(GAttrib *attrib, guint8 opcode, const guint8 *pdu,
> +                               guint16 len, GAttribResultFunc func,
> +                               gpointer user_data, GDestroyNotify notify)
> +{
> +       return g_attrib_send_seq(attrib, FALSE, 0, opcode,
> +                               pdu, len, func, user_data, notify);
Coding style issue here.

Claudio

Re: [RFC 1/2] Add g_attrib_send_seq()

[Brian Gix]

Thanks Claudio,

On Wed, 2010-12-22 at 19:29 -0300, Claudio Takahasi wrote:
> Hi Brian,
> 
> On Fri, Dec 17, 2010 at 7:48 PM, Brian Gix <bgix@codeaurora.org> wrote:
> > Add g_attrib_send_seq() as an extension to g_attrib_send().
> >        g_attrib_send_seq() functionally queues an entire
> >        "GATT Procedure" as defined in the BT Core v4.0.
> >        The intention is that the full procedure is run
> >        to completion before the next GATT Procedure is
> >        started. Subsequent ATT requests to a continuing
> >        procedure are added to the head of the Attrib queue.
> >
> > Fix g_attrib_send() to be the degenerative case of g_attrib_send_seq()
> >        This function now chains to g_attrib_send_seq() with arguments
> >        indicating that it is *not* a compound (multi-step) GATT
> >        procedure, but rather that the entire procedure is performed
> >        with a single ATT opcode request/response.
> >
> > Fix received_data() to recognize that incoming response is (or isn't)
> >        part of a compound Procedure, so that it waits for additional
> >        requests before servicing the Attrib queue.
> > ---
> >  attrib/gattrib.c |   44 ++++++++++++++++++++++++++++++++++++--------
> >  attrib/gattrib.h |    4 ++++
> >  2 files changed, 40 insertions(+), 8 deletions(-)
> >
> > diff --git a/attrib/gattrib.c b/attrib/gattrib.c
> > index eace01b..8ef5d92 100644
> > --- a/attrib/gattrib.c
> > +++ b/attrib/gattrib.c
> > @@ -58,6 +58,7 @@ struct command {
> >        guint16 len;
> >        guint8 expected;
> >        gboolean sent;
> > +       gboolean compound;
> >        GAttribResultFunc func;
> >        gpointer user_data;
> >        GDestroyNotify notify;
> > @@ -285,6 +286,7 @@ static gboolean received_data(GIOChannel *io, GIOCondition cond, gpointer data)
> >        uint8_t buf[512], status;
> >        gsize len;
> >        GIOStatus iostat;
> > +       gboolean compound = FALSE;
> Intialization is not necessary.

There is a path to it's usage (a "goto done") just after
g_io_channel_read_chars call.

> 
> >
> >        if (cond & (G_IO_HUP | G_IO_ERR | G_IO_NVAL)) {
> >                attrib->read_watch = 0;
> > @@ -319,6 +321,8 @@ static gboolean received_data(GIOChannel *io, GIOCondition cond, gpointer data)
> >                return attrib->events != NULL;
> >        }
> >
> > +       compound = cmd->compound;
> > +
> >        if (buf[0] == ATT_OP_ERROR) {
> >                status = buf[4];
> >                goto done;
> > @@ -332,7 +336,8 @@ static gboolean received_data(GIOChannel *io, GIOCondition cond, gpointer data)
> >        status = 0;
> >
> >  done:
> > -       if (attrib->queue && g_queue_is_empty(attrib->queue) == FALSE)
> > +       if (!compound && attrib->queue &&
> > +                       g_queue_is_empty(attrib->queue) == FALSE)
> >                wake_up_sender(attrib);
> >
> >        if (cmd) {
> > @@ -340,6 +345,11 @@ done:
> >                        cmd->func(status, buf, len, cmd->user_data);
> >
> >                command_destroy(cmd);
> > +
> > +               if (compound && attrib->queue &&
> > +                       g_queue_is_empty(attrib->queue) == FALSE)
> > +                       wake_up_sender(attrib);
> > +
> Any chance to change the logic to avoid duplicated verification?
> No mater the value of "compound" wake_up_sender() is always called. Is
> the order important?

This is probably the ugliest part of my additions.

Here is the problem:
I wanted to delay the call to wake_up_sender() until *after* the
invocation of the cmd->func() callback, because if the the subsequent
ATT command is to be sent next, it needs to be added to the head of the
queue before the sender is woken up. wake_up_sender calls
g_io_add_watch_full(), and I am unsure how the process threading works
here, so I don't know if received_data() is going to be preempted (and
send the current queue head) by that call.

I do see that the call to wake_up_sender is called by the g_attrib_send*
function, but only if the addition was to a previously empty queue (so
that the count is now 1).  It appeared to me that care was taken by the
original coder to make sure wake_up_sender() was called exactly once for
each ATT command that was queued, and since only a single ATT command
can be outstanding at a time, I believe this is the correct
functionality.

Also, there is the problem that just because a GATT procedure *may* be
compound, there is no guarantee that it *will* need to queue an
additional ATT command. In the read characteristic case, for instance,
if the result is less than 22 bytes, it will not end up being truly
compound at all. However, we do not know that until we have processed
the first result.

I could get around the "two code blocks that do almost the same thing"
problem by the following changes:
	
1. In g_attrib_send_seq(), pull "if length == 1 then wake_up_sender"
	block out one level, so that it is invoked for both the
	push_head and push_tail cases.

2. In received_data(), Check the queue member count prior to invoking
	cmd->func, and then calling wake_up_sender afterwards if the
	prior count was non-zero. We can't check the current count
	at that point, because wake_up_sender may have already been
	called in g_attrib_send_seq.

3. Then, we can totally eliminate the "compound" local var, and
	structure member, making the whole thing cleaner.


Should I try that?

> 
> >        }
> >
> >        return TRUE;
> > @@ -367,12 +377,16 @@ GAttrib *g_attrib_new(GIOChannel *io)
> >        return g_attrib_ref(attrib);
> >  }
> >
> > -guint g_attrib_send(GAttrib *attrib, guint8 opcode, const guint8 *pdu,
> > -                               guint16 len, GAttribResultFunc func,
> > -                               gpointer user_data, GDestroyNotify notify)
> > +guint g_attrib_send_seq(GAttrib *attrib, gboolean compound, guint id,
> > +                               guint8 opcode, const guint8 *pdu, guint16 len,
> > +                               GAttribResultFunc func, gpointer user_data,
> > +                               GDestroyNotify notify)
> >  {
> >        struct command *c;
> >
> > +       if (attrib == NULL || attrib->queue == NULL)
> > +               return 0;
> > +
> Is it necessary to check if queue is NULL?

I don't know.  What happens if g_queue_push_head or g_queue_push_tail is
passed a NULL pointer for the queue? I noticed that it is checked by
g_attrib_cancel, and was unsure as to why it would be different.

> 
> >        c = g_try_new0(struct command, 1);
> >        if (c == NULL)
> >                return 0;
> > @@ -385,16 +399,30 @@ guint g_attrib_send(GAttrib *attrib, guint8 opcode, const guint8 *pdu,
> >        c->func = func;
> >        c->user_data = user_data;
> >        c->notify = notify;
> > -       c->id = ++attrib->next_cmd_id;
> > +       c->compound = compound;
> >
> > -       g_queue_push_tail(attrib->queue, c);
> > +       if (id) {
> > +               c->id = id;
> > +               g_queue_push_head(attrib->queue, c);
> > +       } else {
> > +               c->id = ++attrib->next_cmd_id;
> > +               g_queue_push_tail(attrib->queue, c);
> >
> > -       if (g_queue_get_length(attrib->queue) == 1)
> > -               wake_up_sender(attrib);
> > +               if (g_queue_get_length(attrib->queue) == 1)
> > +                       wake_up_sender(attrib);
> > +       }
> >
> >        return c->id;
> >  }
> >
> > +guint g_attrib_send(GAttrib *attrib, guint8 opcode, const guint8 *pdu,
> > +                               guint16 len, GAttribResultFunc func,
> > +                               gpointer user_data, GDestroyNotify notify)
> > +{
> > +       return g_attrib_send_seq(attrib, FALSE, 0, opcode,
> > +                               pdu, len, func, user_data, notify);
> Coding style issue here.

What is the issue?  I saw in g_attrib_new() that a return of a call to
g_attrib_ref was tail-chain-returned.

> 
> Claudio

Thanks,

-- 
Brian Gix
bgix@codeaurora.org
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum

Re: [RFC 1/2] Add g_attrib_send_seq()

[Vinicius Costa Gomes]

Hi Brian,

First of all, sorry for the delay.

On 15:13 Wed 22 Dec, Brian Gix wrote:
> Thanks Claudio,
> 
> On Wed, 2010-12-22 at 19:29 -0300, Claudio Takahasi wrote:
> > Hi Brian,
> > 
> > On Fri, Dec 17, 2010 at 7:48 PM, Brian Gix <bgix@codeaurora.org> wrote:
> > > Add g_attrib_send_seq() as an extension to g_attrib_send().
> > >        g_attrib_send_seq() functionally queues an entire
> > >        "GATT Procedure" as defined in the BT Core v4.0.
> > >        The intention is that the full procedure is run
> > >        to completion before the next GATT Procedure is
> > >        started. Subsequent ATT requests to a continuing
> > >        procedure are added to the head of the Attrib queue.
> > >
> > > Fix g_attrib_send() to be the degenerative case of g_attrib_send_seq()
> > >        This function now chains to g_attrib_send_seq() with arguments
> > >        indicating that it is *not* a compound (multi-step) GATT
> > >        procedure, but rather that the entire procedure is performed
> > >        with a single ATT opcode request/response.
> > >
> > > Fix received_data() to recognize that incoming response is (or isn't)
> > >        part of a compound Procedure, so that it waits for additional
> > >        requests before servicing the Attrib queue.
> > > ---
> > >  attrib/gattrib.c |   44 ++++++++++++++++++++++++++++++++++++--------
> > >  attrib/gattrib.h |    4 ++++
> > >  2 files changed, 40 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/attrib/gattrib.c b/attrib/gattrib.c
> > > index eace01b..8ef5d92 100644
> > > --- a/attrib/gattrib.c
> > > +++ b/attrib/gattrib.c
> > > @@ -58,6 +58,7 @@ struct command {
> > >        guint16 len;
> > >        guint8 expected;
> > >        gboolean sent;
> > > +       gboolean compound;
> > >        GAttribResultFunc func;
> > >        gpointer user_data;
> > >        GDestroyNotify notify;
> > > @@ -285,6 +286,7 @@ static gboolean received_data(GIOChannel *io, GIOCondition cond, gpointer data)
> > >        uint8_t buf[512], status;
> > >        gsize len;
> > >        GIOStatus iostat;
> > > +       gboolean compound = FALSE;
> > Intialization is not necessary.
> 
> There is a path to it's usage (a "goto done") just after
> g_io_channel_read_chars call.
> 
> > 
> > >
> > >        if (cond & (G_IO_HUP | G_IO_ERR | G_IO_NVAL)) {
> > >                attrib->read_watch = 0;
> > > @@ -319,6 +321,8 @@ static gboolean received_data(GIOChannel *io, GIOCondition cond, gpointer data)
> > >                return attrib->events != NULL;
> > >        }
> > >
> > > +       compound = cmd->compound;
> > > +
> > >        if (buf[0] == ATT_OP_ERROR) {
> > >                status = buf[4];
> > >                goto done;
> > > @@ -332,7 +336,8 @@ static gboolean received_data(GIOChannel *io, GIOCondition cond, gpointer data)
> > >        status = 0;
> > >
> > >  done:
> > > -       if (attrib->queue && g_queue_is_empty(attrib->queue) == FALSE)
> > > +       if (!compound && attrib->queue &&
> > > +                       g_queue_is_empty(attrib->queue) == FALSE)
> > >                wake_up_sender(attrib);
> > >
> > >        if (cmd) {
> > > @@ -340,6 +345,11 @@ done:
> > >                        cmd->func(status, buf, len, cmd->user_data);
> > >
> > >                command_destroy(cmd);
> > > +
> > > +               if (compound && attrib->queue &&
> > > +                       g_queue_is_empty(attrib->queue) == FALSE)
> > > +                       wake_up_sender(attrib);
> > > +
> > Any chance to change the logic to avoid duplicated verification?
> > No mater the value of "compound" wake_up_sender() is always called. Is
> > the order important?
> 
> This is probably the ugliest part of my additions.
> 
> Here is the problem:
> I wanted to delay the call to wake_up_sender() until *after* the
> invocation of the cmd->func() callback, because if the the subsequent
> ATT command is to be sent next, it needs to be added to the head of the
> queue before the sender is woken up. wake_up_sender calls
> g_io_add_watch_full(), and I am unsure how the process threading works
> here, so I don't know if received_data() is going to be preempted (and
> send the current queue head) by that call.

It is really simple, it was not meant to work on multi-threaded enviroments.
receeived_data() is going to be called after control is back to the mainloop. 
It is not meant to be reentrant.

> 
> I do see that the call to wake_up_sender is called by the g_attrib_send*
> function, but only if the addition was to a previously empty queue (so
> that the count is now 1).  It appeared to me that care was taken by the
> original coder to make sure wake_up_sender() was called exactly once for
> each ATT command that was queued, and since only a single ATT command
> can be outstanding at a time, I believe this is the correct
> functionality.

I think that Claudio was referring just to the changes that you made to
received_data(). And considering what I said above (that it's not meant to be
thread-safe) I think that received_data could stay unchanged.

And g_attrib_send_seq() could have a bug when the command at the head was 
already sent but is waiting for a response. If the command is always added to
the head of the queue, could it be so the order of the command of a compound 
procedure is inverted?

And taking a closer look at the spec, it was clear to me that only a higher
level profile (above GATT) is able to know that a characteristic needs to be
read by using the "Read Long Characteristic Value" procedure -- which I think is
what brought this discussion, right? or you already have another need for 
these procedures? -- Which gives me the impression that this should
be dealt by the profile implementation. Which gives us more time to think about
how to implement this correctly ;-) in case the need arrise.

> 
> Also, there is the problem that just because a GATT procedure *may* be
> compound, there is no guarantee that it *will* need to queue an
> additional ATT command. In the read characteristic case, for instance,
> if the result is less than 22 bytes, it will not end up being truly
> compound at all. However, we do not know that until we have processed
> the first result.
> 
> I could get around the "two code blocks that do almost the same thing"
> problem by the following changes:
> 	
> 1. In g_attrib_send_seq(), pull "if length == 1 then wake_up_sender"
> 	block out one level, so that it is invoked for both the
> 	push_head and push_tail cases.
> 
> 2. In received_data(), Check the queue member count prior to invoking
> 	cmd->func, and then calling wake_up_sender afterwards if the
> 	prior count was non-zero. We can't check the current count
> 	at that point, because wake_up_sender may have already been
> 	called in g_attrib_send_seq.
> 
> 3. Then, we can totally eliminate the "compound" local var, and
> 	structure member, making the whole thing cleaner.
> 
> 
> Should I try that?
> 
> > 
> > >        }
> > >
> > >        return TRUE;
> > > @@ -367,12 +377,16 @@ GAttrib *g_attrib_new(GIOChannel *io)
> > >        return g_attrib_ref(attrib);
> > >  }
> > >
> > > -guint g_attrib_send(GAttrib *attrib, guint8 opcode, const guint8 *pdu,
> > > -                               guint16 len, GAttribResultFunc func,
> > > -                               gpointer user_data, GDestroyNotify notify)
> > > +guint g_attrib_send_seq(GAttrib *attrib, gboolean compound, guint id,
> > > +                               guint8 opcode, const guint8 *pdu, guint16 len,
> > > +                               GAttribResultFunc func, gpointer user_data,
> > > +                               GDestroyNotify notify)
> > >  {
> > >        struct command *c;
> > >
> > > +       if (attrib == NULL || attrib->queue == NULL)
> > > +               return 0;
> > > +
> > Is it necessary to check if queue is NULL?
> 
> I don't know.  What happens if g_queue_push_head or g_queue_push_tail is
> passed a NULL pointer for the queue? I noticed that it is checked by
> g_attrib_cancel, and was unsure as to why it would be different.
> 

It would not crash, but I think that GLib would give an warning. Doing the
check is correct.

> > 
> > >        c = g_try_new0(struct command, 1);
> > >        if (c == NULL)
> > >                return 0;
> > > @@ -385,16 +399,30 @@ guint g_attrib_send(GAttrib *attrib, guint8 opcode, const guint8 *pdu,
> > >        c->func = func;
> > >        c->user_data = user_data;
> > >        c->notify = notify;
> > > -       c->id = ++attrib->next_cmd_id;
> > > +       c->compound = compound;
> > >
> > > -       g_queue_push_tail(attrib->queue, c);
> > > +       if (id) {
> > > +               c->id = id;
> > > +               g_queue_push_head(attrib->queue, c);
> > > +       } else {
> > > +               c->id = ++attrib->next_cmd_id;
> > > +               g_queue_push_tail(attrib->queue, c);
> > >
> > > -       if (g_queue_get_length(attrib->queue) == 1)
> > > -               wake_up_sender(attrib);
> > > +               if (g_queue_get_length(attrib->queue) == 1)
> > > +                       wake_up_sender(attrib);
> > > +       }
> > >
> > >        return c->id;
> > >  }
> > >
> > > +guint g_attrib_send(GAttrib *attrib, guint8 opcode, const guint8 *pdu,
> > > +                               guint16 len, GAttribResultFunc func,
> > > +                               gpointer user_data, GDestroyNotify notify)
> > > +{
> > > +       return g_attrib_send_seq(attrib, FALSE, 0, opcode,
> > > +                               pdu, len, func, user_data, notify);
> > Coding style issue here.
> 
> What is the issue?  I saw in g_attrib_new() that a return of a call to
> g_attrib_ref was tail-chain-returned.

I think it was more about indentation, something like this looks better:

return g_attrib_send_seq(attrib, FALSE, 0, opcode, pdu, len, func,
						user_data, notify);

> 
> > 
> > Claudio
> 
> Thanks,
> 
> -- 
> Brian Gix
> bgix@codeaurora.org
> Employee of Qualcomm Innovation Center, Inc.
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Cheers,
--
Vinicius

Re: [RFC 1/2] Add g_attrib_send_seq()

[bgix]

Hi Vinicius,

I'm sorry, but I submitted patches before I was aware that you had
responded.

On Tue, 2010-12-28 at 18:48 -0300, Vinicius Costa Gomes wrote:
> Hi Brian,
> 
> First of all, sorry for the delay.
> 
[...]
> > I wanted to delay the call to wake_up_sender() until *after* the
> > invocation of the cmd->func() callback, because if the the subsequent
> > ATT command is to be sent next, it needs to be added to the head of the
> > queue before the sender is woken up. wake_up_sender calls
> > g_io_add_watch_full(), and I am unsure how the process threading works
> > here, so I don't know if received_data() is going to be preempted (and
> > send the current queue head) by that call.
> 
> It is really simple, it was not meant to work on multi-threaded enviroments.
> receeived_data() is going to be called after control is back to the mainloop. 
> It is not meant to be reentrant.

Understood.

> 
> > 
> > I do see that the call to wake_up_sender is called by the g_attrib_send*
> > function, but only if the addition was to a previously empty queue (so
> > that the count is now 1).  It appeared to me that care was taken by the
> > original coder to make sure wake_up_sender() was called exactly once for
> > each ATT command that was queued, and since only a single ATT command
> > can be outstanding at a time, I believe this is the correct
> > functionality.
> 
> I think that Claudio was referring just to the changes that you made to
> received_data(). And considering what I said above (that it's not meant to be
> thread-safe) I think that received_data could stay unchanged.
> 
> And g_attrib_send_seq() could have a bug when the command at the head was 
> already sent but is waiting for a response. If the command is always added to
> the head of the queue, could it be so the order of the command of a compound 
> procedure is inverted?

In the patches I resent, I eliminated g_attrib_send_seq in favour of a
single API with the original name (g_attrib_send) which has an
additional parameter.

The intention of the version of the call to enqueue at the head of the
queue, is that it take place within the scope of the cmd->func()
callback.  Perhaps there is a way to code for that, by checking within
g_attrib_send that the head of the queue has not been sent, but from
within cmd->func() it should definitely not have sent the next pkt,
assuming what you stated about re-entrancy above holds true.

> 
> And taking a closer look at the spec, it was clear to me that only a higher
> level profile (above GATT) is able to know that a characteristic needs to be
> read by using the "Read Long Characteristic Value" procedure -- which I think is
> what brought this discussion, right? or you already have another need for 
> these procedures? -- Which gives me the impression that this should
> be dealt by the profile implementation. Which gives us more time to think about
> how to implement this correctly ;-) in case the need arrise.

I don't believe this is correct. Reading a Characteristic Value is a
GATT procedure, not a Profile procedure.  And any String based
Characteristic is a candidate for being longer than the LE default MTU.
The Reading of Long characteristics as a procedure is enumerated in the
GATT specification.

> 
[...]
> > 
> > I don't know.  What happens if g_queue_push_head or g_queue_push_tail is
> > passed a NULL pointer for the queue? I noticed that it is checked by
> > g_attrib_cancel, and was unsure as to why it would be different.
> > 
> 
> It would not crash, but I think that GLib would give an warning. Doing the
> check is correct.

And I just removed it from my patch.  oops.

> 
[...]
> > > > +guint g_attrib_send(GAttrib *attrib, guint8 opcode, const guint8 *pdu,
> > > > +                               guint16 len, GAttribResultFunc func,
> > > > +                               gpointer user_data, GDestroyNotify notify)
> > > > +{
> > > > +       return g_attrib_send_seq(attrib, FALSE, 0, opcode,
> > > > +                               pdu, len, func, user_data, notify);
> > > Coding style issue here.
> > 
> > What is the issue?  I saw in g_attrib_new() that a return of a call to
> > g_attrib_ref was tail-chain-returned.
> 
> I think it was more about indentation, something like this looks better:
> 
> return g_attrib_send_seq(attrib, FALSE, 0, opcode, pdu, len, func,
> 						user_data, notify);
> 

OK.

-- 
Brian Gix
bgix@codeaurora.org
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum

Re: [RFC 1/2] Add g_attrib_send_seq()

[Brian Gix]

Hi Vinicius,

On Tue, 2010-12-28 at 18:48 -0300, Vinicius Costa Gomes wrote:
> Hi Brian,
> 
> First of all, sorry for the delay.
> 
[...]
> 
> And taking a closer look at the spec, it was clear to me that only a higher
> level profile (above GATT) is able to know that a characteristic needs to be
> read by using the "Read Long Characteristic Value" procedure -- which I think is
> what brought this discussion, right? or you already have another need for 
> these procedures? -- Which gives me the impression that this should
> be dealt by the profile implementation. Which gives us more time to think about
> how to implement this correctly ;-) in case the need arrise.

It is true that the "Read Long Characteristic Value" is defined as a
distinct GATT procedure from the "Read Characteristic Value" procedure
(section 4.8.3 of Vol 3, as opposed to section 4.3.1). However it was
purposefully designed such that the two procedures could be overloaded
onto a single API such as I have done with the patch(es). (See the Note
just above Figure 4.10)

I believe that overloading the "Read Characteristic Value" in this way
is a valuable extension for a couple of reasons. It simplifies the API
(including the DBus API) by not having two APIs that do essentially the
same thing. It simplifies the API by not requiring that a client know
information ahead of time, such as the length of strings, on a remote
server.

The fact that this GATT procedure is Optional rather than Mandatory is
more a function of common sense rather than a statement of whether it is
explicitly needed for a particular profile.  For instance a Client may
not have the ability to display strings, so there is no point from a
specification standpoint of forcing it to read data it is not useful to
it.  And if a Server with limited resources decides to limit
Characteristic Values to 22 octets or less, there is no point in
requiring it to respond successfully to a READ_BLOB request.

However, as BlueZ will be heavily used, particularly as an LE client, in
environments with significant resources (Tablets, Cellphones, Desktops),
I think it would be short sighted to require each profile and/or each
client app to handle Long and Short Characteristic Values differently,
unless there is a compelling technical reason.  I personally find the
compelling technical reason to be behind overloading the single, simple
API.

[...]
> 
> Cheers,
> --
> Vinicius

-- 
Brian Gix
bgix@codeaurora.org
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum

[RFC 2/2] Fix gatt_read_char() to work with long attributes

[Brian Gix]

Fix gatt_read_char() so that it can correctly read an attribute longer
 	than (MTU-1) octets long. This is intended to be the first
 	use of the compound (multi-step) GATT Procedure mechanism
 	g_attrib_send_seq(), which will run the GATT procedure to completion
 	before executing any additional GATT Procedures. Functionaly, it
 	will check the result length of the original READ_REQ, and if
 	it is equal to the DEAFULT_MTU (23) octets long, it will make
 	a series of READ_BLOB_REQs until the entire remote attribute has
 	been read, after which it returns the result to the original
 	caller as if the data had been retrieved in a single ATT resp.

---
 attrib/gatt.c |  134 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 132 insertions(+), 2 deletions(-)

diff --git a/attrib/gatt.c b/attrib/gatt.c
index bca8b49..3a89b30 100644
--- a/attrib/gatt.c
+++ b/attrib/gatt.c
@@ -97,15 +97,145 @@ guint gatt_read_char_by_uuid(GAttrib *attrib, uint16_t start, uint16_t end,
 					pdu, plen, func, user_data, NULL);
 }
 
+struct read_long_data {
+	GAttrib *attrib;
+	GAttribResultFunc func;
+	gpointer user_data;
+	guint8 *buffer;
+	guint16 size;
+	guint16 handle;
+	guint id;
+	guint8 ref;
+};
+
+static void read_long_destroy(gpointer user_data)
+{
+	struct read_long_data *long_read = user_data;
+
+	if (--long_read->ref)
+		return;
+
+	if (long_read->buffer != NULL)
+		g_free(long_read->buffer);
+
+	g_free(long_read);
+}
+
+static void read_blob_helper(guint8 status, const guint8 *rpdu, guint16 rlen,
+							gpointer user_data)
+{
+	struct read_long_data *long_read = user_data;
+	uint8_t pdu[ATT_DEFAULT_MTU];
+	guint8 *tmp;
+	guint16 plen;
+	guint id;
+
+	if (status == ATT_ECODE_ATTR_NOT_LONG ||
+					status == ATT_ECODE_INVALID_OFFSET) {
+		status = 0;
+		goto done;
+	}
+
+	if (status != 0 || rlen == 1)
+		goto done;
+
+	tmp = g_try_realloc(long_read->buffer, long_read->size + rlen - 1);
+
+	if (tmp == NULL) {
+		status = ATT_ECODE_INSUFF_RESOURCES;
+		goto done;
+	}
+
+	memcpy(&tmp[long_read->size], &rpdu[1], rlen - 1);
+	long_read->buffer = tmp;
+	long_read->size += rlen - 1;
+
+	if (rlen < ATT_DEFAULT_MTU)
+		goto done;
+
+	plen = enc_read_blob_req(long_read->handle, long_read->size - 1,
+							pdu, sizeof(pdu));
+	id = g_attrib_send_seq(long_read->attrib, TRUE, long_read->id,
+				ATT_OP_READ_BLOB_REQ, pdu, plen,
+				read_blob_helper, long_read, read_long_destroy);
+
+	if (id != 0) {
+		long_read->ref++;
+		return;
+	}
+
+	status = ATT_ECODE_IO;
+
+done:
+	long_read->func(status, long_read->buffer, long_read->size,
+							long_read->user_data);
+}
+
+static void read_char_helper(guint8 status, const guint8 *rpdu,
+					guint16 rlen, gpointer user_data)
+{
+	struct read_long_data *long_read = user_data;
+	uint8_t pdu[ATT_DEFAULT_MTU];
+	guint16 plen;
+	guint id;
+
+	if (status != 0 || rlen < ATT_DEFAULT_MTU)
+		goto done;
+
+	long_read->buffer = g_malloc(rlen);
+
+	if (long_read->buffer == NULL)
+		goto done;
+
+	memcpy(long_read->buffer, rpdu, rlen);
+	long_read->size = rlen;
+
+	plen = enc_read_blob_req(long_read->handle, rlen - 1, pdu, sizeof(pdu));
+	id = g_attrib_send_seq(long_read->attrib, TRUE, long_read->id,
+			ATT_OP_READ_BLOB_REQ, pdu, plen, read_blob_helper,
+			long_read, read_long_destroy);
+
+	if (id != 0) {
+		long_read->ref++;
+		return;
+	}
+
+	status = ATT_ECODE_IO;
+
+done:
+	long_read->func(status, rpdu, rlen, long_read->user_data);
+}
+
 guint gatt_read_char(GAttrib *attrib, uint16_t handle, GAttribResultFunc func,
 							gpointer user_data)
 {
 	uint8_t pdu[ATT_DEFAULT_MTU];
 	guint16 plen;
+	guint id;
+	struct read_long_data *long_read;
+
+	long_read = g_try_new0(struct read_long_data, 1);
+
+	if (long_read == NULL)
+		return 0;
+
+	long_read->attrib = attrib;
+	long_read->func = func;
+	long_read->user_data = user_data;
+	long_read->handle = handle;
 
 	plen = enc_read_req(handle, pdu, sizeof(pdu));
-	return g_attrib_send(attrib, ATT_OP_READ_REQ, pdu, plen, func,
-							user_data, NULL);
+	id = g_attrib_send_seq(attrib, TRUE, 0, ATT_OP_READ_REQ, pdu, plen,
+				read_char_helper, long_read, read_long_destroy);
+
+	if (id == 0)
+		g_free(long_read);
+	else {
+		long_read->ref++;
+		long_read->id = id;
+	}
+
+	return id;
 }
 
 guint gatt_write_char(GAttrib *attrib, uint16_t handle, uint8_t *value,
-- 
1.7.2.2

Re: [RFC 0/1] Implement Compound (Multi-step) GATT Procedure Support

[Brian Gix]

Hi Claudio & Vinicius,


On Fri, 2010-12-17 at 14:48 -0800, Brian Gix wrote:
> The following two proposed patches implement a method for correctly
> staging GATT procedures that require multiple ATT req/resp exchanges.

If you guys get a chance, could you consider this proposed change
to gattrib.[ch] and the subsequent change to gatt.c? It differs from my
initial proposed patch which subverted the gattrib queuing mechanism. It
now lets each ATT command run to completion. It also fixes the memory
leak potential that Vinicius spotted.  I think it is important that you
give it at least a cursory look, because you guys know more about bluez
GATT than anyone else here at this point.  Otherwise, I will resubmit
this version as a patch request. -- thx

> 
> The first RFC / PATCH is gattrib.[ch], which allows the caller to specify
> a gboolean indicating that the command being queued is Compound
> (even if the procedure ends up being single/atomic) and a queue ID
> number, which allows the caller to cancel the GATT procedure at any
> stage of the transaction.
> 
> IF -
> The ATT opcode being queued is specified as compound, the resulting response
> will not cause the next item in the queue to be sent until *after* the
> response has been forwarded to the caller via the response callback.
> 
> IF -
> The ATT opcode being queued is *not* compound, the resulting response
> will service the pkt queue immediately (legacy functionality).
> 
> IF -
> The ID passed to g_attrib_send_seq is ZERO, the command pkt will
> be added to the TAIL of the queue, and a new ID assigned to it.
> (Legacy functionality)
> 
> IF -
> The ID passed to g_attrib_send_seq is NON-ZERO, the command pkt
> will be added to the HEAD of the queue, causing it to be the next
> pkt sent to the remote device.  The NON-ZERO ID is also then able
> to cancel the command.
> 
> 
> The second RFC / PATCH is to gatt.c. It modifies the existing gatt_read_char()
> function to recognize the need for a compound Read procedure, and implements
> it as a READ_REQ + READ_BLOB_REQ + READ_BLOB_REQ ...<etc> as needed, using
> the g_attrib_send_seq() logic from the first RFC / PATCH.
> 

Re: [RFC 0/1] Implement Compound (Multi-step) GATT Procedure Support

[Claudio Takahasi]

Hi Brian,

On Tue, Dec 21, 2010 at 2:25 PM, Brian Gix <bgix@codeaurora.org> wrote:
> Hi Claudio & Vinicius,
>
>
> On Fri, 2010-12-17 at 14:48 -0800, Brian Gix wrote:
>> The following two proposed patches implement a method for correctly
>> staging GATT procedures that require multiple ATT req/resp exchanges.
>
> If you guys get a chance, could you consider this proposed change
> to gattrib.[ch] and the subsequent change to gatt.c? It differs from my
> initial proposed patch which subverted the gattrib queuing mechanism. It
> now lets each ATT command run to completion. It also fixes the memory
> leak potential that Vinicius spotted.  I think it is important that you
> give it at least a cursory look, because you guys know more about bluez
> GATT than anyone else here at this point.  Otherwise, I will resubmit
> this version as a patch request. -- thx
>

ok. We can review your patches.
Could you please send patches to change gatttool to support this new feature?
How are you testing it? Do you have a modified BlueZ attribute server
or another GATT server?

Claudio

Re: [RFC 0/1] Implement Compound (Multi-step) GATT Procedure Support

[Brian Gix]

Hi Caudio,

On Tue, 2010-12-21 at 16:25 -0300, Claudio Takahasi wrote:
> Hi Brian,
> 
> On Tue, Dec 21, 2010 at 2:25 PM, Brian Gix <bgix@codeaurora.org> wrote:
> > Hi Claudio & Vinicius,
> >
> >
> > On Fri, 2010-12-17 at 14:48 -0800, Brian Gix wrote:
> >> The following two proposed patches implement a method for correctly
> >> staging GATT procedures that require multiple ATT req/resp exchanges.
> >
> > If you guys get a chance, could you consider this proposed change
> > to gattrib.[ch] and the subsequent change to gatt.c? It differs from my
> > initial proposed patch which subverted the gattrib queuing mechanism. It
> > now lets each ATT command run to completion. It also fixes the memory
> > leak potential that Vinicius spotted.  I think it is important that you
> > give it at least a cursory look, because you guys know more about bluez
> > GATT than anyone else here at this point.  Otherwise, I will resubmit
> > this version as a patch request. -- thx
> >
> 
> ok. We can review your patches.
> Could you please send patches to change gatttool to support this new feature?
> How are you testing it? Do you have a modified BlueZ attribute server
> or another GATT server?

There are no changes required to gatttool. The implemented high level
function is still to read an attribute, and the function works the same
if the remote attribute is shorter than 22 octets, and will be "long"
only if the first response indicates a remote attribute of 22 octets or
longer. (accounting for the one octet for the response hdr)

I am testing this against against the internal Qualcomm GATT server,
which has been tested at the past two UPFs. This is also the same stack
which I hope to use to pre-vet the SM changes you are leading, prior to
the UPF in Las Vegas.

As my next task, I could implement the capability to support long reads
in the BlueZ GATT server. (likely followed by long writes in both the
GATT client and server of BlueZ)

> 
> Claudio

Thanks,

-- 
Brian Gix
bgix@codeaurora.org
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum