| Message ID | 20220831091912.47894-2-hadess@hadess.net (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [BlueZ,v4,1/6] adapter: Keep track of whether the adapter is rfkill'ed | expand |
Hi Bastien, On Wed, Aug 31, 2022 at 2:32 AM Bastien Nocera <hadess@hadess.net> 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 >
On Wed, 2022-08-31 at 13:03 -0700, Luiz Augusto von Dentz wrote: > > +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. Done, with the rest of the changes, in v5. I've also fixed the initial rfkill state if the adapter is blocked before bluetoothd is started.
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 +}; + 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 */ 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; + } +} + 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); + + 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); }