diff mbox series

[BlueZ] device: add MTU D-Bus property

Message ID 20210826164211.2936133-1-david@lechnology.com (mailing list archive)
State New, archived
Headers show
Series [BlueZ] device: add MTU D-Bus property | expand

Commit Message

David Lechner Aug. 26, 2021, 4:42 p.m. UTC
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].

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.

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

Comments

Luiz Augusto von Dentz Aug. 27, 2021, 1:17 a.m. UTC | #1
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
>
David Lechner Aug. 27, 2021, 3:19 p.m. UTC | #2
On 8/26/21 8:17 PM, Luiz Augusto von Dentz wrote:
> 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

Access to the MTU has been a highly requested feature in
WebBluetooth[1] with reasonable use cases.

[1]: https://github.com/WebBluetoothCG/web-bluetooth/issues/383

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

I'm coming at this from the cross-platform point of view. Windows,
Apple and Android don't have an equivalent of AcquireWrite. They have
device-level methods/properties to get the MTU [2][3][4]. So instead
of forcing WebBluetooth to have an API compatible with AcquireWrite
it would be very nice to be able to get the required information
in a different way that is similar to the existing methods on other
platforms.

Maybe there are better alternatives to get the same information?
For example, the Apple API doesn't actually mention MTU, but rather
allows you to get the maximum write size for Write Without Response,
which I think is the value most use cases need (it just happens to
be MTU - 3).

[2]: https://docs.microsoft.com/en-us/uwp/api/windows.devices.bluetooth.genericattributeprofile.gattsession.maxpdusize?view=winrt-19041
[3]: https://developer.apple.com/documentation/corebluetooth/cbperipheral/1620312-maximumwritevaluelength
[4]: https://developer.android.com/reference/android/bluetooth/BluetoothGattCallback.html#onMtuChanged(android.bluetooth.BluetoothGatt,%20int,%20int)

> 
>> 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.
>
bluez.test.bot@gmail.com Aug. 27, 2021, 11:55 p.m. UTC | #3
This is automated email and please do not reply to this email!

Dear submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=537891

---Test result---

Test Summary:
CheckPatch                    FAIL      0.41 seconds
GitLint                       PASS      0.12 seconds
Prep - Setup ELL              PASS      45.55 seconds
Build - Prep                  PASS      0.15 seconds
Build - Configure             PASS      8.05 seconds
Build - Make                  PASS      198.09 seconds
Make Check                    PASS      8.91 seconds
Make Distcheck                PASS      234.38 seconds
Build w/ext ELL - Configure   PASS      8.19 seconds
Build w/ext ELL - Make        PASS      186.01 seconds

Details
##############################
Test: CheckPatch - FAIL
Desc: Run checkpatch.pl script with rule in .checkpatch.conf
Output:
device: add MTU D-Bus property
WARNING:LINE_SPACING: Missing a blank line after declarations
#62: FILE: src/device.c:1481:
+	dbus_uint16_t mtu = bt_gatt_client_get_mtu(device->client);
+	dbus_message_iter_append_basic(iter, DBUS_TYPE_UINT16, &mtu);

- total: 0 errors, 1 warnings, 56 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

"[PATCH] device: add MTU D-Bus property" has style problems, please review.

NOTE: Ignored message types: COMMIT_MESSAGE COMPLEX_MACRO CONST_STRUCT FILE_PATH_CHANGES MISSING_SIGN_OFF PREFER_PACKED SPDX_LICENSE_TAG SPLIT_STRING SSCANF_TO_KSTRTO

NOTE: If any of the errors are false positives, please report
      them to the maintainer, see CHECKPATCH in MAINTAINERS.


##############################
Test: GitLint - PASS
Desc: Run gitlint with rule in .gitlint

##############################
Test: Prep - Setup ELL - PASS
Desc: Clone, build, and install ELL

##############################
Test: Build - Prep - PASS
Desc: Prepare environment for build

##############################
Test: Build - Configure - PASS
Desc: Configure the BlueZ source tree

##############################
Test: Build - Make - PASS
Desc: Build the BlueZ source tree

##############################
Test: Make Check - PASS
Desc: Run 'make check'

##############################
Test: Make Distcheck - PASS
Desc: Run distcheck to check the distribution

##############################
Test: Build w/ext ELL - Configure - PASS
Desc: Configure BlueZ source with '--enable-external-ell' configuration

##############################
Test: Build w/ext ELL - Make - PASS
Desc: Build BlueZ source with '--enable-external-ell' configuration



---
Regards,
Linux Bluetooth
diff mbox series

Patch

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);