Hi Luiz,
On Tue, Aug 4, 2020 at 11:54 AM Luiz Augusto von Dentz
<luiz.dentz@xxxxxxxxx> wrote:
>
> 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.
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
