[pulseaudio-discuss] [PATCH v5 5/7] bluetooth: Auto recover if profile is 'off'

[Tanu Kaskinen]

On Fri, 2017-05-05 at 16:41 +0300, Luiz Augusto von Dentz wrote:
> Hi Tanu,
> 
> On Thu, May 4, 2017 at 3:29 PM, Tanu Kaskinen <tanuk at iki.fi> wrote:
> > On Thu, 2017-05-04 at 12:58 +0300, Luiz Augusto von Dentz wrote:
> > > From: Luiz Augusto von Dentz <luiz.von.dentz at intel.com>
> > > 
> > > This means something went wrong, which in case of ofono backend it is
> > > probably due to the profile not connecting immediately, but it can be
> > > safely restored in that case the transport is playing which means the
> > > profile has recovered connectivity.
> > > ---
> > >  src/modules/bluetooth/module-bluez5-device.c | 8 +++++++-
> > >  1 file changed, 7 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/src/modules/bluetooth/module-bluez5-device.c b/src/modules/bluetooth/module-bluez5-device.c
> > > index a96da17..2f0ec97 100644
> > > --- a/src/modules/bluetooth/module-bluez5-device.c
> > > +++ b/src/modules/bluetooth/module-bluez5-device.c
> > > @@ -2060,8 +2060,14 @@ static pa_hook_result_t transport_state_changed_cb(pa_bluetooth_discovery *y, pa
> > >      if (t == u->transport && t->state <= PA_BLUETOOTH_TRANSPORT_STATE_DISCONNECTED)
> > >          pa_assert_se(pa_card_set_profile(u->card, pa_hashmap_get(u->card->profiles, "off"), false) >= 0);
> > > 
> > > -    if (t->device == u->device)
> > > +    if (t->device == u->device) {
> > > +        /* Auto recover from errors causing the profile to be set to off */
> > > +        if (u->profile == PA_BLUETOOTH_PROFILE_OFF && t->state == PA_BLUETOOTH_TRANSPORT_STATE_PLAYING) {
> > > +            pa_log_debug("Switching to profile %s", pa_bluetooth_profile_to_string(t->profile));
> > > +            pa_assert_se(pa_card_set_profile(u->card, pa_hashmap_get(u->card->profiles, pa_bluetooth_profile_to_string(t->profile)), false) >= 0);
> > 
> > Regarding the assertion, how do you know that the card profile switch
> > will always succeed? Is there no IO involved? If you somehow know that
> > it will always succeed, there should be a comment explaining how you
> > know that.
> 
> The playing state should guarantee the fd is available, so there
> shouldn't be any problem in this regard.

At least start_thread() can fail, and that will make the profile change
fail.

> > I know there are already assertions when switching to the off profile,
> > but that's a special profile - activating it should never fail.
> > 
> > This change seems dubious in another way too. So far we've kept all
> > automatic profile switch code out of module-bluez5-device (apart from
> > switches to "off" when things fail). Automatic switching is handled by
> > module-bluetooth-policy instead. If I understood correctly, this is a
> > fix for a situation where we already tried to switch to a profile, but
> > it failed first. The code is lacking any checks that would do the
> > profile change only if there really was a prior profile switch attempt.
> 
> It is exactly to counter the profiles switching 'off' which is done in
> the device itself, we could of course move this to policy as well
> though. Note that in most cases it is better to switch to a working
> profile than 'off' since that for sure won't render anything.

If we try to switch to some profile and it fails and as a result the
profile gets set to "off", then I think it's fine to fix that problem
in module-bluez5-device, but as I said, there's nothing in this patch
that would ensure that this is done *only* when this situation occurs.
If there was no problem earlier, then module-bluez5-device shouldn't
automatically change its profile, that kind of stuff belongs to module-
bluetooth-policy.

Georg told me that this patch isn't needed anyway, after the "bluez5-
device: Correctly handle suspend/resume cyle for audio gateway role of
ofono backend" patch. If that's true, then can we just revert this?

-- 
Tanu

https://www.patreon.com/tanuk