adapter: Implement PowerState property

This property should allow any program to show the transitional state,
not just the one that requested the change, and will also show
transitional states that were the results of other system changes, like
rfkill changes.
---

Downstream bug in gnome-bluetooth:
https://gitlab.gnome.org/GNOME/gnome-bluetooth/-/issues/121

Note that this probably doesn't handle multiple, conflicting requests
for power on, or power off. Is there a good way to protect against
that?

 client/main.c       |  1 +
 doc/adapter-api.txt | 14 ++++++++++
 src/adapter.c       | 66 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 81 insertions(+)

This is an automated email and please do not reply to this email.

Dear Submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
While preparing the CI tests, the patches you submitted couldn't be applied to the current HEAD of the repository.

----- Output -----
error: corrupt patch at line 20
hint: Use 'git am --show-current-patch' to see the failed patch


Please resolve the issue and submit the patches again.


---
Regards,
Linux Bluetooth

On Thu, 2022-08-25 at 15:26 +0200, Bastien Nocera wrote:
> This property should allow any program to show the transitional
> state,
> not just the one that requested the change, and will also show
> transitional states that were the results of other system changes,
> like
> rfkill changes.

Looks like the bot doesn't like where I put those comments.

If anyone can comment on the API I used, and I'll iterate the actual
implementation. I'd like the API to be settled by the time GNOME 43
ships, so we can rely on it there.

Cheers

Hi Archie and Bastien,

On Thu, Aug 25, 2022 at 4:06 PM Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
>
> Hi Bastien,
>
> On Thu, Aug 25, 2022 at 8:32 AM Bastien Nocera <hadess@hadess.net> wrote:
> >
> > On Thu, 2022-08-25 at 15:26 +0200, Bastien Nocera wrote:
> > > This property should allow any program to show the transitional
> > > state,
> > > not just the one that requested the change, and will also show
> > > transitional states that were the results of other system changes,
> > > like
> > > rfkill changes.
> >
> > Looks like the bot doesn't like where I put those comments.
> >
> > If anyone can comment on the API I used, and I'll iterate the actual
> > implementation. I'd like the API to be settled by the time GNOME 43
> > ships, so we can rely on it there.
>
> I wonder what are you actually after with these changes, in most cases
> I'd say the changes shall just be queued, anyway perhaps the problem
> was something related to:
>
> https://git.kernel.org/pub/scm/bluetooth/bluez.git/commit/?id=ede7b915980fbc80eff80aa189c35ca016956c61

Btw, I think this shows that perhaps it would be best practice to
create a github issue when there are bugs for Gnome/Chrome OS so we
can properly use the Fixes: tag to close them which makes it easier
for downstream to find out if a similar issue was fixed and attempt to
reuse the same fix.

On Thu, 2022-08-25 at 16:06 -0700, Luiz Augusto von Dentz wrote:
> Hi Bastien,
> 
> On Thu, Aug 25, 2022 at 8:32 AM Bastien Nocera <hadess@hadess.net>
> wrote:
> > 
> > On Thu, 2022-08-25 at 15:26 +0200, Bastien Nocera wrote:
> > > This property should allow any program to show the transitional
> > > state,
> > > not just the one that requested the change, and will also show
> > > transitional states that were the results of other system
> > > changes,
> > > like
> > > rfkill changes.
> > 
> > Looks like the bot doesn't like where I put those comments.
> > 
> > If anyone can comment on the API I used, and I'll iterate the
> > actual
> > implementation. I'd like the API to be settled by the time GNOME 43
> > ships, so we can rely on it there.
> 
> I wonder what are you actually after with these changes, in most
> cases
> I'd say the changes shall just be queued, anyway perhaps the problem
> was something related to:
> 
> https://git.kernel.org/pub/scm/bluetooth/bluez.git/commit/?id=ede7b915980fbc80eff80aa189c35ca016956c61
> 

That helps with one of the problems I ran into (resetting the
transitional state on failure), thanks.

What I'm after is a transitional state when the Bluetooth adapter is
being powered on, as it can take more than "an instant" (aka 200msec)
to turn on, and sometimes much longer.

Users in that transition period can wonder why the adapter is already
on but not usable, or still off. This is even worse if the enablement
doesn't work, as it could show that the device was on/available for a
time before stopping being available, depending on which option the UI
developer took, which is obviously not true.

I updated the patch for the latest git and completed the commit
message, let me know if that helps.

On Thu, 2022-08-25 at 16:10 -0700, Luiz Augusto von Dentz wrote:
> Hi Archie and Bastien,
> 
> On Thu, Aug 25, 2022 at 4:06 PM Luiz Augusto von Dentz
> <luiz.dentz@gmail.com> wrote:
> > 
> > Hi Bastien,
> > 
> > On Thu, Aug 25, 2022 at 8:32 AM Bastien Nocera <hadess@hadess.net>
> > wrote:
> > > 
> > > On Thu, 2022-08-25 at 15:26 +0200, Bastien Nocera wrote:
> > > > This property should allow any program to show the transitional
> > > > state,
> > > > not just the one that requested the change, and will also show
> > > > transitional states that were the results of other system
> > > > changes,
> > > > like
> > > > rfkill changes.
> > > 
> > > Looks like the bot doesn't like where I put those comments.
> > > 
> > > If anyone can comment on the API I used, and I'll iterate the
> > > actual
> > > implementation. I'd like the API to be settled by the time GNOME
> > > 43
> > > ships, so we can rely on it there.
> > 
> > I wonder what are you actually after with these changes, in most
> > cases
> > I'd say the changes shall just be queued, anyway perhaps the
> > problem
> > was something related to:
> > 
> > https://git.kernel.org/pub/scm/bluetooth/bluez.git/commit/?id=ede7b915980fbc80eff80aa189c35ca016956c61
> 
> Btw, I think this shows that perhaps it would be best practice to
> create a github issue when there are bugs for Gnome/Chrome OS so we
> can properly use the Fixes: tag to close them which makes it easier
> for downstream to find out if a similar issue was fixed and attempt
> to
> reuse the same fix.

That's not the fix we need in this particular case, but I'm fine with
filing GitHub bugs if you prefer.

Hi Bastien,

On Thu, Aug 25, 2022 at 6:40 AM Bastien Nocera <hadess@hadess.net> wrote:
>
> This property should allow any program to show the transitional state,
> not just the one that requested the change, and will also show
> transitional states that were the results of other system changes, like
> rfkill changes.
> ---
>
> Downstream bug in gnome-bluetooth:
> https://gitlab.gnome.org/GNOME/gnome-bluetooth/-/issues/121
>
> Note that this probably doesn't handle multiple, conflicting requests
> for power on, or power off. Is there a good way to protect against
> that?
>
>  client/main.c       |  1 +
>  doc/adapter-api.txt | 14 ++++++++++
>  src/adapter.c       | 66 +++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 81 insertions(+)
>
> diff --git a/client/main.c b/client/main.c
> index 19139d15b..ddd97c23c 100644
> --- a/client/main.c
> +++ b/client/main.c
> @@ -981,6 +981,7 @@ static void cmd_show(int argc, char *argv[])
>         print_property(adapter->proxy, "Alias");
>         print_property(adapter->proxy, "Class");
>         print_property(adapter->proxy, "Powered");
> +       print_property(adapter->proxy, "PowerState");
>         print_property(adapter->proxy, "Discoverable");
>         print_property(adapter->proxy, "DiscoverableTimeout");
>         print_property(adapter->proxy, "Pairable");
> diff --git a/doc/adapter-api.txt b/doc/adapter-api.txt
> index 48466ab75..5bdb9c34e 100644
> --- a/doc/adapter-api.txt
> +++ b/doc/adapter-api.txt
> @@ -269,6 +269,20 @@ Properties string Address [readonly]
>                         restart or unplugging of the adapter it will reset
>                         back to false.
>
> +               string PowerState [readonly]
> +
> +                       The power state of an adapter.
> +
> +                       The power state will show whether the adapter is
> +                       turning off, or turning on, as well as being on
> +                       or off.
> +
> +                       Possible values:
> +                               "on" - powered on
> +                               "off" - powered off
> +                               "turning-on" - transitioning from "off" to "on"
> +                               "turning-off" - transitioning from "on" to "off"

This changes need to be split in its own patch, also not long ago I
was discussing with Marcel about MGMT power states vs rfkill, they
don't seem to be inline with each other, while rfkill does reflect on
the MGMT interface powered states doesn't which means the driver are
not aware of it since afaik the MGMT states are not communicated back
to the driver, so perhaps we should reflect this distinction on
PowerState with a dedicated state for rfkill or we start using rfkill
ourselves directly via daemon using a dedicated property.

>                 boolean Discoverable [readwrite]
>
>                         Switch an adapter to discoverable or non-discoverable
> diff --git a/src/adapter.c b/src/adapter.c
> index ec26aab1a..3b0237708 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;
>
> @@ -252,6 +258,7 @@ struct btd_adapter {
>         char *short_name;               /* controller short name */
>         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 */
> @@ -579,6 +586,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);
> @@ -635,6 +644,11 @@ static void new_settings_callback(uint16_t index, uint16_t length,
>         if (settings == adapter->current_settings)
>                 return;
>
> +       if ((adapter->current_settings & MGMT_SETTING_POWERED) !=
> +           (settings & MGMT_SETTING_POWERED)) {
> +               adapter->power_state_target = ADAPTER_POWER_STATE_TARGET_NONE;
> +       }
> +
>         DBG("Settings: 0x%08x", settings);
>
>         settings_changed(adapter, settings);
> @@ -684,6 +698,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;
> @@ -708,6 +727,12 @@ static bool set_mode(struct btd_adapter *adapter, uint16_t opcode,
>                                 set_mode_complete, adapter, NULL) > 0)
>                 return true;
>
> +       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);
>
> @@ -2878,6 +2903,7 @@ static gboolean property_get_mode(struct btd_adapter *adapter,
>  struct property_set_data {
>         struct btd_adapter *adapter;
>         GDBusPendingPropertySet id;
> +       uint32_t setting;
>  };
>
>  static void property_set_mode_complete(uint8_t status, uint16_t length,
> @@ -2888,6 +2914,9 @@ static void property_set_mode_complete(uint8_t status, uint16_t length,
>
>         DBG("%s (0x%02x)", mgmt_errstr(status), status);
>
> +       if (data->setting == MGMT_SETTING_POWERED)
> +               adapter->power_state_target = ADAPTER_POWER_STATE_TARGET_NONE;
> +
>         if (status != MGMT_STATUS_SUCCESS) {
>                 const char *dbus_err;
>
> @@ -3025,6 +3054,15 @@ static void property_set_mode(struct btd_adapter *adapter, uint32_t setting,
>
>         data->adapter = adapter;
>         data->id = id;
> +       data->setting = setting;
> +
> +       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)
> @@ -3062,6 +3100,29 @@ 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->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 = "turning-on";
> +               else
> +                       str = "turning-off";
> +       }
> +
> +       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)
>  {
> @@ -3700,6 +3761,7 @@ 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 },
>         { "Discoverable", "b", property_get_discoverable,
>                                         property_set_discoverable },
>         { "DiscoverableTimeout", "u", property_get_discoverable_timeout,
> @@ -5506,6 +5568,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);
>
> @@ -7249,6 +7313,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 Fri, 2022-08-26 at 11:50 -0700, Luiz Augusto von Dentz wrote:
> Hi Bastien,
> 
> On Thu, Aug 25, 2022 at 6:40 AM Bastien Nocera <hadess@hadess.net>
> wrote:
> > 
> > This property should allow any program to show the transitional
> > state,
> > not just the one that requested the change, and will also show
> > transitional states that were the results of other system changes,
> > like
> > rfkill changes.
> > ---
> > 
> > Downstream bug in gnome-bluetooth:
> > https://gitlab.gnome.org/GNOME/gnome-bluetooth/-/issues/121
> > 
> > Note that this probably doesn't handle multiple, conflicting
> > requests
> > for power on, or power off. Is there a good way to protect against
> > that?
> > 
> >  client/main.c       |  1 +
> >  doc/adapter-api.txt | 14 ++++++++++
> >  src/adapter.c       | 66
> > +++++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 81 insertions(+)
> > 
> > diff --git a/client/main.c b/client/main.c
> > index 19139d15b..ddd97c23c 100644
> > --- a/client/main.c
> > +++ b/client/main.c
> > @@ -981,6 +981,7 @@ static void cmd_show(int argc, char *argv[])
> >         print_property(adapter->proxy, "Alias");
> >         print_property(adapter->proxy, "Class");
> >         print_property(adapter->proxy, "Powered");
> > +       print_property(adapter->proxy, "PowerState");
> >         print_property(adapter->proxy, "Discoverable");
> >         print_property(adapter->proxy, "DiscoverableTimeout");
> >         print_property(adapter->proxy, "Pairable");
> > diff --git a/doc/adapter-api.txt b/doc/adapter-api.txt
> > index 48466ab75..5bdb9c34e 100644
> > --- a/doc/adapter-api.txt
> > +++ b/doc/adapter-api.txt
> > @@ -269,6 +269,20 @@ Properties string Address [readonly]
> >                         restart or unplugging of the adapter it
> > will reset
> >                         back to false.
> > 
> > +               string PowerState [readonly]
> > +
> > +                       The power state of an adapter.
> > +
> > +                       The power state will show whether the
> > adapter is
> > +                       turning off, or turning on, as well as
> > being on
> > +                       or off.
> > +
> > +                       Possible values:
> > +                               "on" - powered on
> > +                               "off" - powered off
> > +                               "turning-on" - transitioning from
> > "off" to "on"
> > +                               "turning-off" - transitioning from
> > "on" to "off"
> 
> This changes need to be split in its own patch,

You want the docs changes to be a separate patch? I can do that, but I
thought that having documentation changes be added along with the
property itself would have been desirable.

>  also not long ago I
> was discussing with Marcel about MGMT power states vs rfkill, they
> don't seem to be inline with each other, while rfkill does reflect on
> the MGMT interface powered states doesn't which means the driver are
> not aware of it since afaik the MGMT states are not communicated back
> to the driver, so perhaps we should reflect this distinction on
> PowerState with a dedicated state for rfkill or we start using rfkill
> ourselves directly via daemon using a dedicated property.

The rfkill calls made by GNOME are usually sent to all the rfkill
devices, rather than targeted at blocking a single adapter. So the
adapter state isn't super interesting in this case, as all the devices
will be either blocked (computer with no platform rfkill), or gone from
the USB bus (computer with a platform rfkill).


The good thing about using a string property here is that we can extend
it. One thing we can do though, to make extensibility easier, is take a
leaf out of the icon naming specification, and use prefixes to encode
the expected state in case the software isn't new enough to know about
a property.

For example:
- "on"
- "off"
- "on-disabling" (transitioning from on to off)
- "off-enabling" (transitioning from off to on)

So we could easily add:
- "off-rfkill" (off and blocked)

Let me know what you think.

Cheers

On Mon, 2022-08-29 at 11:47 +0200, Bastien Nocera wrote:
> The rfkill calls made by GNOME are usually sent to all the rfkill
> devices, rather than targeted at blocking a single adapter. So the
> adapter state isn't super interesting in this case, as all the
> devices
> will be either blocked (computer with no platform rfkill), or gone
> from
> the USB bus (computer with a platform rfkill).
> 
> 
> The good thing about using a string property here is that we can
> extend
> it. One thing we can do though, to make extensibility easier, is take
> a
> leaf out of the icon naming specification, and use prefixes to encode
> the expected state in case the software isn't new enough to know
> about
> a property.
> 
> For example:
> - "on"
> - "off"
> - "on-disabling" (transitioning from on to off)
> - "off-enabling" (transitioning from off to on)
> 
> So we could easily add:
> - "off-rfkill" (off and blocked)
> 
> Let me know what you think.

Poking again, as I really need to land this soon (and by soon I mean
last week ;)

Hi Bastien,

On Tue, Aug 30, 2022 at 9:59 AM Bastien Nocera <hadess@hadess.net> wrote:
>
> On Mon, 2022-08-29 at 11:47 +0200, Bastien Nocera wrote:
> > The rfkill calls made by GNOME are usually sent to all the rfkill
> > devices, rather than targeted at blocking a single adapter. So the
> > adapter state isn't super interesting in this case, as all the
> > devices
> > will be either blocked (computer with no platform rfkill), or gone
> > from
> > the USB bus (computer with a platform rfkill).
> >
> >
> > The good thing about using a string property here is that we can
> > extend
> > it. One thing we can do though, to make extensibility easier, is take
> > a
> > leaf out of the icon naming specification, and use prefixes to encode
> > the expected state in case the software isn't new enough to know
> > about
> > a property.
> >
> > For example:
> > - "on"
> > - "off"
> > - "on-disabling" (transitioning from on to off)
> > - "off-enabling" (transitioning from off to on)
> >
> > So we could easily add:
> > - "off-rfkill" (off and blocked)
> >
> > Let me know what you think.
>
> Poking again, as I really need to land this soon (and by soon I mean
> last week ;)

off-rfkill sounds fine to me, btw introduce it as experimental so we
can still make changes if need to.