Hi Arman, On Thu, Feb 5, 2015 at 12:05 AM, Arman Uguray <armansito@xxxxxxxxxxxx> wrote: > Hi Luiz, > >> On Wed, Feb 4, 2015 at 6:26 AM, Luiz Augusto von Dentz <luiz.dentz@xxxxxxxxx> wrote: >> Hi Arman, >> >> On Wed, Feb 4, 2015 at 5:58 AM, Arman Uguray <armansito@xxxxxxxxxxxx> wrote: >>> Certain peripherals can expose a GATT characteristic with the >>> notify/indicate property but with no Client Characteristic Configuration >>> descriptor. Since bt_gatt_client_register_notify enforces a CCC as a >>> requirement, it returns an error for such characteristics. >> >> Do you have some evidence of this? I don't mind not being too strict >> while receiving but we need to document why we are doing these >> changes,... > > I came across this while testing with a Nexus 6 device running Android > L. The application exposes a "Service Changed" characteristic with no > CCC, which is causing bt_gatt_client's initialization sequence to fail > (since it assumes that there will be a CCC and treats the absence as > an error). This seemed strange to me, so I did some digging, and the > Bluetooth developer page actually lists "None" for "Descriptors" > (https://developer.bluetooth.org/gatt/services/Pages/ServiceViewer.aspx?u=org.bluetooth.service.generic_attribute.xml) > of this characteristic. > > However, this seemed incorrect to me, so I checked the Core Spec v4.2, > Vol 3, Part G, Section 7.1 "Service Changed", which states: "This > [Service Changed] Characteristic Value shall be configured to be > indicated, using the Client Characteristic Configuration descriptor by > a client. Indications caused by changes to the Service Changed > Characteristic Value shall be considered lost if the client has not > enabled indications in the Client Configuration Characteristic > Descriptor." Which means that the CCC is certainly mandatory, so this > must be an error in the developer page. It is probably a bug, and in this case the device might not even be able to qualify since service changed has 2 tests: TP/GAS/CL/BV-01-C and TP/GAS/SR/BV-01-C. We should probably communicate that the documentation needs fixing and the TS should probably be updated as well to reflect CCC is mandatory in this case. > Regardless, though, even if this is an error in the BlueDroid stack, > it means that some devices may have incorrectly implemented the spec > here, so we should probably tolerate this. BlueZ's server > implementation SHOULD regardless expose this descriptor, but the > client IMO shouldn't fail if the CCC generally doesn't exist. Provided the SIG don't come with invalid tests for service changed that is probably okay for the client, for the server I don't how it would pass TP/GAS/SR/BV-01-C without CCC except if PTS is ignoring the lack of CCC which IMO defeats the purpose of the test. So the argument for service changed is probably not valid here, but for custom attributes where there is no tests to validate the implemenation perhaps you have a point, but note that it still will need to set BT_GATT_CHRC_PROP_NOTIFY and BT_GATT_CHRC_PROP_INDICATE, btw such policies perhaps should be done directly in shared/gatt-client.c so we don't need have duplicated logic for Android. > >> ...and btw this perhaps makes my argument of enabling >> notification by default even stronger since if such devices really >> exists they might be notifying us since the beginning. >> > > Hmm, I don't know how this makes that argument any stronger. > bt_gatt_client already automatically registers for Service Changed, > and for external applications we still want to only notify if an app > is interested (which they can do now across connections). > > Generally, I want to treat this as a corner-case, since it's not > really spec compliant behavior. Either way, it's probably not that bad > to have this fix so that we don't bork out immediately with devices > that behave incorrectly. Actually, it might be even more spec > compliant for us to simply ignore the Service Changed characteristic > if it's discovered but doesn't have a CCC, since the spec says > "changes to the Service Changed Characteristic Value shall be > considered lost if the client has not enabled indications in the > Client Configuration Characteristic Descriptor", which would end up > being a simpler fix on our end. > > Either way, I'll wait for your response to determine what the right > behavior is here. For service changed perhaps, but since we are using bt_gatt_client_register_notify for any notification we would need to have a special case for service changed alone so I would not bother since broken stacks in this respect will probably not pass the qualification if the test spec where implemented properly, for any other char I would not be so strict since some OS may allow adding a characteristic with notifiy/indicate bits sets but without a CCC to enable them, actually Android probably allows that to happen already, sadly I think even our BlueZ for Android currently allows that. >> The other problem I now see is that there is probably no way to rate >> limit the notification, because they are profile specific, so if you >> had a problem with too much traffic on D-Bus this may already happen >> even if we register the notification on demand. >> > > Right, though I guess this is a separate issue that can be dealt with > at the D-Bus API level. shared/gatt-client has to still send all > notifications up, otherwise things like HoG wouldn't work well with a > rate limit. Anyway, we can do this at a D-Bus API level separately. > I'll add this to the general TODO list. > >>> This patch fixes this behavior so that bt_gatt_client_register_notify >>> immediately registers the callback and returns success for >>> characteristics with no CCC. >>> --- >>> src/shared/gatt-client.c | 43 +++++++++++++++++++++++++------------------ >>> 1 file changed, 25 insertions(+), 18 deletions(-) >>> >>> diff --git a/src/shared/gatt-client.c b/src/shared/gatt-client.c >>> index 5edb991..d0fc054 100644 >>> --- a/src/shared/gatt-client.c >>> +++ b/src/shared/gatt-client.c >>> @@ -88,6 +88,7 @@ struct bt_gatt_client { >>> * the remote peripheral. >>> */ >>> unsigned int svc_chngd_ind_id; >>> + bool svc_chngd_registered; >>> struct queue *svc_chngd_queue; /* Queued service changed events */ >>> bool in_svc_chngd; >>> >>> @@ -234,12 +235,6 @@ static struct notify_chrc *notify_chrc_create(struct bt_gatt_client *client, >>> &properties, NULL)) >>> return NULL; >>> >>> - /* Find the CCC characteristic */ >>> - ccc = NULL; >>> - gatt_db_service_foreach_desc(attr, find_ccc, &ccc); >>> - if (!ccc) >>> - return NULL; >>> - >>> chrc = new0(struct notify_chrc, 1); >>> if (!chrc) >>> return NULL; >>> @@ -250,8 +245,17 @@ static struct notify_chrc *notify_chrc_create(struct bt_gatt_client *client, >>> return NULL; >>> } >>> >>> + /* >>> + * Find the CCC characteristic. Some characteristics that allow >>> + * notifications may not have a CCC descriptor. We treat these as >>> + * automatically successful. >>> + */ >>> + ccc = NULL; >>> + gatt_db_service_foreach_desc(attr, find_ccc, &ccc); >>> + if (ccc) >>> + chrc->ccc_handle = gatt_db_attribute_get_handle(ccc); >>> + >>> chrc->value_handle = value_handle; >>> - chrc->ccc_handle = gatt_db_attribute_get_handle(ccc); >>> chrc->properties = properties; >>> >>> queue_push_tail(client->notify_chrcs, chrc); >>> @@ -1020,6 +1024,7 @@ static void service_changed_reregister_cb(uint16_t att_ecode, void *user_data) >>> util_debug(client->debug_callback, client->debug_data, >>> "Re-registered handler for \"Service Changed\" after " >>> "change in GATT service"); >>> + client->svc_chngd_registered = true; >>> return; >>> } >>> >>> @@ -1089,6 +1094,7 @@ static void service_changed_complete(struct discovery_op *op, bool success, >>> /* The GATT service was modified. Re-register the handler for >>> * indications from the "Service Changed" characteristic. >>> */ >>> + client->svc_chngd_registered = false; >>> client->svc_chngd_ind_id = bt_gatt_client_register_notify(client, >>> gatt_db_attribute_get_handle(attr), >>> service_changed_reregister_cb, >>> @@ -1197,6 +1203,7 @@ static void service_changed_register_cb(uint16_t att_ecode, void *user_data) >>> goto done; >>> } >>> >>> + client->svc_chngd_registered = true; >>> client->ready = true; >>> success = true; >>> util_debug(client->debug_callback, client->debug_data, >>> @@ -1239,13 +1246,16 @@ static void init_complete(struct discovery_op *op, bool success, >>> service_changed_register_cb, >>> service_changed_cb, >>> client, NULL); >>> - client->ready = false; >>> + >>> + if (!client->svc_chngd_registered) >>> + client->ready = false; >>> >>> if (client->svc_chngd_ind_id) >>> return; >>> >>> util_debug(client->debug_callback, client->debug_data, >>> "Failed to register handler for \"Service Changed\""); >>> + success = false; >>> >>> fail: >>> util_debug(client->debug_callback, client->debug_data, >>> @@ -1423,18 +1433,17 @@ static void complete_unregister_notify(void *data) >>> */ >>> if (notify_data->att_id) { >>> bt_att_cancel(notify_data->client->att, notify_data->att_id); >>> - notify_data_unref(notify_data); >>> - return; >>> + goto done; >>> } >>> >>> - if (__sync_sub_and_fetch(¬ify_data->chrc->notify_count, 1)) { >>> - notify_data_unref(notify_data); >>> - return; >>> - } >>> + if (__sync_sub_and_fetch(¬ify_data->chrc->notify_count, 1) || >>> + !notify_data->chrc->ccc_handle) >>> + goto done; >>> >>> if (notify_data_write_ccc(notify_data, false, disable_ccc_callback)) >>> return; >>> >>> +done: >>> notify_data_unref(notify_data); >>> } >>> >>> @@ -2571,9 +2580,7 @@ unsigned int bt_gatt_client_register_notify(struct bt_gatt_client *client, >>> /* Add the handler to the bt_gatt_client's general list */ >>> queue_push_tail(client->notify_list, notify_data); >>> >>> - /* Assign an ID to the handler and notify the caller that it was >>> - * successfully registered. >>> - */ >>> + /* Assign an ID to the handler. */ >>> if (client->next_reg_id < 1) >>> client->next_reg_id = 1; >>> >>> @@ -2591,7 +2598,7 @@ unsigned int bt_gatt_client_register_notify(struct bt_gatt_client *client, >>> /* >>> * If the ref count is not zero, then notifications are already enabled. >>> */ >>> - if (chrc->notify_count > 0) { >>> + if (chrc->notify_count > 0 || !chrc->ccc_handle) { >>> complete_notify_request(notify_data); >>> return notify_data->id; >>> } >>> -- >>> 2.2.0.rc0.207.ga3a616c >>> >>> -- >>> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in >>> the body of a message to majordomo@xxxxxxxxxxxxxxx >>> More majordomo info at http://vger.kernel.org/majordomo-info.html >> >> >> >> -- >> Luiz Augusto von Dentz > > Thanks, > Arman -- Luiz Augusto von Dentz -- To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
