Hi Abhishek,
On Wed, Aug 5, 2020 at 8:48 PM Abhishek Pandit-Subedi
<abhishekpandit@xxxxxxxxxxxx> wrote:
>
> 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.
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
