[pulseaudio-discuss] [PATCH 5/5] backend-native: add a new native headset backend

[Wim Taymans]

> > +    if (imtu)
> > +        *imtu = 48;
> > +
> > +    if (omtu)
> > +        *omtu = 48;
>
> Out of curiosity, are these values fixed in the spec?
>

They are not but the kernel does not want to give us the real MTU values so
this is hardcoded for now.

> +    if (events & PA_IO_EVENT_INPUT) {
> > +        char buf[512];
> > +        ssize_t len;
> > +
> > +        len = read (fd, buf, 511);
> > +        buf[len] = 0;
> > +        pa_log("RFCOMM << %s", buf);
> > +
> > +        pa_log("RFCOMM >> OK");
> > +        len = write (fd, "\r\nOK\r\n", 5);
> > +        if (len < 0)
> > +            pa_log_error("RFCOMM write error: %s", pa_cstrerror(errno));
>
> Should we go to fail if this happens?
>

I'm ignoring this error for now, it's not critical and I expect real
failures such as disconnection etc to be handled with the HANGUP and ERROR
events.


>  d = pa_bluetooth_discovery_get_device_by_path(b->discovery, path);
> > +    if (d == NULL) {
> > +        pa_log_error("Device doesnt exist for %s", path);
> > +        goto fail;
> > +    }
> > +
> > +    if (pa_streq(handler, HSP_AG_PROFILE)) {
> > +        p = PA_BLUETOOTH_PROFILE_HEADSET_HEAD_UNIT;
> > +    } else
> > +        goto refused;
>
> Is the else possible at all?
>
>
It should not be, I'll make it assert.



> > +        dbus_message_iter_recurse(&element_i, &dict_i);
> > +
> > +        if (key == NULL)
> > +            break;
> > +        if (dbus_message_iter_get_arg_type(&dict_i) != DBUS_TYPE_STRING)
> > +            break;
> > +
> > +        dbus_message_iter_get_basic(&dict_i, &key);
> > +
> > +        if (!dbus_message_iter_next(&dict_i))
> > +            break;
> > +
> > +        if (dbus_message_iter_get_arg_type(&dict_i) !=
> DBUS_TYPE_VARIANT)
> > +            break;
> > +
> > +        pa_log_debug("key=%s", key);
> > +
> > +        dbus_message_iter_next(&element_i);
> > +    }
>
> If we're not doing anything with the keys, maybe we don't need to
> parse any of this?
>

I'll remove it. It should only contain a version number but I don't plan to
do anything with that for now.


>
> > +    trfc =  pa_xnew0(struct transport_rfcomm, 1);
> > +    trfc->rfcomm_fd = fd;
> > +    trfc->rfcomm_io = b->core->mainloop->io_new(b->core->mainloop, fd,
> PA_IO_EVENT_INPUT|PA_IO_EVENT_HANGUP,
> > +        rfcomm_io_callback, t);
>
> Can reading/writing on the RFCOMM fd block the mainloop for long periods?
>

The socket flags are set G_IO_FLAG_NONBLOCK in bluez, AFAICS.


>
> > +    trfc->backend = b;
>
> Can the backend be destroyed before the transport? Maybe we should
> only reference the mainloop instead of the backend here.
>

I'll have a look if all cleanup scenarios work fine. We should probably
just ref the mainloop, indeed.



> > +static DBusMessage *profile_request_disconnection(DBusConnection *conn,
> DBusMessage *m, void *userdata) {
> > +    DBusMessage *r;
> > +
> > +    pa_assert_se(r = dbus_message_new_method_return(m));
>
> How is the transport torn down in this case?
>

Bluez closes the RFCOMM socket, that makes us stop. Another case is when
the SCO socket is closed. We free the transport, which makes us remove the
device. I'll see if I can close it myself instead.


>
> > +
> > +    } else if (dbus_message_is_method_call(m, BLUEZ_PROFILE_INTERFACE,
> "Release")) {
> > +    } else if (dbus_message_is_method_call(m, BLUEZ_PROFILE_INTERFACE,
> "Cancel")) {
>
> There doesn't actually seem to be a 'Cancel' method.
>

I will remove it. It looks like it is supposed to be part of the API here:

http://git.kernel.org/cgit/bluetooth/bluez.git/tree/profiles/iap/main.c#n228

and in some exampled, but it does not look like it is used here:

is http://git.kernel.org/cgit/bluetooth/bluez.git/tree/src/profile.c

> +    switch(profile) {
> > +        case PA_BLUETOOTH_PROFILE_HEADSET_HEAD_UNIT:
> > +            object_name = HSP_AG_PROFILE;
> > +            uuid = PA_BLUETOOTH_UUID_HSP_AG;
> > +            break;
> > +        default:
> > +            pa_assert_not_reached();
> > +            break;
> > +    }
> > +
> pa_assert_se(dbus_connection_register_object_path(pa_dbus_connection_get(b->connection),
> > +                  object_name, &vtable_profile, b));
> > +    register_profile (b, object_name, uuid);
>
> There doesn't seem to be a way to handle failure when registering the
> profile. Should we perhaps be unregistering the object path?
>
> yes, will do.


> > +
> > +    profile_done(backend, PA_BLUETOOTH_PROFILE_HEADSET_HEAD_UNIT);
>
> backend->connection needs to be unref'ed here.
>
> Will do,

Thanks for reviewing, will post new patches with all suggestions and fixes
soon,

Wim
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/pulseaudio-discuss/attachments/20140915/554d6e81/attachment.html>