[pulseaudio-discuss] [PATCH] bluetooth: Unload the device module when the device becomes disconnected.

[Tanu Kaskinen]

On Mon, 2012-11-19 at 09:32 +0100, Mikel Astiz wrote:
> Hi Tanu,
> 
> On Sun, Nov 18, 2012 at 10:55 PM, Tanu Kaskinen <tanuk at iki.fi> wrote:
> > The bug was most likely introduced in this commit:
> > http://cgit.freedesktop.org/pulseaudio/pulseaudio/commit/?id=ea45f2c7951a81b2f43029892535d6a6280eb97e
> 
> Yes, I checked this and you're right, it was introduced by this
> commit. Sorry for that.
> 
> > BugLink: https://bugs.freedesktop.org/show_bug.cgi?id=57239
> > ---
> >  src/modules/bluetooth/bluetooth-util.c          |   10 +++++++---
> >  src/modules/bluetooth/bluetooth-util.h          |    1 +
> >  src/modules/bluetooth/module-bluetooth-device.c |   24 ++++++++++++++++++++---
> >  3 files changed, 29 insertions(+), 6 deletions(-)
> >
> > diff --git a/src/modules/bluetooth/bluetooth-util.c b/src/modules/bluetooth/bluetooth-util.c
> > index 272b6ce..402bcdc 100644
> > --- a/src/modules/bluetooth/bluetooth-util.c
> > +++ b/src/modules/bluetooth/bluetooth-util.c
> > @@ -345,9 +345,13 @@ static int parse_device_property(pa_bluetooth_discovery *y, pa_bluetooth_device
> >
> >              if (pa_streq(key, "Paired"))
> >                  d->paired = !!value;
> > -            else if (pa_streq(key, "Connected"))
> > -                d->device_connected = !!value;
> > -            else if (pa_streq(key, "Trusted"))
> > +            else if (pa_streq(key, "Connected")) {
> > +                if (d->device_connected != !!value) {
> > +                    d->device_connected = !!value;
> > +                    pa_log_debug("Device %s %s.", d->path, d->device_connected ? "connected" : "disconnected");
> > +                    pa_hook_fire(&d->hooks[PA_BLUETOOTH_DEVICE_HOOK_CONNECTED_CHANGED], NULL);
> 
> I agree about need to have a hook to report changes regarding the
> connection state, but I think the approach needs some improvement.
> You're using the Device.Connected property, while
> module-bluetooth-discover is additionally checking if at least one
> audio profile is connected.
> 
> This can be problematic is you have some other Bluetooth profile
> connected. In this case, the problem described in the bug report would
> reproduce: when some audio profile gets reconnected,
> module-bluetooth-discover would load the module again, leading to two
> instances of module-bluetotoh-device. The reason is Device.Connected
> was never set to false, and thus module-bluetooth-device was never
> unloaded.
> 
> Therefore, I would suggest that the new hook is fired in filter_cb(),
> immediately before run_callback() is called. This could also scale
> better, if at some point we wanted to modify the card according to
> which profiles are currently connected (i.e. create card profile only
> if corresponding bluetooth profile is connected).
> 
> > +                }
> > +            } else if (pa_streq(key, "Trusted"))
> >                  d->trusted = !!value;
> >
> >  /*             pa_log_debug("Value %s", pa_yes_no(value)); */
> > diff --git a/src/modules/bluetooth/bluetooth-util.h b/src/modules/bluetooth/bluetooth-util.h
> > index 8a3f2ad..30dd2e8 100644
> > --- a/src/modules/bluetooth/bluetooth-util.h
> > +++ b/src/modules/bluetooth/bluetooth-util.h
> > @@ -94,6 +94,7 @@ typedef enum pa_bt_audio_state {
> >  /* Hook data: pa_bluetooth_device pointer. */
> >  typedef enum pa_bluetooth_device_hook {
> >      PA_BLUETOOTH_DEVICE_HOOK_REMOVED, /* Call data: NULL. */
> > +    PA_BLUETOOTH_DEVICE_HOOK_CONNECTED_CHANGED, /* Call data: NULL. */
> >      PA_BLUETOOTH_DEVICE_HOOK_UUID_ADDED, /* Call data: const char *uuid. */
> >      PA_BLUETOOTH_DEVICE_HOOK_MAX
> >  } pa_bluetooth_device_hook_t;
> > diff --git a/src/modules/bluetooth/module-bluetooth-device.c b/src/modules/bluetooth/module-bluetooth-device.c
> > index 8a9d39f..f7a45b0 100644
> > --- a/src/modules/bluetooth/module-bluetooth-device.c
> > +++ b/src/modules/bluetooth/module-bluetooth-device.c
> > @@ -146,6 +146,7 @@ struct userdata {
> >      char *accesstype;
> >      pa_hook_slot *transport_removed_slot;
> >      pa_hook_slot *device_removed_slot;
> > +    pa_hook_slot *device_connected_changed_slot;
> >
> >      pa_bluetooth_discovery *discovery;
> >      pa_bool_t auto_connect;
> > @@ -2522,6 +2523,20 @@ static pa_hook_result_t device_removed_cb(pa_bluetooth_device *d, void *call_dat
> >      return PA_HOOK_OK;
> >  }
> >
> > +/* Run from main thread */
> > +static pa_hook_result_t device_connected_changed_cb(pa_bluetooth_device *d, void *call_data, struct userdata *u) {
> > +    pa_assert(d);
> > +    pa_assert(u);
> > +
> > +    if (d->device_connected)
> > +        return PA_HOOK_OK;
> 
> If you agree with the idea described above, the condition here should
> match the one in module-bluetooth-discover:
> 
> if ((d->audio_state >= PA_BT_AUDIO_STATE_CONNECTED ||
>      d->audio_source_state >= PA_BT_AUDIO_STATE_CONNECTED ||
>      d->hfgw_state >= PA_BT_AUDIO_STATE_CONNECTED))
>     return PA_HOOK_OK;
> 
> > +
> > +    pa_log_debug("Unloading module, because device %s became disconnected.", d->path);
> > +    pa_module_unload(u->core, u->module, TRUE);
> > +
> > +    return PA_HOOK_OK;
> > +}
> > +
> >  int pa__init(pa_module* m) {
> >      pa_modargs *ma;
> >      uint32_t channels;
> > @@ -2594,6 +2609,8 @@ int pa__init(pa_module* m) {
> >
> >      u->device_removed_slot = pa_hook_connect(&device->hooks[PA_BLUETOOTH_DEVICE_HOOK_REMOVED], PA_HOOK_NORMAL,
> >                                               (pa_hook_cb_t) device_removed_cb, u);
> > +    u->device_connected_changed_slot = pa_hook_connect(&device->hooks[PA_BLUETOOTH_DEVICE_HOOK_CONNECTED_CHANGED], PA_HOOK_NORMAL,
> > +                                                       (pa_hook_cb_t) device_connected_changed_cb, u);
> >
> >      u->device = device;
> >
> > @@ -2684,10 +2701,11 @@ void pa__done(pa_module *m) {
> >
> >      stop_thread(u);
> >
> > -    if (u->device_removed_slot) {
> > +    if (u->device_connected_changed_slot)
> > +        pa_hook_slot_free(u->device_connected_changed_slot);
> > +
> > +    if (u->device_removed_slot)
> >          pa_hook_slot_free(u->device_removed_slot);
> > -        u->device_removed_slot = NULL;
> > -    }
> >
> >      if (USE_SCO_OVER_PCM(u))
> >          restore_sco_volume_callbacks(u);
> > --
> 
> Additionally, for a later patch, we should probably modify
> module-bluetooth-discover such that we don't have similar problems in
> the future. Basically, we could unregister the device module from the
> hashmap only when the module has been unloaded, without relying on
> some policy that needs to match 100% the unloading policy in
> module-bluetooth-device. Otherwise the hashmap gets out of sync and
> thus we can end up having duplicated instances of
> module-bluetooth-device.

Thanks for feedback. So, the module should be unloaded when no audio
profiles are connected. Do you see any reason why the checks have to be
as complex as they are now in module-bluetooth-discovery: check for
"dead", device connected, audio connected and each profile connected? Is
the multitude of checks there only to paper over hypothetical bugs in
bluez? I mean, it looks like it should be enough to monitor the state of
the org.bluez.Audio interface. If the device is disconnected, surely the
Audio state becomes disconnected too? And if none of the individual
audio profiles are connected, shouldn't the general Audio state be
disconnected too in that case?

-- 
Tanu