Hi Luiz,
> On Wed, Nov 19, 2014 at 9:09 AM, Luiz Augusto von Dentz <luiz.dentz@xxxxxxxxx> wrote:
> 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.
>
At least from what I've seen, the profiles using different roles (such
as proximity) handle the client and server operations somewhat
orthogonally, so that's why I wasn't worried. I prefer not rushing
anything too but I would still like to handle this in steps, so we'll
probably have some profile code during the transition process that
does client stuff through shared/gatt-client and server role stuff
through src/attrib-server. That's why we went through the trouble of
turning GAttrib into a wrapper around bt_att so that we can do this in
small steps :)
>> 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.
>
A unit test definitely makes sense. Would we want a standalone
unit/test-gatt-coex or would we want to work this into unit/test-gatt?
We need to start writing server role tests as well, so let's decide
where both of those should go.
In addition we could add this as a feature to tools/btgatt-server and
tools/btgatt-client as well. I kind of like that they are separate but
perhaps we'll want to merge these tools in the future.
>> 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.
>
Sorry, I'm not fully understanding the attach/detach bit. Currently
you could do bt_gatt_server_new(att, db) and that "attaches" the
server to the bt_att and when you're done you unref the bt_gatt_server
(same for bt_gatt_client). So you could create both on the same bt_att
now and that should work, they should be able to coexist just fine.
>>>
>>>> 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
Thanks,
Arman
--
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
