mbox series

[Bluez,v2,0/3] adapter: Reconnect audio when resuming from suspend

Message ID 20200818232822.1645054-1-abhishekpandit@chromium.org (mailing list archive)
Headers show
Series adapter: Reconnect audio when resuming from suspend | expand

Message

Abhishek Pandit-Subedi Aug. 18, 2020, 11:28 p.m. UTC
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

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).

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.

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(-)

Comments

Abhishek Pandit-Subedi Aug. 26, 2020, 5:41 p.m. UTC | #1
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
>
> 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).
>
> 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.
>
> 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 Aug. 27, 2020, 6:20 a.m. UTC | #2
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?

> > 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.

> > 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.

> > 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
> >
Abhishek Pandit-Subedi Aug. 27, 2020, 9:13 p.m. UTC | #3
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.

>
> > > 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
Abhishek Pandit-Subedi Aug. 27, 2020, 9:18 p.m. UTC | #4
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).

Correction: As mentioned in my response to patch 3 in this series:
https://lore.kernel.org/linux-bluetooth/CANFp7mV03KvqpOH=LBU=0tBDhgK5K2YJ6rxYvkDKmyer4n_gLw@mail.gmail.com/

>
> >
> > > > 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.
>
> >
> > > > 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 Aug. 28, 2020, 5:22 p.m. UTC | #5
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.

> >
> > > > 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
Abhishek Pandit-Subedi Aug. 28, 2020, 5:38 p.m. UTC | #6
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