diff mbox series

[BlueZ] gatt-database: Fix notifying on indicatable attr

Message ID 20210219174946.599144-1-curtis@maves.io (mailing list archive)
State Accepted
Delegated to: Luiz Von Dentz
Headers show
Series [BlueZ] gatt-database: Fix notifying on indicatable attr | expand

Commit Message

Curtis Maves Feb. 19, 2021, 5:49 p.m. UTC
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(-)

Comments

bluez.test.bot@gmail.com Feb. 19, 2021, 6:37 p.m. UTC | #1
This is automated email and please do not reply to this email!

Dear submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=435711

---Test result---

##############################
Test: CheckPatch - PASS

##############################
Test: CheckGitLint - PASS

##############################
Test: CheckBuild - PASS

##############################
Test: MakeCheck - PASS



---
Regards,
Linux Bluetooth
Luiz Augusto von Dentz Feb. 20, 2021, 12:55 a.m. UTC | #2
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
>
>
Curtis Maves Feb. 20, 2021, 1:18 a.m. UTC | #3
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.
--
Curtis Maves
Luiz Augusto von Dentz Feb. 20, 2021, 6:07 a.m. UTC | #4
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
Curtis Maves Feb. 20, 2021, 6:29 p.m. UTC | #5
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
Luiz Augusto von Dentz Feb. 22, 2021, 6:24 p.m. UTC | #6
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 Feb. 22, 2021, 6:26 p.m. UTC | #7
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);
>
>         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
>

Applied, thanks.
diff mbox series

Patch

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);
 
 	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,