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

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

[Abhishek Pandit-Subedi]

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

[Bluez PATCH v2 1/3] mgmt: Add controller suspend and resume events

[Abhishek Pandit-Subedi]

Add the controller suspend and resume events.

Reviewed-by: Sonny Sasaka <sonnysasaka@chromium.org>
Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
---

Changes in v2: None

 lib/mgmt.h | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/lib/mgmt.h b/lib/mgmt.h
index a800bcab4..46d894ae9 100644
--- a/lib/mgmt.h
+++ b/lib/mgmt.h
@@ -772,6 +772,7 @@ struct mgmt_ev_device_connected {
 #define MGMT_DEV_DISCONN_TIMEOUT	0x01
 #define MGMT_DEV_DISCONN_LOCAL_HOST	0x02
 #define MGMT_DEV_DISCONN_REMOTE		0x03
+#define MGMT_DEV_DISCONN_LOCAL_HOST_SUSPEND	0x05
 
 #define MGMT_EV_DEVICE_DISCONNECTED	0x000C
 struct mgmt_ev_device_disconnected {
@@ -959,6 +960,17 @@ struct mgmt_ev_adv_monitor_removed {
 	uint16_t monitor_handle;
 }  __packed;
 
+#define MGMT_EV_CONTROLLER_SUSPEND		0x002d
+struct mgmt_ev_controller_suspend {
+	uint8_t suspend_state;
+} __packed;
+
+#define MGMT_EV_CONTROLLER_RESUME		0x002e
+struct mgmt_ev_controller_resume {
+	struct mgmt_addr_info addr;
+	uint8_t wake_reason;
+} __packed;
+
 static const char *mgmt_op[] = {
 	"<0x0000>",
 	"Read Version",
@@ -1088,6 +1100,8 @@ static const char *mgmt_ev[] = {
 	"Device Flags Changed",
 	"Advertisement Monitor Added",			/* 0x002b */
 	"Advertisement Monitor Removed",
+	"Controller Suspend",
+	"Controller Resume",
 };
 
 static const char *mgmt_status[] = {
-- 
2.28.0.297.g1956fa8f8d-goog

[Bluez PATCH v2 2/3] monitor: Add btmon support for Suspend and Resume events

[Abhishek Pandit-Subedi]

Add support to pretty print Suspend and Resume mgmt events in btmon.

Example:

@ MGMT Event: Controller Suspended (0x002d) plen 1
        Suspend state: Page scanning and/or passive scanning (2)

@ MGMT Event: Controller Resumed (0x002e) plen 8
        Wake reason: Remote wake due to peer device connection (2)
        LE Address: CD:F3:CD:13:C5:9A (OUI CD-F3-CD)

Reviewed-by: Sonny Sasaka <sonnysasaka@chromium.org>
Reviewed-by: Miao-chen Chou <mcchou@chromium.org>
Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
---

Changes in v2: None

 monitor/packet.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 55 insertions(+)

diff --git a/monitor/packet.c b/monitor/packet.c
index 431a39b66..451630e04 100644
--- a/monitor/packet.c
+++ b/monitor/packet.c
@@ -13555,6 +13555,9 @@ static void mgmt_device_disconnected_evt(const void *data, uint16_t size)
 	case 0x04:
 		str = "Connection terminated due to authentication failure";
 		break;
+	case 0x05:
+		str = "Connection terminated by local host for suspend";
+		break;
 	default:
 		str = "Reserved";
 		break;
@@ -13782,6 +13785,54 @@ static void mgmt_device_flags_changed_evt(const void *data, uint16_t size)
 	mgmt_print_added_device_flags("Current Flags", current_flags);
 }
 
+static void mgmt_controller_suspend_evt(const void *data, uint16_t size)
+{
+	uint8_t state = get_u8(data);
+	char *str;
+
+	switch (state) {
+	case 0x0:
+		str = "Controller running (failed to suspend)";
+		break;
+	case 0x1:
+		str = "Disconnected and not scanning";
+		break;
+	case 0x2:
+		str = "Page scanning and/or passive scanning";
+		break;
+	default:
+		str = "Unknown suspend state";
+		break;
+	}
+
+	print_field("Suspend state: %s (%d)", str, state);
+}
+
+static void mgmt_controller_resume_evt(const void *data, uint16_t size)
+{
+	uint8_t addr_type = get_u8(data + 6);
+	uint8_t wake_reason = get_u8(data + 7);
+	char *str;
+
+	switch (wake_reason) {
+	case 0x0:
+		str = "Resume from non-Bluetooth wake source";
+		break;
+	case 0x1:
+		str = "Wake due to unexpected event";
+		break;
+	case 0x2:
+		str = "Remote wake due to peer device connection";
+		break;
+	default:
+		str = "Unknown wake reason";
+		break;
+	}
+
+	print_field("Wake reason: %s (%d)", str, wake_reason);
+	mgmt_print_address(data, addr_type);
+}
+
 static const struct mgmt_data mgmt_event_table[] = {
 	{ 0x0001, "Command Complete",
 			mgmt_command_complete_evt, 3, false },
@@ -13863,6 +13914,10 @@ static const struct mgmt_data mgmt_event_table[] = {
 			mgmt_exp_feature_changed_evt, 20, true },
 	{ 0x002a, "Device Flags Changed",
 			mgmt_device_flags_changed_evt, 15, true },
+	{ 0x002d, "Controller Suspended",
+			mgmt_controller_suspend_evt, 1, true },
+	{ 0x002e, "Controller Resumed",
+			mgmt_controller_resume_evt, 8, true },
 	{ }
 };
 
-- 
2.28.0.297.g1956fa8f8d-goog

[Bluez PATCH v2 3/3] policy: Reconnect audio on controller resume

[Abhishek Pandit-Subedi]

During system suspend, all peer devices are disconnected. On resume, HID
devices will reconnect but audio devices stay disconnected. As a quality
of life improvement, mark audio devices that were disconnected due to
suspend and attempt to reconnect them when the controller resumes (after
a delay for better co-existence with Wi-Fi).

Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
---

Changes in v2:
- Refactored to use policy instead of connecting directly in adapter

 plugins/policy.c | 64 +++++++++++++++++++++++++++++++++++++++++++++---
 src/adapter.c    | 45 ++++++++++++++++++++++++++++++++++
 src/adapter.h    |  6 +++++
 src/main.c       |  1 +
 src/main.conf    |  9 +++++++
 5 files changed, 121 insertions(+), 4 deletions(-)

diff --git a/plugins/policy.c b/plugins/policy.c
index de51e58b9..b07a997b9 100644
--- a/plugins/policy.c
+++ b/plugins/policy.c
@@ -75,6 +75,9 @@ static const int default_intervals[] = { 1, 2, 4, 8, 16, 32, 64 };
 static int *reconnect_intervals = NULL;
 static size_t reconnect_intervals_len = 0;
 
+static const int default_reconnect_delay = 5;
+static int resume_reconnect_delay = 5;
+
 static GSList *reconnects = NULL;
 
 static unsigned int service_id = 0;
@@ -93,6 +96,8 @@ struct policy_data {
 	uint8_t ct_retries;
 	guint tg_timer;
 	uint8_t tg_retries;
+
+	bool reconnect_on_resume;
 };
 
 static struct reconnect_data *reconnect_find(struct btd_device *dev)
@@ -214,6 +219,7 @@ static gboolean policy_connect_sink(gpointer user_data)
 
 	data->sink_timer = 0;
 	data->sink_retries++;
+	data->reconnect_on_resume = false;
 
 	service = btd_device_get_service(data->dev, A2DP_SINK_UUID);
 	if (service != NULL)
@@ -222,16 +228,20 @@ static gboolean policy_connect_sink(gpointer user_data)
 	return FALSE;
 }
 
-static void policy_set_sink_timer(struct policy_data *data)
+static void policy_set_sink_timer_internal(struct policy_data *data, int timeout)
 {
 	if (data->sink_timer > 0)
 		g_source_remove(data->sink_timer);
 
-	data->sink_timer = g_timeout_add_seconds(SINK_RETRY_TIMEOUT,
-							policy_connect_sink,
+	data->sink_timer = g_timeout_add_seconds(timeout, policy_connect_sink,
 							data);
 }
 
+static void policy_set_sink_timer(struct policy_data *data)
+{
+	policy_set_sink_timer_internal(data, SINK_RETRY_TIMEOUT);
+}
+
 static void sink_cb(struct btd_service *service, btd_service_state_t old_state,
 						btd_service_state_t new_state)
 {
@@ -743,10 +753,30 @@ static void reconnect_set_timer(struct reconnect_data *reconnect)
 static void disconnect_cb(struct btd_device *dev, uint8_t reason)
 {
 	struct reconnect_data *reconnect;
+	struct btd_service *service;
+	struct policy_data *data;
+	bool do_reconnect = false;
 
 	DBG("reason %u", reason);
 
-	if (reason != MGMT_DEV_DISCONN_TIMEOUT)
+	switch(reason) {
+	case MGMT_DEV_DISCONN_LOCAL_HOST:
+	case MGMT_DEV_DISCONN_REMOTE:
+		do_reconnect = true;
+		break;
+	/* Disconnect due to suspend will queue reconnect on resume for a2dp */
+	case MGMT_DEV_DISCONN_LOCAL_HOST_SUSPEND:
+		service = btd_device_get_service(dev, A2DP_SINK_UUID);
+		if (service && (data = policy_get_data(dev))) {
+			data->reconnect_on_resume = true;
+		}
+		break;
+	/* All other cases do not result in reconnect */
+	default:
+		break;
+	}
+
+	if (!do_reconnect)
 		return;
 
 	reconnect = reconnect_find(dev);
@@ -761,6 +791,23 @@ static void disconnect_cb(struct btd_device *dev, uint8_t reason)
 	reconnect_set_timer(reconnect);
 }
 
+static void controller_resume_cb(uint8_t wake_reason, const bdaddr_t *addr,
+					const uint8_t addr_type)
+{
+	struct btd_device *dev;
+	GSList *l;
+
+	/* Check if any devices needed to be reconnected on resume */
+	for (l = devices; l; l = g_slist_next(l)) {
+		struct policy_data *data = l->data;
+
+		if (data->reconnect_on_resume) {
+			policy_set_sink_timer_internal(data,
+						       resume_reconnect_delay);
+		}
+	}
+}
+
 static void conn_fail_cb(struct btd_device *dev, uint8_t status)
 {
 	struct reconnect_data *reconnect;
@@ -854,9 +901,17 @@ static int policy_init(void)
 	auto_enable = g_key_file_get_boolean(conf, "Policy", "AutoEnable",
 									NULL);
 
+	resume_reconnect_delay = g_key_file_get_integer(
+			conf, "Policy", "ReconnectAudioDelay", &gerr);
+
+	if (gerr) {
+		g_clear_error(&gerr);
+		resume_reconnect_delay = default_reconnect_delay;
+	}
 done:
 	if (reconnect_uuids && reconnect_uuids[0] && reconnect_attempts) {
 		btd_add_disconnect_cb(disconnect_cb);
+		btd_add_controller_resume_cb(controller_resume_cb);
 		btd_add_conn_fail_cb(conn_fail_cb);
 	}
 
@@ -869,6 +924,7 @@ done:
 static void policy_exit(void)
 {
 	btd_remove_disconnect_cb(disconnect_cb);
+	btd_remove_controller_resume_cb(controller_resume_cb);
 	btd_remove_conn_fail_cb(conn_fail_cb);
 
 	if (reconnect_uuids)
diff --git a/src/adapter.c b/src/adapter.c
index 5e896a9f0..7526feb9e 100644
--- a/src/adapter.c
+++ b/src/adapter.c
@@ -139,6 +139,7 @@ static GSList *adapter_drivers = NULL;
 
 static GSList *disconnect_list = NULL;
 static GSList *conn_fail_list = NULL;
+static GSList *controller_resume_list = NULL;
 
 struct link_key_info {
 	bdaddr_t bdaddr;
@@ -8766,6 +8767,45 @@ static void connected_callback(uint16_t index, uint16_t length,
 	eir_data_free(&eir_data);
 }
 
+static void controller_resume_notify(const uint8_t wake_reason,
+					const bdaddr_t *addr,
+					const uint8_t addr_type)
+{
+	GSList *l;
+
+	for (l = controller_resume_list; l; l = g_slist_next(l)) {
+		btd_controller_resume_cb resume_cb = l->data;
+		resume_cb(wake_reason, addr, addr_type);
+	}
+}
+
+static void controller_resume_callback(uint16_t index, uint16_t length,
+				       const void *param, void *user_data)
+{
+	const struct mgmt_ev_controller_resume *ev = param;
+	struct btd_adapter *adapter = user_data;
+
+	if (length < sizeof(*ev)) {
+		btd_error(adapter->dev_id, "Too small device resume event");
+		return;
+	}
+
+	DBG("Controller resume with wake event 0x%x", ev->wake_reason);
+
+	controller_resume_notify(ev->wake_reason, &ev->addr.bdaddr,
+				 ev->addr.type);
+}
+
+void btd_add_controller_resume_cb(btd_controller_resume_cb func)
+{
+	controller_resume_list = g_slist_append(controller_resume_list, func);
+}
+
+void btd_remove_controller_resume_cb(btd_controller_resume_cb func)
+{
+	controller_resume_list = g_slist_remove(controller_resume_list, func);
+}
+
 static void device_blocked_callback(uint16_t index, uint16_t length,
 					const void *param, void *user_data)
 {
@@ -9389,6 +9429,11 @@ static void read_info_complete(uint8_t status, uint16_t length,
 						user_passkey_notify_callback,
 						adapter, NULL);
 
+	mgmt_register(adapter->mgmt, MGMT_EV_CONTROLLER_RESUME,
+						adapter->dev_id,
+						controller_resume_callback,
+						adapter, NULL);
+
 	set_dev_class(adapter);
 
 	set_name(adapter, btd_adapter_get_name(adapter));
diff --git a/src/adapter.h b/src/adapter.h
index f8ac20261..5527e4205 100644
--- a/src/adapter.h
+++ b/src/adapter.h
@@ -67,6 +67,12 @@ typedef void (*btd_conn_fail_cb) (struct btd_device *device, uint8_t status);
 void btd_add_conn_fail_cb(btd_conn_fail_cb func);
 void btd_remove_conn_fail_cb(btd_conn_fail_cb func);
 
+typedef void (*btd_controller_resume_cb)(const uint8_t wake_reason,
+					const bdaddr_t *addr,
+					const uint8_t addr_type);
+void btd_add_controller_resume_cb(btd_controller_resume_cb func);
+void btd_remove_controller_resume_cb(btd_controller_resume_cb func);
+
 struct btd_adapter *adapter_find(const bdaddr_t *sba);
 struct btd_adapter *adapter_find_by_id(int id);
 void adapter_foreach(adapter_cb func, gpointer user_data);
diff --git a/src/main.c b/src/main.c
index 2c083de67..a1c3e7e9f 100644
--- a/src/main.c
+++ b/src/main.c
@@ -131,6 +131,7 @@ static const char *policy_options[] = {
 	"ReconnectAttempts",
 	"ReconnectIntervals",
 	"AutoEnable",
+	"ReconnectAudioDelay",
 	NULL
 };
 
diff --git a/src/main.conf b/src/main.conf
index f41203b96..8d74a2aec 100644
--- a/src/main.conf
+++ b/src/main.conf
@@ -198,3 +198,12 @@
 # This includes adapters present on start as well as adapters that are plugged
 # in later on. Defaults to 'false'.
 #AutoEnable=false
+
+# Audio devices that were disconnected due to suspend will be reconnected on
+# resume. ReconnectAudioDelay determines the delay between when the controller
+# resumes from suspend and a connection attempt is made. A longer delay can be
+# better for co-existence with Wi-Fi.
+# The value is in seconds.
+# Default: 5
+#ReconnectAudioDelay = 5
+
-- 
2.28.0.297.g1956fa8f8d-goog

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

[Abhishek Pandit-Subedi]

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
>

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

[Luiz Augusto von Dentz]

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



-- 
Luiz Augusto von Dentz

Re: [Bluez PATCH v2 3/3] policy: Reconnect audio on controller resume

[Luiz Augusto von Dentz]

Hi Abhishek,

On Tue, Aug 18, 2020 at 4:28 PM Abhishek Pandit-Subedi
<abhishekpandit@chromium.org> wrote:
>
> During system suspend, all peer devices are disconnected. On resume, HID
> devices will reconnect but audio devices stay disconnected. As a quality
> of life improvement, mark audio devices that were disconnected due to
> suspend and attempt to reconnect them when the controller resumes (after
> a delay for better co-existence with Wi-Fi).
>
> Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
> ---
>
> Changes in v2:
> - Refactored to use policy instead of connecting directly in adapter
>
>  plugins/policy.c | 64 +++++++++++++++++++++++++++++++++++++++++++++---
>  src/adapter.c    | 45 ++++++++++++++++++++++++++++++++++
>  src/adapter.h    |  6 +++++
>  src/main.c       |  1 +
>  src/main.conf    |  9 +++++++
>  5 files changed, 121 insertions(+), 4 deletions(-)
>
> diff --git a/plugins/policy.c b/plugins/policy.c
> index de51e58b9..b07a997b9 100644
> --- a/plugins/policy.c
> +++ b/plugins/policy.c
> @@ -75,6 +75,9 @@ static const int default_intervals[] = { 1, 2, 4, 8, 16, 32, 64 };
>  static int *reconnect_intervals = NULL;
>  static size_t reconnect_intervals_len = 0;
>
> +static const int default_reconnect_delay = 5;
> +static int resume_reconnect_delay = 5;
> +
>  static GSList *reconnects = NULL;
>
>  static unsigned int service_id = 0;
> @@ -93,6 +96,8 @@ struct policy_data {
>         uint8_t ct_retries;
>         guint tg_timer;
>         uint8_t tg_retries;
> +
> +       bool reconnect_on_resume;
>  };
>
>  static struct reconnect_data *reconnect_find(struct btd_device *dev)
> @@ -214,6 +219,7 @@ static gboolean policy_connect_sink(gpointer user_data)
>
>         data->sink_timer = 0;
>         data->sink_retries++;
> +       data->reconnect_on_resume = false;
>
>         service = btd_device_get_service(data->dev, A2DP_SINK_UUID);
>         if (service != NULL)
> @@ -222,16 +228,20 @@ static gboolean policy_connect_sink(gpointer user_data)
>         return FALSE;
>  }
>
> -static void policy_set_sink_timer(struct policy_data *data)
> +static void policy_set_sink_timer_internal(struct policy_data *data, int timeout)
>  {
>         if (data->sink_timer > 0)
>                 g_source_remove(data->sink_timer);
>
> -       data->sink_timer = g_timeout_add_seconds(SINK_RETRY_TIMEOUT,
> -                                                       policy_connect_sink,
> +       data->sink_timer = g_timeout_add_seconds(timeout, policy_connect_sink,
>                                                         data);
>  }
>
> +static void policy_set_sink_timer(struct policy_data *data)
> +{
> +       policy_set_sink_timer_internal(data, SINK_RETRY_TIMEOUT);
> +}
> +
>  static void sink_cb(struct btd_service *service, btd_service_state_t old_state,
>                                                 btd_service_state_t new_state)
>  {
> @@ -743,10 +753,30 @@ static void reconnect_set_timer(struct reconnect_data *reconnect)
>  static void disconnect_cb(struct btd_device *dev, uint8_t reason)
>  {
>         struct reconnect_data *reconnect;
> +       struct btd_service *service;
> +       struct policy_data *data;
> +       bool do_reconnect = false;
>
>         DBG("reason %u", reason);
>
> -       if (reason != MGMT_DEV_DISCONN_TIMEOUT)
> +       switch(reason) {
> +       case MGMT_DEV_DISCONN_LOCAL_HOST:
> +       case MGMT_DEV_DISCONN_REMOTE:
> +               do_reconnect = true;
> +               break;
> +       /* Disconnect due to suspend will queue reconnect on resume for a2dp */
> +       case MGMT_DEV_DISCONN_LOCAL_HOST_SUSPEND:
> +               service = btd_device_get_service(dev, A2DP_SINK_UUID);
> +               if (service && (data = policy_get_data(dev))) {
> +                       data->reconnect_on_resume = true;
> +               }

Can't we just program the timer directly here? Or would that wakeup
the system, I would imagine all timers are disabled while suspended
which means when the system resumes so does the timers which would
then trigger the reconnection logic.

> +               break;
> +       /* All other cases do not result in reconnect */
> +       default:
> +               break;
> +       }
> +
> +       if (!do_reconnect)
>                 return;
>
>         reconnect = reconnect_find(dev);
> @@ -761,6 +791,23 @@ static void disconnect_cb(struct btd_device *dev, uint8_t reason)
>         reconnect_set_timer(reconnect);
>  }
>
> +static void controller_resume_cb(uint8_t wake_reason, const bdaddr_t *addr,
> +                                       const uint8_t addr_type)
> +{
> +       struct btd_device *dev;
> +       GSList *l;
> +
> +       /* Check if any devices needed to be reconnected on resume */
> +       for (l = devices; l; l = g_slist_next(l)) {
> +               struct policy_data *data = l->data;
> +
> +               if (data->reconnect_on_resume) {
> +                       policy_set_sink_timer_internal(data,
> +                                                      resume_reconnect_delay);
> +               }
> +       }
> +}
> +
>  static void conn_fail_cb(struct btd_device *dev, uint8_t status)
>  {
>         struct reconnect_data *reconnect;
> @@ -854,9 +901,17 @@ static int policy_init(void)
>         auto_enable = g_key_file_get_boolean(conf, "Policy", "AutoEnable",
>                                                                         NULL);
>
> +       resume_reconnect_delay = g_key_file_get_integer(
> +                       conf, "Policy", "ReconnectAudioDelay", &gerr);
> +
> +       if (gerr) {
> +               g_clear_error(&gerr);
> +               resume_reconnect_delay = default_reconnect_delay;
> +       }
>  done:
>         if (reconnect_uuids && reconnect_uuids[0] && reconnect_attempts) {
>                 btd_add_disconnect_cb(disconnect_cb);
> +               btd_add_controller_resume_cb(controller_resume_cb);
>                 btd_add_conn_fail_cb(conn_fail_cb);
>         }
>
> @@ -869,6 +924,7 @@ done:
>  static void policy_exit(void)
>  {
>         btd_remove_disconnect_cb(disconnect_cb);
> +       btd_remove_controller_resume_cb(controller_resume_cb);
>         btd_remove_conn_fail_cb(conn_fail_cb);
>
>         if (reconnect_uuids)
> diff --git a/src/adapter.c b/src/adapter.c
> index 5e896a9f0..7526feb9e 100644
> --- a/src/adapter.c
> +++ b/src/adapter.c
> @@ -139,6 +139,7 @@ static GSList *adapter_drivers = NULL;
>
>  static GSList *disconnect_list = NULL;
>  static GSList *conn_fail_list = NULL;
> +static GSList *controller_resume_list = NULL;
>
>  struct link_key_info {
>         bdaddr_t bdaddr;
> @@ -8766,6 +8767,45 @@ static void connected_callback(uint16_t index, uint16_t length,
>         eir_data_free(&eir_data);
>  }
>
> +static void controller_resume_notify(const uint8_t wake_reason,
> +                                       const bdaddr_t *addr,
> +                                       const uint8_t addr_type)
> +{
> +       GSList *l;
> +
> +       for (l = controller_resume_list; l; l = g_slist_next(l)) {
> +               btd_controller_resume_cb resume_cb = l->data;
> +               resume_cb(wake_reason, addr, addr_type);
> +       }
> +}
> +
> +static void controller_resume_callback(uint16_t index, uint16_t length,
> +                                      const void *param, void *user_data)
> +{
> +       const struct mgmt_ev_controller_resume *ev = param;
> +       struct btd_adapter *adapter = user_data;
> +
> +       if (length < sizeof(*ev)) {
> +               btd_error(adapter->dev_id, "Too small device resume event");
> +               return;
> +       }
> +
> +       DBG("Controller resume with wake event 0x%x", ev->wake_reason);
> +
> +       controller_resume_notify(ev->wake_reason, &ev->addr.bdaddr,
> +                                ev->addr.type);
> +}
> +
> +void btd_add_controller_resume_cb(btd_controller_resume_cb func)
> +{
> +       controller_resume_list = g_slist_append(controller_resume_list, func);
> +}
> +
> +void btd_remove_controller_resume_cb(btd_controller_resume_cb func)
> +{
> +       controller_resume_list = g_slist_remove(controller_resume_list, func);
> +}
> +
>  static void device_blocked_callback(uint16_t index, uint16_t length,
>                                         const void *param, void *user_data)
>  {
> @@ -9389,6 +9429,11 @@ static void read_info_complete(uint8_t status, uint16_t length,
>                                                 user_passkey_notify_callback,
>                                                 adapter, NULL);
>
> +       mgmt_register(adapter->mgmt, MGMT_EV_CONTROLLER_RESUME,
> +                                               adapter->dev_id,
> +                                               controller_resume_callback,
> +                                               adapter, NULL);
> +
>         set_dev_class(adapter);
>
>         set_name(adapter, btd_adapter_get_name(adapter));
> diff --git a/src/adapter.h b/src/adapter.h
> index f8ac20261..5527e4205 100644
> --- a/src/adapter.h
> +++ b/src/adapter.h
> @@ -67,6 +67,12 @@ typedef void (*btd_conn_fail_cb) (struct btd_device *device, uint8_t status);
>  void btd_add_conn_fail_cb(btd_conn_fail_cb func);
>  void btd_remove_conn_fail_cb(btd_conn_fail_cb func);
>
> +typedef void (*btd_controller_resume_cb)(const uint8_t wake_reason,
> +                                       const bdaddr_t *addr,
> +                                       const uint8_t addr_type);
> +void btd_add_controller_resume_cb(btd_controller_resume_cb func);
> +void btd_remove_controller_resume_cb(btd_controller_resume_cb func);

If we can program the timer just before we suspend this is not really
necessary, but if you still thing that would be a good idea to notify
Id put it in btd_adapter_driver with a callback for suspend and
resume, that way we don't have to maintain multiple lists for each of
the callbacks.

>  struct btd_adapter *adapter_find(const bdaddr_t *sba);
>  struct btd_adapter *adapter_find_by_id(int id);
>  void adapter_foreach(adapter_cb func, gpointer user_data);
> diff --git a/src/main.c b/src/main.c
> index 2c083de67..a1c3e7e9f 100644
> --- a/src/main.c
> +++ b/src/main.c
> @@ -131,6 +131,7 @@ static const char *policy_options[] = {
>         "ReconnectAttempts",
>         "ReconnectIntervals",
>         "AutoEnable",
> +       "ReconnectAudioDelay",
>         NULL
>  };
>
> diff --git a/src/main.conf b/src/main.conf
> index f41203b96..8d74a2aec 100644
> --- a/src/main.conf
> +++ b/src/main.conf
> @@ -198,3 +198,12 @@
>  # This includes adapters present on start as well as adapters that are plugged
>  # in later on. Defaults to 'false'.
>  #AutoEnable=false
> +
> +# Audio devices that were disconnected due to suspend will be reconnected on
> +# resume. ReconnectAudioDelay determines the delay between when the controller
> +# resumes from suspend and a connection attempt is made. A longer delay can be
> +# better for co-existence with Wi-Fi.
> +# The value is in seconds.
> +# Default: 5
> +#ReconnectAudioDelay = 5
> +
> --
> 2.28.0.297.g1956fa8f8d-goog
>


-- 
Luiz Augusto von Dentz

Re: [Bluez PATCH v2 3/3] policy: Reconnect audio on controller resume

[Abhishek Pandit-Subedi]

Hi Luiz,

On Wed, Aug 26, 2020 at 11:32 PM Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
>
> Hi Abhishek,
>
> On Tue, Aug 18, 2020 at 4:28 PM Abhishek Pandit-Subedi
> <abhishekpandit@chromium.org> wrote:
> >
> > During system suspend, all peer devices are disconnected. On resume, HID
> > devices will reconnect but audio devices stay disconnected. As a quality
> > of life improvement, mark audio devices that were disconnected due to
> > suspend and attempt to reconnect them when the controller resumes (after
> > a delay for better co-existence with Wi-Fi).
> >
> > Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
> > ---
> >
> > Changes in v2:
> > - Refactored to use policy instead of connecting directly in adapter
> >
> >  plugins/policy.c | 64 +++++++++++++++++++++++++++++++++++++++++++++---
> >  src/adapter.c    | 45 ++++++++++++++++++++++++++++++++++
> >  src/adapter.h    |  6 +++++
> >  src/main.c       |  1 +
> >  src/main.conf    |  9 +++++++
> >  5 files changed, 121 insertions(+), 4 deletions(-)
> >
> > diff --git a/plugins/policy.c b/plugins/policy.c
> > index de51e58b9..b07a997b9 100644
> > --- a/plugins/policy.c
> > +++ b/plugins/policy.c
> > @@ -75,6 +75,9 @@ static const int default_intervals[] = { 1, 2, 4, 8, 16, 32, 64 };
> >  static int *reconnect_intervals = NULL;
> >  static size_t reconnect_intervals_len = 0;
> >
> > +static const int default_reconnect_delay = 5;
> > +static int resume_reconnect_delay = 5;
> > +
> >  static GSList *reconnects = NULL;
> >
> >  static unsigned int service_id = 0;
> > @@ -93,6 +96,8 @@ struct policy_data {
> >         uint8_t ct_retries;
> >         guint tg_timer;
> >         uint8_t tg_retries;
> > +
> > +       bool reconnect_on_resume;
> >  };
> >
> >  static struct reconnect_data *reconnect_find(struct btd_device *dev)
> > @@ -214,6 +219,7 @@ static gboolean policy_connect_sink(gpointer user_data)
> >
> >         data->sink_timer = 0;
> >         data->sink_retries++;
> > +       data->reconnect_on_resume = false;
> >
> >         service = btd_device_get_service(data->dev, A2DP_SINK_UUID);
> >         if (service != NULL)
> > @@ -222,16 +228,20 @@ static gboolean policy_connect_sink(gpointer user_data)
> >         return FALSE;
> >  }
> >
> > -static void policy_set_sink_timer(struct policy_data *data)
> > +static void policy_set_sink_timer_internal(struct policy_data *data, int timeout)
> >  {
> >         if (data->sink_timer > 0)
> >                 g_source_remove(data->sink_timer);
> >
> > -       data->sink_timer = g_timeout_add_seconds(SINK_RETRY_TIMEOUT,
> > -                                                       policy_connect_sink,
> > +       data->sink_timer = g_timeout_add_seconds(timeout, policy_connect_sink,
> >                                                         data);
> >  }
> >
> > +static void policy_set_sink_timer(struct policy_data *data)
> > +{
> > +       policy_set_sink_timer_internal(data, SINK_RETRY_TIMEOUT);
> > +}
> > +
> >  static void sink_cb(struct btd_service *service, btd_service_state_t old_state,
> >                                                 btd_service_state_t new_state)
> >  {
> > @@ -743,10 +753,30 @@ static void reconnect_set_timer(struct reconnect_data *reconnect)
> >  static void disconnect_cb(struct btd_device *dev, uint8_t reason)
> >  {
> >         struct reconnect_data *reconnect;
> > +       struct btd_service *service;
> > +       struct policy_data *data;
> > +       bool do_reconnect = false;
> >
> >         DBG("reason %u", reason);
> >
> > -       if (reason != MGMT_DEV_DISCONN_TIMEOUT)
> > +       switch(reason) {
> > +       case MGMT_DEV_DISCONN_LOCAL_HOST:
> > +       case MGMT_DEV_DISCONN_REMOTE:
> > +               do_reconnect = true;
> > +               break;
> > +       /* Disconnect due to suspend will queue reconnect on resume for a2dp */
> > +       case MGMT_DEV_DISCONN_LOCAL_HOST_SUSPEND:
> > +               service = btd_device_get_service(dev, A2DP_SINK_UUID);
> > +               if (service && (data = policy_get_data(dev))) {
> > +                       data->reconnect_on_resume = true;
> > +               }
>
> Can't we just program the timer directly here? Or would that wakeup
> the system, I would imagine all timers are disabled while suspended
> which means when the system resumes so does the timers which would
> then trigger the reconnection logic.

This approach works if every resume is user initiated and keeps the
device awake long enough to reconnect to the audio device. On
ChromeOS, this isn't always the case. We have a feature called Dark
Resume that occurs when the system wakes to handle an event that
doesn't require user intervention and suspends again without ever
turning on the screen. See:
https://chromium.googlesource.com/chromiumos/platform2/+/master/power_manager/docs/dark_resume.md.
A primary user for this is a periodic wake-up that checks battery
percentage and shuts down the system if it's too low.

One of the tests I ran specifically for this is a suspend stress test
with a wake time between 2-4s (i.e. suspend_stress_test -c 10
--wake_min 2 --wake_max 3) which emulates dark resumes. If we set the
timer when disconnecting, the device would attempt a connection on one
of the wakeups and fail (since we suspended without completing the
connection).

>
> > +               break;
> > +       /* All other cases do not result in reconnect */
> > +       default:
> > +               break;
> > +       }
> > +
> > +       if (!do_reconnect)
> >                 return;
> >
> >         reconnect = reconnect_find(dev);
> > @@ -761,6 +791,23 @@ static void disconnect_cb(struct btd_device *dev, uint8_t reason)
> >         reconnect_set_timer(reconnect);
> >  }
> >
> > +static void controller_resume_cb(uint8_t wake_reason, const bdaddr_t *addr,
> > +                                       const uint8_t addr_type)
> > +{
> > +       struct btd_device *dev;
> > +       GSList *l;
> > +
> > +       /* Check if any devices needed to be reconnected on resume */
> > +       for (l = devices; l; l = g_slist_next(l)) {
> > +               struct policy_data *data = l->data;
> > +
> > +               if (data->reconnect_on_resume) {
> > +                       policy_set_sink_timer_internal(data,
> > +                                                      resume_reconnect_delay);
> > +               }
> > +       }
> > +}
> > +
> >  static void conn_fail_cb(struct btd_device *dev, uint8_t status)
> >  {
> >         struct reconnect_data *reconnect;
> > @@ -854,9 +901,17 @@ static int policy_init(void)
> >         auto_enable = g_key_file_get_boolean(conf, "Policy", "AutoEnable",
> >                                                                         NULL);
> >
> > +       resume_reconnect_delay = g_key_file_get_integer(
> > +                       conf, "Policy", "ReconnectAudioDelay", &gerr);
> > +
> > +       if (gerr) {
> > +               g_clear_error(&gerr);
> > +               resume_reconnect_delay = default_reconnect_delay;
> > +       }
> >  done:
> >         if (reconnect_uuids && reconnect_uuids[0] && reconnect_attempts) {
> >                 btd_add_disconnect_cb(disconnect_cb);
> > +               btd_add_controller_resume_cb(controller_resume_cb);
> >                 btd_add_conn_fail_cb(conn_fail_cb);
> >         }
> >
> > @@ -869,6 +924,7 @@ done:
> >  static void policy_exit(void)
> >  {
> >         btd_remove_disconnect_cb(disconnect_cb);
> > +       btd_remove_controller_resume_cb(controller_resume_cb);
> >         btd_remove_conn_fail_cb(conn_fail_cb);
> >
> >         if (reconnect_uuids)
> > diff --git a/src/adapter.c b/src/adapter.c
> > index 5e896a9f0..7526feb9e 100644
> > --- a/src/adapter.c
> > +++ b/src/adapter.c
> > @@ -139,6 +139,7 @@ static GSList *adapter_drivers = NULL;
> >
> >  static GSList *disconnect_list = NULL;
> >  static GSList *conn_fail_list = NULL;
> > +static GSList *controller_resume_list = NULL;
> >
> >  struct link_key_info {
> >         bdaddr_t bdaddr;
> > @@ -8766,6 +8767,45 @@ static void connected_callback(uint16_t index, uint16_t length,
> >         eir_data_free(&eir_data);
> >  }
> >
> > +static void controller_resume_notify(const uint8_t wake_reason,
> > +                                       const bdaddr_t *addr,
> > +                                       const uint8_t addr_type)
> > +{
> > +       GSList *l;
> > +
> > +       for (l = controller_resume_list; l; l = g_slist_next(l)) {
> > +               btd_controller_resume_cb resume_cb = l->data;
> > +               resume_cb(wake_reason, addr, addr_type);
> > +       }
> > +}
> > +
> > +static void controller_resume_callback(uint16_t index, uint16_t length,
> > +                                      const void *param, void *user_data)
> > +{
> > +       const struct mgmt_ev_controller_resume *ev = param;
> > +       struct btd_adapter *adapter = user_data;
> > +
> > +       if (length < sizeof(*ev)) {
> > +               btd_error(adapter->dev_id, "Too small device resume event");
> > +               return;
> > +       }
> > +
> > +       DBG("Controller resume with wake event 0x%x", ev->wake_reason);
> > +
> > +       controller_resume_notify(ev->wake_reason, &ev->addr.bdaddr,
> > +                                ev->addr.type);
> > +}
> > +
> > +void btd_add_controller_resume_cb(btd_controller_resume_cb func)
> > +{
> > +       controller_resume_list = g_slist_append(controller_resume_list, func);
> > +}
> > +
> > +void btd_remove_controller_resume_cb(btd_controller_resume_cb func)
> > +{
> > +       controller_resume_list = g_slist_remove(controller_resume_list, func);
> > +}
> > +
> >  static void device_blocked_callback(uint16_t index, uint16_t length,
> >                                         const void *param, void *user_data)
> >  {
> > @@ -9389,6 +9429,11 @@ static void read_info_complete(uint8_t status, uint16_t length,
> >                                                 user_passkey_notify_callback,
> >                                                 adapter, NULL);
> >
> > +       mgmt_register(adapter->mgmt, MGMT_EV_CONTROLLER_RESUME,
> > +                                               adapter->dev_id,
> > +                                               controller_resume_callback,
> > +                                               adapter, NULL);
> > +
> >         set_dev_class(adapter);
> >
> >         set_name(adapter, btd_adapter_get_name(adapter));
> > diff --git a/src/adapter.h b/src/adapter.h
> > index f8ac20261..5527e4205 100644
> > --- a/src/adapter.h
> > +++ b/src/adapter.h
> > @@ -67,6 +67,12 @@ typedef void (*btd_conn_fail_cb) (struct btd_device *device, uint8_t status);
> >  void btd_add_conn_fail_cb(btd_conn_fail_cb func);
> >  void btd_remove_conn_fail_cb(btd_conn_fail_cb func);
> >
> > +typedef void (*btd_controller_resume_cb)(const uint8_t wake_reason,
> > +                                       const bdaddr_t *addr,
> > +                                       const uint8_t addr_type);
> > +void btd_add_controller_resume_cb(btd_controller_resume_cb func);
> > +void btd_remove_controller_resume_cb(btd_controller_resume_cb func);
>
> If we can program the timer just before we suspend this is not really
> necessary, but if you still thing that would be a good idea to notify
> Id put it in btd_adapter_driver with a callback for suspend and
> resume, that way we don't have to maintain multiple lists for each of
> the callbacks.

The adapter driver seems like a reasonable place to put this so let me
port that over.

>
> >  struct btd_adapter *adapter_find(const bdaddr_t *sba);
> >  struct btd_adapter *adapter_find_by_id(int id);
> >  void adapter_foreach(adapter_cb func, gpointer user_data);
> > diff --git a/src/main.c b/src/main.c
> > index 2c083de67..a1c3e7e9f 100644
> > --- a/src/main.c
> > +++ b/src/main.c
> > @@ -131,6 +131,7 @@ static const char *policy_options[] = {
> >         "ReconnectAttempts",
> >         "ReconnectIntervals",
> >         "AutoEnable",
> > +       "ReconnectAudioDelay",
> >         NULL
> >  };
> >
> > diff --git a/src/main.conf b/src/main.conf
> > index f41203b96..8d74a2aec 100644
> > --- a/src/main.conf
> > +++ b/src/main.conf
> > @@ -198,3 +198,12 @@
> >  # This includes adapters present on start as well as adapters that are plugged
> >  # in later on. Defaults to 'false'.
> >  #AutoEnable=false
> > +
> > +# Audio devices that were disconnected due to suspend will be reconnected on
> > +# resume. ReconnectAudioDelay determines the delay between when the controller
> > +# resumes from suspend and a connection attempt is made. A longer delay can be
> > +# better for co-existence with Wi-Fi.
> > +# The value is in seconds.
> > +# Default: 5
> > +#ReconnectAudioDelay = 5
> > +
> > --
> > 2.28.0.297.g1956fa8f8d-goog
> >
>
>
> --
> Luiz Augusto von Dentz

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

[Abhishek Pandit-Subedi]

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

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

[Abhishek Pandit-Subedi]

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

Re: [Bluez PATCH v2 3/3] policy: Reconnect audio on controller resume

[Luiz Augusto von Dentz]

Hi Abhishek,

On Thu, Aug 27, 2020 at 2:08 PM Abhishek Pandit-Subedi
<abhishekpandit@chromium.org> wrote:
>
> Hi Luiz,
>
> On Wed, Aug 26, 2020 at 11:32 PM Luiz Augusto von Dentz
> <luiz.dentz@gmail.com> wrote:
> >
> > Hi Abhishek,
> >
> > On Tue, Aug 18, 2020 at 4:28 PM Abhishek Pandit-Subedi
> > <abhishekpandit@chromium.org> wrote:
> > >
> > > During system suspend, all peer devices are disconnected. On resume, HID
> > > devices will reconnect but audio devices stay disconnected. As a quality
> > > of life improvement, mark audio devices that were disconnected due to
> > > suspend and attempt to reconnect them when the controller resumes (after
> > > a delay for better co-existence with Wi-Fi).
> > >
> > > Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
> > > ---
> > >
> > > Changes in v2:
> > > - Refactored to use policy instead of connecting directly in adapter
> > >
> > >  plugins/policy.c | 64 +++++++++++++++++++++++++++++++++++++++++++++---
> > >  src/adapter.c    | 45 ++++++++++++++++++++++++++++++++++
> > >  src/adapter.h    |  6 +++++
> > >  src/main.c       |  1 +
> > >  src/main.conf    |  9 +++++++
> > >  5 files changed, 121 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/plugins/policy.c b/plugins/policy.c
> > > index de51e58b9..b07a997b9 100644
> > > --- a/plugins/policy.c
> > > +++ b/plugins/policy.c
> > > @@ -75,6 +75,9 @@ static const int default_intervals[] = { 1, 2, 4, 8, 16, 32, 64 };
> > >  static int *reconnect_intervals = NULL;
> > >  static size_t reconnect_intervals_len = 0;
> > >
> > > +static const int default_reconnect_delay = 5;
> > > +static int resume_reconnect_delay = 5;
> > > +
> > >  static GSList *reconnects = NULL;
> > >
> > >  static unsigned int service_id = 0;
> > > @@ -93,6 +96,8 @@ struct policy_data {
> > >         uint8_t ct_retries;
> > >         guint tg_timer;
> > >         uint8_t tg_retries;
> > > +
> > > +       bool reconnect_on_resume;
> > >  };
> > >
> > >  static struct reconnect_data *reconnect_find(struct btd_device *dev)
> > > @@ -214,6 +219,7 @@ static gboolean policy_connect_sink(gpointer user_data)
> > >
> > >         data->sink_timer = 0;
> > >         data->sink_retries++;
> > > +       data->reconnect_on_resume = false;
> > >
> > >         service = btd_device_get_service(data->dev, A2DP_SINK_UUID);
> > >         if (service != NULL)
> > > @@ -222,16 +228,20 @@ static gboolean policy_connect_sink(gpointer user_data)
> > >         return FALSE;
> > >  }
> > >
> > > -static void policy_set_sink_timer(struct policy_data *data)
> > > +static void policy_set_sink_timer_internal(struct policy_data *data, int timeout)
> > >  {
> > >         if (data->sink_timer > 0)
> > >                 g_source_remove(data->sink_timer);
> > >
> > > -       data->sink_timer = g_timeout_add_seconds(SINK_RETRY_TIMEOUT,
> > > -                                                       policy_connect_sink,
> > > +       data->sink_timer = g_timeout_add_seconds(timeout, policy_connect_sink,
> > >                                                         data);
> > >  }
> > >
> > > +static void policy_set_sink_timer(struct policy_data *data)
> > > +{
> > > +       policy_set_sink_timer_internal(data, SINK_RETRY_TIMEOUT);
> > > +}
> > > +
> > >  static void sink_cb(struct btd_service *service, btd_service_state_t old_state,
> > >                                                 btd_service_state_t new_state)
> > >  {
> > > @@ -743,10 +753,30 @@ static void reconnect_set_timer(struct reconnect_data *reconnect)
> > >  static void disconnect_cb(struct btd_device *dev, uint8_t reason)
> > >  {
> > >         struct reconnect_data *reconnect;
> > > +       struct btd_service *service;
> > > +       struct policy_data *data;
> > > +       bool do_reconnect = false;
> > >
> > >         DBG("reason %u", reason);
> > >
> > > -       if (reason != MGMT_DEV_DISCONN_TIMEOUT)
> > > +       switch(reason) {
> > > +       case MGMT_DEV_DISCONN_LOCAL_HOST:
> > > +       case MGMT_DEV_DISCONN_REMOTE:
> > > +               do_reconnect = true;
> > > +               break;
> > > +       /* Disconnect due to suspend will queue reconnect on resume for a2dp */
> > > +       case MGMT_DEV_DISCONN_LOCAL_HOST_SUSPEND:
> > > +               service = btd_device_get_service(dev, A2DP_SINK_UUID);
> > > +               if (service && (data = policy_get_data(dev))) {
> > > +                       data->reconnect_on_resume = true;
> > > +               }
> >
> > Can't we just program the timer directly here? Or would that wakeup
> > the system, I would imagine all timers are disabled while suspended
> > which means when the system resumes so does the timers which would
> > then trigger the reconnection logic.
>
> This approach works if every resume is user initiated and keeps the
> device awake long enough to reconnect to the audio device. On
> ChromeOS, this isn't always the case. We have a feature called Dark
> Resume that occurs when the system wakes to handle an event that
> doesn't require user intervention and suspends again without ever
> turning on the screen. See:
> https://chromium.googlesource.com/chromiumos/platform2/+/master/power_manager/docs/dark_resume.md.
> A primary user for this is a periodic wake-up that checks battery
> percentage and shuts down the system if it's too low.

Ok I didn't know such a feature exists in ChromeOS, but doesn't Dark
Resumes should be handled by the kernel instead of each userspace
component? I suppose there quite a few timers that may have side
effects if dark resumes actually resume them.

> One of the tests I ran specifically for this is a suspend stress test
> with a wake time between 2-4s (i.e. suspend_stress_test -c 10
> --wake_min 2 --wake_max 3) which emulates dark resumes. If we set the
> timer when disconnecting, the device would attempt a connection on one
> of the wakeups and fail (since we suspended without completing the
> connection).

We could perhaps check if the kernel is capable of emitting
suspend/resume events and if not just program the timer, or
alternatively add another config option to indicate when the system
supports such concept of Dark Resumes.

> >
> > > +               break;
> > > +       /* All other cases do not result in reconnect */
> > > +       default:
> > > +               break;
> > > +       }
> > > +
> > > +       if (!do_reconnect)
> > >                 return;
> > >
> > >         reconnect = reconnect_find(dev);
> > > @@ -761,6 +791,23 @@ static void disconnect_cb(struct btd_device *dev, uint8_t reason)
> > >         reconnect_set_timer(reconnect);
> > >  }
> > >
> > > +static void controller_resume_cb(uint8_t wake_reason, const bdaddr_t *addr,
> > > +                                       const uint8_t addr_type)
> > > +{
> > > +       struct btd_device *dev;
> > > +       GSList *l;
> > > +
> > > +       /* Check if any devices needed to be reconnected on resume */
> > > +       for (l = devices; l; l = g_slist_next(l)) {
> > > +               struct policy_data *data = l->data;
> > > +
> > > +               if (data->reconnect_on_resume) {
> > > +                       policy_set_sink_timer_internal(data,
> > > +                                                      resume_reconnect_delay);
> > > +               }
> > > +       }
> > > +}
> > > +
> > >  static void conn_fail_cb(struct btd_device *dev, uint8_t status)
> > >  {
> > >         struct reconnect_data *reconnect;
> > > @@ -854,9 +901,17 @@ static int policy_init(void)
> > >         auto_enable = g_key_file_get_boolean(conf, "Policy", "AutoEnable",
> > >                                                                         NULL);
> > >
> > > +       resume_reconnect_delay = g_key_file_get_integer(
> > > +                       conf, "Policy", "ReconnectAudioDelay", &gerr);
> > > +
> > > +       if (gerr) {
> > > +               g_clear_error(&gerr);
> > > +               resume_reconnect_delay = default_reconnect_delay;
> > > +       }
> > >  done:
> > >         if (reconnect_uuids && reconnect_uuids[0] && reconnect_attempts) {
> > >                 btd_add_disconnect_cb(disconnect_cb);
> > > +               btd_add_controller_resume_cb(controller_resume_cb);
> > >                 btd_add_conn_fail_cb(conn_fail_cb);
> > >         }
> > >
> > > @@ -869,6 +924,7 @@ done:
> > >  static void policy_exit(void)
> > >  {
> > >         btd_remove_disconnect_cb(disconnect_cb);
> > > +       btd_remove_controller_resume_cb(controller_resume_cb);
> > >         btd_remove_conn_fail_cb(conn_fail_cb);
> > >
> > >         if (reconnect_uuids)
> > > diff --git a/src/adapter.c b/src/adapter.c
> > > index 5e896a9f0..7526feb9e 100644
> > > --- a/src/adapter.c
> > > +++ b/src/adapter.c
> > > @@ -139,6 +139,7 @@ static GSList *adapter_drivers = NULL;
> > >
> > >  static GSList *disconnect_list = NULL;
> > >  static GSList *conn_fail_list = NULL;
> > > +static GSList *controller_resume_list = NULL;
> > >
> > >  struct link_key_info {
> > >         bdaddr_t bdaddr;
> > > @@ -8766,6 +8767,45 @@ static void connected_callback(uint16_t index, uint16_t length,
> > >         eir_data_free(&eir_data);
> > >  }
> > >
> > > +static void controller_resume_notify(const uint8_t wake_reason,
> > > +                                       const bdaddr_t *addr,
> > > +                                       const uint8_t addr_type)
> > > +{
> > > +       GSList *l;
> > > +
> > > +       for (l = controller_resume_list; l; l = g_slist_next(l)) {
> > > +               btd_controller_resume_cb resume_cb = l->data;
> > > +               resume_cb(wake_reason, addr, addr_type);
> > > +       }
> > > +}
> > > +
> > > +static void controller_resume_callback(uint16_t index, uint16_t length,
> > > +                                      const void *param, void *user_data)
> > > +{
> > > +       const struct mgmt_ev_controller_resume *ev = param;
> > > +       struct btd_adapter *adapter = user_data;
> > > +
> > > +       if (length < sizeof(*ev)) {
> > > +               btd_error(adapter->dev_id, "Too small device resume event");
> > > +               return;
> > > +       }
> > > +
> > > +       DBG("Controller resume with wake event 0x%x", ev->wake_reason);
> > > +
> > > +       controller_resume_notify(ev->wake_reason, &ev->addr.bdaddr,
> > > +                                ev->addr.type);
> > > +}
> > > +
> > > +void btd_add_controller_resume_cb(btd_controller_resume_cb func)
> > > +{
> > > +       controller_resume_list = g_slist_append(controller_resume_list, func);
> > > +}
> > > +
> > > +void btd_remove_controller_resume_cb(btd_controller_resume_cb func)
> > > +{
> > > +       controller_resume_list = g_slist_remove(controller_resume_list, func);
> > > +}
> > > +
> > >  static void device_blocked_callback(uint16_t index, uint16_t length,
> > >                                         const void *param, void *user_data)
> > >  {
> > > @@ -9389,6 +9429,11 @@ static void read_info_complete(uint8_t status, uint16_t length,
> > >                                                 user_passkey_notify_callback,
> > >                                                 adapter, NULL);
> > >
> > > +       mgmt_register(adapter->mgmt, MGMT_EV_CONTROLLER_RESUME,
> > > +                                               adapter->dev_id,
> > > +                                               controller_resume_callback,
> > > +                                               adapter, NULL);
> > > +
> > >         set_dev_class(adapter);
> > >
> > >         set_name(adapter, btd_adapter_get_name(adapter));
> > > diff --git a/src/adapter.h b/src/adapter.h
> > > index f8ac20261..5527e4205 100644
> > > --- a/src/adapter.h
> > > +++ b/src/adapter.h
> > > @@ -67,6 +67,12 @@ typedef void (*btd_conn_fail_cb) (struct btd_device *device, uint8_t status);
> > >  void btd_add_conn_fail_cb(btd_conn_fail_cb func);
> > >  void btd_remove_conn_fail_cb(btd_conn_fail_cb func);
> > >
> > > +typedef void (*btd_controller_resume_cb)(const uint8_t wake_reason,
> > > +                                       const bdaddr_t *addr,
> > > +                                       const uint8_t addr_type);
> > > +void btd_add_controller_resume_cb(btd_controller_resume_cb func);
> > > +void btd_remove_controller_resume_cb(btd_controller_resume_cb func);
> >
> > If we can program the timer just before we suspend this is not really
> > necessary, but if you still thing that would be a good idea to notify
> > Id put it in btd_adapter_driver with a callback for suspend and
> > resume, that way we don't have to maintain multiple lists for each of
> > the callbacks.
>
> The adapter driver seems like a reasonable place to put this so let me
> port that over.
>
> >
> > >  struct btd_adapter *adapter_find(const bdaddr_t *sba);
> > >  struct btd_adapter *adapter_find_by_id(int id);
> > >  void adapter_foreach(adapter_cb func, gpointer user_data);
> > > diff --git a/src/main.c b/src/main.c
> > > index 2c083de67..a1c3e7e9f 100644
> > > --- a/src/main.c
> > > +++ b/src/main.c
> > > @@ -131,6 +131,7 @@ static const char *policy_options[] = {
> > >         "ReconnectAttempts",
> > >         "ReconnectIntervals",
> > >         "AutoEnable",
> > > +       "ReconnectAudioDelay",
> > >         NULL
> > >  };
> > >
> > > diff --git a/src/main.conf b/src/main.conf
> > > index f41203b96..8d74a2aec 100644
> > > --- a/src/main.conf
> > > +++ b/src/main.conf
> > > @@ -198,3 +198,12 @@
> > >  # This includes adapters present on start as well as adapters that are plugged
> > >  # in later on. Defaults to 'false'.
> > >  #AutoEnable=false
> > > +
> > > +# Audio devices that were disconnected due to suspend will be reconnected on
> > > +# resume. ReconnectAudioDelay determines the delay between when the controller
> > > +# resumes from suspend and a connection attempt is made. A longer delay can be
> > > +# better for co-existence with Wi-Fi.
> > > +# The value is in seconds.
> > > +# Default: 5
> > > +#ReconnectAudioDelay = 5
> > > +
> > > --
> > > 2.28.0.297.g1956fa8f8d-goog
> > >
> >
> >
> > --
> > Luiz Augusto von Dentz



-- 
Luiz Augusto von Dentz

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

[Luiz Augusto von Dentz]

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



-- 
Luiz Augusto von Dentz

Re: [Bluez PATCH v2 3/3] policy: Reconnect audio on controller resume

[Abhishek Pandit-Subedi]

Hi Luiz,

On Fri, Aug 28, 2020 at 10:09 AM Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
>
> Hi Abhishek,
>
> On Thu, Aug 27, 2020 at 2:08 PM Abhishek Pandit-Subedi
> <abhishekpandit@chromium.org> wrote:
> >
> > Hi Luiz,
> >
> > On Wed, Aug 26, 2020 at 11:32 PM Luiz Augusto von Dentz
> > <luiz.dentz@gmail.com> wrote:
> > >
> > > Hi Abhishek,
> > >
> > > On Tue, Aug 18, 2020 at 4:28 PM Abhishek Pandit-Subedi
> > > <abhishekpandit@chromium.org> wrote:
> > > >
> > > > During system suspend, all peer devices are disconnected. On resume, HID
> > > > devices will reconnect but audio devices stay disconnected. As a quality
> > > > of life improvement, mark audio devices that were disconnected due to
> > > > suspend and attempt to reconnect them when the controller resumes (after
> > > > a delay for better co-existence with Wi-Fi).
> > > >
> > > > Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
> > > > ---
> > > >
> > > > Changes in v2:
> > > > - Refactored to use policy instead of connecting directly in adapter
> > > >
> > > >  plugins/policy.c | 64 +++++++++++++++++++++++++++++++++++++++++++++---
> > > >  src/adapter.c    | 45 ++++++++++++++++++++++++++++++++++
> > > >  src/adapter.h    |  6 +++++
> > > >  src/main.c       |  1 +
> > > >  src/main.conf    |  9 +++++++
> > > >  5 files changed, 121 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/plugins/policy.c b/plugins/policy.c
> > > > index de51e58b9..b07a997b9 100644
> > > > --- a/plugins/policy.c
> > > > +++ b/plugins/policy.c
> > > > @@ -75,6 +75,9 @@ static const int default_intervals[] = { 1, 2, 4, 8, 16, 32, 64 };
> > > >  static int *reconnect_intervals = NULL;
> > > >  static size_t reconnect_intervals_len = 0;
> > > >
> > > > +static const int default_reconnect_delay = 5;
> > > > +static int resume_reconnect_delay = 5;
> > > > +
> > > >  static GSList *reconnects = NULL;
> > > >
> > > >  static unsigned int service_id = 0;
> > > > @@ -93,6 +96,8 @@ struct policy_data {
> > > >         uint8_t ct_retries;
> > > >         guint tg_timer;
> > > >         uint8_t tg_retries;
> > > > +
> > > > +       bool reconnect_on_resume;
> > > >  };
> > > >
> > > >  static struct reconnect_data *reconnect_find(struct btd_device *dev)
> > > > @@ -214,6 +219,7 @@ static gboolean policy_connect_sink(gpointer user_data)
> > > >
> > > >         data->sink_timer = 0;
> > > >         data->sink_retries++;
> > > > +       data->reconnect_on_resume = false;
> > > >
> > > >         service = btd_device_get_service(data->dev, A2DP_SINK_UUID);
> > > >         if (service != NULL)
> > > > @@ -222,16 +228,20 @@ static gboolean policy_connect_sink(gpointer user_data)
> > > >         return FALSE;
> > > >  }
> > > >
> > > > -static void policy_set_sink_timer(struct policy_data *data)
> > > > +static void policy_set_sink_timer_internal(struct policy_data *data, int timeout)
> > > >  {
> > > >         if (data->sink_timer > 0)
> > > >                 g_source_remove(data->sink_timer);
> > > >
> > > > -       data->sink_timer = g_timeout_add_seconds(SINK_RETRY_TIMEOUT,
> > > > -                                                       policy_connect_sink,
> > > > +       data->sink_timer = g_timeout_add_seconds(timeout, policy_connect_sink,
> > > >                                                         data);
> > > >  }
> > > >
> > > > +static void policy_set_sink_timer(struct policy_data *data)
> > > > +{
> > > > +       policy_set_sink_timer_internal(data, SINK_RETRY_TIMEOUT);
> > > > +}
> > > > +
> > > >  static void sink_cb(struct btd_service *service, btd_service_state_t old_state,
> > > >                                                 btd_service_state_t new_state)
> > > >  {
> > > > @@ -743,10 +753,30 @@ static void reconnect_set_timer(struct reconnect_data *reconnect)
> > > >  static void disconnect_cb(struct btd_device *dev, uint8_t reason)
> > > >  {
> > > >         struct reconnect_data *reconnect;
> > > > +       struct btd_service *service;
> > > > +       struct policy_data *data;
> > > > +       bool do_reconnect = false;
> > > >
> > > >         DBG("reason %u", reason);
> > > >
> > > > -       if (reason != MGMT_DEV_DISCONN_TIMEOUT)
> > > > +       switch(reason) {
> > > > +       case MGMT_DEV_DISCONN_LOCAL_HOST:
> > > > +       case MGMT_DEV_DISCONN_REMOTE:
> > > > +               do_reconnect = true;
> > > > +               break;
> > > > +       /* Disconnect due to suspend will queue reconnect on resume for a2dp */
> > > > +       case MGMT_DEV_DISCONN_LOCAL_HOST_SUSPEND:
> > > > +               service = btd_device_get_service(dev, A2DP_SINK_UUID);
> > > > +               if (service && (data = policy_get_data(dev))) {
> > > > +                       data->reconnect_on_resume = true;
> > > > +               }
> > >
> > > Can't we just program the timer directly here? Or would that wakeup
> > > the system, I would imagine all timers are disabled while suspended
> > > which means when the system resumes so does the timers which would
> > > then trigger the reconnection logic.
> >
> > This approach works if every resume is user initiated and keeps the
> > device awake long enough to reconnect to the audio device. On
> > ChromeOS, this isn't always the case. We have a feature called Dark
> > Resume that occurs when the system wakes to handle an event that
> > doesn't require user intervention and suspends again without ever
> > turning on the screen. See:
> > https://chromium.googlesource.com/chromiumos/platform2/+/master/power_manager/docs/dark_resume.md.
> > A primary user for this is a periodic wake-up that checks battery
> > percentage and shuts down the system if it's too low.
>
> Ok I didn't know such a feature exists in ChromeOS, but doesn't Dark
> Resumes should be handled by the kernel instead of each userspace
> component? I suppose there quite a few timers that may have side
> effects if dark resumes actually resume them.
>

I'm not aware of the history around it but I found another page with
more context of use cases (like actually letting a user space
application run as would happen on a smartphone):
https://www.chromium.org/chromium-os/chromiumos-design-docs/lucid-sleep

> > One of the tests I ran specifically for this is a suspend stress test
> > with a wake time between 2-4s (i.e. suspend_stress_test -c 10
> > --wake_min 2 --wake_max 3) which emulates dark resumes. If we set the
> > timer when disconnecting, the device would attempt a connection on one
> > of the wakeups and fail (since we suspended without completing the
> > connection).
>
> We could perhaps check if the kernel is capable of emitting
> suspend/resume events and if not just program the timer, or
> alternatively add another config option to indicate when the system
> supports such concept of Dark Resumes.
>

How does the following sound:

* On disconnect, I always queue the reconnect but with the resume
delay set instead of the interval[0] value. I also set the
resume_reconnect flag
* On every resume event (or dark resume event), I reset the timer and
add resume_delay seconds if resume_reconnect is set
* The first time the reconnect_timeout runs, I clear the resume_reconnect flag

The delay will only be reapplied if the kernel is capable of emitting
suspend/resume events and/or dark resume events.

> > >
> > > > +               break;
> > > > +       /* All other cases do not result in reconnect */
> > > > +       default:
> > > > +               break;
> > > > +       }
> > > > +
> > > > +       if (!do_reconnect)
> > > >                 return;
> > > >
> > > >         reconnect = reconnect_find(dev);
> > > > @@ -761,6 +791,23 @@ static void disconnect_cb(struct btd_device *dev, uint8_t reason)
> > > >         reconnect_set_timer(reconnect);
> > > >  }
> > > >
> > > > +static void controller_resume_cb(uint8_t wake_reason, const bdaddr_t *addr,
> > > > +                                       const uint8_t addr_type)
> > > > +{
> > > > +       struct btd_device *dev;
> > > > +       GSList *l;
> > > > +
> > > > +       /* Check if any devices needed to be reconnected on resume */
> > > > +       for (l = devices; l; l = g_slist_next(l)) {
> > > > +               struct policy_data *data = l->data;
> > > > +
> > > > +               if (data->reconnect_on_resume) {
> > > > +                       policy_set_sink_timer_internal(data,
> > > > +                                                      resume_reconnect_delay);
> > > > +               }
> > > > +       }
> > > > +}
> > > > +
> > > >  static void conn_fail_cb(struct btd_device *dev, uint8_t status)
> > > >  {
> > > >         struct reconnect_data *reconnect;
> > > > @@ -854,9 +901,17 @@ static int policy_init(void)
> > > >         auto_enable = g_key_file_get_boolean(conf, "Policy", "AutoEnable",
> > > >                                                                         NULL);
> > > >
> > > > +       resume_reconnect_delay = g_key_file_get_integer(
> > > > +                       conf, "Policy", "ReconnectAudioDelay", &gerr);
> > > > +
> > > > +       if (gerr) {
> > > > +               g_clear_error(&gerr);
> > > > +               resume_reconnect_delay = default_reconnect_delay;
> > > > +       }
> > > >  done:
> > > >         if (reconnect_uuids && reconnect_uuids[0] && reconnect_attempts) {
> > > >                 btd_add_disconnect_cb(disconnect_cb);
> > > > +               btd_add_controller_resume_cb(controller_resume_cb);
> > > >                 btd_add_conn_fail_cb(conn_fail_cb);
> > > >         }
> > > >
> > > > @@ -869,6 +924,7 @@ done:
> > > >  static void policy_exit(void)
> > > >  {
> > > >         btd_remove_disconnect_cb(disconnect_cb);
> > > > +       btd_remove_controller_resume_cb(controller_resume_cb);
> > > >         btd_remove_conn_fail_cb(conn_fail_cb);
> > > >
> > > >         if (reconnect_uuids)
> > > > diff --git a/src/adapter.c b/src/adapter.c
> > > > index 5e896a9f0..7526feb9e 100644
> > > > --- a/src/adapter.c
> > > > +++ b/src/adapter.c
> > > > @@ -139,6 +139,7 @@ static GSList *adapter_drivers = NULL;
> > > >
> > > >  static GSList *disconnect_list = NULL;
> > > >  static GSList *conn_fail_list = NULL;
> > > > +static GSList *controller_resume_list = NULL;
> > > >
> > > >  struct link_key_info {
> > > >         bdaddr_t bdaddr;
> > > > @@ -8766,6 +8767,45 @@ static void connected_callback(uint16_t index, uint16_t length,
> > > >         eir_data_free(&eir_data);
> > > >  }
> > > >
> > > > +static void controller_resume_notify(const uint8_t wake_reason,
> > > > +                                       const bdaddr_t *addr,
> > > > +                                       const uint8_t addr_type)
> > > > +{
> > > > +       GSList *l;
> > > > +
> > > > +       for (l = controller_resume_list; l; l = g_slist_next(l)) {
> > > > +               btd_controller_resume_cb resume_cb = l->data;
> > > > +               resume_cb(wake_reason, addr, addr_type);
> > > > +       }
> > > > +}
> > > > +
> > > > +static void controller_resume_callback(uint16_t index, uint16_t length,
> > > > +                                      const void *param, void *user_data)
> > > > +{
> > > > +       const struct mgmt_ev_controller_resume *ev = param;
> > > > +       struct btd_adapter *adapter = user_data;
> > > > +
> > > > +       if (length < sizeof(*ev)) {
> > > > +               btd_error(adapter->dev_id, "Too small device resume event");
> > > > +               return;
> > > > +       }
> > > > +
> > > > +       DBG("Controller resume with wake event 0x%x", ev->wake_reason);
> > > > +
> > > > +       controller_resume_notify(ev->wake_reason, &ev->addr.bdaddr,
> > > > +                                ev->addr.type);
> > > > +}
> > > > +
> > > > +void btd_add_controller_resume_cb(btd_controller_resume_cb func)
> > > > +{
> > > > +       controller_resume_list = g_slist_append(controller_resume_list, func);
> > > > +}
> > > > +
> > > > +void btd_remove_controller_resume_cb(btd_controller_resume_cb func)
> > > > +{
> > > > +       controller_resume_list = g_slist_remove(controller_resume_list, func);
> > > > +}
> > > > +
> > > >  static void device_blocked_callback(uint16_t index, uint16_t length,
> > > >                                         const void *param, void *user_data)
> > > >  {
> > > > @@ -9389,6 +9429,11 @@ static void read_info_complete(uint8_t status, uint16_t length,
> > > >                                                 user_passkey_notify_callback,
> > > >                                                 adapter, NULL);
> > > >
> > > > +       mgmt_register(adapter->mgmt, MGMT_EV_CONTROLLER_RESUME,
> > > > +                                               adapter->dev_id,
> > > > +                                               controller_resume_callback,
> > > > +                                               adapter, NULL);
> > > > +
> > > >         set_dev_class(adapter);
> > > >
> > > >         set_name(adapter, btd_adapter_get_name(adapter));
> > > > diff --git a/src/adapter.h b/src/adapter.h
> > > > index f8ac20261..5527e4205 100644
> > > > --- a/src/adapter.h
> > > > +++ b/src/adapter.h
> > > > @@ -67,6 +67,12 @@ typedef void (*btd_conn_fail_cb) (struct btd_device *device, uint8_t status);
> > > >  void btd_add_conn_fail_cb(btd_conn_fail_cb func);
> > > >  void btd_remove_conn_fail_cb(btd_conn_fail_cb func);
> > > >
> > > > +typedef void (*btd_controller_resume_cb)(const uint8_t wake_reason,
> > > > +                                       const bdaddr_t *addr,
> > > > +                                       const uint8_t addr_type);
> > > > +void btd_add_controller_resume_cb(btd_controller_resume_cb func);
> > > > +void btd_remove_controller_resume_cb(btd_controller_resume_cb func);
> > >
> > > If we can program the timer just before we suspend this is not really
> > > necessary, but if you still thing that would be a good idea to notify
> > > Id put it in btd_adapter_driver with a callback for suspend and
> > > resume, that way we don't have to maintain multiple lists for each of
> > > the callbacks.
> >
> > The adapter driver seems like a reasonable place to put this so let me
> > port that over.
> >
> > >
> > > >  struct btd_adapter *adapter_find(const bdaddr_t *sba);
> > > >  struct btd_adapter *adapter_find_by_id(int id);
> > > >  void adapter_foreach(adapter_cb func, gpointer user_data);
> > > > diff --git a/src/main.c b/src/main.c
> > > > index 2c083de67..a1c3e7e9f 100644
> > > > --- a/src/main.c
> > > > +++ b/src/main.c
> > > > @@ -131,6 +131,7 @@ static const char *policy_options[] = {
> > > >         "ReconnectAttempts",
> > > >         "ReconnectIntervals",
> > > >         "AutoEnable",
> > > > +       "ReconnectAudioDelay",
> > > >         NULL
> > > >  };
> > > >
> > > > diff --git a/src/main.conf b/src/main.conf
> > > > index f41203b96..8d74a2aec 100644
> > > > --- a/src/main.conf
> > > > +++ b/src/main.conf
> > > > @@ -198,3 +198,12 @@
> > > >  # This includes adapters present on start as well as adapters that are plugged
> > > >  # in later on. Defaults to 'false'.
> > > >  #AutoEnable=false
> > > > +
> > > > +# Audio devices that were disconnected due to suspend will be reconnected on
> > > > +# resume. ReconnectAudioDelay determines the delay between when the controller
> > > > +# resumes from suspend and a connection attempt is made. A longer delay can be
> > > > +# better for co-existence with Wi-Fi.
> > > > +# The value is in seconds.
> > > > +# Default: 5
> > > > +#ReconnectAudioDelay = 5
> > > > +
> > > > --
> > > > 2.28.0.297.g1956fa8f8d-goog
> > > >
> > >
> > >
> > > --
> > > Luiz Augusto von Dentz
>
>
>
> --
> Luiz Augusto von Dentz

Thanks,
Abhishek

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

[Abhishek Pandit-Subedi]

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

Re: [Bluez PATCH v2 3/3] policy: Reconnect audio on controller resume

[Luiz Augusto von Dentz]

Hi Abhishek,

On Fri, Aug 28, 2020 at 10:31 AM Abhishek Pandit-Subedi
<abhishekpandit@chromium.org> wrote:
>
> Hi Luiz,
>
> On Fri, Aug 28, 2020 at 10:09 AM Luiz Augusto von Dentz
> <luiz.dentz@gmail.com> wrote:
> >
> > Hi Abhishek,
> >
> > On Thu, Aug 27, 2020 at 2:08 PM Abhishek Pandit-Subedi
> > <abhishekpandit@chromium.org> wrote:
> > >
> > > Hi Luiz,
> > >
> > > On Wed, Aug 26, 2020 at 11:32 PM Luiz Augusto von Dentz
> > > <luiz.dentz@gmail.com> wrote:
> > > >
> > > > Hi Abhishek,
> > > >
> > > > On Tue, Aug 18, 2020 at 4:28 PM Abhishek Pandit-Subedi
> > > > <abhishekpandit@chromium.org> wrote:
> > > > >
> > > > > During system suspend, all peer devices are disconnected. On resume, HID
> > > > > devices will reconnect but audio devices stay disconnected. As a quality
> > > > > of life improvement, mark audio devices that were disconnected due to
> > > > > suspend and attempt to reconnect them when the controller resumes (after
> > > > > a delay for better co-existence with Wi-Fi).
> > > > >
> > > > > Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
> > > > > ---
> > > > >
> > > > > Changes in v2:
> > > > > - Refactored to use policy instead of connecting directly in adapter
> > > > >
> > > > >  plugins/policy.c | 64 +++++++++++++++++++++++++++++++++++++++++++++---
> > > > >  src/adapter.c    | 45 ++++++++++++++++++++++++++++++++++
> > > > >  src/adapter.h    |  6 +++++
> > > > >  src/main.c       |  1 +
> > > > >  src/main.conf    |  9 +++++++
> > > > >  5 files changed, 121 insertions(+), 4 deletions(-)
> > > > >
> > > > > diff --git a/plugins/policy.c b/plugins/policy.c
> > > > > index de51e58b9..b07a997b9 100644
> > > > > --- a/plugins/policy.c
> > > > > +++ b/plugins/policy.c
> > > > > @@ -75,6 +75,9 @@ static const int default_intervals[] = { 1, 2, 4, 8, 16, 32, 64 };
> > > > >  static int *reconnect_intervals = NULL;
> > > > >  static size_t reconnect_intervals_len = 0;
> > > > >
> > > > > +static const int default_reconnect_delay = 5;
> > > > > +static int resume_reconnect_delay = 5;
> > > > > +
> > > > >  static GSList *reconnects = NULL;
> > > > >
> > > > >  static unsigned int service_id = 0;
> > > > > @@ -93,6 +96,8 @@ struct policy_data {
> > > > >         uint8_t ct_retries;
> > > > >         guint tg_timer;
> > > > >         uint8_t tg_retries;
> > > > > +
> > > > > +       bool reconnect_on_resume;
> > > > >  };
> > > > >
> > > > >  static struct reconnect_data *reconnect_find(struct btd_device *dev)
> > > > > @@ -214,6 +219,7 @@ static gboolean policy_connect_sink(gpointer user_data)
> > > > >
> > > > >         data->sink_timer = 0;
> > > > >         data->sink_retries++;
> > > > > +       data->reconnect_on_resume = false;
> > > > >
> > > > >         service = btd_device_get_service(data->dev, A2DP_SINK_UUID);
> > > > >         if (service != NULL)
> > > > > @@ -222,16 +228,20 @@ static gboolean policy_connect_sink(gpointer user_data)
> > > > >         return FALSE;
> > > > >  }
> > > > >
> > > > > -static void policy_set_sink_timer(struct policy_data *data)
> > > > > +static void policy_set_sink_timer_internal(struct policy_data *data, int timeout)
> > > > >  {
> > > > >         if (data->sink_timer > 0)
> > > > >                 g_source_remove(data->sink_timer);
> > > > >
> > > > > -       data->sink_timer = g_timeout_add_seconds(SINK_RETRY_TIMEOUT,
> > > > > -                                                       policy_connect_sink,
> > > > > +       data->sink_timer = g_timeout_add_seconds(timeout, policy_connect_sink,
> > > > >                                                         data);
> > > > >  }
> > > > >
> > > > > +static void policy_set_sink_timer(struct policy_data *data)
> > > > > +{
> > > > > +       policy_set_sink_timer_internal(data, SINK_RETRY_TIMEOUT);
> > > > > +}
> > > > > +
> > > > >  static void sink_cb(struct btd_service *service, btd_service_state_t old_state,
> > > > >                                                 btd_service_state_t new_state)
> > > > >  {
> > > > > @@ -743,10 +753,30 @@ static void reconnect_set_timer(struct reconnect_data *reconnect)
> > > > >  static void disconnect_cb(struct btd_device *dev, uint8_t reason)
> > > > >  {
> > > > >         struct reconnect_data *reconnect;
> > > > > +       struct btd_service *service;
> > > > > +       struct policy_data *data;
> > > > > +       bool do_reconnect = false;
> > > > >
> > > > >         DBG("reason %u", reason);
> > > > >
> > > > > -       if (reason != MGMT_DEV_DISCONN_TIMEOUT)
> > > > > +       switch(reason) {
> > > > > +       case MGMT_DEV_DISCONN_LOCAL_HOST:
> > > > > +       case MGMT_DEV_DISCONN_REMOTE:
> > > > > +               do_reconnect = true;
> > > > > +               break;
> > > > > +       /* Disconnect due to suspend will queue reconnect on resume for a2dp */
> > > > > +       case MGMT_DEV_DISCONN_LOCAL_HOST_SUSPEND:
> > > > > +               service = btd_device_get_service(dev, A2DP_SINK_UUID);
> > > > > +               if (service && (data = policy_get_data(dev))) {
> > > > > +                       data->reconnect_on_resume = true;
> > > > > +               }
> > > >
> > > > Can't we just program the timer directly here? Or would that wakeup
> > > > the system, I would imagine all timers are disabled while suspended
> > > > which means when the system resumes so does the timers which would
> > > > then trigger the reconnection logic.
> > >
> > > This approach works if every resume is user initiated and keeps the
> > > device awake long enough to reconnect to the audio device. On
> > > ChromeOS, this isn't always the case. We have a feature called Dark
> > > Resume that occurs when the system wakes to handle an event that
> > > doesn't require user intervention and suspends again without ever
> > > turning on the screen. See:
> > > https://chromium.googlesource.com/chromiumos/platform2/+/master/power_manager/docs/dark_resume.md.
> > > A primary user for this is a periodic wake-up that checks battery
> > > percentage and shuts down the system if it's too low.
> >
> > Ok I didn't know such a feature exists in ChromeOS, but doesn't Dark
> > Resumes should be handled by the kernel instead of each userspace
> > component? I suppose there quite a few timers that may have side
> > effects if dark resumes actually resume them.
> >
>
> I'm not aware of the history around it but I found another page with
> more context of use cases (like actually letting a user space
> application run as would happen on a smartphone):
> https://www.chromium.org/chromium-os/chromiumos-design-docs/lucid-sleep
>
> > > One of the tests I ran specifically for this is a suspend stress test
> > > with a wake time between 2-4s (i.e. suspend_stress_test -c 10
> > > --wake_min 2 --wake_max 3) which emulates dark resumes. If we set the
> > > timer when disconnecting, the device would attempt a connection on one
> > > of the wakeups and fail (since we suspended without completing the
> > > connection).
> >
> > We could perhaps check if the kernel is capable of emitting
> > suspend/resume events and if not just program the timer, or
> > alternatively add another config option to indicate when the system
> > supports such concept of Dark Resumes.
> >
>
> How does the following sound:
>
> * On disconnect, I always queue the reconnect but with the resume
> delay set instead of the interval[0] value. I also set the
> resume_reconnect flag
> * On every resume event (or dark resume event), I reset the timer and
> add resume_delay seconds if resume_reconnect is set
> * The first time the reconnect_timeout runs, I clear the resume_reconnect flag
>
> The delay will only be reapplied if the kernel is capable of emitting
> suspend/resume events and/or dark resume events.

That should work as well, does full resumes need to reset the timer
though? Or we don't have any means to identify the resume type? Well
either way I guess the difference would be quite small since the
timeout is the same just when it is programmed (on suspend vs on
resume).

> > > >
> > > > > +               break;
> > > > > +       /* All other cases do not result in reconnect */
> > > > > +       default:
> > > > > +               break;
> > > > > +       }
> > > > > +
> > > > > +       if (!do_reconnect)
> > > > >                 return;
> > > > >
> > > > >         reconnect = reconnect_find(dev);
> > > > > @@ -761,6 +791,23 @@ static void disconnect_cb(struct btd_device *dev, uint8_t reason)
> > > > >         reconnect_set_timer(reconnect);
> > > > >  }
> > > > >
> > > > > +static void controller_resume_cb(uint8_t wake_reason, const bdaddr_t *addr,
> > > > > +                                       const uint8_t addr_type)
> > > > > +{
> > > > > +       struct btd_device *dev;
> > > > > +       GSList *l;
> > > > > +
> > > > > +       /* Check if any devices needed to be reconnected on resume */
> > > > > +       for (l = devices; l; l = g_slist_next(l)) {
> > > > > +               struct policy_data *data = l->data;
> > > > > +
> > > > > +               if (data->reconnect_on_resume) {
> > > > > +                       policy_set_sink_timer_internal(data,
> > > > > +                                                      resume_reconnect_delay);
> > > > > +               }
> > > > > +       }
> > > > > +}
> > > > > +
> > > > >  static void conn_fail_cb(struct btd_device *dev, uint8_t status)
> > > > >  {
> > > > >         struct reconnect_data *reconnect;
> > > > > @@ -854,9 +901,17 @@ static int policy_init(void)
> > > > >         auto_enable = g_key_file_get_boolean(conf, "Policy", "AutoEnable",
> > > > >                                                                         NULL);
> > > > >
> > > > > +       resume_reconnect_delay = g_key_file_get_integer(
> > > > > +                       conf, "Policy", "ReconnectAudioDelay", &gerr);
> > > > > +
> > > > > +       if (gerr) {
> > > > > +               g_clear_error(&gerr);
> > > > > +               resume_reconnect_delay = default_reconnect_delay;
> > > > > +       }
> > > > >  done:
> > > > >         if (reconnect_uuids && reconnect_uuids[0] && reconnect_attempts) {
> > > > >                 btd_add_disconnect_cb(disconnect_cb);
> > > > > +               btd_add_controller_resume_cb(controller_resume_cb);
> > > > >                 btd_add_conn_fail_cb(conn_fail_cb);
> > > > >         }
> > > > >
> > > > > @@ -869,6 +924,7 @@ done:
> > > > >  static void policy_exit(void)
> > > > >  {
> > > > >         btd_remove_disconnect_cb(disconnect_cb);
> > > > > +       btd_remove_controller_resume_cb(controller_resume_cb);
> > > > >         btd_remove_conn_fail_cb(conn_fail_cb);
> > > > >
> > > > >         if (reconnect_uuids)
> > > > > diff --git a/src/adapter.c b/src/adapter.c
> > > > > index 5e896a9f0..7526feb9e 100644
> > > > > --- a/src/adapter.c
> > > > > +++ b/src/adapter.c
> > > > > @@ -139,6 +139,7 @@ static GSList *adapter_drivers = NULL;
> > > > >
> > > > >  static GSList *disconnect_list = NULL;
> > > > >  static GSList *conn_fail_list = NULL;
> > > > > +static GSList *controller_resume_list = NULL;
> > > > >
> > > > >  struct link_key_info {
> > > > >         bdaddr_t bdaddr;
> > > > > @@ -8766,6 +8767,45 @@ static void connected_callback(uint16_t index, uint16_t length,
> > > > >         eir_data_free(&eir_data);
> > > > >  }
> > > > >
> > > > > +static void controller_resume_notify(const uint8_t wake_reason,
> > > > > +                                       const bdaddr_t *addr,
> > > > > +                                       const uint8_t addr_type)
> > > > > +{
> > > > > +       GSList *l;
> > > > > +
> > > > > +       for (l = controller_resume_list; l; l = g_slist_next(l)) {
> > > > > +               btd_controller_resume_cb resume_cb = l->data;
> > > > > +               resume_cb(wake_reason, addr, addr_type);
> > > > > +       }
> > > > > +}
> > > > > +
> > > > > +static void controller_resume_callback(uint16_t index, uint16_t length,
> > > > > +                                      const void *param, void *user_data)
> > > > > +{
> > > > > +       const struct mgmt_ev_controller_resume *ev = param;
> > > > > +       struct btd_adapter *adapter = user_data;
> > > > > +
> > > > > +       if (length < sizeof(*ev)) {
> > > > > +               btd_error(adapter->dev_id, "Too small device resume event");
> > > > > +               return;
> > > > > +       }
> > > > > +
> > > > > +       DBG("Controller resume with wake event 0x%x", ev->wake_reason);
> > > > > +
> > > > > +       controller_resume_notify(ev->wake_reason, &ev->addr.bdaddr,
> > > > > +                                ev->addr.type);
> > > > > +}
> > > > > +
> > > > > +void btd_add_controller_resume_cb(btd_controller_resume_cb func)
> > > > > +{
> > > > > +       controller_resume_list = g_slist_append(controller_resume_list, func);
> > > > > +}
> > > > > +
> > > > > +void btd_remove_controller_resume_cb(btd_controller_resume_cb func)
> > > > > +{
> > > > > +       controller_resume_list = g_slist_remove(controller_resume_list, func);
> > > > > +}
> > > > > +
> > > > >  static void device_blocked_callback(uint16_t index, uint16_t length,
> > > > >                                         const void *param, void *user_data)
> > > > >  {
> > > > > @@ -9389,6 +9429,11 @@ static void read_info_complete(uint8_t status, uint16_t length,
> > > > >                                                 user_passkey_notify_callback,
> > > > >                                                 adapter, NULL);
> > > > >
> > > > > +       mgmt_register(adapter->mgmt, MGMT_EV_CONTROLLER_RESUME,
> > > > > +                                               adapter->dev_id,
> > > > > +                                               controller_resume_callback,
> > > > > +                                               adapter, NULL);
> > > > > +
> > > > >         set_dev_class(adapter);
> > > > >
> > > > >         set_name(adapter, btd_adapter_get_name(adapter));
> > > > > diff --git a/src/adapter.h b/src/adapter.h
> > > > > index f8ac20261..5527e4205 100644
> > > > > --- a/src/adapter.h
> > > > > +++ b/src/adapter.h
> > > > > @@ -67,6 +67,12 @@ typedef void (*btd_conn_fail_cb) (struct btd_device *device, uint8_t status);
> > > > >  void btd_add_conn_fail_cb(btd_conn_fail_cb func);
> > > > >  void btd_remove_conn_fail_cb(btd_conn_fail_cb func);
> > > > >
> > > > > +typedef void (*btd_controller_resume_cb)(const uint8_t wake_reason,
> > > > > +                                       const bdaddr_t *addr,
> > > > > +                                       const uint8_t addr_type);
> > > > > +void btd_add_controller_resume_cb(btd_controller_resume_cb func);
> > > > > +void btd_remove_controller_resume_cb(btd_controller_resume_cb func);
> > > >
> > > > If we can program the timer just before we suspend this is not really
> > > > necessary, but if you still thing that would be a good idea to notify
> > > > Id put it in btd_adapter_driver with a callback for suspend and
> > > > resume, that way we don't have to maintain multiple lists for each of
> > > > the callbacks.
> > >
> > > The adapter driver seems like a reasonable place to put this so let me
> > > port that over.
> > >
> > > >
> > > > >  struct btd_adapter *adapter_find(const bdaddr_t *sba);
> > > > >  struct btd_adapter *adapter_find_by_id(int id);
> > > > >  void adapter_foreach(adapter_cb func, gpointer user_data);
> > > > > diff --git a/src/main.c b/src/main.c
> > > > > index 2c083de67..a1c3e7e9f 100644
> > > > > --- a/src/main.c
> > > > > +++ b/src/main.c
> > > > > @@ -131,6 +131,7 @@ static const char *policy_options[] = {
> > > > >         "ReconnectAttempts",
> > > > >         "ReconnectIntervals",
> > > > >         "AutoEnable",
> > > > > +       "ReconnectAudioDelay",
> > > > >         NULL
> > > > >  };
> > > > >
> > > > > diff --git a/src/main.conf b/src/main.conf
> > > > > index f41203b96..8d74a2aec 100644
> > > > > --- a/src/main.conf
> > > > > +++ b/src/main.conf
> > > > > @@ -198,3 +198,12 @@
> > > > >  # This includes adapters present on start as well as adapters that are plugged
> > > > >  # in later on. Defaults to 'false'.
> > > > >  #AutoEnable=false
> > > > > +
> > > > > +# Audio devices that were disconnected due to suspend will be reconnected on
> > > > > +# resume. ReconnectAudioDelay determines the delay between when the controller
> > > > > +# resumes from suspend and a connection attempt is made. A longer delay can be
> > > > > +# better for co-existence with Wi-Fi.
> > > > > +# The value is in seconds.
> > > > > +# Default: 5
> > > > > +#ReconnectAudioDelay = 5
> > > > > +
> > > > > --
> > > > > 2.28.0.297.g1956fa8f8d-goog
> > > > >
> > > >
> > > >
> > > > --
> > > > Luiz Augusto von Dentz
> >
> >
> >
> > --
> > Luiz Augusto von Dentz
>
> Thanks,
> Abhishek



-- 
Luiz Augusto von Dentz

Re: [Bluez PATCH v2 3/3] policy: Reconnect audio on controller resume

[Abhishek Pandit-Subedi]

Hi Luiz,

On Fri, Aug 28, 2020 at 10:56 AM Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
>
> Hi Abhishek,
>
> On Fri, Aug 28, 2020 at 10:31 AM Abhishek Pandit-Subedi
> <abhishekpandit@chromium.org> wrote:
> >
> > Hi Luiz,
> >
> > On Fri, Aug 28, 2020 at 10:09 AM Luiz Augusto von Dentz
> > <luiz.dentz@gmail.com> wrote:
> > >
> > > Hi Abhishek,
> > >
> > > On Thu, Aug 27, 2020 at 2:08 PM Abhishek Pandit-Subedi
> > > <abhishekpandit@chromium.org> wrote:
> > > >
> > > > Hi Luiz,
> > > >
> > > > On Wed, Aug 26, 2020 at 11:32 PM Luiz Augusto von Dentz
> > > > <luiz.dentz@gmail.com> wrote:
> > > > >
> > > > > Hi Abhishek,
> > > > >
> > > > > On Tue, Aug 18, 2020 at 4:28 PM Abhishek Pandit-Subedi
> > > > > <abhishekpandit@chromium.org> wrote:
> > > > > >
> > > > > > During system suspend, all peer devices are disconnected. On resume, HID
> > > > > > devices will reconnect but audio devices stay disconnected. As a quality
> > > > > > of life improvement, mark audio devices that were disconnected due to
> > > > > > suspend and attempt to reconnect them when the controller resumes (after
> > > > > > a delay for better co-existence with Wi-Fi).
> > > > > >
> > > > > > Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
> > > > > > ---
> > > > > >
> > > > > > Changes in v2:
> > > > > > - Refactored to use policy instead of connecting directly in adapter
> > > > > >
> > > > > >  plugins/policy.c | 64 +++++++++++++++++++++++++++++++++++++++++++++---
> > > > > >  src/adapter.c    | 45 ++++++++++++++++++++++++++++++++++
> > > > > >  src/adapter.h    |  6 +++++
> > > > > >  src/main.c       |  1 +
> > > > > >  src/main.conf    |  9 +++++++
> > > > > >  5 files changed, 121 insertions(+), 4 deletions(-)
> > > > > >
> > > > > > diff --git a/plugins/policy.c b/plugins/policy.c
> > > > > > index de51e58b9..b07a997b9 100644
> > > > > > --- a/plugins/policy.c
> > > > > > +++ b/plugins/policy.c
> > > > > > @@ -75,6 +75,9 @@ static const int default_intervals[] = { 1, 2, 4, 8, 16, 32, 64 };
> > > > > >  static int *reconnect_intervals = NULL;
> > > > > >  static size_t reconnect_intervals_len = 0;
> > > > > >
> > > > > > +static const int default_reconnect_delay = 5;
> > > > > > +static int resume_reconnect_delay = 5;
> > > > > > +
> > > > > >  static GSList *reconnects = NULL;
> > > > > >
> > > > > >  static unsigned int service_id = 0;
> > > > > > @@ -93,6 +96,8 @@ struct policy_data {
> > > > > >         uint8_t ct_retries;
> > > > > >         guint tg_timer;
> > > > > >         uint8_t tg_retries;
> > > > > > +
> > > > > > +       bool reconnect_on_resume;
> > > > > >  };
> > > > > >
> > > > > >  static struct reconnect_data *reconnect_find(struct btd_device *dev)
> > > > > > @@ -214,6 +219,7 @@ static gboolean policy_connect_sink(gpointer user_data)
> > > > > >
> > > > > >         data->sink_timer = 0;
> > > > > >         data->sink_retries++;
> > > > > > +       data->reconnect_on_resume = false;
> > > > > >
> > > > > >         service = btd_device_get_service(data->dev, A2DP_SINK_UUID);
> > > > > >         if (service != NULL)
> > > > > > @@ -222,16 +228,20 @@ static gboolean policy_connect_sink(gpointer user_data)
> > > > > >         return FALSE;
> > > > > >  }
> > > > > >
> > > > > > -static void policy_set_sink_timer(struct policy_data *data)
> > > > > > +static void policy_set_sink_timer_internal(struct policy_data *data, int timeout)
> > > > > >  {
> > > > > >         if (data->sink_timer > 0)
> > > > > >                 g_source_remove(data->sink_timer);
> > > > > >
> > > > > > -       data->sink_timer = g_timeout_add_seconds(SINK_RETRY_TIMEOUT,
> > > > > > -                                                       policy_connect_sink,
> > > > > > +       data->sink_timer = g_timeout_add_seconds(timeout, policy_connect_sink,
> > > > > >                                                         data);
> > > > > >  }
> > > > > >
> > > > > > +static void policy_set_sink_timer(struct policy_data *data)
> > > > > > +{
> > > > > > +       policy_set_sink_timer_internal(data, SINK_RETRY_TIMEOUT);
> > > > > > +}
> > > > > > +
> > > > > >  static void sink_cb(struct btd_service *service, btd_service_state_t old_state,
> > > > > >                                                 btd_service_state_t new_state)
> > > > > >  {
> > > > > > @@ -743,10 +753,30 @@ static void reconnect_set_timer(struct reconnect_data *reconnect)
> > > > > >  static void disconnect_cb(struct btd_device *dev, uint8_t reason)
> > > > > >  {
> > > > > >         struct reconnect_data *reconnect;
> > > > > > +       struct btd_service *service;
> > > > > > +       struct policy_data *data;
> > > > > > +       bool do_reconnect = false;
> > > > > >
> > > > > >         DBG("reason %u", reason);
> > > > > >
> > > > > > -       if (reason != MGMT_DEV_DISCONN_TIMEOUT)
> > > > > > +       switch(reason) {
> > > > > > +       case MGMT_DEV_DISCONN_LOCAL_HOST:
> > > > > > +       case MGMT_DEV_DISCONN_REMOTE:
> > > > > > +               do_reconnect = true;
> > > > > > +               break;
> > > > > > +       /* Disconnect due to suspend will queue reconnect on resume for a2dp */
> > > > > > +       case MGMT_DEV_DISCONN_LOCAL_HOST_SUSPEND:
> > > > > > +               service = btd_device_get_service(dev, A2DP_SINK_UUID);
> > > > > > +               if (service && (data = policy_get_data(dev))) {
> > > > > > +                       data->reconnect_on_resume = true;
> > > > > > +               }
> > > > >
> > > > > Can't we just program the timer directly here? Or would that wakeup
> > > > > the system, I would imagine all timers are disabled while suspended
> > > > > which means when the system resumes so does the timers which would
> > > > > then trigger the reconnection logic.
> > > >
> > > > This approach works if every resume is user initiated and keeps the
> > > > device awake long enough to reconnect to the audio device. On
> > > > ChromeOS, this isn't always the case. We have a feature called Dark
> > > > Resume that occurs when the system wakes to handle an event that
> > > > doesn't require user intervention and suspends again without ever
> > > > turning on the screen. See:
> > > > https://chromium.googlesource.com/chromiumos/platform2/+/master/power_manager/docs/dark_resume.md.
> > > > A primary user for this is a periodic wake-up that checks battery
> > > > percentage and shuts down the system if it's too low.
> > >
> > > Ok I didn't know such a feature exists in ChromeOS, but doesn't Dark
> > > Resumes should be handled by the kernel instead of each userspace
> > > component? I suppose there quite a few timers that may have side
> > > effects if dark resumes actually resume them.
> > >
> >
> > I'm not aware of the history around it but I found another page with
> > more context of use cases (like actually letting a user space
> > application run as would happen on a smartphone):
> > https://www.chromium.org/chromium-os/chromiumos-design-docs/lucid-sleep
> >
> > > > One of the tests I ran specifically for this is a suspend stress test
> > > > with a wake time between 2-4s (i.e. suspend_stress_test -c 10
> > > > --wake_min 2 --wake_max 3) which emulates dark resumes. If we set the
> > > > timer when disconnecting, the device would attempt a connection on one
> > > > of the wakeups and fail (since we suspended without completing the
> > > > connection).
> > >
> > > We could perhaps check if the kernel is capable of emitting
> > > suspend/resume events and if not just program the timer, or
> > > alternatively add another config option to indicate when the system
> > > supports such concept of Dark Resumes.
> > >
> >
> > How does the following sound:
> >
> > * On disconnect, I always queue the reconnect but with the resume
> > delay set instead of the interval[0] value. I also set the
> > resume_reconnect flag
> > * On every resume event (or dark resume event), I reset the timer and
> > add resume_delay seconds if resume_reconnect is set
> > * The first time the reconnect_timeout runs, I clear the resume_reconnect flag
> >
> > The delay will only be reapplied if the kernel is capable of emitting
> > suspend/resume events and/or dark resume events.
>
> That should work as well, does full resumes need to reset the timer
> though? Or we don't have any means to identify the resume type? Well
> either way I guess the difference would be quite small since the
> timeout is the same just when it is programmed (on suspend vs on
> resume).

The timer exists to avoid trying to connect at the same time wi-fi may
be trying to associate.

By the way, I had a chance to test all this today and setting the
reconnect timer on disconnect causes the connection to sometimes occur
before the resume event has a chance to run. Depending on the exact
timing, this will likely break the desired dark resume behavior.
I tried `suspend_stress_test --wake_min=1 --wake_max=1 -c 5` and had
the headset reconnect on the 3rd suspend (instead of only at the end)
when using a ResumeDelay of 2.

So I propose the following behavior instead:
* On disconnect, queue a reconnect if reason != SUSPEND. If reason ==
SUSPEND, set reconnect->on_resume = true
* If we support dark resume events (i.e. chromium), set the timer if
resume reason == full resume and reconnect->on_resume = true (this
will probably be a CHROMIUM patch that we carry in our repositories)
* If we support kernel events (and not dark resume), set the timer if
reconnect->on_resume = true (this will be the upstream version)

I will update the resume delay to 2 and test further with some older
Chromebooks. If this reduces the chance of success of BT reconnecting,
we can come back and update this later.

>
> > > > >
> > > > > > +               break;
> > > > > > +       /* All other cases do not result in reconnect */
> > > > > > +       default:
> > > > > > +               break;
> > > > > > +       }
> > > > > > +
> > > > > > +       if (!do_reconnect)
> > > > > >                 return;
> > > > > >
> > > > > >         reconnect = reconnect_find(dev);
> > > > > > @@ -761,6 +791,23 @@ static void disconnect_cb(struct btd_device *dev, uint8_t reason)
> > > > > >         reconnect_set_timer(reconnect);
> > > > > >  }
> > > > > >
> > > > > > +static void controller_resume_cb(uint8_t wake_reason, const bdaddr_t *addr,
> > > > > > +                                       const uint8_t addr_type)
> > > > > > +{
> > > > > > +       struct btd_device *dev;
> > > > > > +       GSList *l;
> > > > > > +
> > > > > > +       /* Check if any devices needed to be reconnected on resume */
> > > > > > +       for (l = devices; l; l = g_slist_next(l)) {
> > > > > > +               struct policy_data *data = l->data;
> > > > > > +
> > > > > > +               if (data->reconnect_on_resume) {
> > > > > > +                       policy_set_sink_timer_internal(data,
> > > > > > +                                                      resume_reconnect_delay);
> > > > > > +               }
> > > > > > +       }
> > > > > > +}
> > > > > > +
> > > > > >  static void conn_fail_cb(struct btd_device *dev, uint8_t status)
> > > > > >  {
> > > > > >         struct reconnect_data *reconnect;
> > > > > > @@ -854,9 +901,17 @@ static int policy_init(void)
> > > > > >         auto_enable = g_key_file_get_boolean(conf, "Policy", "AutoEnable",
> > > > > >                                                                         NULL);
> > > > > >
> > > > > > +       resume_reconnect_delay = g_key_file_get_integer(
> > > > > > +                       conf, "Policy", "ReconnectAudioDelay", &gerr);
> > > > > > +
> > > > > > +       if (gerr) {
> > > > > > +               g_clear_error(&gerr);
> > > > > > +               resume_reconnect_delay = default_reconnect_delay;
> > > > > > +       }
> > > > > >  done:
> > > > > >         if (reconnect_uuids && reconnect_uuids[0] && reconnect_attempts) {
> > > > > >                 btd_add_disconnect_cb(disconnect_cb);
> > > > > > +               btd_add_controller_resume_cb(controller_resume_cb);
> > > > > >                 btd_add_conn_fail_cb(conn_fail_cb);
> > > > > >         }
> > > > > >
> > > > > > @@ -869,6 +924,7 @@ done:
> > > > > >  static void policy_exit(void)
> > > > > >  {
> > > > > >         btd_remove_disconnect_cb(disconnect_cb);
> > > > > > +       btd_remove_controller_resume_cb(controller_resume_cb);
> > > > > >         btd_remove_conn_fail_cb(conn_fail_cb);
> > > > > >
> > > > > >         if (reconnect_uuids)
> > > > > > diff --git a/src/adapter.c b/src/adapter.c
> > > > > > index 5e896a9f0..7526feb9e 100644
> > > > > > --- a/src/adapter.c
> > > > > > +++ b/src/adapter.c
> > > > > > @@ -139,6 +139,7 @@ static GSList *adapter_drivers = NULL;
> > > > > >
> > > > > >  static GSList *disconnect_list = NULL;
> > > > > >  static GSList *conn_fail_list = NULL;
> > > > > > +static GSList *controller_resume_list = NULL;
> > > > > >
> > > > > >  struct link_key_info {
> > > > > >         bdaddr_t bdaddr;
> > > > > > @@ -8766,6 +8767,45 @@ static void connected_callback(uint16_t index, uint16_t length,
> > > > > >         eir_data_free(&eir_data);
> > > > > >  }
> > > > > >
> > > > > > +static void controller_resume_notify(const uint8_t wake_reason,
> > > > > > +                                       const bdaddr_t *addr,
> > > > > > +                                       const uint8_t addr_type)
> > > > > > +{
> > > > > > +       GSList *l;
> > > > > > +
> > > > > > +       for (l = controller_resume_list; l; l = g_slist_next(l)) {
> > > > > > +               btd_controller_resume_cb resume_cb = l->data;
> > > > > > +               resume_cb(wake_reason, addr, addr_type);
> > > > > > +       }
> > > > > > +}
> > > > > > +
> > > > > > +static void controller_resume_callback(uint16_t index, uint16_t length,
> > > > > > +                                      const void *param, void *user_data)
> > > > > > +{
> > > > > > +       const struct mgmt_ev_controller_resume *ev = param;
> > > > > > +       struct btd_adapter *adapter = user_data;
> > > > > > +
> > > > > > +       if (length < sizeof(*ev)) {
> > > > > > +               btd_error(adapter->dev_id, "Too small device resume event");
> > > > > > +               return;
> > > > > > +       }
> > > > > > +
> > > > > > +       DBG("Controller resume with wake event 0x%x", ev->wake_reason);
> > > > > > +
> > > > > > +       controller_resume_notify(ev->wake_reason, &ev->addr.bdaddr,
> > > > > > +                                ev->addr.type);
> > > > > > +}
> > > > > > +
> > > > > > +void btd_add_controller_resume_cb(btd_controller_resume_cb func)
> > > > > > +{
> > > > > > +       controller_resume_list = g_slist_append(controller_resume_list, func);
> > > > > > +}
> > > > > > +
> > > > > > +void btd_remove_controller_resume_cb(btd_controller_resume_cb func)
> > > > > > +{
> > > > > > +       controller_resume_list = g_slist_remove(controller_resume_list, func);
> > > > > > +}
> > > > > > +
> > > > > >  static void device_blocked_callback(uint16_t index, uint16_t length,
> > > > > >                                         const void *param, void *user_data)
> > > > > >  {
> > > > > > @@ -9389,6 +9429,11 @@ static void read_info_complete(uint8_t status, uint16_t length,
> > > > > >                                                 user_passkey_notify_callback,
> > > > > >                                                 adapter, NULL);
> > > > > >
> > > > > > +       mgmt_register(adapter->mgmt, MGMT_EV_CONTROLLER_RESUME,
> > > > > > +                                               adapter->dev_id,
> > > > > > +                                               controller_resume_callback,
> > > > > > +                                               adapter, NULL);
> > > > > > +
> > > > > >         set_dev_class(adapter);
> > > > > >
> > > > > >         set_name(adapter, btd_adapter_get_name(adapter));
> > > > > > diff --git a/src/adapter.h b/src/adapter.h
> > > > > > index f8ac20261..5527e4205 100644
> > > > > > --- a/src/adapter.h
> > > > > > +++ b/src/adapter.h
> > > > > > @@ -67,6 +67,12 @@ typedef void (*btd_conn_fail_cb) (struct btd_device *device, uint8_t status);
> > > > > >  void btd_add_conn_fail_cb(btd_conn_fail_cb func);
> > > > > >  void btd_remove_conn_fail_cb(btd_conn_fail_cb func);
> > > > > >
> > > > > > +typedef void (*btd_controller_resume_cb)(const uint8_t wake_reason,
> > > > > > +                                       const bdaddr_t *addr,
> > > > > > +                                       const uint8_t addr_type);
> > > > > > +void btd_add_controller_resume_cb(btd_controller_resume_cb func);
> > > > > > +void btd_remove_controller_resume_cb(btd_controller_resume_cb func);
> > > > >
> > > > > If we can program the timer just before we suspend this is not really
> > > > > necessary, but if you still thing that would be a good idea to notify
> > > > > Id put it in btd_adapter_driver with a callback for suspend and
> > > > > resume, that way we don't have to maintain multiple lists for each of
> > > > > the callbacks.
> > > >
> > > > The adapter driver seems like a reasonable place to put this so let me
> > > > port that over.
> > > >
> > > > >
> > > > > >  struct btd_adapter *adapter_find(const bdaddr_t *sba);
> > > > > >  struct btd_adapter *adapter_find_by_id(int id);
> > > > > >  void adapter_foreach(adapter_cb func, gpointer user_data);
> > > > > > diff --git a/src/main.c b/src/main.c
> > > > > > index 2c083de67..a1c3e7e9f 100644
> > > > > > --- a/src/main.c
> > > > > > +++ b/src/main.c
> > > > > > @@ -131,6 +131,7 @@ static const char *policy_options[] = {
> > > > > >         "ReconnectAttempts",
> > > > > >         "ReconnectIntervals",
> > > > > >         "AutoEnable",
> > > > > > +       "ReconnectAudioDelay",
> > > > > >         NULL
> > > > > >  };
> > > > > >
> > > > > > diff --git a/src/main.conf b/src/main.conf
> > > > > > index f41203b96..8d74a2aec 100644
> > > > > > --- a/src/main.conf
> > > > > > +++ b/src/main.conf
> > > > > > @@ -198,3 +198,12 @@
> > > > > >  # This includes adapters present on start as well as adapters that are plugged
> > > > > >  # in later on. Defaults to 'false'.
> > > > > >  #AutoEnable=false
> > > > > > +
> > > > > > +# Audio devices that were disconnected due to suspend will be reconnected on
> > > > > > +# resume. ReconnectAudioDelay determines the delay between when the controller
> > > > > > +# resumes from suspend and a connection attempt is made. A longer delay can be
> > > > > > +# better for co-existence with Wi-Fi.
> > > > > > +# The value is in seconds.
> > > > > > +# Default: 5
> > > > > > +#ReconnectAudioDelay = 5
> > > > > > +
> > > > > > --
> > > > > > 2.28.0.297.g1956fa8f8d-goog
> > > > > >
> > > > >
> > > > >
> > > > > --
> > > > > Luiz Augusto von Dentz
> > >
> > >
> > >
> > > --
> > > Luiz Augusto von Dentz
> >
> > Thanks,
> > Abhishek
>
>
>
> --
> Luiz Augusto von Dentz