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

[Denis Kenzior <denkenz@gmail.com>]

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