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 19:09:11 +0200 Message-ID: Subject: Re: [PATCH BlueZ 06/12] core: device: Use shared/gatt-client for GATT. From: Luiz Augusto von Dentz To: Arman Uguray Cc: "linux-bluetooth@vger.kernel.org" Content-Type: text/plain; charset=UTF-8 Sender: linux-bluetooth-owner@vger.kernel.org List-ID: 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. > 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@vger.kernel.org >>> More majordomo info at http://vger.kernel.org/majordomo-info.html >> >> >> >> -- >> Luiz Augusto von Dentz > > Thanks, > Arman -- Luiz Augusto von Dentz