[PATCH 1/5] handsfree-audio: Add support for initiating SCO connections

[PATCH 1/5] handsfree-audio: Add support for initiating SCO connections

[Vinicius Costa Gomes]

[-- Attachment #1: Type: text/plain, Size: 3084 bytes --]

When calling the card's .Connect() method, we should be able to
establish a SCO connection.

Right now, we only have support for establishing the SCO connection
directly, this is what is expected from HFP 1.5 HF/AG devices.
---
 src/handsfree-audio.c | 93 ++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 92 insertions(+), 1 deletion(-)

diff --git a/src/handsfree-audio.c b/src/handsfree-audio.c
index c7fa2fb..795a68b 100644
--- a/src/handsfree-audio.c
+++ b/src/handsfree-audio.c
@@ -53,6 +53,7 @@ struct ofono_handsfree_card {
 	char *remote;
 	char *local;
 	char *path;
+	DBusMessage *msg;
 	const struct ofono_handsfree_card_driver *driver;
 	void *driver_data;
 };
@@ -235,10 +236,100 @@ static DBusMessage *card_get_properties(DBusConnection *conn,
 	return reply;
 }
 
+static int card_connect_sco(struct ofono_handsfree_card *card)
+{
+	struct sockaddr_sco addr;
+	int sk, ret;
+
+	sk = socket(PF_BLUETOOTH, SOCK_SEQPACKET | O_NONBLOCK | SOCK_CLOEXEC,
+								BTPROTO_SCO);
+	if (sk < 0)
+		return -1;
+
+	/* Bind to local address */
+	memset(&addr, 0, sizeof(addr));
+	addr.sco_family = AF_BLUETOOTH;
+	bt_str2ba(card->local, &addr.sco_bdaddr);
+
+	if (bind(sk, (struct sockaddr *) &addr, sizeof(addr)) < 0) {
+		close(sk);
+		return -1;
+	}
+
+	/* Connect to remote device */
+	memset(&addr, 0, sizeof(addr));
+	addr.sco_family = AF_BLUETOOTH;
+	bt_str2ba(card->remote, &addr.sco_bdaddr);
+
+	ret = connect(sk, (struct sockaddr *) &addr, sizeof(addr));
+	if (ret < 0 && errno != EINPROGRESS) {
+		close(sk);
+		return -1;
+	}
+
+	return sk;
+}
+
+static gboolean sco_connect_cb(GIOChannel *io, GIOCondition cond,
+							gpointer user_data)
+
+{
+	struct ofono_handsfree_card *card = user_data;
+	DBusMessage *reply;
+	int sk;
+
+	if (agent == NULL) {
+		/* There's no agent, so there's no one to reply to */
+		reply = NULL;
+		goto done;
+	}
+
+	if (cond & (G_IO_ERR | G_IO_HUP | G_IO_NVAL)) {
+		reply = __ofono_error_failed(card->msg);
+		goto done;
+	}
+
+	sk = g_io_channel_unix_get_fd(io);
+
+	close(sk);
+
+	reply = dbus_message_new_method_return(card->msg);
+
+done:
+	if (reply)
+		g_dbus_send_message(ofono_dbus_get_connection(), reply);
+
+	dbus_message_unref(card->msg);
+	card->msg = NULL;
+
+	return FALSE;
+}
+
 static DBusMessage *card_connect(DBusConnection *conn,
 						DBusMessage *msg, void *data)
 {
-	return __ofono_error_not_implemented(msg);
+	struct ofono_handsfree_card *card = data;
+	GIOChannel *io;
+	int sk;
+
+	if (agent == NULL)
+		return __ofono_error_not_available(msg);
+
+	if (card->msg)
+		return __ofono_error_busy(msg);
+
+	sk = card_connect_sco(card);
+	if (sk < 0)
+		return __ofono_error_failed(msg);
+
+	io = g_io_channel_unix_new(sk);
+	g_io_add_watch(io, G_IO_OUT | G_IO_ERR | G_IO_HUP | G_IO_NVAL,
+						sco_connect_cb, card);
+	g_io_channel_unref(io);
+
+	card->msg = dbus_message_ref(msg);
+
+	return NULL;
 }
 
 static const GDBusMethodTable card_methods[] = {
-- 
1.8.2

[PATCH 2/5] handsfree-audio: Add support for sending the SCO socket

[Vinicius Costa Gomes]

[-- Attachment #1: Type: text/plain, Size: 565 bytes --]

Send the SCO socket to the agent associated with the card that
just got connected.
---
 src/handsfree-audio.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/src/handsfree-audio.c b/src/handsfree-audio.c
index 795a68b..4232074 100644
--- a/src/handsfree-audio.c
+++ b/src/handsfree-audio.c
@@ -291,6 +291,8 @@ static gboolean sco_connect_cb(GIOChannel *io, GIOCondition cond,
 
 	sk = g_io_channel_unix_get_fd(io);
 
+	send_new_connection(card->path, sk);
+
 	close(sk);
 
 	reply = dbus_message_new_method_return(card->msg);
-- 
1.8.2

[PATCH 3/5] handsfree-audio: Reject .Connect() from other senders

[Vinicius Costa Gomes]

[-- Attachment #1: Type: text/plain, Size: 746 bytes --]

Only the agent should be able to call .Connect() on the card.
---
 src/handsfree-audio.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/src/handsfree-audio.c b/src/handsfree-audio.c
index 4232074..f8df6d6 100644
--- a/src/handsfree-audio.c
+++ b/src/handsfree-audio.c
@@ -312,11 +312,17 @@ static DBusMessage *card_connect(DBusConnection *conn,
 {
 	struct ofono_handsfree_card *card = data;
 	GIOChannel *io;
+	const char *sender;
 	int sk;
 
 	if (agent == NULL)
 		return __ofono_error_not_available(msg);
 
+	sender = dbus_message_get_sender(msg);
+
+	if (!g_str_equal(sender, agent->owner))
+		return __ofono_error_not_allowed(msg);
+
 	if (card->msg)
 		return __ofono_error_busy(msg);
 
-- 
1.8.2

[PATCH 4/5] hfp_hf_bluez5: Add a card driver for HFP 1.6

[Vinicius Costa Gomes]

[-- Attachment #1: Type: text/plain, Size: 2082 bytes --]

This is just the skeleton of a Handsfree Audio Card for the HF side of
HFP 1.6.
---
 plugins/hfp_hf_bluez5.c | 37 +++++++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/plugins/hfp_hf_bluez5.c b/plugins/hfp_hf_bluez5.c
index ff8afba..89b60c8 100644
--- a/plugins/hfp_hf_bluez5.c
+++ b/plugins/hfp_hf_bluez5.c
@@ -60,6 +60,8 @@
 
 #define HFP_EXT_PROFILE_PATH   "/bluetooth/profile/hfp_hf"
 
+#define HFP16_HF_DRIVER		"hfp16-hf-driver"
+
 struct hfp {
 	struct hfp_slc_info info;
 	DBusMessage *msg;
@@ -310,6 +312,31 @@ static struct ofono_modem_driver hfp_driver = {
 	.post_sim	= hfp_post_sim,
 };
 
+static int hfp16_card_probe(struct ofono_handsfree_card *card,
+					unsigned int vendor, void *data)
+{
+	return 0;
+}
+
+static void hfp16_card_remove(struct ofono_handsfree_card *card)
+{
+
+}
+
+static int hfp16_card_connect(struct ofono_handsfree_card *card,
+					ofono_handsfree_card_connect_cb_t cb,
+					void *data)
+{
+	return -ENOSYS;
+}
+
+static struct ofono_handsfree_card_driver hfp16_hf_driver = {
+	.name		= HFP16_HF_DRIVER,
+	.probe		= hfp16_card_probe,
+	.remove		= hfp16_card_remove,
+	.connect	= hfp16_card_connect,
+};
+
 static ofono_bool_t device_path_compare(struct ofono_modem *modem,
 					void *userdata)
 {
@@ -616,6 +643,13 @@ static int hfp_init(void)
 		return -EIO;
 	}
 
+	err = ofono_handsfree_card_driver_register(&hfp16_hf_driver);
+	if (err < 0) {
+		g_dbus_unregister_interface(conn, HFP_EXT_PROFILE_PATH,
+						BLUEZ_PROFILE_INTERFACE);
+		return err;
+	}
+
 	err = ofono_modem_driver_register(&hfp_driver);
 	if (err < 0) {
 		g_dbus_unregister_interface(conn, HFP_EXT_PROFILE_PATH,
@@ -647,6 +681,9 @@ static void hfp_exit(void)
 	bt_unregister_profile(conn, HFP_EXT_PROFILE_PATH);
 	g_dbus_unregister_interface(conn, HFP_EXT_PROFILE_PATH,
 						BLUEZ_PROFILE_INTERFACE);
+
+	ofono_handsfree_card_driver_unregister(&hfp16_hf_driver);
+
 	ofono_modem_driver_unregister(&hfp_driver);
 	g_dbus_client_unref(bluez);
 
-- 
1.8.2

[PATCH 5/5] handsfree-audio: Add .Connect using the card driver

[Vinicius Costa Gomes]

[-- Attachment #1: Type: text/plain, Size: 1695 bytes --]

---
 src/handsfree-audio.c | 38 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 38 insertions(+)

diff --git a/src/handsfree-audio.c b/src/handsfree-audio.c
index f8df6d6..9d3d64e 100644
--- a/src/handsfree-audio.c
+++ b/src/handsfree-audio.c
@@ -307,6 +307,40 @@ done:
 	return FALSE;
 }
 
+static void card_connect_reply_cb(const struct ofono_error *error, void *data)
+{
+	struct ofono_handsfree_card *card = data;
+	DBusMessage *reply;
+
+	if (error->type == OFONO_ERROR_TYPE_NO_ERROR)
+		reply = dbus_message_new_method_return(card->msg);
+	else
+		reply = __ofono_error_failed(card->msg);
+
+	g_dbus_send_message(ofono_dbus_get_connection(), reply);
+
+	dbus_message_unref(card->msg);
+	card->msg = NULL;
+}
+
+static DBusMessage *card_connect_driver(struct ofono_handsfree_card *card,
+							DBusMessage *msg)
+{
+	const struct ofono_handsfree_card_driver *driver = card->driver;
+	int err;
+
+	err = driver->connect(card, card_connect_reply_cb, card);
+	if (err == -EINPROGRESS) {
+		card->msg = dbus_message_ref(msg);
+		return NULL;
+	}
+
+	if (err < 0)
+		return __ofono_error_failed(msg);
+
+	return dbus_message_new_method_return(msg);
+}
+
 static DBusMessage *card_connect(DBusConnection *conn,
 						DBusMessage *msg, void *data)
 {
@@ -326,6 +360,10 @@ static DBusMessage *card_connect(DBusConnection *conn,
 	if (card->msg)
 		return __ofono_error_busy(msg);
 
+	if (card->driver && card->driver->connect)
+		return card_connect_driver(card, msg);
+
+	/* There's no driver, fallback to direct SCO connection */
 	sk = card_connect_sco(card);
 	if (sk < 0)
 		return __ofono_error_failed(msg);
-- 
1.8.2

Re: [PATCH 5/5] handsfree-audio: Add .Connect using the card driver

[Vinicius Costa Gomes]

[-- Attachment #1: Type: text/plain, Size: 2192 bytes --]

Hi,

On 19:05 Mon 25 Mar, Vinicius Costa Gomes wrote:
> ---
>  src/handsfree-audio.c | 38 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 38 insertions(+)
> 
> diff --git a/src/handsfree-audio.c b/src/handsfree-audio.c
> index f8df6d6..9d3d64e 100644
> --- a/src/handsfree-audio.c
> +++ b/src/handsfree-audio.c
> @@ -307,6 +307,40 @@ done:
>  	return FALSE;
>  }
>  
> +static void card_connect_reply_cb(const struct ofono_error *error, void *data)
> +{
> +	struct ofono_handsfree_card *card = data;
> +	DBusMessage *reply;
> +
> +	if (error->type == OFONO_ERROR_TYPE_NO_ERROR)
> +		reply = dbus_message_new_method_return(card->msg);
> +	else
> +		reply = __ofono_error_failed(card->msg);
> +
> +	g_dbus_send_message(ofono_dbus_get_connection(), reply);
> +
> +	dbus_message_unref(card->msg);
> +	card->msg = NULL;
> +}
> +
> +static DBusMessage *card_connect_driver(struct ofono_handsfree_card *card,
> +							DBusMessage *msg)
> +{
> +	const struct ofono_handsfree_card_driver *driver = card->driver;
> +	int err;
> +
> +	err = driver->connect(card, card_connect_reply_cb, card);
> +	if (err == -EINPROGRESS) {
> +		card->msg = dbus_message_ref(msg);
> +		return NULL;
> +	}
> +
> +	if (err < 0)
> +		return __ofono_error_failed(msg);
> +
> +	return dbus_message_new_method_return(msg);
> +}
> +
>  static DBusMessage *card_connect(DBusConnection *conn,
>  						DBusMessage *msg, void *data)
>  {
> @@ -326,6 +360,10 @@ static DBusMessage *card_connect(DBusConnection *conn,
>  	if (card->msg)
>  		return __ofono_error_busy(msg);
>  
> +	if (card->driver && card->driver->connect)
> +		return card_connect_driver(card, msg);

In the case that both sides are HFP 1.6, but the Codec Negotiation feature is not
available on one of them, we should fallback to the direct codec connection
procedure.

I will send a version of this patch that makes it easier to handle this case,
without much repeated code.

> +
> +	/* There's no driver, fallback to direct SCO connection */
>  	sk = card_connect_sco(card);
>  	if (sk < 0)
>  		return __ofono_error_failed(msg);
> -- 
> 1.8.2
> 


Cheers,
-- 
Vinicius

[PATCH] handsfree-audio: Add .Connect using the card driver

[Vinicius Costa Gomes]

[-- Attachment #1: Type: text/plain, Size: 1992 bytes --]

Now each handsfree implementation may be notified that a card wants
its audio to be connected.

In case, that the cards wishes to fallback to the default SCO connection
procedure, it just needs to return -ENOSYS.
---
 src/handsfree-audio.c | 34 +++++++++++++++++++++++++++++++++-
 1 file changed, 33 insertions(+), 1 deletion(-)

diff --git a/src/handsfree-audio.c b/src/handsfree-audio.c
index f8df6d6..ace3a4f 100644
--- a/src/handsfree-audio.c
+++ b/src/handsfree-audio.c
@@ -307,13 +307,30 @@ done:
 	return FALSE;
 }
 
+static void card_connect_reply_cb(const struct ofono_error *error, void *data)
+{
+	struct ofono_handsfree_card *card = data;
+	DBusMessage *reply;
+
+	if (error->type == OFONO_ERROR_TYPE_NO_ERROR)
+		reply = dbus_message_new_method_return(card->msg);
+	else
+		reply = __ofono_error_failed(card->msg);
+
+	g_dbus_send_message(ofono_dbus_get_connection(), reply);
+
+	dbus_message_unref(card->msg);
+	card->msg = NULL;
+}
+
 static DBusMessage *card_connect(DBusConnection *conn,
 						DBusMessage *msg, void *data)
 {
 	struct ofono_handsfree_card *card = data;
+	const struct ofono_handsfree_card_driver *driver = card->driver;
 	GIOChannel *io;
 	const char *sender;
-	int sk;
+	int sk, err;
 
 	if (agent == NULL)
 		return __ofono_error_not_available(msg);
@@ -326,6 +343,21 @@ static DBusMessage *card_connect(DBusConnection *conn,
 	if (card->msg)
 		return __ofono_error_busy(msg);
 
+	if (!driver || !driver->connect)
+		goto fallback;
+
+	err = driver->connect(card, card_connect_reply_cb, card);
+	if (err == -EINPROGRESS) {
+		card->msg = dbus_message_ref(msg);
+		return NULL;
+	}
+
+	/* ENOSYS means the driver doesn't want to connect itself */
+	if (err < 0 && err != -ENOSYS)
+		return __ofono_error_failed(msg);
+
+fallback:
+	/* There's no driver, fallback to direct SCO connection */
 	sk = card_connect_sco(card);
 	if (sk < 0)
 		return __ofono_error_failed(msg);
-- 
1.8.2

Re: [PATCH 1/5] handsfree-audio: Add support for initiating SCO connections

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 511 bytes --]

Hi Vinicius,

On 03/25/2013 05:05 PM, Vinicius Costa Gomes wrote:
> When calling the card's .Connect() method, we should be able to
> establish a SCO connection.
>
> Right now, we only have support for establishing the SCO connection
> directly, this is what is expected from HFP 1.5 HF/AG devices.
> ---
>   src/handsfree-audio.c | 93 ++++++++++++++++++++++++++++++++++++++++++++++++++-
>   1 file changed, 92 insertions(+), 1 deletion(-)
>

Patch has been applied, thanks.

Regards,
-Denis

Re: [PATCH 2/5] handsfree-audio: Add support for sending the SCO socket

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 293 bytes --]

Hi Vinicius,

On 03/25/2013 05:05 PM, Vinicius Costa Gomes wrote:
> Send the SCO socket to the agent associated with the card that
> just got connected.
> ---
>   src/handsfree-audio.c | 2 ++
>   1 file changed, 2 insertions(+)

Patch has been applied, thanks.

Regards,
-Denis

Re: [PATCH 3/5] handsfree-audio: Reject .Connect() from other senders

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 276 bytes --]

Hi Vinicius,

On 03/25/2013 05:05 PM, Vinicius Costa Gomes wrote:
> Only the agent should be able to call .Connect() on the card.
> ---
>   src/handsfree-audio.c | 6 ++++++
>   1 file changed, 6 insertions(+)
>

Patch has been applied, thanks.

Regards,
-Denis

Re: [PATCH 4/5] hfp_hf_bluez5: Add a card driver for HFP 1.6

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 2453 bytes --]

Hi Vinicius,

On 03/25/2013 05:05 PM, Vinicius Costa Gomes wrote:
> This is just the skeleton of a Handsfree Audio Card for the HF side of
> HFP 1.6.
> ---
>   plugins/hfp_hf_bluez5.c | 37 +++++++++++++++++++++++++++++++++++++
>   1 file changed, 37 insertions(+)
>
> diff --git a/plugins/hfp_hf_bluez5.c b/plugins/hfp_hf_bluez5.c
> index ff8afba..89b60c8 100644
> --- a/plugins/hfp_hf_bluez5.c
> +++ b/plugins/hfp_hf_bluez5.c
> @@ -60,6 +60,8 @@
>
>   #define HFP_EXT_PROFILE_PATH   "/bluetooth/profile/hfp_hf"
>
> +#define HFP16_HF_DRIVER		"hfp16-hf-driver"
> +
>   struct hfp {
>   	struct hfp_slc_info info;
>   	DBusMessage *msg;
> @@ -310,6 +312,31 @@ static struct ofono_modem_driver hfp_driver = {
>   	.post_sim	= hfp_post_sim,
>   };
>
> +static int hfp16_card_probe(struct ofono_handsfree_card *card,
> +					unsigned int vendor, void *data)
> +{
> +	return 0;
> +}
> +
> +static void hfp16_card_remove(struct ofono_handsfree_card *card)
> +{
> +
> +}
> +
> +static int hfp16_card_connect(struct ofono_handsfree_card *card,
> +					ofono_handsfree_card_connect_cb_t cb,
> +					void *data)
> +{
> +	return -ENOSYS;

This function should be declared to return void to be consistent with 
everything else.  Whoops.  My fault.  I fixed it upstream.

> +}
> +
> +static struct ofono_handsfree_card_driver hfp16_hf_driver = {
> +	.name		= HFP16_HF_DRIVER,
> +	.probe		= hfp16_card_probe,
> +	.remove		= hfp16_card_remove,
> +	.connect	= hfp16_card_connect,
> +};
> +
>   static ofono_bool_t device_path_compare(struct ofono_modem *modem,
>   					void *userdata)
>   {
> @@ -616,6 +643,13 @@ static int hfp_init(void)
>   		return -EIO;
>   	}
>
> +	err = ofono_handsfree_card_driver_register(&hfp16_hf_driver);
> +	if (err<  0) {
> +		g_dbus_unregister_interface(conn, HFP_EXT_PROFILE_PATH,
> +						BLUEZ_PROFILE_INTERFACE);
> +		return err;
> +	}
> +
>   	err = ofono_modem_driver_register(&hfp_driver);
>   	if (err<  0) {
>   		g_dbus_unregister_interface(conn, HFP_EXT_PROFILE_PATH,
> @@ -647,6 +681,9 @@ static void hfp_exit(void)
>   	bt_unregister_profile(conn, HFP_EXT_PROFILE_PATH);
>   	g_dbus_unregister_interface(conn, HFP_EXT_PROFILE_PATH,
>   						BLUEZ_PROFILE_INTERFACE);
> +
> +	ofono_handsfree_card_driver_unregister(&hfp16_hf_driver);
> +
>   	ofono_modem_driver_unregister(&hfp_driver);
>   	g_dbus_client_unref(bluez);
>

Regards,
-Denis

Re: [PATCH] handsfree-audio: Add .Connect using the card driver

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 3019 bytes --]

Hi Vinicius,

On 03/26/2013 04:28 PM, Vinicius Costa Gomes wrote:
> Now each handsfree implementation may be notified that a card wants
> its audio to be connected.
>
> In case, that the cards wishes to fallback to the default SCO connection
> procedure, it just needs to return -ENOSYS.

Are you interpreting section 4.11.2:
"For all HF initiated audio connection establishments for which both 
sides support the Codec Negotiation feature, the HF shall trigger the AG 
to establish a Codec Connection. This is necessary because only the AG 
knows about the codec selection and settings of the network."

... to mean that AT+BCC is only sent when both sides support codec 
negotiation?  e.g. if one or the other side does not support codec 
negotiation, then we are supposed to fall back to old SCO establishment 
rules procedures?

> ---
>   src/handsfree-audio.c | 34 +++++++++++++++++++++++++++++++++-
>   1 file changed, 33 insertions(+), 1 deletion(-)
>
> diff --git a/src/handsfree-audio.c b/src/handsfree-audio.c
> index f8df6d6..ace3a4f 100644
> --- a/src/handsfree-audio.c
> +++ b/src/handsfree-audio.c
> @@ -307,13 +307,30 @@ done:
>   	return FALSE;
>   }
>
> +static void card_connect_reply_cb(const struct ofono_error *error, void *data)
> +{
> +	struct ofono_handsfree_card *card = data;
> +	DBusMessage *reply;
> +
> +	if (error->type == OFONO_ERROR_TYPE_NO_ERROR)
> +		reply = dbus_message_new_method_return(card->msg);
> +	else
> +		reply = __ofono_error_failed(card->msg);
> +
> +	g_dbus_send_message(ofono_dbus_get_connection(), reply);
> +
> +	dbus_message_unref(card->msg);
> +	card->msg = NULL;

Please replace the above three lines with __ofono_dbus_pending_reply.

> +}
> +
>   static DBusMessage *card_connect(DBusConnection *conn,
>   						DBusMessage *msg, void *data)
>   {
>   	struct ofono_handsfree_card *card = data;
> +	const struct ofono_handsfree_card_driver *driver = card->driver;
>   	GIOChannel *io;
>   	const char *sender;
> -	int sk;
> +	int sk, err;
>
>   	if (agent == NULL)
>   		return __ofono_error_not_available(msg);
> @@ -326,6 +343,21 @@ static DBusMessage *card_connect(DBusConnection *conn,
>   	if (card->msg)
>   		return __ofono_error_busy(msg);
>
> +	if (!driver || !driver->connect)
> +		goto fallback;
> +
> +	err = driver->connect(card, card_connect_reply_cb, card);
> +	if (err == -EINPROGRESS) {
> +		card->msg = dbus_message_ref(msg);
> +		return NULL;

As mentioned earlier, the driver should not return an int, however, lets 
revisit this part.  I want to better understand the AT+BCC and codec 
negotiation interactions.

> +	}
> +
> +	/* ENOSYS means the driver doesn't want to connect itself */
> +	if (err<  0&&  err != -ENOSYS)
> +		return __ofono_error_failed(msg);
> +
> +fallback:
> +	/* There's no driver, fallback to direct SCO connection */
>   	sk = card_connect_sco(card);
>   	if (sk<  0)
>   		return __ofono_error_failed(msg);

Regards,
-Denis

Re: [PATCH] handsfree-audio: Add .Connect using the card driver

[Vinicius Costa Gomes]

[-- Attachment #1: Type: text/plain, Size: 3960 bytes --]

Hi Denis,

On 23:59 Tue 26 Mar, Denis Kenzior wrote:
> Hi Vinicius,
> 
> On 03/26/2013 04:28 PM, Vinicius Costa Gomes wrote:
> >Now each handsfree implementation may be notified that a card wants
> >its audio to be connected.
> >
> >In case, that the cards wishes to fallback to the default SCO connection
> >procedure, it just needs to return -ENOSYS.
> 
> Are you interpreting section 4.11.2:
> "For all HF initiated audio connection establishments for which both
> sides support the Codec Negotiation feature, the HF shall trigger
> the AG to establish a Codec Connection. This is necessary because
> only the AG knows about the codec selection and settings of the
> network."
> 
> ... to mean that AT+BCC is only sent when both sides support codec
> negotiation?  e.g. if one or the other side does not support codec
> negotiation, then we are supposed to fall back to old SCO
> establishment rules procedures?

Yes. This behaviour is even covered in one of the PTS tests: TP/ACC/BV-03-I
(from the description of the test, "To verify the capability of the HF
initiating a legacy audio connection with an AG that does not support the
Codec Connection setup procedure. The HF should be able to correctly negotiate
a working legacy audio connection to the AG.")

> 
> >---
> >  src/handsfree-audio.c | 34 +++++++++++++++++++++++++++++++++-
> >  1 file changed, 33 insertions(+), 1 deletion(-)
> >
> >diff --git a/src/handsfree-audio.c b/src/handsfree-audio.c
> >index f8df6d6..ace3a4f 100644
> >--- a/src/handsfree-audio.c
> >+++ b/src/handsfree-audio.c
> >@@ -307,13 +307,30 @@ done:
> >  	return FALSE;
> >  }
> >
> >+static void card_connect_reply_cb(const struct ofono_error *error, void *data)
> >+{
> >+	struct ofono_handsfree_card *card = data;
> >+	DBusMessage *reply;
> >+
> >+	if (error->type == OFONO_ERROR_TYPE_NO_ERROR)
> >+		reply = dbus_message_new_method_return(card->msg);
> >+	else
> >+		reply = __ofono_error_failed(card->msg);
> >+
> >+	g_dbus_send_message(ofono_dbus_get_connection(), reply);
> >+
> >+	dbus_message_unref(card->msg);
> >+	card->msg = NULL;
> 
> Please replace the above three lines with __ofono_dbus_pending_reply.

Sure. Will fix.

> 
> >+}
> >+
> >  static DBusMessage *card_connect(DBusConnection *conn,
> >  						DBusMessage *msg, void *data)
> >  {
> >  	struct ofono_handsfree_card *card = data;
> >+	const struct ofono_handsfree_card_driver *driver = card->driver;
> >  	GIOChannel *io;
> >  	const char *sender;
> >-	int sk;
> >+	int sk, err;
> >
> >  	if (agent == NULL)
> >  		return __ofono_error_not_available(msg);
> >@@ -326,6 +343,21 @@ static DBusMessage *card_connect(DBusConnection *conn,
> >  	if (card->msg)
> >  		return __ofono_error_busy(msg);
> >
> >+	if (!driver || !driver->connect)
> >+		goto fallback;
> >+
> >+	err = driver->connect(card, card_connect_reply_cb, card);
> >+	if (err == -EINPROGRESS) {
> >+		card->msg = dbus_message_ref(msg);
> >+		return NULL;
> 
> As mentioned earlier, the driver should not return an int, however,
> lets revisit this part.  I want to better understand the AT+BCC and
> codec negotiation interactions.


Of course. 

Another thing that may make sense to bring up now is the AG case: if we want
to have a HFP 1.6 capable AG, we may want to select the codec (send a +BCS)
before establishing the SCO connection. I was thinking of setting a specific
error code in the callback to signify that the core handsfree-audio may
connect the SCO at that point.


> 
> >+	}
> >+
> >+	/* ENOSYS means the driver doesn't want to connect itself */
> >+	if (err<  0&&  err != -ENOSYS)
> >+		return __ofono_error_failed(msg);
> >+
> >+fallback:
> >+	/* There's no driver, fallback to direct SCO connection */
> >  	sk = card_connect_sco(card);
> >  	if (sk<  0)
> >  		return __ofono_error_failed(msg);
> 
> Regards,
> -Denis


Cheers,
-- 
Vinicius

Re: [PATCH] handsfree-audio: Add .Connect using the card driver

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 2800 bytes --]

Hi Vinicius,

On 03/27/2013 09:22 AM, Vinicius Costa Gomes wrote:
> Hi Denis,
>
> On 23:59 Tue 26 Mar, Denis Kenzior wrote:
>> Hi Vinicius,
>>
>> On 03/26/2013 04:28 PM, Vinicius Costa Gomes wrote:
>>> Now each handsfree implementation may be notified that a card wants
>>> its audio to be connected.
>>>
>>> In case, that the cards wishes to fallback to the default SCO connection
>>> procedure, it just needs to return -ENOSYS.
>>
>> Are you interpreting section 4.11.2:
>> "For all HF initiated audio connection establishments for which both
>> sides support the Codec Negotiation feature, the HF shall trigger
>> the AG to establish a Codec Connection. This is necessary because
>> only the AG knows about the codec selection and settings of the
>> network."
>>
>> ... to mean that AT+BCC is only sent when both sides support codec
>> negotiation?  e.g. if one or the other side does not support codec
>> negotiation, then we are supposed to fall back to old SCO
>> establishment rules procedures?
>
> Yes. This behaviour is even covered in one of the PTS tests: TP/ACC/BV-03-I
> (from the description of the test, "To verify the capability of the HF
> initiating a legacy audio connection with an AG that does not support the
> Codec Connection setup procedure. The HF should be able to correctly negotiate
> a working legacy audio connection to the AG.")
>

Okay, that makes things a little harder since we cannot determine the 
driver to use until the SLC is established.

<snip>

>>> +	if (!driver || !driver->connect)
>>> +		goto fallback;
>>> +
>>> +	err = driver->connect(card, card_connect_reply_cb, card);
>>> +	if (err == -EINPROGRESS) {
>>> +		card->msg = dbus_message_ref(msg);
>>> +		return NULL;
>>
>> As mentioned earlier, the driver should not return an int, however,
>> lets revisit this part.  I want to better understand the AT+BCC and
>> codec negotiation interactions.
>
>
> Of course.
>
> Another thing that may make sense to bring up now is the AG case: if we want
> to have a HFP 1.6 capable AG, we may want to select the codec (send a +BCS)
> before establishing the SCO connection. I was thinking of setting a specific
> error code in the callback to signify that the core handsfree-audio may
> connect the SCO at that point.
>

I thought of setting error codes in callbacks, but I'm not happy with 
that solution.  Perhaps we should make SCO establishment a part of the 
card utility API.  e.g. ofono_handsfree_card_connect_sco().  That way 
the AG can run the required +BCS transactions and call the above 
function when it comes to making the SCO connection.

For HFP 1.6/1.5, we can either use +BCC or call 
ofono_handsfree_card_connect_sco directly.

Thoughts, other ideas?

Regards,
-Denis

Re: [PATCH] handsfree-audio: Add .Connect using the card driver

[Vinicius Costa Gomes]

[-- Attachment #1: Type: text/plain, Size: 2145 bytes --]

Hi Denis,

On 09:32 Wed 27 Mar, Denis Kenzior wrote:

> >
> >Yes. This behaviour is even covered in one of the PTS tests: TP/ACC/BV-03-I
> >(from the description of the test, "To verify the capability of the HF
> >initiating a legacy audio connection with an AG that does not support the
> >Codec Connection setup procedure. The HF should be able to correctly negotiate
> >a working legacy audio connection to the AG.")
> >
> 
> Okay, that makes things a little harder since we cannot determine
> the driver to use until the SLC is established.


Yeah.

> 
> <snip>
> 
> >>>+	if (!driver || !driver->connect)
> >>>+		goto fallback;
> >>>+
> >>>+	err = driver->connect(card, card_connect_reply_cb, card);
> >>>+	if (err == -EINPROGRESS) {
> >>>+		card->msg = dbus_message_ref(msg);
> >>>+		return NULL;
> >>
> >>As mentioned earlier, the driver should not return an int, however,
> >>lets revisit this part.  I want to better understand the AT+BCC and
> >>codec negotiation interactions.
> >
> >
> >Of course.
> >
> >Another thing that may make sense to bring up now is the AG case: if we want
> >to have a HFP 1.6 capable AG, we may want to select the codec (send a +BCS)
> >before establishing the SCO connection. I was thinking of setting a specific
> >error code in the callback to signify that the core handsfree-audio may
> >connect the SCO at that point.
> >
> 
> I thought of setting error codes in callbacks, but I'm not happy
> with that solution.  Perhaps we should make SCO establishment a part
> of the card utility API.  e.g. ofono_handsfree_card_connect_sco().
> That way the AG can run the required +BCS transactions and call the
> above function when it comes to making the SCO connection.

Sounds good. This aproach may even make the case that "we" are the AG and we
receive a AT+BCC easier to handle.

> 
> For HFP 1.6/1.5, we can either use +BCC or call
> ofono_handsfree_card_connect_sco directly.
> 
> Thoughts, other ideas?

Right now, I don't have any. I will give it a try and see how it ends up
looking.

> 
> Regards,
> -Denis


Cheers,
-- 
Vinicius