Hi Arman, On Tue, Dec 2, 2014 at 12:46 PM, Luiz Augusto von Dentz <luiz.dentz@xxxxxxxxx> wrote: > Hi Arman, > > On Mon, Dec 1, 2014 at 7:57 PM, Arman Uguray <armansito@xxxxxxxxxxxx> wrote: >> Hi Marcel & Luiz, >> >>> On Mon, Dec 1, 2014 at 9:19 AM, Marcel Holtmann <marcel@xxxxxxxxxxxx> wrote: >>> 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. >>> >> >> After our brief chat on IRC, please confirm that this is more or less >> what we want: >> >> bt_gatt_client_new(att, mtu, db) -> adds ref to db. >> bt_gatt_client_new(att, mtu, NULL) -> creates its own db >> >> Either function might cause discovery based on whether or not the db >> is empty. If it's empty, client always starts discovery, otherwise it >> skips it. > > Sounds good. Sorry for not responding on IRC yesterday, I was away and > only noticed today that you have been there. I went ahead and implemented the reference counting for gatt_db, I will try to be online later today in case you want some to discuss more about this. >> bt_gatt_client_get_db -> returns db. This would be necessary if we >> want to allow passing NULL to bt_gatt_client_new. > > Hmm, but if we pass NULL, which I suppose is to enable things like > unit tests, why would we want to pull the db out of it? As I mention > from the core we would probably create the db when pairing so we get > notification regarding services changes from discovery, with this mind > I guess if the client pass NULL this means the db would be internal. > > Btw, I guess we should associate a valid db if service changed CCC has > been written or perhaps we clear the db if discovery is interrupted > before that happens but I guess in some cases we may have a db without > a service changed, in case we don't pair for example, in that case we > need to rediscover the handles before letting any profile connect > otherwise they may end up using invalid handles. > >> I think all of these are minor details that can be revised later, but >> I still need something as a starting point. >> >> In addition, I'm thinking that gatt-db should have service added and >> removed events that can be triggered by set_active and the clear >> functions. Something like >> >> gatt_db_register_service_callbacks(db, added_cb, removed_cb); >> >> In the client case, added_cb would only be called once all attributes >> belonging to the service have been discovered and inserted into the >> db. Client would also communicate services that are affected by a >> "Service Changed" indication via these callbacks. Either way, I >> suggest adding the service callback mechanism in a later patch set. > > Sounds good, but we might have to introduce this before we start doing > changing the core since this will probably be the preferable way the > discovery interact with core. > >> Let me know of this makes sense to everyone. >> >>> Regards >>> >>> Marcel >>> >> >> Arman > > > > -- > Luiz Augusto von Dentz -- Luiz Augusto von Dentz -- To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
