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

[Tanu Kaskinen]

On Mon, 2014-09-08 at 10:40 +0300, Luiz Augusto von Dentz wrote:
> 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.

Ok, do you think the documentation will appear soon?

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

Well, my thinking is that if the an object (transport) is permanently
associated with another object (card), there should be a way to get from
the first object to the second object by following one or more pointers,
rather than having to search for it from some container.

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

What if Connect() fails, do we need to react to that somehow?

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

Nice to know, but I don't think this really answers my questions :) Are
there some situations where the hardcoded 48 doesn't work or works
somehow suboptimally?

-- 
Tanu