[RFC BlueZ 0/1] Introduce bt_gatt.

[RFC BlueZ 0/1] Introduce bt_gatt.

[Arman Uguray]

bt_gatt implements GATT client operations and will be backed by bt_att. It is
effectively a rewrite of attrib/gatt, where it provides a more high-level
abstraction over GATT operation results (instead of returning raw PDUs) so that
ATT protocol level encoding/decoding is taken care of by the internal bt_att
instance. This is also meant to be a more thorough implementation of GATT
procedures, however some of them are omitted (for now):

  - "Read Using Characteristic UUID"
  - "Read Multiple Characteristic Values"
  - "Signed Write Without Response"
  - "Characteristic Value Reliable Writes"

These operations can be easily added in the future as needed, since bt_att will
provide all possible ATT protocol interactions.

bt_gatt doesn't implement attribute caching and is not a complete client
implementation on its own. It will be used together with gatt_db, where a higher
level abstraction will be responsible for managing lookups from the database vs
over-the-air and updating the database as needed. bt_gatt only provides the
hooks for performing all the necessary over-the-air GATT procedures.

Comments welcome.

Arman Uguray (1):
  src/shared/gatt: Introduce bt_gatt skeleton.

 Makefile.am       |   3 +-
 src/shared/gatt.c | 192 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 src/shared/gatt.h | 189 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 383 insertions(+), 1 deletion(-)
 create mode 100644 src/shared/gatt.c
 create mode 100644 src/shared/gatt.h

-- 
1.8.3.2

[RFC BlueZ 1/1] src/shared/gatt: Introduce bt_gatt skeleton.

[Arman Uguray]

This patch introduces src/shared/gatt, which provides an interface for
GATT client operations. bt_gatt will internally use src/shared/att for
ATT protocol client-side operations. bt_gatt is meant to be used
together with src/shared/gatt-db to implement a complete GATT client.
---
 Makefile.am       |   3 +-
 src/shared/gatt.c | 192 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 src/shared/gatt.h | 189 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 383 insertions(+), 1 deletion(-)
 create mode 100644 src/shared/gatt.c
 create mode 100644 src/shared/gatt.h

diff --git a/Makefile.am b/Makefile.am
index 04be6c4..8b731f0 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -156,7 +156,8 @@ src_bluetoothd_SOURCES = $(builtin_sources) \
 			src/shared/queue.h src/shared/queue.c \
 			src/shared/util.h src/shared/util.c \
 			src/shared/mgmt.h src/shared/mgmt.c \
-			src/shared/att.h src/shared/att.c
+			src/shared/att.h src/shared/att.c \
+			src/shared/gatt.h src/shared/gatt.c
 src_bluetoothd_LDADD = lib/libbluetooth-internal.la gdbus/libgdbus-internal.la \
 			@GLIB_LIBS@ @DBUS_LIBS@ -ldl -lrt
 src_bluetoothd_LDFLAGS = $(AM_LDFLAGS) -Wl,--export-dynamic \
diff --git a/src/shared/gatt.c b/src/shared/gatt.c
new file mode 100644
index 0000000..2e701dc
--- /dev/null
+++ b/src/shared/gatt.c
@@ -0,0 +1,192 @@
+/*
+ *
+ *  BlueZ - Bluetooth protocol stack for Linux
+ *
+ *  Copyright (C) 2014  Google Inc.
+ *
+ *
+ *  This library is free software; you can redistribute it and/or
+ *  modify it under the terms of the GNU Lesser General Public
+ *  License as published by the Free Software Foundation; either
+ *  version 2.1 of the License, or (at your option) any later version.
+ *
+ *  This library is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ *  Lesser General Public License for more details.
+ *
+ *  You should have received a copy of the GNU Lesser General Public
+ *  License along with this library; if not, write to the Free Software
+ *  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+ *
+ */
+
+#include "lib/uuid.h"
+#include "src/shared/queue.h"
+#include "src/shared/gatt.h"
+#include "src/shared/att.h"
+
+struct bt_gatt {
+	struct bt_att *att;
+};
+
+struct bt_gatt *bt_gatt_new(int fd)
+{
+	/* TODO: Implement. */
+	return NULL;
+}
+
+void bt_gatt_destroy(struct bt_gatt *gatt)
+{
+	/* TODO: Implement. */
+}
+
+
+unsigned int bt_gatt_register(struct bt_gatt *gatt, uint16_t value_handle,
+					bt_gatt_event_callback_t callback,
+					void *user_data)
+{
+	/* TODO: Implement. */
+	return 0;
+}
+
+bool bt_gatt_unregister(unsigned int id)
+{
+	/* TODO: Implement. */
+	return false;
+}
+
+bool bt_gatt_exchange_mtu(struct bt_gatt *gatt, uint16_t mtu,
+				bt_gatt_exchange_mtu_callback_t callback,
+				void *user_data)
+{
+	/* TODO: Implement. */
+	return false;
+}
+
+uint16_t get_mtu(struct bt_gatt *gatt)
+{
+	/* TODO: Implement. */
+	return 0;
+}
+
+bool bt_gatt_discover_primary_services(struct bt_gatt *gatt, bt_uuid_t *uuid,
+					bt_gatt_discovery_callback_t callback,
+					void *user_data)
+{
+	/* TODO: Implement. */
+	return false;
+}
+
+bool bt_gatt_discover_included_services(struct bt_gatt *gatt,
+					struct bt_gatt_handle_range range,
+					bt_uuid_t *uuid,
+					bt_gatt_discovery_callback_t callback,
+					void *user_data)
+{
+	/* TODO: Implement. */
+	return false;
+}
+
+bool bt_gatt_discover_characteristics(struct bt_gatt *gatt,
+					struct bt_gatt_handle_range range,
+					bt_uuid_t *uuid,
+					bt_gatt_discovery_callback_t callback,
+					void *user_data)
+{
+	/* TODO: Implement. */
+	return false;
+}
+
+bool bt_gatt_discover_descriptors(struct bt_gatt *gatt,
+					struct bt_gatt_handle_range range,
+					bt_gatt_discovery_callback_t callback,
+					void *user_data)
+{
+	/* TODO: Implement. */
+	return false;
+}
+
+bool bt_gatt_read_characteristic_value(struct bt_gatt *gatt,
+					uint16_t value_handle,
+					bt_gatt_read_callback_t callback,
+					void *user_data)
+{
+	/* TODO: Implement. */
+	return false;
+}
+
+bool bt_gatt_read_long_characteristic_values(struct bt_gatt *gatt,
+					uint16_t value_handle, uint16_t offset,
+					bt_gatt_read_callback_t callback,
+					void *user_data)
+{
+	/* TODO: Implement. */
+	return false;
+}
+
+bool bt_gatt_write_without_response(struct bt_gatt *gatt,
+					uint16_t value_handle,
+					uint8_t *value, uint16_t length)
+{
+	/* TODO: Implement. */
+	return false;
+}
+
+bool bt_gatt_write_characteristic_value(struct bt_gatt *gatt,
+					uint16_t value_handle,
+					uint8_t *value, uint16_t length,
+					bt_gatt_result_callback_t callback,
+					void *user_data)
+{
+	/* TODO: Implement. */
+	return false;
+}
+
+bool bt_gatt_write_long_characteristic_values(struct bt_gatt *gatt,
+					uint16_t value_handle, uint16_t offset,
+					uint8_t *value, uint16_t length,
+					bt_gatt_result_callback_t callback,
+					void *user_data)
+{
+	/* TODO: Implement. */
+	return false;
+}
+
+bool bt_gatt_read_characteristic_descriptors(struct bt_gatt *gatt,
+					uint16_t value_handle,
+					bt_gatt_read_callback_t callback,
+					void *user_data)
+{
+	/* TODO: Implement. */
+	return false;
+}
+
+bool bt_gatt_read_long_characteristic_descriptors(struct bt_gatt *gatt,
+					uint16_t value_handle, uint16_t offset,
+					bt_gatt_read_callback_t callback,
+					void *user_data)
+{
+	/* TODO: Implement. */
+	return false;
+}
+
+bool bt_gatt_write_characteristic_descriptors(struct bt_gatt *gatt,
+					uint16_t value_handle,
+					uint8_t *value, uint16_t length,
+					bt_gatt_result_callback_t callback,
+					void *user_data)
+{
+	/* TODO: Implement. */
+	return false;
+}
+
+bool bt_gatt_write_long_characteristic_descriptors(struct bt_gatt *gatt,
+					uint16_t value_handle, uint16_t offset,
+					uint8_t *value, uint16_t length,
+					bt_gatt_result_callback_t callback,
+					void *user_data)
+{
+	/* TODO: Implement. */
+	return false;
+}
diff --git a/src/shared/gatt.h b/src/shared/gatt.h
new file mode 100644
index 0000000..624bd87
--- /dev/null
+++ b/src/shared/gatt.h
@@ -0,0 +1,189 @@
+/*
+ *
+ *  BlueZ - Bluetooth protocol stack for Linux
+ *
+ *  Copyright (C) 2014  Google Inc.
+ *
+ *
+ *  This library is free software; you can redistribute it and/or
+ *  modify it under the terms of the GNU Lesser General Public
+ *  License as published by the Free Software Foundation; either
+ *  version 2.1 of the License, or (at your option) any later version.
+ *
+ *  This library is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ *  Lesser General Public License for more details.
+ *
+ *  You should have received a copy of the GNU Lesser General Public
+ *  License along with this library; if not, write to the Free Software
+ *  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+ *
+ */
+
+/*
+ * An instance of struct bt_gatt comprises a client-side implementation
+ * of the Generic Attribute Profile (GATT) with regards to a specific remote
+ * GATT server. Each instance is created with a file descriptor that represents
+ * the logical link to the server, which must be configured s.t. communication
+ * via the Attribute Protocol is possible (e.g. an L2CAP LE connection over the
+ * ATT channel). bt_gatt provides functions to perform GATT client
+ * operations, such as primary service discovery, characteristic reads and
+ * writes, etc which are performed over the given connection.
+ *
+ * Most GATT clients will implement some sort of attribute caching to reduce the
+ * number operations performed over-the-air. While an instance of bt_gatt does
+ * not maintain a cache of attributes, it provides the means to get notified
+ * when notifications/indications are received. This can be used to maintain and
+ * update a separate database following events such as indications from the
+ * Service Changed characteristic or based on updates to the underlying
+ * connection (e.g. a disconnect event).
+ */
+
+#include <stdbool.h>
+#include <stdint.h>
+
+struct bt_gatt;
+
+struct bt_gatt_handle_range {
+	uint16_t start;
+	uint16_t end;
+};
+
+struct bt_gatt_service {
+	bt_uuid_t uuid;
+	struct bt_gatt_handle_range handle_range;
+};
+
+struct bt_gatt_characteristic {
+	uint8_t properties;
+	uint16_t value_handle;
+	bt_uuid_t uuid;
+	struct bt_gatt_handle_range handle_range;
+};
+
+struct bt_gatt_descriptor {
+	uint16_t handle;
+	bt_uuid_t uuid;
+};
+
+typedef enum {
+	BT_GATT_EVENT_TYPE_HANDLE_VALUE_NOTIFICATION,
+	BT_GATT_EVENT_TYPE_HANDLE_VALUE_INDICATION,
+} bt_gatt_event_type_t;
+
+typedef void (*bt_gatt_confirmation_callback_t)(void);
+
+typedef void (*bt_gatt_event_callback_t)(bt_gatt_event_type_t event_type,
+				uint16_t value_handle,
+				uint8_t *value, uint16_t length,
+				bt_gatt_confirmation_callback_t conf_callback,
+				void *user_data);
+
+typedef void (*bt_gatt_exchange_mtu_callback_t)(uint16_t final_mtu,
+							void *user_data);
+
+/*
+ * "status" is set to 0 if the operation was successful. Otherwise, it is set
+ * to the ATT protocol error code. See the error codes defined in
+ * src/shared/att.h.
+ */
+typedef void (*bt_gatt_discovery_callback_t)(uint8_t status,
+							struct queue *list,
+							void *user_data);
+
+typedef void (*bt_gatt_read_callback_t)(uint8_t status, uint8_t *value,
+					uint16_t length, void *user_data);
+
+typedef void (*bt_gatt_result_callback_t)(uint8_t status, void *user_data);
+
+struct bt_gatt *bt_gatt_new(int fd);
+void bt_gatt_destroy(struct bt_gatt *gatt);
+
+
+/******* Notifications/Indications *******/
+unsigned int bt_gatt_register(struct bt_gatt *gatt, uint16_t value_handle,
+					bt_gatt_event_callback_t callback,
+					void *user_data);
+bool bt_gatt_unregister(unsigned int id);
+
+/******* Server Configuration *******/
+bool bt_gatt_exchange_mtu(struct bt_gatt *gatt, uint16_t mtu,
+				bt_gatt_exchange_mtu_callback_t callback,
+				void *user_data);
+
+uint16_t get_mtu(struct bt_gatt *gatt);
+
+/******* Primary Service/Relationship/Characteristic/Descriptor discovery *****/
+bool bt_gatt_discover_primary_services(struct bt_gatt *gatt, bt_uuid_t *uuid,
+					bt_gatt_discovery_callback_t callback,
+					void *user_data);
+
+bool bt_gatt_discover_included_services(struct bt_gatt *gatt,
+					struct bt_gatt_handle_range range,
+					bt_uuid_t *uuid,
+					bt_gatt_discovery_callback_t callback,
+					void *user_data);
+
+bool bt_gatt_discover_characteristics(struct bt_gatt *gatt,
+					struct bt_gatt_handle_range range,
+					bt_uuid_t *uuid,
+					bt_gatt_discovery_callback_t callback,
+					void *user_data);
+
+bool bt_gatt_discover_descriptors(struct bt_gatt *gatt,
+					struct bt_gatt_handle_range range,
+					bt_gatt_discovery_callback_t callback,
+					void *user_data);
+
+/******* Characteristic Value Read *******/
+bool bt_gatt_read_characteristic_value(struct bt_gatt *gatt,
+					uint16_t value_handle,
+					bt_gatt_read_callback_t callback,
+					void *user_data);
+
+bool bt_gatt_read_long_characteristic_values(struct bt_gatt *gatt,
+					uint16_t value_handle, uint16_t offset,
+					bt_gatt_read_callback_t callback,
+					void *user_data);
+
+/******* Characteristic Value Write *******/
+bool bt_gatt_write_without_response(struct bt_gatt *gatt,
+					uint16_t value_handle,
+					uint8_t *value, uint16_t length);
+
+bool bt_gatt_write_characteristic_value(struct bt_gatt *gatt,
+					uint16_t value_handle,
+					uint8_t *value, uint16_t length,
+					bt_gatt_result_callback_t callback,
+					void *user_data);
+
+bool bt_gatt_write_long_characteristic_values(struct bt_gatt *gatt,
+					uint16_t value_handle, uint16_t offset,
+					uint8_t *value, uint16_t length,
+					bt_gatt_result_callback_t callback,
+					void *user_data);
+
+/******* Characteristic Descriptor Value Read *******/
+bool bt_gatt_read_characteristic_descriptors(struct bt_gatt *gatt,
+					uint16_t value_handle,
+					bt_gatt_read_callback_t callback,
+					void *user_data);
+
+bool bt_gatt_read_long_characteristic_descriptors(struct bt_gatt *gatt,
+					uint16_t value_handle, uint16_t offset,
+					bt_gatt_read_callback_t callback,
+					void *user_data);
+
+/******* Characteristic Descriptor Value Write *******/
+bool bt_gatt_write_characteristic_descriptors(struct bt_gatt *gatt,
+					uint16_t value_handle,
+					uint8_t *value, uint16_t length,
+					bt_gatt_result_callback_t callback,
+					void *user_data);
+
+bool bt_gatt_write_long_characteristic_descriptors(struct bt_gatt *gatt,
+					uint16_t value_handle, uint16_t offset,
+					uint8_t *value, uint16_t length,
+					bt_gatt_result_callback_t callback,
+					void *user_data);
-- 
1.8.3.2

Re: [RFC BlueZ 1/1] src/shared/gatt: Introduce bt_gatt skeleton.

[Marcel Holtmann]

Hi Arman,

> This patch introduces src/shared/gatt, which provides an interface for
> GATT client operations. bt_gatt will internally use src/shared/att for
> ATT protocol client-side operations. bt_gatt is meant to be used
> together with src/shared/gatt-db to implement a complete GATT client.
> ---
> Makefile.am       |   3 +-
> src/shared/gatt.c | 192 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> src/shared/gatt.h | 189 +++++++++++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 383 insertions(+), 1 deletion(-)
> create mode 100644 src/shared/gatt.c
> create mode 100644 src/shared/gatt.h
> 
> diff --git a/Makefile.am b/Makefile.am
> index 04be6c4..8b731f0 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -156,7 +156,8 @@ src_bluetoothd_SOURCES = $(builtin_sources) \
> 			src/shared/queue.h src/shared/queue.c \
> 			src/shared/util.h src/shared/util.c \
> 			src/shared/mgmt.h src/shared/mgmt.c \
> -			src/shared/att.h src/shared/att.c
> +			src/shared/att.h src/shared/att.c \
> +			src/shared/gatt.h src/shared/gatt.c
> src_bluetoothd_LDADD = lib/libbluetooth-internal.la gdbus/libgdbus-internal.la \
> 			@GLIB_LIBS@ @DBUS_LIBS@ -ldl -lrt
> src_bluetoothd_LDFLAGS = $(AM_LDFLAGS) -Wl,--export-dynamic \
> diff --git a/src/shared/gatt.c b/src/shared/gatt.c
> new file mode 100644
> index 0000000..2e701dc
> --- /dev/null
> +++ b/src/shared/gatt.c
> @@ -0,0 +1,192 @@
> +/*
> + *
> + *  BlueZ - Bluetooth protocol stack for Linux
> + *
> + *  Copyright (C) 2014  Google Inc.
> + *
> + *
> + *  This library is free software; you can redistribute it and/or
> + *  modify it under the terms of the GNU Lesser General Public
> + *  License as published by the Free Software Foundation; either
> + *  version 2.1 of the License, or (at your option) any later version.
> + *
> + *  This library is distributed in the hope that it will be useful,
> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + *  Lesser General Public License for more details.
> + *
> + *  You should have received a copy of the GNU Lesser General Public
> + *  License along with this library; if not, write to the Free Software
> + *  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
> + *
> + */
> +
> +#include "lib/uuid.h"
> +#include "src/shared/queue.h"
> +#include "src/shared/gatt.h"
> +#include "src/shared/att.h"
> +
> +struct bt_gatt {
> +	struct bt_att *att;
> +};
> +
> +struct bt_gatt *bt_gatt_new(int fd)
> +{
> +	/* TODO: Implement. */
> +	return NULL;
> +}
> +
> +void bt_gatt_destroy(struct bt_gatt *gatt)
> +{
> +	/* TODO: Implement. */
> +}
> +
> +
> +unsigned int bt_gatt_register(struct bt_gatt *gatt, uint16_t value_handle,
> +					bt_gatt_event_callback_t callback,
> +					void *user_data)
> +{
> +	/* TODO: Implement. */
> +	return 0;
> +}
> +
> +bool bt_gatt_unregister(unsigned int id)
> +{
> +	/* TODO: Implement. */
> +	return false;
> +}
> +
> +bool bt_gatt_exchange_mtu(struct bt_gatt *gatt, uint16_t mtu,
> +				bt_gatt_exchange_mtu_callback_t callback,
> +				void *user_data)
> +{
> +	/* TODO: Implement. */
> +	return false;
> +}
> +
> +uint16_t get_mtu(struct bt_gatt *gatt)
> +{
> +	/* TODO: Implement. */
> +	return 0;
> +}
> +
> +bool bt_gatt_discover_primary_services(struct bt_gatt *gatt, bt_uuid_t *uuid,
> +					bt_gatt_discovery_callback_t callback,
> +					void *user_data)
> +{
> +	/* TODO: Implement. */
> +	return false;
> +}
> +
> +bool bt_gatt_discover_included_services(struct bt_gatt *gatt,
> +					struct bt_gatt_handle_range range,
> +					bt_uuid_t *uuid,
> +					bt_gatt_discovery_callback_t callback,
> +					void *user_data)
> +{
> +	/* TODO: Implement. */
> +	return false;
> +}
> +
> +bool bt_gatt_discover_characteristics(struct bt_gatt *gatt,
> +					struct bt_gatt_handle_range range,
> +					bt_uuid_t *uuid,
> +					bt_gatt_discovery_callback_t callback,
> +					void *user_data)
> +{
> +	/* TODO: Implement. */
> +	return false;
> +}
> +
> +bool bt_gatt_discover_descriptors(struct bt_gatt *gatt,
> +					struct bt_gatt_handle_range range,
> +					bt_gatt_discovery_callback_t callback,
> +					void *user_data)
> +{
> +	/* TODO: Implement. */
> +	return false;
> +}
> +
> +bool bt_gatt_read_characteristic_value(struct bt_gatt *gatt,
> +					uint16_t value_handle,
> +					bt_gatt_read_callback_t callback,
> +					void *user_data)
> +{
> +	/* TODO: Implement. */
> +	return false;
> +}
> +
> +bool bt_gatt_read_long_characteristic_values(struct bt_gatt *gatt,
> +					uint16_t value_handle, uint16_t offset,
> +					bt_gatt_read_callback_t callback,
> +					void *user_data)
> +{
> +	/* TODO: Implement. */
> +	return false;
> +}
> +
> +bool bt_gatt_write_without_response(struct bt_gatt *gatt,
> +					uint16_t value_handle,
> +					uint8_t *value, uint16_t length)
> +{
> +	/* TODO: Implement. */
> +	return false;
> +}
> +
> +bool bt_gatt_write_characteristic_value(struct bt_gatt *gatt,
> +					uint16_t value_handle,
> +					uint8_t *value, uint16_t length,
> +					bt_gatt_result_callback_t callback,
> +					void *user_data)
> +{
> +	/* TODO: Implement. */
> +	return false;
> +}
> +
> +bool bt_gatt_write_long_characteristic_values(struct bt_gatt *gatt,
> +					uint16_t value_handle, uint16_t offset,
> +					uint8_t *value, uint16_t length,
> +					bt_gatt_result_callback_t callback,
> +					void *user_data)
> +{
> +	/* TODO: Implement. */
> +	return false;
> +}
> +
> +bool bt_gatt_read_characteristic_descriptors(struct bt_gatt *gatt,
> +					uint16_t value_handle,
> +					bt_gatt_read_callback_t callback,
> +					void *user_data)
> +{
> +	/* TODO: Implement. */
> +	return false;
> +}
> +
> +bool bt_gatt_read_long_characteristic_descriptors(struct bt_gatt *gatt,
> +					uint16_t value_handle, uint16_t offset,
> +					bt_gatt_read_callback_t callback,
> +					void *user_data)
> +{
> +	/* TODO: Implement. */
> +	return false;
> +}
> +
> +bool bt_gatt_write_characteristic_descriptors(struct bt_gatt *gatt,
> +					uint16_t value_handle,
> +					uint8_t *value, uint16_t length,
> +					bt_gatt_result_callback_t callback,
> +					void *user_data)
> +{
> +	/* TODO: Implement. */
> +	return false;
> +}
> +
> +bool bt_gatt_write_long_characteristic_descriptors(struct bt_gatt *gatt,
> +					uint16_t value_handle, uint16_t offset,
> +					uint8_t *value, uint16_t length,
> +					bt_gatt_result_callback_t callback,
> +					void *user_data)
> +{
> +	/* TODO: Implement. */
> +	return false;
> +}
> diff --git a/src/shared/gatt.h b/src/shared/gatt.h
> new file mode 100644
> index 0000000..624bd87
> --- /dev/null
> +++ b/src/shared/gatt.h
> @@ -0,0 +1,189 @@
> +/*
> + *
> + *  BlueZ - Bluetooth protocol stack for Linux
> + *
> + *  Copyright (C) 2014  Google Inc.
> + *
> + *
> + *  This library is free software; you can redistribute it and/or
> + *  modify it under the terms of the GNU Lesser General Public
> + *  License as published by the Free Software Foundation; either
> + *  version 2.1 of the License, or (at your option) any later version.
> + *
> + *  This library is distributed in the hope that it will be useful,
> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + *  Lesser General Public License for more details.
> + *
> + *  You should have received a copy of the GNU Lesser General Public
> + *  License along with this library; if not, write to the Free Software
> + *  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
> + *
> + */
> +
> +/*
> + * An instance of struct bt_gatt comprises a client-side implementation
> + * of the Generic Attribute Profile (GATT) with regards to a specific remote
> + * GATT server. Each instance is created with a file descriptor that represents
> + * the logical link to the server, which must be configured s.t. communication
> + * via the Attribute Protocol is possible (e.g. an L2CAP LE connection over the
> + * ATT channel). bt_gatt provides functions to perform GATT client
> + * operations, such as primary service discovery, characteristic reads and
> + * writes, etc which are performed over the given connection.
> + *
> + * Most GATT clients will implement some sort of attribute caching to reduce the
> + * number operations performed over-the-air. While an instance of bt_gatt does
> + * not maintain a cache of attributes, it provides the means to get notified
> + * when notifications/indications are received. This can be used to maintain and
> + * update a separate database following events such as indications from the
> + * Service Changed characteristic or based on updates to the underlying
> + * connection (e.g. a disconnect event).
> + */

I do not like to have these ones in the header. At least not at the top right after the copyright notice. Put them into the code or closer to the actual structs. In most cases I found to have these kind of detailed comments really useful close to the area of the code that uses it. Since that is where you normally end up tracing the bug to.

> +
> +#include <stdbool.h>
> +#include <stdint.h>
> +
> +struct bt_gatt;
> +
> +struct bt_gatt_handle_range {
> +	uint16_t start;
> +	uint16_t end;
> +};

Should we just do start + end in the parameter list? What good is a struct doing here?

> +
> +struct bt_gatt_service {
> +	bt_uuid_t uuid;
> +	struct bt_gatt_handle_range handle_range;
> +};

The bt_gatt_handle_range gets pretty long here. Feels not right and might turn the code unreadable.

> +
> +struct bt_gatt_characteristic {
> +	uint8_t properties;
> +	uint16_t value_handle;
> +	bt_uuid_t uuid;
> +	struct bt_gatt_handle_range handle_range;
> +};
> +
> +struct bt_gatt_descriptor {
> +	uint16_t handle;
> +	bt_uuid_t uuid;
> +};

I am trying to get away from the bt_uuid_t. In my new SDP code I just have it has uint8_t uuid[16] and have the code internally convert from 16-bit / 32-bit to 128-bit on the fly. Meaning I always store the 128-bit version. And that is what we should do anyway. If we can shorten the UUID, we do that. Otherwise we just send the 128-bit version.

> +
> +typedef enum {
> +	BT_GATT_EVENT_TYPE_HANDLE_VALUE_NOTIFICATION,
> +	BT_GATT_EVENT_TYPE_HANDLE_VALUE_INDICATION,
> +} bt_gatt_event_type_t;

Long long names. Don’t really like that. I thin we need to be a bit more creative here.

> +
> +typedef void (*bt_gatt_confirmation_callback_t)(void);
> +
> +typedef void (*bt_gatt_event_callback_t)(bt_gatt_event_type_t event_type,
> +				uint16_t value_handle,
> +				uint8_t *value, uint16_t length,
> +				bt_gatt_confirmation_callback_t conf_callback,
> +				void *user_data);
> +
> +typedef void (*bt_gatt_exchange_mtu_callback_t)(uint16_t final_mtu,
> +							void *user_data);
> +
> +/*
> + * "status" is set to 0 if the operation was successful. Otherwise, it is set
> + * to the ATT protocol error code. See the error codes defined in
> + * src/shared/att.h.
> + */
> +typedef void (*bt_gatt_discovery_callback_t)(uint8_t status,
> +							struct queue *list,
> +							void *user_data);

Might want to call it results instead of list. Or call it services.

> +
> +typedef void (*bt_gatt_read_callback_t)(uint8_t status, uint8_t *value,
> +					uint16_t length, void *user_data);
> +
> +typedef void (*bt_gatt_result_callback_t)(uint8_t status, void *user_data);
> +
> +struct bt_gatt *bt_gatt_new(int fd);
> +void bt_gatt_destroy(struct bt_gatt *gatt);
> +
> +
> +/******* Notifications/Indications *******/

Please do not use these style of comments in the header files. They just get messy over time.

> +unsigned int bt_gatt_register(struct bt_gatt *gatt, uint16_t value_handle,
> +					bt_gatt_event_callback_t callback,
> +					void *user_data);
> +bool bt_gatt_unregister(unsigned int id);
> +
> +/******* Server Configuration *******/
> +bool bt_gatt_exchange_mtu(struct bt_gatt *gatt, uint16_t mtu,
> +				bt_gatt_exchange_mtu_callback_t callback,
> +				void *user_data);
> +
> +uint16_t get_mtu(struct bt_gatt *gatt);

This is wrong in the header. We need namespacing. Or a gatt-private.h header.

> +
> +/******* Primary Service/Relationship/Characteristic/Descriptor discovery *****/
> +bool bt_gatt_discover_primary_services(struct bt_gatt *gatt, bt_uuid_t *uuid,
> +					bt_gatt_discovery_callback_t callback,
> +					void *user_data);
> +
> +bool bt_gatt_discover_included_services(struct bt_gatt *gatt,
> +					struct bt_gatt_handle_range range,
> +					bt_uuid_t *uuid,
> +					bt_gatt_discovery_callback_t callback,
> +					void *user_data);
> +
> +bool bt_gatt_discover_characteristics(struct bt_gatt *gatt,
> +					struct bt_gatt_handle_range range,
> +					bt_uuid_t *uuid,
> +					bt_gatt_discovery_callback_t callback,
> +					void *user_data);
> +
> +bool bt_gatt_discover_descriptors(struct bt_gatt *gatt,
> +					struct bt_gatt_handle_range range,
> +					bt_gatt_discovery_callback_t callback,
> +					void *user_data);

Should we really expose these low-level operations? Why not just have one bt_gatt_discover() that does all the magic for us. At the end of the day, that is what most devices do when they connect to a remote device. They will update the services they either know or have to discover new ones.

> +
> +/******* Characteristic Value Read *******/
> +bool bt_gatt_read_characteristic_value(struct bt_gatt *gatt,
> +					uint16_t value_handle,
> +					bt_gatt_read_callback_t callback,
> +					void *user_data);
> +
> +bool bt_gatt_read_long_characteristic_values(struct bt_gatt *gatt,
> +					uint16_t value_handle, uint16_t offset,
> +					bt_gatt_read_callback_t callback,
> +					void *user_data);

Function names get longer and longer. We need better names and more important shorter ones.

> +
> +/******* Characteristic Value Write *******/
> +bool bt_gatt_write_without_response(struct bt_gatt *gatt,
> +					uint16_t value_handle,
> +					uint8_t *value, uint16_t length);
> +
> +bool bt_gatt_write_characteristic_value(struct bt_gatt *gatt,
> +					uint16_t value_handle,
> +					uint8_t *value, uint16_t length,
> +					bt_gatt_result_callback_t callback,
> +					void *user_data);
> +
> +bool bt_gatt_write_long_characteristic_values(struct bt_gatt *gatt,
> +					uint16_t value_handle, uint16_t offset,
> +					uint8_t *value, uint16_t length,
> +					bt_gatt_result_callback_t callback,
> +					void *user_data);
> +
> +/******* Characteristic Descriptor Value Read *******/
> +bool bt_gatt_read_characteristic_descriptors(struct bt_gatt *gatt,
> +					uint16_t value_handle,
> +					bt_gatt_read_callback_t callback,
> +					void *user_data);
> +
> +bool bt_gatt_read_long_characteristic_descriptors(struct bt_gatt *gatt,
> +					uint16_t value_handle, uint16_t offset,
> +					bt_gatt_read_callback_t callback,
> +					void *user_data);
> +
> +/******* Characteristic Descriptor Value Write *******/
> +bool bt_gatt_write_characteristic_descriptors(struct bt_gatt *gatt,
> +					uint16_t value_handle,
> +					uint8_t *value, uint16_t length,
> +					bt_gatt_result_callback_t callback,
> +					void *user_data);
> +
> +bool bt_gatt_write_long_characteristic_descriptors(struct bt_gatt *gatt,
> +					uint16_t value_handle, uint16_t offset,
> +					uint8_t *value, uint16_t length,
> +					bt_gatt_result_callback_t callback,
> +					void *user_data);

Regards

Marcel

Re: [RFC BlueZ 1/1] src/shared/gatt: Introduce bt_gatt skeleton.

[Arman Uguray]

Hi Marcel,

>> +bool bt_gatt_discover_primary_services(struct bt_gatt *gatt, bt_uuid_t =
*uuid,
>> +                                     bt_gatt_discovery_callback_t callb=
ack,
>> +                                     void *user_data);
>> +
>> +bool bt_gatt_discover_included_services(struct bt_gatt *gatt,
>> +                                     struct bt_gatt_handle_range range,
>> +                                     bt_uuid_t *uuid,
>> +                                     bt_gatt_discovery_callback_t callb=
ack,
>> +                                     void *user_data);
>> +
>> +bool bt_gatt_discover_characteristics(struct bt_gatt *gatt,
>> +                                     struct bt_gatt_handle_range range,
>> +                                     bt_uuid_t *uuid,
>> +                                     bt_gatt_discovery_callback_t callb=
ack,
>> +                                     void *user_data);
>> +
>> +bool bt_gatt_discover_descriptors(struct bt_gatt *gatt,
>> +                                     struct bt_gatt_handle_range range,
>> +                                     bt_gatt_discovery_callback_t callb=
ack,
>> +                                     void *user_data);
>
> Should we really expose these low-level operations? Why not just have one=
 bt_gatt_discover() that does all the magic for us. At the end of the day, =
that is what most devices do when they connect to a remote device. They wil=
l update the services they either know or have to discover new ones.

I think we should have them. I pretty much see bt_gatt as an interface
that translates GATT feature procedures to the underlying ATT protocol
operations. I leave the magic to be the responsibility of the of the
upper-layer, where we will have a client implementation that brings
together src/shared/gatt and src/shared/gatt-db. I'm thinking
something along the lines of a btd_gatt_client for bluetoothd, that
discovers everything, caches all attributes, and makes them accessible
to all the profiles/plugins (this is actually what I'm doing initially
on Chrome OS using GAttrib, since we're against a deadline for
Chrome). Eventually we could use this to remove the attio callbacks.

So, to answer your question, I think having these functions at this
level will be useful, especially for testing when we start writing PTS
style tests for GATT.

As for your other comments, I will make names shorter. I'm used to
writing long and very explicit function names, but I figured I would
upload them and see if someone yells at me :P

Cheers,
Arman

Re: [RFC BlueZ 1/1] src/shared/gatt: Introduce bt_gatt skeleton.

[Marcel Holtmann]

Hi Arman,

>>> +bool bt_gatt_discover_primary_services(struct bt_gatt *gatt, bt_uuid_t *uuid,
>>> +                                     bt_gatt_discovery_callback_t callback,
>>> +                                     void *user_data);
>>> +
>>> +bool bt_gatt_discover_included_services(struct bt_gatt *gatt,
>>> +                                     struct bt_gatt_handle_range range,
>>> +                                     bt_uuid_t *uuid,
>>> +                                     bt_gatt_discovery_callback_t callback,
>>> +                                     void *user_data);
>>> +
>>> +bool bt_gatt_discover_characteristics(struct bt_gatt *gatt,
>>> +                                     struct bt_gatt_handle_range range,
>>> +                                     bt_uuid_t *uuid,
>>> +                                     bt_gatt_discovery_callback_t callback,
>>> +                                     void *user_data);
>>> +
>>> +bool bt_gatt_discover_descriptors(struct bt_gatt *gatt,
>>> +                                     struct bt_gatt_handle_range range,
>>> +                                     bt_gatt_discovery_callback_t callback,
>>> +                                     void *user_data);
>> 
>> Should we really expose these low-level operations? Why not just have one bt_gatt_discover() that does all the magic for us. At the end of the day, that is what most devices do when they connect to a remote device. They will update the services they either know or have to discover new ones.
> 
> I think we should have them. I pretty much see bt_gatt as an interface
> that translates GATT feature procedures to the underlying ATT protocol
> operations. I leave the magic to be the responsibility of the of the
> upper-layer, where we will have a client implementation that brings
> together src/shared/gatt and src/shared/gatt-db. I'm thinking
> something along the lines of a btd_gatt_client for bluetoothd, that
> discovers everything, caches all attributes, and makes them accessible
> to all the profiles/plugins (this is actually what I'm doing initially
> on Chrome OS using GAttrib, since we're against a deadline for
> Chrome). Eventually we could use this to remove the attio callbacks.

so in the long run I want to share as much code of BlueZ for Android with BlueZ upstream code. Meaning that we should just implement the actual procedures we are doing and not having to repeat the magic to make service discovery smart.

> So, to answer your question, I think having these functions at this
> level will be useful, especially for testing when we start writing PTS
> style tests for GATT.

Do you already happen to have a list of PTS test cases that would need special handling?

Regards

Marcel