weixin_39794130
weixin_39794130
2021-01-10 05:14

mcumgr: smp_bt: wrong notify MTU calculation with CONFIG_BT_GATT_NOTIFY_MULTIPLE

Describe the bug It is probably not the source of the bug, but currently the MTU calculation for fragmentet bt_smp packets fails to calculate the correct avaliable data length. The length is calculated as

MTU - 3
https://github.com/zephyrproject-rtos/zephyr/blob/1b1d728a188ddfb0c4252ec7f389fb2be71816ee/subsys/mgmt/smp_bt.c#L136

but

bt_gatt_notify
needs 5 additional bytes instead of 3 if CONFIG_BT_GATT_NOTIFY_MULTIPLE is set.

https://github.com/zephyrproject-rtos/zephyr/blob/1b1d728a188ddfb0c4252ec7f389fb2be71816ee/subsys/bluetooth/host/gatt.c#L1721

Discovered with the mcumgr image list command that generates a response of length 258, when both slots are used (see below for patch with additional log messages).


[00:00:17.537,872] <wrn> bt_att: No ATT channel for MTU 263, max 261
[00:00:17.537,872] <wrn> bt_gatt: No buffer available to send notification, mult
</wrn></wrn>

I assume the bug is actually using the

gatt_notify_mult
, even though
bt_gatt_notify
is called with the conn obj in https://github.com/zephyrproject-rtos/zephyr/blob/1b1d728a188ddfb0c4252ec7f389fb2be71816ee/subsys/mgmt/smp_bt.c#L101?

Workarounds 1. Increase CONFIG_BT_L2CAP_TX_MTU to prevent fragmentation of bt_smp responses (>= 263?) 2. Disable CONFIG_BT_GATT_NOTIFY_MULTIPLE 3. Change

mtu - 3
to
mtu - 5
in
bt_smp.c:smp_bt_get_mtu

To Reproduce Steps to reproduce the behavior: 1. mkdir build; cd build 2. cmake -DBOARD=board_xyz samples/subsys/mgmt/mcumgr/smp_svr/ 3. make 4. mcumgr image list 5. see error

Expected behavior bt_smp does not (always) fail to send notifications/command responses

Impact None for me, as single connection only does not need notify multiple. Otherwise pretty bad.

Screenshots or console output


CONFIG_BT_GATT_NOTIFY_MULTIPLE=y
CONFIG_BT_L2CAP_TX_MTU=261

[00:00:17.537,841] <wrn> bt_att: MTU: 261
[00:00:17.537,841] <wrn> mcumgr: conn 0x200026ec, len: 258
[00:00:17.537,872] <wrn> bt_gatt: Buff len(nfy) 4 + params len: 258
[00:00:17.537,872] <wrn> bt_att: No ATT channel for MTU 263, max 261
[00:00:17.537,872] <wrn> bt_gatt: No buffer available to send notification, mult
</wrn></wrn></wrn></wrn></wrn>

or


# CONFIG_BT_GATT_NOTIFY_MULTIPLE is not set
CONFIG_BT_L2CAP_TX_MTU=261

[00:00:36.543,884] <wrn> bt_att: MTU: 261
[00:00:36.543,884] <err> mcumgr: conn 0x200026e4, len: 258
</err></wrn>

Environment (please complete the following information): - OS: MacOS - Toolchain gnuarmemb - zephyr v2.3.0 and master

Additional context Changes for logging:


diff --git a/subsys/bluetooth/host/att.c b/subsys/bluetooth/host/att.c
index 06a7fd0daa..6490b5cc7c 100644
--- a/subsys/bluetooth/host/att.c
+++ b/subsys/bluetooth/host/att.c
@@ -2357,6 +2357,7 @@ struct net_buf *bt_att_create_pdu(struct bt_conn *conn, u8_t op, size_t len)
 {
    struct bt_att *att;
    struct bt_att_chan *chan, *tmp;
+   u16_t max_mtu = 0;

    att = att_get(conn);
    if (!att) {
@@ -2365,13 +2366,14 @@ struct net_buf *bt_att_create_pdu(struct bt_conn *conn, u8_t op, size_t len)

    SYS_SLIST_FOR_EACH_CONTAINER_SAFE(&att->chans, chan, tmp, node) {
        if (len + sizeof(op) > chan->chan.tx.mtu) {
+           max_mtu = MAX(chan->chan.tx.mtu, max_mtu);
            continue;
        }

        return bt_att_chan_create_pdu(chan, op, len);
    }

-   BT_WARN("No ATT channel for MTU %zu", len + sizeof(op));
+   BT_WARN("No ATT channel for MTU %zu, max %d", len + sizeof(op), max_mtu);

    return NULL;
 }
@@ -2783,7 +2785,7 @@ u16_t bt_att_get_mtu(struct bt_conn *conn)
            mtu = chan->chan.tx.mtu;
        }
    }
-
+   BT_WARN("MTU: %d", mtu);
    return mtu;
 }

diff --git a/subsys/bluetooth/host/gatt.c b/subsys/bluetooth/host/gatt.c
index 12d3d88542..f649f3ce5b 100644
--- a/subsys/bluetooth/host/gatt.c
+++ b/subsys/bluetooth/host/gatt.c
@@ -1715,12 +1715,12 @@ static int gatt_notify_mult(struct bt_conn *conn, u16_t handle,
            return ret;
        }
    }
-
+   BT_WARN("Buff len(nfy) %d + params len: %d", sizeof(*nfy),  params->len);
    if (!*buf) {
        *buf = bt_att_create_pdu(conn, BT_ATT_OP_NOTIFY_MULT,
                     sizeof(*nfy) + params->len);
        if (!*buf) {
-           BT_WARN("No buffer available to send notification");
+           BT_WARN("No buffer available to send notification, mult");
            return -ENOMEM;
        }
        /* Set user_data so it can be restored when sending */
@@ -1769,7 +1769,7 @@ static int gatt_notify(struct bt_conn *conn, u16_t handle,
    buf = bt_att_create_pdu(conn, BT_ATT_OP_NOTIFY,
                sizeof(*nfy) + params->len);
    if (!buf) {
-       BT_WARN("No buffer available to send notification");
+       BT_WARN("No buffer available to send notification, single");
        return -ENOMEM;
    }

diff --git a/subsys/mgmt/smp_bt.c b/subsys/mgmt/smp_bt.c
index d76de20b28..b758efda40 100644
--- a/subsys/mgmt/smp_bt.c
+++ b/subsys/mgmt/smp_bt.c
@@ -21,6 +21,9 @@

 #include <mgmt>

+#include <logging>
+LOG_MODULE_REGISTER(mcumgr, CONFIG_MCUMGR_LOG_LEVEL);
+
 struct device;

 struct smp_bt_user_data {
@@ -170,6 +173,7 @@ static int smp_bt_tx_pkt(struct zephyr_smp_transport *zst, struct net_buf *nb)
    if (conn == NULL) {
        rc = -1;
    } else {
+       LOG_ERR("conn %p, len: %d", conn, nb->len);
        rc = smp_bt_tx_rsp(conn, nb->data, nb->len);
        bt_conn_unref(conn);
    }
</logging></mgmt>

该提问来源于开源项目:zephyrproject-rtos/zephyr

  • 点赞
  • 写回答
  • 关注问题
  • 收藏
  • 复制链接分享
  • 邀请回答

6条回答

  • weixin_39531604 wdk199512 4月前

    -nordic can you please take a look? would you mind sending a PR?

    点赞 评论 复制链接分享
  • weixin_39597868 weixin_39597868 4月前

    Thank you for a detailed bug-report. The new GATT feature notify multiple should not break the API in this way. I have created a fix that sets it as default off, since it changes the behavior in a way that breaks existing applications.

    点赞 评论 复制链接分享
  • weixin_39794130 weixin_39794130 4月前

    The question remains if the

    bt_smp.c:smp_bt_get_mtu
    still must be adjusted, if the CONFIG_BT_GATT_NOTIFY_MULTIPLE is enabled (see workaround 3. above). This would be fixing the 'update' application to the change buffer requirements.
    点赞 评论 复制链接分享
  • weixin_39597868 weixin_39597868 4月前

    Yes if you wish to use GATT NOTIFY MULTIPLE. But there does not appear to be a good way for the application to determine if notify multiple will be used. We need to expose this information to the application in some way.

    点赞 评论 复制链接分享
  • weixin_39892460 weixin_39892460 4月前

    Hardcoding -3 is also a bad idea, because applications may actually want to set notify multiple then that would be broken anyway, so perhaps having a check fo IS_ENABLED(CONFIG_BT_GATT_NOTIFY_MULTIPLE) return mtu - 5; instead, but Im afraid we really want a runtime option if the application want to use the entire payload for doing streaming, but that would probably need an API change so the stack can avoid the grouping logic.

    点赞 评论 复制链接分享
  • weixin_39597868 weixin_39597868 4月前

    Hardcoding -3 is also a bad idea, because applications may actually want to set notify multiple then that would be broken anyway, so perhaps having a check fo IS_ENABLED(CONFIG_BT_GATT_NOTIFY_MULTIPLE) return mtu - 5

    But there is an MTU per L2CAP channel. I think the best is to leave it as is, MTU - 3 would give the best throughput when the application wants to send large data. The notify multiple seems to be best suited for many small notify values.

    点赞 评论 复制链接分享

相关推荐