Is this a bug in the nrf_ble_gatt in SDK 16 ?

[Nguyen Hoan Hoang]

Hi,

I found some thing not seems to be right in SDK 16.  I wonder if this is intentional or a bug.  Bluetooth 5 max MTU can be up to 512 bytes.  I found the definitions in the structures bellow that MTU size is uint16_t but data length is uint8_t.  This seems not rights also nrf_ble_gatt sets a hardcoded max data length to 251.  Which is Bluetooth 4.2 max data length.  That too is bad.  So how did the Bluetooth 5 throughput test demo able to set data length to 512 bytes when it is hard coded in nrf_ble_gatt to 251 ?  

Data length defined as uint8_t in nrf_ble_gatt.h which is max at 255.  So cannot use 512 bytes of Bluetooth 5.

/**@brief   GATT information for each connection. */
typedef struct
{
    uint16_t att_mtu_desired;               //!< Requested ATT_MTU size (in bytes).
    uint16_t att_mtu_effective;             //!< Effective ATT_MTU size (in bytes).
    bool     att_mtu_exchange_pending;      //!< Indicates that an ATT_MTU exchange request is pending (the call to @ref sd_ble_gattc_exchange_mtu_request returned @ref NRF_ERROR_BUSY).
    bool     att_mtu_exchange_requested;    //!< Indicates that an ATT_MTU exchange request was made.
#if !defined (S112) && !defined(S312)
    uint8_t  data_length_desired;           //!< Desired data length (in bytes).
    uint8_t  data_length_effective;         //!< Requested data length (in bytes).
#endif // !defined (S112) && !defined(S312)
} nrf_ble_gatt_link_t;


/**@brief   GATT structure that contains status information for the GATT module. */
struct nrf_ble_gatt_s
{
    uint16_t                   att_mtu_desired_periph;          //!< Requested ATT_MTU size for the next peripheral connection that is established.
    uint16_t                   att_mtu_desired_central;         //!< Requested ATT_MTU size for the next central connection that is established.
    uint8_t                    data_length;                     //!< Data length to use for the next connection that is established.
    nrf_ble_gatt_link_t        links[NRF_BLE_GATT_LINK_COUNT];  //!< GATT related information for all active connections.
    nrf_ble_gatt_evt_handler_t evt_handler;                     //!< GATT event handler.
};

Data length is hardcoded inside nrf_ble_gatt.c

#define BLE_GAP_DATA_LENGTH_DEFAULT     27          //!< The stack's default data length.
#define BLE_GAP_DATA_LENGTH_MAX         251         //!< Maximum data length.


ret_code_t nrf_ble_gatt_data_length_set(nrf_ble_gatt_t * p_gatt,
                                        uint16_t         conn_handle,
                                        uint8_t          data_length)
{
    if (p_gatt == NULL)
    {
        return NRF_ERROR_NULL;
    }

    // Check early to avoid requesting an invalid data length for upcoming connections.
    if (   (data_length > BLE_GAP_DATA_LENGTH_MAX)
        || (data_length < BLE_GAP_DATA_LENGTH_DEFAULT))
    {
        return NRF_ERROR_INVALID_PARAM;
    }

    if (conn_handle == BLE_CONN_HANDLE_INVALID)
    {
        // Save value and request upon connection.
        p_gatt->data_length = data_length;
        return NRF_SUCCESS;
    }

    if (conn_handle >= NRF_BLE_GATT_LINK_COUNT)
    {
        return NRF_ERROR_INVALID_PARAM;
    }

    // Request data length on existing link.
    p_gatt->links[conn_handle].data_length_desired = data_length;

    return data_length_update(conn_handle, data_length);
}

[Vidar Berg]

Hi,

The data length is referring to the LL packet length sent on air which actually can be smaller than the size of the MTU. An ATT packet will be split into multiple radio packets in that case. This is explained a bit more in the SDK documentation here: GATT Module. The "Bluetooth Low Energy data throughput" chapter of the SDS may also be interesting to read if you are looking to increase the transfer speed.

[Nguyen Hoan Hoang]

I understand that the data length can be smaller than MTU.  The question is that this is supposed to be Bluetooth 5 compatible therefore the length could be up to 512 bytes.  The nrf_ble_gatt hardcoded a limit of 251 of Bluetooth 4.2, so an uint8_t is sufficient but 512 will not and cause length corruption.  Is this a bug and should be changed to uint16_t instead ?  

[Vidar Berg]

The length value will not overflow with the current implementation because neither the RADIO or Softdevice support data lengths above 251. Also, the max. length is 251 bytes according to the table in core spec 5, vol 6, part b, section 4.5.10

[Nguyen Hoan Hoang]

oh, thanks for the explanation.  That's weird sd_ble_gap_data_length_update takes data length parameter as uint16_t. 

[Vidar Berg]

I haven't noticed that the SD API used uin16_t. Maybe it would have made more sense to keep the same data type in the GATT module as well. I guess it was done to make it more clear what the actual range is. Anyway, the Softdevice will return an error if you try to enable it with data lenght support above 251. 

[Nguyen Hoan Hoang]

Got ya! thanks.  I was confused about the fact that the data length was hardcoded at 251. Now I know where that comes from.  I would make the define in the .h so the firmware can validate forehand instead of let the SDK causing an assert. and with a comment as the reason why.