Received-SPF: pass (google.com: domain of linux-bluetooth-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210219174946.599144-1-curtis@maves.io> <CABBYNZKakfpzTOdZaoa0tZ5Umqu9MKMmkMfNN51XY5owC_KWwQ@mail.gmail.com>
 <177bd04559f.d1a7c05c116102.319856870975137121@maves.io> <CABBYNZJJiXJJqtRMXKEvpwN-mj=eht9wdTgrK1p_Fo_36gK4Zw@mail.gmail.com>
 <177c0b3e32a.c0b69e35170119.443115256308628377@maves.io>
In-Reply-To: <177c0b3e32a.c0b69e35170119.443115256308628377@maves.io>
From:   Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date:   Mon, 22 Feb 2021 10:24:49 -0800
Message-ID: <CABBYNZ++t8Hu2b--jCwMkTwB5WeCDuMv_EU0z-cBetCjJpaZcA@mail.gmail.com>
Subject: Re: [PATCH BlueZ] gatt-database: Fix notifying on indicatable attr
To:     Curtis Maves <curtis@maves.io>
Cc:     "linux-bluetooth@vger.kernel.org" <linux-bluetooth@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi Curtis,

On Sat, Feb 20, 2021 at 10:29 AM Curtis Maves <curtis@maves.io> wrote:
>
> 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 see, I probably overlooked this change when reviewing the first time.

> I looked around but did not find any on my own.
> --
> Curtis Maves
>


-- 
Luiz Augusto von Dentz