From: Arman Uguray <[email protected]>
Hi everyone,
I'm working on the GATT client API and I have an initial implementation that
exposes basic GATT service and characteristic objects via D-Bus. Before I go
any further in the implementation, I wanted to get some feedback on whether or
not I've been taking the right approach so far. Also, since this is the first
time I'm contributing to BlueZ, there might be some general style/convention
issues that I would like to get corrected sooner than later :)
This set includes two patches that add support for GATT service and
characteristic objects. Characteristic descriptors, characteristic value
read/write requests, characteristic value notifications and indications,
characteristic properties (flags) and permissions, and included services are
not yet implemented. The main implementation choices that I've made that I would
especially like comments on are the following:
1. A GATT service is represented by instances of btd_gatt_dbus_service. A
btd_device directly creates and owns services. It might actually be better to
keep track of services and the devices that created them in src/gatt-dbus.c,
as the way I implemented it might make managing included services difficult.
2. btd_gatt_dbus_service objects get created in
src/device.c:register_all_services. This may not be the right place to create
them, but every time register_all_services gets called, a btd_device removes
all GATT services that it created and reconstructs them. I took this route
to keep the initial implementation simple but there might be a smarter way to
go about it.
3. GATT service hierarchies are not cached and services are created only after
we connect to a remote device.
4. A btd_device creates a btd_gatt_dbus_service object for all GATT primary
services that it has created. Each service then individually does
characteristic discovery based on it's handle range.
5. GAttrib is used for discovery. GATT services obtain the GAttrib instance
from btd_device by using its attio callbacks. I did it this way solely based
on profiles (profiles/gatt/gas.c and profiles/gatt/heartrate.c) and for all I
know this might be an absolutely terrible way of managing the ATT connection
to the remote device, so please tell me whether or not this is the right
approach.
6. The first patch introduces a change to the way device_browse_primary gets
called in device.c:att_connect_cb; the existing code doesn't do primary
service discovery unless (as far as I understand) the connect request came
over D-Bus, so an auto-connect won't discover remote GATT services, for
example. I made a change here, not because I intended to change that code
path, but because I was curious why it was done the way it is.
Arman Uguray (2):
gatt-dbus: Add remote GATT service objects.
gatt-dbus: Add remote GATT characteristic objects.
src/device.c | 47 ++++++++++-
src/gatt-dbus.c | 255 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-
src/gatt-dbus.h | 25 ++++++
3 files changed, 321 insertions(+), 6 deletions(-)
--
1.8.3.2
Hi Lizardo,
First of all, sorry about the many small style issues. I don't usually
write a lot of C, so thanks for taking the time to address my style
mistakes. Please see my answers inline.
> What does "CL" mean?
Please ignore it. It's a Google/Chromium way of referring to a patch
and I used the term out of habit. What I really meant was "patch".
> Regarding coding style: we mostly follow kernel style, so be sure that
> your patches pass the "checkpatch.pl" script (available on the kernel
> sources). You can use this call as a start (which is what I use
> personally):
>
> ~/trees/linux.git/scripts/checkpatch.pl \
> --no-signoff --ignore
> INITIALISED_STATIC,NEW_TYPEDEFS,VOLATILE,CAMELCASE,FSF_MAILING_ADDRESS
> --show-types --mailback -
>
> See below for comments on the most obvious issues, but I suggest you
> clear all errors/warnings reported by checkpatch. Also make sure you
> are building your code using "./bootstrap-configure && make" and that
> you have run "make check && make distcheck" at least once.
Thanks, I will do this from now on.
> Why are you skipping those services from The D-Bus API? Some
> application why want access to the "raw" data such as GAP Appearance.
I was trying to decide whether or not it makes sense for an external
application to interact with the GATT/GAP services directly when BlueZ
takes care of the specifics. This was something I wasn't really sure
about myself, but I included this in the patch anyway to see what you
think. I think it's ok to simply expose them for now but maybe
abstract the specifics away later in a higher-level API?
>> - if (device->connect) {
>> - if (!device->le_state.svc_resolved)
>> - device_browse_primary(device, NULL);
>> + if (!device->le_state.svc_resolved)
>> + device_browse_primary(device, NULL);
>>
>> + if (device->connect) {
>
>
> Are these changes related your last point on the cover letter? If yes,
> please move it to a separate commit with a descriptive commit message
> explaining the problem.
Yes they are. The reason that I included it in this patch is the
following: once BlueZ has connected to a device, it seems to mark it
as non-temporary and store the data. If, after connecting to a device,
I power off the device and then on again, BlueZ actually automatically
connects to it. It appears that, since BlueZ already caches the list
of service UUIDs that it fetched from the device, it doesn't run
service discovery again (afaict). This makes sense, but because of the
way I wired up GATT service object creation in this patch, GATT
service objects don't get created in the auto-connect case.
I kept this change in this patch to kind of initiate this discussion,
as I'm not quite convinced yet whether or not the way I register the
btd_gatt_dbus_service objects in device.c is the right way and I
wanted to hear you opinion on that. I'll remove this change from this
patch for now.
-Arman
Hi Lizardo,
>> I'm working on the GATT client API and I have an initial implementation that
>> exposes basic GATT service and characteristic objects via D-Bus. Before I go
>> any further in the implementation, I wanted to get some feedback on whether or
>> not I've been taking the right approach so far. Also, since this is the first
>> time I'm contributing to BlueZ, there might be some general style/convention
>> issues that I would like to get corrected sooner than later :)
>
> First of all, sorry about the delay on reviewing your patches. Claudio
> and myself have been busy on our patches related to GATT server API
> which is taking most of our time available for BlueZ tasks.
Not a problem at all.
>> 6. The first patch introduces a change to the way device_browse_primary gets
>> called in device.c:att_connect_cb; the existing code doesn't do primary
>> service discovery unless (as far as I understand) the connect request came
>> over D-Bus, so an auto-connect won't discover remote GATT services, for
>> example. I made a change here, not because I intended to change that code
>> path, but because I was curious why it was done the way it is.
>
> I will comment inline on the other two messages, but this last point
> is not clear to me. What do you mean by "auto-connect"? What are the
> exact steps to reproduce this?
So, it appears to me that if bluez finds a device and caches it (I've
connected to it once) and I power off the adapter and then back on, it
will try to connect to it automatically, unless I have explicitly
disconnected from the device. In this case bluez doesn't rediscover
the services on the device, as it already caches all the UUIDs. I will
make further comments on the patch itself.
Hi Arman,
On Wed, Mar 12, 2014 at 12:04 AM, <[email protected]> wrote:
> From: Arman Uguray <[email protected]>
>
> This CL adds remote GATT characteristic objects to the D-Bus API. The
> API only exposes the UUID; the characteristic value and permissions have
> not yet been implemented.
Same general comments about coding style apply to this patch as well.
A few other comments below.
> ---
> src/gatt-dbus.c | 165 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 164 insertions(+), 1 deletion(-)
>
> diff --git a/src/gatt-dbus.c b/src/gatt-dbus.c
> index 60b87fa..db98137 100644
> --- a/src/gatt-dbus.c
> +++ b/src/gatt-dbus.c
> @@ -52,8 +52,24 @@
>
> struct btd_gatt_dbus_service {
> bt_uuid_t uuid;
> + struct att_range range;
> + GAttrib *attrib;
> + guint attioid;
> + guint request;
> +
> struct btd_device *device;
> char *path;
> + GSList *characteristics;
> +};
> +
> +struct gatt_dbus_characteristic {
> + bt_uuid_t uuid;
> + uint8_t handle;
> + uint8_t value_handle;
I wonder why handle/value_handle are uint8_t and not uint16_t.
> + uint8_t properties;
> +
> + struct btd_gatt_dbus_service *service;
> + char *path;
> };
>
> struct external_app {
> @@ -491,19 +507,156 @@ static gboolean service_property_get_uuid(const GDBusPropertyTable *property,
> return TRUE;
> }
>
> +static gboolean characteristic_property_get_uuid(
> + const GDBusPropertyTable *table,
> + DBusMessageIter *iter, void *data)
> +{
> + char uuid[MAX_LEN_UUID_STR + 1];
> + const char *ptr = uuid;
> + struct gatt_dbus_characteristic *chr = data;
> +
> + bt_uuid_to_string(&chr->uuid, uuid, sizeof(uuid));
> + dbus_message_iter_append_basic(iter, DBUS_TYPE_STRING, &ptr);
> +
> + return TRUE;
> +}
> +
> static const GDBusPropertyTable service_properties[] = {
> { "UUID", "s", service_property_get_uuid },
> {}
> };
>
> +static const GDBusPropertyTable characteristic_properties[] = {
> + { "UUID", "s", characteristic_property_get_uuid },
> + {}
> +};
> +
> +static void unregister_characteristic(gpointer user_data)
> +{
> + struct gatt_dbus_characteristic *chr = user_data;
> + g_dbus_unregister_interface(btd_get_dbus_connection(),
> + chr->path, GATT_CHR_IFACE);
> +}
> +
> +static void attio_cleanup(struct btd_gatt_dbus_service *service)
> +{
> + if (!service->attrib)
> + return;
> + GAttrib *attrib = service->attrib;
Declaration should be in the beginning of the function.
> + service->attrib = NULL;
> + if (service->request) {
> + g_attrib_cancel(attrib, service->request);
> + service->request = 0;
> + }
> + g_attrib_unref(attrib);
> +}
> +
> static void service_free(gpointer user_data)
> {
> struct btd_gatt_dbus_service *service = user_data;
>
> + g_slist_free_full(service->characteristics, unregister_characteristic);
> +
> + if (service->attioid) {
> + btd_device_remove_attio_callback(service->device,
> + service->attioid);
> + }
As an exception to the "use kernel coding style" rule, if() blocks
with a single statement should not have brackets ({}).
> + attio_cleanup(service);
> g_free(service->path);
> g_free(service);
> }
>
> +static void characteristic_free(gpointer user_data)
> +{
> + struct gatt_dbus_characteristic *characteristic = user_data;
> +
> + g_free(characteristic->path);
> + g_free(characteristic);
> +}
> +
> +static struct gatt_dbus_characteristic *characteristic_create(
> + struct btd_gatt_dbus_service *service,
> + struct gatt_char *chr)
> +{
> + struct gatt_dbus_characteristic *characteristic;
> + bt_uuid_t uuid;
> +
> + DBG("GATT characteristic UUID: %s", chr->uuid);
> +
> + characteristic = g_try_malloc0(sizeof(struct gatt_dbus_characteristic));
We usually use g_new0() or g_try_new0() for structs.
> + if (characteristic == NULL)
> + return NULL;
> +
> + characteristic->path = g_strdup_printf("%s/char%04X", service->path,
> + chr->handle);
> + characteristic->service = service;
> + characteristic->handle = chr->handle;
> + characteristic->value_handle = chr->value_handle;
> + characteristic->properties = chr->properties;
> +
> + if (bt_string_to_uuid(&uuid, chr->uuid)) {
> + error("Characteristic has invalid UUID: %s", chr->uuid);
> + goto fail;
> + }
> +
> + bt_uuid_to_uuid128(&uuid, &characteristic->uuid);
> +
> + DBG("Creating GATT characteristic %s", characteristic->path);
> +
> + if (g_dbus_register_interface(btd_get_dbus_connection(),
> + characteristic->path,
> + GATT_CHR_IFACE, NULL, NULL,
> + characteristic_properties,
> + characteristic,
> + characteristic_free) == FALSE) {
> + error("Unable to register GATT characteristic: %s", chr->uuid);
> + goto fail;
> + }
> +
> + return characteristic;
> +
> +fail:
> + characteristic_free(characteristic);
> + return NULL;
> +}
> +
> +static void discover_char_cb(uint8_t status, GSList *chars, void *user_data)
> +{
> + struct btd_gatt_dbus_service *service = user_data;
Add an empty line after variable declaration (at least for function level ones).
> + service->request = 0;
> + if (status) {
> + error("Characteristic discovery failed.");
> + return;
> + }
> + for (; chars; chars = chars->next) {
> + struct gatt_char *chr = chars->data;
> + struct gatt_dbus_characteristic *dbus_chr =
> + characteristic_create(service, chr);
> + if (dbus_chr == NULL)
> + continue;
> + service->characteristics = g_slist_append(
> + service->characteristics, dbus_chr);
> + }
> + attio_cleanup(service);
> +}
> +
> +static void attio_connected_cb(GAttrib *attrib, gpointer user_data)
> +{
> + struct btd_gatt_dbus_service *service = user_data;
Same as above.
> + service->attrib = g_attrib_ref(attrib);
> + service->request = gatt_discover_char(service->attrib,
> + service->range.start,
> + service->range.end,
> + NULL, discover_char_cb,
> + service);
> +}
> +
> +static void attio_disconnected_cb(gpointer user_data)
> +{
> + struct btd_gatt_dbus_service *service = user_data;
> + attio_cleanup(service);
> +}
> +
> struct btd_gatt_dbus_service *btd_gatt_dbus_service_create(
> struct btd_device *device, struct gatt_primary *primary)
> {
> @@ -520,6 +673,12 @@ struct btd_gatt_dbus_service *btd_gatt_dbus_service_create(
> service->path = g_strdup_printf("%s/service%04X", device_path,
> primary->range.start);
>
> + service->device = device;
> + service->range = primary->range;
> + service->characteristics = NULL;
> + service->attrib = NULL;
> + service->request = 0;
If you use g_try_new0(), there is no need to initialize pointers to
NULL or integers to 0.
> +
> if (bt_string_to_uuid(&uuid, primary->uuid)) {
> error("Primary has invalid UUID: %s", primary->uuid);
> goto fail;
> @@ -541,7 +700,11 @@ struct btd_gatt_dbus_service *btd_gatt_dbus_service_create(
> goto fail;
> }
>
> - service->device = device;
> + service->attioid = service->attioid = btd_device_add_attio_callback(
> + service->device,
> + attio_connected_cb,
> + attio_disconnected_cb, service);
> +
> return service;
>
> fail:
Best Regards,
--
Anderson Lizardo
http://www.indt.org/?lang=en
INdT - Manaus - Brazil
Hi Arman,
On Wed, Mar 12, 2014 at 12:03 AM, <[email protected]> wrote:
> From: Arman Uguray <[email protected]>
>
> This CL adds the initial GATT client D-Bus API support for remote primary GATT
What does "CL" mean?
Regarding coding style: we mostly follow kernel style, so be sure that
your patches pass the "checkpatch.pl" script (available on the kernel
sources). You can use this call as a start (which is what I use
personally):
~/trees/linux.git/scripts/checkpatch.pl \
--no-signoff --ignore
INITIALISED_STATIC,NEW_TYPEDEFS,VOLATILE,CAMELCASE,FSF_MAILING_ADDRESS
--show-types --mailback -
See below for comments on the most obvious issues, but I suggest you
clear all errors/warnings reported by checkpatch. Also make sure you
are building your code using "./bootstrap-configure && make" and that
you have run "make check && make distcheck" at least once.
> services. Characteristics, descriptors, and included services are not yet
> handled.
> ---
> src/device.c | 47 +++++++++++++++++++++++++++--
> src/gatt-dbus.c | 92 +++++++++++++++++++++++++++++++++++++++++++++++++++++++--
> src/gatt-dbus.h | 25 ++++++++++++++++
> 3 files changed, 158 insertions(+), 6 deletions(-)
>
> diff --git a/src/device.c b/src/device.c
> index e624445..0623e4d 100644
> --- a/src/device.c
> +++ b/src/device.c
> @@ -65,6 +65,7 @@
> #include "textfile.h"
> #include "storage.h"
> #include "attrib-server.h"
> +#include "gatt.h"
>
> #define IO_CAPABILITY_NOINPUTNOOUTPUT 0x03
>
> @@ -186,6 +187,7 @@ struct btd_device {
> GSList *uuids;
> GSList *primaries; /* List of primary services */
> GSList *services; /* List of btd_service */
> + GSList *gatt_services; /* List of GATT DBus Services */
> GSList *pending; /* Pending services */
> GSList *watches; /* List of disconnect_data */
> gboolean temporary;
> @@ -511,11 +513,18 @@ static void svc_dev_remove(gpointer user_data)
> g_free(cb);
> }
>
> +static void gatt_dbus_service_free(gpointer user_data)
> +{
> + struct btd_gatt_dbus_service *service = user_data;
> + btd_gatt_dbus_service_free(service);
> +}
> +
> static void device_free(gpointer user_data)
> {
> struct btd_device *device = user_data;
>
> g_slist_free_full(device->uuids, g_free);
> + g_slist_free_full(device->gatt_services, gatt_dbus_service_free);
> g_slist_free_full(device->primaries, g_free);
> g_slist_free_full(device->attios, g_free);
> g_slist_free_full(device->attios_offline, g_free);
> @@ -2287,6 +2296,7 @@ static struct btd_device *device_new(struct btd_adapter *adapter,
>
> str2ba(address, &device->bdaddr);
> device->adapter = adapter;
> + device->gatt_services = NULL;
You do not need to initialize with NULL because device is allocated
with g_try_malloc0() (which zeroes memory).
>
> return btd_device_ref(device);
> }
> @@ -3317,6 +3327,35 @@ done:
> return FALSE;
> }
>
> +static void expose_btd_dbus_gatt_services(struct btd_device *device)
> +{
> + GSList *l;
> +
> + /* Clear current list of GATT services. */
> + g_slist_free_full(device->gatt_services, gatt_dbus_service_free);
> + device->gatt_services = NULL;
Coding style issue: add an empty line here.
> + for (l = device->primaries; l; l = g_slist_next(l)) {
> + struct gatt_primary *prim = l->data;
> +
> + /* Don't create objects for the GAP and GATT services. */
> + if (bt_uuid_strcmp(prim->uuid, GATT_UUID) == 0) {
Current style is to use "if (!bt_uuid_strcmp(...))" for new code.
> + DBG("Skipping GATT UUID for GATT service objects.");
> + continue;
> + }
> + if (bt_uuid_strcmp(prim->uuid, GAP_UUID) == 0) {
Same as above.
> + DBG("Skipping GAP UUID for GATT service objects.");
> + continue;
> + }
Why are you skipping those services from The D-Bus API? Some
application why want access to the "raw" data such as GAP Appearance.
> +
> + struct btd_gatt_dbus_service *service;
Declarations should come at the beginning of the code block (e.g. function).
> + service = btd_gatt_dbus_service_create(device, prim);
> + if (service == NULL)
> + continue;
Better print an error() above before continue?
> + device->gatt_services = g_slist_append(device->gatt_services,
> + service);
> + }
> +}
> +
> static void register_all_services(struct browse_req *req, GSList *services)
> {
> struct btd_device *device = req->device;
> @@ -3329,6 +3368,8 @@ static void register_all_services(struct browse_req *req, GSList *services)
>
> device_register_primaries(device, g_slist_copy(services), -1);
>
> + expose_btd_dbus_gatt_services(device);
> +
> device_probe_profiles(device, req->profiles_added);
>
> if (device->attios == NULL && device->attios_offline == NULL)
> @@ -3501,10 +3542,10 @@ done:
> bonding_request_free(device->bonding);
> }
>
> - if (device->connect) {
> - if (!device->le_state.svc_resolved)
> - device_browse_primary(device, NULL);
> + if (!device->le_state.svc_resolved)
> + device_browse_primary(device, NULL);
>
> + if (device->connect) {
Are these changes related your last point on the cover letter? If yes,
please move it to a separate commit with a descriptive commit message
explaining the problem.
> if (err < 0)
> reply = btd_error_failed(device->connect,
> strerror(-err));
> [...]
> diff --git a/src/gatt-dbus.h b/src/gatt-dbus.h
> index 310cfa9..08d3f11 100644
> --- a/src/gatt-dbus.h
> +++ b/src/gatt-dbus.h
> @@ -21,5 +21,30 @@
> *
> */
>
> +struct btd_gatt_dbus_service;
> +struct btd_device;
> +struct gatt_primary;
Usually declarations like the above, unless the symbols are local to
the C file. What you need to do is make sure the files #including
gatt-dbus.h also #include "src/device.h" before.
> +
> gboolean gatt_dbus_manager_register(void);
> void gatt_dbus_manager_unregister(void);
> +
> +/*
> + * btd_gatt_dbus_service_create - Create a GATT service that represents a remote
To keep consistency with nomenclature used on the gatt-api.txt, I
would call these:
btd_gatt_dbus_service_register()
btd_gatt_dbus_service_unregister()
> + * primary GATT service and expose it via D-Bus.
> + *
> + * @device: The remote device that hosts the GATT service.
> + * @primary: The primary GATT service.
> + *
> + * Returns a reference to the GATT service object. In case of error, NULL is
> + * returned.
> + */
> +struct btd_gatt_dbus_service *btd_gatt_dbus_service_create(
> + struct btd_device *device, struct gatt_primary *primary);
> +
> +/*
> + * btd_gatt_dbus_service_free - Unregister a GATT service as a D-Bus object and
> + * free up its memory.
> + *
> + * @service: The GATT service object to remove.
> + */
> +void btd_gatt_dbus_service_free(struct btd_gatt_dbus_service* service);
Also a general comment regarding indentation:
BlueZ has a specific (but simple to remember) indentation rule: if a
line goes over 80 columns, break it into multiple lines, such as:
- line continuations of function calls and declarations are after the
opening "(" of the first line.
- line continuations are aligned to the left, up to the 80 column limit
- line continuations are aligned to right between themselves.
You can break this rule to keep readability (e.g. when function name
is too long), but I recommend trying to stick to it when possible. If
in doubt, take a look at recent files on BlueZ source for examples,
e.g. src/shared/btsnoop.h.
Best Regards,
--
Anderson Lizardo
http://www.indt.org/?lang=en
INdT - Manaus - Brazil
Hi Arman,
On Wed, Mar 12, 2014 at 12:03 AM, <[email protected]> wrote:
> From: Arman Uguray <[email protected]>
>
> Hi everyone,
>
> I'm working on the GATT client API and I have an initial implementation that
> exposes basic GATT service and characteristic objects via D-Bus. Before I go
> any further in the implementation, I wanted to get some feedback on whether or
> not I've been taking the right approach so far. Also, since this is the first
> time I'm contributing to BlueZ, there might be some general style/convention
> issues that I would like to get corrected sooner than later :)
First of all, sorry about the delay on reviewing your patches. Claudio
and myself have been busy on our patches related to GATT server API
which is taking most of our time available for BlueZ tasks.
> 6. The first patch introduces a change to the way device_browse_primary gets
> called in device.c:att_connect_cb; the existing code doesn't do primary
> service discovery unless (as far as I understand) the connect request came
> over D-Bus, so an auto-connect won't discover remote GATT services, for
> example. I made a change here, not because I intended to change that code
> path, but because I was curious why it was done the way it is.
I will comment inline on the other two messages, but this last point
is not clear to me. What do you mean by "auto-connect"? What are the
exact steps to reproduce this?
Best Regards,
--
Anderson Lizardo
http://www.indt.org/?lang=en
INdT - Manaus - Brazil
From: Arman Uguray <[email protected]>
This CL adds remote GATT characteristic objects to the D-Bus API. The
API only exposes the UUID; the characteristic value and permissions have
not yet been implemented.
---
src/gatt-dbus.c | 165 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 164 insertions(+), 1 deletion(-)
diff --git a/src/gatt-dbus.c b/src/gatt-dbus.c
index 60b87fa..db98137 100644
--- a/src/gatt-dbus.c
+++ b/src/gatt-dbus.c
@@ -52,8 +52,24 @@
struct btd_gatt_dbus_service {
bt_uuid_t uuid;
+ struct att_range range;
+ GAttrib *attrib;
+ guint attioid;
+ guint request;
+
struct btd_device *device;
char *path;
+ GSList *characteristics;
+};
+
+struct gatt_dbus_characteristic {
+ bt_uuid_t uuid;
+ uint8_t handle;
+ uint8_t value_handle;
+ uint8_t properties;
+
+ struct btd_gatt_dbus_service *service;
+ char *path;
};
struct external_app {
@@ -491,19 +507,156 @@ static gboolean service_property_get_uuid(const GDBusPropertyTable *property,
return TRUE;
}
+static gboolean characteristic_property_get_uuid(
+ const GDBusPropertyTable *table,
+ DBusMessageIter *iter, void *data)
+{
+ char uuid[MAX_LEN_UUID_STR + 1];
+ const char *ptr = uuid;
+ struct gatt_dbus_characteristic *chr = data;
+
+ bt_uuid_to_string(&chr->uuid, uuid, sizeof(uuid));
+ dbus_message_iter_append_basic(iter, DBUS_TYPE_STRING, &ptr);
+
+ return TRUE;
+}
+
static const GDBusPropertyTable service_properties[] = {
{ "UUID", "s", service_property_get_uuid },
{}
};
+static const GDBusPropertyTable characteristic_properties[] = {
+ { "UUID", "s", characteristic_property_get_uuid },
+ {}
+};
+
+static void unregister_characteristic(gpointer user_data)
+{
+ struct gatt_dbus_characteristic *chr = user_data;
+ g_dbus_unregister_interface(btd_get_dbus_connection(),
+ chr->path, GATT_CHR_IFACE);
+}
+
+static void attio_cleanup(struct btd_gatt_dbus_service *service)
+{
+ if (!service->attrib)
+ return;
+ GAttrib *attrib = service->attrib;
+ service->attrib = NULL;
+ if (service->request) {
+ g_attrib_cancel(attrib, service->request);
+ service->request = 0;
+ }
+ g_attrib_unref(attrib);
+}
+
static void service_free(gpointer user_data)
{
struct btd_gatt_dbus_service *service = user_data;
+ g_slist_free_full(service->characteristics, unregister_characteristic);
+
+ if (service->attioid) {
+ btd_device_remove_attio_callback(service->device,
+ service->attioid);
+ }
+ attio_cleanup(service);
g_free(service->path);
g_free(service);
}
+static void characteristic_free(gpointer user_data)
+{
+ struct gatt_dbus_characteristic *characteristic = user_data;
+
+ g_free(characteristic->path);
+ g_free(characteristic);
+}
+
+static struct gatt_dbus_characteristic *characteristic_create(
+ struct btd_gatt_dbus_service *service,
+ struct gatt_char *chr)
+{
+ struct gatt_dbus_characteristic *characteristic;
+ bt_uuid_t uuid;
+
+ DBG("GATT characteristic UUID: %s", chr->uuid);
+
+ characteristic = g_try_malloc0(sizeof(struct gatt_dbus_characteristic));
+ if (characteristic == NULL)
+ return NULL;
+
+ characteristic->path = g_strdup_printf("%s/char%04X", service->path,
+ chr->handle);
+ characteristic->service = service;
+ characteristic->handle = chr->handle;
+ characteristic->value_handle = chr->value_handle;
+ characteristic->properties = chr->properties;
+
+ if (bt_string_to_uuid(&uuid, chr->uuid)) {
+ error("Characteristic has invalid UUID: %s", chr->uuid);
+ goto fail;
+ }
+
+ bt_uuid_to_uuid128(&uuid, &characteristic->uuid);
+
+ DBG("Creating GATT characteristic %s", characteristic->path);
+
+ if (g_dbus_register_interface(btd_get_dbus_connection(),
+ characteristic->path,
+ GATT_CHR_IFACE, NULL, NULL,
+ characteristic_properties,
+ characteristic,
+ characteristic_free) == FALSE) {
+ error("Unable to register GATT characteristic: %s", chr->uuid);
+ goto fail;
+ }
+
+ return characteristic;
+
+fail:
+ characteristic_free(characteristic);
+ return NULL;
+}
+
+static void discover_char_cb(uint8_t status, GSList *chars, void *user_data)
+{
+ struct btd_gatt_dbus_service *service = user_data;
+ service->request = 0;
+ if (status) {
+ error("Characteristic discovery failed.");
+ return;
+ }
+ for (; chars; chars = chars->next) {
+ struct gatt_char *chr = chars->data;
+ struct gatt_dbus_characteristic *dbus_chr =
+ characteristic_create(service, chr);
+ if (dbus_chr == NULL)
+ continue;
+ service->characteristics = g_slist_append(
+ service->characteristics, dbus_chr);
+ }
+ attio_cleanup(service);
+}
+
+static void attio_connected_cb(GAttrib *attrib, gpointer user_data)
+{
+ struct btd_gatt_dbus_service *service = user_data;
+ service->attrib = g_attrib_ref(attrib);
+ service->request = gatt_discover_char(service->attrib,
+ service->range.start,
+ service->range.end,
+ NULL, discover_char_cb,
+ service);
+}
+
+static void attio_disconnected_cb(gpointer user_data)
+{
+ struct btd_gatt_dbus_service *service = user_data;
+ attio_cleanup(service);
+}
+
struct btd_gatt_dbus_service *btd_gatt_dbus_service_create(
struct btd_device *device, struct gatt_primary *primary)
{
@@ -520,6 +673,12 @@ struct btd_gatt_dbus_service *btd_gatt_dbus_service_create(
service->path = g_strdup_printf("%s/service%04X", device_path,
primary->range.start);
+ service->device = device;
+ service->range = primary->range;
+ service->characteristics = NULL;
+ service->attrib = NULL;
+ service->request = 0;
+
if (bt_string_to_uuid(&uuid, primary->uuid)) {
error("Primary has invalid UUID: %s", primary->uuid);
goto fail;
@@ -541,7 +700,11 @@ struct btd_gatt_dbus_service *btd_gatt_dbus_service_create(
goto fail;
}
- service->device = device;
+ service->attioid = service->attioid = btd_device_add_attio_callback(
+ service->device,
+ attio_connected_cb,
+ attio_disconnected_cb, service);
+
return service;
fail:
--
1.8.3.2
From: Arman Uguray <[email protected]>
This CL adds the initial GATT client D-Bus API support for remote primary GATT
services. Characteristics, descriptors, and included services are not yet
handled.
---
src/device.c | 47 +++++++++++++++++++++++++++--
src/gatt-dbus.c | 92 +++++++++++++++++++++++++++++++++++++++++++++++++++++++--
src/gatt-dbus.h | 25 ++++++++++++++++
3 files changed, 158 insertions(+), 6 deletions(-)
diff --git a/src/device.c b/src/device.c
index e624445..0623e4d 100644
--- a/src/device.c
+++ b/src/device.c
@@ -65,6 +65,7 @@
#include "textfile.h"
#include "storage.h"
#include "attrib-server.h"
+#include "gatt.h"
#define IO_CAPABILITY_NOINPUTNOOUTPUT 0x03
@@ -186,6 +187,7 @@ struct btd_device {
GSList *uuids;
GSList *primaries; /* List of primary services */
GSList *services; /* List of btd_service */
+ GSList *gatt_services; /* List of GATT DBus Services */
GSList *pending; /* Pending services */
GSList *watches; /* List of disconnect_data */
gboolean temporary;
@@ -511,11 +513,18 @@ static void svc_dev_remove(gpointer user_data)
g_free(cb);
}
+static void gatt_dbus_service_free(gpointer user_data)
+{
+ struct btd_gatt_dbus_service *service = user_data;
+ btd_gatt_dbus_service_free(service);
+}
+
static void device_free(gpointer user_data)
{
struct btd_device *device = user_data;
g_slist_free_full(device->uuids, g_free);
+ g_slist_free_full(device->gatt_services, gatt_dbus_service_free);
g_slist_free_full(device->primaries, g_free);
g_slist_free_full(device->attios, g_free);
g_slist_free_full(device->attios_offline, g_free);
@@ -2287,6 +2296,7 @@ static struct btd_device *device_new(struct btd_adapter *adapter,
str2ba(address, &device->bdaddr);
device->adapter = adapter;
+ device->gatt_services = NULL;
return btd_device_ref(device);
}
@@ -3317,6 +3327,35 @@ done:
return FALSE;
}
+static void expose_btd_dbus_gatt_services(struct btd_device *device)
+{
+ GSList *l;
+
+ /* Clear current list of GATT services. */
+ g_slist_free_full(device->gatt_services, gatt_dbus_service_free);
+ device->gatt_services = NULL;
+ for (l = device->primaries; l; l = g_slist_next(l)) {
+ struct gatt_primary *prim = l->data;
+
+ /* Don't create objects for the GAP and GATT services. */
+ if (bt_uuid_strcmp(prim->uuid, GATT_UUID) == 0) {
+ DBG("Skipping GATT UUID for GATT service objects.");
+ continue;
+ }
+ if (bt_uuid_strcmp(prim->uuid, GAP_UUID) == 0) {
+ DBG("Skipping GAP UUID for GATT service objects.");
+ continue;
+ }
+
+ struct btd_gatt_dbus_service *service;
+ service = btd_gatt_dbus_service_create(device, prim);
+ if (service == NULL)
+ continue;
+ device->gatt_services = g_slist_append(device->gatt_services,
+ service);
+ }
+}
+
static void register_all_services(struct browse_req *req, GSList *services)
{
struct btd_device *device = req->device;
@@ -3329,6 +3368,8 @@ static void register_all_services(struct browse_req *req, GSList *services)
device_register_primaries(device, g_slist_copy(services), -1);
+ expose_btd_dbus_gatt_services(device);
+
device_probe_profiles(device, req->profiles_added);
if (device->attios == NULL && device->attios_offline == NULL)
@@ -3501,10 +3542,10 @@ done:
bonding_request_free(device->bonding);
}
- if (device->connect) {
- if (!device->le_state.svc_resolved)
- device_browse_primary(device, NULL);
+ if (!device->le_state.svc_resolved)
+ device_browse_primary(device, NULL);
+ if (device->connect) {
if (err < 0)
reply = btd_error_failed(device->connect,
strerror(-err));
diff --git a/src/gatt-dbus.c b/src/gatt-dbus.c
index c5f1597..60b87fa 100644
--- a/src/gatt-dbus.c
+++ b/src/gatt-dbus.c
@@ -37,6 +37,9 @@
#include "lib/uuid.h"
#include "dbus-common.h"
#include "log.h"
+#include "attrib/att.h"
+#include "attrib/gattrib.h"
+#include "attrib/gatt.h"
#include "error.h"
#include "gatt.h"
@@ -47,6 +50,12 @@
#define GATT_CHR_IFACE "org.bluez.GattCharacteristic1"
#define GATT_DESCRIPTOR_IFACE "org.bluez.GattDescriptor1"
+struct btd_gatt_dbus_service {
+ bt_uuid_t uuid;
+ struct btd_device *device;
+ char *path;
+};
+
struct external_app {
char *owner;
char *path;
@@ -436,7 +445,7 @@ static DBusMessage *unregister_service(DBusConnection *conn,
return dbus_message_new_method_return(msg);
}
-static const GDBusMethodTable methods[] = {
+static const GDBusMethodTable manager_methods[] = {
{ GDBUS_EXPERIMENTAL_ASYNC_METHOD("RegisterService",
GDBUS_ARGS({ "service", "o"},
{ "options", "a{sv}"}),
@@ -450,8 +459,8 @@ static const GDBusMethodTable methods[] = {
gboolean gatt_dbus_manager_register(void)
{
if (g_dbus_register_interface(btd_get_dbus_connection(),
- "/org/bluez", GATT_MGR_IFACE,
- methods, NULL, NULL, NULL, NULL) == FALSE)
+ "/org/bluez", GATT_MGR_IFACE, manager_methods,
+ NULL, NULL, NULL, NULL) == FALSE)
return FALSE;
proxy_hash = g_hash_table_new_full(g_direct_hash, g_direct_equal,
@@ -468,3 +477,80 @@ void gatt_dbus_manager_unregister(void)
g_dbus_unregister_interface(btd_get_dbus_connection(), "/org/bluez",
GATT_MGR_IFACE);
}
+
+static gboolean service_property_get_uuid(const GDBusPropertyTable *property,
+ DBusMessageIter *iter, void *data)
+{
+ char uuid[MAX_LEN_UUID_STR + 1];
+ const char *ptr = uuid;
+ struct btd_gatt_dbus_service *service = data;
+
+ bt_uuid_to_string(&service->uuid, uuid, sizeof(uuid));
+ dbus_message_iter_append_basic(iter, DBUS_TYPE_STRING, &ptr);
+
+ return TRUE;
+}
+
+static const GDBusPropertyTable service_properties[] = {
+ { "UUID", "s", service_property_get_uuid },
+ {}
+};
+
+static void service_free(gpointer user_data)
+{
+ struct btd_gatt_dbus_service *service = user_data;
+
+ g_free(service->path);
+ g_free(service);
+}
+
+struct btd_gatt_dbus_service *btd_gatt_dbus_service_create(
+ struct btd_device *device, struct gatt_primary *primary)
+{
+ struct btd_gatt_dbus_service *service;
+ bt_uuid_t uuid;
+ const char *device_path = device_get_path(device);
+
+ DBG("GATT service UUID: %s", primary->uuid);
+
+ service = g_try_malloc0(sizeof(struct btd_gatt_dbus_service));
+ if (service == NULL)
+ return NULL;
+
+ service->path = g_strdup_printf("%s/service%04X", device_path,
+ primary->range.start);
+
+ if (bt_string_to_uuid(&uuid, primary->uuid)) {
+ error("Primary has invalid UUID: %s", primary->uuid);
+ goto fail;
+ }
+
+ bt_uuid_to_uuid128(&uuid, &service->uuid);
+
+ DBG("Creating GATT service %s", service->path);
+
+ if (g_dbus_register_interface(btd_get_dbus_connection(),
+ service->path, GATT_SERVICE_IFACE,
+ NULL, NULL, service_properties,
+ service,
+ service_free) == FALSE) {
+ char device_addr[18];
+ ba2str(device_get_address(device), device_addr);
+ error("Unable to register GATT service: UUID: %s, device: %s",
+ primary->uuid, device_addr);
+ goto fail;
+ }
+
+ service->device = device;
+ return service;
+
+fail:
+ service_free(service);
+ return NULL;
+}
+
+void btd_gatt_dbus_service_free(struct btd_gatt_dbus_service* service)
+{
+ g_dbus_unregister_interface(btd_get_dbus_connection(),
+ service->path, GATT_SERVICE_IFACE);
+}
diff --git a/src/gatt-dbus.h b/src/gatt-dbus.h
index 310cfa9..08d3f11 100644
--- a/src/gatt-dbus.h
+++ b/src/gatt-dbus.h
@@ -21,5 +21,30 @@
*
*/
+struct btd_gatt_dbus_service;
+struct btd_device;
+struct gatt_primary;
+
gboolean gatt_dbus_manager_register(void);
void gatt_dbus_manager_unregister(void);
+
+/*
+ * btd_gatt_dbus_service_create - Create a GATT service that represents a remote
+ * primary GATT service and expose it via D-Bus.
+ *
+ * @device: The remote device that hosts the GATT service.
+ * @primary: The primary GATT service.
+ *
+ * Returns a reference to the GATT service object. In case of error, NULL is
+ * returned.
+ */
+struct btd_gatt_dbus_service *btd_gatt_dbus_service_create(
+ struct btd_device *device, struct gatt_primary *primary);
+
+/*
+ * btd_gatt_dbus_service_free - Unregister a GATT service as a D-Bus object and
+ * free up its memory.
+ *
+ * @service: The GATT service object to remove.
+ */
+void btd_gatt_dbus_service_free(struct btd_gatt_dbus_service* service);
--
1.8.3.2