Bug/Improvement in handling NRF_ERROR_RESOURCES response in gatt queue NRF_BLE_GQ_REQ_GATTS_HVX processing

[Anthony Ambuehl]

I recently started using the gatt queue as a mechanism to buffer and send HVX notifications.  At times our system may produce asynchronous notification events faster than the bluetooth link can process them.  Instead of implementing scheduled retries, its convenient to utilize the GATT queue mechanism that is demonstrated in the various GATT client examples.  

A small issue was discovered in the processing of the return code from sd_ble_gatts_hvx.  Unlike the other sd_ble_gatts_** functions, this function may also return NRF_ERROR_RESOURCES, which is effectively the same as NRF_ERROR_BUSY.  Without any modifications, the GATT queue module will produce an error log every time an HVX (notification) is unable to be sent, AND the HVX is dropped.    This behavior does not fit the intended purpose of the GATT queue, therefore I suggest modifying the err_code checking after sd_ble_gatts_hvx to convert the NRF_ERROR_RESOURCES error to be NRF_ERROR_BUSY.  

Here are the diffs for my changes:

 

diff --git a/components/ble/nrf_ble_gq/nrf_ble_gq.c b/components/ble/nrf_ble_gq/nrf_ble_gq.c
index 9ef131c..f9ade8c 100644
--- a/components/ble/nrf_ble_gq/nrf_ble_gq.c
+++ b/components/ble/nrf_ble_gq/nrf_ble_gq.c
@@ -260,6 +260,10 @@ static void queue_process(nrf_queue_t const * const p_queue, uint16_t conn_handl
                 {
                     err_code = NRF_ERROR_DATA_SIZE;
                 }
+                if (err_code == NRF_ERROR_RESOURCES)
+                {
+                    err_code = NRF_ERROR_BUSY;
+                }
             } break;

             default:
@@ -387,7 +391,10 @@ static bool request_process(nrf_ble_gq_req_t const * const p_req, uint16_t conn_
             {
                 err_code = NRF_ERROR_DATA_SIZE;
             }
-
+            if (err_code == NRF_ERROR_RESOURCES)
+            {
+                err_code = NRF_ERROR_BUSY;
+            }
         } break;

         default:

[Einar Thorsrud]

Hi,

Thank you for reporting this. I have made an internal bug report so that this may be addressed in a future release.

[mkincaid]

I've just run into this same issue on SDK 17.1.0. The GATT queue does not retry HVX if it gets an NRF_ERROR_RESOURCES (which happens when you try to send a blast of notifications).

Is this still scheduled to be fixed?

My change was slightly different from Anthony's. In nrf_ble_gq.c there is code that checks for NRF_ERROR_BUSY, and if it wasn't that specific error it would pop the queued HVX off the queue.

if (err_code == NRF_ERROR_BUSY) // Softdevice is processing another GATT request.
{
    NRF_LOG_DEBUG("SD is currently busy. The GATT request procedure will be attempted \
                  again later.");
}
else
{
    // Remove last request descriptor from the queue and free data associated with it.
    if (m_req_data_alloc[ble_req.type] != NULL)
    {
        nrf_memobj_free(ble_req.p_mem_obj);
        NRF_LOG_DEBUG("Pointer to freed memory block: %p.", ble_req.p_mem_obj);
    }
    UNUSED_RETURN_VALUE(nrf_queue_pop(p_queue, &ble_req));

    request_err_code_handle(&ble_req, conn_handle, err_code);
}

Now I explicitly check for success, if not then leave it on the queue.

if (err_code == NRF_SUCCESS)
{
    // Remove last request descriptor from the queue and free data associated with it.
    if (m_req_data_alloc[ble_req.type] != NULL)
    {
        nrf_memobj_free(ble_req.p_mem_obj);
        NRF_LOG_DEBUG("Pointer to freed memory block: %p.", ble_req.p_mem_obj);
    }
    UNUSED_RETURN_VALUE(nrf_queue_pop(p_queue, &ble_req));

    request_err_code_handle(&ble_req, conn_handle, err_code);
}
else  // Softdevice is processing another GATT request.
{
    NRF_LOG_DEBUG("SD is currently busy. The GATT request procedure will be attempted \
                  again later.");
}

[Anthony Ambuehl]

mkincaid,  I would be concerned with  your approach, there are some error codes returned that are not due to lack of resources.  Errors such as NRF_ERROR_INVALID_PARAM or NRF_ERROR_NULL will never resolve, also errors due to disconnect such as NRF_ERROR_INVALID_STATE will not resolve quickly.   

Unfortunately Nordic does not quickly solve these reported problems, I have some bugs from 2-3 years ago that are still not fixed in latest releases, also I think the focus is on transition to NRF Connect SDK / ZephyrOS.

Hope this helps!

[mkincaid]

Thank you that is a good point. I would rather remove from the queue then retry forever if there is not a chance it will succeed.