Received-SPF: pass (google.com: domain of linux-bluetooth-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210826164211.2936133-1-david@lechnology.com>
In-Reply-To: <20210826164211.2936133-1-david@lechnology.com>
From:   Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date:   Thu, 26 Aug 2021 18:17:00 -0700
Message-ID: <CABBYNZK9-0b0O_FOzYQ8+iB-dDqC0RHxLCBkN8TibR2opJoD=A@mail.gmail.com>
Subject: Re: [PATCH BlueZ] device: add MTU D-Bus property
To:     David Lechner <david@lechnology.com>
Cc:     "linux-bluetooth@vger.kernel.org" <linux-bluetooth@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi David,

On Thu, Aug 26, 2021 at 9:45 AM David Lechner <david@lechnology.com> wrote:
>
> When using GATT write without response, it is useful to know how much
> data can be sent in a single write. This value is the negotiated MTU
> minus 3 bytes.
>
> The D-bus method org.bluez.GattCharacteristic1.AcquireWrite returns the
> MTU exactly for this reason. However, when using the alternate API
> org.bluez.GattCharacteristic1.WriteValue with the options dictionary
> { "type": "command" }, there is no current way to get the MTU value.
> If the value is too large, then the method returns "Failed to initiate
> write" [org.bluez.Error.Failed].

In most cases the MTU is not that useful since each attribute has a
maximum length of just a few bytes, the MTU is only really useful for
control points which I rather have using Acquire* variants. Note that
for long values the attribute must support Write Long Procedure and
afaik WriteValue does support that so it can fragment the data and
send according to the MTU.

> This adds an "MTU" property to the org.bluez.Device1 interface that
> is emitted in gatt_client_ready_cb() which is after the MTU exchange
> has taken place.

This would not work for the likes of EATT which don't require rx and
tx MTU to be symmetric, with the likes of Acquire we could in theory
even assign a dedicated EATT channel if we have to.

> Signed-off-by: David Lechner <david@lechnology.com>
> ---
>  client/main.c      |  1 +
>  doc/device-api.txt |  4 ++++
>  src/device.c       | 24 ++++++++++++++++++++++++
>  3 files changed, 29 insertions(+)
>
> diff --git a/client/main.c b/client/main.c
> index 506602bbd..b12a7da3e 100644
> --- a/client/main.c
> +++ b/client/main.c
> @@ -1754,6 +1754,7 @@ static void cmd_info(int argc, char *argv[])
>         print_property(proxy, "TxPower");
>         print_property(proxy, "AdvertisingFlags");
>         print_property(proxy, "AdvertisingData");
> +       print_property(proxy, "MTU");
>
>         battery_proxy = find_proxies_by_path(battery_proxies,
>                                         g_dbus_proxy_get_path(proxy));
> diff --git a/doc/device-api.txt b/doc/device-api.txt
> index 4e824d2de..030873821 100644
> --- a/doc/device-api.txt
> +++ b/doc/device-api.txt
> @@ -272,3 +272,7 @@ Properties  string Address [readonly]
>                         Example:
>                                 <Transport Discovery> <Organization Flags...>
>                                 0x26                   0x01         0x01...
> +
> +               uint16 MTU [readonly, optional]
> +
> +                       The exchanged MTU (GATT client only).
> diff --git a/src/device.c b/src/device.c
> index 26a01612a..898f98da7 100644
> --- a/src/device.c
> +++ b/src/device.c
> @@ -1471,6 +1471,26 @@ static gboolean dev_property_wake_allowed_exist(
>         return device_get_wake_support(device);
>  }
>
> +static gboolean
> +dev_property_get_mtu(const GDBusPropertyTable *property,
> +                    DBusMessageIter *iter, void *data)
> +{
> +       struct btd_device *device = data;
> +
> +       dbus_uint16_t mtu = bt_gatt_client_get_mtu(device->client);
> +       dbus_message_iter_append_basic(iter, DBUS_TYPE_UINT16, &mtu);
> +
> +       return TRUE;
> +}
> +
> +static gboolean
> +dev_property_mtu_exist(const GDBusPropertyTable *property, void *data)
> +{
> +       struct btd_device *device = data;
> +
> +       return bt_gatt_client_get_mtu(device->client) != 0;
> +}
> +
>  static bool disconnect_all(gpointer user_data)
>  {
>         struct btd_device *device = user_data;
> @@ -3014,6 +3034,7 @@ static const GDBusPropertyTable device_properties[] = {
>         { "WakeAllowed", "b", dev_property_get_wake_allowed,
>                                 dev_property_set_wake_allowed,
>                                 dev_property_wake_allowed_exist },
> +       { "MTU", "q", dev_property_get_mtu, NULL, dev_property_mtu_exist },
>         { }
>  };
>
> @@ -5245,6 +5266,9 @@ static void gatt_client_ready_cb(bool success, uint8_t att_ecode,
>                 return;
>         }
>
> +       g_dbus_emit_property_changed(dbus_conn, device->path,
> +                                       DEVICE_INTERFACE, "MTU");
> +
>         register_gatt_services(device);
>
>         btd_gatt_client_ready(device->client_dbus);
> --
> 2.25.1
>


-- 
Luiz Augusto von Dentz