Hi Luiz, > On Thu, Feb 5, 2015 at 1:29 AM, Luiz Augusto von Dentz <luiz.dentz@xxxxxxxxx> wrote: > 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 ping -- 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
