[PATCH BlueZ 1/2] shared/gatt-client: Remove services after Service Changed indication

[PATCH BlueZ 1/2] shared/gatt-client: Remove services after Service Changed indication

[Marcin Kraglak]

Clear services in range of Service Changed indication.
>From specification, Version 4.2 [Vol 3, Part G] 2.5.2 Attribute Caching:
"The client, upon receiving a Handle Value Indication containing the range of
affected Attribute Handles, shall consider the attribute cache invalid over
the affected Attribute Handle range".
---
 src/shared/gatt-client.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/src/shared/gatt-client.c b/src/shared/gatt-client.c
index 0134721..7d23702 100644
--- a/src/shared/gatt-client.c
+++ b/src/shared/gatt-client.c
@@ -1475,8 +1475,6 @@ static void service_changed_complete(struct discovery_op *op, bool success,
 		util_debug(client->debug_callback, client->debug_data,
 			"Failed to discover services within changed range - "
 			"error: 0x%02x", att_ecode);
-
-		gatt_db_clear_range(client->db, start_handle, end_handle);
 	}
 
 	/* Notify the upper layer of changed services */
@@ -1514,7 +1512,8 @@ static void service_changed_failure(struct discovery_op *op)
 {
 	struct bt_gatt_client *client = op->client;
 
-	gatt_db_clear_range(client->db, op->start, op->end);
+	util_debug(client->debug_callback, client->debug_data,
+					"Failed to process Service Changed");
 }
 
 static void process_service_changed(struct bt_gatt_client *client,
@@ -1523,6 +1522,8 @@ static void process_service_changed(struct bt_gatt_client *client,
 {
 	struct discovery_op *op;
 
+	gatt_db_clear_range(client->db, start_handle, end_handle);
+
 	op = discovery_op_create(client, start_handle, end_handle,
 						service_changed_complete,
 						service_changed_failure);
-- 
2.4.3

[PATCH BlueZ 2/2] core/gatt-client: Add support for Includes property

[Marcin Kraglak]

Add implementation of Includes property in GATT service interface.
---
 src/gatt-client.c | 144 +++++++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 136 insertions(+), 8 deletions(-)

diff --git a/src/gatt-client.c b/src/gatt-client.c
index 114981c..15219e2 100644
--- a/src/gatt-client.c
+++ b/src/gatt-client.c
@@ -72,6 +72,15 @@ struct service {
 	bt_uuid_t uuid;
 	char *path;
 	struct queue *chrcs;
+	struct queue *incl_services;
+};
+
+struct incl_service {
+	uint16_t handle;
+	uint16_t start_handle;
+	uint16_t end_handle;
+	struct service *service;
+	struct service *incl_service;
 };
 
 typedef bool (*async_dbus_op_complete_t)(void *data);
@@ -1398,10 +1407,39 @@ static gboolean service_get_primary(const GDBusPropertyTable *property,
 	return TRUE;
 }
 
+static void append_incl_service_path(void *data, void *user_data)
+{
+	struct incl_service *incl_service = data;
+	DBusMessageIter *array = user_data;
+
+	if (!incl_service->incl_service)
+		return;
+
+	dbus_message_iter_append_basic(array, DBUS_TYPE_OBJECT_PATH,
+					&incl_service->incl_service->path);
+}
+
+static gboolean service_get_includes(const GDBusPropertyTable *property,
+					DBusMessageIter *iter, void *data)
+{
+	struct service *service = data;
+	DBusMessageIter array;
+
+	dbus_message_iter_open_container(iter, DBUS_TYPE_ARRAY, "{o}", &array);
+
+	queue_foreach(service->incl_services, append_incl_service_path, &array);
+
+	dbus_message_iter_close_container(iter, &array);
+
+	return TRUE;
+
+}
+
 static const GDBusPropertyTable service_properties[] = {
 	{ "UUID", "s", service_get_uuid },
 	{ "Device", "o", service_get_device },
 	{ "Primary", "b", service_get_primary },
+	{ "Includes", "ao", service_get_includes },
 	{ }
 };
 
@@ -1410,10 +1448,40 @@ static void service_free(void *data)
 	struct service *service = data;
 
 	queue_destroy(service->chrcs, NULL);  /* List should be empty here */
+	queue_destroy(service->incl_services, NULL);
 	g_free(service->path);
 	free(service);
 }
 
+static bool match_service_handle(const void *a, const void *b)
+{
+	const struct service *service = a;
+	uint16_t start_handle = PTR_TO_UINT(b);
+
+	return service->start_handle == start_handle;
+}
+
+static void add_included_service(struct gatt_db_attribute *attrib,
+							void *user_data)
+{
+	struct service *service = user_data;
+	struct incl_service *incl_service;
+	struct btd_gatt_client *client = service->client;
+
+	incl_service = new0(struct incl_service, 1);
+	incl_service->service = service;
+
+	gatt_db_attribute_get_incl_data(attrib, &incl_service->handle,
+					&incl_service->start_handle,
+					&incl_service->end_handle);
+
+	incl_service->incl_service = queue_find(client->services,
+				match_service_handle,
+				UINT_TO_PTR(incl_service->start_handle));
+
+	queue_push_tail(service->incl_services, incl_service);
+}
+
 static struct service *service_create(struct gatt_db_attribute *attr,
 						struct btd_gatt_client *client)
 {
@@ -1423,6 +1491,7 @@ static struct service *service_create(struct gatt_db_attribute *attr,
 
 	service = new0(struct service, 1);
 	service->chrcs = queue_new();
+	service->incl_services = queue_new();
 	service->client = client;
 
 	gatt_db_attribute_get_service_data(attr, &service->start_handle,
@@ -1434,6 +1503,8 @@ static struct service *service_create(struct gatt_db_attribute *attr,
 	service->path = g_strdup_printf("%s/service%04x", device_path,
 							service->start_handle);
 
+	gatt_db_service_foreach_incl(attr, add_included_service, service);
+
 	if (!g_dbus_register_interface(btd_get_dbus_connection(), service->path,
 						GATT_SERVICE_IFACE,
 						NULL, NULL,
@@ -1459,6 +1530,63 @@ static struct service *service_create(struct gatt_db_attribute *attr,
 	return service;
 }
 
+static void incl_service_on_unregister(void *data, void *user_data)
+{
+	struct incl_service *incl_service = (struct incl_service *) data;
+	struct service *service = (struct service *) user_data;
+
+	if (incl_service->incl_service == service) {
+		incl_service->incl_service = NULL;
+
+		g_dbus_emit_property_changed(btd_get_dbus_connection(),
+						incl_service->service->path,
+						GATT_SERVICE_IFACE,
+						"Includes");
+	}
+}
+
+static void incl_service_on_register(void *data, void *user_data)
+{
+	struct incl_service *incl_service = (struct incl_service *) data;
+	struct service *service = (struct service *) user_data;
+
+	if (!incl_service->incl_service &&
+			incl_service->start_handle == service->start_handle) {
+		incl_service->incl_service = service;
+
+		g_dbus_emit_property_changed(btd_get_dbus_connection(),
+						incl_service->service->path,
+						GATT_SERVICE_IFACE,
+						"Includes");
+	}
+}
+
+struct foreach_incl_service_data {
+	queue_foreach_func_t func;
+	void *user_data;
+};
+
+static void incl_service_cb(void *data, void *user_data)
+{
+	struct service *service = (struct service *) data;
+	struct foreach_incl_service_data *incl_data =
+				(struct foreach_incl_service_data *) user_data;
+
+	queue_foreach(service->incl_services, incl_data->func,
+							incl_data->user_data);
+}
+
+static void client_foreach_incl_service(struct btd_gatt_client *client,
+				queue_foreach_func_t func, void *user_data)
+{
+	struct foreach_incl_service_data incl_data = {
+		.user_data = user_data,
+		.func = func,
+	};
+
+	queue_foreach(client->services, incl_service_cb, &incl_data);
+}
+
 static void unregister_service(void *data)
 {
 	struct service *service = data;
@@ -1466,6 +1594,11 @@ static void unregister_service(void *data)
 	DBG("Removing GATT service: %s", service->path);
 
 	queue_remove_all(service->chrcs, NULL, NULL, unregister_characteristic);
+	queue_remove_all(service->incl_services, NULL, NULL, free);
+
+	/* Make sure that no one included service points to this one */
+	client_foreach_incl_service(service->client,
+					incl_service_on_unregister, service);
 
 	g_dbus_unregister_interface(btd_get_dbus_connection(), service->path,
 							GATT_SERVICE_IFACE);
@@ -1564,6 +1697,9 @@ static void export_service(struct gatt_db_attribute *attr, void *user_data)
 		return;
 	}
 
+	/* Check if any included service points to this one */
+	client_foreach_incl_service(client, incl_service_on_register, service);
+
 	queue_push_tail(client->services, service);
 }
 
@@ -1691,14 +1827,6 @@ void btd_gatt_client_service_added(struct btd_gatt_client *client,
 	export_service(attrib, client);
 }
 
-static bool match_service_handle(const void *a, const void *b)
-{
-	const struct service *service = a;
-	uint16_t start_handle = PTR_TO_UINT(b);
-
-	return service->start_handle == start_handle;
-}
-
 void btd_gatt_client_service_removed(struct btd_gatt_client *client,
 					struct gatt_db_attribute *attrib)
 {
-- 
2.4.3

Re: [PATCH BlueZ 1/2] shared/gatt-client: Remove services after Service Changed indication

[Luiz Augusto von Dentz]

Hi Marcin,

On Tue, Apr 25, 2017 at 2:56 PM, Marcin Kraglak
<marcin.kraglak@tieto.com> wrote:
> Clear services in range of Service Changed indication.
> From specification, Version 4.2 [Vol 3, Part G] 2.5.2 Attribute Caching:
> "The client, upon receiving a Handle Value Indication containing the range of
> affected Attribute Handles, shall consider the attribute cache invalid over
> the affected Attribute Handle range".
> ---
>  src/shared/gatt-client.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/src/shared/gatt-client.c b/src/shared/gatt-client.c
> index 0134721..7d23702 100644
> --- a/src/shared/gatt-client.c
> +++ b/src/shared/gatt-client.c
> @@ -1475,8 +1475,6 @@ static void service_changed_complete(struct discovery_op *op, bool success,
>                 util_debug(client->debug_callback, client->debug_data,
>                         "Failed to discover services within changed range - "
>                         "error: 0x%02x", att_ecode);
> -
> -               gatt_db_clear_range(client->db, start_handle, end_handle);
>         }
>
>         /* Notify the upper layer of changed services */
> @@ -1514,7 +1512,8 @@ static void service_changed_failure(struct discovery_op *op)
>  {
>         struct bt_gatt_client *client = op->client;
>
> -       gatt_db_clear_range(client->db, op->start, op->end);
> +       util_debug(client->debug_callback, client->debug_data,
> +                                       "Failed to process Service Changed");
>  }
>
>  static void process_service_changed(struct bt_gatt_client *client,
> @@ -1523,6 +1522,8 @@ static void process_service_changed(struct bt_gatt_client *client,
>  {
>         struct discovery_op *op;
>
> +       gatt_db_clear_range(client->db, start_handle, end_handle);
> +
>         op = discovery_op_create(client, start_handle, end_handle,
>                                                 service_changed_complete,
>                                                 service_changed_failure);
> --
> 2.4.3

The code used to do exactly that, but it turn out it very inefficient
with devices that just loose the entire database when rebooted so we
started doing cache validation so we don't remove the services upfront
but check if the service range has changed so we prevent rediscovering
everything.


-- 
Luiz Augusto von Dentz

Re: [PATCH BlueZ 2/2] core/gatt-client: Add support for Includes property

[Luiz Augusto von Dentz]

Hi Marcin,

On Tue, Apr 25, 2017 at 2:56 PM, Marcin Kraglak
<marcin.kraglak@tieto.com> wrote:
> Add implementation of Includes property in GATT service interface.
> ---
>  src/gatt-client.c | 144 +++++++++++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 136 insertions(+), 8 deletions(-)
>
> diff --git a/src/gatt-client.c b/src/gatt-client.c
> index 114981c..15219e2 100644
> --- a/src/gatt-client.c
> +++ b/src/gatt-client.c
> @@ -72,6 +72,15 @@ struct service {
>         bt_uuid_t uuid;
>         char *path;
>         struct queue *chrcs;
> +       struct queue *incl_services;
> +};
> +
> +struct incl_service {
> +       uint16_t handle;
> +       uint16_t start_handle;
> +       uint16_t end_handle;
> +       struct service *service;
> +       struct service *incl_service;

This looks wrong, either we have the struct service representing the
including or we create a new as above but not both.

>  };
>
>  typedef bool (*async_dbus_op_complete_t)(void *data);
> @@ -1398,10 +1407,39 @@ static gboolean service_get_primary(const GDBusPropertyTable *property,
>         return TRUE;
>  }
>
> +static void append_incl_service_path(void *data, void *user_data)
> +{
> +       struct incl_service *incl_service = data;
> +       DBusMessageIter *array = user_data;
> +
> +       if (!incl_service->incl_service)
> +               return;
> +
> +       dbus_message_iter_append_basic(array, DBUS_TYPE_OBJECT_PATH,
> +                                       &incl_service->incl_service->path);
> +}
> +
> +static gboolean service_get_includes(const GDBusPropertyTable *property,
> +                                       DBusMessageIter *iter, void *data)
> +{
> +       struct service *service = data;
> +       DBusMessageIter array;
> +
> +       dbus_message_iter_open_container(iter, DBUS_TYPE_ARRAY, "{o}", &array);
> +
> +       queue_foreach(service->incl_services, append_incl_service_path, &array);
> +
> +       dbus_message_iter_close_container(iter, &array);
> +
> +       return TRUE;
> +
> +}
> +
>  static const GDBusPropertyTable service_properties[] = {
>         { "UUID", "s", service_get_uuid },
>         { "Device", "o", service_get_device },
>         { "Primary", "b", service_get_primary },
> +       { "Includes", "ao", service_get_includes },
>         { }
>  };
>
> @@ -1410,10 +1448,40 @@ static void service_free(void *data)
>         struct service *service = data;
>
>         queue_destroy(service->chrcs, NULL);  /* List should be empty here */
> +       queue_destroy(service->incl_services, NULL);
>         g_free(service->path);
>         free(service);
>  }
>
> +static bool match_service_handle(const void *a, const void *b)
> +{
> +       const struct service *service = a;
> +       uint16_t start_handle = PTR_TO_UINT(b);
> +
> +       return service->start_handle == start_handle;
> +}
> +
> +static void add_included_service(struct gatt_db_attribute *attrib,
> +                                                       void *user_data)
> +{
> +       struct service *service = user_data;
> +       struct incl_service *incl_service;
> +       struct btd_gatt_client *client = service->client;
> +
> +       incl_service = new0(struct incl_service, 1);
> +       incl_service->service = service;
> +
> +       gatt_db_attribute_get_incl_data(attrib, &incl_service->handle,
> +                                       &incl_service->start_handle,
> +                                       &incl_service->end_handle);

This only seems to be useful for matching the included services,
actually once we find what is the service why don't we keep the list
of only the struct service as the only thing we use is the path.

> +       incl_service->incl_service = queue_find(client->services,
> +                               match_service_handle,
> +                               UINT_TO_PTR(incl_service->start_handle));
> +
> +       queue_push_tail(service->incl_services, incl_service);
> +}
> +
>  static struct service *service_create(struct gatt_db_attribute *attr,
>                                                 struct btd_gatt_client *client)
>  {
> @@ -1423,6 +1491,7 @@ static struct service *service_create(struct gatt_db_attribute *attr,
>
>         service = new0(struct service, 1);
>         service->chrcs = queue_new();
> +       service->incl_services = queue_new();
>         service->client = client;
>
>         gatt_db_attribute_get_service_data(attr, &service->start_handle,
> @@ -1434,6 +1503,8 @@ static struct service *service_create(struct gatt_db_attribute *attr,
>         service->path = g_strdup_printf("%s/service%04x", device_path,
>                                                         service->start_handle);
>
> +       gatt_db_service_foreach_incl(attr, add_included_service, service);
> +
>         if (!g_dbus_register_interface(btd_get_dbus_connection(), service->path,
>                                                 GATT_SERVICE_IFACE,
>                                                 NULL, NULL,
> @@ -1459,6 +1530,63 @@ static struct service *service_create(struct gatt_db_attribute *attr,
>         return service;
>  }
>
> +static void incl_service_on_unregister(void *data, void *user_data)
> +{
> +       struct incl_service *incl_service = (struct incl_service *) data;
> +       struct service *service = (struct service *) user_data;
> +
> +       if (incl_service->incl_service == service) {
> +               incl_service->incl_service = NULL;
> +
> +               g_dbus_emit_property_changed(btd_get_dbus_connection(),
> +                                               incl_service->service->path,
> +                                               GATT_SERVICE_IFACE,
> +                                               "Includes");
> +       }
> +}
> +
> +static void incl_service_on_register(void *data, void *user_data)
> +{
> +       struct incl_service *incl_service = (struct incl_service *) data;
> +       struct service *service = (struct service *) user_data;
> +
> +       if (!incl_service->incl_service &&
> +                       incl_service->start_handle == service->start_handle) {
> +               incl_service->incl_service = service;
> +
> +               g_dbus_emit_property_changed(btd_get_dbus_connection(),
> +                                               incl_service->service->path,
> +                                               GATT_SERVICE_IFACE,
> +                                               "Includes");
> +       }
> +}
> +
> +struct foreach_incl_service_data {
> +       queue_foreach_func_t func;
> +       void *user_data;
> +};
> +
> +static void incl_service_cb(void *data, void *user_data)
> +{
> +       struct service *service = (struct service *) data;
> +       struct foreach_incl_service_data *incl_data =
> +                               (struct foreach_incl_service_data *) user_data;
> +
> +       queue_foreach(service->incl_services, incl_data->func,
> +                                                       incl_data->user_data);
> +}
> +
> +static void client_foreach_incl_service(struct btd_gatt_client *client,
> +                               queue_foreach_func_t func, void *user_data)
> +{
> +       struct foreach_incl_service_data incl_data = {
> +               .user_data = user_data,
> +               .func = func,
> +       };
> +
> +       queue_foreach(client->services, incl_service_cb, &incl_data);
> +}
> +
>  static void unregister_service(void *data)
>  {
>         struct service *service = data;
> @@ -1466,6 +1594,11 @@ static void unregister_service(void *data)
>         DBG("Removing GATT service: %s", service->path);
>
>         queue_remove_all(service->chrcs, NULL, NULL, unregister_characteristic);
> +       queue_remove_all(service->incl_services, NULL, NULL, free);
> +
> +       /* Make sure that no one included service points to this one */
> +       client_foreach_incl_service(service->client,
> +                                       incl_service_on_unregister, service);
>
>         g_dbus_unregister_interface(btd_get_dbus_connection(), service->path,
>                                                         GATT_SERVICE_IFACE);
> @@ -1564,6 +1697,9 @@ static void export_service(struct gatt_db_attribute *attr, void *user_data)
>                 return;
>         }
>
> +       /* Check if any included service points to this one */
> +       client_foreach_incl_service(client, incl_service_on_register, service);
> +
>         queue_push_tail(client->services, service);
>  }
>
> @@ -1691,14 +1827,6 @@ void btd_gatt_client_service_added(struct btd_gatt_client *client,
>         export_service(attrib, client);
>  }
>
> -static bool match_service_handle(const void *a, const void *b)
> -{
> -       const struct service *service = a;
> -       uint16_t start_handle = PTR_TO_UINT(b);
> -
> -       return service->start_handle == start_handle;
> -}
> -
>  void btd_gatt_client_service_removed(struct btd_gatt_client *client,
>                                         struct gatt_db_attribute *attrib)
>  {
> --
> 2.4.3
>
> --
> 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

Re: [PATCH BlueZ 2/2] core/gatt-client: Add support for Includes property

[Marcin Kraglak]

Hi Luiz,

On 25 April 2017 at 20:51, Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote:
> Hi Marcin,
>
> On Tue, Apr 25, 2017 at 2:56 PM, Marcin Kraglak
> <marcin.kraglak@tieto.com> wrote:
>> Add implementation of Includes property in GATT service interface.
>> ---
>>  src/gatt-client.c | 144 +++++++++++++++++++++++++++++++++++++++++++++++++++---
>>  1 file changed, 136 insertions(+), 8 deletions(-)
>>
>> diff --git a/src/gatt-client.c b/src/gatt-client.c
>> index 114981c..15219e2 100644
>> --- a/src/gatt-client.c
>> +++ b/src/gatt-client.c
>> @@ -72,6 +72,15 @@ struct service {
>>         bt_uuid_t uuid;
>>         char *path;
>>         struct queue *chrcs;
>> +       struct queue *incl_services;
>> +};
>> +
>> +struct incl_service {
>> +       uint16_t handle;
>> +       uint16_t start_handle;
>> +       uint16_t end_handle;
>> +       struct service *service;
>> +       struct service *incl_service;
>
> This looks wrong, either we have the struct service representing the
> including or we create a new as above but not both.
>
>>  };
>>
>>  typedef bool (*async_dbus_op_complete_t)(void *data);
>> @@ -1398,10 +1407,39 @@ static gboolean service_get_primary(const GDBusPropertyTable *property,
>>         return TRUE;
>>  }
>>
>> +static void append_incl_service_path(void *data, void *user_data)
>> +{
>> +       struct incl_service *incl_service = data;
>> +       DBusMessageIter *array = user_data;
>> +
>> +       if (!incl_service->incl_service)
>> +               return;
>> +
>> +       dbus_message_iter_append_basic(array, DBUS_TYPE_OBJECT_PATH,
>> +                                       &incl_service->incl_service->path);
>> +}
>> +
>> +static gboolean service_get_includes(const GDBusPropertyTable *property,
>> +                                       DBusMessageIter *iter, void *data)
>> +{
>> +       struct service *service = data;
>> +       DBusMessageIter array;
>> +
>> +       dbus_message_iter_open_container(iter, DBUS_TYPE_ARRAY, "{o}", &array);
>> +
>> +       queue_foreach(service->incl_services, append_incl_service_path, &array);
>> +
>> +       dbus_message_iter_close_container(iter, &array);
>> +
>> +       return TRUE;
>> +
>> +}
>> +
>>  static const GDBusPropertyTable service_properties[] = {
>>         { "UUID", "s", service_get_uuid },
>>         { "Device", "o", service_get_device },
>>         { "Primary", "b", service_get_primary },
>> +       { "Includes", "ao", service_get_includes },
>>         { }
>>  };
>>
>> @@ -1410,10 +1448,40 @@ static void service_free(void *data)
>>         struct service *service = data;
>>
>>         queue_destroy(service->chrcs, NULL);  /* List should be empty here */
>> +       queue_destroy(service->incl_services, NULL);
>>         g_free(service->path);
>>         free(service);
>>  }
>>
>> +static bool match_service_handle(const void *a, const void *b)
>> +{
>> +       const struct service *service = a;
>> +       uint16_t start_handle = PTR_TO_UINT(b);
>> +
>> +       return service->start_handle == start_handle;
>> +}
>> +
>> +static void add_included_service(struct gatt_db_attribute *attrib,
>> +                                                       void *user_data)
>> +{
>> +       struct service *service = user_data;
>> +       struct incl_service *incl_service;
>> +       struct btd_gatt_client *client = service->client;
>> +
>> +       incl_service = new0(struct incl_service, 1);
>> +       incl_service->service = service;
>> +
>> +       gatt_db_attribute_get_incl_data(attrib, &incl_service->handle,
>> +                                       &incl_service->start_handle,
>> +                                       &incl_service->end_handle);
>
> This only seems to be useful for matching the included services,
> actually once we find what is the service why don't we keep the list
> of only the struct service as the only thing we use is the path.
Because included service may not be in the service list yet. We can
omit handle, end handle and not store it, but start handle is needed
for matching when included service will be registered.

>
>> +       incl_service->incl_service = queue_find(client->services,
>> +                               match_service_handle,
>> +                               UINT_TO_PTR(incl_service->start_handle));
>> +
>> +       queue_push_tail(service->incl_services, incl_service);
>> +}
>> +
>>  static struct service *service_create(struct gatt_db_attribute *attr,
>>                                                 struct btd_gatt_client *client)
>>  {
>> @@ -1423,6 +1491,7 @@ static struct service *service_create(struct gatt_db_attribute *attr,
>>
>>         service = new0(struct service, 1);
>>         service->chrcs = queue_new();
>> +       service->incl_services = queue_new();
>>         service->client = client;
>>
>>         gatt_db_attribute_get_service_data(attr, &service->start_handle,
>> @@ -1434,6 +1503,8 @@ static struct service *service_create(struct gatt_db_attribute *attr,
>>         service->path = g_strdup_printf("%s/service%04x", device_path,
>>                                                         service->start_handle);
>>
>> +       gatt_db_service_foreach_incl(attr, add_included_service, service);
>> +
>>         if (!g_dbus_register_interface(btd_get_dbus_connection(), service->path,
>>                                                 GATT_SERVICE_IFACE,
>>                                                 NULL, NULL,
>> @@ -1459,6 +1530,63 @@ static struct service *service_create(struct gatt_db_attribute *attr,
>>         return service;
>>  }
>>
>> +static void incl_service_on_unregister(void *data, void *user_data)
>> +{
>> +       struct incl_service *incl_service = (struct incl_service *) data;
>> +       struct service *service = (struct service *) user_data;
>> +
>> +       if (incl_service->incl_service == service) {
>> +               incl_service->incl_service = NULL;
>> +
>> +               g_dbus_emit_property_changed(btd_get_dbus_connection(),
>> +                                               incl_service->service->path,
>> +                                               GATT_SERVICE_IFACE,
>> +                                               "Includes");
>> +       }
>> +}
>> +
>> +static void incl_service_on_register(void *data, void *user_data)
>> +{
>> +       struct incl_service *incl_service = (struct incl_service *) data;
>> +       struct service *service = (struct service *) user_data;
>> +
>> +       if (!incl_service->incl_service &&
>> +                       incl_service->start_handle == service->start_handle) {
>> +               incl_service->incl_service = service;
>> +
>> +               g_dbus_emit_property_changed(btd_get_dbus_connection(),
>> +                                               incl_service->service->path,
>> +                                               GATT_SERVICE_IFACE,
>> +                                               "Includes");
>> +       }
>> +}
>> +
>> +struct foreach_incl_service_data {
>> +       queue_foreach_func_t func;
>> +       void *user_data;
>> +};
>> +
>> +static void incl_service_cb(void *data, void *user_data)
>> +{
>> +       struct service *service = (struct service *) data;
>> +       struct foreach_incl_service_data *incl_data =
>> +                               (struct foreach_incl_service_data *) user_data;
>> +
>> +       queue_foreach(service->incl_services, incl_data->func,
>> +                                                       incl_data->user_data);
>> +}
>> +
>> +static void client_foreach_incl_service(struct btd_gatt_client *client,
>> +                               queue_foreach_func_t func, void *user_data)
>> +{
>> +       struct foreach_incl_service_data incl_data = {
>> +               .user_data = user_data,
>> +               .func = func,
>> +       };
>> +
>> +       queue_foreach(client->services, incl_service_cb, &incl_data);
>> +}
>> +
>>  static void unregister_service(void *data)
>>  {
>>         struct service *service = data;
>> @@ -1466,6 +1594,11 @@ static void unregister_service(void *data)
>>         DBG("Removing GATT service: %s", service->path);
>>
>>         queue_remove_all(service->chrcs, NULL, NULL, unregister_characteristic);
>> +       queue_remove_all(service->incl_services, NULL, NULL, free);
>> +
>> +       /* Make sure that no one included service points to this one */
>> +       client_foreach_incl_service(service->client,
>> +                                       incl_service_on_unregister, service);
>>
>>         g_dbus_unregister_interface(btd_get_dbus_connection(), service->path,
>>                                                         GATT_SERVICE_IFACE);
>> @@ -1564,6 +1697,9 @@ static void export_service(struct gatt_db_attribute *attr, void *user_data)
>>                 return;
>>         }
>>
>> +       /* Check if any included service points to this one */
>> +       client_foreach_incl_service(client, incl_service_on_register, service);
>> +
>>         queue_push_tail(client->services, service);
>>  }
>>
>> @@ -1691,14 +1827,6 @@ void btd_gatt_client_service_added(struct btd_gatt_client *client,
>>         export_service(attrib, client);
>>  }
>>
>> -static bool match_service_handle(const void *a, const void *b)
>> -{
>> -       const struct service *service = a;
>> -       uint16_t start_handle = PTR_TO_UINT(b);
>> -
>> -       return service->start_handle == start_handle;
>> -}
>> -
>>  void btd_gatt_client_service_removed(struct btd_gatt_client *client,
>>                                         struct gatt_db_attribute *attrib)
>>  {
>> --
>> 2.4.3
>>
>> --
>> 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



-- 
BR
Marcin Kraglak

Re: [PATCH BlueZ 1/2] shared/gatt-client: Remove services after Service Changed indication

[Marcin Kraglak]

Hi Luiz,

On 25 April 2017 at 20:21, Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote:
> Hi Marcin,
>
> On Tue, Apr 25, 2017 at 2:56 PM, Marcin Kraglak
> <marcin.kraglak@tieto.com> wrote:
>> Clear services in range of Service Changed indication.
>> From specification, Version 4.2 [Vol 3, Part G] 2.5.2 Attribute Caching:
>> "The client, upon receiving a Handle Value Indication containing the range of
>> affected Attribute Handles, shall consider the attribute cache invalid over
>> the affected Attribute Handle range".
>> ---
>>  src/shared/gatt-client.c | 7 ++++---
>>  1 file changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/src/shared/gatt-client.c b/src/shared/gatt-client.c
>> index 0134721..7d23702 100644
>> --- a/src/shared/gatt-client.c
>> +++ b/src/shared/gatt-client.c
>> @@ -1475,8 +1475,6 @@ static void service_changed_complete(struct discovery_op *op, bool success,
>>                 util_debug(client->debug_callback, client->debug_data,
>>                         "Failed to discover services within changed range - "
>>                         "error: 0x%02x", att_ecode);
>> -
>> -               gatt_db_clear_range(client->db, start_handle, end_handle);
>>         }
>>
>>         /* Notify the upper layer of changed services */
>> @@ -1514,7 +1512,8 @@ static void service_changed_failure(struct discovery_op *op)
>>  {
>>         struct bt_gatt_client *client = op->client;
>>
>> -       gatt_db_clear_range(client->db, op->start, op->end);
>> +       util_debug(client->debug_callback, client->debug_data,
>> +                                       "Failed to process Service Changed");
>>  }
>>
>>  static void process_service_changed(struct bt_gatt_client *client,
>> @@ -1523,6 +1522,8 @@ static void process_service_changed(struct bt_gatt_client *client,
>>  {
>>         struct discovery_op *op;
>>
>> +       gatt_db_clear_range(client->db, start_handle, end_handle);
>> +
>>         op = discovery_op_create(client, start_handle, end_handle,
>>                                                 service_changed_complete,
>>                                                 service_changed_failure);
>> --
>> 2.4.3
>
> The code used to do exactly that, but it turn out it very inefficient
> with devices that just loose the entire database when rebooted so we
> started doing cache validation so we don't remove the services upfront
> but check if the service range has changed so we prevent rediscovering
> everything.
>
I see. Currently cached services, which were not found in new
discovery after reconnect/service change (they were simply removed),
are not unregistered. I guess it is not what we expect? If not I can
fix it in another patch.
>
> --
> Luiz Augusto von Dentz



-- 
BR
Marcin Kraglak

Re: [PATCH BlueZ 1/2] shared/gatt-client: Remove services after Service Changed indication

[Luiz Augusto von Dentz]

Hi Marcin,

On Wed, Apr 26, 2017 at 8:16 AM, Marcin Kraglak
<marcin.kraglak@tieto.com> wrote:
> Hi Luiz,
>
> On 25 April 2017 at 20:21, Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote:
>> Hi Marcin,
>>
>> On Tue, Apr 25, 2017 at 2:56 PM, Marcin Kraglak
>> <marcin.kraglak@tieto.com> wrote:
>>> Clear services in range of Service Changed indication.
>>> From specification, Version 4.2 [Vol 3, Part G] 2.5.2 Attribute Caching:
>>> "The client, upon receiving a Handle Value Indication containing the range of
>>> affected Attribute Handles, shall consider the attribute cache invalid over
>>> the affected Attribute Handle range".
>>> ---
>>>  src/shared/gatt-client.c | 7 ++++---
>>>  1 file changed, 4 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/src/shared/gatt-client.c b/src/shared/gatt-client.c
>>> index 0134721..7d23702 100644
>>> --- a/src/shared/gatt-client.c
>>> +++ b/src/shared/gatt-client.c
>>> @@ -1475,8 +1475,6 @@ static void service_changed_complete(struct discovery_op *op, bool success,
>>>                 util_debug(client->debug_callback, client->debug_data,
>>>                         "Failed to discover services within changed range - "
>>>                         "error: 0x%02x", att_ecode);
>>> -
>>> -               gatt_db_clear_range(client->db, start_handle, end_handle);
>>>         }
>>>
>>>         /* Notify the upper layer of changed services */
>>> @@ -1514,7 +1512,8 @@ static void service_changed_failure(struct discovery_op *op)
>>>  {
>>>         struct bt_gatt_client *client = op->client;
>>>
>>> -       gatt_db_clear_range(client->db, op->start, op->end);
>>> +       util_debug(client->debug_callback, client->debug_data,
>>> +                                       "Failed to process Service Changed");
>>>  }
>>>
>>>  static void process_service_changed(struct bt_gatt_client *client,
>>> @@ -1523,6 +1522,8 @@ static void process_service_changed(struct bt_gatt_client *client,
>>>  {
>>>         struct discovery_op *op;
>>>
>>> +       gatt_db_clear_range(client->db, start_handle, end_handle);
>>> +
>>>         op = discovery_op_create(client, start_handle, end_handle,
>>>                                                 service_changed_complete,
>>>                                                 service_changed_failure);
>>> --
>>> 2.4.3
>>
>> The code used to do exactly that, but it turn out it very inefficient
>> with devices that just loose the entire database when rebooted so we
>> started doing cache validation so we don't remove the services upfront
>> but check if the service range has changed so we prevent rediscovering
>> everything.
>>
> I see. Currently cached services, which were not found in new
> discovery after reconnect/service change (they were simply removed),
> are not unregistered. I guess it is not what we expect? If not I can
> fix it in another patch.

There is are calls to gatt_db_clear_range, so you are saying these do
not affect the D-Bus objects? If that is the case then yes we need to
fix it, but is it so that the services really changed because if they
don't there is no reason to remove the objects.

>> --
>> Luiz Augusto von Dentz
>
>
>
> --
> BR
> Marcin Kraglak



-- 
Luiz Augusto von Dentz

Re: [PATCH BlueZ 1/2] shared/gatt-client: Remove services after Service Changed indication

[Marcin Kraglak]

Hi Luiz,

On 26 April 2017 at 11:07, Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote:
> Hi Marcin,
>
> On Wed, Apr 26, 2017 at 8:16 AM, Marcin Kraglak
> <marcin.kraglak@tieto.com> wrote:
>> Hi Luiz,
>>
>> On 25 April 2017 at 20:21, Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote:
>>> Hi Marcin,
>>>
>>> On Tue, Apr 25, 2017 at 2:56 PM, Marcin Kraglak
>>> <marcin.kraglak@tieto.com> wrote:
>>>> Clear services in range of Service Changed indication.
>>>> From specification, Version 4.2 [Vol 3, Part G] 2.5.2 Attribute Caching:
>>>> "The client, upon receiving a Handle Value Indication containing the range of
>>>> affected Attribute Handles, shall consider the attribute cache invalid over
>>>> the affected Attribute Handle range".
>>>> ---
>>>>  src/shared/gatt-client.c | 7 ++++---
>>>>  1 file changed, 4 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/src/shared/gatt-client.c b/src/shared/gatt-client.c
>>>> index 0134721..7d23702 100644
>>>> --- a/src/shared/gatt-client.c
>>>> +++ b/src/shared/gatt-client.c
>>>> @@ -1475,8 +1475,6 @@ static void service_changed_complete(struct discovery_op *op, bool success,
>>>>                 util_debug(client->debug_callback, client->debug_data,
>>>>                         "Failed to discover services within changed range - "
>>>>                         "error: 0x%02x", att_ecode);
>>>> -
>>>> -               gatt_db_clear_range(client->db, start_handle, end_handle);
>>>>         }
>>>>
>>>>         /* Notify the upper layer of changed services */
>>>> @@ -1514,7 +1512,8 @@ static void service_changed_failure(struct discovery_op *op)
>>>>  {
>>>>         struct bt_gatt_client *client = op->client;
>>>>
>>>> -       gatt_db_clear_range(client->db, op->start, op->end);
>>>> +       util_debug(client->debug_callback, client->debug_data,
>>>> +                                       "Failed to process Service Changed");
>>>>  }
>>>>
>>>>  static void process_service_changed(struct bt_gatt_client *client,
>>>> @@ -1523,6 +1522,8 @@ static void process_service_changed(struct bt_gatt_client *client,
>>>>  {
>>>>         struct discovery_op *op;
>>>>
>>>> +       gatt_db_clear_range(client->db, start_handle, end_handle);
>>>> +
>>>>         op = discovery_op_create(client, start_handle, end_handle,
>>>>                                                 service_changed_complete,
>>>>                                                 service_changed_failure);
>>>> --
>>>> 2.4.3
>>>
>>> The code used to do exactly that, but it turn out it very inefficient
>>> with devices that just loose the entire database when rebooted so we
>>> started doing cache validation so we don't remove the services upfront
>>> but check if the service range has changed so we prevent rediscovering
>>> everything.
>>>
>> I see. Currently cached services, which were not found in new
>> discovery after reconnect/service change (they were simply removed),
>> are not unregistered. I guess it is not what we expect? If not I can
>> fix it in another patch.
>
> There is are calls to gatt_db_clear_range, so you are saying these do
> not affect the D-Bus objects? If that is the case then yes we need to
> fix it, but is it so that the services really changed because if they
> don't there is no reason to remove the objects.

I mean situation when you had A, B service and remove B (no new
service will be put in range of B) - then B will be still be exposed.
If there will be new one in range of B, B will be removed.
>
>>> --
>>> Luiz Augusto von Dentz
>>
>>
>>
>> --
>> BR
>> Marcin Kraglak
>
>
>
> --
> Luiz Augusto von Dentz



-- 
BR
Marcin Kraglak