Write over QWR max length causes error

[nrbrook]

Firstly, I have the following definitions in sdk_config.h and when declaring the qwr buffers:

#ifndef NRF_SDH_BLE_GATT_MAX_MTU_SIZE
#ifndef TR_SLU_CONFIG_MAX_MSG_LEN
#define TR_SLU_CONFIG_MAX_MSG_LEN 240
#endif

#if TR_SLU_CONFIG_MAX_MSG_LEN > (251-5)
#define NRF_SDH_BLE_GATT_MAX_MTU_SIZE 251
#else
#define NRF_SDH_BLE_GATT_MAX_MTU_SIZE TR_SLU_CONFIG_MAX_MSG_LEN + 5
#endif
#endif

#ifndef NRF_SDH_BLE_GAP_DATA_LENGTH
#if NRF_SDH_BLE_GATT_MAX_MTU_SIZE < 251
#define NRF_SDH_BLE_GAP_DATA_LENGTH NRF_SDH_BLE_GATT_MAX_MTU_SIZE
#else
#define NRF_SDH_BLE_GAP_DATA_LENGTH 251
#endif
#endif

#define PREPARE_WRITE_REQUEST_OVERHEAD 5
#define MIN_DATA_LENGTH 20
#define MAX_NUM_PREPARE_WRITE_REQS (TR_SLU_CONFIG_MAX_MSG_LEN / MIN_DATA_LENGTH)
#define QWR_MEM_BUFF_SIZE TR_SLU_CONFIG_MAX_MSG_LEN + (PREPARE_WRITE_REQUEST_OVERHEAD * MAX_NUM_PREPARE_WRITE_REQS)
static uint8_t qwr_buffers[NRF_SDH_BLE_TOTAL_LINK_COUNT][QWR_MEM_BUFF_SIZE];

Note that in the above, the intention is that TR_SLU_CONFIG_MAX_LENGTH could be modified to be larger (e.g. 600) without modifying the other definitions.

I am not sure if these are correct as I couldn't find detailed documentation on these configurations, i.e. what is the maximum possible value of NRF_SDH_BLE_GATT_MAX_MTU_SIZE? What is the relationship between NRF_SDH_BLE_GATT_MAX_MTU_SIZE and NRF_SDH_BLE_GAP_DATA_LENGTH? Does NRF_SDH_BLE_GAP_DATA_LENGTH have to be less than NRF_SDH_BLE_GATT_MAX_MTU_SIZE? Is there any overhead that should be considered between NRF_SDH_BLE_GAP_DATA_LENGTH and NRF_SDH_BLE_GATT_MAX_MTU_SIZE? Is the overhead of 5 bytes used here correct for QWR packets? Perhaps you could help me with these questions.

With the above configuration, if I write a packet that is larger than the QWR buffer e.g. 1kb from my iPhone to the QWR characteristic, I get an error code of 8 (NRF_ERROR_INVALID_STATE) when the qwr library sends a response to the write request:

I can see from a packet log that the softdevice has already responded with 0x09 Prepare Queue Full, so this error must be because the QWR library is attempting to respond again - why? Is there any way to avoid this issue or should I just ignore NRF_ERROR_INVALID_STATE errors from the QWR library?

[Einar Thorsrud]

Hi,

What is the relationship between NRF_SDH_BLE_GATT_MAX_MTU_SIZE and NRF_SDH_BLE_GAP_DATA_LENGTH?

These are different concepts for different layers (GAP and GATT). A common configuration when you use DLE and want the longest possible GAP packets is to set NRF_SDH_BLE_GATT_MAX_MTU_SIZE to 247 and NRF_SDH_BLE_GAP_DATA_LENGTH. This is the maximum GAP size. GATT MTU can be larger however, but then the GATT packets are split across several GAP packets.

Does NRF_SDH_BLE_GAP_DATA_LENGTH have to be less than NRF_SDH_BLE_GATT_MAX_MTU_SIZE?

No.

Is there any overhead that should be considered between NRF_SDH_BLE_GAP_DATA_LENGTH and NRF_SDH_BLE_GATT_MAX_MTU_SIZE?

Nothing specifically for queued write. The "optimal" configuration for maximum throughput is normally 247 and 251 for GATT and GAP respectively, and the inherent overhead of queued write (2 byte handle + 2 byte offset). 

I can see from a packet log that the softdevice has already responded with 0x09 Prepare Queue Full, so this error must be because the QWR library is attempting to respond again - why? Is there any way to avoid this issue or should I just ignore NRF_ERROR_INVALID_STATE errors from the QWR library?

I do not have the full overview here, but is it so that you should have executed the write, but did not do so (as the prepare queue is full, assuming you have ensured it is large enough for your use case).

By the way, why are you using queued write? That adds some overhead and complexity, and normally the only real good reason for using it is if you want to change the value of several characteristics in one go, transferring a lot of data, and "activating" the change in one instance.

[nrbrook]

I read around a bit more and have a better understanding of the concepts now. Please correct me if any of this is wrong. I can't actually change NRF_SDH_BLE_GAP_DATA_LENGTH as I'm using S112 which doesn't support DLE. But NRF_SDH_BLE_GAP_DATA_LENGTH represents the maximum GAP data length that can be sent in one BLE packet. NRF_SDH_BLE_GATT_MAX_MTU_SIZE represents the maximum data length that can be sent in a single GATT write and can be split across multiple BLE packets, so NRF_SDH_BLE_GATT_MAX_MTU_SIZE can be larger than NRF_SDH_BLE_GAP_DATA_LENGTH.

I believe NRF_SDH_BLE_GATT_MAX_MTU_SIZE maximum is 517 - 4 (l2cap header size) = 513, is that correct?

Variable length characteristics are limited to 512 bytes (BLE_GATTS_VAR_ATTR_LEN_MAX), why is this?

Einar Thorsrud said:

I do not have the full overview here, but is it so that you should have executed the write, but did not do so (as the prepare queue is full, assuming you have ensured it is large enough for your use case).

I'm not sure I understand what you are saying here – this error occurs on a prepare write request, before any QWR events occur as the execute write request has not been performed at this point. I think the SD responds automatically to the prepare write request as the user mem buffer is too small, but then the QWR library also attempts to respond with a success response and fails. I'm not sure if the QWR library has any way of knowing the SD has already responded.

Einar Thorsrud said:

By the way, why are you using queued write?

If you have a characteristic that is longer than the negotiated MTU and write data that is longer than the MTU, iOS (and Android too I believe) will automatically use prepare/execute write request to send the data.

[nrbrook]

The issue seems to be that the behaviour of S112 7.2.0 softdevice does not seem to match the message sequence charts. This chart:

https://infocenter.nordicsemi.com/index.jsp?topic=%2Fcom.nordic.infocenter.s112.api.v7.3.0%2Fgroup___b_l_e___g_a_t_t_s___q_u_e_u_e_d___w_r_i_t_e___q_u_e_u_e___f_u_l_l___m_s_c.html

suggests that when a user mem block is returned from the application (by the qwr library) no BLE_GATTS_OP_PREP_WRITE_REQ events are sent to the application, only a BLE_GATTS_OP_EXEC_WRITE_REQ_NOW event. However the opposite is true – only BLE_GATTS_OP_PREP_WRITE_REQ are sent, and BLE_GATTS_OP_EXEC_WRITE_REQ_NOW is not sent when the prepare queue has insufficient space. While the softdevice does indeed respond directly to the client with a prepare queue full error, it also then sends a BLE_GATTS_OP_PREP_WRITE_REQ event to the application, and the qwr library then attempts to send its own response.

Even the success chart suggests no BLE_GATTS_OP_PREP_WRITE_REQ events are sent to the App, which is incorrect: infocenter.nordicsemi.com/index.jsp

The QWR library depends on receiving the BLE_GATTS_OP_PREP_WRITE_REQ events to track which attributes have been written in order to dispatch the events to the user's code for each attribute. However the fact that the SD sends the queue full response but not success responses (as the charts incorrectly indicate) means when a queue full event occurs it causes the error in the QWR library. If there is a way for the QWR library to check if the SD already responded, it doesn't check it. However, it could deduce that the SD will have responded by checking (offset + length <= user mem buffer) before sending a response.

I added some logging to the QWR library and reproduced the issue.

Error case (data written > QWR buffer length):

<info> app: User mem reply 625 bytes
<info> app: Prepare write offset 0 length 288
<info> app: Prepare write offset 288 length 288
<info> app: Prepare write offset 576 length 288
<info> app: Error 8 occurred when sending response
<info> app: User mem release

Success case (data written < QWR buffer length):

<info> app: User mem reply 625 bytes
<info> app: Prepare write offset 0 length 288
<info> app: Prepare write offset 288 length 184
<info> app: Execute write offset 0 length 0
<info> app: User mem release

[Einar Thorsrud]

Hi,

I did not get around to look at this today, but will see early next week. Can you share the project you use to reproduce this?

[nrbrook]

I attached an example project modified from the stock qwr example to extend the mtu and maximum characteristic length. Place in examples/ble_peripheral/experimental. Write >512 bytes of data to the characteristic from an iPhone and the problem is exhibited.ble_app_queued_writes.zip

Strangely, with the default MTU and characteristic fixed length and length (128) configuration, I saw the same issue occurring but in on_execute_write when it attempts to respond. My guess is that because the characteristic data length is not variable, when the SD gets more data than the characteristic length it responds instead with incorrect length error and triggers a BLE_GATTS_OP_EXEC_WRITE_REQ_NOW event instead. But the SD should also surely respond with the invalid length error when the characteristic is variable and the prepare writes go beyond the maximum length of the characteristic? This would seem like a SD bug.

FWIW, my fix for QWR library looks like this:

if ((p_evt_write->offset + p_evt_write->len) <= p_qwr->mem_buffer.len) {
        // the length of data written is valid, attempt to respond
        err_code = sd_ble_gatts_rw_authorize_reply(p_qwr->conn_handle, &auth_reply);
        if (err_code != NRF_SUCCESS)
        {
            // Cancel the current operation.
            p_qwr->nb_written_handles = 0;

            // Report error to application.
            p_qwr->error_handler(err_code);
        }
    } else {
        // The length of data written is invalid, do not attempt to respond
        NRF_LOG_WARNING("Invalid length of data written: %d + %d = %d bytes, mem buffer provided = %d bytes", p_evt_write->offset, p_evt_write->len, p_evt_write->offset + p_evt_write->len, p_qwr->mem_buffer.len);

        // Cancel the current operation.
        p_qwr->nb_written_handles = 0;
    }

Fixing the similar issue in on_execute_write might be more difficult as I'm not sure the library can know the fixed length of the characteristic...

[Einar Thorsrud]

Thanks for the detailed description and suggested fix. I will forward this internally so that it can be fixed in a potential future release.