Hi Bastien, On Wed, Aug 31, 2022 at 2:32 AM Bastien Nocera <hadess@xxxxxxxxxx> wrote: > > This property should allow any program to show whether an adapter is in > the process of being turned on. > > As turning on an adapter isn't instantaneous, it's important that the UI > reflects the transitional state of the adapter's power, and doesn't > assume the device is already turned on but not yet working, or still off > despite having requested for it to be turned on, in both cases making > the UI feel unresponsive. > > This can also not be implemented in front-ends directly as, then, > the status of an adapter wouldn't be reflected correctly in the Settings > window if it's turned on in the system menu. Implementing it in the > front-ends would also preclude from having feedback about the state of > the adapter when bluetoothd is the one powering up the adapter after the > rfkill was unblocked. > > See https://gitlab.gnome.org/GNOME/gnome-bluetooth/-/issues/121 > and the original https://gitlab.gnome.org/GNOME/gnome-shell/-/issues/5773 > --- > src/adapter.c | 88 ++++++++++++++++++++++++++++++++++++++++++++++++++- > 1 file changed, 87 insertions(+), 1 deletion(-) > > diff --git a/src/adapter.c b/src/adapter.c > index 641db67f9..e295ef247 100644 > --- a/src/adapter.c > +++ b/src/adapter.c > @@ -239,6 +239,12 @@ struct btd_adapter_pin_cb_iter { > /* When the iterator reaches the end, it is NULL and attempt is 0 */ > }; > > +enum { > + ADAPTER_POWER_STATE_TARGET_NONE = 0, > + ADAPTER_POWER_STATE_TARGET_OFF, > + ADAPTER_POWER_STATE_TARGET_ON > +}; Lets take out the TARGET portion and have all the states PowerState can assume including the transitioning ones. > + > struct btd_adapter { > int ref_count; > > @@ -253,6 +259,7 @@ struct btd_adapter { > bool blocked; /* whether rfkill is enabled */ > uint32_t supported_settings; /* controller supported settings */ > uint32_t pending_settings; /* pending controller settings */ > + uint32_t power_state_target; /* the target power state */ Let's have the value stored as the enum here so it reflects directly the values PowerState property can assume. > uint32_t current_settings; /* current controller settings */ > > char *path; /* adapter object path */ > @@ -580,6 +587,8 @@ static void settings_changed(struct btd_adapter *adapter, uint32_t settings) > if (changed_mask & MGMT_SETTING_POWERED) { > g_dbus_emit_property_changed(dbus_conn, adapter->path, > ADAPTER_INTERFACE, "Powered"); > + g_dbus_emit_property_changed(dbus_conn, adapter->path, > + ADAPTER_INTERFACE, "PowerState"); > > if (adapter->current_settings & MGMT_SETTING_POWERED) { > adapter_start(adapter); > @@ -619,6 +628,16 @@ static void settings_changed(struct btd_adapter *adapter, uint32_t settings) > } > } > > +static void reset_power_state_target(struct btd_adapter *adapter, uint8_t value) > +{ > + if ((value && > + adapter->power_state_target == ADAPTER_POWER_STATE_TARGET_ON) || > + (!value && > + adapter->power_state_target == ADAPTER_POWER_STATE_TARGET_OFF)) { > + adapter->power_state_target = ADAPTER_POWER_STATE_TARGET_NONE; > + } > +} Id suggest we add DBG statement to make it easier to debug, and rework a little bit so it takes care of updating the property when the state changes: static void adapter_set_power_state(struct btd_adapter *adapter, enum value) { if (adapter->power_state == value) return; DBG("%s", adapter_power_state_str(value)); ...update... g_dbus_emit_property_changed(dbus_conn, adapter->path, ADAPTER_INTERFACE, "PowerState"); } > static void new_settings_callback(uint16_t index, uint16_t length, > const void *param, void *user_data) > { > @@ -636,6 +655,9 @@ static void new_settings_callback(uint16_t index, uint16_t length, > if (settings == adapter->current_settings) > return; > > + if ((adapter->current_settings ^ settings) & MGMT_SETTING_POWERED) > + reset_power_state_target(adapter, settings & MGMT_SETTING_POWERED ? 0x01 : 0x00); > + > DBG("Settings: 0x%08x", settings); > > settings_changed(adapter, settings); > @@ -644,6 +666,7 @@ static void new_settings_callback(uint16_t index, uint16_t length, > struct set_mode_data { > struct btd_adapter *adapter; > uint32_t setting; > + uint8_t value; > }; > > static void set_mode_complete(uint8_t status, uint16_t length, > @@ -658,6 +681,8 @@ static void set_mode_complete(uint8_t status, uint16_t length, > if (status == MGMT_STATUS_RFKILLED) > adapter->blocked = true; > adapter->pending_settings &= ~data->setting; > + if (data->setting & MGMT_SETTING_POWERED) > + reset_power_state_target(adapter, data->value); > return; > } > > @@ -695,6 +720,11 @@ static bool set_mode(struct btd_adapter *adapter, uint16_t opcode, > switch (opcode) { > case MGMT_OP_SET_POWERED: > setting = MGMT_SETTING_POWERED; > + adapter->power_state_target = mode ? > + ADAPTER_POWER_STATE_TARGET_ON : > + ADAPTER_POWER_STATE_TARGET_OFF; > + g_dbus_emit_property_changed(dbus_conn, adapter->path, > + ADAPTER_INTERFACE, "PowerState"); > break; > case MGMT_OP_SET_CONNECTABLE: > setting = MGMT_SETTING_CONNECTABLE; > @@ -715,6 +745,7 @@ static bool set_mode(struct btd_adapter *adapter, uint16_t opcode, > data = g_new0(struct set_mode_data, 1); > data->adapter = adapter; > data->setting = setting; > + data->value = mode; > > if (mgmt_send(adapter->mgmt, opcode, > adapter->dev_id, sizeof(cp), &cp, > @@ -722,8 +753,13 @@ static bool set_mode(struct btd_adapter *adapter, uint16_t opcode, > adapter->pending_settings |= setting; > return true; > } > - > g_free(data); > + if (setting == MGMT_SETTING_POWERED) { > + /* cancel the earlier setting */ > + adapter->power_state_target = ADAPTER_POWER_STATE_TARGET_NONE; > + g_dbus_emit_property_changed(dbus_conn, adapter->path, > + ADAPTER_INTERFACE, "PowerState"); > + } > btd_error(adapter->dev_id, "Failed to set mode for index %u", > adapter->dev_id); > > @@ -2901,6 +2937,7 @@ struct property_set_data { > struct btd_adapter *adapter; > uint32_t setting; > GDBusPendingPropertySet id; > + uint8_t value; > }; > > static void property_set_mode_complete(uint8_t status, uint16_t length, > @@ -2928,6 +2965,8 @@ static void property_set_mode_complete(uint8_t status, uint16_t length, > mgmt_errstr(status)); > > adapter->pending_settings &= ~data->setting; > + if (data->setting & MGMT_SETTING_POWERED) > + reset_power_state_target(adapter, data->value); > return; > } > > @@ -3051,6 +3090,16 @@ static void property_set_mode(struct btd_adapter *adapter, uint32_t setting, > data->adapter = adapter; > data->setting = setting; > data->id = id; > + data->setting = setting; > + data->value = mode; > + > + if (setting == MGMT_SETTING_POWERED) { > + adapter->power_state_target = mode ? > + ADAPTER_POWER_STATE_TARGET_ON : > + ADAPTER_POWER_STATE_TARGET_OFF; > + g_dbus_emit_property_changed(dbus_conn, adapter->path, > + ADAPTER_INTERFACE, "PowerState"); > + } > > if (mgmt_send(adapter->mgmt, opcode, adapter->dev_id, len, param, > property_set_mode_complete, data, g_free) > 0) { > @@ -3059,6 +3108,12 @@ static void property_set_mode(struct btd_adapter *adapter, uint32_t setting, > } > > g_free(data); > + if (setting == MGMT_SETTING_POWERED) { > + /* cancel the earlier setting */ > + adapter->power_state_target = ADAPTER_POWER_STATE_TARGET_NONE; > + g_dbus_emit_property_changed(dbus_conn, adapter->path, > + ADAPTER_INTERFACE, "PowerState"); > + } > > failed: > btd_error(adapter->dev_id, "Failed to set mode for index %u", > @@ -3090,6 +3145,31 @@ static void property_set_powered(const GDBusPropertyTable *property, > property_set_mode(adapter, MGMT_SETTING_POWERED, iter, id); > } > > +static gboolean property_get_power_state(const GDBusPropertyTable *property, > + DBusMessageIter *iter, void *user_data) > +{ > + struct btd_adapter *adapter = user_data; > + const char *str; > + > + if (adapter->blocked) { > + str = "off-blocked"; > + } else if (adapter->power_state_target == ADAPTER_POWER_STATE_TARGET_NONE) { > + if (adapter->current_settings & MGMT_SETTING_POWERED) > + str = "on"; > + else > + str = "off"; > + } else { > + if (adapter->power_state_target == ADAPTER_POWER_STATE_TARGET_ON) > + str = "off-enabling"; > + else > + str = "on-disabling"; > + } > + > + dbus_message_iter_append_basic(iter, DBUS_TYPE_STRING, &str); With the suggestion above this should be just a matter of: const char *str = adapter_power_state_str(adapter->power_state); dbus_message_iter_append_basic(iter, DBUS_TYPE_STRING, &str); So any updates to the enum, etc, are reflected directly as well since we will need to update adapter_power_state_str. > + return TRUE; > +} > + > static gboolean property_get_discoverable(const GDBusPropertyTable *property, > DBusMessageIter *iter, void *user_data) > { > @@ -3728,6 +3808,8 @@ static const GDBusPropertyTable adapter_properties[] = { > { "Alias", "s", property_get_alias, property_set_alias }, > { "Class", "u", property_get_class }, > { "Powered", "b", property_get_powered, property_set_powered }, > + { "PowerState", "s", property_get_power_state, NULL, NULL, > + G_DBUS_PROPERTY_FLAG_EXPERIMENTAL }, > { "Discoverable", "b", property_get_discoverable, > property_set_discoverable }, > { "DiscoverableTimeout", "u", property_get_discoverable_timeout, > @@ -5534,6 +5616,8 @@ static void adapter_start(struct btd_adapter *adapter) > { > g_dbus_emit_property_changed(dbus_conn, adapter->path, > ADAPTER_INTERFACE, "Powered"); > + g_dbus_emit_property_changed(dbus_conn, adapter->path, > + ADAPTER_INTERFACE, "PowerState"); > > DBG("adapter %s has been enabled", adapter->path); > > @@ -7277,6 +7361,8 @@ static void adapter_stop(struct btd_adapter *adapter) > > g_dbus_emit_property_changed(dbus_conn, adapter->path, > ADAPTER_INTERFACE, "Powered"); > + g_dbus_emit_property_changed(dbus_conn, adapter->path, > + ADAPTER_INTERFACE, "PowerState"); > > DBG("adapter %s has been disabled", adapter->path); > } > -- > 2.37.2 > -- Luiz Augusto von Dentz
