linux-bluetooth.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Curtis Maves <curtis@maves.io>
To: "Luiz Augusto von Dentz" <luiz.dentz@gmail.com>
Cc: "linux-bluetooth@vger.kernel.org" <linux-bluetooth@vger.kernel.org>
Subject: Re: [PATCH BlueZ] gatt-database: Fix notifying on indicatable attr
Date: Sat, 20 Feb 2021 13:29:10 -0500	[thread overview]
Message-ID: <177c0b3e32a.c0b69e35170119.443115256308628377@maves.io> (raw)
In-Reply-To: <CABBYNZJJiXJJqtRMXKEvpwN-mj=eht9wdTgrK1p_Fo_36gK4Zw@mail.gmail.com>

Hi Luiz,
---- On Sat, 20 Feb 2021 01:07:57 -0500 Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote ----

 > Hi Curtis, 
 >  
 > On Fri, Feb 19, 2021 at 5:18 PM Curtis Maves <curtis@maves.io> wrote: 
 > > 
 > > Hi Luiz, 
 > > ---- On Fri, 19 Feb 2021 19:55:06 -0500 Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote ---- 
 > > 
 > >  > Hi Curtis, 
 > >  > 
 > >  > On Fri, Feb 19, 2021 at 10:11 AM Curtis <curtis@maves.io> wrote: 
 > >  > > 
 > >  > > When a local GATT characteristic has both the indicate and notify 
 > >  > > properties, notifications will not be send to clients requesting them. 
 > >  > > This change fixes this, allowing for notifications to be sent. 
 > >  > > 
 > >  > > Also simplifies logic about when notifications/indications should 
 > >  > > be sent. 
 > >  > > --- 
 > >  > >  src/gatt-database.c | 15 ++++++--------- 
 > >  > >  1 file changed, 6 insertions(+), 9 deletions(-) 
 > >  > > 
 > >  > > diff --git a/src/gatt-database.c b/src/gatt-database.c 
 > >  > > index d635c3214..bd5864bcd 100644 
 > >  > > --- a/src/gatt-database.c 
 > >  > > +++ b/src/gatt-database.c 
 > >  > > @@ -1344,10 +1344,7 @@ static void send_notification_to_device(void *data, void *user_data) 
 > >  > >         } 
 > >  > > 
 > >  > >         ccc = find_ccc_state(device_state, notify->ccc_handle); 
 > >  > > -       if (!ccc) 
 > >  > > -               return; 
 > >  > > - 
 > >  > > -       if (!ccc->value || (notify->conf && !(ccc->value & 0x0002))) 
 > >  > > +       if (!ccc || !(ccc->value & 0x0003)) 
 > >  > >                 return; 
 > >  > > 
 > >  > >         device = btd_adapter_find_device(notify->database->adapter, 
 > >  > > @@ -1374,7 +1371,7 @@ static void send_notification_to_device(void *data, void *user_data) 
 > >  > >          * TODO: If the device is not connected but bonded, send the 
 > >  > >          * notification/indication when it becomes connected. 
 > >  > >          */ 
 > >  > > -       if (!notify->conf) { 
 > >  > > +       if (!(ccc->value & 0x0002)) { 
 > >  > >                 DBG("GATT server sending notification"); 
 > >  > >                 bt_gatt_server_send_notification(server, 
 > >  > >                                         notify->handle, notify->value, 
 > >  > > @@ -2415,8 +2412,8 @@ static bool sock_io_read(struct io *io, void *user_data) 
 > >  > >                                 gatt_db_attribute_get_handle(chrc->attrib), 
 > >  > >                                 buf, bytes_read, 
 > >  > >                                 gatt_db_attribute_get_handle(chrc->ccc), 
 > >  > > -                               chrc->props & BT_GATT_CHRC_PROP_INDICATE ? 
 > >  > > -                               conf_cb : NULL, chrc->proxy); 
 > >  > > +                               conf_cb, 
 > >  > > +                               chrc->proxy); 
 > >  > 
 > >  > Not why are you changing this code to always set the conf_cb? This 
 > >  > would then always send indication rather then notifications: 
 > >  > 
 > >  > https://git.kernel.org/pub/scm/bluetooth/bluez.git/tree/src/gatt-database.c#n1387 
 > >  > 
 > >  > We might need to check what value it stored in the ccc state if both 
 > >  > indication and notification is supported. 
 > >  > 
 > >  > > 
 > >  > >         return true; 
 > >  > >  } 
 > >  > > @@ -2725,8 +2722,8 @@ static void property_changed_cb(GDBusProxy *proxy, const char *name, 
 > >  > >                                 gatt_db_attribute_get_handle(chrc->attrib), 
 > >  > >                                 value, len, 
 > >  > >                                 gatt_db_attribute_get_handle(chrc->ccc), 
 > >  > > -                               chrc->props & BT_GATT_CHRC_PROP_INDICATE ? 
 > >  > > -                               conf_cb : NULL, proxy); 
 > >  > > +                               conf_cb, 
 > >  > > +                               proxy); 
 > >  > >  } 
 > >  > > 
 > >  > >  static bool database_add_ccc(struct external_service *service, 
 > >  > > -- 
 > >  > > 2.30.1 
 > >  > > 
 > >  > > 
 > >  > 
 > >  > 
 > >  > -- 
 > >  > Luiz Augusto von Dentz 
 > >  > 
 > > 
 > > This patch changes the if-statement around sending notifications to check that the 
 > > ccc->value is not indicating rather than checking if conf_cb (notify->conf) is null. 
 > > This change makes unnecessary to conditionally pass the conf_cb. It's now simpler to always pass it. 
 >  
 > What Im saying is that we can't do this: 
 >  
 > if (!notify->conf) { 
 > DBG("GATT server sending notification"); 
 >  
 > conf callback will always be set so instead we need to change that to: 
 >  
 > if (ccc->value != 0x02) 
 >  
 >  
 >  
 > > -- 
 > > Curtis Maves 
 >  
 >  
 >  
 > -- 
 > Luiz Augusto von Dentz 
 > 
I agree that we can no longer do the following on line 1377:
 > if (!notify->conf) { 
 > DBG("GATT server sending notification"); 

As you said the ccc value needs to be tested instead. 
This part of the patch  already makes a change similar to what you suggested:
 > >  > > @@ -1374,7 +1371,7 @@ static void send_notification_to_device(void *data, void *user_data) 
 > >  > >          * TODO: If the device is not connected but bonded, send the 
 > >  > >          * notification/indication when it becomes connected. 
 > >  > >          */ 
 > >  > > -       if (!notify->conf) { 
 > >  > > +       if (!(ccc->value & 0x0002)) { 
Is there anywhere else where notify->conf is checked?
I looked around but did not find any on my own.
--
Curtis Maves


  reply	other threads:[~2021-02-20 18:30 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-02-19 17:49 [PATCH BlueZ] gatt-database: Fix notifying on indicatable attr Curtis
2021-02-19 18:37 ` [BlueZ] " bluez.test.bot
2021-02-20  0:55 ` [PATCH BlueZ] " Luiz Augusto von Dentz
2021-02-20  1:18   ` Curtis Maves
2021-02-20  6:07     ` Luiz Augusto von Dentz
2021-02-20 18:29       ` Curtis Maves [this message]
2021-02-22 18:24         ` Luiz Augusto von Dentz
2021-02-22 18:26 ` Luiz Augusto von Dentz

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=177c0b3e32a.c0b69e35170119.443115256308628377@maves.io \
    --to=curtis@maves.io \
    --cc=linux-bluetooth@vger.kernel.org \
    --cc=luiz.dentz@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).