Received-SPF: pass (google.com: domain of linux-bluetooth-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220418174929.2777339-1-jiangzp@google.com> <20220418174914.Bluez.v2.1.I6ab300fa4999c9310f4cb6fc09b1290edb6b2c2b@changeid>
In-Reply-To: <20220418174914.Bluez.v2.1.I6ab300fa4999c9310f4cb6fc09b1290edb6b2c2b@changeid>
From:   Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date:   Mon, 18 Apr 2022 15:41:23 -0700
Message-ID: <CABBYNZL7YZFch1ExcgsFV1AJV2ZBq_dXSyQFR=Zhb9Xjyk66Jg@mail.gmail.com>
Subject: Re: [Bluez PATCH v2 1/2] device: Add "Bonded" flag to dbus property
To:     Zhengping Jiang <jiangzp@google.com>
Cc:     "linux-bluetooth@vger.kernel.org" <linux-bluetooth@vger.kernel.org>,
        ChromeOS Bluetooth Upstreaming 
        <chromeos-bluetooth-upstreaming@chromium.org>,
        Sonny Sasaka <sonnysasaka@chromium.org>,
        Yun-Hao Chung <howardchung@chromium.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi Zhengping,

On Mon, Apr 18, 2022 at 10:49 AM Zhengping Jiang <jiangzp@google.com> wrote:
>
> Add "Bonded" to dbus device property table. When setting the "Bonded
> flag, check the status of the Bonded property first. If the Bonded
> property is changed, send property changed signal.
>
> Reviewed-by: Sonny Sasaka <sonnysasaka@chromium.org>
> Reviewed-by: Yun-Hao Chung <howardchung@chromium.org>
>
> Signed-off-by: Zhengping Jiang <jiangzp@google.com>
> ---
>
> Changes in v2:
> - Move one variable declaration to the top following C90 standard
>
> Changes in v1:
> - Add "Bonded" to D-Bus interface
> - Send property changed signal if the bonded flag is changed
>
>  doc/device-api.txt |  4 ++++
>  src/device.c       | 40 +++++++++++++++++++++++++++++++++++-----
>  2 files changed, 39 insertions(+), 5 deletions(-)
>
> diff --git a/doc/device-api.txt b/doc/device-api.txt
> index 4e824d2dec17..6162755f954c 100644
> --- a/doc/device-api.txt
> +++ b/doc/device-api.txt
> @@ -171,6 +171,10 @@ Properties string Address [readonly]
>
>                         Indicates if the remote device is paired.
>
> +               boolean Bonded [readonly]
> +
> +                       Indicates if the remote device is bonded.

It is probably a good idea to add a description about Bonded vs
Paired. Btw, API documentation should be in a separate patch.

> +
>                 boolean Connected [readonly]
>
>                         Indicates if the remote device is currently connected.
> diff --git a/src/device.c b/src/device.c
> index 8dc12d026827..868c41f025d9 100644
> --- a/src/device.c
> +++ b/src/device.c
> @@ -1042,6 +1042,22 @@ static gboolean dev_property_get_paired(const GDBusPropertyTable *property,
>         return TRUE;
>  }
>
> +static gboolean dev_property_get_bonded(const GDBusPropertyTable *property,
> +                                       DBusMessageIter *iter, void *data)
> +{
> +       struct btd_device *dev = data;
> +       dbus_bool_t val;
> +
> +       if (dev->bredr_state.bonded || dev->le_state.bonded)
> +               val = TRUE;
> +       else
> +               val = FALSE;
> +
> +       dbus_message_iter_append_basic(iter, DBUS_TYPE_BOOLEAN, &val);
> +
> +       return TRUE;
> +}
> +
>  static gboolean dev_property_get_legacy(const GDBusPropertyTable *property,
>                                         DBusMessageIter *iter, void *data)
>  {
> @@ -3120,6 +3136,7 @@ static const GDBusPropertyTable device_properties[] = {
>         { "Icon", "s", dev_property_get_icon, NULL,
>                                         dev_property_exists_icon },
>         { "Paired", "b", dev_property_get_paired },
> +       { "Bonded", "b", dev_property_get_bonded },
>         { "Trusted", "b", dev_property_get_trusted, dev_property_set_trusted },
>         { "Blocked", "b", dev_property_get_blocked, dev_property_set_blocked },
>         { "LegacyPairing", "b", dev_property_get_legacy },
> @@ -6114,17 +6131,30 @@ void btd_device_set_trusted(struct btd_device *device, gboolean trusted)
>
>  void device_set_bonded(struct btd_device *device, uint8_t bdaddr_type)
>  {
> +       struct bearer_state *state;
> +
>         if (!device)
>                 return;
>
> -       DBG("");
> +       state = get_state(device, bdaddr_type);
>
> -       if (bdaddr_type == BDADDR_BREDR)
> -               device->bredr_state.bonded = true;
> -       else
> -               device->le_state.bonded = true;
> +       if (state->bonded)
> +               return;
> +
> +       DBG("setting bonded for device to true");
> +
> +       state->bonded = true;
>
>         btd_device_set_temporary(device, false);
> +
> +       /* If the other bearer state was already true we don't need to
> +        * send any property signals.
> +        */
> +       if (device->bredr_state.bonded == device->le_state.bonded)
> +               return;
> +
> +       g_dbus_emit_property_changed(dbus_conn, device->path,
> +                                               DEVICE_INTERFACE, "Bonded");
>  }
>
>  void device_set_legacy(struct btd_device *device, bool legacy)
> --
> 2.36.0.rc0.470.gd361397f0d-goog
>


-- 
Luiz Augusto von Dentz