Re: [Bluez PATCH v2 0/3] adapter: Reconnect audio when resuming from suspend - Abhishek Pandit-Subedi

From: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
To: Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Cc: Marcel Holtmann <marcel@holtmann.org>,
	ChromeOS Bluetooth Upstreaming 
	<chromeos-bluetooth-upstreaming@chromium.org>,
	Bluez mailing list <linux-bluetooth@vger.kernel.org>
Subject: Re: [Bluez PATCH v2 0/3] adapter: Reconnect audio when resuming from suspend
Date: Fri, 28 Aug 2020 10:38:51 -0700	[thread overview]
Message-ID: <CANFp7mVWAWkDgYJT61xGKAgYzycbjyaePngFho9dcfqkxRcuPg@mail.gmail.com> (raw)
In-Reply-To: <CABBYNZ+YMfvcVnB7afXyFa_bz2oNaivgsmyhS=yOGrKuh4JG9A@mail.gmail.com>

Hey Luiz,

On Fri, Aug 28, 2020 at 10:22 AM Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
>
> Hi Abhishek,
>
> On Thu, Aug 27, 2020 at 2:13 PM Abhishek Pandit-Subedi
> <abhishekpandit@chromium.org> wrote:
> >
> > Hi Luiz,
> >
> > On Wed, Aug 26, 2020 at 11:21 PM Luiz Augusto von Dentz
> > <luiz.dentz@gmail.com> wrote:
> > >
> > > Hi Abhishek,
> > >
> > > On Wed, Aug 26, 2020 at 10:41 AM Abhishek Pandit-Subedi
> > > <abhishekpandit@chromium.org> wrote:
> > > >
> > > > Hi Luiz,
> > > >
> > > > Could you PTAL at this v2 patch series. I refactored the audio
> > > > reconnect to use the policy plugin instead of doing the reconnect as
> > > > part of the adapter logic, as requested.
> > > >
> > > > Thanks
> > > > Abhishek
> > > >
> > > > On Tue, Aug 18, 2020 at 4:28 PM Abhishek Pandit-Subedi
> > > > <abhishekpandit@chromium.org> wrote:
> > > > >
> > > > >
> > > > > Hi Luiz and Marcel,
> > > > >
> > > > > This is a quality of life improvement for the behavior of audio devices
> > > > > during system suspend. This depends on a kernel change that emits
> > > > > suspend/resume events:
> > > > >
> > > > > https://patchwork.kernel.org/project/bluetooth/list/?series=325771
> > >
> > > So we could not just use the disconnect reason like I suggested?
> >
> > I am using the disconnect reason to mark the device for reconnection
> > and only queueing it for reconnect on resume. As mentioned in the
> > patch, this is to account for resumes that are not user driven and
> > will suspend almost immediately again (i.e. periodic wake up to check
> > battery level and see if we need to shut down).
> >
> > >
> > > > > Right now, audio devices will be disconnected as part of suspend but
> > > > > won't be reconnected when the system resumes without user interaction.
> > > > > This is annoying to some users as it causes an interruption to their
> > > > > normal work flow.
> > > > >
> > > > > This change reconnects audio devices that were disconnected for suspend
> > > > > using the following logic:
> > > > >
> > > > >  * Register a callback for controller resume in the policy plugin.
> > > > >  * In the device disconnect callback, mark any devices with the A2DP
> > > > >    service uuid for reconnect on resume after a delay.
> > > > >  * In the controller resume callback, queue any policy items that are
> > > > >    marked to reconnect on resume for connection with the
> > > > >    ReconnectAudioDelay value (default = 5s for Wi-Fi coexistence).
> > >
> > > Something like ResumeDelay would probably be better.
> >
> > Sure, I will rename this.
> >
> > >
> > > > > A reconnect is only attempted once after the controller resumes and the
> > > > > delay between resume and reconnect is configurable via the
> > > > > ReconnectAudioDelay key in the Policy settings. The 5s delay was chosen
> > > > > arbitrarily and I think anywhere up to 10s is probably ok. A longer
> > > > > delay is better to account for spurious wakeups and Wi-Fi reconnection
> > > > > time (avoiding any co-ex issues) at the downside of reconnection speed.
> > >
> > > I would keep the same logic as out of range so the platforms can
> > > customize the number of attempts.
> >
> > So the first reconnect attempt after resume should be separately
> > configurable (i.e. 5s) but subsequent reconnect attempts would use the
> > reconnect-intervals values? That sounds good to me.
>
> Right, though 5 seconds seems awfully long compared to the normal
> first attempt, so perhaps we could default it to just 1-2 seconds.
>

I will change the default to 2s and make sure we test this on some
older chipsets to check for interference with Wi-Fi.

> > >
> > > > > Here are the tests I have done with this:
> > > > > - Single suspend and verified the headphones reconnect
> > > > > - Suspend stress test for 25 iterations and verify both Wi-Fi and
> > > > >   Bluetooth audio reconnect on resume. (Ran with wake minimum time of
> > > > >   10s)
> > > > > - Suspend test with wake time < 5s to verify that BT reconnect isn't
> > > > >   attempted. Ran 5 iterations with low wake time and then let it stay
> > > > >   awake to confirm reconnect finally completed after 5s+ wake time.
> > > > > - Suspend test with wake time between 3s - 6s. Ran with 5 iterations and
> > > > >   verified it wasn't connected at the end. A connection attempt was
> > > > >   made but not completed due to suspend. A reconnect attempt was not
> > > > >   made afterwards, which is by design.
> > > > >
> > > > >   Luiz@ Marcel@: Does this sound ok (give up after an attempt)?
> > > > >
> > > > > I've tested this on a Pixelbook Go (AC-9260 controller) and HP
> > > > > Chromebook 14a (RTL8822CE controller) with GID6B headset.
> > > > >
> > > > > I've also tested this with the Pixel Buds 2. These earbuds actually
> > > > > reconnect automatically to the Chromebook (even without this policy
> > > > > change) and I verified that the new changes don't break the reconnection
> > > > > mechanism.
> > > > >
> > > > > Thanks
> > > > > Abhishek
> > > > >
> > > > >
> > > > > Changes in v2:
> > > > > - Refactored to use policy instead of connecting directly in adapter
> > > > >
> > > > > Abhishek Pandit-Subedi (3):
> > > > >   mgmt: Add controller suspend and resume events
> > > > >   monitor: Add btmon support for Suspend and Resume events
> > > > >   policy: Reconnect audio on controller resume
> > > > >
> > > > >  lib/mgmt.h       | 14 +++++++++++
> > > > >  monitor/packet.c | 55 +++++++++++++++++++++++++++++++++++++++++
> > > > >  plugins/policy.c | 64 +++++++++++++++++++++++++++++++++++++++++++++---
> > > > >  src/adapter.c    | 45 ++++++++++++++++++++++++++++++++++
> > > > >  src/adapter.h    |  6 +++++
> > > > >  src/main.c       |  1 +
> > > > >  src/main.conf    |  9 +++++++
> > > > >  7 files changed, 190 insertions(+), 4 deletions(-)
> > > > >
> > > > > --
> > > > > 2.28.0.297.g1956fa8f8d-goog
> > > > >
> > >
> > >
> > >
> > > --
> > > Luiz Augusto von Dentz
>
>
>
> --
> Luiz Augusto von Dentz

Thanks
Abhishek