[pulseaudio-discuss] [PATCH] bluetooth: Unload the device module when the device becomes disconnected.
Mikel Astiz
mikel.astiz.oss at gmail.com
Mon Nov 19 02:52:06 PST 2012
Hi Tanu,
On Mon, Nov 19, 2012 at 11:38 AM, Tanu Kaskinen <tanuk at iki.fi> wrote:
> 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.
org.bluez.Audio abstracts the headset role only, that's A2DP source
and HFP/HSP gateway need to be checked additionally.
> 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?
On the other hand, the device's state reflects any Bluetooth profile,
since it exposes the state of the low level ACL link.
This means that if Device.State==disconnected, then every other state
should be disconnected (including Audio.State). So yes, anything else
would be a bug in BlueZ.
Note however that the implication is one way only: even if all audio
profiles get disconnected, the device could still be connected (i.e.
file being transferred). That was the issue with your patch that I was
trying to point out.
Cheers,
Mikel
More information about the pulseaudio-discuss
mailing list