Re: [PATCH BlueZ v2 1/3] src/device: Add Disconnected signal to propagate disconnection reason

[Frédéric Danis <frederic.danis@xxxxxxxxxxxxx>]

Hi Luiz,

On 20/05/2025 18:41, Luiz Augusto von Dentz wrote:

> Hi Frédéric,
>
> On Tue, May 20, 2025 at 12:26 PM Frédéric Danis
> <frederic.danis@xxxxxxxxxxxxx> wrote:
> > Currently a client application is informed of the disconnection by the
> > update of the Connected property to false.
> > This sends a Disconnected signal with the disconnection reason before
> > the property is updated.
> >
> > This helps client application to know the reason for the disconnection
> > and to take appropriate action.
> > ---
> > v1->v2: Propagate numerical reason instead of text one
> >
> >   src/adapter.c | 13 ++++++++-----
> >   src/device.c  | 16 ++++++++++++++--
> >   src/device.h  |  3 ++-
> >   3 files changed, 24 insertions(+), 8 deletions(-)
> >
> > diff --git a/src/adapter.c b/src/adapter.c
> > index fd425e6d2..a10721489 100644
> > --- a/src/adapter.c
> > +++ b/src/adapter.c
> > @@ -7549,7 +7549,8 @@ struct agent *adapter_get_agent(struct btd_adapter *adapter)
> >
> >   static void adapter_remove_connection(struct btd_adapter *adapter,
> >                                                  struct btd_device *device,
> > -                                               uint8_t bdaddr_type)
> > +                                               uint8_t bdaddr_type,
> > +                                               uint8_t reason)
> >   {
> >          bool remove_device = false;
> >
> > @@ -7560,7 +7561,7 @@ static void adapter_remove_connection(struct btd_adapter *adapter,
> >                  return;
> >          }
> >
> > -       device_remove_connection(device, bdaddr_type, &remove_device);
> > +       device_remove_connection(device, bdaddr_type, &remove_device, reason);
> >
> >          device_cancel_authentication(device, TRUE);
> >
> > @@ -7601,9 +7602,11 @@ static void adapter_stop(struct btd_adapter *adapter)
> >                  struct btd_device *device = adapter->connections->data;
> >                  uint8_t addr_type = btd_device_get_bdaddr_type(device);
> >
> > -               adapter_remove_connection(adapter, device, BDADDR_BREDR);
> > +               adapter_remove_connection(adapter, device, BDADDR_BREDR,
> > +                                               MGMT_DEV_DISCONN_UNKNOWN);
> >                  if (addr_type != BDADDR_BREDR)
> > -                       adapter_remove_connection(adapter, device, addr_type);
> > +                       adapter_remove_connection(adapter, device, addr_type,
> > +                                               MGMT_DEV_DISCONN_UNKNOWN);
> >          }
> >
> >          g_dbus_emit_property_changed(dbus_conn, adapter->path,
> > @@ -8551,7 +8554,7 @@ static void dev_disconnected(struct btd_adapter *adapter,
> >
> >          device = btd_adapter_find_device(adapter, &addr->bdaddr, addr->type);
> >          if (device) {
> > -               adapter_remove_connection(adapter, device, addr->type);
> > +               adapter_remove_connection(adapter, device, addr->type, reason);
> >                  disconnect_notify(device, reason);
> >          }
> >
> > diff --git a/src/device.c b/src/device.c
> > index d230af0a8..00a0fbfc7 100644
> > --- a/src/device.c
> > +++ b/src/device.c
> > @@ -3417,6 +3417,12 @@ static const GDBusMethodTable device_methods[] = {
> >          { }
> >   };
> >
> > +static const GDBusSignalTable device_signals[] = {
> > +       { GDBUS_SIGNAL("Disconnected",
> > +                       GDBUS_ARGS({ "reason", "y" })) },
> Ive changed my mind regarding this, this should actually have the same
> format as Device.Connect error reply, so we use the same domain of
> errors org.bluez.Error.{Name} followed by its message.

I'm not sure to understand this point as some of the possible reason are not errors
like MGMT_DEV_DISCONN_LOCAL_HOST or MGMT_DEV_DISCONN_REMOTE, and so org.bluez.Error.{Name}
seems inappropriate to me here.

To be more similar to other methods, the Disconnected signal may provide the reason as text
and numerical value, which can be useful in case of 'disconnection-unknown' text.

--
Frédéric Danis
Senior Software Engineer

Collabora Ltd.
Platinum Building, St John's Innovation Park, Cambridge CB4 0DS, United Kingdom
Registered in England & Wales, no. 5513718