Re: [PATCH BlueZ 3/8] shared/gatt-client: Store services in gatt_db.

[Marcel Holtmann <marcel@holtmann.org>]

Hi Arman,

>>>>> This patch rewrites the service discovery logic inside
>>>>> shared/gatt-client. The internal service_list structure has been
>>>>> entirely removed and services are stored in a gatt_db instance.
>>>>> Initially, gatt-client creates and owns the life-time of the gatt_db.
>>>> 
>>>> Im trying to figure out the reason why you want to start with your own
>>>> gatt_db, is it because it lacks reference counting, if that is the
>>>> case it should be trivial to add it.
>>>> 
>>> 
>>> Initially, yes, the lack of reference counting is one reason, which I
>>> was thinking of adding to gatt-db eventually. Though, what I had in
>>> mind was that, the gatt-db would be created by gatt-client if you want
>>> it to perform discovery, otherwise if you construct it with gatt-db
>>> then it wouldn't do discovery, which would address the permanent cache
>>> case. So, we would have two "new" functions:
>>> 
>>>   bt_gatt_client_new
>>>   bt_gatt_client_new_from_db
>>> 
>>> In the first case, if the upper layer wants to make the gatt-db
>>> outlive the gatt-client, in the future they can just add a reference
>>> to it and own it and in the next connection they can create the client
>>> using that same db instance. This is kind of a rough idea right now
>>> but I think it makes sense.
>>> 
>>> We can also keep both functions but have both accept a gatt-db as a
>>> parameter. Not sure what's best here really.
>> 
>> For unit test maybe we can have bt_gatt_client_new_default, iirc
>> Marcel suggested something like this for naming, but for the core
>> daemon it can create a empty db when pairing so even the initial
>> discovery would use it. This comes back to the idea of having the
>> callbacks in the db to attribute added/removed, with this logic we
>> could start creating D-Bus objects by the time a services is
>> discovered (provided we discover everything necessary) and not only
>> when bt_gatt_client signals it is done discovering, actually with the
>> current concept we could be spamming the bus a little too much since
>> the objects would be created all at same time this would cause several
>> InterfacesAdded signals in a row at the end of pairing.
>> 
> 
> I don't know about "_default" naming but are we talking about something like:
> 
>  bt_gatt_client_new(att, mtu, db)  /* Does discovery */
>  bt_gatt_client_new_from_cache(att, mtu, db)  /* No discovery */
> 
> So both functions get passed a db either way?

actually we might just better only use bt_gatt_client_new() and provide functionality to flush certain devices from the cache. In case we do want to trigger a discovery or re-discovery of devices.

The cache should just work and do the right thing. We should be able set max cacheable service, devices etc. So lets just give the GATT client a cache to work with and that is it. Everything else is an implementation detail between the client and the cache.

I prefer that every client has a cache attached to it (or allow NULL in case we really want to run without a cache). If the cache is independent, it can easily do the right thing for us.

Regards

Marcel