Return-Path: MIME-Version: 1.0 In-Reply-To: References: <1416252138-17477-1-git-send-email-armansito@chromium.org> <1416252138-17477-7-git-send-email-armansito@chromium.org> Date: Wed, 19 Nov 2014 09:31:29 -0800 Message-ID: Subject: Re: [PATCH BlueZ 06/12] core: device: Use shared/gatt-client for GATT. From: Arman Uguray To: Luiz Augusto von Dentz Cc: "linux-bluetooth@vger.kernel.org" Content-Type: text/plain; charset=UTF-8 Sender: linux-bluetooth-owner@vger.kernel.org List-ID: Hi Luiz, > On Wed, Nov 19, 2014 at 9:09 AM, Luiz Augusto von Dentz wrote: > Hi Arman, > > On Wed, Nov 19, 2014 at 6:42 PM, Arman Uguray wrote: >> Hi Luiz, >> >>> On Wed, Nov 19, 2014 at 7:43 AM, Luiz Augusto von Dentz wrote: >>> Hi Arman, >>> >>> On Mon, Nov 17, 2014 at 9:22 PM, Arman Uguray 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@vger.kernel.org >>>> More majordomo info at http://vger.kernel.org/majordomo-info.html >>> >>> >>> >>> -- >>> Luiz Augusto von Dentz >> >> Thanks, >> Arman > > > > -- > Luiz Augusto von Dentz Thanks, Arman