[pulseaudio-discuss] [PATCHv2 27/60] bluetooth: Create pa_bluetooth_transport for BlueZ 5 support

[Tanu Kaskinen]

On Mon, 2013-08-19 at 13:44 -0300, João Paulo Rechi Vita wrote:
> On Fri, Aug 16, 2013 at 12:32 PM, Tanu Kaskinen
> <tanu.kaskinen at linux.intel.com> wrote:
> > On Tue, 2013-08-13 at 01:54 -0300, jprvita at gmail.com wrote:
> >> From: João Paulo Rechi Vita <jprvita at openbossa.org>
> >>
> >> Create the pa_bluetooth_transport structure to store information about
> >> the bluetooth transport and utility functions to manipulate this
> >> structure. The acquire() and release() operations are function pointers
> >> in the pa_bluetooth_transport structure to make possible for different
> >> transport backends to provide different implementations of these
> >> operations. Thre is also a userdata field for the transport backend
> >> provide data for the acquire/release functions.
> >> ---
> >>  src/modules/bluetooth/bluez5-util.c | 145 ++++++++++++++++++++++++++++++++++++
> >>  src/modules/bluetooth/bluez5-util.h |  43 +++++++++++
> >>  2 files changed, 188 insertions(+)
> >>
> >> diff --git a/src/modules/bluetooth/bluez5-util.c b/src/modules/bluetooth/bluez5-util.c
> >> index 1972a92..69eb759 100644
> >> --- a/src/modules/bluetooth/bluez5-util.c
> >> +++ b/src/modules/bluetooth/bluez5-util.c
> >> @@ -36,6 +36,7 @@
> >>  #include "bluez5-util.h"
> >>
> >>  #define BLUEZ_SERVICE "org.bluez"
> >> +#define BLUEZ_MEDIA_TRANSPORT_INTERFACE BLUEZ_SERVICE ".MediaTransport1"
> >>
> >>  struct pa_bluetooth_discovery {
> >>      PA_REFCNT_DECLARE;
> >> @@ -45,8 +46,131 @@ struct pa_bluetooth_discovery {
> >>      bool filter_added;
> >>      pa_hook hooks[PA_BLUETOOTH_HOOK_MAX];
> >>      pa_hashmap *devices;
> >> +    pa_hashmap *transports;
> >>  };
> >>
> >> +pa_bluetooth_transport *pa_bluetooth_transport_new(pa_bluetooth_device *d, const char *owner, const char *path,
> >> +                                                          pa_bluetooth_profile_t p, const uint8_t *config, size_t size) {
> >
> > Unaligned parameter list.
> >
> > There is some precedence for wrapping long parameter lists like this:
> >
> > pa_bluetooth_transport *pa_bluetooth_transport_new(
> >         pa_bluetooth_device *d,
> >         const char *owner,
> >         const char *path,
> >         pa_bluetooth_profile_t p,
> >         const uint8_t *config,
> >         size_t size) {
> >
> 
> Ok. Maybe we should add this to the official coding style guidelines
> in the wiki, although I personally dislike having the function
> parameters aligned in the same level as the function implementation.
> My preference would be to have them aligned after the opening
> parenthesis.

The parameters in my suggestion aren't aligned in the same level as the
function implementation - the indentation for the parameters is 8
spaces, while the function implementation is indented by 4 spaces.

I'll discuss with Arun and David about standardizing this or some other
style.

> >>  static void device_free(pa_bluetooth_device *d) {
> >> +    unsigned i;
> >> +
> >>      pa_assert(d);
> >>
> >> +    for (i = 0; i < PA_BLUETOOTH_PROFILE_COUNT; i++) {
> >> +        pa_bluetooth_transport *t;
> >> +
> >> +        if (!(t = d->transports[i]))
> >> +            continue;
> >> +
> >> +        d->transports[i] = NULL;
> >> +        transport_state_changed(t, PA_BLUETOOTH_TRANSPORT_STATE_DISCONNECTED);
> >> +        /* TODO: check if there is no risk of calling
> >> +         * transport_connection_changed_cb() when the transport is already freed */
> >> +        pa_bluetooth_transport_free(t);
> >
> > IMO pa_bluetooth_transport_free() should be called by the transport
> > code, because the backend also creates the transport. The backend may
> > need a callback for getting notified about the device going away. Or
> > perhaps there should be pa_bluetooth_transport_kill(), with a kill()
> > callback in the backend. This would be similar to how sink inputs are
> > killed if the sink they're connected to goes away.
> >
> > If the backend is responsible for calling pa_bluetooth_transport_free(),
> > the core should still ensure that t->device is set to NULL and that the
> > transport is removed from discovery->transports (feel free to create
> > pa_bluetooth_transport_unlink() for this purpose, if you want).
> >
> 
> I'll answer this comment in the next message in this thread due to the
> additional comment in that message.
> 
> > As for the TODO item, I don't know what it means. There is no
> > transport_connection_changed_cb() function.
> >
> 
> transport_connection_changed_cb() is the callback in
> module-bluez5-device for the hook
> PA_BLUETOOTH_HOOK_TRANSPORT_STATE_CHANGED, which is fired by the
> transport_state_changed() call right before the TODO line. Depending
> on the way hook callbacks are called by the mainloop t may be already
> freed by the pa_bluetooth_transport_free() call after the TODO line.

Ok, so you mean transport_state_changed_cb(), not
transport_connection_changed_cb(). If I understood correctly, you worry
that the hook callbacks might get called asynchronously. Don't worry,
they're always synchronous.

-- 
Tanu