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

[Denis Kenzior <denkenz@gmail.com>]

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

Hi Vinicius,

On 03/20/2013 06:03 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 | 97 ++++++++++++++++++++++++++++++++++++++++++++++++++-
>   1 file changed, 96 insertions(+), 1 deletion(-)
>
> diff --git a/src/handsfree-audio.c b/src/handsfree-audio.c
> index c7fa2fb..1da2224 100644
> --- a/src/handsfree-audio.c
> +++ b/src/handsfree-audio.c
> @@ -53,6 +53,8 @@ struct ofono_handsfree_card {
>   	char *remote;
>   	char *local;
>   	char *path;
> +	DBusMessage *msg;
> +	guint sco_watch;
>   	const struct ofono_handsfree_card_driver *driver;
>   	void *driver_data;
>   };
> @@ -235,10 +237,103 @@ 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)
> +		return FALSE;
> +

This seems wrong.  We can't just return here.

> +	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:
> +	g_dbus_send_message(ofono_dbus_get_connection(), reply);
> +
> +	return FALSE;
> +}
> +
> +static void sco_watch_destroy(gpointer user_data)
> +{
> +	struct ofono_handsfree_card *card = user_data;
> +
> +	card->sco_watch = 0;
> +	dbus_message_unref(card->msg);
> +}
> +

I would get rid of sco_watch_destroy.  Once we go to AT+BCC, the 
sco_watch and card->msg will be unrelated.

>   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->sco_watch)
> +		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);
> +	card->sco_watch = g_io_add_watch_full(io, G_PRIORITY_DEFAULT,
> +				G_IO_OUT | G_IO_ERR | G_IO_HUP | G_IO_NVAL,
> +				sco_connect_cb, card, sco_watch_destroy);
> +
> +	g_io_channel_unref(io);
> +
> +	card->msg = dbus_message_ref(msg);
> +
> +	return NULL;
>   }
>
>   static const GDBusMethodTable card_methods[] = {

Regards,
-Denis