Hi Luiz,
> On Wed, Dec 17, 2014 at 4:57 AM, Luiz Augusto von Dentz <luiz.dentz@xxxxxxxxx> wrote:
> Hi Arman,
>
> On Wed, Dec 17, 2014 at 4:07 AM, Arman Uguray <armansito@xxxxxxxxxxxx> wrote:
>> This patch correctly integrates all profile calls into the various GATT
>> client events (ready, service-added, service-removed) so that profiles
>> can perform the necessary updates. The major changes introduced in this
>> this patch are:
>>
>> 1. Added new profile probe/remove functions for GATT services, which
>> operate on a btd_device's client db and initialize btd_service
>> instances with start & end handles:
>> - device_probe_gatt_profiles
>> - device_probe_gatt_profile
>> - device_remove_gatt_profile
>> - device_accept_gatt_profiles
>>
>> 2. device_probe_gatt_profiles is called when the gatt-client becomes
>> ready. Profiles are then notified with the "accept" call.
>>
>> 3. device_probe_gatt_profile is called when a new GATT service is
>> added to the db.
>>
>> 4. device_remove_gatt_profile is called when a GATT service is removed
>> from the db.
>>
>> 5. Profiles are notified that the gatt-client is ready via the
>> btd_service "accept" function on a per-service basis. Currently this
>> is always called immediately after a probe.
>
> After probe? I thought the idea was to indicate that a connection
> becomes available, if it is called after probe that means it is called
> just once?
>
At least the way I did it is I call probe every time the gatt-client
becomes ready and, from then on, when a new service is added. If the
gatt-client is ready at that time, I also call accept. My idea here
was that if the device is bonded and we have a live cache, then
profiles will be probed beforehand and then receive the accept call
later when there's a connection and gatt-client becomes ready.
Does this behavior make sense or did you have something different in mind?
>> ---
>> src/device.c | 261 ++++++++++++++++++++++++++++++++++++++++++++++++-----------
>> 1 file changed, 212 insertions(+), 49 deletions(-)
>>
>> diff --git a/src/device.c b/src/device.c
>> index 854d9f3..6242c76 100644
>> --- a/src/device.c
>> +++ b/src/device.c
>> @@ -292,6 +292,27 @@ static GSList *find_service_with_state(GSList *list,
>> return NULL;
>> }
>>
>> +static GSList *find_service_with_gatt_handles(GSList *list,
>> + uint16_t start_handle,
>> + uint16_t end_handle)
>> +{
>> + GSList *l;
>> + uint16_t svc_start, svc_end;
>> +
>> + for (l = list; l != NULL; l = g_slist_next(l)) {
>> + struct btd_service *service = l->data;
>> +
>> + if (!btd_service_get_gatt_handles(service, &svc_start,
>> + &svc_end))
>> + continue;
>> +
>> + if (svc_start == start_handle && svc_end == end_handle)
>> + return l;
>> + }
>> +
>> + return NULL;
>> +}
>> +
>> static void update_technologies(GKeyFile *file, struct btd_device *dev)
>> {
>> const char *list[2];
>> @@ -2390,15 +2411,168 @@ static void add_primary(struct gatt_db_attribute *attr, void *user_data)
>> *new_services = g_slist_append(*new_services, prim);
>> }
>>
>> +static void device_add_uuids(struct btd_device *device, GSList *uuids)
>> +{
>> + GSList *l;
>> +
>> + for (l = uuids; l != NULL; l = g_slist_next(l)) {
>> + GSList *match = g_slist_find_custom(device->uuids, l->data,
>> + bt_uuid_strcmp);
>> + if (match)
>> + continue;
>> +
>> + device->uuids = g_slist_insert_sorted(device->uuids,
>> + g_strdup(l->data),
>> + bt_uuid_strcmp);
>> + }
>> +
>> + g_dbus_emit_property_changed(dbus_conn, device->path,
>> + DEVICE_INTERFACE, "UUIDs");
>> +}
>> +
>> static void store_services(struct btd_device *device);
>>
>> +struct gatt_probe_data {
>> + struct btd_device *dev;
>> + bool add_uuid_to_device;
>> + GSList *uuids;
>> + struct gatt_db_attribute *cur_attr;
>> + char cur_uuid[MAX_LEN_UUID_STR];
>> + uint16_t start_handle, end_handle;
>> +};
>> +
>> +static bool device_match_profile(struct btd_device *device,
>> + struct btd_profile *profile,
>> + GSList *uuids)
>> +{
>> + if (profile->remote_uuid == NULL)
>> + return false;
>> +
>> + if (g_slist_find_custom(uuids, profile->remote_uuid,
>> + bt_uuid_strcmp) == NULL)
>> + return false;
>> +
>> + return true;
>> +}
>> +
>> +static void dev_probe_gatt(struct btd_profile *p, void *user_data)
>> +{
>> + struct gatt_probe_data *data = user_data;
>> + struct btd_service *service;
>> +
>> + if (p->device_probe == NULL)
>> + return;
>> +
>> + if (!device_match_profile(data->dev, p, g_slist_last(data->uuids)))
>> + return;
>
> Using g_slist_last is not very smart since afaik GSList has to iterate
> to to access the last.
>
I didn't realize that. I'll fix this in v5.
>> +
>> + service = service_create_gatt(data->dev, p, data->start_handle,
>> + data->end_handle);
>> + if (!service)
>> + return;
>> +
>> + if (service_probe(service) < 0) {
>> + btd_service_unref(service);
>> + return;
>> + }
>> +
>> + data->dev->services = g_slist_append(data->dev->services, service);
>> +}
>> +
>> +static void dev_probe_gatt_profile(struct gatt_db_attribute *attr,
>> + void *user_data)
>> +{
>> + struct gatt_probe_data *data = user_data;
>> + bt_uuid_t uuid;
>> + GSList *l;
>> +
>> + gatt_db_attribute_get_service_data(attr, &data->start_handle,
>> + &data->end_handle, NULL,
>> + &uuid);
>> + bt_uuid_to_string(&uuid, data->cur_uuid, sizeof(data->cur_uuid));
>> +
>> + data->uuids = g_slist_append(data->uuids, g_strdup(data->cur_uuid));
>
> If the order in the list is not really important than use prepend and
> g_slist_first in dev_probe_gatt, actually do we even need this list
> since we are adding one by one the UUIDs?
>
We probably may not need it in this case. The list is only needed when
we call device_probe_gatt_profiles so that the UUIDs property can be
updated all at once. So I can conditionally populate this based on
data->add_uuid_to_device.
>> + data->cur_attr = attr;
>> +
>> + btd_profile_foreach(dev_probe_gatt, data);
>> +
>> + if (!data->add_uuid_to_device)
>> + return;
>> +
>> + l = g_slist_append(l, g_strdup(data->cur_uuid));
>> + device_add_uuids(data->dev, l);
>> +}
>> +
>> +static void device_probe_gatt_profile(struct btd_device *device,
>> + struct gatt_db_attribute *attr)
>> +{
>> + struct gatt_probe_data data;
>> +
>> + memset(&data, 0, sizeof(data));
>> +
>> + data.dev = device;
>> + data.add_uuid_to_device = true;
>> +
>> + dev_probe_gatt_profile(attr, &data);
>> + g_slist_free_full(data.uuids, g_free);
>> +}
>> +
>> +static void device_probe_gatt_profiles(struct btd_device *device)
>> +{
>> + struct gatt_probe_data data;
>> + char addr[18];
>> +
>> + ba2str(&device->bdaddr, addr);
>> +
>> + if (device->blocked) {
>> + DBG("Skipping profiles for blocked device %s", addr);
>> + return;
>> + }
>> +
>> + memset(&data, 0, sizeof(data));
>> +
>> + data.dev = device;
>> +
>> + gatt_db_foreach_service(device->db, NULL, dev_probe_gatt_profile,
>> + &data);
>> + device_add_uuids(device, data.uuids);
>> + g_slist_free_full(data.uuids, g_free);
>> +}
>> +
>> +static void device_accept_gatt_profiles(struct btd_device *device)
>> +{
>> + GSList *l;
>> +
>> + for (l = device->services; l != NULL; l = g_slist_next(l))
>> + service_accept(l->data);
>> +}
>> +
>> +static void device_remove_gatt_profile(struct btd_device *device,
>> + struct gatt_db_attribute *attr)
>> +{
>> + uint16_t start, end;
>> + struct btd_service *service;
>> + GSList *l;
>> +
>> + gatt_db_attribute_get_service_handles(attr, &start, &end);
>> +
>> + l = find_service_with_gatt_handles(device->services, start, end);
>> + if (!l)
>> + return;
>> +
>> + service = l->data;
>> + device->services = g_slist_delete_link(device->services, l);
>> + device->pending = g_slist_remove(device->pending, service);
>> + service_remove(service);
>> +}
>> +
>> static void gatt_service_added(struct gatt_db_attribute *attr, void *user_data)
>> {
>> struct btd_device *device = user_data;
>> - struct gatt_primary *prim;
>> GSList *new_service = NULL;
>> - GSList *profiles_added = NULL;
>> uint16_t start, end;
>> + GSList *l;
>>
>> if (!bt_gatt_client_is_ready(device->client))
>> return;
>> @@ -2417,14 +2591,13 @@ static void gatt_service_added(struct gatt_db_attribute *attr, void *user_data)
>>
>> device_register_primaries(device, new_service, -1);
>>
>> - prim = new_service->data;
>> - profiles_added = g_slist_append(profiles_added, g_strdup(prim->uuid));
>> + device_probe_gatt_profile(device, attr);
>>
>> - device_probe_profiles(device, profiles_added);
>> + l = find_service_with_gatt_handles(device->services, start, end);
>> + if (l)
>> + service_accept(l->data);
>>
>> store_services(device);
>> -
>> - g_slist_free_full(profiles_added, g_free);
>> }
>>
>> static gint prim_attr_cmp(gconstpointer a, gconstpointer b)
>> @@ -2438,6 +2611,14 @@ static gint prim_attr_cmp(gconstpointer a, gconstpointer b)
>> return !(prim->range.start == start && prim->range.end == end);
>> }
>>
>> +static gint prim_uuid_cmp(gconstpointer a, gconstpointer b)
>> +{
>> + const struct gatt_primary *prim = a;
>> + const char *uuid = b;
>> +
>> + return bt_uuid_strcmp(prim->uuid, uuid);
>> +}
>> +
>> static void gatt_service_removed(struct gatt_db_attribute *attr,
>> void *user_data)
>> {
>> @@ -2461,20 +2642,24 @@ static void gatt_service_removed(struct gatt_db_attribute *attr,
>> prim = l->data;
>> device->primaries = g_slist_delete_link(device->primaries, l);
>>
>> - /* Remove the corresponding UUIDs entry */
>> - l = g_slist_find_custom(device->uuids, prim->uuid, bt_uuid_strcmp);
>> - device->uuids = g_slist_delete_link(device->uuids, l);
>> - g_free(prim);
>> -
>> - store_services(device);
>> + device_remove_gatt_profile(device, attr);
>>
>> /*
>> - * TODO: Notify the profiles somehow. It may be sufficient for each
>> - * profile to register a service_removed handler.
>> + * Remove the corresponding UUIDs entry, only if this is the last
>> + * service with this UUID.
>> */
>> + l = g_slist_find_custom(device->uuids, prim->uuid, bt_uuid_strcmp);
>>
>> - g_dbus_emit_property_changed(dbus_conn, device->path,
>> + if (!g_slist_find_custom(device->primaries, prim->uuid,
>> + prim_uuid_cmp)) {
>> + device->uuids = g_slist_delete_link(device->uuids, l);
>> + g_dbus_emit_property_changed(dbus_conn, device->path,
>> DEVICE_INTERFACE, "UUIDs");
>> + }
>> +
>> + g_free(prim);
>> +
>> + store_services(device);
>> }
>>
>> static struct btd_device *device_new(struct btd_adapter *adapter,
>> @@ -2975,20 +3160,6 @@ GSList *btd_device_get_uuids(struct btd_device *device)
>> return device->uuids;
>> }
>>
>> -static bool device_match_profile(struct btd_device *device,
>> - struct btd_profile *profile,
>> - GSList *uuids)
>> -{
>> - if (profile->remote_uuid == NULL)
>> - return false;
>> -
>> - if (g_slist_find_custom(uuids, profile->remote_uuid,
>> - bt_uuid_strcmp) == NULL)
>> - return false;
>> -
>> - return true;
>> -}
>> -
>> struct probe_data {
>> struct btd_device *dev;
>> GSList *uuids;
>> @@ -3065,7 +3236,6 @@ void device_remove_profile(gpointer a, gpointer b)
>> void device_probe_profiles(struct btd_device *device, GSList *uuids)
>> {
>> struct probe_data d = { device, uuids };
>> - GSList *l;
>> char addr[18];
>>
>> ba2str(&device->bdaddr, addr);
>> @@ -3080,19 +3250,7 @@ void device_probe_profiles(struct btd_device *device, GSList *uuids)
>> btd_profile_foreach(dev_probe, &d);
>>
>> add_uuids:
>> - for (l = uuids; l != NULL; l = g_slist_next(l)) {
>> - GSList *match = g_slist_find_custom(device->uuids, l->data,
>> - bt_uuid_strcmp);
>> - if (match)
>> - continue;
>> -
>> - device->uuids = g_slist_insert_sorted(device->uuids,
>> - g_strdup(l->data),
>> - bt_uuid_strcmp);
>> - }
>> -
>> - g_dbus_emit_property_changed(dbus_conn, device->path,
>> - DEVICE_INTERFACE, "UUIDs");
>> + device_add_uuids(device, uuids);
>> }
>>
>> static void store_sdp_record(GKeyFile *key_file, sdp_record_t *rec)
>> @@ -3412,6 +3570,13 @@ static void search_cb(sdp_list_t *recs, int err, gpointer user_data)
>> if (primaries)
>> device_register_primaries(device, primaries, ATT_PSM);
>>
>> + /*
>> + * TODO: The btd_service instances for GATT services need to be
>> + * initialized with the service handles. Eventually this code should
>> + * perform ATT protocol service discovery over the ATT PSM to obtain
>> + * the full list of services and populate a client-role gatt_db over
>> + * BR/EDR.
>> + */
>> device_probe_profiles(device, req->profiles_added);
>>
>> /* Propagate services changes */
>> @@ -3640,7 +3805,7 @@ static void register_gatt_services(struct browse_req *req)
>>
>> device_register_primaries(device, services, -1);
>>
>> - device_probe_profiles(device, req->profiles_added);
>> + device_probe_gatt_profiles(device);
>>
>> device_svc_resolved(device, device->bdaddr_type, 0);
>>
>> @@ -3675,10 +3840,8 @@ static void gatt_client_ready_cb(bool success, uint8_t att_ecode,
>> if (device->browse)
>> register_gatt_services(device->browse);
>>
>> - /*
>> - * TODO: Change attio callbacks to accept a gatt-client instead of a
>> - * GAttrib.
>> - */
>> + device_accept_gatt_profiles(device);
>> +
>> g_slist_foreach(device->attios, attio_connected, device->attrib);
>> }
>>
>> --
>> 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@xxxxxxxxxxxxxxx
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
>
>
> --
> 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
