[PATCH] gatt-server: Implement NegotiatedMTU property on Device1

[PATCH] gatt-server: Implement NegotiatedMTU property on Device1

[Jonah Petri]

It can be useful for programs using GATT to be aware of the maximum
transmittable packet size, as negotiated by bluetoothd.  This change
exposes the negotiated MTU over D-Bus, if such a negotiation has
occurred.
---
 client/main.c            |  1 +
 doc/device-api.txt       |  5 +++++
 src/device.c             | 31 +++++++++++++++++++++++++++++++
 src/shared/gatt-server.c | 15 +++++++++++++++
 src/shared/gatt-server.h |  3 +++
 5 files changed, 55 insertions(+)

diff --git a/client/main.c b/client/main.c
index 255cbd5..6435674 100644
--- a/client/main.c
+++ b/client/main.c
@@ -1372,6 +1372,7 @@ static void cmd_info(const char *arg)
 	print_property(proxy, "ServiceData");
 	print_property(proxy, "RSSI");
 	print_property(proxy, "TxPower");
+	print_property(proxy, "NegotiatedMTU");
 }
=20
 static void pair_reply(DBusMessage *message, void *user_data)
diff --git a/doc/device-api.txt b/doc/device-api.txt
index 13b2881..908616b 100644
--- a/doc/device-api.txt
+++ b/doc/device-api.txt
@@ -234,3 +234,8 @@ Properties	string Address [readonly]
 		array{byte} AdvertisingFlags [readonly, experimental]
=20
 			The Advertising Data Flags of the remote device.
+
+		int16 NegotiatedMTU [readonly, optional, experimental]
+
+			The ATT MTU negotiated with the connected host.
+			Available only once MTU negotiation is complete.
diff --git a/src/device.c b/src/device.c
index 8693eb8..1b171ee 100644
--- a/src/device.c
+++ b/src/device.c
@@ -930,6 +930,35 @@ static gboolean dev_property_exists_tx_power(const =
GDBusPropertyTable *property,
 	return TRUE;
 }
=20
+static gboolean dev_property_get_negotiated_mtu(
+					const GDBusPropertyTable * =
property,
+					DBusMessageIter *iter, void =
*data)
+{
+	const struct btd_device *dev =3D data;
+	dbus_int16_t val =3D 0;
+	int err;
+
+	err =3D bt_gatt_server_get_negotiated_mtu(dev->server, &val);
+	if (err !=3D 0)
+		return FALSE;
+
+	dbus_message_iter_append_basic(iter, DBUS_TYPE_INT16, &val);
+
+	return TRUE;
+}
+
+static gboolean dev_property_exists_negotiated_mtu(
+					const GDBusPropertyTable * =
property,
+					void *data)
+{
+	const struct btd_device *dev =3D data;
+	int err;
+
+	err =3D bt_gatt_server_get_negotiated_mtu(dev->server, NULL);
+
+	return err =3D=3D 0;
+}
+
 static gboolean
 dev_property_get_svc_resolved(const GDBusPropertyTable *property,
 					DBusMessageIter *iter, void =
*data)
@@ -2561,6 +2590,8 @@ static const GDBusPropertyTable =
device_properties[] =3D {
 				NULL, dev_property_service_data_exist },
 	{ "TxPower", "n", dev_property_get_tx_power, NULL,
 					dev_property_exists_tx_power },
+	{ "NegotiatedMTU", "n", dev_property_get_negotiated_mtu, NULL,
+					=
dev_property_exists_negotiated_mtu },
 	{ "ServicesResolved", "b", dev_property_get_svc_resolved, NULL, =
NULL },
 	{ "AdvertisingFlags", "ay", dev_property_get_flags, NULL,
 					dev_property_flags_exist,
diff --git a/src/shared/gatt-server.c b/src/shared/gatt-server.c
index 79e01c8..2727f09 100644
--- a/src/shared/gatt-server.c
+++ b/src/shared/gatt-server.c
@@ -89,6 +89,7 @@ struct bt_gatt_server {
 	struct bt_att *att;
 	int ref_count;
 	uint16_t mtu;
+	bool mtu_negotiated;
=20
 	unsigned int mtu_id;
 	unsigned int read_by_grp_type_id;
@@ -1378,12 +1379,25 @@ static void exchange_mtu_cb(uint8_t opcode, =
const void *pdu,
=20
 	/* Set MTU to be the minimum */
 	server->mtu =3D final_mtu;
+	server->mtu_negotiated =3D true;
 	bt_att_set_mtu(server->att, final_mtu);
=20
 	util_debug(server->debug_callback, server->debug_data,
 			"MTU exchange complete, with MTU: %u", =
final_mtu);
 }
=20
+int bt_gatt_server_get_negotiated_mtu(const struct bt_gatt_server =
*server,
+				int16_t *mtu)
+{
+	if (!server || !server->mtu_negotiated)
+		return -1;
+
+	if (mtu)
+		*mtu =3D server->mtu;
+
+	return 0;
+}
+
 static bool gatt_server_register_att_handlers(struct bt_gatt_server =
*server)
 {
 	/* Exchange MTU */
@@ -1493,6 +1507,7 @@ struct bt_gatt_server *bt_gatt_server_new(struct =
gatt_db *db,
 	server->db =3D gatt_db_ref(db);
 	server->att =3D bt_att_ref(att);
 	server->mtu =3D MAX(mtu, BT_ATT_DEFAULT_LE_MTU);
+	server->mtu_negotiated =3D false;
 	server->max_prep_queue_len =3D DEFAULT_MAX_PREP_QUEUE_LEN;
 	server->prep_queue =3D queue_new();
=20
diff --git a/src/shared/gatt-server.h b/src/shared/gatt-server.h
index 0e480e1..0b99da5 100644
--- a/src/shared/gatt-server.h
+++ b/src/shared/gatt-server.h
@@ -31,6 +31,9 @@ struct bt_gatt_server *bt_gatt_server_new(struct =
gatt_db *db,
 struct bt_gatt_server *bt_gatt_server_ref(struct bt_gatt_server =
*server);
 void bt_gatt_server_unref(struct bt_gatt_server *server);
=20
+int bt_gatt_server_get_negotiated_mtu(const struct bt_gatt_server =
*server,
+					int16_t *mtu);
+
 typedef void (*bt_gatt_server_destroy_func_t)(void *user_data);
 typedef void (*bt_gatt_server_debug_func_t)(const char *str, void =
*user_data);
 typedef void (*bt_gatt_server_conf_func_t)(void *user_data);
--=20
2.10.0

Re: [PATCH] gatt-server: Implement NegotiatedMTU property on Device1

[Luiz Augusto von Dentz]

Hi Jonah,

On Fri, Jul 14, 2017 at 6:56 PM, Jonah Petri <jonah@sense.com> wrote:
> It can be useful for programs using GATT to be aware of the maximum
> transmittable packet size, as negotiated by bluetoothd.  This change
> exposes the negotiated MTU over D-Bus, if such a negotiation has
> occurred.
> ---
>  client/main.c            |  1 +
>  doc/device-api.txt       |  5 +++++
>  src/device.c             | 31 +++++++++++++++++++++++++++++++
>  src/shared/gatt-server.c | 15 +++++++++++++++
>  src/shared/gatt-server.h |  3 +++
>  5 files changed, 55 insertions(+)
>
> diff --git a/client/main.c b/client/main.c
> index 255cbd5..6435674 100644
> --- a/client/main.c
> +++ b/client/main.c
> @@ -1372,6 +1372,7 @@ static void cmd_info(const char *arg)
>         print_property(proxy, "ServiceData");
>         print_property(proxy, "RSSI");
>         print_property(proxy, "TxPower");
> +       print_property(proxy, "NegotiatedMTU");
>  }
>
>  static void pair_reply(DBusMessage *message, void *user_data)
> diff --git a/doc/device-api.txt b/doc/device-api.txt
> index 13b2881..908616b 100644
> --- a/doc/device-api.txt
> +++ b/doc/device-api.txt
> @@ -234,3 +234,8 @@ Properties  string Address [readonly]
>                 array{byte} AdvertisingFlags [readonly, experimental]
>
>                         The Advertising Data Flags of the remote device.
> +
> +               int16 NegotiatedMTU [readonly, optional, experimental]
> +
> +                       The ATT MTU negotiated with the connected host.
> +                       Available only once MTU negotiation is complete.

Despite being odd that we start exposing transport specific details on
device interface, there maybe a chance this is useful if we can
properly detect BR/EDR MTU not only LE, though this means checking the
via bt_att API not bt_gatt_server. Obviously, this would have to be
exposed even before it is negotiated since LE start with 23 but on
BR/EDR this is negotiated via L2CAP signaling, and perhaps it should
not really be MTU that we expose but the actual payload without the
headers that bluetoothd takes care of adding.

> diff --git a/src/device.c b/src/device.c
> index 8693eb8..1b171ee 100644
> --- a/src/device.c
> +++ b/src/device.c
> @@ -930,6 +930,35 @@ static gboolean dev_property_exists_tx_power(const GDBusPropertyTable *property,
>         return TRUE;
>  }
>
> +static gboolean dev_property_get_negotiated_mtu(
> +                                       const GDBusPropertyTable * property,
> +                                       DBusMessageIter *iter, void *data)
> +{
> +       const struct btd_device *dev = data;
> +       dbus_int16_t val = 0;
> +       int err;
> +
> +       err = bt_gatt_server_get_negotiated_mtu(dev->server, &val);
> +       if (err != 0)
> +               return FALSE;
> +
> +       dbus_message_iter_append_basic(iter, DBUS_TYPE_INT16, &val);
> +
> +       return TRUE;
> +}
> +
> +static gboolean dev_property_exists_negotiated_mtu(
> +                                       const GDBusPropertyTable * property,
> +                                       void *data)
> +{
> +       const struct btd_device *dev = data;
> +       int err;
> +
> +       err = bt_gatt_server_get_negotiated_mtu(dev->server, NULL);
> +
> +       return err == 0;
> +}
> +
>  static gboolean
>  dev_property_get_svc_resolved(const GDBusPropertyTable *property,
>                                         DBusMessageIter *iter, void *data)
> @@ -2561,6 +2590,8 @@ static const GDBusPropertyTable device_properties[] = {
>                                 NULL, dev_property_service_data_exist },
>         { "TxPower", "n", dev_property_get_tx_power, NULL,
>                                         dev_property_exists_tx_power },
> +       { "NegotiatedMTU", "n", dev_property_get_negotiated_mtu, NULL,
> +                                       dev_property_exists_negotiated_mtu },

AttMTU or GattMTU would probably indicate better what this is for.

>         { "ServicesResolved", "b", dev_property_get_svc_resolved, NULL, NULL },
>         { "AdvertisingFlags", "ay", dev_property_get_flags, NULL,
>                                         dev_property_flags_exist,
> diff --git a/src/shared/gatt-server.c b/src/shared/gatt-server.c
> index 79e01c8..2727f09 100644
> --- a/src/shared/gatt-server.c
> +++ b/src/shared/gatt-server.c
> @@ -89,6 +89,7 @@ struct bt_gatt_server {
>         struct bt_att *att;
>         int ref_count;
>         uint16_t mtu;
> +       bool mtu_negotiated;
>
>         unsigned int mtu_id;
>         unsigned int read_by_grp_type_id;
> @@ -1378,12 +1379,25 @@ static void exchange_mtu_cb(uint8_t opcode, const void *pdu,
>
>         /* Set MTU to be the minimum */
>         server->mtu = final_mtu;
> +       server->mtu_negotiated = true;

We do have bt_att_*_mtu for this and the ready callback should
indicate when we are done with exchange, or perhaps we want a callback
for when the MTU is changed at bt_att in case we there is a race where
the application wants to write/read immediately, though there will
always be a small race since the attributes can be cached once
connected the application may write/read while exchange is pending.

>         bt_att_set_mtu(server->att, final_mtu);
>
>         util_debug(server->debug_callback, server->debug_data,
>                         "MTU exchange complete, with MTU: %u", final_mtu);
>  }
>
> +int bt_gatt_server_get_negotiated_mtu(const struct bt_gatt_server *server,
> +                               int16_t *mtu)
> +{
> +       if (!server || !server->mtu_negotiated)
> +               return -1;
> +
> +       if (mtu)
> +               *mtu = server->mtu;
> +
> +       return 0;
> +}
> +
>  static bool gatt_server_register_att_handlers(struct bt_gatt_server *server)
>  {
>         /* Exchange MTU */
> @@ -1493,6 +1507,7 @@ struct bt_gatt_server *bt_gatt_server_new(struct gatt_db *db,
>         server->db = gatt_db_ref(db);
>         server->att = bt_att_ref(att);
>         server->mtu = MAX(mtu, BT_ATT_DEFAULT_LE_MTU);
> +       server->mtu_negotiated = false;
>         server->max_prep_queue_len = DEFAULT_MAX_PREP_QUEUE_LEN;
>         server->prep_queue = queue_new();
>
> diff --git a/src/shared/gatt-server.h b/src/shared/gatt-server.h
> index 0e480e1..0b99da5 100644
> --- a/src/shared/gatt-server.h
> +++ b/src/shared/gatt-server.h
> @@ -31,6 +31,9 @@ struct bt_gatt_server *bt_gatt_server_new(struct gatt_db *db,
>  struct bt_gatt_server *bt_gatt_server_ref(struct bt_gatt_server *server);
>  void bt_gatt_server_unref(struct bt_gatt_server *server);
>
> +int bt_gatt_server_get_negotiated_mtu(const struct bt_gatt_server *server,
> +                                       int16_t *mtu);
> +
>  typedef void (*bt_gatt_server_destroy_func_t)(void *user_data);
>  typedef void (*bt_gatt_server_debug_func_t)(const char *str, void *user_data);
>  typedef void (*bt_gatt_server_conf_func_t)(void *user_data);
> --
> 2.10.0
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
Luiz Augusto von Dentz

Re: [PATCH] gatt-server: Implement NegotiatedMTU property on Device1

[Marcel Holtmann]

Hi Luiz,

>> It can be useful for programs using GATT to be aware of the maximum
>> transmittable packet size, as negotiated by bluetoothd.  This change
>> exposes the negotiated MTU over D-Bus, if such a negotiation has
>> occurred.
>> ---
>> client/main.c            |  1 +
>> doc/device-api.txt       |  5 +++++
>> src/device.c             | 31 +++++++++++++++++++++++++++++++
>> src/shared/gatt-server.c | 15 +++++++++++++++
>> src/shared/gatt-server.h |  3 +++
>> 5 files changed, 55 insertions(+)
>> 
>> diff --git a/client/main.c b/client/main.c
>> index 255cbd5..6435674 100644
>> --- a/client/main.c
>> +++ b/client/main.c
>> @@ -1372,6 +1372,7 @@ static void cmd_info(const char *arg)
>>        print_property(proxy, "ServiceData");
>>        print_property(proxy, "RSSI");
>>        print_property(proxy, "TxPower");
>> +       print_property(proxy, "NegotiatedMTU");
>> }
>> 
>> static void pair_reply(DBusMessage *message, void *user_data)
>> diff --git a/doc/device-api.txt b/doc/device-api.txt
>> index 13b2881..908616b 100644
>> --- a/doc/device-api.txt
>> +++ b/doc/device-api.txt
>> @@ -234,3 +234,8 @@ Properties  string Address [readonly]
>>                array{byte} AdvertisingFlags [readonly, experimental]
>> 
>>                        The Advertising Data Flags of the remote device.
>> +
>> +               int16 NegotiatedMTU [readonly, optional, experimental]
>> +
>> +                       The ATT MTU negotiated with the connected host.
>> +                       Available only once MTU negotiation is complete.
> 
> Despite being odd that we start exposing transport specific details on
> device interface, there maybe a chance this is useful if we can
> properly detect BR/EDR MTU not only LE, though this means checking the
> via bt_att API not bt_gatt_server. Obviously, this would have to be
> exposed even before it is negotiated since LE start with 23 but on
> BR/EDR this is negotiated via L2CAP signaling, and perhaps it should
> not really be MTU that we expose but the actual payload without the
> headers that bluetoothd takes care of adding.

I think that exposing max payload is a better idea.

>> diff --git a/src/device.c b/src/device.c
>> index 8693eb8..1b171ee 100644
>> --- a/src/device.c
>> +++ b/src/device.c
>> @@ -930,6 +930,35 @@ static gboolean dev_property_exists_tx_power(const GDBusPropertyTable *property,
>>        return TRUE;
>> }
>> 
>> +static gboolean dev_property_get_negotiated_mtu(
>> +                                       const GDBusPropertyTable * property,
>> +                                       DBusMessageIter *iter, void *data)
>> +{
>> +       const struct btd_device *dev = data;
>> +       dbus_int16_t val = 0;
>> +       int err;
>> +
>> +       err = bt_gatt_server_get_negotiated_mtu(dev->server, &val);
>> +       if (err != 0)
>> +               return FALSE;
>> +
>> +       dbus_message_iter_append_basic(iter, DBUS_TYPE_INT16, &val);
>> +
>> +       return TRUE;
>> +}
>> +
>> +static gboolean dev_property_exists_negotiated_mtu(
>> +                                       const GDBusPropertyTable * property,
>> +                                       void *data)
>> +{
>> +       const struct btd_device *dev = data;
>> +       int err;
>> +
>> +       err = bt_gatt_server_get_negotiated_mtu(dev->server, NULL);
>> +
>> +       return err == 0;
>> +}
>> +
>> static gboolean
>> dev_property_get_svc_resolved(const GDBusPropertyTable *property,
>>                                        DBusMessageIter *iter, void *data)
>> @@ -2561,6 +2590,8 @@ static const GDBusPropertyTable device_properties[] = {
>>                                NULL, dev_property_service_data_exist },
>>        { "TxPower", "n", dev_property_get_tx_power, NULL,
>>                                        dev_property_exists_tx_power },
>> +       { "NegotiatedMTU", "n", dev_property_get_negotiated_mtu, NULL,
>> +                                       dev_property_exists_negotiated_mtu },
> 
> AttMTU or GattMTU would probably indicate better what this is for.

So why not MaxAttributePayload or something along the lines.

Regards

Marcel

Re: [PATCH] gatt-server: Implement NegotiatedMTU property on Device1

[Luiz Augusto von Dentz]

Hi Marcel,

On Sat, Jul 15, 2017 at 9:03 PM, Marcel Holtmann <marcel@holtmann.org> wrote:
> Hi Luiz,
>
>>> It can be useful for programs using GATT to be aware of the maximum
>>> transmittable packet size, as negotiated by bluetoothd.  This change
>>> exposes the negotiated MTU over D-Bus, if such a negotiation has
>>> occurred.
>>> ---
>>> client/main.c            |  1 +
>>> doc/device-api.txt       |  5 +++++
>>> src/device.c             | 31 +++++++++++++++++++++++++++++++
>>> src/shared/gatt-server.c | 15 +++++++++++++++
>>> src/shared/gatt-server.h |  3 +++
>>> 5 files changed, 55 insertions(+)
>>>
>>> diff --git a/client/main.c b/client/main.c
>>> index 255cbd5..6435674 100644
>>> --- a/client/main.c
>>> +++ b/client/main.c
>>> @@ -1372,6 +1372,7 @@ static void cmd_info(const char *arg)
>>>        print_property(proxy, "ServiceData");
>>>        print_property(proxy, "RSSI");
>>>        print_property(proxy, "TxPower");
>>> +       print_property(proxy, "NegotiatedMTU");
>>> }
>>>
>>> static void pair_reply(DBusMessage *message, void *user_data)
>>> diff --git a/doc/device-api.txt b/doc/device-api.txt
>>> index 13b2881..908616b 100644
>>> --- a/doc/device-api.txt
>>> +++ b/doc/device-api.txt
>>> @@ -234,3 +234,8 @@ Properties  string Address [readonly]
>>>                array{byte} AdvertisingFlags [readonly, experimental]
>>>
>>>                        The Advertising Data Flags of the remote device.
>>> +
>>> +               int16 NegotiatedMTU [readonly, optional, experimental]
>>> +
>>> +                       The ATT MTU negotiated with the connected host.
>>> +                       Available only once MTU negotiation is complete.
>>
>> Despite being odd that we start exposing transport specific details on
>> device interface, there maybe a chance this is useful if we can
>> properly detect BR/EDR MTU not only LE, though this means checking the
>> via bt_att API not bt_gatt_server. Obviously, this would have to be
>> exposed even before it is negotiated since LE start with 23 but on
>> BR/EDR this is negotiated via L2CAP signaling, and perhaps it should
>> not really be MTU that we expose but the actual payload without the
>> headers that bluetoothd takes care of adding.
>
> I think that exposing max payload is a better idea.
>
>>> diff --git a/src/device.c b/src/device.c
>>> index 8693eb8..1b171ee 100644
>>> --- a/src/device.c
>>> +++ b/src/device.c
>>> @@ -930,6 +930,35 @@ static gboolean dev_property_exists_tx_power(const GDBusPropertyTable *property,
>>>        return TRUE;
>>> }
>>>
>>> +static gboolean dev_property_get_negotiated_mtu(
>>> +                                       const GDBusPropertyTable * property,
>>> +                                       DBusMessageIter *iter, void *data)
>>> +{
>>> +       const struct btd_device *dev = data;
>>> +       dbus_int16_t val = 0;
>>> +       int err;
>>> +
>>> +       err = bt_gatt_server_get_negotiated_mtu(dev->server, &val);
>>> +       if (err != 0)
>>> +               return FALSE;
>>> +
>>> +       dbus_message_iter_append_basic(iter, DBUS_TYPE_INT16, &val);
>>> +
>>> +       return TRUE;
>>> +}
>>> +
>>> +static gboolean dev_property_exists_negotiated_mtu(
>>> +                                       const GDBusPropertyTable * property,
>>> +                                       void *data)
>>> +{
>>> +       const struct btd_device *dev = data;
>>> +       int err;
>>> +
>>> +       err = bt_gatt_server_get_negotiated_mtu(dev->server, NULL);
>>> +
>>> +       return err == 0;
>>> +}
>>> +
>>> static gboolean
>>> dev_property_get_svc_resolved(const GDBusPropertyTable *property,
>>>                                        DBusMessageIter *iter, void *data)
>>> @@ -2561,6 +2590,8 @@ static const GDBusPropertyTable device_properties[] = {
>>>                                NULL, dev_property_service_data_exist },
>>>        { "TxPower", "n", dev_property_get_tx_power, NULL,
>>>                                        dev_property_exists_tx_power },
>>> +       { "NegotiatedMTU", "n", dev_property_get_negotiated_mtu, NULL,
>>> +                                       dev_property_exists_negotiated_mtu },
>>
>> AttMTU or GattMTU would probably indicate better what this is for.
>
> So why not MaxAttributePayload or something along the lines.

Yep, that sounds much better indeed.

> Regards
>
> Marcel
>



-- 
Luiz Augusto von Dentz

Re: [PATCH] gatt-server: Implement NegotiatedMTU property on Device1

[Jonah Petri]

[-- Attachment #1: Type: text/plain, Size: 1841 bytes --]

Hello Marcel and Luiz,

Thanks for looking at the patch!

> On Jul 15, 2017, at 2:03 PM, Marcel Holtmann <marcel@holtmann.org> wrote:
> 
>>> --- a/doc/device-api.txt
>>> +++ b/doc/device-api.txt
>>> @@ -234,3 +234,8 @@ Properties  string Address [readonly]
>>>               array{byte} AdvertisingFlags [readonly, experimental]
>>> 
>>>                       The Advertising Data Flags of the remote device.
>>> +
>>> +               int16 NegotiatedMTU [readonly, optional, experimental]
>>> +
>>> +                       The ATT MTU negotiated with the connected host.
>>> +                       Available only once MTU negotiation is complete.
>> 
>> Despite being odd that we start exposing transport specific details on
>> device interface, there maybe a chance this is useful if we can
>> properly detect BR/EDR MTU not only LE, though this means checking the
>> via bt_att API not bt_gatt_server. Obviously, this would have to be
>> exposed even before it is negotiated since LE start with 23 but on
>> BR/EDR this is negotiated via L2CAP signaling, and perhaps it should
>> not really be MTU that we expose but the actual payload without the
>> headers that bluetoothd takes care of adding.
> 
> I think that exposing max payload is a better idea.

I am fine with the revised naming, and revising the exposed device attribute to represent the max ATT payload.  However, how can a dbus client know whether the MTU negotiation has occurred or not?  The point of the patch is to allow for clients to take advantage of a larger negotiated MTU.

The negotiation of the MTU can be a significant event to the client interacting via dbus, so how should the completion of that negotiation be exposed, if not via an attribute as I proposed above?  Some boolean "MaxPayloadNegotiationCompleted" method?

Jonah

[-- Attachment #2: Type: text/html, Size: 4461 bytes --]

Re: [PATCH] gatt-server: Implement NegotiatedMTU property on Device1

[Jonah Petri]

Hello Marcel and Luiz,

Thanks for looking at the patch!

> On Jul 15, 2017, at 2:03 PM, Marcel Holtmann <marcel@holtmann.org> =
wrote:
>=20
>>> --- a/doc/device-api.txt
>>> +++ b/doc/device-api.txt
>>> @@ -234,3 +234,8 @@ Properties  string Address [readonly]
>>>               array{byte} AdvertisingFlags [readonly, experimental]
>>>=20
>>>                       The Advertising Data Flags of the remote =
device.
>>> +
>>> +               int16 NegotiatedMTU [readonly, optional, =
experimental]
>>> +
>>> +                       The ATT MTU negotiated with the connected =
host.
>>> +                       Available only once MTU negotiation is =
complete.
>>=20
>> Despite being odd that we start exposing transport specific details =
on
>> device interface, there maybe a chance this is useful if we can
>> properly detect BR/EDR MTU not only LE, though this means checking =
the
>> via bt_att API not bt_gatt_server. Obviously, this would have to be
>> exposed even before it is negotiated since LE start with 23 but on
>> BR/EDR this is negotiated via L2CAP signaling, and perhaps it should
>> not really be MTU that we expose but the actual payload without the
>> headers that bluetoothd takes care of adding.
>=20
> I think that exposing max payload is a better idea.

I am fine with the revised naming, and revising the exposed device =
attribute to represent the max ATT payload.  However, how can a dbus =
client know whether the MTU negotiation has occurred or not?  The point =
of the patch is to allow for clients to take advantage of a larger =
negotiated MTU.

The negotiation of the MTU can be a significant event to the client =
interacting via dbus, so how should the completion of that negotiation =
be exposed, if not via an attribute as I proposed above?  Some boolean =
"MaxPayloadNegotiationCompleted" method?

Jonah=

Re: [PATCH] gatt-server: Implement NegotiatedMTU property on Device1

[Marcel Holtmann]

Hi Jonah,

>>>> --- a/doc/device-api.txt
>>>> +++ b/doc/device-api.txt
>>>> @@ -234,3 +234,8 @@ Properties  string Address [readonly]
>>>>              array{byte} AdvertisingFlags [readonly, experimental]
>>>> 
>>>>                      The Advertising Data Flags of the remote device.
>>>> +
>>>> +               int16 NegotiatedMTU [readonly, optional, experimental]
>>>> +
>>>> +                       The ATT MTU negotiated with the connected host.
>>>> +                       Available only once MTU negotiation is complete.
>>> 
>>> Despite being odd that we start exposing transport specific details on
>>> device interface, there maybe a chance this is useful if we can
>>> properly detect BR/EDR MTU not only LE, though this means checking the
>>> via bt_att API not bt_gatt_server. Obviously, this would have to be
>>> exposed even before it is negotiated since LE start with 23 but on
>>> BR/EDR this is negotiated via L2CAP signaling, and perhaps it should
>>> not really be MTU that we expose but the actual payload without the
>>> headers that bluetoothd takes care of adding.
>> 
>> I think that exposing max payload is a better idea.
> 
> I am fine with the revised naming, and revising the exposed device attribute to represent the max ATT payload.  However, how can a dbus client know whether the MTU negotiation has occurred or not?  The point of the patch is to allow for clients to take advantage of a larger negotiated MTU.
> 
> The negotiation of the MTU can be a significant event to the client interacting via dbus, so how should the completion of that negotiation be exposed, if not via an attribute as I proposed above?  Some boolean "MaxPayloadNegotiationCompleted" method?

there will be PropertiesChanged signal once the MTU has been altered. Otherwise it will start out with 23 in case of LE or the minimum of whatever L2CAP has configured for BR/EDR. We do the same for RFCOMM btw. where it takes the minimum value from the two MTUs.

Regards

Marcel

Re: [PATCH] gatt-server: Implement NegotiatedMTU property on Device1

[Jonah Petri]

Hi Marcel,

> On Jul 17, 2017, at 2:08 PM, Marcel Holtmann <marcel@holtmann.org> =
wrote:
>=20
>>> I think that exposing max payload is a better idea.
>>=20
>> I am fine with the revised naming, and revising the exposed device =
attribute to represent the max ATT payload.  However, how can a dbus =
client know whether the MTU negotiation has occurred or not?  The point =
of the patch is to allow for clients to take advantage of a larger =
negotiated MTU.
>>=20
>> The negotiation of the MTU can be a significant event to the client =
interacting via dbus, so how should the completion of that negotiation =
be exposed, if not via an attribute as I proposed above?  Some boolean =
"MaxPayloadNegotiationCompleted" method?
>=20
> there will be PropertiesChanged signal once the MTU has been altered. =
Otherwise it will start out with 23 in case of LE or the minimum of =
whatever L2CAP has configured for BR/EDR. We do the same for RFCOMM btw. =
where it takes the minimum value from the two MTUs.
>=20

I understand.  However, there is no way for a dbus client to tell the =
difference between these two scenarios:

1) The MTU has been negotiated to 23  (in the case of LE)
2) The MTU negotiation has not been completed

I expect many clients making use of this information will want to wait =
until the MTU negotiation is complete, and then begin normal operation.  =
A single attribute representing the current MTU seems to not be =
sufficient to implement such a dbus client.

Jonah=

Re: [PATCH] gatt-server: Implement NegotiatedMTU property on Device1

[Marcel Holtmann]

Hi Jonah,

>>>> I think that exposing max payload is a better idea.
>>> 
>>> I am fine with the revised naming, and revising the exposed device attribute to represent the max ATT payload.  However, how can a dbus client know whether the MTU negotiation has occurred or not?  The point of the patch is to allow for clients to take advantage of a larger negotiated MTU.
>>> 
>>> The negotiation of the MTU can be a significant event to the client interacting via dbus, so how should the completion of that negotiation be exposed, if not via an attribute as I proposed above?  Some boolean "MaxPayloadNegotiationCompleted" method?
>> 
>> there will be PropertiesChanged signal once the MTU has been altered. Otherwise it will start out with 23 in case of LE or the minimum of whatever L2CAP has configured for BR/EDR. We do the same for RFCOMM btw. where it takes the minimum value from the two MTUs.
>> 
> 
> I understand.  However, there is no way for a dbus client to tell the difference between these two scenarios:
> 
> 1) The MTU has been negotiated to 23  (in the case of LE)
> 2) The MTU negotiation has not been completed
> 
> I expect many clients making use of this information will want to wait until the MTU negotiation is complete, and then begin normal operation.  A single attribute representing the current MTU seems to not be sufficient to implement such a dbus client.

the other option is to leave the property out until the MTU has been negotiated. So in case of LE and Exchange MTU has not yet happened, that property does not exist. In BR/EDR it will always exist since L2CAP has finished already.

So clients can start whenever they see the PropertiesChanged signal or get the property in the first place.

Regards

Marcel

Re: [PATCH] gatt-server: Implement NegotiatedMTU property on Device1

[Jonah Petri]

[-- Attachment #1: Type: text/plain, Size: 1578 bytes --]

Hi Marcel,

> On Jul 18, 2017, at 2:42 AM, Marcel Holtmann <marcel@holtmann.org> wrote:
> 
>>>> The negotiation of the MTU can be a significant event to the client interacting via dbus, so how should the completion of that negotiation be exposed, if not via an attribute as I proposed above?  Some boolean "MaxPayloadNegotiationCompleted" method?
>>> 
>>> there will be PropertiesChanged signal once the MTU has been altered. Otherwise it will start out with 23 in case of LE or the minimum of whatever L2CAP has configured for BR/EDR. We do the same for RFCOMM btw. where it takes the minimum value from the two MTUs.
>>> 
>> 
>> I understand.  However, there is no way for a dbus client to tell the difference between these two scenarios:
>> 
>> 1) The MTU has been negotiated to 23  (in the case of LE)
>> 2) The MTU negotiation has not been completed
>> 
>> I expect many clients making use of this information will want to wait until the MTU negotiation is complete, and then begin normal operation.  A single attribute representing the current MTU seems to not be sufficient to implement such a dbus client.
> 
> the other option is to leave the property out until the MTU has been negotiated. So in case of LE and Exchange MTU has not yet happened, that property does not exist. In BR/EDR it will always exist since L2CAP has finished already.
> 
> So clients can start whenever they see the PropertiesChanged signal or get the property in the first place.
> 

This seems fine to me.  Thanks for the feedback.  I will submit a new patch.

Jonah


[-- Attachment #2: Type: text/html, Size: 4720 bytes --]

Re: [PATCH] gatt-server: Implement NegotiatedMTU property on Device1

[Jonah Petri]

Hi Marcel,

> On Jul 18, 2017, at 2:42 AM, Marcel Holtmann <marcel@holtmann.org> =
wrote:
>=20
>>>> The negotiation of the MTU can be a significant event to the client =
interacting via dbus, so how should the completion of that negotiation =
be exposed, if not via an attribute as I proposed above?  Some boolean =
"MaxPayloadNegotiationCompleted" method?
>>>=20
>>> there will be PropertiesChanged signal once the MTU has been =
altered. Otherwise it will start out with 23 in case of LE or the =
minimum of whatever L2CAP has configured for BR/EDR. We do the same for =
RFCOMM btw. where it takes the minimum value from the two MTUs.
>>>=20
>>=20
>> I understand.  However, there is no way for a dbus client to tell the =
difference between these two scenarios:
>>=20
>> 1) The MTU has been negotiated to 23  (in the case of LE)
>> 2) The MTU negotiation has not been completed
>>=20
>> I expect many clients making use of this information will want to =
wait until the MTU negotiation is complete, and then begin normal =
operation.  A single attribute representing the current MTU seems to not =
be sufficient to implement such a dbus client.
>=20
> the other option is to leave the property out until the MTU has been =
negotiated. So in case of LE and Exchange MTU has not yet happened, that =
property does not exist. In BR/EDR it will always exist since L2CAP has =
finished already.
>=20
> So clients can start whenever they see the PropertiesChanged signal or =
get the property in the first place.
>=20

This seems fine to me.  Thanks for the feedback.  I will submit a new =
patch.

Jonah

Re: [PATCH] gatt-server: Implement NegotiatedMTU property on Device1

[Olivier MARTIN]

On 14.07.2017 17:56, Jonah Petri wrote:
> It can be useful for programs using GATT to be aware of the maximum
> transmittable packet size, as negotiated by bluetoothd.  This change
> exposes the negotiated MTU over D-Bus, if such a negotiation has
> occurred.
> ---
>  client/main.c            |  1 +
>  doc/device-api.txt       |  5 +++++
>  src/device.c             | 31 +++++++++++++++++++++++++++++++
>  src/shared/gatt-server.c | 15 +++++++++++++++
>  src/shared/gatt-server.h |  3 +++
>  5 files changed, 55 insertions(+)
> 
> diff --git a/client/main.c b/client/main.c
> index 255cbd5..6435674 100644
> --- a/client/main.c
> +++ b/client/main.c
> @@ -1372,6 +1372,7 @@ static void cmd_info(const char *arg)
>  	print_property(proxy, "ServiceData");
>  	print_property(proxy, "RSSI");
>  	print_property(proxy, "TxPower");
> +	print_property(proxy, "NegotiatedMTU");
>  }
> 
>  static void pair_reply(DBusMessage *message, void *user_data)
> diff --git a/doc/device-api.txt b/doc/device-api.txt
> index 13b2881..908616b 100644
> --- a/doc/device-api.txt
> +++ b/doc/device-api.txt
> @@ -234,3 +234,8 @@ Properties	string Address [readonly]
>  		array{byte} AdvertisingFlags [readonly, experimental]
> 
>  			The Advertising Data Flags of the remote device.
> +
> +		int16 NegotiatedMTU [readonly, optional, experimental]
> +
> +			The ATT MTU negotiated with the connected host.
> +			Available only once MTU negotiation is complete.
> diff --git a/src/device.c b/src/device.c
> index 8693eb8..1b171ee 100644
> --- a/src/device.c
> +++ b/src/device.c
> @@ -930,6 +930,35 @@ static gboolean
> dev_property_exists_tx_power(const GDBusPropertyTable *property,
>  	return TRUE;
>  }
> 
> +static gboolean dev_property_get_negotiated_mtu(
> +					const GDBusPropertyTable * property,
> +					DBusMessageIter *iter, void *data)
> +{
> +	const struct btd_device *dev = data;
> +	dbus_int16_t val = 0;
> +	int err;
> +
> +	err = bt_gatt_server_get_negotiated_mtu(dev->server, &val);
> +	if (err != 0)
> +		return FALSE;
> +
> +	dbus_message_iter_append_basic(iter, DBUS_TYPE_INT16, &val);
> +
> +	return TRUE;
> +}
> +
> +static gboolean dev_property_exists_negotiated_mtu(
> +					const GDBusPropertyTable * property,
> +					void *data)
> +{
> +	const struct btd_device *dev = data;
> +	int err;
> +
> +	err = bt_gatt_server_get_negotiated_mtu(dev->server, NULL);
> +
> +	return err == 0;
> +}
> +
>  static gboolean
>  dev_property_get_svc_resolved(const GDBusPropertyTable *property,
>  					DBusMessageIter *iter, void *data)
> @@ -2561,6 +2590,8 @@ static const GDBusPropertyTable 
> device_properties[] = {
>  				NULL, dev_property_service_data_exist },
>  	{ "TxPower", "n", dev_property_get_tx_power, NULL,
>  					dev_property_exists_tx_power },
> +	{ "NegotiatedMTU", "n", dev_property_get_negotiated_mtu, NULL,
> +					dev_property_exists_negotiated_mtu },
>  	{ "ServicesResolved", "b", dev_property_get_svc_resolved, NULL, NULL 
> },
>  	{ "AdvertisingFlags", "ay", dev_property_get_flags, NULL,
>  					dev_property_flags_exist,
> diff --git a/src/shared/gatt-server.c b/src/shared/gatt-server.c
> index 79e01c8..2727f09 100644
> --- a/src/shared/gatt-server.c
> +++ b/src/shared/gatt-server.c
> @@ -89,6 +89,7 @@ struct bt_gatt_server {
>  	struct bt_att *att;
>  	int ref_count;
>  	uint16_t mtu;
> +	bool mtu_negotiated;
> 
>  	unsigned int mtu_id;
>  	unsigned int read_by_grp_type_id;
> @@ -1378,12 +1379,25 @@ static void exchange_mtu_cb(uint8_t opcode,
> const void *pdu,
> 
>  	/* Set MTU to be the minimum */
>  	server->mtu = final_mtu;
> +	server->mtu_negotiated = true;
>  	bt_att_set_mtu(server->att, final_mtu);
> 
>  	util_debug(server->debug_callback, server->debug_data,
>  			"MTU exchange complete, with MTU: %u", final_mtu);
>  }
> 
> +int bt_gatt_server_get_negotiated_mtu(const struct bt_gatt_server 
> *server,
> +				int16_t *mtu)
> +{
> +	if (!server || !server->mtu_negotiated)
> +		return -1;
> +
> +	if (mtu)
> +		*mtu = server->mtu;
> +
> +	return 0;
> +}
> +
>  static bool gatt_server_register_att_handlers(struct bt_gatt_server 
> *server)
>  {
>  	/* Exchange MTU */
> @@ -1493,6 +1507,7 @@ struct bt_gatt_server *bt_gatt_server_new(struct
> gatt_db *db,
>  	server->db = gatt_db_ref(db);
>  	server->att = bt_att_ref(att);
>  	server->mtu = MAX(mtu, BT_ATT_DEFAULT_LE_MTU);
> +	server->mtu_negotiated = false;
>  	server->max_prep_queue_len = DEFAULT_MAX_PREP_QUEUE_LEN;
>  	server->prep_queue = queue_new();
> 
> diff --git a/src/shared/gatt-server.h b/src/shared/gatt-server.h
> index 0e480e1..0b99da5 100644
> --- a/src/shared/gatt-server.h
> +++ b/src/shared/gatt-server.h
> @@ -31,6 +31,9 @@ struct bt_gatt_server *bt_gatt_server_new(struct 
> gatt_db *db,
>  struct bt_gatt_server *bt_gatt_server_ref(struct bt_gatt_server 
> *server);
>  void bt_gatt_server_unref(struct bt_gatt_server *server);
> 
> +int bt_gatt_server_get_negotiated_mtu(const struct bt_gatt_server 
> *server,
> +					int16_t *mtu);
> +
>  typedef void (*bt_gatt_server_destroy_func_t)(void *user_data);
>  typedef void (*bt_gatt_server_debug_func_t)(const char *str, void 
> *user_data);
>  typedef void (*bt_gatt_server_conf_func_t)(void *user_data);

When I started to do the same exercise last week, I exposed two 
different MTUs: GATT Server MTU (in GATT Manager DBUS API) and GATT 
Client MTU (in Device DBUS API) as I had the impression they were 
different.
Instead of only exposing the negociated MTU I also exposed the current 
MTU (ie: the value before being negociated). The idea was to send a DBUS 
Signal when a new MTU was negociated to tell DBus-based applications a 
new MTU has been negociated.

Re: [PATCH] gatt-server: Implement NegotiatedMTU property on Device1

[Emil Lenngren]

2017-07-17 16:07 GMT+02:00 Olivier MARTIN <olivier@labapart.com>:

> When I started to do the same exercise last week, I exposed two different MTUs: GATT Server MTU (in GATT Manager DBUS API) and GATT Client MTU (in Device DBUS API) as I had the impression they were different.
> Instead of only exposing the negociated MTU I also exposed the current MTU (ie: the value before being negociated). The idea was to send a DBUS Signal when a new MTU was negociated to tell DBus-based applications a new MTU has been negociated.


As you can read in the standard (Core 5.0) at Vol 3, Part F, Chapter 3.4.2.2,
"If a device is both a client and a server, the following rules shall apply:
1. A device's Exchange MTU Request shall contain the same MTU as the
device's Exchange MTU Response (i.e. the MTU shall be symmetric).
3. It is permitted, (but not necessary - see 2.) to exchange MTU in both
directions, but the MTUs shall be the same in each direction (see 1.)"

So for a given link, the MTU is same for both being a client and server.

Also make sure there are no race conditions in the way that an app
sends some value with the wrong MTU since it is just being updated.
(There is a large list of "gotchas" in 3.4.2.2 as well as in figure
3.1).

Re: [PATCH] gatt-server: Implement NegotiatedMTU property on Device1

[Luiz Augusto von Dentz]

Hi Emil, Olivier,

On Mon, Jul 17, 2017 at 5:59 PM, Emil Lenngren <emil.lenngren@gmail.com> wrote:
> 2017-07-17 16:07 GMT+02:00 Olivier MARTIN <olivier@labapart.com>:
>
>> When I started to do the same exercise last week, I exposed two different MTUs: GATT Server MTU (in GATT Manager DBUS API) and GATT Client MTU (in Device DBUS API) as I had the impression they were different.
>> Instead of only exposing the negociated MTU I also exposed the current MTU (ie: the value before being negociated). The idea was to send a DBUS Signal when a new MTU was negociated to tell DBus-based applications a new MTU has been negociated.
>
>
> As you can read in the standard (Core 5.0) at Vol 3, Part F, Chapter 3.4.2.2,
> "If a device is both a client and a server, the following rules shall apply:
> 1. A device's Exchange MTU Request shall contain the same MTU as the
> device's Exchange MTU Response (i.e. the MTU shall be symmetric).
> 3. It is permitted, (but not necessary - see 2.) to exchange MTU in both
> directions, but the MTUs shall be the same in each direction (see 1.)"
>
> So for a given link, the MTU is same for both being a client and server.

As long as it is and LE link you are correct, but ATT over BR/EDR uses
L2CAP signaling to negotiate the MTU which does not impose the same
rules as above, so ultimately the ATT can be different, luckily the
AcquireWrite and AcquireRead don't need changing since we can have
different MTU for each direction.

> Also make sure there are no race conditions in the way that an app
> sends some value with the wrong MTU since it is just being updated.
> (There is a large list of "gotchas" in 3.4.2.2 as well as in figure
> 3.1).

And this gotcha of the MTU may not be symmetric over BR/EDR is not
even there, no wonder most OSes get it wrong, this maybe the reason
with some Androids, perhaps most, ATT/GATT don't work over BR/EDR
since they might assume MTU of 23 bytes.

> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
Luiz Augusto von Dentz