Hi all,
In the case where bluez is bonded with a remote device we don't want
to always perform service discovery, but instead to cache the
attribute data permanently and just use that on reconnections.
Currently shared/gatt-client automatically discovers all services when
created (via bt_gatt_client_new) so we need to add support to populate
a shared/gatt-client based on cached data in storage, since a
bt_gatt_client instance acts as our in-memory attribute cache for the
client role.
I initially thought about storing everything in INI format and loading
things directly from a file, via some function like
bt_gatt_client_new_from_storage. The biggest problem with this
approach is parsing the INI format; since we don't want to pull in any
external dependencies into shared/gatt (e.g. glib) we would possibly
have to write our own key-value file parser for shared/ which seems
like overkill. We could possibly invent some simpler blob format for
shared/gatt-client but people brought to my attention on IRC that we
may want to leave the exact storage format to the upper-layer, e.g.
desktop and android versions of bluetoothd may want to use different
formats for their attribute cache that's consistent with the platform.
So I decided to go with something generic instead. We would basically
add functions to manually populate a gatt-client and mark the client
as ready only after it's services have been populated. So we would
have these API changes:
1. A new argument to bt_gatt_client_new, to tell it not to perform
discovery. Something like "bool discover_services".
/* Don't perform discovery */
bt_gatt_client_new(att, mtu, false);
This would basically just leave the structure in "init" state and not
become ready.
2. We would then add a new structure to populate a gatt-client.
struct bt_gatt_populate { .. }; /* Probably something with a better name */
bool bt_gatt_populate_start(struct bt_gatt_populate *, struct
bt_gatt_client *);
bool bt_gatt_populate_add_service(....)
bool bt_gatt_populate_add_characteristic(...)
bool bt_gatt_populate_add_descriptor(....)
bool bt_gatt_populate_add_include(....)
bool bt_gatt_populate_cancel(...)
bool bt_gatt_populate_commit(struct bt_gatt_populate *);
The start function would tell gatt-client that it's being populated so
it would prevent this from being called twice. All of the _add_*
functions would create a temporary tree owned by the populate
structure, using the same internal structures defined in
shared/gatt-client.c. Calling bt_gatt_populate_commit would then pass
the ownership of the services to the bt_gatt_client, and
bt_gatt_client would perform the MTU exchange, subscribe to
notifications from the "Service Changed" characteristic if one exists,
mark itself as ready and invoke the ready handler.
If the upper-layer wants to store the current contents of gatt-client,
they can do so by iterating through its services and caching them to a
file directly. This way, shared/gatt-client would remain agnostic to
any storage format.
Comments requested.
Arman
Hi Marcel,
On Sat, Nov 22, 2014 at 5:17 AM, Marcel Holtmann <[email protected]> wrot=
e:
> Hi Arman,
>
>>>> In the case where bluez is bonded with a remote device we don't want
>>>> to always perform service discovery, but instead to cache the
>>>> attribute data permanently and just use that on reconnections.
>>>> Currently shared/gatt-client automatically discovers all services when
>>>> created (via bt_gatt_client_new) so we need to add support to populate
>>>> a shared/gatt-client based on cached data in storage, since a
>>>> bt_gatt_client instance acts as our in-memory attribute cache for the
>>>> client role.
>>>>
>>>> I initially thought about storing everything in INI format and loading
>>>> things directly from a file, via some function like
>>>> bt_gatt_client_new_from_storage. The biggest problem with this
>>>> approach is parsing the INI format; since we don't want to pull in any
>>>> external dependencies into shared/gatt (e.g. glib) we would possibly
>>>> have to write our own key-value file parser for shared/ which seems
>>>> like overkill. We could possibly invent some simpler blob format for
>>>> shared/gatt-client but people brought to my attention on IRC that we
>>>> may want to leave the exact storage format to the upper-layer, e.g.
>>>> desktop and android versions of bluetoothd may want to use different
>>>> formats for their attribute cache that's consistent with the platform.
>>>
>>> what we could do is just store the plain binary data. These are cache f=
iles and if you remove them, that just means next time we have to do full s=
ervice discovery again.
>>>
>>>> So I decided to go with something generic instead. We would basically
>>>> add functions to manually populate a gatt-client and mark the client
>>>> as ready only after it's services have been populated. So we would
>>>> have these API changes:
>>>>
>>>> 1. A new argument to bt_gatt_client_new, to tell it not to perform
>>>> discovery. Something like "bool discover_services".
>>>>
>>>> /* Don't perform discovery */
>>>> bt_gatt_client_new(att, mtu, false);
>>>>
>>>> This would basically just leave the structure in "init" state and not
>>>> become ready.
>>>
>>> Adding tons of parameters to a _new() is not really a good idea. I woul=
d always add a special _new_without_discovery() or something similar to our=
API. If we decide to go this route.
>>>
>>>>
>>>> 2. We would then add a new structure to populate a gatt-client.
>>>>
>>>> struct bt_gatt_populate { .. }; /* Probably something with a better=
name */
>>>>
>>>> bool bt_gatt_populate_start(struct bt_gatt_populate *, struct
>>>> bt_gatt_client *);
>>>> bool bt_gatt_populate_add_service(....)
>>>> bool bt_gatt_populate_add_characteristic(...)
>>>> bool bt_gatt_populate_add_descriptor(....)
>>>> bool bt_gatt_populate_add_include(....)
>>>> bool bt_gatt_populate_cancel(...)
>>>> bool bt_gatt_populate_commit(struct bt_gatt_populate *);
>>>>
>>>> The start function would tell gatt-client that it's being populated so
>>>> it would prevent this from being called twice. All of the _add_*
>>>> functions would create a temporary tree owned by the populate
>>>> structure, using the same internal structures defined in
>>>> shared/gatt-client.c. Calling bt_gatt_populate_commit would then pass
>>>> the ownership of the services to the bt_gatt_client, and
>>>> bt_gatt_client would perform the MTU exchange, subscribe to
>>>> notifications from the "Service Changed" characteristic if one exists,
>>>> mark itself as ready and invoke the ready handler.
>>>>
>>>> If the upper-layer wants to store the current contents of gatt-client,
>>>> they can do so by iterating through its services and caching them to a
>>>> file directly. This way, shared/gatt-client would remain agnostic to
>>>> any storage format.
>>>
>>> One other way of doing something like this is doing it this way:
>>>
>>> struct bt_gatt_storage;
>>>
>>> struct bt_gatt_storage *bt_gatt_storage_new()
>>> bt_gatt_storage_add_x
>>>
>>> struct bt_gatt_client *bt_gatt_client_from_storage(struct bt_gat=
t_storage);
>>>
>>> I am not sure this is actually any better. It is just an idea, but we m=
ight want to just store plain binary data of cached services in a binary ca=
ched file. I think that we can have a binary storage format for the cached =
data and then we just point the bt_gatt_client_new_with_cahce() to that fil=
e and let it load if present. So the upper layers would just decide where t=
hat file is located.
>>>
>>
>> I think in the end the exact format of the data should be up to the
>> upper layer, whether its binary data or INI format. After some
>> discussions on IRC we decided that this storage could be represented
>> by shared/gatt-db, which already has these functions to add services,
>> characteristics, etc. But we would need to make shared/gatt-db provide
>> a client role abstraction as well, since it's more meant for the
>> server role at the moment.
>>
>> This way bluetoothd could for example just read the INI file, create a
>> gatt_db and do something like bt_gatt_client_from_db(db, mtu);
>
> following up on my other email, then yes, we could have a bt_gatt_cache t=
hat is a collection of bt_gatt_db objects. That however is an implementatio=
n detail to me. I think it is important that we have a global concept of a =
cache. And as said, just having an in-memory cache will already solve 99% o=
f the use cases where we want to avoid service discovery for already known =
devices. Only cold-boot would be an issue and honestly that is a penalty th=
at I would happily accept for a first iteration. Which means we do not have=
to bother with file formats in the beginning.
I guess we are more or less in the same page then, I just did not
thought about the global cache but yes it could well be a collection
of bt_gatt_db per device. I also agree we could have a in-memory cache
to start with until we decide a good format to store persistently,
Szymon actually suggested doing the cache in binary format like we
have for SDP, perhaps one service per line, but that is something we
can give more thought. I guess we can experiment with HoG here, since
once you configure CCC we should not miss any event from the point we
connect.
--=20
Luiz Augusto von Dentz
Hi Arman,
>>> In the case where bluez is bonded with a remote device we don't want
>>> to always perform service discovery, but instead to cache the
>>> attribute data permanently and just use that on reconnections.
>>> Currently shared/gatt-client automatically discovers all services when
>>> created (via bt_gatt_client_new) so we need to add support to populate
>>> a shared/gatt-client based on cached data in storage, since a
>>> bt_gatt_client instance acts as our in-memory attribute cache for the
>>> client role.
>>>
>>> I initially thought about storing everything in INI format and loading
>>> things directly from a file, via some function like
>>> bt_gatt_client_new_from_storage. The biggest problem with this
>>> approach is parsing the INI format; since we don't want to pull in any
>>> external dependencies into shared/gatt (e.g. glib) we would possibly
>>> have to write our own key-value file parser for shared/ which seems
>>> like overkill. We could possibly invent some simpler blob format for
>>> shared/gatt-client but people brought to my attention on IRC that we
>>> may want to leave the exact storage format to the upper-layer, e.g.
>>> desktop and android versions of bluetoothd may want to use different
>>> formats for their attribute cache that's consistent with the platform.
>>
>> what we could do is just store the plain binary data. These are cache files and if you remove them, that just means next time we have to do full service discovery again.
>>
>>> So I decided to go with something generic instead. We would basically
>>> add functions to manually populate a gatt-client and mark the client
>>> as ready only after it's services have been populated. So we would
>>> have these API changes:
>>>
>>> 1. A new argument to bt_gatt_client_new, to tell it not to perform
>>> discovery. Something like "bool discover_services".
>>>
>>> /* Don't perform discovery */
>>> bt_gatt_client_new(att, mtu, false);
>>>
>>> This would basically just leave the structure in "init" state and not
>>> become ready.
>>
>> Adding tons of parameters to a _new() is not really a good idea. I would always add a special _new_without_discovery() or something similar to our API. If we decide to go this route.
>>
>>>
>>> 2. We would then add a new structure to populate a gatt-client.
>>>
>>> struct bt_gatt_populate { .. }; /* Probably something with a better name */
>>>
>>> bool bt_gatt_populate_start(struct bt_gatt_populate *, struct
>>> bt_gatt_client *);
>>> bool bt_gatt_populate_add_service(....)
>>> bool bt_gatt_populate_add_characteristic(...)
>>> bool bt_gatt_populate_add_descriptor(....)
>>> bool bt_gatt_populate_add_include(....)
>>> bool bt_gatt_populate_cancel(...)
>>> bool bt_gatt_populate_commit(struct bt_gatt_populate *);
>>>
>>> The start function would tell gatt-client that it's being populated so
>>> it would prevent this from being called twice. All of the _add_*
>>> functions would create a temporary tree owned by the populate
>>> structure, using the same internal structures defined in
>>> shared/gatt-client.c. Calling bt_gatt_populate_commit would then pass
>>> the ownership of the services to the bt_gatt_client, and
>>> bt_gatt_client would perform the MTU exchange, subscribe to
>>> notifications from the "Service Changed" characteristic if one exists,
>>> mark itself as ready and invoke the ready handler.
>>>
>>> If the upper-layer wants to store the current contents of gatt-client,
>>> they can do so by iterating through its services and caching them to a
>>> file directly. This way, shared/gatt-client would remain agnostic to
>>> any storage format.
>>
>> One other way of doing something like this is doing it this way:
>>
>> struct bt_gatt_storage;
>>
>> struct bt_gatt_storage *bt_gatt_storage_new()
>> bt_gatt_storage_add_x
>>
>> struct bt_gatt_client *bt_gatt_client_from_storage(struct bt_gatt_storage);
>>
>> I am not sure this is actually any better. It is just an idea, but we might want to just store plain binary data of cached services in a binary cached file. I think that we can have a binary storage format for the cached data and then we just point the bt_gatt_client_new_with_cahce() to that file and let it load if present. So the upper layers would just decide where that file is located.
>>
>
> I think in the end the exact format of the data should be up to the
> upper layer, whether its binary data or INI format. After some
> discussions on IRC we decided that this storage could be represented
> by shared/gatt-db, which already has these functions to add services,
> characteristics, etc. But we would need to make shared/gatt-db provide
> a client role abstraction as well, since it's more meant for the
> server role at the moment.
>
> This way bluetoothd could for example just read the INI file, create a
> gatt_db and do something like bt_gatt_client_from_db(db, mtu);
following up on my other email, then yes, we could have a bt_gatt_cache that is a collection of bt_gatt_db objects. That however is an implementation detail to me. I think it is important that we have a global concept of a cache. And as said, just having an in-memory cache will already solve 99% of the use cases where we want to avoid service discovery for already known devices. Only cold-boot would be an issue and honestly that is a penalty that I would happily accept for a first iteration. Which means we do not have to bother with file formats in the beginning.
Regards
Marcel
Hi Arman,
>> In the case where bluez is bonded with a remote device we don't want
>> to always perform service discovery, but instead to cache the
>> attribute data permanently and just use that on reconnections.
>> Currently shared/gatt-client automatically discovers all services when
>> created (via bt_gatt_client_new) so we need to add support to populate
>> a shared/gatt-client based on cached data in storage, since a
>> bt_gatt_client instance acts as our in-memory attribute cache for the
>> client role.
>>
>> I initially thought about storing everything in INI format and loading
>> things directly from a file, via some function like
>> bt_gatt_client_new_from_storage. The biggest problem with this
>> approach is parsing the INI format; since we don't want to pull in any
>> external dependencies into shared/gatt (e.g. glib) we would possibly
>> have to write our own key-value file parser for shared/ which seems
>> like overkill. We could possibly invent some simpler blob format for
>> shared/gatt-client but people brought to my attention on IRC that we
>> may want to leave the exact storage format to the upper-layer, e.g.
>> desktop and android versions of bluetoothd may want to use different
>> formats for their attribute cache that's consistent with the platform.
>
> what we could do is just store the plain binary data. These are cache files and if you remove them, that just means next time we have to do full service discovery again.
>
>> So I decided to go with something generic instead. We would basically
>> add functions to manually populate a gatt-client and mark the client
>> as ready only after it's services have been populated. So we would
>> have these API changes:
>>
>> 1. A new argument to bt_gatt_client_new, to tell it not to perform
>> discovery. Something like "bool discover_services".
>>
>> /* Don't perform discovery */
>> bt_gatt_client_new(att, mtu, false);
>>
>> This would basically just leave the structure in "init" state and not
>> become ready.
>
> Adding tons of parameters to a _new() is not really a good idea. I would always add a special _new_without_discovery() or something similar to our API. If we decide to go this route.
>
>>
>> 2. We would then add a new structure to populate a gatt-client.
>>
>> struct bt_gatt_populate { .. }; /* Probably something with a better name */
>>
>> bool bt_gatt_populate_start(struct bt_gatt_populate *, struct
>> bt_gatt_client *);
>> bool bt_gatt_populate_add_service(....)
>> bool bt_gatt_populate_add_characteristic(...)
>> bool bt_gatt_populate_add_descriptor(....)
>> bool bt_gatt_populate_add_include(....)
>> bool bt_gatt_populate_cancel(...)
>> bool bt_gatt_populate_commit(struct bt_gatt_populate *);
>>
>> The start function would tell gatt-client that it's being populated so
>> it would prevent this from being called twice. All of the _add_*
>> functions would create a temporary tree owned by the populate
>> structure, using the same internal structures defined in
>> shared/gatt-client.c. Calling bt_gatt_populate_commit would then pass
>> the ownership of the services to the bt_gatt_client, and
>> bt_gatt_client would perform the MTU exchange, subscribe to
>> notifications from the "Service Changed" characteristic if one exists,
>> mark itself as ready and invoke the ready handler.
>>
>> If the upper-layer wants to store the current contents of gatt-client,
>> they can do so by iterating through its services and caching them to a
>> file directly. This way, shared/gatt-client would remain agnostic to
>> any storage format.
>
> One other way of doing something like this is doing it this way:
>
> struct bt_gatt_storage;
>
> struct bt_gatt_storage *bt_gatt_storage_new()
> bt_gatt_storage_add_x
>
> struct bt_gatt_client *bt_gatt_client_from_storage(struct bt_gatt_storage);
>
> I am not sure this is actually any better. It is just an idea, but we might want to just store plain binary data of cached services in a binary cached file. I think that we can have a binary storage format for the cached data and then we just point the bt_gatt_client_new_with_cahce() to that file and let it load if present. So the upper layers would just decide where that file is located.
coming to think about it we should create a global GATT cache that is really global for that process. Being it bluetoothd or some other client tool. The advantage is then that this cache could be memory only or also optionally use some disk backend if it wants to. My thinking is that just keeping service information in memory so that a bt_gatt_client_new() can re-use next time would be a huge improvement already. Meaning that on reboot or restart of the daemon, you have to do service discovery again, but how many times does that really happen.
struct bt_gatt_cache;
struct bt_gatt_cache_new(void);
struct bt_gatt_cache_ref(struct bt_gatt_cache *cache);
void bt_gatt_cache_unref(struct bt_gatt_cache *cache);
struct bt_gatt_client_with_cache(struct bt_att *att, uint16_t mtu,
struct bt_gatt_cache *cache, uint64_t identifier);
This means we attach a reference of the global cache to each client instance and we allow the caller to create a unique identifier for each client. In most cases that will be just derived from the identity address since that is as unique as it gets, but for being flexible we might want to allow the caller to specify it. We need that when running unit tests over unix sockets. One alternative is that we introduce a bt_att_get_identifer helper that does it for us and creates some sort of unique id based on either the identity address or something from the unix socket.
With this in mind then our GATT client just tells the cache what it wants to be cached and can retrieve it. It means that from a client API nothing changes. And if we want to we can put the cache implementation details into gatt-cache.c and we could allow replacing it with a NULL operation or some alternative formats (if we see the need for it).
Regards
Marcel
Hi Marcel,
> On Fri, Nov 21, 2014 at 6:26 PM, Marcel Holtmann <[email protected]> wr=
ote:
> Hi Arman,
>
>> In the case where bluez is bonded with a remote device we don't want
>> to always perform service discovery, but instead to cache the
>> attribute data permanently and just use that on reconnections.
>> Currently shared/gatt-client automatically discovers all services when
>> created (via bt_gatt_client_new) so we need to add support to populate
>> a shared/gatt-client based on cached data in storage, since a
>> bt_gatt_client instance acts as our in-memory attribute cache for the
>> client role.
>>
>> I initially thought about storing everything in INI format and loading
>> things directly from a file, via some function like
>> bt_gatt_client_new_from_storage. The biggest problem with this
>> approach is parsing the INI format; since we don't want to pull in any
>> external dependencies into shared/gatt (e.g. glib) we would possibly
>> have to write our own key-value file parser for shared/ which seems
>> like overkill. We could possibly invent some simpler blob format for
>> shared/gatt-client but people brought to my attention on IRC that we
>> may want to leave the exact storage format to the upper-layer, e.g.
>> desktop and android versions of bluetoothd may want to use different
>> formats for their attribute cache that's consistent with the platform.
>
> what we could do is just store the plain binary data. These are cache fil=
es and if you remove them, that just means next time we have to do full ser=
vice discovery again.
>
>> So I decided to go with something generic instead. We would basically
>> add functions to manually populate a gatt-client and mark the client
>> as ready only after it's services have been populated. So we would
>> have these API changes:
>>
>> 1. A new argument to bt_gatt_client_new, to tell it not to perform
>> discovery. Something like "bool discover_services".
>>
>> /* Don't perform discovery */
>> bt_gatt_client_new(att, mtu, false);
>>
>> This would basically just leave the structure in "init" state and not
>> become ready.
>
> Adding tons of parameters to a _new() is not really a good idea. I would =
always add a special _new_without_discovery() or something similar to our A=
PI. If we decide to go this route.
>
>>
>> 2. We would then add a new structure to populate a gatt-client.
>>
>> struct bt_gatt_populate { .. }; /* Probably something with a better =
name */
>>
>> bool bt_gatt_populate_start(struct bt_gatt_populate *, struct
>> bt_gatt_client *);
>> bool bt_gatt_populate_add_service(....)
>> bool bt_gatt_populate_add_characteristic(...)
>> bool bt_gatt_populate_add_descriptor(....)
>> bool bt_gatt_populate_add_include(....)
>> bool bt_gatt_populate_cancel(...)
>> bool bt_gatt_populate_commit(struct bt_gatt_populate *);
>>
>> The start function would tell gatt-client that it's being populated so
>> it would prevent this from being called twice. All of the _add_*
>> functions would create a temporary tree owned by the populate
>> structure, using the same internal structures defined in
>> shared/gatt-client.c. Calling bt_gatt_populate_commit would then pass
>> the ownership of the services to the bt_gatt_client, and
>> bt_gatt_client would perform the MTU exchange, subscribe to
>> notifications from the "Service Changed" characteristic if one exists,
>> mark itself as ready and invoke the ready handler.
>>
>> If the upper-layer wants to store the current contents of gatt-client,
>> they can do so by iterating through its services and caching them to a
>> file directly. This way, shared/gatt-client would remain agnostic to
>> any storage format.
>
> One other way of doing something like this is doing it this way:
>
> struct bt_gatt_storage;
>
> struct bt_gatt_storage *bt_gatt_storage_new()
> bt_gatt_storage_add_x
>
> struct bt_gatt_client *bt_gatt_client_from_storage(struct bt_gatt=
_storage);
>
> I am not sure this is actually any better. It is just an idea, but we mig=
ht want to just store plain binary data of cached services in a binary cach=
ed file. I think that we can have a binary storage format for the cached da=
ta and then we just point the bt_gatt_client_new_with_cahce() to that file =
and let it load if present. So the upper layers would just decide where tha=
t file is located.
>
I think in the end the exact format of the data should be up to the
upper layer, whether its binary data or INI format. After some
discussions on IRC we decided that this storage could be represented
by shared/gatt-db, which already has these functions to add services,
characteristics, etc. But we would need to make shared/gatt-db provide
a client role abstraction as well, since it's more meant for the
server role at the moment.
This way bluetoothd could for example just read the INI file, create a
gatt_db and do something like bt_gatt_client_from_db(db, mtu);
> Regards
>
> Marcel
>
Arman
Hi Arman,
> In the case where bluez is bonded with a remote device we don't want
> to always perform service discovery, but instead to cache the
> attribute data permanently and just use that on reconnections.
> Currently shared/gatt-client automatically discovers all services when
> created (via bt_gatt_client_new) so we need to add support to populate
> a shared/gatt-client based on cached data in storage, since a
> bt_gatt_client instance acts as our in-memory attribute cache for the
> client role.
>
> I initially thought about storing everything in INI format and loading
> things directly from a file, via some function like
> bt_gatt_client_new_from_storage. The biggest problem with this
> approach is parsing the INI format; since we don't want to pull in any
> external dependencies into shared/gatt (e.g. glib) we would possibly
> have to write our own key-value file parser for shared/ which seems
> like overkill. We could possibly invent some simpler blob format for
> shared/gatt-client but people brought to my attention on IRC that we
> may want to leave the exact storage format to the upper-layer, e.g.
> desktop and android versions of bluetoothd may want to use different
> formats for their attribute cache that's consistent with the platform.
what we could do is just store the plain binary data. These are cache files and if you remove them, that just means next time we have to do full service discovery again.
> So I decided to go with something generic instead. We would basically
> add functions to manually populate a gatt-client and mark the client
> as ready only after it's services have been populated. So we would
> have these API changes:
>
> 1. A new argument to bt_gatt_client_new, to tell it not to perform
> discovery. Something like "bool discover_services".
>
> /* Don't perform discovery */
> bt_gatt_client_new(att, mtu, false);
>
> This would basically just leave the structure in "init" state and not
> become ready.
Adding tons of parameters to a _new() is not really a good idea. I would always add a special _new_without_discovery() or something similar to our API. If we decide to go this route.
>
> 2. We would then add a new structure to populate a gatt-client.
>
> struct bt_gatt_populate { .. }; /* Probably something with a better name */
>
> bool bt_gatt_populate_start(struct bt_gatt_populate *, struct
> bt_gatt_client *);
> bool bt_gatt_populate_add_service(....)
> bool bt_gatt_populate_add_characteristic(...)
> bool bt_gatt_populate_add_descriptor(....)
> bool bt_gatt_populate_add_include(....)
> bool bt_gatt_populate_cancel(...)
> bool bt_gatt_populate_commit(struct bt_gatt_populate *);
>
> The start function would tell gatt-client that it's being populated so
> it would prevent this from being called twice. All of the _add_*
> functions would create a temporary tree owned by the populate
> structure, using the same internal structures defined in
> shared/gatt-client.c. Calling bt_gatt_populate_commit would then pass
> the ownership of the services to the bt_gatt_client, and
> bt_gatt_client would perform the MTU exchange, subscribe to
> notifications from the "Service Changed" characteristic if one exists,
> mark itself as ready and invoke the ready handler.
>
> If the upper-layer wants to store the current contents of gatt-client,
> they can do so by iterating through its services and caching them to a
> file directly. This way, shared/gatt-client would remain agnostic to
> any storage format.
One other way of doing something like this is doing it this way:
struct bt_gatt_storage;
struct bt_gatt_storage *bt_gatt_storage_new()
bt_gatt_storage_add_x
struct bt_gatt_client *bt_gatt_client_from_storage(struct bt_gatt_storage);
I am not sure this is actually any better. It is just an idea, but we might want to just store plain binary data of cached services in a binary cached file. I think that we can have a binary storage format for the cached data and then we just point the bt_gatt_client_new_with_cahce() to that file and let it load if present. So the upper layers would just decide where that file is located.
Regards
Marcel
Hi Luiz,
> On Fri, Nov 21, 2014 at 6:24 AM, Luiz Augusto von Dentz <[email protected]> wrote:
> Hi Arman,
>
> On Thu, Nov 20, 2014 at 5:08 AM, Arman Uguray <[email protected]> wrote:
>> Hi,
>>
>>> On Wed, Nov 19, 2014 at 1:48 PM, Arman Uguray <[email protected]> wrote:
>>> Hi all,
>>>
>>> In the case where bluez is bonded with a remote device we don't want
>>> to always perform service discovery, but instead to cache the
>>> attribute data permanently and just use that on reconnections.
>>> Currently shared/gatt-client automatically discovers all services when
>>> created (via bt_gatt_client_new) so we need to add support to populate
>>> a shared/gatt-client based on cached data in storage, since a
>>> bt_gatt_client instance acts as our in-memory attribute cache for the
>>> client role.
>>>
>>> I initially thought about storing everything in INI format and loading
>>> things directly from a file, via some function like
>>> bt_gatt_client_new_from_storage. The biggest problem with this
>>> approach is parsing the INI format; since we don't want to pull in any
>>> external dependencies into shared/gatt (e.g. glib) we would possibly
>>> have to write our own key-value file parser for shared/ which seems
>>> like overkill. We could possibly invent some simpler blob format for
>>> shared/gatt-client but people brought to my attention on IRC that we
>>> may want to leave the exact storage format to the upper-layer, e.g.
>>> desktop and android versions of bluetoothd may want to use different
>>> formats for their attribute cache that's consistent with the platform.
>>>
>>> So I decided to go with something generic instead. We would basically
>>> add functions to manually populate a gatt-client and mark the client
>>> as ready only after it's services have been populated. So we would
>>> have these API changes:
>>>
>>> 1. A new argument to bt_gatt_client_new, to tell it not to perform
>>> discovery. Something like "bool discover_services".
>>>
>>> /* Don't perform discovery */
>>> bt_gatt_client_new(att, mtu, false);
>>>
>>> This would basically just leave the structure in "init" state and not
>>> become ready.
>>>
>>> 2. We would then add a new structure to populate a gatt-client.
>>>
>>> struct bt_gatt_populate { .. }; /* Probably something with a better name */
>>>
>>> bool bt_gatt_populate_start(struct bt_gatt_populate *, struct
>>> bt_gatt_client *);
>>> bool bt_gatt_populate_add_service(....)
>>> bool bt_gatt_populate_add_characteristic(...)
>>> bool bt_gatt_populate_add_descriptor(....)
>>> bool bt_gatt_populate_add_include(....)
>>> bool bt_gatt_populate_cancel(...)
>>> bool bt_gatt_populate_commit(struct bt_gatt_populate *);
>>>
>>> The start function would tell gatt-client that it's being populated so
>>> it would prevent this from being called twice. All of the _add_*
>>> functions would create a temporary tree owned by the populate
>>> structure, using the same internal structures defined in
>>> shared/gatt-client.c. Calling bt_gatt_populate_commit would then pass
>>> the ownership of the services to the bt_gatt_client, and
>>> bt_gatt_client would perform the MTU exchange, subscribe to
>>> notifications from the "Service Changed" characteristic if one exists,
>>> mark itself as ready and invoke the ready handler.
>>>
>>> If the upper-layer wants to store the current contents of gatt-client,
>>> they can do so by iterating through its services and caching them to a
>>> file directly. This way, shared/gatt-client would remain agnostic to
>>> any storage format.
>>>
>>> Comments requested.
>>> Arman
>
> I discussed with Johan about this, perhaps we should reuse bt_gatt_db
> also for client which could simplify the API quite a bit, and remove
> the need of iterators. So basically each device would have a instance
> of bt_gatt_db which can be populated from the storage on init but more
> importantly we can keep it alive as longs as the device is paired so
> it became very similar to what we are already doing with
> bt_gatt_server. We probably gonna need to adapt a little bit
> bt_gatt_db, perhaps a reset is needed if service changes, etc, for the
> iterators we could probably replace with gatt_db_find_by_type then you
> pass the type you want: primary, secondary, etc, which in my opinion
> is a much simpler API. Also we could take advantage of
> gatt_db_attribute so that the read and write callbacks are registered
> by the core which takes care of invoking bt_gatt_client that way the
> interface between core and plugins could be done entirely with
> gatt_db_attribute.
>
> What do you thing? It probably sets us back a little bit in terms of
> converting the core but I think it is worth it considering since it
> would align server and client db abstraction.
>
I'm not against simplifying the iterators or perhaps making db and
client look similar but I think that the current gatt_db API is too
low-level for client usage. Every time a profile wants to iterate
through characteristics, or query a characteristics descriptors, they
are going to have to make multiple calls to read_by_type,
find_information, etc, which will return partial information, then
obtain the attribute values, decode and interpret them. I don't see
this as a clean GATT abstraction. To be frank, if that's what we're
going to end up doing, the new shared/ code won't be much of an
improvement over attrib/ which at least provides this kind of
abstraction.
Also note that from the perspective of a user of gatt_db, this
wouldn't make client/server cases consistent. A user of gatt_db
registers services, characteristics, descriptors, not individual
"attributes" so to say. The user is still dealing with a higher level
abstraction, and functions like read_by_type, which map to ATT
protocol operations, are only called by bt_gatt_server, so the user
doesn't have to interact with them.
So, I think we're going to need to stick with the iterators or if
we're going to use gatt_db for pre-populating a bt_gatt_client or
perhaps use one to store remote attributes in general, then we'll want
to provide a similar abstraction for gatt_db, so that one can iterate
through its services, characteristics, etc.
> --
> Luiz Augusto von Dentz
-Arman
On Fri, Nov 21, 2014 at 6:24 AM, Luiz Augusto von Dentz
<[email protected]> wrote:
> Hi Arman,
>
> On Thu, Nov 20, 2014 at 5:08 AM, Arman Uguray <[email protected]> wrote:
>> Hi,
>>
>>> On Wed, Nov 19, 2014 at 1:48 PM, Arman Uguray <[email protected]> wrote:
>>> Hi all,
>>>
>>> In the case where bluez is bonded with a remote device we don't want
>>> to always perform service discovery, but instead to cache the
>>> attribute data permanently and just use that on reconnections.
>>> Currently shared/gatt-client automatically discovers all services when
>>> created (via bt_gatt_client_new) so we need to add support to populate
>>> a shared/gatt-client based on cached data in storage, since a
>>> bt_gatt_client instance acts as our in-memory attribute cache for the
>>> client role.
>>>
>>> I initially thought about storing everything in INI format and loading
>>> things directly from a file, via some function like
>>> bt_gatt_client_new_from_storage. The biggest problem with this
>>> approach is parsing the INI format; since we don't want to pull in any
>>> external dependencies into shared/gatt (e.g. glib) we would possibly
>>> have to write our own key-value file parser for shared/ which seems
>>> like overkill. We could possibly invent some simpler blob format for
>>> shared/gatt-client but people brought to my attention on IRC that we
>>> may want to leave the exact storage format to the upper-layer, e.g.
>>> desktop and android versions of bluetoothd may want to use different
>>> formats for their attribute cache that's consistent with the platform.
>>>
>>> So I decided to go with something generic instead. We would basically
>>> add functions to manually populate a gatt-client and mark the client
>>> as ready only after it's services have been populated. So we would
>>> have these API changes:
>>>
>>> 1. A new argument to bt_gatt_client_new, to tell it not to perform
>>> discovery. Something like "bool discover_services".
>>>
>>> /* Don't perform discovery */
>>> bt_gatt_client_new(att, mtu, false);
>>>
>>> This would basically just leave the structure in "init" state and not
>>> become ready.
>>>
>>> 2. We would then add a new structure to populate a gatt-client.
>>>
>>> struct bt_gatt_populate { .. }; /* Probably something with a better name */
>>>
>>> bool bt_gatt_populate_start(struct bt_gatt_populate *, struct
>>> bt_gatt_client *);
>>> bool bt_gatt_populate_add_service(....)
>>> bool bt_gatt_populate_add_characteristic(...)
>>> bool bt_gatt_populate_add_descriptor(....)
>>> bool bt_gatt_populate_add_include(....)
>>> bool bt_gatt_populate_cancel(...)
>>> bool bt_gatt_populate_commit(struct bt_gatt_populate *);
>>>
>>> The start function would tell gatt-client that it's being populated so
>>> it would prevent this from being called twice. All of the _add_*
>>> functions would create a temporary tree owned by the populate
>>> structure, using the same internal structures defined in
>>> shared/gatt-client.c. Calling bt_gatt_populate_commit would then pass
>>> the ownership of the services to the bt_gatt_client, and
>>> bt_gatt_client would perform the MTU exchange, subscribe to
>>> notifications from the "Service Changed" characteristic if one exists,
>>> mark itself as ready and invoke the ready handler.
>>>
>>> If the upper-layer wants to store the current contents of gatt-client,
>>> they can do so by iterating through its services and caching them to a
>>> file directly. This way, shared/gatt-client would remain agnostic to
>>> any storage format.
>>>
>>> Comments requested.
>>> Arman
>
> I discussed with Johan about this, perhaps we should reuse bt_gatt_db
> also for client which could simplify the API quite a bit, and remove
> the need of iterators. So basically each device would have a instance
> of bt_gatt_db which can be populated from the storage on init but more
> importantly we can keep it alive as longs as the device is paired so
> it became very similar to what we are already doing with
> bt_gatt_server. We probably gonna need to adapt a little bit
> bt_gatt_db, perhaps a reset is needed if service changes, etc, for the
> iterators we could probably replace with gatt_db_find_by_type then you
> pass the type you want: primary, secondary, etc, which in my opinion
> is a much simpler API. Also we could take advantage of
> gatt_db_attribute so that the read and write callbacks are registered
> by the core which takes care of invoking bt_gatt_client that way the
> interface between core and plugins could be done entirely with
> gatt_db_attribute.
>
> What do you thing? It probably sets us back a little bit in terms of
> converting the core but I think it is worth it considering since it
> would align server and client db abstraction.
>
> --
> Luiz Augusto von Dentz
Hi Arman,
On Thu, Nov 20, 2014 at 5:08 AM, Arman Uguray <[email protected]> wrote:
> Hi,
>
>> On Wed, Nov 19, 2014 at 1:48 PM, Arman Uguray <[email protected]> wrote:
>> Hi all,
>>
>> In the case where bluez is bonded with a remote device we don't want
>> to always perform service discovery, but instead to cache the
>> attribute data permanently and just use that on reconnections.
>> Currently shared/gatt-client automatically discovers all services when
>> created (via bt_gatt_client_new) so we need to add support to populate
>> a shared/gatt-client based on cached data in storage, since a
>> bt_gatt_client instance acts as our in-memory attribute cache for the
>> client role.
>>
>> I initially thought about storing everything in INI format and loading
>> things directly from a file, via some function like
>> bt_gatt_client_new_from_storage. The biggest problem with this
>> approach is parsing the INI format; since we don't want to pull in any
>> external dependencies into shared/gatt (e.g. glib) we would possibly
>> have to write our own key-value file parser for shared/ which seems
>> like overkill. We could possibly invent some simpler blob format for
>> shared/gatt-client but people brought to my attention on IRC that we
>> may want to leave the exact storage format to the upper-layer, e.g.
>> desktop and android versions of bluetoothd may want to use different
>> formats for their attribute cache that's consistent with the platform.
>>
>> So I decided to go with something generic instead. We would basically
>> add functions to manually populate a gatt-client and mark the client
>> as ready only after it's services have been populated. So we would
>> have these API changes:
>>
>> 1. A new argument to bt_gatt_client_new, to tell it not to perform
>> discovery. Something like "bool discover_services".
>>
>> /* Don't perform discovery */
>> bt_gatt_client_new(att, mtu, false);
>>
>> This would basically just leave the structure in "init" state and not
>> become ready.
>>
>> 2. We would then add a new structure to populate a gatt-client.
>>
>> struct bt_gatt_populate { .. }; /* Probably something with a better name */
>>
>> bool bt_gatt_populate_start(struct bt_gatt_populate *, struct
>> bt_gatt_client *);
>> bool bt_gatt_populate_add_service(....)
>> bool bt_gatt_populate_add_characteristic(...)
>> bool bt_gatt_populate_add_descriptor(....)
>> bool bt_gatt_populate_add_include(....)
>> bool bt_gatt_populate_cancel(...)
>> bool bt_gatt_populate_commit(struct bt_gatt_populate *);
>>
>> The start function would tell gatt-client that it's being populated so
>> it would prevent this from being called twice. All of the _add_*
>> functions would create a temporary tree owned by the populate
>> structure, using the same internal structures defined in
>> shared/gatt-client.c. Calling bt_gatt_populate_commit would then pass
>> the ownership of the services to the bt_gatt_client, and
>> bt_gatt_client would perform the MTU exchange, subscribe to
>> notifications from the "Service Changed" characteristic if one exists,
>> mark itself as ready and invoke the ready handler.
>>
>> If the upper-layer wants to store the current contents of gatt-client,
>> they can do so by iterating through its services and caching them to a
>> file directly. This way, shared/gatt-client would remain agnostic to
>> any storage format.
>>
>> Comments requested.
>> Arman
I discussed with Johan about this, perhaps we should reuse bt_gatt_db
also for client which could simplify the API quite a bit, and remove
the need of iterators. So basically each device would have a instance
of bt_gatt_db which can be populated from the storage on init but more
importantly we can keep it alive as longs as the device is paired so
it became very similar to what we are already doing with
bt_gatt_server. We probably gonna need to adapt a little bit
bt_gatt_db, perhaps a reset is needed if service changes, etc, for the
iterators we could probably replace with gatt_db_find_by_type then you
pass the type you want: primary, secondary, etc, which in my opinion
is a much simpler API. Also we could take advantage of
gatt_db_attribute so that the read and write callbacks are registered
by the core which takes care of invoking bt_gatt_client that way the
interface between core and plugins could be done entirely with
gatt_db_attribute.
What do you thing? It probably sets us back a little bit in terms of
converting the core but I think it is worth it considering since it
would align server and client db abstraction.
--
Luiz Augusto von Dentz
Hi,
> On Wed, Nov 19, 2014 at 1:48 PM, Arman Uguray <[email protected]> wrote:
> Hi all,
>
> In the case where bluez is bonded with a remote device we don't want
> to always perform service discovery, but instead to cache the
> attribute data permanently and just use that on reconnections.
> Currently shared/gatt-client automatically discovers all services when
> created (via bt_gatt_client_new) so we need to add support to populate
> a shared/gatt-client based on cached data in storage, since a
> bt_gatt_client instance acts as our in-memory attribute cache for the
> client role.
>
> I initially thought about storing everything in INI format and loading
> things directly from a file, via some function like
> bt_gatt_client_new_from_storage. The biggest problem with this
> approach is parsing the INI format; since we don't want to pull in any
> external dependencies into shared/gatt (e.g. glib) we would possibly
> have to write our own key-value file parser for shared/ which seems
> like overkill. We could possibly invent some simpler blob format for
> shared/gatt-client but people brought to my attention on IRC that we
> may want to leave the exact storage format to the upper-layer, e.g.
> desktop and android versions of bluetoothd may want to use different
> formats for their attribute cache that's consistent with the platform.
>
> So I decided to go with something generic instead. We would basically
> add functions to manually populate a gatt-client and mark the client
> as ready only after it's services have been populated. So we would
> have these API changes:
>
> 1. A new argument to bt_gatt_client_new, to tell it not to perform
> discovery. Something like "bool discover_services".
>
> /* Don't perform discovery */
> bt_gatt_client_new(att, mtu, false);
>
> This would basically just leave the structure in "init" state and not
> become ready.
>
> 2. We would then add a new structure to populate a gatt-client.
>
> struct bt_gatt_populate { .. }; /* Probably something with a better name */
>
> bool bt_gatt_populate_start(struct bt_gatt_populate *, struct
> bt_gatt_client *);
> bool bt_gatt_populate_add_service(....)
> bool bt_gatt_populate_add_characteristic(...)
> bool bt_gatt_populate_add_descriptor(....)
> bool bt_gatt_populate_add_include(....)
> bool bt_gatt_populate_cancel(...)
> bool bt_gatt_populate_commit(struct bt_gatt_populate *);
>
> The start function would tell gatt-client that it's being populated so
> it would prevent this from being called twice. All of the _add_*
> functions would create a temporary tree owned by the populate
> structure, using the same internal structures defined in
> shared/gatt-client.c. Calling bt_gatt_populate_commit would then pass
> the ownership of the services to the bt_gatt_client, and
> bt_gatt_client would perform the MTU exchange, subscribe to
> notifications from the "Service Changed" characteristic if one exists,
> mark itself as ready and invoke the ready handler.
>
> If the upper-layer wants to store the current contents of gatt-client,
> they can do so by iterating through its services and caching them to a
> file directly. This way, shared/gatt-client would remain agnostic to
> any storage format.
>
> Comments requested.
> Arman
I just sent out a patch as RFC that implements this.
Cheers,
Arman