Re: [PATCH BlueZ 06/12] core: device: Use shared/gatt-client for GATT.

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Arman,

On Wed, Nov 19, 2014 at 6:42 PM, Arman Uguray <armansito@xxxxxxxxxxxx> wrote:
> Hi Luiz,
>
>> On Wed, Nov 19, 2014 at 7:43 AM, Luiz Augusto von Dentz <luiz.dentz@xxxxxxxxx> wrote:
>> Hi Arman,
>>
>> On Mon, Nov 17, 2014 at 9:22 PM, Arman Uguray <armansito@xxxxxxxxxxxx> wrote:
>>> This patch changes the GATT service discovery code path with the
>>> following:
>>>    - As part for device_attach_attrib, a bt_gatt_client structure is
>>>    created that performs MTU exchange and service discovery, caches the
>>>    services in an internal list, and handles the remote GATT service.
>>>
>>>    - The device_browse_primary code path obtains service information
>>>    by iterating through bt_gatt_client's services, instead of directly
>>>    initiating primary and secondary service discovery using attrib/gatt.
>>>    If the bt_gatt_client isn't ready at the time, the code defers
>>>    registration of services and profile probing until the ready handler
>>>    is called.
>>>
>>>    - The attio connected callbacks are invoked from bt_gatt_client's
>>>    ready handler instead of device_attach_attrib.
>>> ---
>>
>> I would suggest that before we start changing the core we make sure we
>> have completed the TODO items for bt_gatt_client/bt_gatt_server, there
>> are items such as gatt-client + gatt-server coexistence that haven't
>> been tested  and I also remember that we had some ideas about a new
>> object that would take care of both client and server under it, the
>> caching probably needs to be addressed as well otherwise
>> bt_gatt_client will always end up discovering the service in every
>> connection.
>>
>
> I think we have to start somewhere. I think at this point we have the
> basic feature set implemented to start converting the core and at
> least have behavior that's equivalent (or better) to what the older
> code did. I didn't want to immediately involve the server until seeing
> how the client fits in and we still have to figure out how not to
> break the existing server role profiles (the experimental ones) while
> integrating bt_gatt_server (or maybe we don't care since those are all
> experimental and we can perhaps just throw out src/attrib-server).
> Either way I wanted to tackle this in several steps and move
> iteratively.

It is not exactly the steps that worries me, but I don't wont to rush
and cause regressions, for example we should not treat client and
server separately since it is valid to have different profiles using
different roles.

> So, I guess the one big item is the missing caching support, which I
> will now get started on, but in the meantime I think we should start
> converting the core, see if there are any bugs and edge cases we have
> to handle, etc. I want to see how well the plugins work with the new
> code and more importantly I want to have the GATT D-Bus API
> implemented and merged into the tree (at least as experimental) and
> get people to start using it and send feedback.

Coexistence of server and client is another one, perhaps we can try to
emulate something in the unit tests.

> In the meantime I'm going to start working on two API functions like
> bt_gatt_client_new_from_storage & bt_gatt_client_store_attributes so
> you can initialize the cache from the contents of a file and commit
> them to a file. That should make things easy to integrate into the
> core.

Yep, something like that will need to be in place and also perhaps
bt_gatt_*attach/bt_gatt_*detach otherwise we can't have the same
connection handled by both client and server at same, this btw could
be solved with a higher level object that takes care of both and
perhaps we can then have bt_gatt_get_client/bt_gatt_get_server.

>>
>>>  src/device.c           | 286 ++++++++++++++++++++++++++-----------------------
>>>  src/device.h           |   2 -
>>>  src/shared/att-types.h |   3 +-
>>>  3 files changed, 152 insertions(+), 139 deletions(-)
>>>
>>> diff --git a/src/device.c b/src/device.c
>>> index 45c7e9c..3d0159e 100644
>>> --- a/src/device.c
>>> +++ b/src/device.c
>>> @@ -74,6 +74,10 @@
>>>  #define DISCONNECT_TIMER       2
>>>  #define DISCOVERY_TIMER                1
>>>
>>> +#ifndef MIN
>>> +#define MIN(a, b) ((a) < (b) ? (a) : (b))
>>> +#endif
>>> +
>>>  static DBusConnection *dbus_conn = NULL;
>>>  unsigned service_state_cb_id;
>>>
>>> @@ -205,9 +209,12 @@ struct btd_device {
>>>         GSList          *attios_offline;
>>>         guint           attachid;               /* Attrib server attach */
>>>
>>> -       struct bt_att *att;     /* The new ATT transport */
>>> +       struct bt_att *att;                     /* The new ATT transport */
>>> +       uint16_t att_mtu;                       /* The ATT MTU */
>>>         unsigned int att_disconn_id;
>>>
>>> +       struct bt_gatt_client *gatt_client;     /* GATT client cache */
>>> +
>>>         struct bearer_state bredr_state;
>>>         struct bearer_state le_state;
>>>
>>> @@ -463,6 +470,18 @@ static void browse_request_free(struct browse_req *req)
>>>         g_free(req);
>>>  }
>>>
>>> +static void gatt_client_cleanup(struct btd_device *device)
>>> +{
>>> +       if (!device->gatt_client)
>>> +               return;
>>> +
>>> +       bt_gatt_client_set_service_changed(device->gatt_client, NULL, NULL,
>>> +                                                                       NULL);
>>> +       bt_gatt_client_set_ready_handler(device->gatt_client, NULL, NULL, NULL);
>>> +       bt_gatt_client_unref(device->gatt_client);
>>> +       device->gatt_client = NULL;
>>> +}
>>> +
>>>  static void attio_cleanup(struct btd_device *device)
>>>  {
>>>         if (device->attachid) {
>>> @@ -480,6 +499,8 @@ static void attio_cleanup(struct btd_device *device)
>>>                 device->att_io = NULL;
>>>         }
>>>
>>> +       gatt_client_cleanup(device);
>>> +
>>>         if (device->att) {
>>>                 bt_att_unref(device->att);
>>>                 device->att = NULL;
>>> @@ -3445,41 +3466,6 @@ done:
>>>         attio_cleanup(device);
>>>  }
>>>
>>> -static void register_all_services(struct browse_req *req, GSList *services)
>>> -{
>>> -       struct btd_device *device = req->device;
>>> -
>>> -       btd_device_set_temporary(device, FALSE);
>>> -
>>> -       update_gatt_services(req, device->primaries, services);
>>> -       g_slist_free_full(device->primaries, g_free);
>>> -       device->primaries = NULL;
>>> -
>>> -       device_register_primaries(device, services, -1);
>>> -
>>> -       device_probe_profiles(device, req->profiles_added);
>>> -
>>> -       if (device->attios == NULL && device->attios_offline == NULL)
>>> -               attio_cleanup(device);
>>> -
>>> -       g_dbus_emit_property_changed(dbus_conn, device->path,
>>> -                                               DEVICE_INTERFACE, "UUIDs");
>>> -
>>> -       device_svc_resolved(device, device->bdaddr_type, 0);
>>> -
>>> -       store_services(device);
>>> -
>>> -       browse_request_free(req);
>>> -}
>>> -
>>> -static int service_by_range_cmp(gconstpointer a, gconstpointer b)
>>> -{
>>> -       const struct gatt_primary *prim = a;
>>> -       const struct att_range *range = b;
>>> -
>>> -       return memcmp(&prim->range, range, sizeof(*range));
>>> -}
>>> -
>>>  static void send_le_browse_response(struct browse_req *req)
>>>  {
>>>         struct btd_device *dev = req->device;
>>> @@ -3510,122 +3496,152 @@ static void send_le_browse_response(struct browse_req *req)
>>>         g_dbus_send_reply(dbus_conn, msg, DBUS_TYPE_INVALID);
>>>  }
>>>
>>> -static void find_included_cb(uint8_t status, GSList *includes, void *user_data)
>>> +/*
>>> + * TODO: Remove this function once btd_device stops storing gatt_primary
>>> + * structures for discovered services.
>>> + */
>>> +static bool populate_primaries(struct btd_device *device, GSList **services)
>>>  {
>>> -       struct included_search *search = user_data;
>>> -       struct btd_device *device = search->req->device;
>>> +       struct bt_gatt_service_iter iter;
>>> +       const bt_gatt_service_t *service;
>>>         struct gatt_primary *prim;
>>> -       GSList *l;
>>> +       uint128_t u128;
>>> +       bt_uuid_t uuid;
>>>
>>> -       DBG("status %u", status);
>>> +       /* Create gatt_primary structures for all primary/secondary services */
>>> +       if (!bt_gatt_service_iter_init(&iter, device->gatt_client))
>>> +               return false;
>>>
>>> -       if (device->attrib == NULL || status) {
>>> -               struct browse_req *req = device->browse;
>>> +       while (bt_gatt_service_iter_next(&iter, &service)) {
>>> +               prim = g_new0(struct gatt_primary, 1);
>>> +               prim->range.start = service->start_handle;
>>> +               prim->range.end = service->end_handle;
>>>
>>> -               if (status)
>>> -                       error("Find included services failed: %s (%d)",
>>> -                                       att_ecode2str(status), status);
>>> -               else
>>> -                       error("Disconnected while doing included discovery");
>>> +               memcpy(u128.data, service->uuid, 16);
>>> +               bt_uuid128_create(&uuid, u128);
>>> +               bt_uuid_to_string(&uuid, prim->uuid, MAX_LEN_UUID_STR + 1);
>>>
>>> -               if (!req)
>>> -                       goto complete;
>>> +               *services = g_slist_append(*services, prim);
>>> +       }
>>>
>>> +       return true;
>>> +}
>>> +
>>> +static void register_gatt_services(struct browse_req *req)
>>> +{
>>> +       struct btd_device *device = req->device;
>>> +       GSList *services = NULL;
>>> +
>>> +       if (!bt_gatt_client_is_ready(device->gatt_client))
>>> +               return;
>>> +
>>> +       if (!populate_primaries(device, &services)) {
>>>                 send_le_browse_response(req);
>>>                 device->browse = NULL;
>>>                 browse_request_free(req);
>>> -
>>> -               goto complete;
>>> +               return;
>>>         }
>>>
>>> -       if (includes == NULL)
>>> -               goto next;
>>> +       btd_device_set_temporary(device, FALSE);
>>>
>>> -       for (l = includes; l; l = l->next) {
>>> -               struct gatt_included *incl = l->data;
>>> +       update_gatt_services(req, device->primaries, services);
>>> +       g_slist_free_full(device->primaries, g_free);
>>> +       device->primaries = NULL;
>>>
>>> -               if (g_slist_find_custom(search->services, &incl->range,
>>> -                                               service_by_range_cmp))
>>> -                       continue;
>>> +       device_register_primaries(device, services, -1);
>>>
>>> -               prim = g_new0(struct gatt_primary, 1);
>>> -               memcpy(prim->uuid, incl->uuid, sizeof(prim->uuid));
>>> -               memcpy(&prim->range, &incl->range, sizeof(prim->range));
>>> +       device_probe_profiles(device, req->profiles_added);
>>>
>>> -               search->services = g_slist_append(search->services, prim);
>>> -       }
>>> +       if (device->attios == NULL && device->attios_offline == NULL)
>>> +               attio_cleanup(device);
>>>
>>> -next:
>>> -       search->current = search->current->next;
>>> -       if (search->current == NULL) {
>>> -               register_all_services(search->req, search->services);
>>> -               search->services = NULL;
>>> -               goto complete;
>>> -       }
>>> +       g_dbus_emit_property_changed(dbus_conn, device->path,
>>> +                                               DEVICE_INTERFACE, "UUIDs");
>>>
>>> -       prim = search->current->data;
>>> -       gatt_find_included(device->attrib, prim->range.start, prim->range.end,
>>> -                                       find_included_cb, search);
>>> -       return;
>>> +       device_svc_resolved(device, device->bdaddr_type, 0);
>>>
>>> -complete:
>>> -       g_slist_free_full(search->services, g_free);
>>> -       g_free(search);
>>> +       store_services(device);
>>> +
>>> +       browse_request_free(req);
>>>  }
>>>
>>> -static void find_included_services(struct browse_req *req, GSList *services)
>>> +static void gatt_client_ready_cb(bool success, uint8_t att_ecode,
>>> +                                                               void *user_data)
>>>  {
>>> -       struct btd_device *device = req->device;
>>> -       struct included_search *search;
>>> -       struct gatt_primary *prim;
>>> -       GSList *l;
>>> +       struct btd_device *device = user_data;
>>>
>>> -       DBG("service count %u", g_slist_length(services));
>>> +       DBG("gatt-client ready -- status: %s, ATT error: %u",
>>> +                                               success ? "success" : "failed",
>>> +                                               att_ecode);
>>> +
>>> +       if (!success) {
>>> +               if (device->browse) {
>>> +                       struct browse_req *req = device->browse;
>>> +                       send_le_browse_response(req);
>>> +                       device->browse = NULL;
>>> +                       browse_request_free(req);
>>> +               }
>>>
>>> -       if (services == NULL) {
>>> -               DBG("No services found");
>>> -               register_all_services(req, NULL);
>>>                 return;
>>>         }
>>>
>>> -       search = g_new0(struct included_search, 1);
>>> -       search->req = req;
>>> +       device->att_mtu = bt_att_get_mtu(device->att);
>>>
>>> -       /* We have to completely duplicate the data in order to have a
>>> -        * clearly defined responsibility of freeing regardless of
>>> -        * failure or success. Otherwise memory leaks are inevitable.
>>> -        */
>>> -       for (l = services; l; l = g_slist_next(l)) {
>>> -               struct gatt_primary *dup;
>>> +       DBG("gatt-client exchanged MTU: %u", device->att_mtu);
>>>
>>> -               dup = g_memdup(l->data, sizeof(struct gatt_primary));
>>> +       if (device->browse)
>>> +               register_gatt_services(device->browse);
>>>
>>> -               search->services = g_slist_append(search->services, dup);
>>> -       }
>>> +       /*
>>> +        * TODO: Change attio callbacks to accept a gatt-client instead of a
>>> +        * GAttrib.
>>> +        */
>>> +       g_slist_foreach(device->attios, attio_connected, device->attrib);
>>> +}
>>>
>>> -       search->current = search->services;
>>> +static void gatt_client_service_changed(uint16_t start_handle,
>>> +                                                       uint16_t end_handle,
>>> +                                                       void *user_data)
>>> +{
>>> +       struct btd_device *device = user_data;
>>> +
>>> +       DBG("gatt-client: Service Changed: start 0x%04x, end: 0x%04x",
>>> +                                               start_handle, end_handle);
>>>
>>> -       prim = search->current->data;
>>> -       gatt_find_included(device->attrib, prim->range.start, prim->range.end,
>>> -                                       find_included_cb, search);
>>> +       /*
>>> +        * TODO: Notify clients that services changed so they can handle it
>>> +        * directly. Remove the profile if a service was removed.
>>> +        */
>>> +       device_browse_primary(device, NULL);
>>>  }
>>>
>>> -static void primary_cb(uint8_t status, GSList *services, void *user_data)
>>> +static void gatt_client_init(struct btd_device *device)
>>>  {
>>> -       struct browse_req *req = user_data;
>>> +       gatt_client_cleanup(device);
>>>
>>> -       DBG("status %u", status);
>>> +       device->gatt_client = bt_gatt_client_new(device->att, device->att_mtu);
>>> +       if (!device->gatt_client) {
>>> +               DBG("Failed to initialize gatt-client");
>>> +               return;
>>> +       }
>>>
>>> -       if (status) {
>>> -               struct btd_device *device = req->device;
>>> +       if (!bt_gatt_client_set_ready_handler(device->gatt_client,
>>> +                                                       gatt_client_ready_cb,
>>> +                                                       device, NULL)) {
>>> +               DBG("Failed to set ready handler for gatt-client");
>>> +               gatt_client_cleanup(device);
>>> +               return;
>>> +       }
>>>
>>> -               send_le_browse_response(req);
>>> -               device->browse = NULL;
>>> -               browse_request_free(req);
>>> +       if (!bt_gatt_client_set_service_changed(device->gatt_client,
>>> +                                               gatt_client_service_changed,
>>> +                                               device, NULL)) {
>>> +               DBG("Failed to set service changed handler for gatt-client");
>>> +               gatt_client_cleanup(device);
>>>                 return;
>>>         }
>>>
>>> -       find_included_services(req, services);
>>> +       DBG("gatt-client created");
>>>  }
>>>
>>>  bool device_attach_attrib(struct btd_device *dev, GIOChannel *io)
>>> @@ -3660,10 +3676,8 @@ bool device_attach_attrib(struct btd_device *dev, GIOChannel *io)
>>>                 }
>>>         }
>>>
>>> -       if (cid == ATT_CID)
>>> -               mtu = BT_ATT_DEFAULT_LE_MTU;
>>> -
>>> -       attrib = g_attrib_new(io, mtu);
>>> +       dev->att_mtu = MIN(mtu, BT_ATT_MAX_LE_MTU);
>>> +       attrib = g_attrib_new(io, dev->att_mtu);
>>>         if (!attrib) {
>>>                 error("Unable to create new GAttrib instance");
>>>                 return false;
>>> @@ -3678,11 +3692,12 @@ bool device_attach_attrib(struct btd_device *dev, GIOChannel *io)
>>>
>>>         dev->attrib = attrib;
>>>         dev->att = bt_att_ref(g_attrib_get_att(attrib));
>>> -
>>>         dev->att_disconn_id = bt_att_register_disconnect(dev->att,
>>>                                                 att_disconnected_cb, dev, NULL);
>>>         bt_att_set_close_on_unref(dev->att, true);
>>>
>>> +       gatt_client_init(dev);
>>> +
>>>         /*
>>>          * Remove the device from the connect_list and give the passive
>>>          * scanning another chance to be restarted in case there are
>>> @@ -3690,8 +3705,6 @@ bool device_attach_attrib(struct btd_device *dev, GIOChannel *io)
>>>          */
>>>         adapter_connect_list_remove(dev->adapter, dev);
>>>
>>> -       g_slist_foreach(dev->attios, attio_connected, dev->attrib);
>>> -
>>>         return true;
>>>  }
>>>
>>> @@ -3845,13 +3858,29 @@ static void att_browse_error_cb(const GError *gerr, gpointer user_data)
>>>         browse_request_free(req);
>>>  }
>>>
>>> +static void browse_gatt_client(struct browse_req *req)
>>> +{
>>> +       struct btd_device *device = req->device;
>>> +
>>> +       if (!device->gatt_client) {
>>> +               DBG("No gatt-client currently attached");
>>> +               return;
>>> +       }
>>> +
>>> +       /*
>>> +        * If gatt-client is ready, then register all services. Otherwise, this
>>> +        * will be deferred until client becomes ready.
>>> +        */
>>> +       if (bt_gatt_client_is_ready(device->gatt_client))
>>> +               register_gatt_services(req);
>>> +}
>>> +
>>>  static void att_browse_cb(gpointer user_data)
>>>  {
>>>         struct att_callbacks *attcb = user_data;
>>>         struct btd_device *device = attcb->user_data;
>>>
>>> -       gatt_discover_primary(device->attrib, NULL, primary_cb,
>>> -                                                       device->browse);
>>> +       browse_gatt_client(device->browse);
>>>  }
>>>
>>>  static int device_browse_primary(struct btd_device *device, DBusMessage *msg)
>>> @@ -3869,7 +3898,7 @@ static int device_browse_primary(struct btd_device *device, DBusMessage *msg)
>>>         device->browse = req;
>>>
>>>         if (device->attrib) {
>>> -               gatt_discover_primary(device->attrib, NULL, primary_cb, req);
>>> +               browse_gatt_client(device->browse);
>>>                 goto done;
>>>         }
>>>
>>> @@ -4738,21 +4767,6 @@ GSList *btd_device_get_primaries(struct btd_device *device)
>>>         return device->primaries;
>>>  }
>>>
>>> -void btd_device_gatt_set_service_changed(struct btd_device *device,
>>> -                                               uint16_t start, uint16_t end)
>>> -{
>>> -       GSList *l;
>>> -
>>> -       for (l = device->primaries; l; l = g_slist_next(l)) {
>>> -               struct gatt_primary *prim = l->data;
>>> -
>>> -               if (start <= prim->range.end && end >= prim->range.start)
>>> -                       prim->changed = TRUE;
>>> -       }
>>> -
>>> -       device_browse_primary(device, NULL);
>>> -}
>>> -
>>>  void btd_device_add_uuid(struct btd_device *device, const char *uuid)
>>>  {
>>>         GSList *uuid_list;
>>> diff --git a/src/device.h b/src/device.h
>>> index 2e0473e..00cb502 100644
>>> --- a/src/device.h
>>> +++ b/src/device.h
>>> @@ -67,8 +67,6 @@ const sdp_record_t *btd_device_get_record(struct btd_device *device,
>>>  struct gatt_primary *btd_device_get_primary(struct btd_device *device,
>>>                                                         const char *uuid);
>>>  GSList *btd_device_get_primaries(struct btd_device *device);
>>> -void btd_device_gatt_set_service_changed(struct btd_device *device,
>>> -                                               uint16_t start, uint16_t end);
>>>  bool device_attach_attrib(struct btd_device *dev, GIOChannel *io);
>>>  void btd_device_add_uuid(struct btd_device *device, const char *uuid);
>>>  void device_add_eir_uuids(struct btd_device *dev, GSList *uuids);
>>> diff --git a/src/shared/att-types.h b/src/shared/att-types.h
>>> index 3d8829a..8b6d537 100644
>>> --- a/src/shared/att-types.h
>>> +++ b/src/shared/att-types.h
>>> @@ -27,7 +27,8 @@
>>>  #define __packed __attribute__((packed))
>>>  #endif
>>>
>>> -#define BT_ATT_DEFAULT_LE_MTU 23
>>> +#define BT_ATT_DEFAULT_LE_MTU  23
>>> +#define BT_ATT_MAX_LE_MTU      517
>>>
>>>  /* ATT protocol opcodes */
>>>  #define BT_ATT_OP_ERROR_RSP                    0x01
>>> --
>>> 2.1.0.rc2.206.gedb03e5
>>>
>>> --
>>> 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




[Index of Archives]     [Bluez Devel]     [Linux Wireless Networking]     [Linux Wireless Personal Area Networking]     [Linux ATH6KL]     [Linux USB Devel]     [Linux Media Drivers]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Big List of Linux Books]

  Powered by Linux