[PATCH v3 0/8] Add support for HandsfreeGateway to Media

[PATCH v3 0/8] Add support for HandsfreeGateway to Media

[Frédéric Dalleau]

Hi,

The first patch in this series fix the issue remaining in v2.
It also fix a crash because the request_stream callback was run synchronously.
After that various fixes and cleanup.

Best regards,
Frédéric

Frédéric Dalleau (8):
  Fix disconnect SCO at same time than RFCOMM
  Fix asynchronously run request stream cb
  Cancel pending callback if stream is canceled
  Fix state to "playing" on SCO establishment
  Set state to "connecting" on connection requested
  Use int instead of GError into callbacks
  Asynchronously run gateway_config_stream cb
  Remove occurences of sco_start_cb

 audio/gateway.c   |  144 +++++++++++++++++++++++++++++++++++-----------------
 audio/gateway.h   |    6 +-
 audio/transport.c |    4 +-
 audio/unix.c      |   21 +++-----
 4 files changed, 110 insertions(+), 65 deletions(-)

[PATCH v3 1/8] Fix disconnect SCO at same time than RFCOMM

[Frédéric Dalleau]

If RFCOMM disconnects, SCO should be disconnected too.
---
 audio/gateway.c |    7 +------
 1 files changed, 1 insertions(+), 6 deletions(-)

diff --git a/audio/gateway.c b/audio/gateway.c
index 142b12e..59c91dd 100644
--- a/audio/gateway.c
+++ b/audio/gateway.c
@@ -222,15 +222,10 @@ static void sco_connect_cb(GIOChannel *chan, GError *err, gpointer user_data)
 static gboolean rfcomm_disconnect_cb(GIOChannel *chan, GIOCondition cond,
 			struct audio_device *dev)
 {
-	struct gateway *gw = dev->gateway;
-
 	if (cond & G_IO_NVAL)
 		return FALSE;
 
-	g_io_channel_shutdown(gw->rfcomm, TRUE, NULL);
-	g_io_channel_unref(gw->rfcomm);
-	gw->rfcomm = NULL;
-	change_state(dev, GATEWAY_STATE_DISCONNECTED);
+	gateway_close(dev);
 
 	return FALSE;
 }
-- 
1.7.1

[PATCH v3 2/8] Fix asynchronously run request stream cb

[Frédéric Dalleau]

---
 audio/gateway.c |   68 +++++++++++++++++++++++++++++++++++++++++++++++-------
 audio/gateway.h |    2 +-
 audio/unix.c    |    6 +---
 3 files changed, 62 insertions(+), 14 deletions(-)

diff --git a/audio/gateway.c b/audio/gateway.c
index 59c91dd..8cbaeb4 100644
--- a/audio/gateway.c
+++ b/audio/gateway.c
@@ -60,6 +60,12 @@ struct hf_agent {
 	guint watch;	/* Disconnect watch */
 };
 
+struct connect_cb {
+	unsigned int id;
+	gateway_stream_cb_t cb;
+	void *cb_data;
+};
+
 struct gateway {
 	gateway_state_t state;
 	GIOChannel *rfcomm;
@@ -67,6 +73,7 @@ struct gateway {
 	GIOChannel *incoming;
 	gateway_stream_cb_t sco_start_cb;
 	void *sco_start_cb_data;
+	GSList *callbacks;
 	struct hf_agent *agent;
 	DBusMessage *msg;
 	int version;
@@ -180,6 +187,41 @@ static gboolean agent_sendfd(struct hf_agent *agent, int fd,
 	return TRUE;
 }
 
+static unsigned int connect_cb_new(struct gateway *gw,
+					gateway_stream_cb_t func,
+					void *user_data)
+{
+	struct connect_cb *cb;
+	static unsigned int free_cb_id = 1;
+
+	if (!func)
+		return 0;
+
+	cb = g_new(struct connect_cb, 1);
+
+	cb->cb = func;
+	cb->cb_data = user_data;
+	cb->id = free_cb_id++;
+
+	gw->callbacks = g_slist_append(gw->callbacks, cb);
+
+	return cb->id;
+}
+
+static void run_connect_cb(struct audio_device *dev, GError *err)
+{
+	struct gateway *gw = dev->gateway;
+	GSList *l;
+
+	for (l = gw->callbacks; l != NULL; l = l->next) {
+		struct connect_cb *cb = l->data;
+		cb->cb(dev, err, cb->cb_data);
+	}
+
+	g_slist_free_full(gw->callbacks, g_free);
+	gw->callbacks = NULL;
+}
+
 static gboolean sco_io_cb(GIOChannel *chan, GIOCondition cond,
 			struct audio_device *dev)
 {
@@ -209,6 +251,8 @@ static void sco_connect_cb(GIOChannel *chan, GError *err, gpointer user_data)
 	if (gw->sco_start_cb)
 		gw->sco_start_cb(dev, err, gw->sco_start_cb_data);
 
+	run_connect_cb(dev, err);
+
 	if (err) {
 		error("sco_connect_cb(): %s", err->message);
 		gateway_close(dev);
@@ -759,22 +803,27 @@ void gateway_start_service(struct audio_device *dev)
 	}
 }
 
+static gboolean request_stream_cb(gpointer data)
+{
+	run_connect_cb(data, NULL);
+	return FALSE;
+}
+
 /* These are functions to be called from unix.c for audio system
  * ifaces (alsa, gstreamer, etc.) */
-gboolean gateway_request_stream(struct audio_device *dev,
+unsigned int gateway_request_stream(struct audio_device *dev,
 				gateway_stream_cb_t cb, void *user_data)
 {
 	struct gateway *gw = dev->gateway;
+	unsigned int id;
 	GError *err = NULL;
 	GIOChannel *io;
 
+	id = connect_cb_new(gw, cb, user_data);
+
 	if (!gw->rfcomm) {
-		gw->sco_start_cb = cb;
-		gw->sco_start_cb_data = user_data;
 		get_records(dev);
 	} else if (!gw->sco) {
-		gw->sco_start_cb = cb;
-		gw->sco_start_cb_data = user_data;
 		io = bt_io_connect(BT_IO_SCO, sco_connect_cb, dev, NULL, &err,
 				BT_IO_OPT_SOURCE_BDADDR, &dev->src,
 				BT_IO_OPT_DEST_BDADDR, &dev->dst,
@@ -782,12 +831,13 @@ gboolean gateway_request_stream(struct audio_device *dev,
 		if (!io) {
 			error("%s", err->message);
 			g_error_free(err);
-			return FALSE;
+			return 0;
 		}
-	} else if (cb)
-		cb(dev, err, user_data);
+	} else {
+		g_idle_add(request_stream_cb, dev);
+	}
 
-	return TRUE;
+	return id;
 }
 
 int gateway_config_stream(struct audio_device *dev, gateway_stream_cb_t sco_cb,
diff --git a/audio/gateway.h b/audio/gateway.h
index 2dca32a..7012fc5 100644
--- a/audio/gateway.h
+++ b/audio/gateway.h
@@ -52,7 +52,7 @@ gboolean gateway_is_connected(struct audio_device *dev);
 int gateway_connect_rfcomm(struct audio_device *dev, GIOChannel *io);
 int gateway_connect_sco(struct audio_device *dev, GIOChannel *chan);
 void gateway_start_service(struct audio_device *device);
-gboolean gateway_request_stream(struct audio_device *dev,
+unsigned int gateway_request_stream(struct audio_device *dev,
 			gateway_stream_cb_t cb, void *user_data);
 int gateway_config_stream(struct audio_device *dev, gateway_stream_cb_t cb,
 			void *user_data);
diff --git a/audio/unix.c b/audio/unix.c
index 1e0ab30..678d097 100644
--- a/audio/unix.c
+++ b/audio/unix.c
@@ -1118,10 +1118,8 @@ static void start_resume(struct audio_device *dev, struct unix_client *client)
 		break;
 
 	case TYPE_GATEWAY:
-		if (gateway_request_stream(dev, gateway_resume_complete, client))
-			id = 1;
-		else
-			id = 0;
+		id = gateway_request_stream(dev, gateway_resume_complete,
+						client);
 		client->cancel = gateway_cancel_stream;
 		break;
 
-- 
1.7.1

[PATCH v3 3/8] Cancel pending callback if stream is canceled

[Frédéric Dalleau]

---
 audio/gateway.c |   20 ++++++++++++++++++++
 1 files changed, 20 insertions(+), 0 deletions(-)

diff --git a/audio/gateway.c b/audio/gateway.c
index 8cbaeb4..c5f21e6 100644
--- a/audio/gateway.c
+++ b/audio/gateway.c
@@ -859,7 +859,27 @@ int gateway_config_stream(struct audio_device *dev, gateway_stream_cb_t sco_cb,
 
 gboolean gateway_cancel_stream(struct audio_device *dev, unsigned int id)
 {
+	struct gateway *gw = dev->gateway;
+	GSList *l;
+	struct connect_cb *cb = NULL;
+
+	for (l = gw->callbacks; l != NULL; l = l->next) {
+		struct connect_cb *tmp = l->data;
+
+		if (tmp->id == id) {
+			cb = tmp;
+			break;
+		}
+	}
+
+	if (!cb)
+		return FALSE;
+
+	gw->callbacks = g_slist_remove(gw->callbacks, cb);
+	g_free(cb);
+
 	gateway_suspend_stream(dev);
+
 	return TRUE;
 }
 
-- 
1.7.1

[PATCH v3 4/8] Fix state to "playing" on SCO establishment

[Frédéric Dalleau]

---
 audio/gateway.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/audio/gateway.c b/audio/gateway.c
index c5f21e6..7bab77c 100644
--- a/audio/gateway.c
+++ b/audio/gateway.c
@@ -261,6 +261,8 @@ static void sco_connect_cb(GIOChannel *chan, GError *err, gpointer user_data)
 
 	g_io_add_watch(gw->sco, G_IO_ERR | G_IO_HUP | G_IO_NVAL,
 				(GIOFunc) sco_io_cb, dev);
+
+	change_state(dev, GATEWAY_STATE_PLAYING);
 }
 
 static gboolean rfcomm_disconnect_cb(GIOChannel *chan, GIOCondition cond,
-- 
1.7.1

[PATCH v3 5/8] Set state to "connecting" on connection requested

[Frédéric Dalleau]

This change will become necessary when
integrating the Audio interface which rely on
state change to confirm that connection
has started successfully.
---
 audio/gateway.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/audio/gateway.c b/audio/gateway.c
index 7bab77c..353473b 100644
--- a/audio/gateway.c
+++ b/audio/gateway.c
@@ -503,8 +503,6 @@ static void get_record_cb(sdp_list_t *recs, int err, gpointer user_data)
 	}
 
 	g_io_channel_unref(io);
-
-	change_state(dev, GATEWAY_STATE_CONNECTING);
 	return;
 
 fail:
@@ -530,6 +528,7 @@ static int get_records(struct audio_device *device)
 {
 	uuid_t uuid;
 
+	change_state(device, GATEWAY_STATE_CONNECTING);
 	sdp_uuid16_create(&uuid, HANDSFREE_AGW_SVCLASS_ID);
 	return bt_search_service(&device->src, &device->dst, &uuid,
 				get_record_cb, device, NULL);
-- 
1.7.1

[PATCH v3 6/8] Use int instead of GError into callbacks

[Frédéric Dalleau]

---
 audio/gateway.c   |   20 ++++++++++----------
 audio/gateway.h   |    4 ++--
 audio/transport.c |    4 ++--
 audio/unix.c      |    8 ++++----
 4 files changed, 18 insertions(+), 18 deletions(-)

diff --git a/audio/gateway.c b/audio/gateway.c
index 353473b..93ff4b9 100644
--- a/audio/gateway.c
+++ b/audio/gateway.c
@@ -208,7 +208,7 @@ static unsigned int connect_cb_new(struct gateway *gw,
 	return cb->id;
 }
 
-static void run_connect_cb(struct audio_device *dev, GError *err)
+static void run_connect_cb(struct audio_device *dev, int err)
 {
 	struct gateway *gw = dev->gateway;
 	GSList *l;
@@ -248,11 +248,6 @@ static void sco_connect_cb(GIOChannel *chan, GError *err, gpointer user_data)
 
 	gw->sco = g_io_channel_ref(chan);
 
-	if (gw->sco_start_cb)
-		gw->sco_start_cb(dev, err, gw->sco_start_cb_data);
-
-	run_connect_cb(dev, err);
-
 	if (err) {
 		error("sco_connect_cb(): %s", err->message);
 		gateway_close(dev);
@@ -263,6 +258,11 @@ static void sco_connect_cb(GIOChannel *chan, GError *err, gpointer user_data)
 				(GIOFunc) sco_io_cb, dev);
 
 	change_state(dev, GATEWAY_STATE_PLAYING);
+
+	if (gw->sco_start_cb)
+		gw->sco_start_cb(dev, 0, gw->sco_start_cb_data);
+
+	run_connect_cb(dev, 0);
 }
 
 static gboolean rfcomm_disconnect_cb(GIOChannel *chan, GIOCondition cond,
@@ -317,7 +317,7 @@ static void rfcomm_connect_cb(GIOChannel *chan, GError *err,
 	if (err) {
 		error("connect(): %s", err->message);
 		if (gw->sco_start_cb)
-			gw->sco_start_cb(dev, err, gw->sco_start_cb_data);
+			gw->sco_start_cb(dev, err->code, gw->sco_start_cb_data);
 		goto fail;
 	}
 
@@ -519,7 +519,7 @@ fail:
 				"connect: %s (%d)", strerror(-err), -err);
 
 	if (gw->sco_start_cb)
-		gw->sco_start_cb(dev, gerr, gw->sco_start_cb_data);
+		gw->sco_start_cb(dev, -err, gw->sco_start_cb_data);
 
 	g_error_free(gerr);
 }
@@ -806,7 +806,7 @@ void gateway_start_service(struct audio_device *dev)
 
 static gboolean request_stream_cb(gpointer data)
 {
-	run_connect_cb(data, NULL);
+	run_connect_cb(data, 0);
 	return FALSE;
 }
 
@@ -853,7 +853,7 @@ int gateway_config_stream(struct audio_device *dev, gateway_stream_cb_t sco_cb,
 	}
 
 	if (sco_cb)
-		sco_cb(dev, NULL, user_data);
+		sco_cb(dev, 0, user_data);
 
 	return 0;
 }
diff --git a/audio/gateway.h b/audio/gateway.h
index 7012fc5..20f90bd 100644
--- a/audio/gateway.h
+++ b/audio/gateway.h
@@ -42,8 +42,8 @@ typedef void (*gateway_state_cb) (struct audio_device *dev,
 					gateway_state_t old_state,
 					gateway_state_t new_state,
 					void *user_data);
-typedef void (*gateway_stream_cb_t) (struct audio_device *dev, GError *err,
-		void *user_data);
+typedef void (*gateway_stream_cb_t) (struct audio_device *dev, int err,
+					void *user_data);
 
 void gateway_set_state(struct audio_device *dev, gateway_state_t new_state);
 void gateway_unregister(struct audio_device *dev);
diff --git a/audio/transport.c b/audio/transport.c
index 2739199..54dabb6 100644
--- a/audio/transport.c
+++ b/audio/transport.c
@@ -437,7 +437,7 @@ static void cancel_headset(struct media_transport *transport, guint id)
 	headset_cancel_stream(transport->device, id);
 }
 
-static void gateway_resume_complete(struct audio_device *dev, GError *err,
+static void gateway_resume_complete(struct audio_device *dev, int err,
 							void *user_data)
 {
 	struct media_owner *owner = user_data;
@@ -453,7 +453,7 @@ static void gateway_resume_complete(struct audio_device *dev, GError *err,
 		goto fail;
 
 	if (err) {
-		error("Failed to resume gateway: error %s", err->message);
+		error("Failed to resume gateway: error %s", strerror(-err));
 		goto fail;
 	}
 
diff --git a/audio/unix.c b/audio/unix.c
index 678d097..111fc57 100644
--- a/audio/unix.c
+++ b/audio/unix.c
@@ -339,14 +339,14 @@ failed:
 	unix_ipc_error(client, BT_SET_CONFIGURATION, EIO);
 }
 
-static void gateway_setup_complete(struct audio_device *dev, GError *err, void *user_data)
+static void gateway_setup_complete(struct audio_device *dev, int err, void *user_data)
 {
 	struct unix_client *client = user_data;
 	char buf[BT_SUGGESTED_BUFFER_SIZE];
 	struct bt_set_configuration_rsp *rsp = (void *) buf;
 
 	if (err) {
-		unix_ipc_error(client, BT_SET_CONFIGURATION, err->code);
+		unix_ipc_error(client, BT_SET_CONFIGURATION, -err);
 		return;
 	}
 
@@ -409,7 +409,7 @@ failed:
 	unix_ipc_error(client, BT_START_STREAM, EIO);
 }
 
-static void gateway_resume_complete(struct audio_device *dev, GError *err, void *user_data)
+static void gateway_resume_complete(struct audio_device *dev, int err, void *user_data)
 {
 	struct unix_client *client = user_data;
 	char buf[BT_SUGGESTED_BUFFER_SIZE];
@@ -417,7 +417,7 @@ static void gateway_resume_complete(struct audio_device *dev, GError *err, void
 	struct bt_new_stream_ind *ind = (void *) buf;
 
 	if (err) {
-		unix_ipc_error(client, BT_START_STREAM, err->code);
+		unix_ipc_error(client, BT_START_STREAM, -err);
 		return;
 	}
 
-- 
1.7.1

[PATCH v3 7/8] Asynchronously run gateway_config_stream cb

[Frédéric Dalleau]

---
 audio/gateway.c |   16 ++++++++--------
 audio/unix.c    |    7 ++-----
 2 files changed, 10 insertions(+), 13 deletions(-)

diff --git a/audio/gateway.c b/audio/gateway.c
index 93ff4b9..e145ced 100644
--- a/audio/gateway.c
+++ b/audio/gateway.c
@@ -841,21 +841,21 @@ unsigned int gateway_request_stream(struct audio_device *dev,
 	return id;
 }
 
-int gateway_config_stream(struct audio_device *dev, gateway_stream_cb_t sco_cb,
+int gateway_config_stream(struct audio_device *dev, gateway_stream_cb_t cb,
 				void *user_data)
 {
 	struct gateway *gw = dev->gateway;
+	unsigned int id;
+
+	id = connect_cb_new(gw, cb, user_data);
 
 	if (!gw->rfcomm) {
-		gw->sco_start_cb = sco_cb;
-		gw->sco_start_cb_data = user_data;
-		return get_records(dev);
+		get_records(dev);
+	} else if (cb) {
+		g_idle_add(request_stream_cb, dev);
 	}
 
-	if (sco_cb)
-		sco_cb(dev, 0, user_data);
-
-	return 0;
+	return id;
 }
 
 gboolean gateway_cancel_stream(struct audio_device *dev, unsigned int id)
diff --git a/audio/unix.c b/audio/unix.c
index 111fc57..d16cbd4 100644
--- a/audio/unix.c
+++ b/audio/unix.c
@@ -1045,11 +1045,8 @@ static void start_config(struct audio_device *dev, struct unix_client *client)
 		client->cancel = headset_cancel_stream;
 		break;
 	case TYPE_GATEWAY:
-		if (gateway_config_stream(dev, gateway_setup_complete, client) >= 0) {
-			client->cancel = gateway_cancel_stream;
-			id = 1;
-		} else
-			id = 0;
+		id = gateway_config_stream(dev, gateway_setup_complete, client);
+		client->cancel = gateway_cancel_stream;
 		break;
 
 	default:
-- 
1.7.1

[PATCH v3 8/8] Remove occurences of sco_start_cb

[Frédéric Dalleau]

---
 audio/gateway.c |   28 ++++++----------------------
 1 files changed, 6 insertions(+), 22 deletions(-)

diff --git a/audio/gateway.c b/audio/gateway.c
index e145ced..34c57df 100644
--- a/audio/gateway.c
+++ b/audio/gateway.c
@@ -71,8 +71,6 @@ struct gateway {
 	GIOChannel *rfcomm;
 	GIOChannel *sco;
 	GIOChannel *incoming;
-	gateway_stream_cb_t sco_start_cb;
-	void *sco_start_cb_data;
 	GSList *callbacks;
 	struct hf_agent *agent;
 	DBusMessage *msg;
@@ -259,9 +257,6 @@ static void sco_connect_cb(GIOChannel *chan, GError *err, gpointer user_data)
 
 	change_state(dev, GATEWAY_STATE_PLAYING);
 
-	if (gw->sco_start_cb)
-		gw->sco_start_cb(dev, 0, gw->sco_start_cb_data);
-
 	run_connect_cb(dev, 0);
 }
 
@@ -316,8 +311,6 @@ static void rfcomm_connect_cb(GIOChannel *chan, GError *err,
 
 	if (err) {
 		error("connect(): %s", err->message);
-		if (gw->sco_start_cb)
-			gw->sco_start_cb(dev, err->code, gw->sco_start_cb_data);
 		goto fail;
 	}
 
@@ -353,7 +346,7 @@ fail:
 		g_dbus_send_message(dev->conn, reply);
 	}
 
-	change_state(dev, GATEWAY_STATE_DISCONNECTED);
+	gateway_close(dev);
 }
 
 static int get_remote_profile_version(sdp_record_t *rec)
@@ -498,7 +491,6 @@ static void get_record_cb(sdp_list_t *recs, int err, gpointer user_data)
 				BT_IO_OPT_INVALID);
 	if (!io) {
 		error("Unable to connect: %s", gerr->message);
-		gateway_close(dev);
 		goto fail;
 	}
 
@@ -512,16 +504,10 @@ fail:
 		g_dbus_send_message(dev->conn, reply);
 	}
 
-	change_state(dev, GATEWAY_STATE_DISCONNECTED);
-
-	if (!gerr)
-		g_set_error(&gerr, BT_IO_ERROR, BT_IO_ERROR_FAILED,
-				"connect: %s (%d)", strerror(-err), -err);
-
-	if (gw->sco_start_cb)
-		gw->sco_start_cb(dev, -err, gw->sco_start_cb_data);
+	gateway_close(dev);
 
-	g_error_free(gerr);
+	if (gerr)
+		g_error_free(gerr);
 }
 
 static int get_records(struct audio_device *device)
@@ -571,11 +557,10 @@ int gateway_close(struct audio_device *device)
 		g_io_channel_shutdown(gw->sco, TRUE, NULL);
 		g_io_channel_unref(gw->sco);
 		gw->sco = NULL;
-		gw->sco_start_cb = NULL;
-		gw->sco_start_cb_data = NULL;
 	}
 
 	change_state(device, GATEWAY_STATE_DISCONNECTED);
+	run_connect_cb(device, -EIO);
 
 	return 0;
 }
@@ -904,8 +889,7 @@ void gateway_suspend_stream(struct audio_device *dev)
 	g_io_channel_shutdown(gw->sco, TRUE, NULL);
 	g_io_channel_unref(gw->sco);
 	gw->sco = NULL;
-	gw->sco_start_cb = NULL;
-	gw->sco_start_cb_data = NULL;
+	run_connect_cb(dev, -ECONNABORTED);
 	change_state(dev, GATEWAY_STATE_CONNECTED);
 }
 
-- 
1.7.1

Re: [PATCH v3 1/8] Fix disconnect SCO at same time than RFCOMM

[Luiz Augusto von Dentz]

Hi Frédéric,

2011/8/26 Frédéric Dalleau <frederic.dalleau@linux.intel.com>:
> If RFCOMM disconnects, SCO should be disconnected too.
> ---
>  audio/gateway.c |    7 +------
>  1 files changed, 1 insertions(+), 6 deletions(-)
>
> diff --git a/audio/gateway.c b/audio/gateway.c
> index 142b12e..59c91dd 100644
> --- a/audio/gateway.c
> +++ b/audio/gateway.c
> @@ -222,15 +222,10 @@ static void sco_connect_cb(GIOChannel *chan, GError *err, gpointer user_data)
>  static gboolean rfcomm_disconnect_cb(GIOChannel *chan, GIOCondition cond,
>                        struct audio_device *dev)
>  {
> -       struct gateway *gw = dev->gateway;
> -
>        if (cond & G_IO_NVAL)
>                return FALSE;
>
> -       g_io_channel_shutdown(gw->rfcomm, TRUE, NULL);
> -       g_io_channel_unref(gw->rfcomm);
> -       gw->rfcomm = NULL;
> -       change_state(dev, GATEWAY_STATE_DISCONNECTED);
> +       gateway_close(dev);
>
>        return FALSE;
>  }
> --
> 1.7.1
>

Looks good, ack.

-- 
Luiz Augusto von Dentz

Re: [PATCH v3 2/8] Fix asynchronously run request stream cb

[Luiz Augusto von Dentz]

Hi Frédéric,

2011/8/26 Frédéric Dalleau <frederic.dalleau@linux.intel.com>:
> ---
>  audio/gateway.c |   68 +++++++++++++++++++++++++++++++++++++++++++++++-------
>  audio/gateway.h |    2 +-
>  audio/unix.c    |    6 +---
>  3 files changed, 62 insertions(+), 14 deletions(-)
>
> diff --git a/audio/gateway.c b/audio/gateway.c
> index 59c91dd..8cbaeb4 100644
> --- a/audio/gateway.c
> +++ b/audio/gateway.c
> @@ -60,6 +60,12 @@ struct hf_agent {
>        guint watch;    /* Disconnect watch */
>  };
>
> +struct connect_cb {
> +       unsigned int id;
> +       gateway_stream_cb_t cb;
> +       void *cb_data;
> +};
> +
>  struct gateway {
>        gateway_state_t state;
>        GIOChannel *rfcomm;
> @@ -67,6 +73,7 @@ struct gateway {
>        GIOChannel *incoming;
>        gateway_stream_cb_t sco_start_cb;
>        void *sco_start_cb_data;
> +       GSList *callbacks;
>        struct hf_agent *agent;
>        DBusMessage *msg;
>        int version;
> @@ -180,6 +187,41 @@ static gboolean agent_sendfd(struct hf_agent *agent, int fd,
>        return TRUE;
>  }
>
> +static unsigned int connect_cb_new(struct gateway *gw,
> +                                       gateway_stream_cb_t func,
> +                                       void *user_data)
> +{
> +       struct connect_cb *cb;
> +       static unsigned int free_cb_id = 1;
> +
> +       if (!func)
> +               return 0;
> +
> +       cb = g_new(struct connect_cb, 1);
> +
> +       cb->cb = func;
> +       cb->cb_data = user_data;
> +       cb->id = free_cb_id++;
> +
> +       gw->callbacks = g_slist_append(gw->callbacks, cb);
> +
> +       return cb->id;
> +}
> +
> +static void run_connect_cb(struct audio_device *dev, GError *err)
> +{
> +       struct gateway *gw = dev->gateway;
> +       GSList *l;
> +
> +       for (l = gw->callbacks; l != NULL; l = l->next) {
> +               struct connect_cb *cb = l->data;
> +               cb->cb(dev, err, cb->cb_data);
> +       }
> +
> +       g_slist_free_full(gw->callbacks, g_free);
> +       gw->callbacks = NULL;
> +}
> +
>  static gboolean sco_io_cb(GIOChannel *chan, GIOCondition cond,
>                        struct audio_device *dev)
>  {
> @@ -209,6 +251,8 @@ static void sco_connect_cb(GIOChannel *chan, GError *err, gpointer user_data)
>        if (gw->sco_start_cb)
>                gw->sco_start_cb(dev, err, gw->sco_start_cb_data);
>
> +       run_connect_cb(dev, err);
> +
>        if (err) {
>                error("sco_connect_cb(): %s", err->message);
>                gateway_close(dev);
> @@ -759,22 +803,27 @@ void gateway_start_service(struct audio_device *dev)
>        }
>  }
>
> +static gboolean request_stream_cb(gpointer data)
> +{
> +       run_connect_cb(data, NULL);
> +       return FALSE;
> +}
> +
>  /* These are functions to be called from unix.c for audio system
>  * ifaces (alsa, gstreamer, etc.) */
> -gboolean gateway_request_stream(struct audio_device *dev,
> +unsigned int gateway_request_stream(struct audio_device *dev,
>                                gateway_stream_cb_t cb, void *user_data)
>  {
>        struct gateway *gw = dev->gateway;
> +       unsigned int id;
>        GError *err = NULL;
>        GIOChannel *io;
>
> +       id = connect_cb_new(gw, cb, user_data);
> +
>        if (!gw->rfcomm) {
> -               gw->sco_start_cb = cb;
> -               gw->sco_start_cb_data = user_data;
>                get_records(dev);
>        } else if (!gw->sco) {
> -               gw->sco_start_cb = cb;
> -               gw->sco_start_cb_data = user_data;

It seems after this changes sco_start_cb won't be necessary anymore,
but apparently you only remove it in another patch, IMO it would be
better to have merged so we know why it was removed.


-- 
Luiz Augusto von Dentz

Re: [PATCH v3 3/8] Cancel pending callback if stream is canceled

[Luiz Augusto von Dentz]

Hi Frédéric,

2011/8/26 Frédéric Dalleau <frederic.dalleau@linux.intel.com>:
> ---
>  audio/gateway.c |   20 ++++++++++++++++++++
>  1 files changed, 20 insertions(+), 0 deletions(-)
>
> diff --git a/audio/gateway.c b/audio/gateway.c
> index 8cbaeb4..c5f21e6 100644
> --- a/audio/gateway.c
> +++ b/audio/gateway.c
> @@ -859,7 +859,27 @@ int gateway_config_stream(struct audio_device *dev, gateway_stream_cb_t sco_cb,
>
>  gboolean gateway_cancel_stream(struct audio_device *dev, unsigned int id)
>  {
> +       struct gateway *gw = dev->gateway;
> +       GSList *l;
> +       struct connect_cb *cb = NULL;
> +
> +       for (l = gw->callbacks; l != NULL; l = l->next) {
> +               struct connect_cb *tmp = l->data;
> +
> +               if (tmp->id == id) {
> +                       cb = tmp;
> +                       break;
> +               }
> +       }
> +
> +       if (!cb)
> +               return FALSE;
> +
> +       gw->callbacks = g_slist_remove(gw->callbacks, cb);
> +       g_free(cb);
> +
>        gateway_suspend_stream(dev);
> +
>        return TRUE;
>  }
>
> --
> 1.7.1
>

Ack.


-- 
Luiz Augusto von Dentz

Re: [PATCH v3 4/8] Fix state to "playing" on SCO establishment

[Luiz Augusto von Dentz]

Hi Frédéric,

2011/8/26 Frédéric Dalleau <frederic.dalleau@linux.intel.com>:
> ---
>  audio/gateway.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
>
> diff --git a/audio/gateway.c b/audio/gateway.c
> index c5f21e6..7bab77c 100644
> --- a/audio/gateway.c
> +++ b/audio/gateway.c
> @@ -261,6 +261,8 @@ static void sco_connect_cb(GIOChannel *chan, GError *err, gpointer user_data)
>
>        g_io_add_watch(gw->sco, G_IO_ERR | G_IO_HUP | G_IO_NVAL,
>                                (GIOFunc) sco_io_cb, dev);
> +
> +       change_state(dev, GATEWAY_STATE_PLAYING);
>  }
>
>  static gboolean rfcomm_disconnect_cb(GIOChannel *chan, GIOCondition cond,
> --
> 1.7.1

Ack.

-- 
Luiz Augusto von Dentz

Re: [PATCH v3 5/8] Set state to "connecting" on connection requested

[Luiz Augusto von Dentz]

Hi Frédéric,

2011/8/26 Frédéric Dalleau <frederic.dalleau@linux.intel.com>:
> This change will become necessary when
> integrating the Audio interface which rely on
> state change to confirm that connection
> has started successfully.
> ---
>  audio/gateway.c |    3 +--
>  1 files changed, 1 insertions(+), 2 deletions(-)
>
> diff --git a/audio/gateway.c b/audio/gateway.c
> index 7bab77c..353473b 100644
> --- a/audio/gateway.c
> +++ b/audio/gateway.c
> @@ -503,8 +503,6 @@ static void get_record_cb(sdp_list_t *recs, int err, gpointer user_data)
>        }
>
>        g_io_channel_unref(io);
> -
> -       change_state(dev, GATEWAY_STATE_CONNECTING);
>        return;
>
>  fail:
> @@ -530,6 +528,7 @@ static int get_records(struct audio_device *device)
>  {
>        uuid_t uuid;
>
> +       change_state(device, GATEWAY_STATE_CONNECTING);
>        sdp_uuid16_create(&uuid, HANDSFREE_AGW_SVCLASS_ID);
>        return bt_search_service(&device->src, &device->dst, &uuid,
>                                get_record_cb, device, NULL);
> --
> 1.7.1
>

How about incoming connections? If you take a look at headset,
audio/manage.c set HEADSET_STATE_CONNECTING, I guess we should have
something similar to gateway, right?

-- 
Luiz Augusto von Dentz

Re: [PATCH v3 6/8] Use int instead of GError into callbacks

[Luiz Augusto von Dentz]

Hi Frédéric,

2011/8/26 Frédéric Dalleau <frederic.dalleau@linux.intel.com>:
> ---
>  audio/gateway.c   |   20 ++++++++++----------
>  audio/gateway.h   |    4 ++--
>  audio/transport.c |    4 ++--
>  audio/unix.c      |    8 ++++----
>  4 files changed, 18 insertions(+), 18 deletions(-)

Some description would be nice here, why do you need such change, is
there something you have changed in another patch that affects the
error handling? In that case I would suggest fixing in place, so we
can relate the changes using e.g. git blame.

> diff --git a/audio/gateway.c b/audio/gateway.c
> index 353473b..93ff4b9 100644
> --- a/audio/gateway.c
> +++ b/audio/gateway.c
> @@ -208,7 +208,7 @@ static unsigned int connect_cb_new(struct gateway *gw,
>        return cb->id;
>  }
>
> -static void run_connect_cb(struct audio_device *dev, GError *err)
> +static void run_connect_cb(struct audio_device *dev, int err)
>  {
>        struct gateway *gw = dev->gateway;
>        GSList *l;
> @@ -248,11 +248,6 @@ static void sco_connect_cb(GIOChannel *chan, GError *err, gpointer user_data)
>
>        gw->sco = g_io_channel_ref(chan);
>
> -       if (gw->sco_start_cb)
> -               gw->sco_start_cb(dev, err, gw->sco_start_cb_data);
> -
> -       run_connect_cb(dev, err);
> -
>        if (err) {
>                error("sco_connect_cb(): %s", err->message);
>                gateway_close(dev);
> @@ -263,6 +258,11 @@ static void sco_connect_cb(GIOChannel *chan, GError *err, gpointer user_data)
>                                (GIOFunc) sco_io_cb, dev);
>
>        change_state(dev, GATEWAY_STATE_PLAYING);
> +
> +       if (gw->sco_start_cb)
> +               gw->sco_start_cb(dev, 0, gw->sco_start_cb_data);
> +
> +       run_connect_cb(dev, 0);
>  }
>
>  static gboolean rfcomm_disconnect_cb(GIOChannel *chan, GIOCondition cond,
> @@ -317,7 +317,7 @@ static void rfcomm_connect_cb(GIOChannel *chan, GError *err,
>        if (err) {
>                error("connect(): %s", err->message);
>                if (gw->sco_start_cb)
> -                       gw->sco_start_cb(dev, err, gw->sco_start_cb_data);
> +                       gw->sco_start_cb(dev, err->code, gw->sco_start_cb_data);
>                goto fail;
>        }
>
> @@ -519,7 +519,7 @@ fail:
>                                "connect: %s (%d)", strerror(-err), -err);
>
>        if (gw->sco_start_cb)
> -               gw->sco_start_cb(dev, gerr, gw->sco_start_cb_data);
> +               gw->sco_start_cb(dev, -err, gw->sco_start_cb_data);
>
>        g_error_free(gerr);
>  }
> @@ -806,7 +806,7 @@ void gateway_start_service(struct audio_device *dev)
>
>  static gboolean request_stream_cb(gpointer data)
>  {
> -       run_connect_cb(data, NULL);
> +       run_connect_cb(data, 0);
>        return FALSE;
>  }
>
> @@ -853,7 +853,7 @@ int gateway_config_stream(struct audio_device *dev, gateway_stream_cb_t sco_cb,
>        }
>
>        if (sco_cb)
> -               sco_cb(dev, NULL, user_data);
> +               sco_cb(dev, 0, user_data);
>
>        return 0;
>  }
> diff --git a/audio/gateway.h b/audio/gateway.h
> index 7012fc5..20f90bd 100644
> --- a/audio/gateway.h
> +++ b/audio/gateway.h
> @@ -42,8 +42,8 @@ typedef void (*gateway_state_cb) (struct audio_device *dev,
>                                        gateway_state_t old_state,
>                                        gateway_state_t new_state,
>                                        void *user_data);
> -typedef void (*gateway_stream_cb_t) (struct audio_device *dev, GError *err,
> -               void *user_data);
> +typedef void (*gateway_stream_cb_t) (struct audio_device *dev, int err,
> +                                       void *user_data);
>
>  void gateway_set_state(struct audio_device *dev, gateway_state_t new_state);
>  void gateway_unregister(struct audio_device *dev);
> diff --git a/audio/transport.c b/audio/transport.c
> index 2739199..54dabb6 100644
> --- a/audio/transport.c
> +++ b/audio/transport.c
> @@ -437,7 +437,7 @@ static void cancel_headset(struct media_transport *transport, guint id)
>        headset_cancel_stream(transport->device, id);
>  }
>
> -static void gateway_resume_complete(struct audio_device *dev, GError *err,
> +static void gateway_resume_complete(struct audio_device *dev, int err,
>                                                        void *user_data)
>  {
>        struct media_owner *owner = user_data;
> @@ -453,7 +453,7 @@ static void gateway_resume_complete(struct audio_device *dev, GError *err,
>                goto fail;
>
>        if (err) {
> -               error("Failed to resume gateway: error %s", err->message);
> +               error("Failed to resume gateway: error %s", strerror(-err));
>                goto fail;
>        }
>
> diff --git a/audio/unix.c b/audio/unix.c
> index 678d097..111fc57 100644
> --- a/audio/unix.c
> +++ b/audio/unix.c
> @@ -339,14 +339,14 @@ failed:
>        unix_ipc_error(client, BT_SET_CONFIGURATION, EIO);
>  }
>
> -static void gateway_setup_complete(struct audio_device *dev, GError *err, void *user_data)
> +static void gateway_setup_complete(struct audio_device *dev, int err, void *user_data)
>  {
>        struct unix_client *client = user_data;
>        char buf[BT_SUGGESTED_BUFFER_SIZE];
>        struct bt_set_configuration_rsp *rsp = (void *) buf;
>
>        if (err) {
> -               unix_ipc_error(client, BT_SET_CONFIGURATION, err->code);
> +               unix_ipc_error(client, BT_SET_CONFIGURATION, -err);
>                return;
>        }
>
> @@ -409,7 +409,7 @@ failed:
>        unix_ipc_error(client, BT_START_STREAM, EIO);
>  }
>
> -static void gateway_resume_complete(struct audio_device *dev, GError *err, void *user_data)
> +static void gateway_resume_complete(struct audio_device *dev, int err, void *user_data)
>  {
>        struct unix_client *client = user_data;
>        char buf[BT_SUGGESTED_BUFFER_SIZE];
> @@ -417,7 +417,7 @@ static void gateway_resume_complete(struct audio_device *dev, GError *err, void
>        struct bt_new_stream_ind *ind = (void *) buf;
>
>        if (err) {
> -               unix_ipc_error(client, BT_START_STREAM, err->code);
> +               unix_ipc_error(client, BT_START_STREAM, -err);
>                return;
>        }
>
> --
> 1.7.1
>
> --
> 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
>



-- 
Luiz Augusto von Dentz

Re: [PATCH v3 7/8] Asynchronously run gateway_config_stream cb

[Luiz Augusto von Dentz]

Hi Frédéric,

2011/8/26 Frédéric Dalleau <frederic.dalleau@linux.intel.com>:
> ---
>  audio/gateway.c |   16 ++++++++--------
>  audio/unix.c    |    7 ++-----
>  2 files changed, 10 insertions(+), 13 deletions(-)
>
> diff --git a/audio/gateway.c b/audio/gateway.c
> index 93ff4b9..e145ced 100644
> --- a/audio/gateway.c
> +++ b/audio/gateway.c
> @@ -841,21 +841,21 @@ unsigned int gateway_request_stream(struct audio_device *dev,
>        return id;
>  }
>
> -int gateway_config_stream(struct audio_device *dev, gateway_stream_cb_t sco_cb,
> +int gateway_config_stream(struct audio_device *dev, gateway_stream_cb_t cb,
>                                void *user_data)
>  {
>        struct gateway *gw = dev->gateway;
> +       unsigned int id;
> +
> +       id = connect_cb_new(gw, cb, user_data);
>
>        if (!gw->rfcomm) {
> -               gw->sco_start_cb = sco_cb;
> -               gw->sco_start_cb_data = user_data;
> -               return get_records(dev);
> +               get_records(dev);
> +       } else if (cb) {
> +               g_idle_add(request_stream_cb, dev);
>        }
>
> -       if (sco_cb)
> -               sco_cb(dev, 0, user_data);
> -
> -       return 0;
> +       return id;
>  }
>
>  gboolean gateway_cancel_stream(struct audio_device *dev, unsigned int id)
> diff --git a/audio/unix.c b/audio/unix.c
> index 111fc57..d16cbd4 100644
> --- a/audio/unix.c
> +++ b/audio/unix.c
> @@ -1045,11 +1045,8 @@ static void start_config(struct audio_device *dev, struct unix_client *client)
>                client->cancel = headset_cancel_stream;
>                break;
>        case TYPE_GATEWAY:
> -               if (gateway_config_stream(dev, gateway_setup_complete, client) >= 0) {
> -                       client->cancel = gateway_cancel_stream;
> -                       id = 1;
> -               } else
> -                       id = 0;
> +               id = gateway_config_stream(dev, gateway_setup_complete, client);
> +               client->cancel = gateway_cancel_stream;
>                break;
>
>        default:
> --
> 1.7.1

It looks like this is also related to the change how gateway callbacks
works, actually the code in unix.c would self generate its id, so
perhaps this should be merged where you change the return of
gateway_config_stream?


-- 
Luiz Augusto von Dentz

Re: [PATCH v3 8/8] Remove occurences of sco_start_cb

[Luiz Augusto von Dentz]

Hi Frédéric,

2011/8/26 Frédéric Dalleau <frederic.dalleau@linux.intel.com>:
> ---
>  audio/gateway.c |   28 ++++++----------------------
>  1 files changed, 6 insertions(+), 22 deletions(-)
>
> diff --git a/audio/gateway.c b/audio/gateway.c
> index e145ced..34c57df 100644
> --- a/audio/gateway.c
> +++ b/audio/gateway.c
> @@ -71,8 +71,6 @@ struct gateway {
>        GIOChannel *rfcomm;
>        GIOChannel *sco;
>        GIOChannel *incoming;
> -       gateway_stream_cb_t sco_start_cb;
> -       void *sco_start_cb_data;
>        GSList *callbacks;
>        struct hf_agent *agent;
>        DBusMessage *msg;
> @@ -259,9 +257,6 @@ static void sco_connect_cb(GIOChannel *chan, GError *err, gpointer user_data)
>
>        change_state(dev, GATEWAY_STATE_PLAYING);
>
> -       if (gw->sco_start_cb)
> -               gw->sco_start_cb(dev, 0, gw->sco_start_cb_data);
> -
>        run_connect_cb(dev, 0);
>  }
>
> @@ -316,8 +311,6 @@ static void rfcomm_connect_cb(GIOChannel *chan, GError *err,
>
>        if (err) {
>                error("connect(): %s", err->message);
> -               if (gw->sco_start_cb)
> -                       gw->sco_start_cb(dev, err->code, gw->sco_start_cb_data);
>                goto fail;
>        }
>
> @@ -353,7 +346,7 @@ fail:
>                g_dbus_send_message(dev->conn, reply);
>        }
>
> -       change_state(dev, GATEWAY_STATE_DISCONNECTED);
> +       gateway_close(dev);
>  }
>
>  static int get_remote_profile_version(sdp_record_t *rec)
> @@ -498,7 +491,6 @@ static void get_record_cb(sdp_list_t *recs, int err, gpointer user_data)
>                                BT_IO_OPT_INVALID);
>        if (!io) {
>                error("Unable to connect: %s", gerr->message);
> -               gateway_close(dev);
>                goto fail;
>        }
>
> @@ -512,16 +504,10 @@ fail:
>                g_dbus_send_message(dev->conn, reply);
>        }
>
> -       change_state(dev, GATEWAY_STATE_DISCONNECTED);
> -
> -       if (!gerr)
> -               g_set_error(&gerr, BT_IO_ERROR, BT_IO_ERROR_FAILED,
> -                               "connect: %s (%d)", strerror(-err), -err);
> -
> -       if (gw->sco_start_cb)
> -               gw->sco_start_cb(dev, -err, gw->sco_start_cb_data);
> +       gateway_close(dev);
>
> -       g_error_free(gerr);
> +       if (gerr)
> +               g_error_free(gerr);
>  }
>
>  static int get_records(struct audio_device *device)
> @@ -571,11 +557,10 @@ int gateway_close(struct audio_device *device)
>                g_io_channel_shutdown(gw->sco, TRUE, NULL);
>                g_io_channel_unref(gw->sco);
>                gw->sco = NULL;
> -               gw->sco_start_cb = NULL;
> -               gw->sco_start_cb_data = NULL;
>        }
>
>        change_state(device, GATEWAY_STATE_DISCONNECTED);
> +       run_connect_cb(device, -EIO);
>
>        return 0;
>  }
> @@ -904,8 +889,7 @@ void gateway_suspend_stream(struct audio_device *dev)
>        g_io_channel_shutdown(gw->sco, TRUE, NULL);
>        g_io_channel_unref(gw->sco);
>        gw->sco = NULL;
> -       gw->sco_start_cb = NULL;
> -       gw->sco_start_cb_data = NULL;
> +       run_connect_cb(dev, -ECONNABORTED);
>        change_state(dev, GATEWAY_STATE_CONNECTED);
>  }
>
> --
> 1.7.1

As I said I would merge this where you actually change the way the
callbacks works.

-- 
Luiz Augusto von Dentz