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

[RFC Bluez PATCH 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:

 * In the Device Disconnected management event, if the disconnect reason
   was 0x5 (Disconnected by Local Host for Suspend) and the device is an
   audio sink (implements major services Audio + Rendering), then it is
   queued for reconnect.
 * When the Controller Resumed management event is seen, we check if
   an audio device needs to be reconnected. If yes, we queue a delayed
   callback to do the reconnection. The delay is 5s by default and is
   meant to allow sufficient time for any Wi-Fi activity that may occur
   during resume (since Bluetooth connect may adversely affect that).

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 General 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'm hoping to
test this with a few more headsets to make sure this is ok and I'm
looking for some early feedback.

Thanks
Abhishek



Abhishek Pandit-Subedi (3):
  mgmt: Add controller suspend and resume events
  monitor: Add btmon support for Suspend and Resume events
  adapter: Reconnect audio on controller resume

 lib/mgmt.h       | 14 +++++++++
 monitor/packet.c | 55 ++++++++++++++++++++++++++++++++
 src/adapter.c    | 82 ++++++++++++++++++++++++++++++++++++++++++++++++
 src/device.c     | 27 ++++++++++++++++
 src/device.h     |  2 ++
 src/main.conf    |  6 ++++
 6 files changed, 186 insertions(+)

-- 
2.28.0.rc0.142.g3c755180ce-goog

[RFC Bluez PATCH 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>
---

 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.rc0.142.g3c755180ce-goog

[RFC Bluez PATCH 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>
---

 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.rc0.142.g3c755180ce-goog

[RFC Bluez PATCH 3/3] adapter: 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, keep track of the last audio device disconnected
during suspend and try to reconnect when the controller resumes from
suspend.

Reviewed-by: Sonny Sasaka <sonnysasaka@chromium.org>
---
Hey Luiz,

On our internal review, two things stood out in this commit that we
weren't able to come to a consensus on internally:

* Is it better to use the audio device class or should we compare to the
  A2DP, HFP and HSP uuids?
* Constructing the dbus message internally before calling dev_connect
  looks a bit weird. I couldn't figure out how to internally trigger
  this function (since it seems to require a msg to respond to on
  success/failure). Any thoughts?


 src/adapter.c | 82 +++++++++++++++++++++++++++++++++++++++++++++++++++
 src/device.c  | 27 +++++++++++++++++
 src/device.h  |  2 ++
 src/main.conf |  6 ++++
 4 files changed, 117 insertions(+)

diff --git a/src/adapter.c b/src/adapter.c
index 5e896a9f0..b1073c439 100644
--- a/src/adapter.c
+++ b/src/adapter.c
@@ -90,6 +90,7 @@
 #define IDLE_DISCOV_TIMEOUT (5)
 #define TEMP_DEV_TIMEOUT (3 * 60)
 #define BONDING_TIMEOUT (2 * 60)
+#define RECONNECT_AUDIO_DELAY (5)
 
 #define SCAN_TYPE_BREDR (1 << BDADDR_BREDR)
 #define SCAN_TYPE_LE ((1 << BDADDR_LE_PUBLIC) | (1 << BDADDR_LE_RANDOM))
@@ -269,6 +270,15 @@ struct btd_adapter {
 	struct btd_device *connect_le;	/* LE device waiting to be connected */
 	sdp_list_t *services;		/* Services associated to adapter */
 
+	/* audio device to reconnect after resuming from suspend */
+	struct reconnect_audio_info {
+		bdaddr_t addr;
+		uint8_t addr_type;
+		bool reconnect;
+        } reconnect_audio;
+	guint reconnect_audio_timeout;  /* timeout for reconnect on resume */
+	uint32_t reconnect_audio_delay; /* delay reconnect after resume */
+
 	struct btd_gatt_database *database;
 	struct btd_adv_manager *adv_manager;
 
@@ -6256,6 +6266,7 @@ static void load_config(struct btd_adapter *adapter)
 	/* Get discoverable mode */
 	adapter->stored_discoverable = g_key_file_get_boolean(key_file,
 					"General", "Discoverable", &gerr);
+
 	if (gerr) {
 		adapter->stored_discoverable = false;
 		g_error_free(gerr);
@@ -6271,6 +6282,16 @@ static void load_config(struct btd_adapter *adapter)
 		gerr = NULL;
 	}
 
+	/* Get audio reconnect delay */
+	adapter->reconnect_audio_delay = g_key_file_get_integer(
+		key_file, "General", "ReconnectAudioDelay", &gerr);
+
+	if (gerr) {
+		adapter->reconnect_audio_delay = RECONNECT_AUDIO_DELAY;
+		g_error_free(gerr);
+		gerr = NULL;
+	}
+
 	g_key_file_free(key_file);
 }
 
@@ -7820,6 +7841,15 @@ static void dev_disconnected(struct btd_adapter *adapter,
 	if (device) {
 		adapter_remove_connection(adapter, device, addr->type);
 		disconnect_notify(device, reason);
+
+		if (reason == MGMT_DEV_DISCONN_LOCAL_HOST_SUSPEND &&
+		    device_class_is_audio(device)) {
+			adapter->reconnect_audio.reconnect = true;
+			adapter->reconnect_audio.addr_type =
+				btd_device_get_bdaddr_type(device);
+			bacpy(&adapter->reconnect_audio.addr,
+			      device_get_address(device));
+		}
 	}
 
 	bonding_attempt_complete(adapter, &addr->bdaddr, addr->type,
@@ -8766,6 +8796,53 @@ static void connected_callback(uint16_t index, uint16_t length,
 	eir_data_free(&eir_data);
 }
 
+static gboolean reconnect_audio_timeout(gpointer user_data)
+{
+	struct btd_adapter *adapter = user_data;
+
+	adapter->reconnect_audio_timeout = 0;
+
+	if (adapter->reconnect_audio.reconnect) {
+		struct btd_device *dev = btd_adapter_find_device(
+			adapter, &adapter->reconnect_audio.addr,
+			adapter->reconnect_audio.addr_type);
+
+		adapter->reconnect_audio.reconnect = false;
+
+		if (!dev || btd_device_is_connected(dev))
+			return FALSE;
+
+		device_internal_connect(dev);
+	}
+
+	return FALSE;
+}
+
+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);
+
+	if (adapter->reconnect_audio_timeout > 0) {
+		g_source_remove(adapter->reconnect_audio_timeout);
+		adapter->reconnect_audio_timeout = 0;
+	}
+
+	if (adapter->reconnect_audio.reconnect) {
+		adapter->reconnect_audio_timeout =
+			g_timeout_add_seconds(adapter->reconnect_audio_delay,
+					      reconnect_audio_timeout, adapter);
+	}
+}
+
 static void device_blocked_callback(uint16_t index, uint16_t length,
 					const void *param, void *user_data)
 {
@@ -9389,6 +9466,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/device.c b/src/device.c
index bb8e07e8f..8b165ffa4 100644
--- a/src/device.c
+++ b/src/device.c
@@ -747,6 +747,12 @@ gboolean device_is_trusted(struct btd_device *device)
 	return device->trusted;
 }
 
+bool device_class_is_audio(struct btd_device *device)
+{
+	/* Look for major service classes Audio (0x20) + Rendering (0x4) */
+	return ((device->class >> 16) & 0x24) == 0x24;
+}
+
 static gboolean dev_property_get_address(const GDBusPropertyTable *property,
 					DBusMessageIter *iter, void *data)
 {
@@ -6853,6 +6859,27 @@ struct btd_service *btd_device_get_service(struct btd_device *dev,
 	return NULL;
 }
 
+/* Internal function to connect to a device. This fakes the dbus message used to
+ * call the "Connect" api on the device so that the same function can be called
+ * by bluez internally.
+ */
+bool device_internal_connect(struct btd_device *dev)
+{
+	DBusMessage *msg;
+
+	if (!device_is_connectable(dev))
+		return false;
+
+	msg = dbus_message_new_method_call("org.bluez",
+						device_get_path(dev),
+						DEVICE_INTERFACE,
+						"Connect");
+	/* Sending the message usually sets serial. Fake it here. */
+	dbus_message_set_serial(msg, 1);
+
+	dev_connect(dbus_conn, msg, dev);
+}
+
 void btd_device_init(void)
 {
 	dbus_conn = btd_get_dbus_connection();
diff --git a/src/device.h b/src/device.h
index 956fec1ae..82f97b5bd 100644
--- a/src/device.h
+++ b/src/device.h
@@ -98,6 +98,7 @@ bool device_is_connectable(struct btd_device *device);
 bool device_is_paired(struct btd_device *device, uint8_t bdaddr_type);
 bool device_is_bonded(struct btd_device *device, uint8_t bdaddr_type);
 gboolean device_is_trusted(struct btd_device *device);
+bool device_class_is_audio(struct btd_device *device);
 void device_set_paired(struct btd_device *dev, uint8_t bdaddr_type);
 void device_set_unpaired(struct btd_device *dev, uint8_t bdaddr_type);
 void btd_device_set_temporary(struct btd_device *device, bool temporary);
@@ -186,6 +187,7 @@ int btd_device_connect_services(struct btd_device *dev, GSList *services);
 uint32_t btd_device_get_current_flags(struct btd_device *dev);
 void btd_device_flags_changed(struct btd_device *dev, uint32_t supported_flags,
 			      uint32_t current_flags);
+bool device_internal_connect(struct btd_device *dev);
 
 void btd_device_init(void);
 void btd_device_cleanup(void);
diff --git a/src/main.conf b/src/main.conf
index f41203b96..c6bb78a84 100644
--- a/src/main.conf
+++ b/src/main.conf
@@ -82,6 +82,12 @@
 # 0 = disable timer, i.e. never keep temporary devices
 #TemporaryTimeout = 30
 
+# How long to wait after controller resume before reconnecting to last used
+# audio device.
+# The value is in seconds.
+# Default: 5
+#ReconnectAudioDelay = 5
+
 [Controller]
 # The following values are used to load default adapter parameters.  BlueZ loads
 # the values into the kernel before the adapter is powered if the kernel
-- 
2.28.0.rc0.142.g3c755180ce-goog

Re: [RFC Bluez PATCH 3/3] adapter: Reconnect audio on controller resume

[Luiz Augusto von Dentz]

Hi Abhishek,

On Tue, Jul 28, 2020 at 6:55 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, keep track of the last audio device disconnected
> during suspend and try to reconnect when the controller resumes from
> suspend.
>
> Reviewed-by: Sonny Sasaka <sonnysasaka@chromium.org>
> ---
> Hey Luiz,
>
> On our internal review, two things stood out in this commit that we
> weren't able to come to a consensus on internally:
>
> * Is it better to use the audio device class or should we compare to the
>   A2DP, HFP and HSP uuids?
> * Constructing the dbus message internally before calling dev_connect
>   looks a bit weird. I couldn't figure out how to internally trigger
>   this function (since it seems to require a msg to respond to on
>   success/failure). Any thoughts?
>
>
>  src/adapter.c | 82 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  src/device.c  | 27 +++++++++++++++++
>  src/device.h  |  2 ++
>  src/main.conf |  6 ++++
>  4 files changed, 117 insertions(+)
>
> diff --git a/src/adapter.c b/src/adapter.c
> index 5e896a9f0..b1073c439 100644
> --- a/src/adapter.c
> +++ b/src/adapter.c
> @@ -90,6 +90,7 @@
>  #define IDLE_DISCOV_TIMEOUT (5)
>  #define TEMP_DEV_TIMEOUT (3 * 60)
>  #define BONDING_TIMEOUT (2 * 60)
> +#define RECONNECT_AUDIO_DELAY (5)
>
>  #define SCAN_TYPE_BREDR (1 << BDADDR_BREDR)
>  #define SCAN_TYPE_LE ((1 << BDADDR_LE_PUBLIC) | (1 << BDADDR_LE_RANDOM))
> @@ -269,6 +270,15 @@ struct btd_adapter {
>         struct btd_device *connect_le;  /* LE device waiting to be connected */
>         sdp_list_t *services;           /* Services associated to adapter */
>
> +       /* audio device to reconnect after resuming from suspend */
> +       struct reconnect_audio_info {
> +               bdaddr_t addr;
> +               uint8_t addr_type;
> +               bool reconnect;
> +        } reconnect_audio;
> +       guint reconnect_audio_timeout;  /* timeout for reconnect on resume */
> +       uint32_t reconnect_audio_delay; /* delay reconnect after resume */
> +
>         struct btd_gatt_database *database;
>         struct btd_adv_manager *adv_manager;
>
> @@ -6256,6 +6266,7 @@ static void load_config(struct btd_adapter *adapter)
>         /* Get discoverable mode */
>         adapter->stored_discoverable = g_key_file_get_boolean(key_file,
>                                         "General", "Discoverable", &gerr);
> +
>         if (gerr) {
>                 adapter->stored_discoverable = false;
>                 g_error_free(gerr);
> @@ -6271,6 +6282,16 @@ static void load_config(struct btd_adapter *adapter)
>                 gerr = NULL;
>         }
>
> +       /* Get audio reconnect delay */
> +       adapter->reconnect_audio_delay = g_key_file_get_integer(
> +               key_file, "General", "ReconnectAudioDelay", &gerr);
> +
> +       if (gerr) {
> +               adapter->reconnect_audio_delay = RECONNECT_AUDIO_DELAY;
> +               g_error_free(gerr);
> +               gerr = NULL;
> +       }
> +
>         g_key_file_free(key_file);
>  }
>
> @@ -7820,6 +7841,15 @@ static void dev_disconnected(struct btd_adapter *adapter,
>         if (device) {
>                 adapter_remove_connection(adapter, device, addr->type);
>                 disconnect_notify(device, reason);
> +
> +               if (reason == MGMT_DEV_DISCONN_LOCAL_HOST_SUSPEND &&
> +                   device_class_is_audio(device)) {
> +                       adapter->reconnect_audio.reconnect = true;
> +                       adapter->reconnect_audio.addr_type =
> +                               btd_device_get_bdaddr_type(device);
> +                       bacpy(&adapter->reconnect_audio.addr,
> +                             device_get_address(device));
> +               }
>         }
>
>         bonding_attempt_complete(adapter, &addr->bdaddr, addr->type,
> @@ -8766,6 +8796,53 @@ static void connected_callback(uint16_t index, uint16_t length,
>         eir_data_free(&eir_data);
>  }
>
> +static gboolean reconnect_audio_timeout(gpointer user_data)
> +{
> +       struct btd_adapter *adapter = user_data;
> +
> +       adapter->reconnect_audio_timeout = 0;
> +
> +       if (adapter->reconnect_audio.reconnect) {
> +               struct btd_device *dev = btd_adapter_find_device(
> +                       adapter, &adapter->reconnect_audio.addr,
> +                       adapter->reconnect_audio.addr_type);
> +
> +               adapter->reconnect_audio.reconnect = false;
> +
> +               if (!dev || btd_device_is_connected(dev))
> +                       return FALSE;
> +
> +               device_internal_connect(dev);
> +       }
> +
> +       return FALSE;
> +}
> +
> +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);
> +
> +       if (adapter->reconnect_audio_timeout > 0) {
> +               g_source_remove(adapter->reconnect_audio_timeout);
> +               adapter->reconnect_audio_timeout = 0;
> +       }
> +
> +       if (adapter->reconnect_audio.reconnect) {
> +               adapter->reconnect_audio_timeout =
> +                       g_timeout_add_seconds(adapter->reconnect_audio_delay,
> +                                             reconnect_audio_timeout, adapter);
> +       }
> +}
> +
>  static void device_blocked_callback(uint16_t index, uint16_t length,
>                                         const void *param, void *user_data)
>  {
> @@ -9389,6 +9466,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/device.c b/src/device.c
> index bb8e07e8f..8b165ffa4 100644
> --- a/src/device.c
> +++ b/src/device.c
> @@ -747,6 +747,12 @@ gboolean device_is_trusted(struct btd_device *device)
>         return device->trusted;
>  }
>
> +bool device_class_is_audio(struct btd_device *device)
> +{
> +       /* Look for major service classes Audio (0x20) + Rendering (0x4) */
> +       return ((device->class >> 16) & 0x24) == 0x24;
> +}
> +
>  static gboolean dev_property_get_address(const GDBusPropertyTable *property,
>                                         DBusMessageIter *iter, void *data)
>  {
> @@ -6853,6 +6859,27 @@ struct btd_service *btd_device_get_service(struct btd_device *dev,
>         return NULL;
>  }
>
> +/* Internal function to connect to a device. This fakes the dbus message used to
> + * call the "Connect" api on the device so that the same function can be called
> + * by bluez internally.
> + */
> +bool device_internal_connect(struct btd_device *dev)
> +{
> +       DBusMessage *msg;
> +
> +       if (!device_is_connectable(dev))
> +               return false;
> +
> +       msg = dbus_message_new_method_call("org.bluez",
> +                                               device_get_path(dev),
> +                                               DEVICE_INTERFACE,
> +                                               "Connect");
> +       /* Sending the message usually sets serial. Fake it here. */
> +       dbus_message_set_serial(msg, 1);
> +
> +       dev_connect(dbus_conn, msg, dev);
> +}
> +
>  void btd_device_init(void)
>  {
>         dbus_conn = btd_get_dbus_connection();
> diff --git a/src/device.h b/src/device.h
> index 956fec1ae..82f97b5bd 100644
> --- a/src/device.h
> +++ b/src/device.h
> @@ -98,6 +98,7 @@ bool device_is_connectable(struct btd_device *device);
>  bool device_is_paired(struct btd_device *device, uint8_t bdaddr_type);
>  bool device_is_bonded(struct btd_device *device, uint8_t bdaddr_type);
>  gboolean device_is_trusted(struct btd_device *device);
> +bool device_class_is_audio(struct btd_device *device);
>  void device_set_paired(struct btd_device *dev, uint8_t bdaddr_type);
>  void device_set_unpaired(struct btd_device *dev, uint8_t bdaddr_type);
>  void btd_device_set_temporary(struct btd_device *device, bool temporary);
> @@ -186,6 +187,7 @@ int btd_device_connect_services(struct btd_device *dev, GSList *services);
>  uint32_t btd_device_get_current_flags(struct btd_device *dev);
>  void btd_device_flags_changed(struct btd_device *dev, uint32_t supported_flags,
>                               uint32_t current_flags);
> +bool device_internal_connect(struct btd_device *dev);
>
>  void btd_device_init(void);
>  void btd_device_cleanup(void);
> diff --git a/src/main.conf b/src/main.conf
> index f41203b96..c6bb78a84 100644
> --- a/src/main.conf
> +++ b/src/main.conf
> @@ -82,6 +82,12 @@
>  # 0 = disable timer, i.e. never keep temporary devices
>  #TemporaryTimeout = 30
>
> +# How long to wait after controller resume before reconnecting to last used
> +# audio device.
> +# The value is in seconds.
> +# Default: 5
> +#ReconnectAudioDelay = 5
> +
>  [Controller]
>  # The following values are used to load default adapter parameters.  BlueZ loads
>  # the values into the kernel before the adapter is powered if the kernel
> --
> 2.28.0.rc0.142.g3c755180ce-goog

Usually connection policy is handled by the policy plugin since there
may be platforms that want implement their own connection policies on
top of bluetoothd so they can just disable the policy plugin, iirc we
do have reconnection policies for unexpected disconnect which should
probably be used in the event a suspend actually trigger a
disconnection see:

https://git.kernel.org/pub/scm/bluetooth/bluez.git/tree/plugins/policy.c#n761

We might need a reason code to indicate to the policy when a
disconnect happens due to suspend logic.

-- 
Luiz Augusto von Dentz

Re: [RFC Bluez PATCH 3/3] adapter: Reconnect audio on controller resume

[Abhishek Pandit-Subedi]

Hi Luiz,

On Tue, Aug 4, 2020 at 11:54 AM Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
>
> Hi Abhishek,
>
> On Tue, Jul 28, 2020 at 6:55 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, keep track of the last audio device disconnected
> > during suspend and try to reconnect when the controller resumes from
> > suspend.
> >
> > Reviewed-by: Sonny Sasaka <sonnysasaka@chromium.org>
> > ---
> > Hey Luiz,
> >
> > On our internal review, two things stood out in this commit that we
> > weren't able to come to a consensus on internally:
> >
> > * Is it better to use the audio device class or should we compare to the
> >   A2DP, HFP and HSP uuids?
> > * Constructing the dbus message internally before calling dev_connect
> >   looks a bit weird. I couldn't figure out how to internally trigger
> >   this function (since it seems to require a msg to respond to on
> >   success/failure). Any thoughts?
> >
> >
> >  src/adapter.c | 82 +++++++++++++++++++++++++++++++++++++++++++++++++++
> >  src/device.c  | 27 +++++++++++++++++
> >  src/device.h  |  2 ++
> >  src/main.conf |  6 ++++
> >  4 files changed, 117 insertions(+)
> >
> > diff --git a/src/adapter.c b/src/adapter.c
> > index 5e896a9f0..b1073c439 100644
> > --- a/src/adapter.c
> > +++ b/src/adapter.c
> > @@ -90,6 +90,7 @@
> >  #define IDLE_DISCOV_TIMEOUT (5)
> >  #define TEMP_DEV_TIMEOUT (3 * 60)
> >  #define BONDING_TIMEOUT (2 * 60)
> > +#define RECONNECT_AUDIO_DELAY (5)
> >
> >  #define SCAN_TYPE_BREDR (1 << BDADDR_BREDR)
> >  #define SCAN_TYPE_LE ((1 << BDADDR_LE_PUBLIC) | (1 << BDADDR_LE_RANDOM))
> > @@ -269,6 +270,15 @@ struct btd_adapter {
> >         struct btd_device *connect_le;  /* LE device waiting to be connected */
> >         sdp_list_t *services;           /* Services associated to adapter */
> >
> > +       /* audio device to reconnect after resuming from suspend */
> > +       struct reconnect_audio_info {
> > +               bdaddr_t addr;
> > +               uint8_t addr_type;
> > +               bool reconnect;
> > +        } reconnect_audio;
> > +       guint reconnect_audio_timeout;  /* timeout for reconnect on resume */
> > +       uint32_t reconnect_audio_delay; /* delay reconnect after resume */
> > +
> >         struct btd_gatt_database *database;
> >         struct btd_adv_manager *adv_manager;
> >
> > @@ -6256,6 +6266,7 @@ static void load_config(struct btd_adapter *adapter)
> >         /* Get discoverable mode */
> >         adapter->stored_discoverable = g_key_file_get_boolean(key_file,
> >                                         "General", "Discoverable", &gerr);
> > +
> >         if (gerr) {
> >                 adapter->stored_discoverable = false;
> >                 g_error_free(gerr);
> > @@ -6271,6 +6282,16 @@ static void load_config(struct btd_adapter *adapter)
> >                 gerr = NULL;
> >         }
> >
> > +       /* Get audio reconnect delay */
> > +       adapter->reconnect_audio_delay = g_key_file_get_integer(
> > +               key_file, "General", "ReconnectAudioDelay", &gerr);
> > +
> > +       if (gerr) {
> > +               adapter->reconnect_audio_delay = RECONNECT_AUDIO_DELAY;
> > +               g_error_free(gerr);
> > +               gerr = NULL;
> > +       }
> > +
> >         g_key_file_free(key_file);
> >  }
> >
> > @@ -7820,6 +7841,15 @@ static void dev_disconnected(struct btd_adapter *adapter,
> >         if (device) {
> >                 adapter_remove_connection(adapter, device, addr->type);
> >                 disconnect_notify(device, reason);
> > +
> > +               if (reason == MGMT_DEV_DISCONN_LOCAL_HOST_SUSPEND &&
> > +                   device_class_is_audio(device)) {
> > +                       adapter->reconnect_audio.reconnect = true;
> > +                       adapter->reconnect_audio.addr_type =
> > +                               btd_device_get_bdaddr_type(device);
> > +                       bacpy(&adapter->reconnect_audio.addr,
> > +                             device_get_address(device));
> > +               }
> >         }
> >
> >         bonding_attempt_complete(adapter, &addr->bdaddr, addr->type,
> > @@ -8766,6 +8796,53 @@ static void connected_callback(uint16_t index, uint16_t length,
> >         eir_data_free(&eir_data);
> >  }
> >
> > +static gboolean reconnect_audio_timeout(gpointer user_data)
> > +{
> > +       struct btd_adapter *adapter = user_data;
> > +
> > +       adapter->reconnect_audio_timeout = 0;
> > +
> > +       if (adapter->reconnect_audio.reconnect) {
> > +               struct btd_device *dev = btd_adapter_find_device(
> > +                       adapter, &adapter->reconnect_audio.addr,
> > +                       adapter->reconnect_audio.addr_type);
> > +
> > +               adapter->reconnect_audio.reconnect = false;
> > +
> > +               if (!dev || btd_device_is_connected(dev))
> > +                       return FALSE;
> > +
> > +               device_internal_connect(dev);
> > +       }
> > +
> > +       return FALSE;
> > +}
> > +
> > +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);
> > +
> > +       if (adapter->reconnect_audio_timeout > 0) {
> > +               g_source_remove(adapter->reconnect_audio_timeout);
> > +               adapter->reconnect_audio_timeout = 0;
> > +       }
> > +
> > +       if (adapter->reconnect_audio.reconnect) {
> > +               adapter->reconnect_audio_timeout =
> > +                       g_timeout_add_seconds(adapter->reconnect_audio_delay,
> > +                                             reconnect_audio_timeout, adapter);
> > +       }
> > +}
> > +
> >  static void device_blocked_callback(uint16_t index, uint16_t length,
> >                                         const void *param, void *user_data)
> >  {
> > @@ -9389,6 +9466,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/device.c b/src/device.c
> > index bb8e07e8f..8b165ffa4 100644
> > --- a/src/device.c
> > +++ b/src/device.c
> > @@ -747,6 +747,12 @@ gboolean device_is_trusted(struct btd_device *device)
> >         return device->trusted;
> >  }
> >
> > +bool device_class_is_audio(struct btd_device *device)
> > +{
> > +       /* Look for major service classes Audio (0x20) + Rendering (0x4) */
> > +       return ((device->class >> 16) & 0x24) == 0x24;
> > +}
> > +
> >  static gboolean dev_property_get_address(const GDBusPropertyTable *property,
> >                                         DBusMessageIter *iter, void *data)
> >  {
> > @@ -6853,6 +6859,27 @@ struct btd_service *btd_device_get_service(struct btd_device *dev,
> >         return NULL;
> >  }
> >
> > +/* Internal function to connect to a device. This fakes the dbus message used to
> > + * call the "Connect" api on the device so that the same function can be called
> > + * by bluez internally.
> > + */
> > +bool device_internal_connect(struct btd_device *dev)
> > +{
> > +       DBusMessage *msg;
> > +
> > +       if (!device_is_connectable(dev))
> > +               return false;
> > +
> > +       msg = dbus_message_new_method_call("org.bluez",
> > +                                               device_get_path(dev),
> > +                                               DEVICE_INTERFACE,
> > +                                               "Connect");
> > +       /* Sending the message usually sets serial. Fake it here. */
> > +       dbus_message_set_serial(msg, 1);
> > +
> > +       dev_connect(dbus_conn, msg, dev);
> > +}
> > +
> >  void btd_device_init(void)
> >  {
> >         dbus_conn = btd_get_dbus_connection();
> > diff --git a/src/device.h b/src/device.h
> > index 956fec1ae..82f97b5bd 100644
> > --- a/src/device.h
> > +++ b/src/device.h
> > @@ -98,6 +98,7 @@ bool device_is_connectable(struct btd_device *device);
> >  bool device_is_paired(struct btd_device *device, uint8_t bdaddr_type);
> >  bool device_is_bonded(struct btd_device *device, uint8_t bdaddr_type);
> >  gboolean device_is_trusted(struct btd_device *device);
> > +bool device_class_is_audio(struct btd_device *device);
> >  void device_set_paired(struct btd_device *dev, uint8_t bdaddr_type);
> >  void device_set_unpaired(struct btd_device *dev, uint8_t bdaddr_type);
> >  void btd_device_set_temporary(struct btd_device *device, bool temporary);
> > @@ -186,6 +187,7 @@ int btd_device_connect_services(struct btd_device *dev, GSList *services);
> >  uint32_t btd_device_get_current_flags(struct btd_device *dev);
> >  void btd_device_flags_changed(struct btd_device *dev, uint32_t supported_flags,
> >                               uint32_t current_flags);
> > +bool device_internal_connect(struct btd_device *dev);
> >
> >  void btd_device_init(void);
> >  void btd_device_cleanup(void);
> > diff --git a/src/main.conf b/src/main.conf
> > index f41203b96..c6bb78a84 100644
> > --- a/src/main.conf
> > +++ b/src/main.conf
> > @@ -82,6 +82,12 @@
> >  # 0 = disable timer, i.e. never keep temporary devices
> >  #TemporaryTimeout = 30
> >
> > +# How long to wait after controller resume before reconnecting to last used
> > +# audio device.
> > +# The value is in seconds.
> > +# Default: 5
> > +#ReconnectAudioDelay = 5
> > +
> >  [Controller]
> >  # The following values are used to load default adapter parameters.  BlueZ loads
> >  # the values into the kernel before the adapter is powered if the kernel
> > --
> > 2.28.0.rc0.142.g3c755180ce-goog
>
> Usually connection policy is handled by the policy plugin since there
> may be platforms that want implement their own connection policies on
> top of bluetoothd so they can just disable the policy plugin, iirc we
> do have reconnection policies for unexpected disconnect which should
> probably be used in the event a suspend actually trigger a
> disconnection see:
>
> https://git.kernel.org/pub/scm/bluetooth/bluez.git/tree/plugins/policy.c#n761
>
> We might need a reason code to indicate to the policy when a
> disconnect happens due to suspend logic.

I am emitting a new reason code for disconnect (local disconnect due
to suspend, 0x5). If this is just passing through the reason from the
mgmt event, I can continue using that.

Will update this patch to use the policy plugin and send up another revision.

Thanks
Abhishek

>
> --
> Luiz Augusto von Dentz

Re: [RFC Bluez PATCH 3/3] adapter: Reconnect audio on controller resume

[Luiz Augusto von Dentz]

Hi Abhishek,

On Wed, Aug 5, 2020 at 8:48 PM Abhishek Pandit-Subedi
<abhishekpandit@chromium.org> wrote:
>
> Hi Luiz,
>
> On Tue, Aug 4, 2020 at 11:54 AM Luiz Augusto von Dentz
> <luiz.dentz@gmail.com> wrote:
> >
> > Hi Abhishek,
> >
> > On Tue, Jul 28, 2020 at 6:55 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, keep track of the last audio device disconnected
> > > during suspend and try to reconnect when the controller resumes from
> > > suspend.
> > >
> > > Reviewed-by: Sonny Sasaka <sonnysasaka@chromium.org>
> > > ---
> > > Hey Luiz,
> > >
> > > On our internal review, two things stood out in this commit that we
> > > weren't able to come to a consensus on internally:
> > >
> > > * Is it better to use the audio device class or should we compare to the
> > >   A2DP, HFP and HSP uuids?
> > > * Constructing the dbus message internally before calling dev_connect
> > >   looks a bit weird. I couldn't figure out how to internally trigger
> > >   this function (since it seems to require a msg to respond to on
> > >   success/failure). Any thoughts?
> > >
> > >
> > >  src/adapter.c | 82 +++++++++++++++++++++++++++++++++++++++++++++++++++
> > >  src/device.c  | 27 +++++++++++++++++
> > >  src/device.h  |  2 ++
> > >  src/main.conf |  6 ++++
> > >  4 files changed, 117 insertions(+)
> > >
> > > diff --git a/src/adapter.c b/src/adapter.c
> > > index 5e896a9f0..b1073c439 100644
> > > --- a/src/adapter.c
> > > +++ b/src/adapter.c
> > > @@ -90,6 +90,7 @@
> > >  #define IDLE_DISCOV_TIMEOUT (5)
> > >  #define TEMP_DEV_TIMEOUT (3 * 60)
> > >  #define BONDING_TIMEOUT (2 * 60)
> > > +#define RECONNECT_AUDIO_DELAY (5)
> > >
> > >  #define SCAN_TYPE_BREDR (1 << BDADDR_BREDR)
> > >  #define SCAN_TYPE_LE ((1 << BDADDR_LE_PUBLIC) | (1 << BDADDR_LE_RANDOM))
> > > @@ -269,6 +270,15 @@ struct btd_adapter {
> > >         struct btd_device *connect_le;  /* LE device waiting to be connected */
> > >         sdp_list_t *services;           /* Services associated to adapter */
> > >
> > > +       /* audio device to reconnect after resuming from suspend */
> > > +       struct reconnect_audio_info {
> > > +               bdaddr_t addr;
> > > +               uint8_t addr_type;
> > > +               bool reconnect;
> > > +        } reconnect_audio;
> > > +       guint reconnect_audio_timeout;  /* timeout for reconnect on resume */
> > > +       uint32_t reconnect_audio_delay; /* delay reconnect after resume */
> > > +
> > >         struct btd_gatt_database *database;
> > >         struct btd_adv_manager *adv_manager;
> > >
> > > @@ -6256,6 +6266,7 @@ static void load_config(struct btd_adapter *adapter)
> > >         /* Get discoverable mode */
> > >         adapter->stored_discoverable = g_key_file_get_boolean(key_file,
> > >                                         "General", "Discoverable", &gerr);
> > > +
> > >         if (gerr) {
> > >                 adapter->stored_discoverable = false;
> > >                 g_error_free(gerr);
> > > @@ -6271,6 +6282,16 @@ static void load_config(struct btd_adapter *adapter)
> > >                 gerr = NULL;
> > >         }
> > >
> > > +       /* Get audio reconnect delay */
> > > +       adapter->reconnect_audio_delay = g_key_file_get_integer(
> > > +               key_file, "General", "ReconnectAudioDelay", &gerr);
> > > +
> > > +       if (gerr) {
> > > +               adapter->reconnect_audio_delay = RECONNECT_AUDIO_DELAY;
> > > +               g_error_free(gerr);
> > > +               gerr = NULL;
> > > +       }
> > > +
> > >         g_key_file_free(key_file);
> > >  }
> > >
> > > @@ -7820,6 +7841,15 @@ static void dev_disconnected(struct btd_adapter *adapter,
> > >         if (device) {
> > >                 adapter_remove_connection(adapter, device, addr->type);
> > >                 disconnect_notify(device, reason);
> > > +
> > > +               if (reason == MGMT_DEV_DISCONN_LOCAL_HOST_SUSPEND &&
> > > +                   device_class_is_audio(device)) {
> > > +                       adapter->reconnect_audio.reconnect = true;
> > > +                       adapter->reconnect_audio.addr_type =
> > > +                               btd_device_get_bdaddr_type(device);
> > > +                       bacpy(&adapter->reconnect_audio.addr,
> > > +                             device_get_address(device));
> > > +               }
> > >         }
> > >
> > >         bonding_attempt_complete(adapter, &addr->bdaddr, addr->type,
> > > @@ -8766,6 +8796,53 @@ static void connected_callback(uint16_t index, uint16_t length,
> > >         eir_data_free(&eir_data);
> > >  }
> > >
> > > +static gboolean reconnect_audio_timeout(gpointer user_data)
> > > +{
> > > +       struct btd_adapter *adapter = user_data;
> > > +
> > > +       adapter->reconnect_audio_timeout = 0;
> > > +
> > > +       if (adapter->reconnect_audio.reconnect) {
> > > +               struct btd_device *dev = btd_adapter_find_device(
> > > +                       adapter, &adapter->reconnect_audio.addr,
> > > +                       adapter->reconnect_audio.addr_type);
> > > +
> > > +               adapter->reconnect_audio.reconnect = false;
> > > +
> > > +               if (!dev || btd_device_is_connected(dev))
> > > +                       return FALSE;
> > > +
> > > +               device_internal_connect(dev);
> > > +       }
> > > +
> > > +       return FALSE;
> > > +}
> > > +
> > > +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);
> > > +
> > > +       if (adapter->reconnect_audio_timeout > 0) {
> > > +               g_source_remove(adapter->reconnect_audio_timeout);
> > > +               adapter->reconnect_audio_timeout = 0;
> > > +       }
> > > +
> > > +       if (adapter->reconnect_audio.reconnect) {
> > > +               adapter->reconnect_audio_timeout =
> > > +                       g_timeout_add_seconds(adapter->reconnect_audio_delay,
> > > +                                             reconnect_audio_timeout, adapter);
> > > +       }
> > > +}
> > > +
> > >  static void device_blocked_callback(uint16_t index, uint16_t length,
> > >                                         const void *param, void *user_data)
> > >  {
> > > @@ -9389,6 +9466,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/device.c b/src/device.c
> > > index bb8e07e8f..8b165ffa4 100644
> > > --- a/src/device.c
> > > +++ b/src/device.c
> > > @@ -747,6 +747,12 @@ gboolean device_is_trusted(struct btd_device *device)
> > >         return device->trusted;
> > >  }
> > >
> > > +bool device_class_is_audio(struct btd_device *device)
> > > +{
> > > +       /* Look for major service classes Audio (0x20) + Rendering (0x4) */
> > > +       return ((device->class >> 16) & 0x24) == 0x24;
> > > +}
> > > +
> > >  static gboolean dev_property_get_address(const GDBusPropertyTable *property,
> > >                                         DBusMessageIter *iter, void *data)
> > >  {
> > > @@ -6853,6 +6859,27 @@ struct btd_service *btd_device_get_service(struct btd_device *dev,
> > >         return NULL;
> > >  }
> > >
> > > +/* Internal function to connect to a device. This fakes the dbus message used to
> > > + * call the "Connect" api on the device so that the same function can be called
> > > + * by bluez internally.
> > > + */
> > > +bool device_internal_connect(struct btd_device *dev)
> > > +{
> > > +       DBusMessage *msg;
> > > +
> > > +       if (!device_is_connectable(dev))
> > > +               return false;
> > > +
> > > +       msg = dbus_message_new_method_call("org.bluez",
> > > +                                               device_get_path(dev),
> > > +                                               DEVICE_INTERFACE,
> > > +                                               "Connect");
> > > +       /* Sending the message usually sets serial. Fake it here. */
> > > +       dbus_message_set_serial(msg, 1);
> > > +
> > > +       dev_connect(dbus_conn, msg, dev);
> > > +}
> > > +
> > >  void btd_device_init(void)
> > >  {
> > >         dbus_conn = btd_get_dbus_connection();
> > > diff --git a/src/device.h b/src/device.h
> > > index 956fec1ae..82f97b5bd 100644
> > > --- a/src/device.h
> > > +++ b/src/device.h
> > > @@ -98,6 +98,7 @@ bool device_is_connectable(struct btd_device *device);
> > >  bool device_is_paired(struct btd_device *device, uint8_t bdaddr_type);
> > >  bool device_is_bonded(struct btd_device *device, uint8_t bdaddr_type);
> > >  gboolean device_is_trusted(struct btd_device *device);
> > > +bool device_class_is_audio(struct btd_device *device);
> > >  void device_set_paired(struct btd_device *dev, uint8_t bdaddr_type);
> > >  void device_set_unpaired(struct btd_device *dev, uint8_t bdaddr_type);
> > >  void btd_device_set_temporary(struct btd_device *device, bool temporary);
> > > @@ -186,6 +187,7 @@ int btd_device_connect_services(struct btd_device *dev, GSList *services);
> > >  uint32_t btd_device_get_current_flags(struct btd_device *dev);
> > >  void btd_device_flags_changed(struct btd_device *dev, uint32_t supported_flags,
> > >                               uint32_t current_flags);
> > > +bool device_internal_connect(struct btd_device *dev);
> > >
> > >  void btd_device_init(void);
> > >  void btd_device_cleanup(void);
> > > diff --git a/src/main.conf b/src/main.conf
> > > index f41203b96..c6bb78a84 100644
> > > --- a/src/main.conf
> > > +++ b/src/main.conf
> > > @@ -82,6 +82,12 @@
> > >  # 0 = disable timer, i.e. never keep temporary devices
> > >  #TemporaryTimeout = 30
> > >
> > > +# How long to wait after controller resume before reconnecting to last used
> > > +# audio device.
> > > +# The value is in seconds.
> > > +# Default: 5
> > > +#ReconnectAudioDelay = 5
> > > +
> > >  [Controller]
> > >  # The following values are used to load default adapter parameters.  BlueZ loads
> > >  # the values into the kernel before the adapter is powered if the kernel
> > > --
> > > 2.28.0.rc0.142.g3c755180ce-goog
> >
> > Usually connection policy is handled by the policy plugin since there
> > may be platforms that want implement their own connection policies on
> > top of bluetoothd so they can just disable the policy plugin, iirc we
> > do have reconnection policies for unexpected disconnect which should
> > probably be used in the event a suspend actually trigger a
> > disconnection see:
> >
> > https://git.kernel.org/pub/scm/bluetooth/bluez.git/tree/plugins/policy.c#n761
> >
> > We might need a reason code to indicate to the policy when a
> > disconnect happens due to suspend logic.
>
> I am emitting a new reason code for disconnect (local disconnect due
> to suspend, 0x5). If this is just passing through the reason from the
> mgmt event, I can continue using that.

Yep.

> Will update this patch to use the policy plugin and send up another revision.

I'd use a switch to catch all reasons that should trigger a reconnection.

> Thanks
> Abhishek
>
> >
> > --
> > Luiz Augusto von Dentz



-- 
Luiz Augusto von Dentz

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

[Abhishek Pandit-Subedi]

Hi,

Gentle reminder that this is waiting for feedback.

Thanks
Abhishek

On Tue, Jul 28, 2020 at 6:55 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:
>
>  * In the Device Disconnected management event, if the disconnect reason
>    was 0x5 (Disconnected by Local Host for Suspend) and the device is an
>    audio sink (implements major services Audio + Rendering), then it is
>    queued for reconnect.
>  * When the Controller Resumed management event is seen, we check if
>    an audio device needs to be reconnected. If yes, we queue a delayed
>    callback to do the reconnection. The delay is 5s by default and is
>    meant to allow sufficient time for any Wi-Fi activity that may occur
>    during resume (since Bluetooth connect may adversely affect that).
>
> 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 General 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'm hoping to
> test this with a few more headsets to make sure this is ok and I'm
> looking for some early feedback.
>
> Thanks
> Abhishek
>
>
>
> Abhishek Pandit-Subedi (3):
>   mgmt: Add controller suspend and resume events
>   monitor: Add btmon support for Suspend and Resume events
>   adapter: Reconnect audio on controller resume
>
>  lib/mgmt.h       | 14 +++++++++
>  monitor/packet.c | 55 ++++++++++++++++++++++++++++++++
>  src/adapter.c    | 82 ++++++++++++++++++++++++++++++++++++++++++++++++
>  src/device.c     | 27 ++++++++++++++++
>  src/device.h     |  2 ++
>  src/main.conf    |  6 ++++
>  6 files changed, 186 insertions(+)
>
> --
> 2.28.0.rc0.142.g3c755180ce-goog
>