[PATCH 1/3] ofono.conf: Punch hole for HandsfreeAudioAgent

[PATCH 1/3] ofono.conf: Punch hole for HandsfreeAudioAgent

[Vinicius Costa Gomes]

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

---
 src/ofono.conf | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/ofono.conf b/src/ofono.conf
index 8a83cd0..ed56d3b 100644
--- a/src/ofono.conf
+++ b/src/ofono.conf
@@ -14,6 +14,7 @@
     <allow send_interface="org.ofono.PushNotificationAgent"/>
     <allow send_interface="org.ofono.SmartMessagingAgent"/>
     <allow send_interface="org.ofono.PositioningRequestAgent"/>
+    <allow send_interface="org.ofono.HandsfreeAudioAgent"/>
   </policy>
 
   <policy at_console="true">
-- 
1.8.1.5

[PATCH 2/3] handsfree-audio: Add support for initiating SCO connections

[Vinicius Costa Gomes]

[-- Attachment #1: Type: text/plain, Size: 3175 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 | 94 ++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 93 insertions(+), 1 deletion(-)

diff --git a/src/handsfree-audio.c b/src/handsfree-audio.c
index c7fa2fb..9eecb8e 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,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 == EAGAIN || 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 (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);
+}
+
 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);
+	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[] = {
-- 
1.8.1.5

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

[Vinicius Costa Gomes]

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

When there's an agent registered, send the SCO file descriptor to the
agent associated with the card after the connection is established.
---
 src/handsfree-audio.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/src/handsfree-audio.c b/src/handsfree-audio.c
index 9eecb8e..129f33f 100644
--- a/src/handsfree-audio.c
+++ b/src/handsfree-audio.c
@@ -286,6 +286,9 @@ static gboolean sco_connect_cb(GIOChannel *io, GIOCondition cond,
 
 	sk = g_io_channel_unix_get_fd(io);
 
+	if (agent)
+		send_new_connection(card->path, sk);
+
 	close(sk);
 
 	reply = dbus_message_new_method_return(card->msg);
-- 
1.8.1.5

Re: [PATCH 1/3] ofono.conf: Punch hole for HandsfreeAudioAgent

[Denis Kenzior]

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

Hi Vinicius,

On 03/19/2013 07:10 PM, Vinicius Costa Gomes wrote:
> ---
>   src/ofono.conf | 1 +
>   1 file changed, 1 insertion(+)
>

Patch has been applied, thanks.

Regards,
-Denis

Re: [PATCH 2/3] handsfree-audio: Add support for initiating SCO connections

[Denis Kenzior]

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

Hi Vinicius,

On 03/19/2013 07:10 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 | 94 ++++++++++++++++++++++++++++++++++++++++++++++++++-
>   1 file changed, 93 insertions(+), 1 deletion(-)
>
> diff --git a/src/handsfree-audio.c b/src/handsfree-audio.c
> index c7fa2fb..9eecb8e 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,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 == EAGAIN || errno == EINPROGRESS)) {

Why do we use EAGAIN here?  I thought only EINPROGRESS can be returned 
from a non-blocking connect.  Or are Bluetooth semantics different?

> +		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 (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);
> +

We probably should be paranoid and check whether we still have the agent.

> +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);
> +}
> +
>   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);

I presume this fails if we already have a SCO link?

> +	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

Re: [PATCH 2/3] handsfree-audio: Add support for initiating SCO connections

[Vinicius Costa Gomes]

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

Hi Denis,

On 22:38 Tue 19 Mar, Denis Kenzior wrote:
> Hi Vinicius,
> 
> On 03/19/2013 07:10 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 | 94 ++++++++++++++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 93 insertions(+), 1 deletion(-)
> >
> >diff --git a/src/handsfree-audio.c b/src/handsfree-audio.c
> >index c7fa2fb..9eecb8e 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,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 == EAGAIN || errno == EINPROGRESS)) {
> 
> Why do we use EAGAIN here?  I thought only EINPROGRESS can be
> returned from a non-blocking connect.  Or are Bluetooth semantics
> different?

Just checked, you are right: only EINPROGRSS will be returned in this case.
Will fix.

> 
> >+		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 (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);
> >+
> 
> We probably should be paranoid and check whether we still have the agent.

For the method return message of the 'Connect()' call, I can't see any gains of
this check, but for sending the 'NewConnection()' message I can (and that is
what the next patch does). Could be that I am missing something.

One thing I just noticed that is missing is that there's no check that only 
the agent may call 'Connect()'. 

> 
> >+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);
> >+}
> >+
> >  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);
> 
> I presume this fails if we already have a SCO link?

It will, but I will replace the check above (card->msg) with a check for
sco_watch, to make it clearer.

> 
> >+	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


Cheers,
-- 
Vinicius

Re: [PATCH 2/3] handsfree-audio: Add support for initiating SCO connections

[Denis Kenzior]

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

Hi Vinicius,

On 03/20/2013 10:15 AM, Vinicius Costa Gomes wrote:
> Hi Denis,
>
> On 22:38 Tue 19 Mar, Denis Kenzior wrote:
>> Hi Vinicius,
>>
>> On 03/19/2013 07:10 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 | 94 ++++++++++++++++++++++++++++++++++++++++++++++++++-
>>>   1 file changed, 93 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/src/handsfree-audio.c b/src/handsfree-audio.c
>>> index c7fa2fb..9eecb8e 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,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 == EAGAIN || errno == EINPROGRESS)) {
>>
>> Why do we use EAGAIN here?  I thought only EINPROGRESS can be
>> returned from a non-blocking connect.  Or are Bluetooth semantics
>> different?
>
> Just checked, you are right: only EINPROGRSS will be returned in this case.
> Will fix.
>
>>
>>> +		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 (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);
>>> +
>>
>> We probably should be paranoid and check whether we still have the agent.
>
> For the method return message of the 'Connect()' call, I can't see any gains of
> this check, but for sending the 'NewConnection()' message I can (and that is
> what the next patch does). Could be that I am missing something.

Just being symmetrical.  In the Connect() implementation you return a 
not_available error if the agent is not registered (before starting the 
connection procedure).

>
> One thing I just noticed that is missing is that there's no check that only
> the agent may call 'Connect()'.
>

Yes, put this in for now.  We might or might not want to relax this 
condition later.

>>
>>> +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);
>>> +}
>>> +
>>>   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);
>>
>> I presume this fails if we already have a SCO link?
>
> It will, but I will replace the check above (card->msg) with a check for
> sco_watch, to make it clearer.
>
>>
>>> +	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
>
>
> Cheers,

Regards,
-Denis