* [PATCH BlueZ] gatt-database: Fix notifying on indicatable attr
@ 2021-02-19 17:49 Curtis
2021-02-19 18:37 ` [BlueZ] " bluez.test.bot
` (2 more replies)
0 siblings, 3 replies; 8+ messages in thread
From: Curtis @ 2021-02-19 17:49 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Curtis
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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* RE: [BlueZ] gatt-database: Fix notifying on indicatable attr
2021-02-19 17:49 [PATCH BlueZ] gatt-database: Fix notifying on indicatable attr Curtis
@ 2021-02-19 18:37 ` bluez.test.bot
2021-02-20 0:55 ` [PATCH BlueZ] " Luiz Augusto von Dentz
2021-02-22 18:26 ` Luiz Augusto von Dentz
2 siblings, 0 replies; 8+ messages in thread
From: bluez.test.bot @ 2021-02-19 18:37 UTC (permalink / raw)
To: linux-bluetooth, curtis
[-- Attachment #1: Type: text/plain, Size: 557 bytes --]
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH BlueZ] gatt-database: Fix notifying on indicatable attr
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 ` Luiz Augusto von Dentz
2021-02-20 1:18 ` Curtis Maves
2021-02-22 18:26 ` Luiz Augusto von Dentz
2 siblings, 1 reply; 8+ messages in thread
From: Luiz Augusto von Dentz @ 2021-02-20 0:55 UTC (permalink / raw)
To: Curtis; +Cc: linux-bluetooth
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH BlueZ] gatt-database: Fix notifying on indicatable attr
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
0 siblings, 1 reply; 8+ messages in thread
From: Curtis Maves @ 2021-02-20 1:18 UTC (permalink / raw)
To: Luiz Augusto von Dentz; +Cc: linux-bluetooth
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH BlueZ] gatt-database: Fix notifying on indicatable attr
2021-02-20 1:18 ` Curtis Maves
@ 2021-02-20 6:07 ` Luiz Augusto von Dentz
2021-02-20 18:29 ` Curtis Maves
0 siblings, 1 reply; 8+ messages in thread
From: Luiz Augusto von Dentz @ 2021-02-20 6:07 UTC (permalink / raw)
To: Curtis Maves; +Cc: linux-bluetooth
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH BlueZ] gatt-database: Fix notifying on indicatable attr
2021-02-20 6:07 ` Luiz Augusto von Dentz
@ 2021-02-20 18:29 ` Curtis Maves
2021-02-22 18:24 ` Luiz Augusto von Dentz
0 siblings, 1 reply; 8+ messages in thread
From: Curtis Maves @ 2021-02-20 18:29 UTC (permalink / raw)
To: Luiz Augusto von Dentz; +Cc: linux-bluetooth
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH BlueZ] gatt-database: Fix notifying on indicatable attr
2021-02-20 18:29 ` Curtis Maves
@ 2021-02-22 18:24 ` Luiz Augusto von Dentz
0 siblings, 0 replies; 8+ messages in thread
From: Luiz Augusto von Dentz @ 2021-02-22 18:24 UTC (permalink / raw)
To: Curtis Maves; +Cc: linux-bluetooth
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH BlueZ] gatt-database: Fix notifying on indicatable attr
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-22 18:26 ` Luiz Augusto von Dentz
2 siblings, 0 replies; 8+ messages in thread
From: Luiz Augusto von Dentz @ 2021-02-22 18:26 UTC (permalink / raw)
To: Curtis; +Cc: linux-bluetooth
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.
--
Luiz Augusto von Dentz
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2021-02-22 18:27 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
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
2021-02-22 18:24 ` Luiz Augusto von Dentz
2021-02-22 18:26 ` Luiz Augusto von Dentz
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).