| Message ID | 20210826164211.2936133-1-david@lechnology.com |
|---|---|
| State | New |
| Headers | show |
| Series | [BlueZ] device: add MTU D-Bus property | expand |
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
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. >
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 --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);
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(+)