2022-04-19 14:54:54

by Luiz Augusto von Dentz

[permalink] [raw]
Subject: Re: [Bluez PATCH v2 1/2] device: Add "Bonded" flag to dbus property

Hi Zhengping,

On Mon, Apr 18, 2022 at 10:49 AM Zhengping Jiang <[email protected]> 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 <[email protected]>
> Reviewed-by: Yun-Hao Chung <[email protected]>
>
> Signed-off-by: Zhengping Jiang <[email protected]>
> ---
>
> 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


2022-05-03 00:37:20

by Luiz Augusto von Dentz

[permalink] [raw]
Subject: Re: [Bluez PATCH v2 1/2] device: Add "Bonded" flag to dbus property

Hi Zhengping,

On Mon, Apr 18, 2022 at 3:41 PM Luiz Augusto von Dentz
<[email protected]> wrote:
>
> Hi Zhengping,
>
> On Mon, Apr 18, 2022 at 10:49 AM Zhengping Jiang <[email protected]> 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 <[email protected]>
> > Reviewed-by: Yun-Hao Chung <[email protected]>
> >
> > Signed-off-by: Zhengping Jiang <[email protected]>
> > ---
> >
> > 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.

Will you be updating following this comments or you are waiting more
feedback from upstream?

> > +
> > 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



--
Luiz Augusto von Dentz