Hi Abhishek,
On Tue, Jul 28, 2020 at 6:55 PM Abhishek Pandit-Subedi
<abhishekpandit@xxxxxxxxxxxx> 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@xxxxxxxxxxxx>
> ---
> 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
