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:
* In the device disconnect callback, mark any devices with the A2DP
service uuid for reconnect. The reconnect will not be queued until
resume.
* In the controller resume callback, queue any policy items that are
marked to reconnect on resume for connection with the ResumeDelay
value (default = 2s).
A reconnect is queued after the controller resumes and the delay
between resume and reconnect is configurable via the ResumeDelay key in
the Policy settings. The 2s 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 = 1s 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 on last resume.
- Suspend test with wake time between 1s - 4s. Ran with 5 iterations and
verified it connected several times in the middle and finally at the
end.
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 v3:
- Refactored resume notification to use btd_adapter_driver
- Renamed ReconnectAudioDelay to ResumeDelay and set default to 2
- Added A2DP_SINK_UUID to default reconnect list
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 | 80 +++++++++++++++++++++++++++++++++++++++---------
src/adapter.c | 36 ++++++++++++++++++++++
src/adapter.h | 2 ++
src/main.c | 1 +
src/main.conf | 11 ++++++-
7 files changed, 184 insertions(+), 15 deletions(-)
--
2.28.0.402.g5ffc5be6b7-goog
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).
---
Changes in v3:
- Refactored resume notification to use btd_adapter_driver
- Renamed ReconnectAudioDelay to ResumeDelay and set default to 2
- Added A2DP_SINK_UUID to default reconnect list
Changes in v2:
- Refactored to use policy instead of connecting directly in adapter
plugins/policy.c | 80 +++++++++++++++++++++++++++++++++++++++---------
src/adapter.c | 36 ++++++++++++++++++++++
src/adapter.h | 2 ++
src/main.c | 1 +
src/main.conf | 11 ++++++-
5 files changed, 115 insertions(+), 15 deletions(-)
diff --git a/plugins/policy.c b/plugins/policy.c
index de51e58b9..5ab65b991 100644
--- a/plugins/policy.c
+++ b/plugins/policy.c
@@ -62,10 +62,12 @@ struct reconnect_data {
guint timer;
bool active;
unsigned int attempt;
+ bool on_resume;
};
static const char *default_reconnect[] = {
- HSP_AG_UUID, HFP_AG_UUID, A2DP_SOURCE_UUID, NULL };
+ HSP_AG_UUID, HFP_AG_UUID, A2DP_SOURCE_UUID,
+ A2DP_SINK_UUID, NULL };
static char **reconnect_uuids = NULL;
static const size_t default_attempts = 7;
@@ -75,6 +77,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_resume_delay = 2;
+static int resume_delay;
+
static GSList *reconnects = NULL;
static unsigned int service_id = 0;
@@ -711,6 +716,9 @@ static gboolean reconnect_timeout(gpointer data)
/* Mark the GSource as invalid */
reconnect->timer = 0;
+ /* Mark any reconnect on resume as handled */
+ reconnect->on_resume = false;
+
err = btd_device_connect_services(reconnect->dev, reconnect->services);
if (err < 0) {
error("Reconnecting services failed: %s (%d)",
@@ -724,14 +732,17 @@ static gboolean reconnect_timeout(gpointer data)
return FALSE;
}
-static void reconnect_set_timer(struct reconnect_data *reconnect)
+static void reconnect_set_timer(struct reconnect_data *reconnect, int timeout)
{
- static int timeout = 0;
+ static int interval_timeout = 0;
reconnect->active = true;
if (reconnect->attempt < reconnect_intervals_len)
- timeout = reconnect_intervals[reconnect->attempt];
+ interval_timeout = reconnect_intervals[reconnect->attempt];
+
+ if (timeout < 0)
+ timeout = interval_timeout;
DBG("attempt %u/%zu %d seconds", reconnect->attempt + 1,
reconnect_attempts, timeout);
@@ -743,10 +754,14 @@ 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;
DBG("reason %u", reason);
- if (reason != MGMT_DEV_DISCONN_TIMEOUT)
+ /* Only attempt reconnect for the following reasons */
+ if (reason != MGMT_DEV_DISCONN_TIMEOUT &&
+ reason != MGMT_DEV_DISCONN_LOCAL_HOST_SUSPEND)
return;
reconnect = reconnect_find(dev);
@@ -755,10 +770,40 @@ static void disconnect_cb(struct btd_device *dev, uint8_t reason)
reconnect_reset(reconnect);
- DBG("Device %s identified for auto-reconnection",
- device_get_path(dev));
+ DBG("Device %s identified for auto-reconnection", device_get_path(dev));
- reconnect_set_timer(reconnect);
+ switch(reason)
+ {
+ case MGMT_DEV_DISCONN_LOCAL_HOST_SUSPEND:
+ if (btd_device_get_service(dev, A2DP_SINK_UUID)) {
+ reconnect->on_resume = true;
+ DBG("%s configured to reconnect on resume",
+ device_get_path(dev));
+ }
+ break;
+ case MGMT_DEV_DISCONN_TIMEOUT:
+ reconnect_set_timer(reconnect, -1);
+ break;
+ default:
+ DBG("Developer error. Reason = %d", reason);
+ break;
+ }
+}
+
+static void policy_adapter_resume(struct btd_adapter *adapter,
+ uint8_t wake_reason, const bdaddr_t *addr,
+ const uint8_t addr_type)
+{
+ GSList *l;
+
+ /* Check if any devices needed to be reconnected on resume */
+ for (l = reconnects; l; l = g_slist_next(l)) {
+ struct reconnect_data *reconnect = l->data;
+
+ if (reconnect->on_resume) {
+ reconnect_set_timer(reconnect, resume_delay);
+ }
+ }
}
static void conn_fail_cb(struct btd_device *dev, uint8_t status)
@@ -786,14 +831,15 @@ static void conn_fail_cb(struct btd_device *dev, uint8_t status)
return;
}
- reconnect_set_timer(reconnect);
+ reconnect_set_timer(reconnect, -1);
}
static int policy_adapter_probe(struct btd_adapter *adapter)
{
DBG("");
- btd_adapter_restore_powered(adapter);
+ if (auto_enable)
+ btd_adapter_restore_powered(adapter);
return 0;
}
@@ -801,6 +847,7 @@ static int policy_adapter_probe(struct btd_adapter *adapter)
static struct btd_adapter_driver policy_driver = {
.name = "policy",
.probe = policy_adapter_probe,
+ .resume = policy_adapter_resume,
};
static int policy_init(void)
@@ -854,14 +901,20 @@ static int policy_init(void)
auto_enable = g_key_file_get_boolean(conf, "Policy", "AutoEnable",
NULL);
+ resume_delay = g_key_file_get_integer(
+ conf, "Policy", "ResumeDelay", &gerr);
+
+ if (gerr) {
+ g_clear_error(&gerr);
+ resume_delay = default_resume_delay;
+ }
done:
if (reconnect_uuids && reconnect_uuids[0] && reconnect_attempts) {
btd_add_disconnect_cb(disconnect_cb);
btd_add_conn_fail_cb(conn_fail_cb);
}
- if (auto_enable)
- btd_register_adapter_driver(&policy_driver);
+ btd_register_adapter_driver(&policy_driver);
return 0;
}
@@ -882,8 +935,7 @@ static void policy_exit(void)
btd_service_remove_state_cb(service_id);
- if (auto_enable)
- btd_unregister_adapter_driver(&policy_driver);
+ btd_unregister_adapter_driver(&policy_driver);
}
BLUETOOTH_PLUGIN_DEFINE(policy, VERSION, BLUETOOTH_PLUGIN_PRIORITY_DEFAULT,
diff --git a/src/adapter.c b/src/adapter.c
index 1435e2bd7..bf355bfa7 100644
--- a/src/adapter.c
+++ b/src/adapter.c
@@ -8776,6 +8776,37 @@ static void connected_callback(uint16_t index, uint16_t length,
eir_data_free(&eir_data);
}
+static void controller_resume_notify(struct btd_adapter *adapter,
+ const uint8_t wake_reason,
+ const bdaddr_t *addr,
+ const uint8_t addr_type)
+{
+ GSList *l;
+
+ for (l = adapter->drivers; l; l = g_slist_next(l)) {
+ struct btd_adapter_driver *driver = l->data;
+ if (driver->resume)
+ driver->resume(adapter, 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;
+ }
+
+ info("Controller resume with wake event 0x%x", ev->wake_reason);
+
+ controller_resume_notify(adapter, ev->wake_reason, &ev->addr.bdaddr,
+ ev->addr.type);
+}
+
static void device_blocked_callback(uint16_t index, uint16_t length,
const void *param, void *user_data)
{
@@ -9399,6 +9430,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..b81ef6a04 100644
--- a/src/adapter.h
+++ b/src/adapter.h
@@ -113,6 +113,8 @@ struct btd_adapter_driver {
const char *name;
int (*probe) (struct btd_adapter *adapter);
void (*remove) (struct btd_adapter *adapter);
+ void (*resume)(struct btd_adapter *adapter, uint8_t wake_reason,
+ const bdaddr_t *addr, const uint8_t addr_type);
};
typedef void (*service_auth_cb) (DBusError *derr, void *user_data);
diff --git a/src/main.c b/src/main.c
index b37c32948..038f867b5 100644
--- a/src/main.c
+++ b/src/main.c
@@ -131,6 +131,7 @@ static const char *policy_options[] = {
"ReconnectAttempts",
"ReconnectIntervals",
"AutoEnable",
+ "ResumeDelay",
NULL
};
diff --git a/src/main.conf b/src/main.conf
index 42f7e41c5..9f882e65a 100644
--- a/src/main.conf
+++ b/src/main.conf
@@ -186,7 +186,7 @@
# timeout). The policy plugin should contain a sane set of values by
# default, but this list can be overridden here. By setting the list to
# empty the reconnection feature gets disabled.
-#ReconnectUUIDs=00001112-0000-1000-8000-00805f9b34fb,0000111f-0000-1000-8000-00805f9b34fb,0000110a-0000-1000-8000-00805f9b34fb
+#ReconnectUUIDs=00001112-0000-1000-8000-00805f9b34fb,0000111f-0000-1000-8000-00805f9b34fb,0000110a-0000-1000-8000-00805f9b34fb,0000110b-0000-1000-8000-00805f9b34fb
# ReconnectAttempts define the number of attempts to reconnect after a link
# lost. Setting the value to 0 disables reconnecting feature.
@@ -202,3 +202,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. ResumeDelay determines the delay between when the controller
+# resumes from suspend and a connection attempt is made. A longer delay is
+# better for better co-existence with Wi-Fi.
+# The value is in seconds.
+# Default: 2
+#ResumeDelay = 2
+
--
2.28.0.402.g5ffc5be6b7-goog
Hi Abhishek,
On Fri, Aug 28, 2020 at 6:02 PM Abhishek Pandit-Subedi
<[email protected]> 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).
>
> ---
>
> Changes in v3:
> - Refactored resume notification to use btd_adapter_driver
> - Renamed ReconnectAudioDelay to ResumeDelay and set default to 2
> - Added A2DP_SINK_UUID to default reconnect list
>
> Changes in v2:
> - Refactored to use policy instead of connecting directly in adapter
>
> plugins/policy.c | 80 +++++++++++++++++++++++++++++++++++++++---------
> src/adapter.c | 36 ++++++++++++++++++++++
> src/adapter.h | 2 ++
> src/main.c | 1 +
> src/main.conf | 11 ++++++-
> 5 files changed, 115 insertions(+), 15 deletions(-)
>
> diff --git a/plugins/policy.c b/plugins/policy.c
> index de51e58b9..5ab65b991 100644
> --- a/plugins/policy.c
> +++ b/plugins/policy.c
> @@ -62,10 +62,12 @@ struct reconnect_data {
> guint timer;
> bool active;
> unsigned int attempt;
> + bool on_resume;
> };
>
> static const char *default_reconnect[] = {
> - HSP_AG_UUID, HFP_AG_UUID, A2DP_SOURCE_UUID, NULL };
> + HSP_AG_UUID, HFP_AG_UUID, A2DP_SOURCE_UUID,
> + A2DP_SINK_UUID, NULL };
Lets have this change in a separate patch.
> static char **reconnect_uuids = NULL;
>
> static const size_t default_attempts = 7;
> @@ -75,6 +77,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_resume_delay = 2;
> +static int resume_delay;
> +
> static GSList *reconnects = NULL;
>
> static unsigned int service_id = 0;
> @@ -711,6 +716,9 @@ static gboolean reconnect_timeout(gpointer data)
> /* Mark the GSource as invalid */
> reconnect->timer = 0;
>
> + /* Mark any reconnect on resume as handled */
> + reconnect->on_resume = false;
> +
> err = btd_device_connect_services(reconnect->dev, reconnect->services);
> if (err < 0) {
> error("Reconnecting services failed: %s (%d)",
> @@ -724,14 +732,17 @@ static gboolean reconnect_timeout(gpointer data)
> return FALSE;
> }
>
> -static void reconnect_set_timer(struct reconnect_data *reconnect)
> +static void reconnect_set_timer(struct reconnect_data *reconnect, int timeout)
> {
> - static int timeout = 0;
> + static int interval_timeout = 0;
>
> reconnect->active = true;
>
> if (reconnect->attempt < reconnect_intervals_len)
> - timeout = reconnect_intervals[reconnect->attempt];
> + interval_timeout = reconnect_intervals[reconnect->attempt];
> +
> + if (timeout < 0)
> + timeout = interval_timeout;
>
> DBG("attempt %u/%zu %d seconds", reconnect->attempt + 1,
> reconnect_attempts, timeout);
> @@ -743,10 +754,14 @@ 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;
>
> DBG("reason %u", reason);
>
> - if (reason != MGMT_DEV_DISCONN_TIMEOUT)
> + /* Only attempt reconnect for the following reasons */
> + if (reason != MGMT_DEV_DISCONN_TIMEOUT &&
> + reason != MGMT_DEV_DISCONN_LOCAL_HOST_SUSPEND)
We might want to rework this to have the reason just checked once.
> return;
>
> reconnect = reconnect_find(dev);
> @@ -755,10 +770,40 @@ static void disconnect_cb(struct btd_device *dev, uint8_t reason)
>
> reconnect_reset(reconnect);
>
> - DBG("Device %s identified for auto-reconnection",
> - device_get_path(dev));
> + DBG("Device %s identified for auto-reconnection", device_get_path(dev));
>
> - reconnect_set_timer(reconnect);
> + switch(reason)
> + {
> + case MGMT_DEV_DISCONN_LOCAL_HOST_SUSPEND:
> + if (btd_device_get_service(dev, A2DP_SINK_UUID)) {
> + reconnect->on_resume = true;
> + DBG("%s configured to reconnect on resume",
> + device_get_path(dev));
I thought your idea was to set the timer here and then reset it
on_resume so it would work even when the kernel does not indicate the
resume with an event.
> + }
> + break;
> + case MGMT_DEV_DISCONN_TIMEOUT:
> + reconnect_set_timer(reconnect, -1);
> + break;
> + default:
> + DBG("Developer error. Reason = %d", reason);
> + break;
> + }
> +}
> +
> +static void policy_adapter_resume(struct btd_adapter *adapter,
> + uint8_t wake_reason, const bdaddr_t *addr,
> + const uint8_t addr_type)
> +{
> + GSList *l;
> +
> + /* Check if any devices needed to be reconnected on resume */
> + for (l = reconnects; l; l = g_slist_next(l)) {
> + struct reconnect_data *reconnect = l->data;
We should be checking if the reconnect matches the adapter otherwise
this could potentially call reconnect_set_timer multiple times for the
same device if there are multiple adapters on the system.
> + if (reconnect->on_resume) {
> + reconnect_set_timer(reconnect, resume_delay);
> + }
> + }
> }
>
> static void conn_fail_cb(struct btd_device *dev, uint8_t status)
> @@ -786,14 +831,15 @@ static void conn_fail_cb(struct btd_device *dev, uint8_t status)
> return;
> }
>
> - reconnect_set_timer(reconnect);
> + reconnect_set_timer(reconnect, -1);
> }
>
> static int policy_adapter_probe(struct btd_adapter *adapter)
> {
> DBG("");
>
> - btd_adapter_restore_powered(adapter);
> + if (auto_enable)
> + btd_adapter_restore_powered(adapter);
>
> return 0;
> }
> @@ -801,6 +847,7 @@ static int policy_adapter_probe(struct btd_adapter *adapter)
> static struct btd_adapter_driver policy_driver = {
> .name = "policy",
> .probe = policy_adapter_probe,
> + .resume = policy_adapter_resume,
> };
>
> static int policy_init(void)
> @@ -854,14 +901,20 @@ static int policy_init(void)
> auto_enable = g_key_file_get_boolean(conf, "Policy", "AutoEnable",
> NULL);
>
> + resume_delay = g_key_file_get_integer(
> + conf, "Policy", "ResumeDelay", &gerr);
> +
> + if (gerr) {
> + g_clear_error(&gerr);
> + resume_delay = default_resume_delay;
> + }
> done:
> if (reconnect_uuids && reconnect_uuids[0] && reconnect_attempts) {
> btd_add_disconnect_cb(disconnect_cb);
> btd_add_conn_fail_cb(conn_fail_cb);
> }
>
> - if (auto_enable)
> - btd_register_adapter_driver(&policy_driver);
> + btd_register_adapter_driver(&policy_driver);
>
> return 0;
> }
> @@ -882,8 +935,7 @@ static void policy_exit(void)
>
> btd_service_remove_state_cb(service_id);
>
> - if (auto_enable)
> - btd_unregister_adapter_driver(&policy_driver);
> + btd_unregister_adapter_driver(&policy_driver);
> }
>
> BLUETOOTH_PLUGIN_DEFINE(policy, VERSION, BLUETOOTH_PLUGIN_PRIORITY_DEFAULT,
> diff --git a/src/adapter.c b/src/adapter.c
> index 1435e2bd7..bf355bfa7 100644
> --- a/src/adapter.c
> +++ b/src/adapter.c
> @@ -8776,6 +8776,37 @@ static void connected_callback(uint16_t index, uint16_t length,
> eir_data_free(&eir_data);
> }
>
> +static void controller_resume_notify(struct btd_adapter *adapter,
> + const uint8_t wake_reason,
> + const bdaddr_t *addr,
> + const uint8_t addr_type)
> +{
> + GSList *l;
> +
> + for (l = adapter->drivers; l; l = g_slist_next(l)) {
> + struct btd_adapter_driver *driver = l->data;
> + if (driver->resume)
> + driver->resume(adapter, wake_reason, addr, addr_type);
This should be in a separate patch, also not sure what is the reason
to pass the address and addr_type on the resume callback? If the
callback requires that it can query it internally.
> + }
> +}
> +
> +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;
> + }
> +
> + info("Controller resume with wake event 0x%x", ev->wake_reason);
> +
> + controller_resume_notify(adapter, ev->wake_reason, &ev->addr.bdaddr,
> + ev->addr.type);
> +}
> +
> static void device_blocked_callback(uint16_t index, uint16_t length,
> const void *param, void *user_data)
> {
> @@ -9399,6 +9430,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..b81ef6a04 100644
> --- a/src/adapter.h
> +++ b/src/adapter.h
> @@ -113,6 +113,8 @@ struct btd_adapter_driver {
> const char *name;
> int (*probe) (struct btd_adapter *adapter);
> void (*remove) (struct btd_adapter *adapter);
> + void (*resume)(struct btd_adapter *adapter, uint8_t wake_reason,
> + const bdaddr_t *addr, const uint8_t addr_type);
> };
>
> typedef void (*service_auth_cb) (DBusError *derr, void *user_data);
> diff --git a/src/main.c b/src/main.c
> index b37c32948..038f867b5 100644
> --- a/src/main.c
> +++ b/src/main.c
> @@ -131,6 +131,7 @@ static const char *policy_options[] = {
> "ReconnectAttempts",
> "ReconnectIntervals",
> "AutoEnable",
> + "ResumeDelay",
> NULL
> };
>
> diff --git a/src/main.conf b/src/main.conf
> index 42f7e41c5..9f882e65a 100644
> --- a/src/main.conf
> +++ b/src/main.conf
> @@ -186,7 +186,7 @@
> # timeout). The policy plugin should contain a sane set of values by
> # default, but this list can be overridden here. By setting the list to
> # empty the reconnection feature gets disabled.
> -#ReconnectUUIDs=00001112-0000-1000-8000-00805f9b34fb,0000111f-0000-1000-8000-00805f9b34fb,0000110a-0000-1000-8000-00805f9b34fb
> +#ReconnectUUIDs=00001112-0000-1000-8000-00805f9b34fb,0000111f-0000-1000-8000-00805f9b34fb,0000110a-0000-1000-8000-00805f9b34fb,0000110b-0000-1000-8000-00805f9b34fb
>
> # ReconnectAttempts define the number of attempts to reconnect after a link
> # lost. Setting the value to 0 disables reconnecting feature.
> @@ -202,3 +202,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. ResumeDelay determines the delay between when the controller
> +# resumes from suspend and a connection attempt is made. A longer delay is
> +# better for better co-existence with Wi-Fi.
> +# The value is in seconds.
> +# Default: 2
> +#ResumeDelay = 2
> +
> --
> 2.28.0.402.g5ffc5be6b7-goog
>
--
Luiz Augusto von Dentz
Hi Luiz,
On Mon, Aug 31, 2020 at 9:57 AM Luiz Augusto von Dentz
<[email protected]> wrote:
>
> Hi Abhishek,
>
> On Fri, Aug 28, 2020 at 6:02 PM Abhishek Pandit-Subedi
> <[email protected]> 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).
> >
> > ---
> >
> > Changes in v3:
> > - Refactored resume notification to use btd_adapter_driver
> > - Renamed ReconnectAudioDelay to ResumeDelay and set default to 2
> > - Added A2DP_SINK_UUID to default reconnect list
> >
> > Changes in v2:
> > - Refactored to use policy instead of connecting directly in adapter
> >
> > plugins/policy.c | 80 +++++++++++++++++++++++++++++++++++++++---------
> > src/adapter.c | 36 ++++++++++++++++++++++
> > src/adapter.h | 2 ++
> > src/main.c | 1 +
> > src/main.conf | 11 ++++++-
> > 5 files changed, 115 insertions(+), 15 deletions(-)
> >
> > diff --git a/plugins/policy.c b/plugins/policy.c
> > index de51e58b9..5ab65b991 100644
> > --- a/plugins/policy.c
> > +++ b/plugins/policy.c
> > @@ -62,10 +62,12 @@ struct reconnect_data {
> > guint timer;
> > bool active;
> > unsigned int attempt;
> > + bool on_resume;
> > };
> >
> > static const char *default_reconnect[] = {
> > - HSP_AG_UUID, HFP_AG_UUID, A2DP_SOURCE_UUID, NULL };
> > + HSP_AG_UUID, HFP_AG_UUID, A2DP_SOURCE_UUID,
> > + A2DP_SINK_UUID, NULL };
>
> Lets have this change in a separate patch.
Ack. Will split into its own patch.
>
> > static char **reconnect_uuids = NULL;
> >
> > static const size_t default_attempts = 7;
> > @@ -75,6 +77,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_resume_delay = 2;
> > +static int resume_delay;
> > +
> > static GSList *reconnects = NULL;
> >
> > static unsigned int service_id = 0;
> > @@ -711,6 +716,9 @@ static gboolean reconnect_timeout(gpointer data)
> > /* Mark the GSource as invalid */
> > reconnect->timer = 0;
> >
> > + /* Mark any reconnect on resume as handled */
> > + reconnect->on_resume = false;
> > +
> > err = btd_device_connect_services(reconnect->dev, reconnect->services);
> > if (err < 0) {
> > error("Reconnecting services failed: %s (%d)",
> > @@ -724,14 +732,17 @@ static gboolean reconnect_timeout(gpointer data)
> > return FALSE;
> > }
> >
> > -static void reconnect_set_timer(struct reconnect_data *reconnect)
> > +static void reconnect_set_timer(struct reconnect_data *reconnect, int timeout)
> > {
> > - static int timeout = 0;
> > + static int interval_timeout = 0;
> >
> > reconnect->active = true;
> >
> > if (reconnect->attempt < reconnect_intervals_len)
> > - timeout = reconnect_intervals[reconnect->attempt];
> > + interval_timeout = reconnect_intervals[reconnect->attempt];
> > +
> > + if (timeout < 0)
> > + timeout = interval_timeout;
> >
> > DBG("attempt %u/%zu %d seconds", reconnect->attempt + 1,
> > reconnect_attempts, timeout);
> > @@ -743,10 +754,14 @@ 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;
> >
> > DBG("reason %u", reason);
> >
> > - if (reason != MGMT_DEV_DISCONN_TIMEOUT)
> > + /* Only attempt reconnect for the following reasons */
> > + if (reason != MGMT_DEV_DISCONN_TIMEOUT &&
> > + reason != MGMT_DEV_DISCONN_LOCAL_HOST_SUSPEND)
>
> We might want to rework this to have the reason just checked once.
With a single check, it was either a lot less clear (break vs return
in switch) or had duplication. This was the clearer option.
i.e.
switch(reason) {
case MGMT_DEV_DISCONN_TIMEOUT:
break;
case MGMT_DEV_DISCONN_LOCAL_HOST_SUSPEND:
if (btd_device_get_service(dev, A2DP_SINK_UUID))
break;
else
return;
default:
return;
}
/* Do the rest of the reconnect */
>
> > return;
> >
> > reconnect = reconnect_find(dev);
> > @@ -755,10 +770,40 @@ static void disconnect_cb(struct btd_device *dev, uint8_t reason)
> >
> > reconnect_reset(reconnect);
> >
> > - DBG("Device %s identified for auto-reconnection",
> > - device_get_path(dev));
> > + DBG("Device %s identified for auto-reconnection", device_get_path(dev));
> >
> > - reconnect_set_timer(reconnect);
> > + switch(reason)
> > + {
> > + case MGMT_DEV_DISCONN_LOCAL_HOST_SUSPEND:
> > + if (btd_device_get_service(dev, A2DP_SINK_UUID)) {
> > + reconnect->on_resume = true;
> > + DBG("%s configured to reconnect on resume",
> > + device_get_path(dev));
>
> I thought your idea was to set the timer here and then reset it
> on_resume so it would work even when the kernel does not indicate the
> resume with an event.
I sent an email on the last thread about this. With a reconnect time
of 2s (and wake time = 1s, suspend time = 10s), if we stage the timer
on disconnect, it seems to run immediately on resume without waiting
to be delayed. This breaks the ability of the resume notifier and dark
resume notifier to delay or stop the reconnection.
This disconnect callback can run before the system suspends (since
userspace isn't frozen when the kernel suspend notifier runs).
Depending on where the disconnect_cb runs, this results in incorrect
behavior of reconnection on resume (i.e. no delays).
To support the case where the kernel doesn't emit a kernel event, I
think it might be better for me to check whether the kernel supports
these events in the btd_adapter_driver probe function and queue on
disconnect if it doesn't. Expect this in the next version.
>
> > + }
> > + break;
> > + case MGMT_DEV_DISCONN_TIMEOUT:
> > + reconnect_set_timer(reconnect, -1);
> > + break;
> > + default:
> > + DBG("Developer error. Reason = %d", reason);
> > + break;
> > + }
> > +}
> > +
> > +static void policy_adapter_resume(struct btd_adapter *adapter,
> > + uint8_t wake_reason, const bdaddr_t *addr,
> > + const uint8_t addr_type)
> > +{
> > + GSList *l;
> > +
> > + /* Check if any devices needed to be reconnected on resume */
> > + for (l = reconnects; l; l = g_slist_next(l)) {
> > + struct reconnect_data *reconnect = l->data;
>
> We should be checking if the reconnect matches the adapter otherwise
> this could potentially call reconnect_set_timer multiple times for the
> same device if there are multiple adapters on the system.
Ack -- will do for the next version.
>
> > + if (reconnect->on_resume) {
> > + reconnect_set_timer(reconnect, resume_delay);
> > + }
> > + }
> > }
> >
> > static void conn_fail_cb(struct btd_device *dev, uint8_t status)
> > @@ -786,14 +831,15 @@ static void conn_fail_cb(struct btd_device *dev, uint8_t status)
> > return;
> > }
> >
> > - reconnect_set_timer(reconnect);
> > + reconnect_set_timer(reconnect, -1);
> > }
> >
> > static int policy_adapter_probe(struct btd_adapter *adapter)
> > {
> > DBG("");
> >
> > - btd_adapter_restore_powered(adapter);
> > + if (auto_enable)
> > + btd_adapter_restore_powered(adapter);
> >
> > return 0;
> > }
> > @@ -801,6 +847,7 @@ static int policy_adapter_probe(struct btd_adapter *adapter)
> > static struct btd_adapter_driver policy_driver = {
> > .name = "policy",
> > .probe = policy_adapter_probe,
> > + .resume = policy_adapter_resume,
> > };
> >
> > static int policy_init(void)
> > @@ -854,14 +901,20 @@ static int policy_init(void)
> > auto_enable = g_key_file_get_boolean(conf, "Policy", "AutoEnable",
> > NULL);
> >
> > + resume_delay = g_key_file_get_integer(
> > + conf, "Policy", "ResumeDelay", &gerr);
> > +
> > + if (gerr) {
> > + g_clear_error(&gerr);
> > + resume_delay = default_resume_delay;
> > + }
> > done:
> > if (reconnect_uuids && reconnect_uuids[0] && reconnect_attempts) {
> > btd_add_disconnect_cb(disconnect_cb);
> > btd_add_conn_fail_cb(conn_fail_cb);
> > }
> >
> > - if (auto_enable)
> > - btd_register_adapter_driver(&policy_driver);
> > + btd_register_adapter_driver(&policy_driver);
> >
> > return 0;
> > }
> > @@ -882,8 +935,7 @@ static void policy_exit(void)
> >
> > btd_service_remove_state_cb(service_id);
> >
> > - if (auto_enable)
> > - btd_unregister_adapter_driver(&policy_driver);
> > + btd_unregister_adapter_driver(&policy_driver);
> > }
> >
> > BLUETOOTH_PLUGIN_DEFINE(policy, VERSION, BLUETOOTH_PLUGIN_PRIORITY_DEFAULT,
> > diff --git a/src/adapter.c b/src/adapter.c
> > index 1435e2bd7..bf355bfa7 100644
> > --- a/src/adapter.c
> > +++ b/src/adapter.c
> > @@ -8776,6 +8776,37 @@ static void connected_callback(uint16_t index, uint16_t length,
> > eir_data_free(&eir_data);
> > }
> >
> > +static void controller_resume_notify(struct btd_adapter *adapter,
> > + const uint8_t wake_reason,
> > + const bdaddr_t *addr,
> > + const uint8_t addr_type)
> > +{
> > + GSList *l;
> > +
> > + for (l = adapter->drivers; l; l = g_slist_next(l)) {
> > + struct btd_adapter_driver *driver = l->data;
> > + if (driver->resume)
> > + driver->resume(adapter, wake_reason, addr, addr_type);
>
> This should be in a separate patch, also not sure what is the reason
> to pass the address and addr_type on the resume callback? If the
> callback requires that it can query it internally.
>
Ack. Will move resume notification changes into its own patch and
remove the unneeded params.
> > + }
> > +}
> > +
> > +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;
> > + }
> > +
> > + info("Controller resume with wake event 0x%x", ev->wake_reason);
> > +
> > + controller_resume_notify(adapter, ev->wake_reason, &ev->addr.bdaddr,
> > + ev->addr.type);
> > +}
> > +
> > static void device_blocked_callback(uint16_t index, uint16_t length,
> > const void *param, void *user_data)
> > {
> > @@ -9399,6 +9430,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..b81ef6a04 100644
> > --- a/src/adapter.h
> > +++ b/src/adapter.h
> > @@ -113,6 +113,8 @@ struct btd_adapter_driver {
> > const char *name;
> > int (*probe) (struct btd_adapter *adapter);
> > void (*remove) (struct btd_adapter *adapter);
> > + void (*resume)(struct btd_adapter *adapter, uint8_t wake_reason,
> > + const bdaddr_t *addr, const uint8_t addr_type);
> > };
> >
> > typedef void (*service_auth_cb) (DBusError *derr, void *user_data);
> > diff --git a/src/main.c b/src/main.c
> > index b37c32948..038f867b5 100644
> > --- a/src/main.c
> > +++ b/src/main.c
> > @@ -131,6 +131,7 @@ static const char *policy_options[] = {
> > "ReconnectAttempts",
> > "ReconnectIntervals",
> > "AutoEnable",
> > + "ResumeDelay",
> > NULL
> > };
> >
> > diff --git a/src/main.conf b/src/main.conf
> > index 42f7e41c5..9f882e65a 100644
> > --- a/src/main.conf
> > +++ b/src/main.conf
> > @@ -186,7 +186,7 @@
> > # timeout). The policy plugin should contain a sane set of values by
> > # default, but this list can be overridden here. By setting the list to
> > # empty the reconnection feature gets disabled.
> > -#ReconnectUUIDs=00001112-0000-1000-8000-00805f9b34fb,0000111f-0000-1000-8000-00805f9b34fb,0000110a-0000-1000-8000-00805f9b34fb
> > +#ReconnectUUIDs=00001112-0000-1000-8000-00805f9b34fb,0000111f-0000-1000-8000-00805f9b34fb,0000110a-0000-1000-8000-00805f9b34fb,0000110b-0000-1000-8000-00805f9b34fb
> >
> > # ReconnectAttempts define the number of attempts to reconnect after a link
> > # lost. Setting the value to 0 disables reconnecting feature.
> > @@ -202,3 +202,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. ResumeDelay determines the delay between when the controller
> > +# resumes from suspend and a connection attempt is made. A longer delay is
> > +# better for better co-existence with Wi-Fi.
> > +# The value is in seconds.
> > +# Default: 2
> > +#ResumeDelay = 2
> > +
> > --
> > 2.28.0.402.g5ffc5be6b7-goog
> >
>
>
> --
> Luiz Augusto von Dentz