Hi Frédéric,
On Wed, May 21, 2025 at 4:49 AM Frédéric Danis
<frederic.danis@xxxxxxxxxxxxx> wrote:
>
> 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.
We can do something like org.bluez.Error.None for these, or use a
different domain like org.bluez.Reason.Local, that still is a fixed
string that client can match, rather than a free format string, note
that sometimes an error can cause a clean disconnect but in that case
we want to capture the error not the disconnect reason.
> 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.
But we are already masking the numerical reason, besides for D-Bus it
sort of makes more sense to use a domain name to describe errors.
--
Luiz Augusto von Dentz
