[RFC BlueZ] doc/device-api: Replace GattServices with Discovering property

[RFC BlueZ] doc/device-api: Replace GattServices with Discovering property

[Luiz Augusto von Dentz]

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

GattServices is not really doing was it was meant to do which was to
track progress of service discovery since it only worked for the very
first time a device is connected but since we no longer remove the
attributes an application would have the false impression the service are
all resolved by the time it reconnects when in fact the service may have
changed.

Futhermore object tracking like it is doing has been obsolete by
ObjectManager so this propose to replace the service discovery tracking
with a boolean property which works both with SDP as well as GATT
discovery.
---
 doc/device-api.txt |  9 +++------
 src/device.c       | 58 +++++++++++-------------------------------------------
 2 files changed, 14 insertions(+), 53 deletions(-)

diff --git a/doc/device-api.txt b/doc/device-api.txt
index a8076a2..9e58664 100644
--- a/doc/device-api.txt
+++ b/doc/device-api.txt
@@ -212,10 +212,7 @@ Properties	string Address [readonly]
 			Service advertisement data. Keys are the UUIDs in
 			string format followed by its byte array value.
 
-		array{object} GattServices [readonly, optional]
+		bool Discovering [readonly, optional, experimental]
 
-			List of GATT service object paths. Each referenced
-			object exports the org.bluez.GattService1 interface and
-			represents a remote GATT service. This property will be
-			updated once all remote GATT services of this device
-			have been discovered and exported over D-Bus.
+			Indicate whether or not service discovery is in
+			progress.
diff --git a/src/device.c b/src/device.c
index 14e850e..4e49652 100644
--- a/src/device.c
+++ b/src/device.c
@@ -238,7 +238,6 @@ struct btd_device {
 	 * attribute cache support can be built.
 	 */
 	struct gatt_db *db;			/* GATT db cache */
-	bool gatt_cache_used;			/* true if discovery skipped */
 	struct bt_gatt_client *client;		/* GATT client instance */
 	struct bt_gatt_server *server;		/* GATT server instance */
 
@@ -534,6 +533,9 @@ static void browse_request_free(struct browse_req *req)
 	if (req->records)
 		sdp_list_free(req->records, (sdp_free_func_t) sdp_record_free);
 
+	g_dbus_emit_property_changed(dbus_conn, req->device->path,
+					DEVICE_INTERFACE, "Discovering");
+
 	g_free(req);
 }
 
@@ -949,38 +951,13 @@ static gboolean dev_property_exists_tx_power(const GDBusPropertyTable *property,
 	return TRUE;
 }
 
-static void append_service_path(const char *path, void *user_data)
-{
-	DBusMessageIter *array = user_data;
-
-	dbus_message_iter_append_basic(array, DBUS_TYPE_OBJECT_PATH, &path);
-}
-
-static gboolean dev_property_get_gatt_services(
-					const GDBusPropertyTable *property,
+static gboolean dev_property_get_discovering(const GDBusPropertyTable *property,
 					DBusMessageIter *iter, void *data)
 {
-	struct btd_device *dev = data;
-	DBusMessageIter array;
-
-	dbus_message_iter_open_container(iter, DBUS_TYPE_ARRAY, "o", &array);
-
-	btd_gatt_client_foreach_service(dev->client_dbus, append_service_path,
-									&array);
-
-	dbus_message_iter_close_container(iter, &array);
-
-	return TRUE;
-}
-
-static gboolean dev_property_exists_gatt_services(
-					const GDBusPropertyTable *property,
-					void *data)
-{
-	struct btd_device *dev = data;
+	struct btd_device *device = data;
+	gboolean val = device->browse ? true : false;
 
-	if (!dev->client || !bt_gatt_client_is_ready(dev->client))
-		return FALSE;
+	dbus_message_iter_append_basic(iter, DBUS_TYPE_BOOLEAN, &val);
 
 	return TRUE;
 }
@@ -2533,8 +2510,7 @@ static const GDBusPropertyTable device_properties[] = {
 	{ "TxPower", "n", dev_property_get_tx_power, NULL,
 					dev_property_exists_tx_power,
 					G_DBUS_PROPERTY_FLAG_EXPERIMENTAL },
-	{ "GattServices", "ao", dev_property_get_gatt_services, NULL,
-					dev_property_exists_gatt_services,
+	{ "Discovering", "b", dev_property_get_discovering, NULL, NULL,
 					G_DBUS_PROPERTY_FLAG_EXPERIMENTAL },
 
 	{ }
@@ -4550,19 +4526,6 @@ static void gatt_client_ready_cb(bool success, uint8_t att_ecode,
 	register_gatt_services(device);
 
 	btd_gatt_client_ready(device->client_dbus);
-
-	/*
-	 * Update the GattServices property. Do this asynchronously since this
-	 * should happen after the "Characteristics" and "Descriptors"
-	 * properties of all services have been asynchronously updated by
-	 * btd_gatt_client.
-	 *
-	 * Service discovery will be skipped and exported objects won't change
-	 * if the attribute cache was populated when bt_gatt_client gets
-	 * initialized, so no need to to send this signal if that's the case.
-	 */
-	if (!device->gatt_cache_used)
-		g_idle_add(gatt_services_changed, device);
 }
 
 static void gatt_client_service_changed(uint16_t start_handle,
@@ -4615,8 +4578,6 @@ static void gatt_client_init(struct btd_device *device)
 		return;
 	}
 
-	device->gatt_cache_used = !gatt_db_isempty(device->db);
-
 	btd_gatt_client_connected(device->client_dbus);
 }
 
@@ -4914,6 +4875,9 @@ static struct browse_req *browse_request_new(struct btd_device *device,
 
 	device->browse = req;
 
+	g_dbus_emit_property_changed(dbus_conn, device->path,
+					DEVICE_INTERFACE, "Discovering");
+
 	if (!msg)
 		return req;
 
-- 
2.5.0

Re: [RFC BlueZ] doc/device-api: Replace GattServices with Discovering property

[Vinicius Costa Gomes]

Hi Luiz,

Luiz Augusto von Dentz <luiz.dentz@gmail.com> writes:

> From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
>
> GattServices is not really doing was it was meant to do which was to
> track progress of service discovery since it only worked for the very
> first time a device is connected but since we no longer remove the
> attributes an application would have the false impression the service are
> all resolved by the time it reconnects when in fact the service may have
> changed.
>
> Futhermore object tracking like it is doing has been obsolete by
> ObjectManager so this propose to replace the service discovery tracking
> with a boolean property which works both with SDP as well as GATT
> discovery.
> ---
>  doc/device-api.txt |  9 +++------
>  src/device.c       | 58 +++++++++++-------------------------------------------
>  2 files changed, 14 insertions(+), 53 deletions(-)
>
> diff --git a/doc/device-api.txt b/doc/device-api.txt
> index a8076a2..9e58664 100644
> --- a/doc/device-api.txt
> +++ b/doc/device-api.txt
> @@ -212,10 +212,7 @@ Properties	string Address [readonly]
>  			Service advertisement data. Keys are the UUIDs in
>  			string format followed by its byte array value.
>
> -		array{object} GattServices [readonly, optional]
> +		bool Discovering [readonly, optional, experimental]
>
> -			List of GATT service object paths. Each referenced
> -			object exports the org.bluez.GattService1 interface and
> -			represents a remote GATT service. This property will be
> -			updated once all remote GATT services of this device
> -			have been discovered and exported over D-Bus.
> +			Indicate whether or not service discovery is in
> +			progress.

I wonder how useful it's going to be.

What I expect from a client is that it made a GetManagedObjects() call
when it entered the bus and added listeners to the InterfacesAdded(),
InterfacesRemoved() and PropertiesChanged() signals, I am not seeing
what kind of value this property is adding to that kind of client.

But yeah, it is better than GattServices.


Cheers,
--
Vinicius

Re: [RFC BlueZ] doc/device-api: Replace GattServices with Discovering property

[Luiz Augusto von Dentz]

Hi Vinicius,

On Thu, Mar 10, 2016 at 8:04 PM, Vinicius Costa Gomes
<vinicius.gomes@intel.com> wrote:
> Hi Luiz,
>
> Luiz Augusto von Dentz <luiz.dentz@gmail.com> writes:
>
>> From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
>>
>> GattServices is not really doing was it was meant to do which was to
>> track progress of service discovery since it only worked for the very
>> first time a device is connected but since we no longer remove the
>> attributes an application would have the false impression the service are
>> all resolved by the time it reconnects when in fact the service may have
>> changed.
>>
>> Futhermore object tracking like it is doing has been obsolete by
>> ObjectManager so this propose to replace the service discovery tracking
>> with a boolean property which works both with SDP as well as GATT
>> discovery.
>> ---
>>  doc/device-api.txt |  9 +++------
>>  src/device.c       | 58 +++++++++++-------------------------------------------
>>  2 files changed, 14 insertions(+), 53 deletions(-)
>>
>> diff --git a/doc/device-api.txt b/doc/device-api.txt
>> index a8076a2..9e58664 100644
>> --- a/doc/device-api.txt
>> +++ b/doc/device-api.txt
>> @@ -212,10 +212,7 @@ Properties       string Address [readonly]
>>                       Service advertisement data. Keys are the UUIDs in
>>                       string format followed by its byte array value.
>>
>> -             array{object} GattServices [readonly, optional]
>> +             bool Discovering [readonly, optional, experimental]
>>
>> -                     List of GATT service object paths. Each referenced
>> -                     object exports the org.bluez.GattService1 interface and
>> -                     represents a remote GATT service. This property will be
>> -                     updated once all remote GATT services of this device
>> -                     have been discovered and exported over D-Bus.
>> +                     Indicate whether or not service discovery is in
>> +                     progress.
>
> I wonder how useful it's going to be.
>
> What I expect from a client is that it made a GetManagedObjects() call
> when it entered the bus and added listeners to the InterfacesAdded(),
> InterfacesRemoved() and PropertiesChanged() signals, I am not seeing
> what kind of value this property is adding to that kind of client.
>
> But yeah, it is better than GattServices.

I have fight this over in Web Bluetooth, but the design there is
different since it does not expect the stacks to build a cache of the
attributes so they end up with something like a GetPrimaryServices
method which would return the current list of services, but it turn
out they could be outdated if the device database has changed:

https://github.com/thegecko/web-bluetooth-dfu/issues/12#issuecomment-194973956

That said there is this problem that if an application start using the
current objects as soon as it reconnects it may get errors if they are
outdated, I still thing this is better than having to wait the
discovery to complete and not being able to do any access to
attributes before that happens, also which objects not being removed
on disconnect we can persist notifications which is something I
believe is not possible with other stacks, it is probably racy to do
in the applications because by the time you have a new connection
comes in it has to react to that to subscribe again but in the
meantime it can loose notification that had happened quickly enough.

-- 
Luiz Augusto von Dentz

Re: [RFC BlueZ] doc/device-api: Replace GattServices with Discovering property

[Andrzej Kaczmarek]

Hi Luiz,

On Thu, Mar 10, 2016 at 5:16 PM, Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
> From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
>
> GattServices is not really doing was it was meant to do which was to
> track progress of service discovery since it only worked for the very
> first time a device is connected but since we no longer remove the
> attributes an application would have the false impression the service are
> all resolved by the time it reconnects when in fact the service may have
> changed.
>
> Futhermore object tracking like it is doing has been obsolete by
> ObjectManager so this propose to replace the service discovery tracking
> with a boolean property which works both with SDP as well as GATT
> discovery.
> ---
>  doc/device-api.txt |  9 +++------
>  src/device.c       | 58 +++++++++++-------------------------------------------
>  2 files changed, 14 insertions(+), 53 deletions(-)
>
> diff --git a/doc/device-api.txt b/doc/device-api.txt
> index a8076a2..9e58664 100644
> --- a/doc/device-api.txt
> +++ b/doc/device-api.txt
> @@ -212,10 +212,7 @@ Properties string Address [readonly]
>                         Service advertisement data. Keys are the UUIDs in
>                         string format followed by its byte array value.
>
> -               array{object} GattServices [readonly, optional]
> +               bool Discovering [readonly, optional, experimental]

It's not optional - there's no exists method (which is fine).

I think a better name for this would be "Discovered" which has initial
value of false (once device is connected) and then changes to true
(once services are ready, either from cache or an actual discovery).
It could also change back to false once re-discovery due to Service
Changed is in progress which I think is also fine.

With "Discovering" the problem is still that once "Connected" changes
to true, reading "Discovering" will return false so you don't quite
know whether discovery already finished or has not yet started.
Application would need to wait for complete sequence to be sure:
Connected=false, Discovering=false
Connected=true, Discovering=false
Connected=false, Discovering=true
Connected=false, Discovering=false

With "Discovered" this is not a problem since sequence is as follows:
Connected=false, Discovered=false
Connected=true, Discovered=false
Connected=true, Discovered=true

> -                       List of GATT service object paths. Each referenced
> -                       object exports the org.bluez.GattService1 interface and
> -                       represents a remote GATT service. This property will be
> -                       updated once all remote GATT services of this device
> -                       have been discovered and exported over D-Bus.
> +                       Indicate whether or not service discovery is in
> +                       progress.

<snip>

BR,
Andrzej

Re: [RFC BlueZ] doc/device-api: Replace GattServices with Discovering property

[Luiz Augusto von Dentz]

Hi Andrzej,

On Fri, Mar 11, 2016 at 1:05 PM, Andrzej Kaczmarek
<andrzej.kaczmarek@codecoup.pl> wrote:
> Hi Luiz,
>
> On Thu, Mar 10, 2016 at 5:16 PM, Luiz Augusto von Dentz
> <luiz.dentz@gmail.com> wrote:
>> From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
>>
>> GattServices is not really doing was it was meant to do which was to
>> track progress of service discovery since it only worked for the very
>> first time a device is connected but since we no longer remove the
>> attributes an application would have the false impression the service are
>> all resolved by the time it reconnects when in fact the service may have
>> changed.
>>
>> Futhermore object tracking like it is doing has been obsolete by
>> ObjectManager so this propose to replace the service discovery tracking
>> with a boolean property which works both with SDP as well as GATT
>> discovery.
>> ---
>>  doc/device-api.txt |  9 +++------
>>  src/device.c       | 58 +++++++++++-------------------------------------------
>>  2 files changed, 14 insertions(+), 53 deletions(-)
>>
>> diff --git a/doc/device-api.txt b/doc/device-api.txt
>> index a8076a2..9e58664 100644
>> --- a/doc/device-api.txt
>> +++ b/doc/device-api.txt
>> @@ -212,10 +212,7 @@ Properties string Address [readonly]
>>                         Service advertisement data. Keys are the UUIDs in
>>                         string format followed by its byte array value.
>>
>> -               array{object} GattServices [readonly, optional]
>> +               bool Discovering [readonly, optional, experimental]
>
> It's not optional - there's no exists method (which is fine).

Indeed, I will fix it.

> I think a better name for this would be "Discovered" which has initial
> value of false (once device is connected) and then changes to true
> (once services are ready, either from cache or an actual discovery).
> It could also change back to false once re-discovery due to Service
> Changed is in progress which I think is also fine.
>
> With "Discovering" the problem is still that once "Connected" changes
> to true, reading "Discovering" will return false so you don't quite
> know whether discovery already finished or has not yet started.
> Application would need to wait for complete sequence to be sure:
> Connected=false, Discovering=false
> Connected=true, Discovering=false
> Connected=false, Discovering=true
> Connected=false, Discovering=false
>
> With "Discovered" this is not a problem since sequence is as follows:
> Connected=false, Discovered=false
> Connected=true, Discovered=false
> Connected=true, Discovered=true

I was actually thinking in using the same terminology we used in the
source code such as ResolvingServices, but changing to Discovered
don't actually fix the problem since you may still end up with
Connected=true, Discovered=true but the flag can still change to
Discovered=false. Anyway Im currently discussing about this changes
makes sense in Web Bluetooth, because Service Changed may come in
later on and there is no API to push the services added/removed to the
application.

>> -                       List of GATT service object paths. Each referenced
>> -                       object exports the org.bluez.GattService1 interface and
>> -                       represents a remote GATT service. This property will be
>> -                       updated once all remote GATT services of this device
>> -                       have been discovered and exported over D-Bus.
>> +                       Indicate whether or not service discovery is in
>> +                       progress.
>
> <snip>
>
> BR,
> Andrzej



-- 
Luiz Augusto von Dentz

Re: [RFC BlueZ] doc/device-api: Replace GattServices with Discovering property

[Andrzej Kaczmarek]

Hi Luiz,


On Fri, Mar 11, 2016 at 12:23 PM, Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
> Hi Andrzej,
>
> On Fri, Mar 11, 2016 at 1:05 PM, Andrzej Kaczmarek
> <andrzej.kaczmarek@codecoup.pl> wrote:
>> Hi Luiz,
>>
>> On Thu, Mar 10, 2016 at 5:16 PM, Luiz Augusto von Dentz
>> <luiz.dentz@gmail.com> wrote:
>>> From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
>>>
>>> GattServices is not really doing was it was meant to do which was to
>>> track progress of service discovery since it only worked for the very
>>> first time a device is connected but since we no longer remove the
>>> attributes an application would have the false impression the service are
>>> all resolved by the time it reconnects when in fact the service may have
>>> changed.
>>>
>>> Futhermore object tracking like it is doing has been obsolete by
>>> ObjectManager so this propose to replace the service discovery tracking
>>> with a boolean property which works both with SDP as well as GATT
>>> discovery.
>>> ---
>>>  doc/device-api.txt |  9 +++------
>>>  src/device.c       | 58 +++++++++++-------------------------------------------
>>>  2 files changed, 14 insertions(+), 53 deletions(-)
>>>
>>> diff --git a/doc/device-api.txt b/doc/device-api.txt
>>> index a8076a2..9e58664 100644
>>> --- a/doc/device-api.txt
>>> +++ b/doc/device-api.txt
>>> @@ -212,10 +212,7 @@ Properties string Address [readonly]
>>>                         Service advertisement data. Keys are the UUIDs in
>>>                         string format followed by its byte array value.
>>>
>>> -               array{object} GattServices [readonly, optional]
>>> +               bool Discovering [readonly, optional, experimental]
>>
>> It's not optional - there's no exists method (which is fine).
>
> Indeed, I will fix it.
>
>> I think a better name for this would be "Discovered" which has initial
>> value of false (once device is connected) and then changes to true
>> (once services are ready, either from cache or an actual discovery).
>> It could also change back to false once re-discovery due to Service
>> Changed is in progress which I think is also fine.
>>
>> With "Discovering" the problem is still that once "Connected" changes
>> to true, reading "Discovering" will return false so you don't quite
>> know whether discovery already finished or has not yet started.
>> Application would need to wait for complete sequence to be sure:
>> Connected=false, Discovering=false
>> Connected=true, Discovering=false
>> Connected=false, Discovering=true
>> Connected=false, Discovering=false
>>
>> With "Discovered" this is not a problem since sequence is as follows:
>> Connected=false, Discovered=false
>> Connected=true, Discovered=false
>> Connected=true, Discovered=true
>
> I was actually thinking in using the same terminology we used in the
> source code such as ResolvingServices, but changing to Discovered
> don't actually fix the problem since you may still end up with
> Connected=true, Discovered=true but the flag can still change to
> Discovered=false. Anyway Im currently discussing about this changes
> makes sense in Web Bluetooth, because Service Changed may come in
> later on and there is no API to push the services added/removed to the
> application.

When Service Changed is received, BlueZ removes old objects and then
adds new ones. And once there's change to Discovered=false it means
device in its current state is not fully discovered, e.g. there's
re-discovery due to Service Changed in this case which makes sense to
me. So what's the problem here?

BR,
Andrzej

Re: [RFC BlueZ] doc/device-api: Replace GattServices with Discovering property

[Luiz Augusto von Dentz]

Hi Andrzej,

On Fri, Mar 11, 2016 at 2:24 PM, Andrzej Kaczmarek
<andrzej.kaczmarek@codecoup.pl> wrote:
> Hi Luiz,
>
>
> On Fri, Mar 11, 2016 at 12:23 PM, Luiz Augusto von Dentz
> <luiz.dentz@gmail.com> wrote:
>> Hi Andrzej,
>>
>> On Fri, Mar 11, 2016 at 1:05 PM, Andrzej Kaczmarek
>> <andrzej.kaczmarek@codecoup.pl> wrote:
>>> Hi Luiz,
>>>
>>> On Thu, Mar 10, 2016 at 5:16 PM, Luiz Augusto von Dentz
>>> <luiz.dentz@gmail.com> wrote:
>>>> From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
>>>>
>>>> GattServices is not really doing was it was meant to do which was to
>>>> track progress of service discovery since it only worked for the very
>>>> first time a device is connected but since we no longer remove the
>>>> attributes an application would have the false impression the service are
>>>> all resolved by the time it reconnects when in fact the service may have
>>>> changed.
>>>>
>>>> Futhermore object tracking like it is doing has been obsolete by
>>>> ObjectManager so this propose to replace the service discovery tracking
>>>> with a boolean property which works both with SDP as well as GATT
>>>> discovery.
>>>> ---
>>>>  doc/device-api.txt |  9 +++------
>>>>  src/device.c       | 58 +++++++++++-------------------------------------------
>>>>  2 files changed, 14 insertions(+), 53 deletions(-)
>>>>
>>>> diff --git a/doc/device-api.txt b/doc/device-api.txt
>>>> index a8076a2..9e58664 100644
>>>> --- a/doc/device-api.txt
>>>> +++ b/doc/device-api.txt
>>>> @@ -212,10 +212,7 @@ Properties string Address [readonly]
>>>>                         Service advertisement data. Keys are the UUIDs in
>>>>                         string format followed by its byte array value.
>>>>
>>>> -               array{object} GattServices [readonly, optional]
>>>> +               bool Discovering [readonly, optional, experimental]
>>>
>>> It's not optional - there's no exists method (which is fine).
>>
>> Indeed, I will fix it.
>>
>>> I think a better name for this would be "Discovered" which has initial
>>> value of false (once device is connected) and then changes to true
>>> (once services are ready, either from cache or an actual discovery).
>>> It could also change back to false once re-discovery due to Service
>>> Changed is in progress which I think is also fine.
>>>
>>> With "Discovering" the problem is still that once "Connected" changes
>>> to true, reading "Discovering" will return false so you don't quite
>>> know whether discovery already finished or has not yet started.
>>> Application would need to wait for complete sequence to be sure:
>>> Connected=false, Discovering=false
>>> Connected=true, Discovering=false
>>> Connected=false, Discovering=true
>>> Connected=false, Discovering=false
>>>
>>> With "Discovered" this is not a problem since sequence is as follows:
>>> Connected=false, Discovered=false
>>> Connected=true, Discovered=false
>>> Connected=true, Discovered=true
>>
>> I was actually thinking in using the same terminology we used in the
>> source code such as ResolvingServices, but changing to Discovered
>> don't actually fix the problem since you may still end up with
>> Connected=true, Discovered=true but the flag can still change to
>> Discovered=false. Anyway Im currently discussing about this changes
>> makes sense in Web Bluetooth, because Service Changed may come in
>> later on and there is no API to push the services added/removed to the
>> application.
>
> When Service Changed is received, BlueZ removes old objects and then
> adds new ones. And once there's change to Discovered=false it means
> device in its current state is not fully discovered, e.g. there's
> re-discovery due to Service Changed in this case which makes sense to
> me. So what's the problem here?

If that was the case, again this is for Web Bluetooth, we wouldn't
need to notify this at all because the application would receive the
updates directly, but since the Web Bluetooth is not designed in this
way you just get the current services if anything is updated later it
causes this problem where the services get updated but the application
don't realize it has happened. So it is all about when the application
actually list the objects, again Im talking about Web Bluetooth
applications, if by the time it calls getPrimaryServices the discovery
is active there is a chance it will return outdated services.
Essentially it is a polling API.

You can read more about this issue here:

https://github.com/thegecko/web-bluetooth-dfu/issues/12#issuecomment-194973956

-- 
Luiz Augusto von Dentz

Re: [RFC BlueZ] doc/device-api: Replace GattServices with Discovering property

[Andrzej Kaczmarek]

Hi Luiz,


On Fri, Mar 11, 2016 at 2:08 PM, Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
> Hi Andrzej,
>
> On Fri, Mar 11, 2016 at 2:24 PM, Andrzej Kaczmarek
> <andrzej.kaczmarek@codecoup.pl> wrote:
>> Hi Luiz,
>>
>>
>> On Fri, Mar 11, 2016 at 12:23 PM, Luiz Augusto von Dentz
>> <luiz.dentz@gmail.com> wrote:
>>> Hi Andrzej,
>>>
>>> On Fri, Mar 11, 2016 at 1:05 PM, Andrzej Kaczmarek
>>> <andrzej.kaczmarek@codecoup.pl> wrote:
>>>> Hi Luiz,
>>>>
>>>> On Thu, Mar 10, 2016 at 5:16 PM, Luiz Augusto von Dentz
>>>> <luiz.dentz@gmail.com> wrote:
>>>>> From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
>>>>>
>>>>> GattServices is not really doing was it was meant to do which was to
>>>>> track progress of service discovery since it only worked for the very
>>>>> first time a device is connected but since we no longer remove the
>>>>> attributes an application would have the false impression the service are
>>>>> all resolved by the time it reconnects when in fact the service may have
>>>>> changed.
>>>>>
>>>>> Futhermore object tracking like it is doing has been obsolete by
>>>>> ObjectManager so this propose to replace the service discovery tracking
>>>>> with a boolean property which works both with SDP as well as GATT
>>>>> discovery.
>>>>> ---
>>>>>  doc/device-api.txt |  9 +++------
>>>>>  src/device.c       | 58 +++++++++++-------------------------------------------
>>>>>  2 files changed, 14 insertions(+), 53 deletions(-)
>>>>>
>>>>> diff --git a/doc/device-api.txt b/doc/device-api.txt
>>>>> index a8076a2..9e58664 100644
>>>>> --- a/doc/device-api.txt
>>>>> +++ b/doc/device-api.txt
>>>>> @@ -212,10 +212,7 @@ Properties string Address [readonly]
>>>>>                         Service advertisement data. Keys are the UUIDs in
>>>>>                         string format followed by its byte array value.
>>>>>
>>>>> -               array{object} GattServices [readonly, optional]
>>>>> +               bool Discovering [readonly, optional, experimental]
>>>>
>>>> It's not optional - there's no exists method (which is fine).
>>>
>>> Indeed, I will fix it.
>>>
>>>> I think a better name for this would be "Discovered" which has initial
>>>> value of false (once device is connected) and then changes to true
>>>> (once services are ready, either from cache or an actual discovery).
>>>> It could also change back to false once re-discovery due to Service
>>>> Changed is in progress which I think is also fine.
>>>>
>>>> With "Discovering" the problem is still that once "Connected" changes
>>>> to true, reading "Discovering" will return false so you don't quite
>>>> know whether discovery already finished or has not yet started.
>>>> Application would need to wait for complete sequence to be sure:
>>>> Connected=false, Discovering=false
>>>> Connected=true, Discovering=false
>>>> Connected=false, Discovering=true
>>>> Connected=false, Discovering=false
>>>>
>>>> With "Discovered" this is not a problem since sequence is as follows:
>>>> Connected=false, Discovered=false
>>>> Connected=true, Discovered=false
>>>> Connected=true, Discovered=true
>>>
>>> I was actually thinking in using the same terminology we used in the
>>> source code such as ResolvingServices, but changing to Discovered
>>> don't actually fix the problem since you may still end up with
>>> Connected=true, Discovered=true but the flag can still change to
>>> Discovered=false. Anyway Im currently discussing about this changes
>>> makes sense in Web Bluetooth, because Service Changed may come in
>>> later on and there is no API to push the services added/removed to the
>>> application.
>>
>> When Service Changed is received, BlueZ removes old objects and then
>> adds new ones. And once there's change to Discovered=false it means
>> device in its current state is not fully discovered, e.g. there's
>> re-discovery due to Service Changed in this case which makes sense to
>> me. So what's the problem here?
>
> If that was the case, again this is for Web Bluetooth, we wouldn't
> need to notify this at all because the application would receive the
> updates directly, but since the Web Bluetooth is not designed in this
> way you just get the current services if anything is updated later it
> causes this problem where the services get updated but the application
> don't realize it has happened. So it is all about when the application
> actually list the objects, again Im talking about Web Bluetooth
> applications, if by the time it calls getPrimaryServices the discovery
> is active there is a chance it will return outdated services.
> Essentially it is a polling API.
>
> You can read more about this issue here:
>
> https://github.com/thegecko/web-bluetooth-dfu/issues/12#issuecomment-194973956

In case discovery is in progress, API such as getPrimaryServices()
could queue response until discovery is done. This is possible with
both "Discovering" and "Discovered", but the latter has significant
advantage here: just after connected, you know exactly when initial
discovery finished, because there will be single transition
Discovered=false->true. In case of "Discovering" there's problem I
mentioned before when you start with Discovering=false so there's
chance application gets services even before initial discovery - we've
seen such races when application is trying to read/write
characteristic using existing D-Bus object just after connection, but
before fixed ATT channel was established in BlueZ.

As for Service Changed, as you mentioned in comment linked above, this
cannot be really solved without some kind of notification to
application. Other possibility (without notifications) is that object
created by "outdated" getPrimaryServices() call with simply return an
error forcing application to call getPrimaryServices() again. But this
has to be solved in Web Bluetooth somehow anyway...

BR,
Andrzej

Re: [RFC BlueZ] doc/device-api: Replace GattServices with Discovering property

[Luiz Augusto von Dentz]

Hi Andrzej,

On Fri, Mar 11, 2016 at 3:41 PM, Andrzej Kaczmarek
<andrzej.kaczmarek@codecoup.pl> wrote:
> Hi Luiz,
>
>
> On Fri, Mar 11, 2016 at 2:08 PM, Luiz Augusto von Dentz
> <luiz.dentz@gmail.com> wrote:
>> Hi Andrzej,
>>
>> On Fri, Mar 11, 2016 at 2:24 PM, Andrzej Kaczmarek
>> <andrzej.kaczmarek@codecoup.pl> wrote:
>>> Hi Luiz,
>>>
>>>
>>> On Fri, Mar 11, 2016 at 12:23 PM, Luiz Augusto von Dentz
>>> <luiz.dentz@gmail.com> wrote:
>>>> Hi Andrzej,
>>>>
>>>> On Fri, Mar 11, 2016 at 1:05 PM, Andrzej Kaczmarek
>>>> <andrzej.kaczmarek@codecoup.pl> wrote:
>>>>> Hi Luiz,
>>>>>
>>>>> On Thu, Mar 10, 2016 at 5:16 PM, Luiz Augusto von Dentz
>>>>> <luiz.dentz@gmail.com> wrote:
>>>>>> From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
>>>>>>
>>>>>> GattServices is not really doing was it was meant to do which was to
>>>>>> track progress of service discovery since it only worked for the very
>>>>>> first time a device is connected but since we no longer remove the
>>>>>> attributes an application would have the false impression the service are
>>>>>> all resolved by the time it reconnects when in fact the service may have
>>>>>> changed.
>>>>>>
>>>>>> Futhermore object tracking like it is doing has been obsolete by
>>>>>> ObjectManager so this propose to replace the service discovery tracking
>>>>>> with a boolean property which works both with SDP as well as GATT
>>>>>> discovery.
>>>>>> ---
>>>>>>  doc/device-api.txt |  9 +++------
>>>>>>  src/device.c       | 58 +++++++++++-------------------------------------------
>>>>>>  2 files changed, 14 insertions(+), 53 deletions(-)
>>>>>>
>>>>>> diff --git a/doc/device-api.txt b/doc/device-api.txt
>>>>>> index a8076a2..9e58664 100644
>>>>>> --- a/doc/device-api.txt
>>>>>> +++ b/doc/device-api.txt
>>>>>> @@ -212,10 +212,7 @@ Properties string Address [readonly]
>>>>>>                         Service advertisement data. Keys are the UUIDs in
>>>>>>                         string format followed by its byte array value.
>>>>>>
>>>>>> -               array{object} GattServices [readonly, optional]
>>>>>> +               bool Discovering [readonly, optional, experimental]
>>>>>
>>>>> It's not optional - there's no exists method (which is fine).
>>>>
>>>> Indeed, I will fix it.
>>>>
>>>>> I think a better name for this would be "Discovered" which has initial
>>>>> value of false (once device is connected) and then changes to true
>>>>> (once services are ready, either from cache or an actual discovery).
>>>>> It could also change back to false once re-discovery due to Service
>>>>> Changed is in progress which I think is also fine.
>>>>>
>>>>> With "Discovering" the problem is still that once "Connected" changes
>>>>> to true, reading "Discovering" will return false so you don't quite
>>>>> know whether discovery already finished or has not yet started.
>>>>> Application would need to wait for complete sequence to be sure:
>>>>> Connected=false, Discovering=false
>>>>> Connected=true, Discovering=false
>>>>> Connected=false, Discovering=true
>>>>> Connected=false, Discovering=false
>>>>>
>>>>> With "Discovered" this is not a problem since sequence is as follows:
>>>>> Connected=false, Discovered=false
>>>>> Connected=true, Discovered=false
>>>>> Connected=true, Discovered=true
>>>>
>>>> I was actually thinking in using the same terminology we used in the
>>>> source code such as ResolvingServices, but changing to Discovered
>>>> don't actually fix the problem since you may still end up with
>>>> Connected=true, Discovered=true but the flag can still change to
>>>> Discovered=false. Anyway Im currently discussing about this changes
>>>> makes sense in Web Bluetooth, because Service Changed may come in
>>>> later on and there is no API to push the services added/removed to the
>>>> application.
>>>
>>> When Service Changed is received, BlueZ removes old objects and then
>>> adds new ones. And once there's change to Discovered=false it means
>>> device in its current state is not fully discovered, e.g. there's
>>> re-discovery due to Service Changed in this case which makes sense to
>>> me. So what's the problem here?
>>
>> If that was the case, again this is for Web Bluetooth, we wouldn't
>> need to notify this at all because the application would receive the
>> updates directly, but since the Web Bluetooth is not designed in this
>> way you just get the current services if anything is updated later it
>> causes this problem where the services get updated but the application
>> don't realize it has happened. So it is all about when the application
>> actually list the objects, again Im talking about Web Bluetooth
>> applications, if by the time it calls getPrimaryServices the discovery
>> is active there is a chance it will return outdated services.
>> Essentially it is a polling API.
>>
>> You can read more about this issue here:
>>
>> https://github.com/thegecko/web-bluetooth-dfu/issues/12#issuecomment-194973956
>
> In case discovery is in progress, API such as getPrimaryServices()
> could queue response until discovery is done. This is possible with
> both "Discovering" and "Discovered", but the latter has significant
> advantage here: just after connected, you know exactly when initial
> discovery finished, because there will be single transition
> Discovered=false->true. In case of "Discovering" there's problem I
> mentioned before when you start with Discovering=false so there's
> chance application gets services even before initial discovery - we've
> seen such races when application is trying to read/write
> characteristic using existing D-Bus object just after connection, but
> before fixed ATT channel was established in BlueZ.

I think this problem with ATT has been solved actually, so the object
have access to bt_gatt_client immediately after connection so
read/write can be processed. Btw, may latest patch actually contain
some changes so the signal order looks like this:

[CHG] Device ... Connected: yes
[CHG] Device ..  ServicesResolved: yes
[CHG] Device ... ServicesResolved: no
[CHG] Device ... Connected: no

It basically follows svc_refreshed flag.

> As for Service Changed, as you mentioned in comment linked above, this
> cannot be really solved without some kind of notification to
> application. Other possibility (without notifications) is that object
> created by "outdated" getPrimaryServices() call with simply return an
> error forcing application to call getPrimaryServices() again. But this
> has to be solved in Web Bluetooth somehow anyway...

Yep, but I guess this is some unlikely to happen during a connection
that nobody care to add an API for it.

-- 
Luiz Augusto von Dentz

Re: [RFC BlueZ] doc/device-api: Replace GattServices with Discovering property

[Andrzej Kaczmarek]

Hi Luiz,


On Fri, Mar 11, 2016 at 3:39 PM, Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
> Hi Andrzej,
>
> On Fri, Mar 11, 2016 at 3:41 PM, Andrzej Kaczmarek
> <andrzej.kaczmarek@codecoup.pl> wrote:
>> Hi Luiz,
>>
>>
>> On Fri, Mar 11, 2016 at 2:08 PM, Luiz Augusto von Dentz
>> <luiz.dentz@gmail.com> wrote:
>>> Hi Andrzej,
>>>
>>> On Fri, Mar 11, 2016 at 2:24 PM, Andrzej Kaczmarek
>>> <andrzej.kaczmarek@codecoup.pl> wrote:
>>>> Hi Luiz,
>>>>
>>>>
>>>> On Fri, Mar 11, 2016 at 12:23 PM, Luiz Augusto von Dentz
>>>> <luiz.dentz@gmail.com> wrote:
>>>>> Hi Andrzej,
>>>>>
>>>>> On Fri, Mar 11, 2016 at 1:05 PM, Andrzej Kaczmarek
>>>>> <andrzej.kaczmarek@codecoup.pl> wrote:
>>>>>> Hi Luiz,
>>>>>>
>>>>>> On Thu, Mar 10, 2016 at 5:16 PM, Luiz Augusto von Dentz
>>>>>> <luiz.dentz@gmail.com> wrote:
>>>>>>> From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
>>>>>>>
>>>>>>> GattServices is not really doing was it was meant to do which was to
>>>>>>> track progress of service discovery since it only worked for the very
>>>>>>> first time a device is connected but since we no longer remove the
>>>>>>> attributes an application would have the false impression the service are
>>>>>>> all resolved by the time it reconnects when in fact the service may have
>>>>>>> changed.
>>>>>>>
>>>>>>> Futhermore object tracking like it is doing has been obsolete by
>>>>>>> ObjectManager so this propose to replace the service discovery tracking
>>>>>>> with a boolean property which works both with SDP as well as GATT
>>>>>>> discovery.
>>>>>>> ---
>>>>>>>  doc/device-api.txt |  9 +++------
>>>>>>>  src/device.c       | 58 +++++++++++-------------------------------------------
>>>>>>>  2 files changed, 14 insertions(+), 53 deletions(-)
>>>>>>>
>>>>>>> diff --git a/doc/device-api.txt b/doc/device-api.txt
>>>>>>> index a8076a2..9e58664 100644
>>>>>>> --- a/doc/device-api.txt
>>>>>>> +++ b/doc/device-api.txt
>>>>>>> @@ -212,10 +212,7 @@ Properties string Address [readonly]
>>>>>>>                         Service advertisement data. Keys are the UUIDs in
>>>>>>>                         string format followed by its byte array value.
>>>>>>>
>>>>>>> -               array{object} GattServices [readonly, optional]
>>>>>>> +               bool Discovering [readonly, optional, experimental]
>>>>>>
>>>>>> It's not optional - there's no exists method (which is fine).
>>>>>
>>>>> Indeed, I will fix it.
>>>>>
>>>>>> I think a better name for this would be "Discovered" which has initial
>>>>>> value of false (once device is connected) and then changes to true
>>>>>> (once services are ready, either from cache or an actual discovery).
>>>>>> It could also change back to false once re-discovery due to Service
>>>>>> Changed is in progress which I think is also fine.
>>>>>>
>>>>>> With "Discovering" the problem is still that once "Connected" changes
>>>>>> to true, reading "Discovering" will return false so you don't quite
>>>>>> know whether discovery already finished or has not yet started.
>>>>>> Application would need to wait for complete sequence to be sure:
>>>>>> Connected=false, Discovering=false
>>>>>> Connected=true, Discovering=false
>>>>>> Connected=false, Discovering=true
>>>>>> Connected=false, Discovering=false
>>>>>>
>>>>>> With "Discovered" this is not a problem since sequence is as follows:
>>>>>> Connected=false, Discovered=false
>>>>>> Connected=true, Discovered=false
>>>>>> Connected=true, Discovered=true
>>>>>
>>>>> I was actually thinking in using the same terminology we used in the
>>>>> source code such as ResolvingServices, but changing to Discovered
>>>>> don't actually fix the problem since you may still end up with
>>>>> Connected=true, Discovered=true but the flag can still change to
>>>>> Discovered=false. Anyway Im currently discussing about this changes
>>>>> makes sense in Web Bluetooth, because Service Changed may come in
>>>>> later on and there is no API to push the services added/removed to the
>>>>> application.
>>>>
>>>> When Service Changed is received, BlueZ removes old objects and then
>>>> adds new ones. And once there's change to Discovered=false it means
>>>> device in its current state is not fully discovered, e.g. there's
>>>> re-discovery due to Service Changed in this case which makes sense to
>>>> me. So what's the problem here?
>>>
>>> If that was the case, again this is for Web Bluetooth, we wouldn't
>>> need to notify this at all because the application would receive the
>>> updates directly, but since the Web Bluetooth is not designed in this
>>> way you just get the current services if anything is updated later it
>>> causes this problem where the services get updated but the application
>>> don't realize it has happened. So it is all about when the application
>>> actually list the objects, again Im talking about Web Bluetooth
>>> applications, if by the time it calls getPrimaryServices the discovery
>>> is active there is a chance it will return outdated services.
>>> Essentially it is a polling API.
>>>
>>> You can read more about this issue here:
>>>
>>> https://github.com/thegecko/web-bluetooth-dfu/issues/12#issuecomment-194973956
>>
>> In case discovery is in progress, API such as getPrimaryServices()
>> could queue response until discovery is done. This is possible with
>> both "Discovering" and "Discovered", but the latter has significant
>> advantage here: just after connected, you know exactly when initial
>> discovery finished, because there will be single transition
>> Discovered=false->true. In case of "Discovering" there's problem I
>> mentioned before when you start with Discovering=false so there's
>> chance application gets services even before initial discovery - we've
>> seen such races when application is trying to read/write
>> characteristic using existing D-Bus object just after connection, but
>> before fixed ATT channel was established in BlueZ.
>
> I think this problem with ATT has been solved actually, so the object
> have access to bt_gatt_client immediately after connection so
> read/write can be processed.

I've just checked on latest master, e.g. try to call
org.bluez.GattCharacteristic1.ReadValue on known object path when
received Connected=true and it fails.

Connected: yes
ERROR:dbus.connection:Exception in handler for D-Bus signal:
Traceback (most recent call last):
  File "/usr/lib/python3/dist-packages/dbus/connection.py", line 230,
in maybe_handle_message
    self._handler(*args, **kwargs)
  File "./andk-test", line 34, in idev_changed_cb
    ichar.ReadValue()
  File "/usr/lib/python3/dist-packages/dbus/proxies.py", line 70, in __call__
    return self._proxy_method(*args, **keywords)
  File "/usr/lib/python3/dist-packages/dbus/proxies.py", line 145, in __call__
    **keywords)
  File "/usr/lib/python3/dist-packages/dbus/connection.py", line 651,
in call_blocking
    message, timeout)
dbus.exceptions.DBusException: org.bluez.Error.Failed: Not connected

> Btw, may latest patch actually contain
> some changes so the signal order looks like this:
>
> [CHG] Device ... Connected: yes
> [CHG] Device ..  ServicesResolved: yes
> [CHG] Device ... ServicesResolved: no
> [CHG] Device ... Connected: no
>
> It basically follows svc_refreshed flag.

Ok, I'll check new patch and comment there.

>> As for Service Changed, as you mentioned in comment linked above, this
>> cannot be really solved without some kind of notification to
>> application. Other possibility (without notifications) is that object
>> created by "outdated" getPrimaryServices() call with simply return an
>> error forcing application to call getPrimaryServices() again. But this
>> has to be solved in Web Bluetooth somehow anyway...
>
> Yep, but I guess this is some unlikely to happen during a connection
> that nobody care to add an API for it.

Android does it when application (un)registers service, iOS does the
same... And for example, Apple's notification service (ANCS)
specification states explicitly that this service may appear and
disappear during connection so implementations are required to use
Service Changed. I guess it's something to be fixed in Web Bluetooth
for sure.

BR,
Andrzej