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

[Tanu Kaskinen]

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?

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

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

> +    }
> +
> +    /* 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?

-- 
Tanu