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

[Mikel Astiz]

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.

Cheers,
Mikel