Hi all,
I've been working on adding the client-side GATT API to BlueZ
src/gatt. It occurred to me (both as I delved into the code and after
discussions with Claudio Takahasi) that combining the "struct io"
based abstraction used by the new btd_attribute and the current
connection management that depends on GAttrib gets a bit messy. For
client mode alone, handling both incoming connections from a remote
device, and issuing outgoing read/write and primary/characteristic
discovery requests CAN be done with GAttrib, but the btd_attribute
abstraction appears to be cleaner I would prefer to keep the client
and server mode implementations consistent.
To that end, I'd like to figure out what's the best way to approach
this. Clearly, the GAttrib dependency runs deep: the API provided in
attrib/gatt.h uses GAttrib and is used by src/device.c and pretty much
all the GATT based profile implementations. The profiles seem to use
the callback interface defined in src/attio.h which is how src/device
allows the profiles to access it's internal GAttrib instance.
attrib/gatt.h is also currently used by gatttool.
Anderson Lizardo and I had some discussions about this and he
mentioned that one of the earlier ideas was to simply replace GAttrib
with struct io. Another thing that he mentioned is to keep the code in
src/gatt restricted to handling client and server side read/write
requests (i.e. attribute access abstraction) and put no discovery
related logic there.
So my question is, how do we want to do this refactor? Do we basically
want to have all references to GAttrib replaced with struct io,
keeping the API in attrib/gatt and the callbacks in src/attio the same
but using struct io instead of GAttrib? Or do we want a more revised
API?
Any input/suggestion will be appreciated. Once we know what exactly it
is that we want with this code, we can come up with a plan to
deprecate GAttrib.
Thanks!
Arman
> To that end, I'd like to figure out what's the best way to approach
> this. Clearly, the GAttrib dependency runs deep: the API provided in
> attrib/gatt.h uses GAttrib and is used by src/device.c and pretty much
> all the GATT based profile implementations. The profiles seem to use
> the callback interface defined in src/attio.h which is how src/device
> allows the profiles to access it's internal GAttrib instance.
> attrib/gatt.h is also currently used by gatttool.
In addition to the above, I realized that recent changes to
android/gatt also use GAttrib. So adding that to list too.
Hi Marcel,
Sounds good. I had a chance to sync up with Claudio and Lizardo
face-to-face today at the Bluetooth World Conference and we had a long
discussion on how to do this refactor. I will go ahead and start a new
(and unit tested) implementation of the ATT layer as "struct bt_att"
as you suggested. I will integrate this with attrib/gatt as a first
step, replacing attrib/att. I will then move on to implement a bt_gatt
and bt_att_db.
-Arman
On Tue, Apr 8, 2014 at 6:28 PM, Marcel Holtmann <[email protected]> wrote:
> Hi Arman,
>
>>> I am pretty open on how many steps we want to take at a time or what we want to do later. What I know for sure is that I
>>> want to rid of GIOChannel and BtIO usage and GLib.
>>
>> I agree with everything you described and I think a clean GLib-free
>> implementation that can be shared among Linux and Android code is the
>> way to go. My only concern is that a D-Bus GATT client API is much
>> needed for Chromium and I'm concerned that the big overhaul involved
>> in removing BtIO and GLib is going to take too long.
>>
>> So my question is this: how open would you be for an initial GATT
>> client API that uses the existing code in attrib/* internally? I'm
>> thinking of something very similar to how android/gatt.c has been
>> implemented so far. So what I have in mind is:
>>
>> - The bt_gatt structure you mentioned, lives in src/shared/gatt.*
>> and internally uses GAttrib, etc. The code is D-Bus free and could be
>> used by the GATT client code in android/gatt as well.
>> - D-Bus code added to src/gatt-dbus.* to expose client objects.
>> This code talks to src/shared/gatt.*.
>> - src/device.* and profiles modified to use the new API with no
>> explicit GIOChannel usage.
>
> I have no problem in doing this in parallel and take smaller or different steps. My main point is that everybody knows where we need to go in the long term.
>
>> What I'm suggesting is basically to make the daemon use the new API
>> first and implement the new API with the existing GLib based code so
>> that we have at least some implementation to work with for Chromium
>> (and we isolate GLib usage to one place as far as GATT is concerned),
>> and THEN implement bt_gatt, bt_att etc. the proper way. Is this
>> something that you would be open to upstreaming? My primary concern is
>> simply the amount of time it will take to implement a new ATT/GATT
>> endpoint but if you believe that we should absolutely do it the right
>> way the first time around then it's not the end of the world.
>
> Go ahead with it. Internal refactoring is simple. Just plan to have unit tests included that check will ensure that all changes you make are tested.
>
> We did this for AVDTP, AVRCP and now MCAP for our refactoring to make code shareable between Android and upstream.
>
> Regards
>
> Marcel
>
Hi Arman,
>> I am pretty open on how many steps we want to take at a time or what we want to do later. What I know for sure is that I
>> want to rid of GIOChannel and BtIO usage and GLib.
>
> I agree with everything you described and I think a clean GLib-free
> implementation that can be shared among Linux and Android code is the
> way to go. My only concern is that a D-Bus GATT client API is much
> needed for Chromium and I'm concerned that the big overhaul involved
> in removing BtIO and GLib is going to take too long.
>
> So my question is this: how open would you be for an initial GATT
> client API that uses the existing code in attrib/* internally? I'm
> thinking of something very similar to how android/gatt.c has been
> implemented so far. So what I have in mind is:
>
> - The bt_gatt structure you mentioned, lives in src/shared/gatt.*
> and internally uses GAttrib, etc. The code is D-Bus free and could be
> used by the GATT client code in android/gatt as well.
> - D-Bus code added to src/gatt-dbus.* to expose client objects.
> This code talks to src/shared/gatt.*.
> - src/device.* and profiles modified to use the new API with no
> explicit GIOChannel usage.
I have no problem in doing this in parallel and take smaller or different steps. My main point is that everybody knows where we need to go in the long term.
> What I'm suggesting is basically to make the daemon use the new API
> first and implement the new API with the existing GLib based code so
> that we have at least some implementation to work with for Chromium
> (and we isolate GLib usage to one place as far as GATT is concerned),
> and THEN implement bt_gatt, bt_att etc. the proper way. Is this
> something that you would be open to upstreaming? My primary concern is
> simply the amount of time it will take to implement a new ATT/GATT
> endpoint but if you believe that we should absolutely do it the right
> way the first time around then it's not the end of the world.
Go ahead with it. Internal refactoring is simple. Just plan to have unit tests included that check will ensure that all changes you make are tested.
We did this for AVDTP, AVRCP and now MCAP for our refactoring to make code shareable between Android and upstream.
Regards
Marcel
Hi Marcel,
> I am pretty open on how many steps we want to take at a time or what we want to do later. What I know for sure is that I
> want to rid of GIOChannel and BtIO usage and GLib.
I agree with everything you described and I think a clean GLib-free
implementation that can be shared among Linux and Android code is the
way to go. My only concern is that a D-Bus GATT client API is much
needed for Chromium and I'm concerned that the big overhaul involved
in removing BtIO and GLib is going to take too long.
So my question is this: how open would you be for an initial GATT
client API that uses the existing code in attrib/* internally? I'm
thinking of something very similar to how android/gatt.c has been
implemented so far. So what I have in mind is:
- The bt_gatt structure you mentioned, lives in src/shared/gatt.*
and internally uses GAttrib, etc. The code is D-Bus free and could be
used by the GATT client code in android/gatt as well.
- D-Bus code added to src/gatt-dbus.* to expose client objects.
This code talks to src/shared/gatt.*.
- src/device.* and profiles modified to use the new API with no
explicit GIOChannel usage.
What I'm suggesting is basically to make the daemon use the new API
first and implement the new API with the existing GLib based code so
that we have at least some implementation to work with for Chromium
(and we isolate GLib usage to one place as far as GATT is concerned),
and THEN implement bt_gatt, bt_att etc. the proper way. Is this
something that you would be open to upstreaming? My primary concern is
simply the amount of time it will take to implement a new ATT/GATT
endpoint but if you believe that we should absolutely do it the right
way the first time around then it's not the end of the world.
Cheers,
Arman
Hi Arman,
>> at the moment ConnectProfile() is BR/EDR specific and it most likely will stay way. So for LE only devices it
>> should just return an error at the moment.
>
> This simplifies things quite a bit.
>
>> We want to use Service Changed notification and do a selective re-discover to avoid the extra overhead that a
>> full discovery might cause. It does not have to be done in the beginning, but that is where this should be
>> heading towards.
>
> That makes sense, and this is what I had in mind as well.
>
>> Some time back I started look into writing src/shared/att.c that really is suppose to be dead simple and just
>> treats ATT protocol as a transport. This is how I see that basic interaction.
>>
>> struct bt_att;
>>
>> struct bt_att *bt_att_new(int fd);
>>
>> struct bt_att *att_ref(struct bt_att *att);
>> void bt_att_unref(struct bt_att *att);
>>
>> bool bt_att_set_debug(struct bt_att *att, bt_att_debug_func_t callback,
>> void *user_data, bt_att_destroy_func_t destroy);
>>
>> bool bt_att_set_close_on_unref(struct bt_att *att, bool do_close);
>>
>> For a lot of new code, I prefer if we are using file descriptors as base. Then it becomes easy to use on a
>> socketpair for unit testing or put this onto a different transport.
>>
>> The GIOChannel and BtIO forces us to have knowledge about every single detail of transport. I think we did
>> this the wrong way around. The actual IO handling should be done inside struct att and be an implementation
>> detail. And it should just deal with the transport and not the setup of the transport. The setup is out of scope
>> for the protocol handling.
>
> That makes sense. So in the end we would have src/device explicitly
> set up the l2cap socket to the remote device and create a struct
> bt_att that it owns and all the profiles would add a ref to that
> struct bt_att to perform I/O?
the way I see the kernel evolving is that we either get an L2CAP fd for the fixed ATT channel from accept() or from connect(). Keep in mind that auto-connect from kernel side we get it via accept().
You would hand over the fd to bt_att and mark it as close_on_unref and then just let bt_att handle the whole handling.
Maybe this needs to be two layered and we should have a bt_gatt that takes a fd and bt_att_db for the database. So internally bt_gatt would then use bt_att and bt_add_db. Just an idea at this point.
> It seems to me that we ultimately want an API that is just like
> attrib/gatt.h that uses "struct bt_att" instead of GAttrib. There is
> already a lot of code in attrib/* that is useful for dealing with the
> attribute protocol and I don't really see the point of throwing that
> code away, especially attrib/att.h can be used (at least initially) by
> the new src/shared/att to encode/decode ATT MTUs.
>
> As for the higher-level GATT interactions: we are clearly moving
> towards a btd_attribute based GATT client/server API. So it makes
> sense to approach this from the top-down. Here is what I propose for a
> step-by-step refactor:
>
> 1. Add src/shared/att.*. Define struct bt_att as a simple wrapper
> around the file descriptor with basic reference counting logic similar
> to src/shared/mgmt (i.e. bt_att_set_close_on_unref, etc.) only.
Since ATT is one transaction at a time, we can make it even simpler. So I want basic PDU handling here. Something like bt_att_send for request/response and bt_att_register for handling notification. The devil is a bit in the details here, but the way the ATT transactions are defined this should be rather simple in the end.
> 2. Define a new GATT API for remote attribute discovery in src/gatt.*.
> Have the functions take in a struct btd_att pointer. Have all
> attributes represented as btd_attribute. Perhaps get this code tested
> by introducing a new gatttool.
Personally I think we should add an interactive mode to btmgmt and also add ATT/GATT support to btmgmt. My thinking is that bluetoothctl is for D-Bus based APIs and btmgmt will handle the low-level mgmt and socket APIs directly. This will be needed for qualification and low-level testing anyway. So a good idea to confine this into a single tool. Remember that in some cases you want to bring up some GATT DB without having to run bluetoothd.
> 3. Implement the GATT discovery functions by using GAttrib inside
> src/gatt.c at first, calling into the functions in attrib/gatt.h.
I think we might want to establish src/shared/gatt.[ch] with bt_gatt as main entry for client and server handling. As I said above, it can take an bt_att_db or even empty db for no registered services.
So what I am thinking in a really dead simple case you do things like this:
fd = l2cap_connect_att_over_le(bdaddr, bdaddr_type);
gatt = bt_gatt_new(fd, NULL);
bt_gatt_set_close_on_unref(gatt, true);
bt_gatt_discover(gatt, callback);
bt_gatt_unref(gatt);
> 4. Partially remove the GAttrib dependency in src/device by having it
> call into the new discovery functions in src/gatt instead of calling
> attrib/gatt functions directly. At this point src/device will probably
> still create the connection using bt_io_connect but it will wrap the
> underlying fd around a "struct bt_att" instead of a GAttrib when
> making calls to src/gatt.
>
> Here we will remove the call to attrib_from_device (defined in
> src/attrib-server.h) from src/device.c. I don't know if this will
> break anything big, but it seems to be how incoming connections are
> currently being handled. I think this logic has been pretty much taken
> over by the new GATT server code in src/gatt.*, but correct me if I'm
> wrong.
>
> To prevent breaking the profiles that use attio and GAttrib,
> src/device will keep notifying registered attio callbacks in this
> step. This means that it will probably create a GAttrib simply for the
> purpose of notifying the profiles but it won't use it to call into
> attrib/gatt functions.
We can just establish the basic shared functions for ATT and GATT first and start testing them out in btmgmt tool. Once we now they work, we can modify the daemon code.
> 5. Have the profiles call into the new functions in src/gatt for
> attribute requests instead of using attrib/gatt. This is the step
> where we remove GAttrib from the profiles and modify attio to take in
> a "struct bt_att".
>
> 6. Remove GAttrib entirely from src/device.
>
> 7. Remove GIOChannel from src/device by removing calls to
> bt_io_connect and have it create the raw fd directly.
>
> Sorry for the long email. Let me know if any of the above doesn't make
> sense or if there is a much better/simpler way to approach this. There
> might also be some things that I'm missing, for example there might be
> complexities involving pairing and bonding steps and incoming
> connections. I'm guessing that once step 5 is done we can add the GATT
> D-Bus client API code. With all 7 steps complete, we can then make
> src/gatt not use GAttrib at all and implement a new ATT I/O mechanism
> using "struct bt_att?.
I am pretty open on how many steps we want to take at a time or what we want to do later. What I know for sure is that I want to rid of GIOChannel and BtIO usage and GLib.
Regards
Marcel
Hi Marcel,
> at the moment ConnectProfile() is BR/EDR specific and it most likely will stay way. So for LE only devices it
> should just return an error at the moment.
This simplifies things quite a bit.
> We want to use Service Changed notification and do a selective re-discover to avoid the extra overhead that a
> full discovery might cause. It does not have to be done in the beginning, but that is where this should be
> heading towards.
That makes sense, and this is what I had in mind as well.
> Some time back I started look into writing src/shared/att.c that really is suppose to be dead simple and just
> treats ATT protocol as a transport. This is how I see that basic interaction.
>
> struct bt_att;
>
> struct bt_att *bt_att_new(int fd);
>
> struct bt_att *att_ref(struct bt_att *att);
> void bt_att_unref(struct bt_att *att);
>
> bool bt_att_set_debug(struct bt_att *att, bt_att_debug_func_t callback,
> void *user_data, bt_att_destroy_func_t destroy);
>
> bool bt_att_set_close_on_unref(struct bt_att *att, bool do_close);
>
> For a lot of new code, I prefer if we are using file descriptors as base. Then it becomes easy to use on a
> socketpair for unit testing or put this onto a different transport.
>
> The GIOChannel and BtIO forces us to have knowledge about every single detail of transport. I think we did
> this the wrong way around. The actual IO handling should be done inside struct att and be an implementation
> detail. And it should just deal with the transport and not the setup of the transport. The setup is out of scope
> for the protocol handling.
That makes sense. So in the end we would have src/device explicitly
set up the l2cap socket to the remote device and create a struct
bt_att that it owns and all the profiles would add a ref to that
struct bt_att to perform I/O?
It seems to me that we ultimately want an API that is just like
attrib/gatt.h that uses "struct bt_att" instead of GAttrib. There is
already a lot of code in attrib/* that is useful for dealing with the
attribute protocol and I don't really see the point of throwing that
code away, especially attrib/att.h can be used (at least initially) by
the new src/shared/att to encode/decode ATT MTUs.
As for the higher-level GATT interactions: we are clearly moving
towards a btd_attribute based GATT client/server API. So it makes
sense to approach this from the top-down. Here is what I propose for a
step-by-step refactor:
1. Add src/shared/att.*. Define struct bt_att as a simple wrapper
around the file descriptor with basic reference counting logic similar
to src/shared/mgmt (i.e. bt_att_set_close_on_unref, etc.) only.
2. Define a new GATT API for remote attribute discovery in src/gatt.*.
Have the functions take in a struct btd_att pointer. Have all
attributes represented as btd_attribute. Perhaps get this code tested
by introducing a new gatttool.
3. Implement the GATT discovery functions by using GAttrib inside
src/gatt.c at first, calling into the functions in attrib/gatt.h.
4. Partially remove the GAttrib dependency in src/device by having it
call into the new discovery functions in src/gatt instead of calling
attrib/gatt functions directly. At this point src/device will probably
still create the connection using bt_io_connect but it will wrap the
underlying fd around a "struct bt_att" instead of a GAttrib when
making calls to src/gatt.
Here we will remove the call to attrib_from_device (defined in
src/attrib-server.h) from src/device.c. I don't know if this will
break anything big, but it seems to be how incoming connections are
currently being handled. I think this logic has been pretty much taken
over by the new GATT server code in src/gatt.*, but correct me if I'm
wrong.
To prevent breaking the profiles that use attio and GAttrib,
src/device will keep notifying registered attio callbacks in this
step. This means that it will probably create a GAttrib simply for the
purpose of notifying the profiles but it won't use it to call into
attrib/gatt functions.
5. Have the profiles call into the new functions in src/gatt for
attribute requests instead of using attrib/gatt. This is the step
where we remove GAttrib from the profiles and modify attio to take in
a "struct bt_att".
6. Remove GAttrib entirely from src/device.
7. Remove GIOChannel from src/device by removing calls to
bt_io_connect and have it create the raw fd directly.
Sorry for the long email. Let me know if any of the above doesn't make
sense or if there is a much better/simpler way to approach this. There
might also be some things that I'm missing, for example there might be
complexities involving pairing and bonding steps and incoming
connections. I'm guessing that once step 5 is done we can add the GATT
D-Bus client API code. With all 7 steps complete, we can then make
src/gatt not use GAttrib at all and implement a new ATT I/O mechanism
using "struct bt_att".
Thanks!
Arman
Hi Arman,
>> Remove GAttrib completely from the source will not be easy. You could
>> try to restrict the usage of GAttrib to src/gatt.c
>> Move discovery to src/gatt.c introduces another problem: probe
>> profiles/services, reply properly for Pair() and ConnectProfiles(),
>> helpers to access attributes of a given service, ...
>
> My understanding of the ConnectProfile() D-Bus method was that it
> pretty much only applies to BR/EDR. Even with GATT over BR/EDR I don't
> know if this method makes sense as connecting to a specific remote
> profile is not something you would do in LE. You would instead read
> several GATT services and interact with all of them based on the
> profile you're implementing. Maybe we can add D-Bus methods to
> interact with specific GATT service UUIDs in the future but I don't
> know if ConnectProfile fits into the overall BLE scheme, though please
> correct me if I'm wrong.
at the moment ConnectProfile() is BR/EDR specific and it most likely will stay way. So for LE only devices it should just return an error at the moment.
> As for the reply to pairing, I don't think much would change. The only
> reason for moving discovery to src/gatt.c is to restrict usage of
> GAttrib to src/gatt.c. In effect, what we would do is add a function,
> e.g. btd_gatt_discover_primary, which would be, from the perspective
> of src/device.c, a thin wrapper around gatt_discover_primary. Instead
> of directly storing a list of gatt_primary, btd_device would now store
> a list of btd_attribute (or handle) of its primary services. Initially
> we would leave probing the way it is but we would have the profiles
> use new functions in src/gatt to access characteristics and
> descriptors, instead of using GAttrib directly.
>
> As for the auto-connect case, whether or not we want to re-discover
> all GATT services is something that I'm not sure about. We can always
> think about this later, though.
We want to use Service Changed notification and do a selective re-discover to avoid the extra overhead that a full discovery might cause. It does not have to be done in the beginning, but that is where this should be heading towards.
>> Probably, we will not be able to remote ATTIO immediately. ATTIO
>> abstraction will be replaced by mgmt commands proposed by "[RFC] doc:
>> Connection Parameters Command". Keep re-connections working with old
>> kernels will be tricky.
>
> Probably not. We can, however, try to at least remove the GAttrib
> dependency from attio. With new discovery accessors in src/gatt, the
> GAttrib pointer in attio_connect_cb would become unnecessary.
>
>> My suggestion is start with small cleanups & improvements, as you
>> probably noticed the code related to BLE & GATT is not easy to
>> understand.
>>
>> My understanding was GIOChannel usage is not encouraged anymore, we
>> should use "struct io" instead. So, replace GIOChannel by struct io
>> could be a good starting point.
>> (please confirm with Johan)
>
> Afaict the primary piece of code concerning attribute discovery that
> uses GIOChannel consists of the bt_io functions in btio/btio.*. If
> Johan agrees, we could start by replacing GIOChannel with struct io in
> btio/btio. Once we do this, we would make GAttrib use struct io
> instead of GIOChannel as the initial step of the refactor. Let me know
> if this is a good approach.
When we are talking about ATT and struct io, I get the feeling we should not even involve btio at all. Especially for the LE only version we are running on L2CAP fixed channel.
The attribute protocol should just run over a file descriptor. Whatever this file descriptor might be. Then the ATT handling needs to wrap this file descriptor into struct io. Similar to how I have done this for src/shared/mgmt.c.
Some time back I started look into writing src/shared/att.c that really is suppose to be dead simple and just treats ATT protocol as a transport. This is how I see that basic interaction.
struct bt_att;
struct bt_att *bt_att_new(int fd);
struct bt_att *att_ref(struct bt_att *att);
void bt_att_unref(struct bt_att *att);
bool bt_att_set_debug(struct bt_att *att, bt_att_debug_func_t callback,
void *user_data, bt_att_destroy_func_t destroy);
bool bt_att_set_close_on_unref(struct bt_att *att, bool do_close);
For a lot of new code, I prefer if we are using file descriptors as base. Then it becomes easy to use on a socketpair for unit testing or put this onto a different transport.
The GIOChannel and BtIO forces us to have knowledge about every single detail of transport. I think we did this the wrong way around. The actual IO handling should be done inside struct att and be an implementation detail. And it should just deal with the transport and not the setup of the transport. The setup is out of scope for the protocol handling.
And I have been looking into similar things for src/shared/sdp.c along the same lines.
struct bt_sdp_server;
struct bt_sdp_server *bt_sdp_server_new(int fd, uint16_t mtu,
struct bt_sdp_db *db);
struct bt_sdp_server *bt_sdp_server_ref(struct bt_sdp_server *server);
void bt_sdp_server_unref(struct bt_sdp_server *server);
bool bt_sdp_server_set_debug(struct bt_sdp_server *server,
bt_sdp_debug_func_t callback,
void *user_data, bt_sdp_destroy_func_t destroy);
bool bt_sdp_server_set_search_attribute_support(struct bt_sdp_server *server,
bool support);
bool bt_sdp_server_set_close_on_unref(struct bt_sdp_server *server,
bool do_close);
I think you get the idea.
Regards
Marcel
Hi Claudio,
> Remove GAttrib completely from the source will not be easy. You could
> try to restrict the usage of GAttrib to src/gatt.c
> Move discovery to src/gatt.c introduces another problem: probe
> profiles/services, reply properly for Pair() and ConnectProfiles(),
> helpers to access attributes of a given service, ...
My understanding of the ConnectProfile() D-Bus method was that it
pretty much only applies to BR/EDR. Even with GATT over BR/EDR I don't
know if this method makes sense as connecting to a specific remote
profile is not something you would do in LE. You would instead read
several GATT services and interact with all of them based on the
profile you're implementing. Maybe we can add D-Bus methods to
interact with specific GATT service UUIDs in the future but I don't
know if ConnectProfile fits into the overall BLE scheme, though please
correct me if I'm wrong.
As for the reply to pairing, I don't think much would change. The only
reason for moving discovery to src/gatt.c is to restrict usage of
GAttrib to src/gatt.c. In effect, what we would do is add a function,
e.g. btd_gatt_discover_primary, which would be, from the perspective
of src/device.c, a thin wrapper around gatt_discover_primary. Instead
of directly storing a list of gatt_primary, btd_device would now store
a list of btd_attribute (or handle) of its primary services. Initially
we would leave probing the way it is but we would have the profiles
use new functions in src/gatt to access characteristics and
descriptors, instead of using GAttrib directly.
As for the auto-connect case, whether or not we want to re-discover
all GATT services is something that I'm not sure about. We can always
think about this later, though.
> Probably, we will not be able to remote ATTIO immediately. ATTIO
> abstraction will be replaced by mgmt commands proposed by "[RFC] doc:
> Connection Parameters Command". Keep re-connections working with old
> kernels will be tricky.
Probably not. We can, however, try to at least remove the GAttrib
dependency from attio. With new discovery accessors in src/gatt, the
GAttrib pointer in attio_connect_cb would become unnecessary.
> My suggestion is start with small cleanups & improvements, as you
> probably noticed the code related to BLE & GATT is not easy to
> understand.
>
> My understanding was GIOChannel usage is not encouraged anymore, we
> should use "struct io" instead. So, replace GIOChannel by struct io
> could be a good starting point.
> (please confirm with Johan)
Afaict the primary piece of code concerning attribute discovery that
uses GIOChannel consists of the bt_io functions in btio/btio.*. If
Johan agrees, we could start by replacing GIOChannel with struct io in
btio/btio. Once we do this, we would make GAttrib use struct io
instead of GIOChannel as the initial step of the refactor. Let me know
if this is a good approach.
-Arman
Hi Arman:
On Thu, Mar 20, 2014 at 7:13 PM, Arman Uguray <[email protected]> wrote:
> Hi all,
>
> I've been working on adding the client-side GATT API to BlueZ
> src/gatt. It occurred to me (both as I delved into the code and after
> discussions with Claudio Takahasi) that combining the "struct io"
> based abstraction used by the new btd_attribute and the current
> connection management that depends on GAttrib gets a bit messy. For
> client mode alone, handling both incoming connections from a remote
> device, and issuing outgoing read/write and primary/characteristic
> discovery requests CAN be done with GAttrib, but the btd_attribute
> abstraction appears to be cleaner I would prefer to keep the client
> and server mode implementations consistent.
>
> To that end, I'd like to figure out what's the best way to approach
> this. Clearly, the GAttrib dependency runs deep: the API provided in
> attrib/gatt.h uses GAttrib and is used by src/device.c and pretty much
> all the GATT based profile implementations. The profiles seem to use
> the callback interface defined in src/attio.h which is how src/device
> allows the profiles to access it's internal GAttrib instance.
> attrib/gatt.h is also currently used by gatttool.
Remove GAttrib completely from the source will not be easy. You could
try to restrict the usage of GAttrib to src/gatt.c
Move discovery to src/gatt.c introduces another problem: probe
profiles/services, reply properly for Pair() and ConnectProfiles(),
helpers to access attributes of a given service, ...
Probably, we will not be able to remote ATTIO immediately. ATTIO
abstraction will be replaced by mgmt commands proposed by "[RFC] doc:
Connection Parameters Command". Keep re-connections working with old
kernels will be tricky.
>
> Anderson Lizardo and I had some discussions about this and he
> mentioned that one of the earlier ideas was to simply replace GAttrib
> with struct io. Another thing that he mentioned is to keep the code in
> src/gatt restricted to handling client and server side read/write
> requests (i.e. attribute access abstraction) and put no discovery
> related logic there.
>
> So my question is, how do we want to do this refactor? Do we basically
> want to have all references to GAttrib replaced with struct io,
> keeping the API in attrib/gatt and the callbacks in src/attio the same
> but using struct io instead of GAttrib? Or do we want a more revised
> API?
My suggestion is start with small cleanups & improvements, as you
probably noticed the code related to BLE & GATT is not easy to
understand.
My understanding was GIOChannel usage is not encouraged anymore, we
should use "struct io" instead. So, replace GIOChannel by struct io
could be a good starting point.
(please confirm with Johan)
Regards,
Claudio