[pulseaudio-discuss] [PATCH 06/17] bluetooth: Implement transport acquire for hf_audio_agent transports

[Luiz Augusto von Dentz]

Hi Tanu,

On Fri, Sep 5, 2014 at 4:03 PM, Tanu Kaskinen
<tanu.kaskinen at linux.intel.com> wrote:
> On Fri, 2014-08-22 at 17:04 +0300, Luiz Augusto von Dentz wrote:
>> From: João Paulo Rechi Vita <jprvita at openbossa.org>
>>
>> ---
>>  src/modules/bluetooth/backend-ofono.c | 66 ++++++++++++++++++++++++++++++++++-
>>  1 file changed, 65 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/modules/bluetooth/backend-ofono.c b/src/modules/bluetooth/backend-ofono.c
>> index 79765bb..5fb99de 100644
>> --- a/src/modules/bluetooth/backend-ofono.c
>> +++ b/src/modules/bluetooth/backend-ofono.c
>> @@ -23,9 +23,13 @@
>>  #include <config.h>
>>  #endif
>>
>> +#include <errno.h>
>> +#include <poll.h>
>> +
>>  #include <pulsecore/core-util.h>
>>  #include <pulsecore/dbus-shared.h>
>>  #include <pulsecore/shared.h>
>> +#include <pulsecore/core-error.h>
>>
>>  #include "bluez5-util.h"
>>
>> @@ -118,8 +122,68 @@ static void hf_audio_card_free(void *data) {
>>      pa_xfree(card);
>>  }
>>
>> +static int socket_accept(int sock)
>> +{
>> +    char c;
>> +    struct pollfd pfd;
>> +
>> +    if (sock < 0)
>> +        return -ENOTCONN;
>> +
>> +    memset(&pfd, 0, sizeof(pfd));
>> +    pfd.fd = sock;
>> +    pfd.events = POLLOUT;
>> +
>> +    if (poll(&pfd, 1, 0) < 0)
>> +        return -errno;
>> +
>> +    /* If socket already writable then it is not in defer setup state */
>
> I think this "defer setup" thing needs to be explained somewhere. What
> is it, why is it needed?

This can probably be added to NewConnection documentation, Defer Setup
is used to authorize connection, the connection setup is interrupted
while in the this state waiting for userspace to authorize which is
done by read.

>> +    if ((pfd.revents & POLLOUT))
>> +        return 0;
>> +
>> +    /* Enable socket by reading 1 byte */
>> +    if (read(sock, &c, 1) < 0)
>> +        return -errno;
>> +
>> +    return 0;
>> +}
>> +
>>  static int hf_audio_agent_transport_acquire(pa_bluetooth_transport *t, bool optional, size_t *imtu, size_t *omtu) {
>> -    return -1;
>> +    pa_bluetooth_backend *backend = t->userdata;
>> +    hf_audio_card *card;
>> +    int err;
>> +
>> +    pa_assert(backend);
>> +    pa_assert_se(card = pa_hashmap_get(backend->cards, t->path));
>
> I think it would be good to set the transport userdata to point to the
> hf_audio_card, and get the backend from the hf_audio_card object.

Either way it is fine with me, we just need to be consistent I guess.

>> +
>> +    if (!optional) {
>> +        DBusMessage *m;
>> +
>> +        pa_assert_se(m = dbus_message_new_method_call(t->owner, t->path, "org.ofono.HandsfreeAudioCard", "Connect"));
>> +        pa_assert_se(dbus_connection_send(pa_dbus_connection_get(backend->connection), m, NULL));
>> +
>> +        return -1;
>
> I'd like to have a comment here explaining what !optional means and why
> it's a good idea to send a Connect message and then return -1. Is this
> some kind of an "asynchronous acquire" thing, i.e. the acquire operation
> will complete once oFono calls NewConnection()? If Connect() returns an
> error, do we need to care about that?

I guess it should be named overwrite or something like that, the idea
is that this would force SCO to connect but since the operation is
asynchronous we have to return -1 because Connect the method does not
return the fd immediately.

>> +    }
>> +
>> +    /* The correct block size should take into account the SCO MTU from
>> +     * the Bluetooth adapter and (for adapters in the USB bus) the MxPS
>> +     * value from the Isoc USB endpoint in use by btusb and should be
>> +     * made available to userspace by the Bluetooth kernel subsystem.
>> +     * Meanwhile the empiric value 48 will be used. */
>
> Do you know what downsides the hardcoding has? Those could be documented
> here. Also, is the kernel bug tracked somewhere?

Actually we have been debating whether kernel could actually do some
sort of flow control, currently the socket MTU is doubled so if we
read via socket options it would be 96.

-- 
Luiz Augusto von Dentz