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@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxx
> 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@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
Cheers,
--
Vinicius
--
To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
