[PATCH BlueZ 0/3] Introduce shared/gatt-server.

[PATCH BlueZ 0/3] Introduce shared/gatt-server.

[Arman Uguray]

This patch set introduces shared/gatt-client and implements the server side of
ATT Exchange MTU request. I made the decision to keep gatt-db internal to
gatt-server due to the following reasons:

  1. I want to keep all ATT protocol details to remain internal to gatt-server
  and gatt-client. gatt-db creates a layering problem for attributes that get
  added with a high-level read/write handler. In this case, gatt-db leaves the
  responsibility of obtaining the attribute value and forming the ATT protocol
  response to the layer that registered the read handler. We want the actual PDU
  construction to be done by gatt-server, hence gatt-server needs to inject its
  own read/write handlers and provide a level of indirection to the upper-layer
  to obtain the value.

  My decision here is to write functions in the bt_gatt_server_* namespace to
  add/remove attributes into the database.

  2. This allows gatt-server to correctly send Service Changed indications
  without having the gatt-db modified behind its back.

  3. gatt-db exposes structures like shared/queue in its public interface which,
  after some mailing discussions, were decided to remain internal to shared
  code.

Arman Uguray (3):
  shared/att: Drop the connection is a request is received while one is
    pending.
  shared/gatt-server: Introduce shared/gatt-server.
  shared/gatt-server: Support Exchange MTU request.

 Makefile.am              |   1 +
 src/shared/att.c         | 101 +++++++++++++---------
 src/shared/gatt-server.c | 219 +++++++++++++++++++++++++++++++++++++++++++++++
 src/shared/gatt-server.h |  47 ++++++++++
 4 files changed, 328 insertions(+), 40 deletions(-)
 create mode 100644 src/shared/gatt-server.c
 create mode 100644 src/shared/gatt-server.h

-- 
2.1.0.rc2.206.gedb03e5

[PATCH BlueZ 1/3] shared/att: Drop the connection is a request is received while one is pending.

[Arman Uguray]

With this patch, when bt_att is being used for the server role, it now makes
sure that a second request drops the connection unless a response for a previous
request has been sent.
---
 src/shared/att.c | 101 +++++++++++++++++++++++++++++++++----------------------
 1 file changed, 61 insertions(+), 40 deletions(-)

diff --git a/src/shared/att.c b/src/shared/att.c
index de35aef..96f34a3 100644
--- a/src/shared/att.c
+++ b/src/shared/att.c
@@ -64,6 +64,8 @@ struct bt_att {
 	bool in_disconn;
 	bool need_disconn_cleanup;
 
+	bool in_req;			/* There's a pending incoming request */
+
 	uint8_t *buf;
 	uint16_t mtu;
 
@@ -460,6 +462,9 @@ static bool can_write_data(struct io *io, void *user_data)
 	case ATT_OP_TYPE_IND:
 		att->pending_ind = op;
 		break;
+	case ATT_OP_TYPE_RSP:
+		/* Set in_req to false to indicate that no request is pending */
+		att->in_req = false;
 	default:
 		destroy_att_send_op(op);
 		return true;
@@ -604,6 +609,46 @@ static void handle_notify(struct bt_att *att, uint8_t opcode, uint8_t *pdu,
 	bt_att_unref(att);
 }
 
+static void disconn_handler(void *data, void *user_data)
+{
+	struct att_disconn *disconn = data;
+
+	if (disconn->removed)
+		return;
+
+	if (disconn->callback)
+		disconn->callback(disconn->user_data);
+}
+
+static bool disconnect_cb(struct io *io, void *user_data)
+{
+	struct bt_att *att = user_data;
+
+	io_destroy(att->io);
+	att->io = NULL;
+
+	util_debug(att->debug_callback, att->debug_data,
+						"Physical link disconnected");
+
+	bt_att_ref(att);
+	att->in_disconn = true;
+	queue_foreach(att->disconn_list, disconn_handler, NULL);
+	att->in_disconn = false;
+
+	if (att->need_disconn_cleanup) {
+		queue_remove_all(att->disconn_list, match_disconn_removed, NULL,
+							destroy_att_disconn);
+		att->need_disconn_cleanup = false;
+	}
+
+	bt_att_cancel_all(att);
+	bt_att_unregister_all(att);
+
+	bt_att_unref(att);
+
+	return false;
+}
+
 static bool can_read_data(struct io *io, void *user_data)
 {
 	struct bt_att *att = user_data;
@@ -635,6 +680,22 @@ static bool can_read_data(struct io *io, void *user_data)
 		util_debug(att->debug_callback, att->debug_data,
 				"ATT opcode cannot be handled: 0x%02x", opcode);
 		break;
+	case ATT_OP_TYPE_REQ:
+		/* If a request is currently pending, then the sequential
+		 * protocol was violated. Disconnect the bearer and notify the
+		 * upper-layer.
+		 */
+		if (att->in_req) {
+			util_debug(att->debug_callback, att->debug_data,
+					"Received request while another is "
+					"pending: 0x%02x", opcode);
+			disconnect_cb(att->io, att);
+			return false;
+		}
+
+		att->in_req = true;
+
+		/* Fall through to the next case */
 	default:
 		/* For all other opcodes notify the upper layer of the PDU and
 		 * let them act on it.
@@ -648,46 +709,6 @@ static bool can_read_data(struct io *io, void *user_data)
 	return true;
 }
 
-static void disconn_handler(void *data, void *user_data)
-{
-	struct att_disconn *disconn = data;
-
-	if (disconn->removed)
-		return;
-
-	if (disconn->callback)
-		disconn->callback(disconn->user_data);
-}
-
-static bool disconnect_cb(struct io *io, void *user_data)
-{
-	struct bt_att *att = user_data;
-
-	io_destroy(att->io);
-	att->io = NULL;
-
-	util_debug(att->debug_callback, att->debug_data,
-						"Physical link disconnected");
-
-	bt_att_ref(att);
-	att->in_disconn = true;
-	queue_foreach(att->disconn_list, disconn_handler, NULL);
-	att->in_disconn = false;
-
-	if (att->need_disconn_cleanup) {
-		queue_remove_all(att->disconn_list, match_disconn_removed, NULL,
-							destroy_att_disconn);
-		att->need_disconn_cleanup = false;
-	}
-
-	bt_att_cancel_all(att);
-	bt_att_unregister_all(att);
-
-	bt_att_unref(att);
-
-	return false;
-}
-
 struct bt_att *bt_att_new(int fd)
 {
 	struct bt_att *att;
-- 
2.1.0.rc2.206.gedb03e5

[PATCH BlueZ 2/3] shared/gatt-server: Introduce shared/gatt-server.

[Arman Uguray]

This patch introduces bt_gatt_server which will implement the server-side of the
ATT protocol over a bt_att structure and construct responses based on a gatt_db
structure.
---
 Makefile.am              |   1 +
 src/shared/gatt-server.c | 155 +++++++++++++++++++++++++++++++++++++++++++++++
 src/shared/gatt-server.h |  47 ++++++++++++++
 3 files changed, 203 insertions(+)
 create mode 100644 src/shared/gatt-server.c
 create mode 100644 src/shared/gatt-server.h

diff --git a/Makefile.am b/Makefile.am
index 1a1a43f..2dfea28 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -114,6 +114,7 @@ shared_sources = src/shared/io.h src/shared/timeout.h \
 			src/shared/att.h src/shared/att.c \
 			src/shared/gatt-helpers.h src/shared/gatt-helpers.c \
 			src/shared/gatt-client.h src/shared/gatt-client.c \
+			src/shared/gatt-server.h src/shared/gatt-server.c \
 			src/shared/gatt-db.h src/shared/gatt-db.c \
 			src/shared/gap.h src/shared/gap.c
 
diff --git a/src/shared/gatt-server.c b/src/shared/gatt-server.c
new file mode 100644
index 0000000..8e32def
--- /dev/null
+++ b/src/shared/gatt-server.c
@@ -0,0 +1,155 @@
+/*
+ *
+ *  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 "src/shared/att.h"
+#include "lib/uuid.h"
+#include "src/shared/queue.h"
+#include "src/shared/gatt-db.h"
+#include "src/shared/gatt-server.h"
+#include "src/shared/gatt-helpers.h"
+#include "src/shared/att-types.h"
+#include "src/shared/util.h"
+
+#ifndef MAX
+#define MAX(a, b) ((a) > (b) ? (a) : (b))
+#endif
+
+struct bt_gatt_server {
+	struct gatt_db *db;
+	struct bt_att *att;
+	int ref_count;
+	uint16_t mtu;
+};
+
+static bool gatt_server_register_att_handlers(struct bt_gatt_server *server)
+{
+	/* TODO */
+	return true;
+}
+
+struct bt_gatt_server *bt_gatt_server_new(struct bt_att *att,
+							uint16_t mtu)
+{
+	struct bt_gatt_server *server;
+
+	if (!att)
+		return NULL;
+
+	server = new0(struct bt_gatt_server, 1);
+	if (!server)
+		return NULL;
+
+	server->db = gatt_db_new();
+	if (!server->db) {
+		free(server);
+		return NULL;
+	}
+
+	server->att = bt_att_ref(att);
+	server->mtu = MAX(mtu, BT_ATT_DEFAULT_LE_MTU);
+
+	if (!gatt_server_register_att_handlers(server)) {
+		gatt_db_destroy(server->db);
+		bt_att_unref(att);
+		free(server);
+		return NULL;
+	}
+
+	return bt_gatt_server_ref(server);
+}
+
+struct bt_gatt_server *bt_gatt_server_ref(struct bt_gatt_server *server)
+{
+	if (!server)
+		return NULL;
+
+	__sync_fetch_and_add(&server->ref_count, 1);
+
+	return server;
+}
+
+void bt_gatt_server_unref(struct bt_gatt_server *server)
+{
+	if (__sync_sub_and_fetch(&server->ref_count, 1))
+		return;
+
+	if (server->debug_destroy)
+		server->debug_destroy(server->debug_data);
+
+	gatt_server_cleanup(server);
+	gatt_db_destroy(server->db);
+
+	free(server);
+}
+
+bool bt_gatt_server_set_debug(struct bt_gatt_server *server,
+					bt_gatt_server_debug_func_t callback,
+					void *user_data,
+					bt_gatt_server_destroy_func_t destroy)
+{
+	if (!server)
+		return false;
+
+	if (server->debug_destroy)
+		server->debug_destroy(server->debug_data);
+
+	server->debug_callback = callback;
+	server->debug_destroy = destroy;
+	server->debug_data = user_data;
+
+	return true;
+}
+
+uint16_t bt_gatt_server_add_service(struct bt_gatt_server *server,
+					const bt_uuid_t *uuid, bool primary,
+					uint16_t num_handles)
+{
+	if (!server)
+		return 0;
+
+	/* TODO: Send service changed signal here if necessary */
+
+	return gatt_db_add_service(server->db, uuid, primary, num_handles);
+}
+
+bool bt_gatt_server_remove_service(struct bt_gatt_server *server,
+							uint16_t handle)
+{
+	if (!server)
+		return false;
+
+	/* TODO: Send service changed signal here if necessary */
+
+	return gatt_db_remove_service(server->db, handle);
+}
+
+bool bt_gatt_server_service_set_active(struct bt_gatt_server *server,
+						uint16_t handle, bool active)
+{
+	if (!server)
+		return false;
+
+	/* TODO: Send service changed signal here if necessary */
+
+	return gatt_db_service_set_active(server->db, handle, active);
+}
diff --git a/src/shared/gatt-server.h b/src/shared/gatt-server.h
new file mode 100644
index 0000000..0027715
--- /dev/null
+++ b/src/shared/gatt-server.h
@@ -0,0 +1,47 @@
+/*
+ *
+ *  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 <stdint.h>
+
+struct bt_gatt_server;
+
+struct bt_gatt_server *bt_gatt_server_new(struct bt_att *att, uint16_t mtu);
+
+struct bt_gatt_server *bt_gatt_server_ref(struct bt_gatt_server *server);
+void bt_gatt_server_unref(struct bt_gatt_server *server);
+
+typedef void (*bt_gatt_server_destroy_func_t)(void *user_data);
+typedef void (*bt_gatt_server_debug_func_t)(const char *str, void *user_data);
+
+bool bt_gatt_server_set_debug(struct bt_gatt_server *server,
+					bt_gatt_server_debug_func_t callback,
+					void *user_data,
+					bt_gatt_server_destroy_func_t destroy);
+
+uint16_t bt_gatt_server_add_service(struct bt_gatt_server *server,
+					const bt_uuid_t *uuid, bool primary,
+					uint16_t num_handles);
+bool bt_gatt_server_remove_service(struct bt_gatt_server *server,
+							uint16_t handle);
+bool bt_gatt_server_service_set_active(struct bt_gatt_server *server,
+						uint16_t handle, bool active);
-- 
2.1.0.rc2.206.gedb03e5

[PATCH BlueZ 3/3] shared/gatt-server: Support Exchange MTU request.

[Arman Uguray]

This patch adds handling for the exchange MTU request.
---
 src/shared/gatt-server.c | 66 +++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 65 insertions(+), 1 deletion(-)

diff --git a/src/shared/gatt-server.c b/src/shared/gatt-server.c
index 8e32def..050aaba 100644
--- a/src/shared/gatt-server.c
+++ b/src/shared/gatt-server.c
@@ -34,16 +34,80 @@
 #define MAX(a, b) ((a) > (b) ? (a) : (b))
 #endif
 
+#ifndef MIN
+#define MIN(a, b) ((a) < (b) ? (a) : (b))
+#endif
+
 struct bt_gatt_server {
 	struct gatt_db *db;
 	struct bt_att *att;
 	int ref_count;
 	uint16_t mtu;
+
+	unsigned int mtu_id;
+
+	bt_gatt_server_debug_func_t debug_callback;
+	bt_gatt_server_destroy_func_t debug_destroy;
+	void *debug_data;
 };
 
+static void gatt_server_unregister_handlers(struct bt_gatt_server *server)
+{
+	bt_att_unregister(server->att, server->mtu_id);
+}
+
+static void gatt_server_cleanup(struct bt_gatt_server *server)
+{
+	gatt_server_unregister_handlers(server);
+	bt_att_unref(server->att);
+	server->att = NULL;
+}
+
+static void encode_error_rsp(uint8_t opcode, uint16_t handle, uint8_t ecode,
+								uint8_t pdu[4])
+{
+	pdu[0] = opcode;
+	pdu[3] = ecode;
+	put_le16(handle, pdu + 1);
+}
+
+static void exchange_mtu_cb(uint8_t opcode, const void *pdu,
+					uint16_t length, void *user_data)
+{
+	struct bt_gatt_server *server = user_data;
+	uint16_t client_rx_mtu;
+	uint16_t final_mtu;
+	uint8_t rsp_pdu[4];
+
+	if (length != 2) {
+		encode_error_rsp(opcode, 0, BT_ATT_ERROR_INVALID_PDU, rsp_pdu);
+		bt_att_send(server->att, BT_ATT_OP_ERROR_RSP, rsp_pdu,
+					sizeof(rsp_pdu), NULL, NULL, NULL);
+		return;
+	}
+
+	client_rx_mtu = get_le16(pdu);
+	final_mtu = MAX(MIN(client_rx_mtu, server->mtu), BT_ATT_DEFAULT_LE_MTU);
+
+	/* Respond with the server MTU */
+	put_le16(server->mtu, rsp_pdu);
+	bt_att_send(server->att, BT_ATT_OP_MTU_RSP, rsp_pdu, 2, NULL, NULL,
+									NULL);
+
+	/* Set the bt_att transport MTU */
+	server->mtu = final_mtu;
+	bt_att_set_mtu(server->att, final_mtu);
+}
+
 static bool gatt_server_register_att_handlers(struct bt_gatt_server *server)
 {
-	/* TODO */
+	/* EXCHANGE MTU request */
+	server->mtu_id = bt_att_register(server->att, BT_ATT_OP_MTU_REQ,
+								exchange_mtu_cb,
+								server, NULL);
+	if (!server->mtu_id)
+		return false;
+
 	return true;
 }
 
-- 
2.1.0.rc2.206.gedb03e5

Re: [PATCH BlueZ 2/3] shared/gatt-server: Introduce shared/gatt-server.

[Luiz Augusto von Dentz]

Hi Arman,

On Sat, Oct 4, 2014 at 1:39 AM, Arman Uguray <armansito@chromium.org> wrote:
> This patch introduces bt_gatt_server which will implement the server-side of the
> ATT protocol over a bt_att structure and construct responses based on a gatt_db
> structure.
> ---
>  Makefile.am              |   1 +
>  src/shared/gatt-server.c | 155 +++++++++++++++++++++++++++++++++++++++++++++++
>  src/shared/gatt-server.h |  47 ++++++++++++++
>  3 files changed, 203 insertions(+)
>  create mode 100644 src/shared/gatt-server.c
>  create mode 100644 src/shared/gatt-server.h
>
> diff --git a/Makefile.am b/Makefile.am
> index 1a1a43f..2dfea28 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -114,6 +114,7 @@ shared_sources = src/shared/io.h src/shared/timeout.h \
>                         src/shared/att.h src/shared/att.c \
>                         src/shared/gatt-helpers.h src/shared/gatt-helpers.c \
>                         src/shared/gatt-client.h src/shared/gatt-client.c \
> +                       src/shared/gatt-server.h src/shared/gatt-server.c \
>                         src/shared/gatt-db.h src/shared/gatt-db.c \
>                         src/shared/gap.h src/shared/gap.c
>
> diff --git a/src/shared/gatt-server.c b/src/shared/gatt-server.c
> new file mode 100644
> index 0000000..8e32def
> --- /dev/null
> +++ b/src/shared/gatt-server.c
> @@ -0,0 +1,155 @@
> +/*
> + *
> + *  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 "src/shared/att.h"
> +#include "lib/uuid.h"
> +#include "src/shared/queue.h"
> +#include "src/shared/gatt-db.h"
> +#include "src/shared/gatt-server.h"
> +#include "src/shared/gatt-helpers.h"
> +#include "src/shared/att-types.h"
> +#include "src/shared/util.h"
> +
> +#ifndef MAX
> +#define MAX(a, b) ((a) > (b) ? (a) : (b))
> +#endif
> +
> +struct bt_gatt_server {
> +       struct gatt_db *db;
> +       struct bt_att *att;
> +       int ref_count;
> +       uint16_t mtu;
> +};
> +
> +static bool gatt_server_register_att_handlers(struct bt_gatt_server *server)
> +{
> +       /* TODO */
> +       return true;
> +}
> +
> +struct bt_gatt_server *bt_gatt_server_new(struct bt_att *att,
> +                                                       uint16_t mtu)
> +{
> +       struct bt_gatt_server *server;
> +
> +       if (!att)
> +               return NULL;
> +
> +       server = new0(struct bt_gatt_server, 1);
> +       if (!server)
> +               return NULL;
> +
> +       server->db = gatt_db_new();
> +       if (!server->db) {
> +               free(server);
> +               return NULL;
> +       }
> +
> +       server->att = bt_att_ref(att);
> +       server->mtu = MAX(mtu, BT_ATT_DEFAULT_LE_MTU);
> +
> +       if (!gatt_server_register_att_handlers(server)) {
> +               gatt_db_destroy(server->db);
> +               bt_att_unref(att);
> +               free(server);
> +               return NULL;
> +       }
> +
> +       return bt_gatt_server_ref(server);
> +}
> +
> +struct bt_gatt_server *bt_gatt_server_ref(struct bt_gatt_server *server)
> +{
> +       if (!server)
> +               return NULL;
> +
> +       __sync_fetch_and_add(&server->ref_count, 1);
> +
> +       return server;
> +}
> +
> +void bt_gatt_server_unref(struct bt_gatt_server *server)
> +{
> +       if (__sync_sub_and_fetch(&server->ref_count, 1))
> +               return;
> +
> +       if (server->debug_destroy)
> +               server->debug_destroy(server->debug_data);
> +
> +       gatt_server_cleanup(server);
> +       gatt_db_destroy(server->db);
> +
> +       free(server);
> +}

Have a function dedicated just to free so in case bt_gatt_server_new
fails you just call it.

> +bool bt_gatt_server_set_debug(struct bt_gatt_server *server,
> +                                       bt_gatt_server_debug_func_t callback,
> +                                       void *user_data,
> +                                       bt_gatt_server_destroy_func_t destroy)
> +{
> +       if (!server)
> +               return false;
> +
> +       if (server->debug_destroy)
> +               server->debug_destroy(server->debug_data);
> +
> +       server->debug_callback = callback;
> +       server->debug_destroy = destroy;
> +       server->debug_data = user_data;
> +
> +       return true;
> +}
> +
> +uint16_t bt_gatt_server_add_service(struct bt_gatt_server *server,
> +                                       const bt_uuid_t *uuid, bool primary,
> +                                       uint16_t num_handles)
> +{
> +       if (!server)
> +               return 0;
> +
> +       /* TODO: Send service changed signal here if necessary */
> +
> +       return gatt_db_add_service(server->db, uuid, primary, num_handles);
> +}
> +
> +bool bt_gatt_server_remove_service(struct bt_gatt_server *server,
> +                                                       uint16_t handle)
> +{
> +       if (!server)
> +               return false;
> +
> +       /* TODO: Send service changed signal here if necessary */
> +
> +       return gatt_db_remove_service(server->db, handle);
> +}
> +
> +bool bt_gatt_server_service_set_active(struct bt_gatt_server *server,
> +                                               uint16_t handle, bool active)
> +{
> +       if (!server)
> +               return false;
> +
> +       /* TODO: Send service changed signal here if necessary */
> +
> +       return gatt_db_service_set_active(server->db, handle, active);
> +}
> diff --git a/src/shared/gatt-server.h b/src/shared/gatt-server.h
> new file mode 100644
> index 0000000..0027715
> --- /dev/null
> +++ b/src/shared/gatt-server.h
> @@ -0,0 +1,47 @@
> +/*
> + *
> + *  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 <stdint.h>
> +
> +struct bt_gatt_server;
> +
> +struct bt_gatt_server *bt_gatt_server_new(struct bt_att *att, uint16_t mtu);
> +
> +struct bt_gatt_server *bt_gatt_server_ref(struct bt_gatt_server *server);
> +void bt_gatt_server_unref(struct bt_gatt_server *server);
> +
> +typedef void (*bt_gatt_server_destroy_func_t)(void *user_data);
> +typedef void (*bt_gatt_server_debug_func_t)(const char *str, void *user_data);
> +
> +bool bt_gatt_server_set_debug(struct bt_gatt_server *server,
> +                                       bt_gatt_server_debug_func_t callback,
> +                                       void *user_data,
> +                                       bt_gatt_server_destroy_func_t destroy);
> +
> +uint16_t bt_gatt_server_add_service(struct bt_gatt_server *server,
> +                                       const bt_uuid_t *uuid, bool primary,
> +                                       uint16_t num_handles);
> +bool bt_gatt_server_remove_service(struct bt_gatt_server *server,
> +                                                       uint16_t handle);
> +bool bt_gatt_server_service_set_active(struct bt_gatt_server *server,
> +                                               uint16_t handle, bool active);
> --
> 2.1.0.rc2.206.gedb03e5

I think it is better to be able to attach/detach and existing db
object because otherwise we will need to reconstruct it on every
connection, furthermore the db might exist anyway since we need to be
able to register services even when we are not connected.

-- 
Luiz Augusto von Dentz

Re: [PATCH BlueZ 2/3] shared/gatt-server: Introduce shared/gatt-server.

[Arman Uguray]

Hi Luiz,

> I think it is better to be able to attach/detach and existing db
> object because otherwise we will need to reconstruct it on every
> connection, furthermore the db might exist anyway since we need to be
> able to register services even when we are not connected.
>

Yeah, I did it this way for the reasons I explained in the cover
letter: I basically want to keep the layering related to ATT protocol
within bt_gatt_server, however the way gatt-db is currently structured
makes it difficult. This particularly arises from the generic
read/write handlers, where an attribute value is stored outside of the
database and fetched via a callback.

When bt_gatt_server receives a request, say Read By Group Type, it
calls gatt_db_read_by_group_type and then calls gatt_db_read for each
attribute handle returned. If there's no value stored in the database
and the value should be obtained elsewhere, then gatt-db invokes the
registered read callback. That read callback needs to eventually make
the value known to bt_gatt_server.

So my initial thought was to add an extra layer of callbacks between
bt_gatt_server and make bt_gatt_server the global server database that
can handle multiple bt_att instances that can be attached/detached on
demand. I think I could go back to the way I had it before, have
bt_gatt_server accept a gatt-db in its constructor, and perhaps handle
a special function for the read handling, such as a
bt_gatt_server_set_read_value_for_handle of sorts that can get called
from the read handler and that helps bt_gatt_server construct the ATT
protocol response, but still there are problems such as how the
generic gatt-db read handler will know which bt_gatt_server instance
to call this function on (as there can be several bt_gatt_server
instances for each device that acts simultaneously as a client and a
server). The way things currently are, you can't reliably pass this
information as user_data in the read callback, i.e. there is currently
no way to say which bt_gatt_server the request originated from. If
gatt-db is internal to bt_gatt_server and there are wrappers for
functions such as gatt_db_add_characteristic in the bt_gatt_server_*
namespace, then we can hook callbacks up in a way that handles this.

So maybe it's better to have a single bt_gatt_server that exists for
the lifetime of the application and for each new connection we create
a bt_att and attach it to bt_gatt_server. Or, we go back to what we
originally had in mind, i.e. one gatt-db and multiple per-connection
bt_gatt_server instances but we will have to rework gatt-db a little
for this. Note that this hasn't been a problem for the android code,
since everything is implemented in one android/gatt layer, hence
gatt-db is inherently internal to the transport handling.

Cheers,
Arman

Re: [PATCH BlueZ 2/3] shared/gatt-server: Introduce shared/gatt-server.

[Luiz Augusto von Dentz]

Hi Arman,

On Mon, Oct 6, 2014 at 4:15 PM, Arman Uguray <armansito@chromium.org> wrote:
> Hi Luiz,
>
>> I think it is better to be able to attach/detach and existing db
>> object because otherwise we will need to reconstruct it on every
>> connection, furthermore the db might exist anyway since we need to be
>> able to register services even when we are not connected.
>>
>
> Yeah, I did it this way for the reasons I explained in the cover
> letter: I basically want to keep the layering related to ATT protocol
> within bt_gatt_server, however the way gatt-db is currently structured
> makes it difficult. This particularly arises from the generic
> read/write handlers, where an attribute value is stored outside of the
> database and fetched via a callback.
>
> When bt_gatt_server receives a request, say Read By Group Type, it
> calls gatt_db_read_by_group_type and then calls gatt_db_read for each
> attribute handle returned. If there's no value stored in the database
> and the value should be obtained elsewhere, then gatt-db invokes the
> registered read callback. That read callback needs to eventually make
> the value known to bt_gatt_server.

Yep, I realize this is a bit messed up because even though
gatt_db_read can be asynchronous it does not take a callback, worse it
takes a bdaddr (most likely inspired by Android HAL)?

> So my initial thought was to add an extra layer of callbacks between
> bt_gatt_server and make bt_gatt_server the global server database that
> can handle multiple bt_att instances that can be attached/detached on
> demand. I think I could go back to the way I had it before, have
> bt_gatt_server accept a gatt-db in its constructor, and perhaps handle
> a special function for the read handling, such as a
> bt_gatt_server_set_read_value_for_handle of sorts that can get called
> from the read handler and that helps bt_gatt_server construct the ATT
> protocol response, but still there are problems such as how the
> generic gatt-db read handler will know which bt_gatt_server instance
> to call this function on (as there can be several bt_gatt_server
> instances for each device that acts simultaneously as a client and a
> server). The way things currently are, you can't reliably pass this
> information as user_data in the read callback, i.e. there is currently
> no way to say which bt_gatt_server the request originated from. If
> gatt-db is internal to bt_gatt_server and there are wrappers for
> functions such as gatt_db_add_characteristic in the bt_gatt_server_*
> namespace, then we can hook callbacks up in a way that handles this.

Id say we need to pass a callback along with a userdata,
gatt_db_read_t can probably take a the callback and another user_data
associated with it so when it is completed it should be called.

> So maybe it's better to have a single bt_gatt_server that exists for
> the lifetime of the application and for each new connection we create
> a bt_att and attach it to bt_gatt_server. Or, we go back to what we
> originally had in mind, i.e. one gatt-db and multiple per-connection
> bt_gatt_server instances but we will have to rework gatt-db a little
> for this. Note that this hasn't been a problem for the android code,
> since everything is implemented in one android/gatt layer, hence
> gatt-db is inherently internal to the transport handling.

Not sure how having bt_gatt_server not per connection would help here,
the problem seems much more related to asynchronous calls, be it
gatt_db or bt_gatt_server it does not seem to change anything you
would still have to create a context that tracks the originating
bt_att to send the response to, Android code seem to be building the
response itself which I think it is to be in someplace else.

-- 
Luiz Augusto von Dentz

Re: [PATCH BlueZ 2/3] shared/gatt-server: Introduce shared/gatt-server.

[Marcel Holtmann]

Hi Luiz,

>>> I think it is better to be able to attach/detach and existing db
>>> object because otherwise we will need to reconstruct it on every
>>> connection, furthermore the db might exist anyway since we need to be
>>> able to register services even when we are not connected.
>>> 
>> 
>> Yeah, I did it this way for the reasons I explained in the cover
>> letter: I basically want to keep the layering related to ATT protocol
>> within bt_gatt_server, however the way gatt-db is currently structured
>> makes it difficult. This particularly arises from the generic
>> read/write handlers, where an attribute value is stored outside of the
>> database and fetched via a callback.
>> 
>> When bt_gatt_server receives a request, say Read By Group Type, it
>> calls gatt_db_read_by_group_type and then calls gatt_db_read for each
>> attribute handle returned. If there's no value stored in the database
>> and the value should be obtained elsewhere, then gatt-db invokes the
>> registered read callback. That read callback needs to eventually make
>> the value known to bt_gatt_server.
> 
> Yep, I realize this is a bit messed up because even though
> gatt_db_read can be asynchronous it does not take a callback, worse it
> takes a bdaddr (most likely inspired by Android HAL)?
> 
>> So my initial thought was to add an extra layer of callbacks between
>> bt_gatt_server and make bt_gatt_server the global server database that
>> can handle multiple bt_att instances that can be attached/detached on
>> demand. I think I could go back to the way I had it before, have
>> bt_gatt_server accept a gatt-db in its constructor, and perhaps handle
>> a special function for the read handling, such as a
>> bt_gatt_server_set_read_value_for_handle of sorts that can get called
>> from the read handler and that helps bt_gatt_server construct the ATT
>> protocol response, but still there are problems such as how the
>> generic gatt-db read handler will know which bt_gatt_server instance
>> to call this function on (as there can be several bt_gatt_server
>> instances for each device that acts simultaneously as a client and a
>> server). The way things currently are, you can't reliably pass this
>> information as user_data in the read callback, i.e. there is currently
>> no way to say which bt_gatt_server the request originated from. If
>> gatt-db is internal to bt_gatt_server and there are wrappers for
>> functions such as gatt_db_add_characteristic in the bt_gatt_server_*
>> namespace, then we can hook callbacks up in a way that handles this.
> 
> Id say we need to pass a callback along with a userdata,
> gatt_db_read_t can probably take a the callback and another user_data
> associated with it so when it is completed it should be called.
> 
>> So maybe it's better to have a single bt_gatt_server that exists for
>> the lifetime of the application and for each new connection we create
>> a bt_att and attach it to bt_gatt_server. Or, we go back to what we
>> originally had in mind, i.e. one gatt-db and multiple per-connection
>> bt_gatt_server instances but we will have to rework gatt-db a little
>> for this. Note that this hasn't been a problem for the android code,
>> since everything is implemented in one android/gatt layer, hence
>> gatt-db is inherently internal to the transport handling.
> 
> Not sure how having bt_gatt_server not per connection would help here,
> the problem seems much more related to asynchronous calls, be it
> gatt_db or bt_gatt_server it does not seem to change anything you
> would still have to create a context that tracks the originating
> bt_att to send the response to, Android code seem to be building the
> response itself which I think it is to be in someplace else.

there is one other thing that needs to be considered. Accessing the GATT database must be possible over LE and BR/EDR. After recent discussion, it is considered that you can discover over either transport and the results should be always the same. Meaning whatever is exposed over LE must also be exposed over BR/EDR and vice versa.

This is leads to the fact that atomic operation must stay atomic. So in theory a BR/EDR operation can delay a LE operation and the other way around. This is something we have to carefully design especially when writing to a handle or updating certain values.

Regards

Marcel

Re: [PATCH BlueZ 2/3] shared/gatt-server: Introduce shared/gatt-server.

[Arman Uguray]

Hi Luiz,

>>> I think it is better to be able to attach/detach and existing db
>>> object because otherwise we will need to reconstruct it on every
>>> connection, furthermore the db might exist anyway since we need to be
>>> able to register services even when we are not connected.
>>>
>>
>> Yeah, I did it this way for the reasons I explained in the cover
>> letter: I basically want to keep the layering related to ATT protocol
>> within bt_gatt_server, however the way gatt-db is currently structured
>> makes it difficult. This particularly arises from the generic
>> read/write handlers, where an attribute value is stored outside of the
>> database and fetched via a callback.
>>
>> When bt_gatt_server receives a request, say Read By Group Type, it
>> calls gatt_db_read_by_group_type and then calls gatt_db_read for each
>> attribute handle returned. If there's no value stored in the database
>> and the value should be obtained elsewhere, then gatt-db invokes the
>> registered read callback. That read callback needs to eventually make
>> the value known to bt_gatt_server.
>
> Yep, I realize this is a bit messed up because even though
> gatt_db_read can be asynchronous it does not take a callback, worse it
> takes a bdaddr (most likely inspired by Android HAL)?
>

Yeah, it looks like it optionally takes in a bdaddr so that it can
determine the security level of the connection for attribute
permission checks. This doesn't work very well in the shared/ case,
since the transport is generic.


>> So my initial thought was to add an extra layer of callbacks between
>> bt_gatt_server and make bt_gatt_server the global server database that
>> can handle multiple bt_att instances that can be attached/detached on
>> demand. I think I could go back to the way I had it before, have
>> bt_gatt_server accept a gatt-db in its constructor, and perhaps handle
>> a special function for the read handling, such as a
>> bt_gatt_server_set_read_value_for_handle of sorts that can get called
>> from the read handler and that helps bt_gatt_server construct the ATT
>> protocol response, but still there are problems such as how the
>> generic gatt-db read handler will know which bt_gatt_server instance
>> to call this function on (as there can be several bt_gatt_server
>> instances for each device that acts simultaneously as a client and a
>> server). The way things currently are, you can't reliably pass this
>> information as user_data in the read callback, i.e. there is currently
>> no way to say which bt_gatt_server the request originated from. If
>> gatt-db is internal to bt_gatt_server and there are wrappers for
>> functions such as gatt_db_add_characteristic in the bt_gatt_server_*
>> namespace, then we can hook callbacks up in a way that handles this.
>
> Id say we need to pass a callback along with a userdata,
> gatt_db_read_t can probably take a the callback and another user_data
> associated with it so when it is completed it should be called.
>

Yep, this is exactly what I had in mind. The problem though is that
somebody still needs to pass in that callback. So maybe we could
change gatt_db_read to accept an additional callback/user_data which
would then get forwarded to the registered gatt_db_read_t. That would
definitely fix the problem here. It would also not break the android
code, since it can already work without invoking the callback. I'll
experiment with this.


>> So maybe it's better to have a single bt_gatt_server that exists for
>> the lifetime of the application and for each new connection we create
>> a bt_att and attach it to bt_gatt_server. Or, we go back to what we
>> originally had in mind, i.e. one gatt-db and multiple per-connection
>> bt_gatt_server instances but we will have to rework gatt-db a little
>> for this. Note that this hasn't been a problem for the android code,
>> since everything is implemented in one android/gatt layer, hence
>> gatt-db is inherently internal to the transport handling.
>
> Not sure how having bt_gatt_server not per connection would help here,
> the problem seems much more related to asynchronous calls, be it
> gatt_db or bt_gatt_server it does not seem to change anything you
> would still have to create a context that tracks the originating
> bt_att to send the response to, Android code seem to be building the
> response itself which I think it is to be in someplace else.

Exactly, if we had a single bt_gatt_server, that bt_gatt_server would
internally have to manage many instances of bt_att. For now I'll steer
away from this idea and see how I can make the asynchronous calls work
via an additional callback.

Cheers,
Arman

Re: [PATCH BlueZ 2/3] shared/gatt-server: Introduce shared/gatt-server.

[Marcin Kraglak]

Hi Arman,

On 6 October 2014 20:03, Arman Uguray <armansito@google.com> wrote:
> Hi Luiz,
>
>>>> I think it is better to be able to attach/detach and existing db
>>>> object because otherwise we will need to reconstruct it on every
>>>> connection, furthermore the db might exist anyway since we need to be
>>>> able to register services even when we are not connected.
>>>>
>>>
>>> Yeah, I did it this way for the reasons I explained in the cover
>>> letter: I basically want to keep the layering related to ATT protocol
>>> within bt_gatt_server, however the way gatt-db is currently structured
>>> makes it difficult. This particularly arises from the generic
>>> read/write handlers, where an attribute value is stored outside of the
>>> database and fetched via a callback.
>>>
>>> When bt_gatt_server receives a request, say Read By Group Type, it
>>> calls gatt_db_read_by_group_type and then calls gatt_db_read for each
>>> attribute handle returned. If there's no value stored in the database
>>> and the value should be obtained elsewhere, then gatt-db invokes the
>>> registered read callback. That read callback needs to eventually make
>>> the value known to bt_gatt_server.
>>
>> Yep, I realize this is a bit messed up because even though
>> gatt_db_read can be asynchronous it does not take a callback, worse it
>> takes a bdaddr (most likely inspired by Android HAL)?
>>
>
> Yeah, it looks like it optionally takes in a bdaddr so that it can
> determine the security level of the connection for attribute
> permission checks. This doesn't work very well in the shared/ case,
> since the transport is generic.
>
>
>>> So my initial thought was to add an extra layer of callbacks between
>>> bt_gatt_server and make bt_gatt_server the global server database that
>>> can handle multiple bt_att instances that can be attached/detached on
>>> demand. I think I could go back to the way I had it before, have
>>> bt_gatt_server accept a gatt-db in its constructor, and perhaps handle
>>> a special function for the read handling, such as a
>>> bt_gatt_server_set_read_value_for_handle of sorts that can get called
>>> from the read handler and that helps bt_gatt_server construct the ATT
>>> protocol response, but still there are problems such as how the
>>> generic gatt-db read handler will know which bt_gatt_server instance
>>> to call this function on (as there can be several bt_gatt_server
>>> instances for each device that acts simultaneously as a client and a
>>> server). The way things currently are, you can't reliably pass this
>>> information as user_data in the read callback, i.e. there is currently
>>> no way to say which bt_gatt_server the request originated from. If
>>> gatt-db is internal to bt_gatt_server and there are wrappers for
>>> functions such as gatt_db_add_characteristic in the bt_gatt_server_*
>>> namespace, then we can hook callbacks up in a way that handles this.
>>
>> Id say we need to pass a callback along with a userdata,
>> gatt_db_read_t can probably take a the callback and another user_data
>> associated with it so when it is completed it should be called.
>>
>
> Yep, this is exactly what I had in mind. The problem though is that
> somebody still needs to pass in that callback. So maybe we could
> change gatt_db_read to accept an additional callback/user_data which
> would then get forwarded to the registered gatt_db_read_t. That would
> definitely fix the problem here. It would also not break the android
> code, since it can already work without invoking the callback. I'll
> experiment with this.

I thought about simple callout, like gatt_db_result().Gatt_db can call
read callback with
specific data, like gatt_db_transaction_data, then, once app, server
or anything will complete
read or write operation, just simply call gatt_db_result() with
previously passed transaction data.

>
>
>>> So maybe it's better to have a single bt_gatt_server that exists for
>>> the lifetime of the application and for each new connection we create
>>> a bt_att and attach it to bt_gatt_server. Or, we go back to what we
>>> originally had in mind, i.e. one gatt-db and multiple per-connection
>>> bt_gatt_server instances but we will have to rework gatt-db a little
>>> for this. Note that this hasn't been a problem for the android code,
>>> since everything is implemented in one android/gatt layer, hence
>>> gatt-db is inherently internal to the transport handling.
>>
>> Not sure how having bt_gatt_server not per connection would help here,
>> the problem seems much more related to asynchronous calls, be it
>> gatt_db or bt_gatt_server it does not seem to change anything you
>> would still have to create a context that tracks the originating
>> bt_att to send the response to, Android code seem to be building the
>> response itself which I think it is to be in someplace else.
>
> Exactly, if we had a single bt_gatt_server, that bt_gatt_server would
> internally have to manage many instances of bt_att. For now I'll steer
> away from this idea and see how I can make the asynchronous calls work
> via an additional callback.
>
> Cheers,
> Arman

BR
Marcin

> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH BlueZ 2/3] shared/gatt-server: Introduce shared/gatt-server.

[Arman Uguray]

Hi Marcin,


>> Yep, this is exactly what I had in mind. The problem though is that
>> somebody still needs to pass in that callback. So maybe we could
>> change gatt_db_read to accept an additional callback/user_data which
>> would then get forwarded to the registered gatt_db_read_t. That would
>> definitely fix the problem here. It would also not break the android
>> code, since it can already work without invoking the callback. I'll
>> experiment with this.
>
> I thought about simple callout, like gatt_db_result().Gatt_db can call
> read callback with
> specific data, like gatt_db_transaction_data, then, once app, server
> or anything will complete
> read or write operation, just simply call gatt_db_result() with
> previously passed transaction data.
>

That makes sense. For now, I'm leaving the callback out of the first
patch set. I'll think about this a little more and upload something
later this week.

Thanks,
Arman