Return-Path: MIME-Version: 1.0 In-Reply-To: References: <1457626604-5656-1-git-send-email-luiz.dentz@gmail.com> Date: Fri, 11 Mar 2016 16:39:32 +0200 Message-ID: Subject: Re: [RFC BlueZ] doc/device-api: Replace GattServices with Discovering property From: Luiz Augusto von Dentz To: Andrzej Kaczmarek Cc: "linux-bluetooth@vger.kernel.org" Content-Type: text/plain; charset=UTF-8 Sender: linux-bluetooth-owner@vger.kernel.org List-ID: Hi Andrzej, On Fri, Mar 11, 2016 at 3:41 PM, Andrzej Kaczmarek wrote: > Hi Luiz, > > > On Fri, Mar 11, 2016 at 2:08 PM, Luiz Augusto von Dentz > wrote: >> Hi Andrzej, >> >> On Fri, Mar 11, 2016 at 2:24 PM, Andrzej Kaczmarek >> wrote: >>> Hi Luiz, >>> >>> >>> On Fri, Mar 11, 2016 at 12:23 PM, Luiz Augusto von Dentz >>> wrote: >>>> Hi Andrzej, >>>> >>>> On Fri, Mar 11, 2016 at 1:05 PM, Andrzej Kaczmarek >>>> wrote: >>>>> Hi Luiz, >>>>> >>>>> On Thu, Mar 10, 2016 at 5:16 PM, Luiz Augusto von Dentz >>>>> wrote: >>>>>> From: Luiz Augusto von Dentz >>>>>> >>>>>> 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