Hi Bastien,
On Tue, Feb 11, 2025 at 11:00 AM Bastien Nocera <hadess@xxxxxxxxxx> wrote:
>
> On Wed, 2025-01-22 at 14:57 -0500, Luiz Augusto von Dentz wrote:
> > Hi Bastien,
> >
> > On Wed, Jan 22, 2025 at 6:33 AM Bastien Nocera <hadess@xxxxxxxxxx>
> > wrote:
> > >
> > > Output clearer debug information so that it's possible to follow
> > > the
> > > decisions made by the bluetoothd daemon when a client such as
> > > bluetoothctl or the GNOME Bluetooth settings ask it to connect to a
> > > device.
> > > ---
> > > client/error.c | 1 +
> > > client/main.c | 5 +++--
> > > src/device.c | 36 +++++++++++++++++++++++++++++-------
> > > 3 files changed, 33 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/client/error.c b/client/error.c
> > > index 975e4030dfc0..aa8a058cce98 100644
> > > --- a/client/error.c
> > > +++ b/client/error.c
> > > @@ -19,6 +19,7 @@ struct {
> > > { "br-connection-profile-unavailable", "Exhausted the list
> > > of BR/EDR profiles to connect to" },
> > > { "br-connection-busy", "Cannot connect, connection busy"
> > > },
> > > { "br-connection-adapter-not-powered", "Cannot connect,
> > > adapter is not powered" },
> > > + { "br-connection-page-timeout", "Device is unpowered or not
> > > in range" },
> >
> > Not really following why do you want to translate the error message
> > in
> > bluetoothctl and not directly on bluetoothd side? Well perhaps there
> > could be application expecting these strings to be sort of errors
> > code
> > really, in that case perhaps this is valid but I'd rather have it
> > output both error.message and its description, but I would begin by
> > defining them in the documentation:
> >
> > https://github.com/bluez/bluez/blob/master/doc/org.bluez.Device.rst#void-connect
> >
> > Right now we only document the error.code not the error.message
>
> I'd like to give the opportunity to front-ends to still be able to
> differentiate the different errors, but also dump in their logs a
> human-readable version of the error if they don't want to differentiate
> it in the UI.
>
> What do you think of passing:
>
> error code = org.bluez.Error.Failed
> error.message = "BlueZ.Error:br-connection-page-timeout:Device is unpowered or not in range"
>
> This would be a bit similar to how GDBus (the GLib one) encodes object
> error codes over the wire:
> https://gitlab.gnome.org/GNOME/glib/blob/glib-2-80/gio/gdbuserror.c#L410
>
> This means that we can get nicer error messages in bluetoothctl, and
> front-ends like gnome-bluetooth can use the error code to translate it
> to other languages, or present it differently.
>
> What do you think?
I fine with that, like I said for me the error message was sort of
free format but perhaps it is not for some application reading into it
as they expect the string to work as error (sub)code, in that case
changing these string may actually be considered breaking an API,
honestly I don't know if we should consider adding more error codes
and leave the message as free format otherwise this will keep
happening.
> >
> > > };
> > >
> > > const char *error_code_to_str(const char *error_code)
> > > diff --git a/client/main.c b/client/main.c
> > > index 322326ab9b80..0c39e8795762 100644
> > > --- a/client/main.c
> > > +++ b/client/main.c
> > > @@ -30,6 +30,7 @@
> > > #include "gdbus/gdbus.h"
> > > #include "print.h"
> > > #include "agent.h"
> > > +#include "error.h"
> > > #include "gatt.h"
> > > #include "advertising.h"
> > > #include "adv_monitor.h"
> > > @@ -1977,8 +1978,8 @@ static void connect_reply(DBusMessage
> > > *message, void *user_data)
> > > dbus_error_init(&error);
> > >
> > > if (dbus_set_error_from_message(&error, message) == TRUE) {
> > > - bt_shell_printf("Failed to connect: %s %s\n",
> > > error.name,
> > > - error.message);
> > > + bt_shell_printf("Failed to connect: %s: %s\n",
> > > error.name,
> > > + error_code_to_str(error.message));
> > > dbus_error_free(&error);
> > > return bt_shell_noninteractive_quit(EXIT_FAILURE);
> > > }
> > > diff --git a/src/device.c b/src/device.c
> > > index e8bff718c201..9ec6b4d4bd2e 100644
> > > --- a/src/device.c
> > > +++ b/src/device.c
> > > @@ -2477,8 +2477,9 @@ static DBusMessage *connect_profiles(struct
> > > btd_device *dev, uint8_t bdaddr_type
> > > DBG("%s %s, client %s", dev->path, uuid ? uuid : "(all)",
> > >
> > > dbus_message_get_sender(msg));
> > >
> > > - if (dev->pending || dev->connect || dev->browse)
> > > + if (dev->pending || dev->connect || dev->browse) {
> > > return btd_error_in_progress_str(msg,
> > > ERR_BREDR_CONN_BUSY);
> > > + }
> > >
> > > if (!btd_adapter_get_powered(dev->adapter)) {
> > > return btd_error_not_ready_str(msg,
> > > @@ -2497,6 +2498,7 @@ static DBusMessage *connect_profiles(struct
> > > btd_device *dev, uint8_t bdaddr_type
> > > "Connect")
> > > &&
> > > find_service_with_state(dev-
> > > >services,
> > >
> > > BTD_SERVICE_STATE_CONNECTED)) {
> > > + DBG("Already connected to
> > > services");
> > > return
> > > dbus_message_new_method_return(msg);
> > > } else {
> > > return
> > > btd_error_not_available_str(msg,
> > > @@ -2509,8 +2511,10 @@ static DBusMessage *connect_profiles(struct
> > > btd_device *dev, uint8_t bdaddr_type
> > >
> > > err = connect_next(dev);
> > > if (err < 0) {
> > > - if (err == -EALREADY)
> > > + if (err == -EALREADY) {
> > > + DBG("Already connected");
> > > return dbus_message_new_method_return(msg);
> > > + }
> > > return btd_error_failed(msg,
> > >
> > > btd_error_bredr_conn_from_errno(err));
> > > }
> > > @@ -2583,12 +2587,20 @@ static uint8_t select_conn_bearer(struct
> > > btd_device *dev)
> > > return dev->bdaddr_type;
> > > }
> > >
> > > +static const char *bdaddr_type_strs[] = {
> > > + "BR/EDR",
> > > + "LE public",
> > > + "LE random"
> > > +};
> > > +
> > > static DBusMessage *dev_connect(DBusConnection *conn, DBusMessage
> > > *msg,
> > > void
> > > *user_data)
> > > {
> > > struct btd_device *dev = user_data;
> > > uint8_t bdaddr_type;
> > >
> > > + DBG("Calling \"Connect\" for device %s", dev->path);
> > > +
> > > if (dev->bredr_state.connected) {
> > > /*
> > > * Check if services have been resolved and there
> > > is at least
> > > @@ -2596,20 +2608,30 @@ static DBusMessage
> > > *dev_connect(DBusConnection *conn, DBusMessage *msg,
> > > */
> > > if (dev->bredr_state.svc_resolved &&
> > > find_service_with_state(dev->services,
> > > -
> > > BTD_SERVICE_STATE_CONNECTED))
> > > +
> > > BTD_SERVICE_STATE_CONNECTED)) {
> > > bdaddr_type = dev->bdaddr_type;
> > > - else
> > > + DBG("Selecting address type %s, as BR/EDR
> > > services are resolved "
> > > + " and connected", bdaddr_type_strs[dev-
> > > >bdaddr_type]);
> > > + } else {
> > > bdaddr_type = BDADDR_BREDR;
> > > - } else if (dev->le_state.connected && dev->bredr)
> > > + DBG("Selecting address type BR/EDR, as
> > > services not resolved "
> > > + "or not connected");
> > > + }
> > > + } else if (dev->le_state.connected && dev->bredr) {
> > > bdaddr_type = BDADDR_BREDR;
> > > - else
> > > + DBG("Selecting address type BR/EDR, as LE already
> > > connected");
> > > + } else {
> > > bdaddr_type = select_conn_bearer(dev);
> > > + DBG("Selecting address type %s",
> > > bdaddr_type_strs[dev->bdaddr_type]);
> > > + }
> > >
> > > if (bdaddr_type != BDADDR_BREDR) {
> > > int err;
> > >
> > > - if (dev->le_state.connected)
> > > + if (dev->le_state.connected) {
> > > + DBG("Device already connected through LE");
> > > return dbus_message_new_method_return(msg);
> > > + }
> > >
> > > btd_device_set_temporary(dev, false);
> > >
> > > --
> > > 2.47.1
> > >
> > >
> >
> >
>
--
Luiz Augusto von Dentz
