Re: [PATCH] shared/gatt: Allow register_notify without CCC

[Arman Uguray <armansito@chromium.org>]

Hi Luiz,

> On Thu, Feb 5, 2015 at 1:29 AM, Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote:
> Hi Arman,
>
> On Thu, Feb 5, 2015 at 12:05 AM, Arman Uguray <armansito@chromium.org> wrote:
>> Hi Luiz,
>>
>>> On Wed, Feb 4, 2015 at 6:26 AM, Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote:
>>> Hi Arman,
>>>
>>> On Wed, Feb 4, 2015 at 5:58 AM, Arman Uguray <armansito@chromium.org> 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(&notify_data->chrc->notify_count, 1)) {
>>>> -               notify_data_unref(notify_data);
>>>> -               return;
>>>> -       }
>>>> +       if (__sync_sub_and_fetch(&notify_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@vger.kernel.org
>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>
>>>
>>>
>>> --
>>> Luiz Augusto von Dentz
>>
>> Thanks,
>> Arman
>
>
>
> --
> Luiz Augusto von Dentz

ping