Hi Curtis,
On Sat, Feb 20, 2021 at 10:29 AM Curtis Maves <curtis@xxxxxxxx> wrote:
>
> Hi Luiz,
> ---- On Sat, 20 Feb 2021 01:07:57 -0500 Luiz Augusto von Dentz <luiz.dentz@xxxxxxxxx> wrote ----
>
> > Hi Curtis,
> >
> > On Fri, Feb 19, 2021 at 5:18 PM Curtis Maves <curtis@xxxxxxxx> wrote:
> > >
> > > Hi Luiz,
> > > ---- On Fri, 19 Feb 2021 19:55:06 -0500 Luiz Augusto von Dentz <luiz.dentz@xxxxxxxxx> wrote ----
> > >
> > > > Hi Curtis,
> > > >
> > > > On Fri, Feb 19, 2021 at 10:11 AM Curtis <curtis@xxxxxxxx> 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
