BLE connection parameters updated with inconsistent values

[ejohnso49]

Describe the bug
I'm running into some unexpected behavior regarding connection parameters. I have a Zephyr peripheral (v2.6.0) which is connecting to my laptop (central device). I have BT_GAP_AUTO_UPDATE_CONN_PARAMS set and I see the peripheral attempt to update the parameters with LL_CONNECTION_PARAM_REQ in a Wireshark trace. It sends a request with the values I've specified with CONFIG_BT_PERIPHERAL_PREF_MIN_INT and
CONFIG_BT_PERIPHERAL_PREF_MAX_INT. My central device then rejects this as it claims that is an unsupported feature (not sure if this is a separate issue). Next the peripheral falls back to the L2CAP update procedure and for some reason this request now has different values for the interval min and max. I've found the section of code where the L2CAP procedure initiates with these other parameter values, from le_conn_update_complete:

struct bt_le_conn_param param;

param.interval_min = conn->le.interval_min;
param.interval_max = conn->le.interval_max;
param.latency = conn->le.pending_latency;
param.timeout = conn->le.pending_timeout;

bt_l2cap_update_conn_param(conn, &param);

It seems like the conn struct already has these set to 24 for min and 40 for max.

To Reproduce
Steps to reproduce the behavior:

1. Build peripheral application which has CONFIG_BT_GAP_AUTO_UPDATE_CONN_PARAMS=y, enables the LL connection param update feature, and sets preferred interval min and maxes to values different than BT_GAP_INIT_CONN.
2. Use a central device which does not support the LL connection param update feature so ensure procedure falls back to L2CAP.

Expected behavior
I would expect that when the update procedure falls back to L2CAP that the same values as specified by CONFIG_BT_PERIPHERAL_PREF_MIN_INT/etc are used during this instead of BT_GAP_INIT_CONN_INT_MIN/etc

Impact
This impacts connection throughput for applications as they might expect a specific interval value range. This potentially could be mitigated by disabling the LL connection parameter update feature as I think the fallback is the specific issue. Additionally if my Central supported the LL connection parameter update feature then I think this issue would be masked.

Logs and console output
Some log output that I added to the code:
I have added a log message at (hci_core.c:1578):

BT_INFO("Attempt L2CAP CPUP, int_min[%d], int_max[%d]", conn->le.interval_min, conn->le.interval_max);

Log Output:

[00:03:06.720,855] <inf> bt_conn: Using kconfigs
[00:03:06.720,886] <inf> bt_conn: conn 0x20001348 features 0x7f params (6-6 0 42)
[00:03:06.721,038] <inf> bt_conn: LE conn param req result [0]
[00:03:06.821,472] <inf> bt_hci_core: status 0x1a, handle 0
[00:03:06.821,502] <inf> bt_hci_core: Attempt L2CAP CPUP, int_min[24], int_max[40]
[00:03:07.088,378] <inf> bt_hci_core: status 0x00, handle 0

• See also attached Wireshark trace: dfu_v2.6.0_subset.pcapng.zip

Environment (please complete the following information):

• OS: MacOS build host, MacOS BLE central device, Zephyr peripheral
• Toolchain: arm-gcc-10
• Commit SHA or Version used: Zephyr v2.6.0

Additional context
The operation that is traced is part of the DFU process via mcumgr and the BLE SMP service.

[joerchan]

Looks like a mix of not saving the needed state for the fallback, or an incomplete saving of the needed state.
Not storing new conn params before calling send_conn_le_param_update:

bt_conn_le_param_update:
        /* if slave conn param update timer expired just send request */
        if (atomic_test_bit(conn->flags, BT_CONN_SLAVE_PARAM_UPDATE)) {
            return send_conn_le_param_update(conn, param);
        }

        /* store new conn params to be used by update timer */
        conn->le.interval_min = param->interval_min;
        conn->le.interval_max = param->interval_max;
        conn->le.pending_latency = param->latency;
        conn->le.pending_timeout = param->timeout;
        atomic_set_bit(conn->flags, BT_CONN_SLAVE_PARAM_SET);

Not storing interval here:

send_conn_le_param_update:
        /* store those in case of fallback to L2CAP */
        if (rc == 0) {
            conn->le.pending_latency = param->latency;
            conn->le.pending_timeout = param->timeout;
        }

[ejohnso49]

@joerchan I think I understand the need to save the interval_min/interval_max fields in send_conn_le_param_update. Can you elaborate more on the first block of code you highlighted in bt_conn_le_param_update? To me it looks like that is okay because you would want to use the application provided params. Am I missing something there?

[joerchan]

@ejohnso49 The first code-block shows that all 4 values are saved when the timer is not expired. But when the timer is expired it goes to send_conn_le_param_update which only saves 2 of the values.