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