Const correctness of the Bluedroid API

Xavi92
Posts: 45
Joined: Thu Mar 28, 2019 2:26 pm

Const correctness of the Bluedroid API

Postby Xavi92 » Mon Apr 27, 2020 7:43 am

Using branch release/v4.1.

From the ble_spp_server_demo.c example:

Code: Select all

static const uint16_t primary_service_uuid = ESP_GATT_UUID_PRI_SERVICE;
/// SPP Service
static const uint16_t spp_service_uuid = 0xABF0;
/* Other variables. */
///Full HRS Database Description - Used to add attributes into the database
static const esp_gatts_attr_db_t spp_gatt_db[SPP_IDX_NB] =
{
    //SPP -  Service Declaration
    [SPP_IDX_SVC]                      	=
    {{ESP_GATT_AUTO_RSP}, {ESP_UUID_LEN_16, (uint8_t *)&primary_service_uuid, ESP_GATT_PERM_READ,
    sizeof(spp_service_uuid), sizeof(spp_service_uuid), (uint8_t *)&spp_service_uuid}},
/* Other members. */
}
This example stores UUIDs and data into non-volatile memory via the 'const' qualifier. However, esp_attr_desc_t takes a read/write pointer to both the UUID and characteristic data, which means the example has to cast the 'const' away from all references to avoid compiler warnings. I don't see where this data is actually being modified and, if it really is, the example would be calling undefined behaviour according to the standard.

I've seen other functions on the BLE API that do not seem to be const-correct and should be also fixed:

esp_ble_gap_start_advertising(): according to the implementation, adv_params is not being modified anywhere, but is rather copied into another instance, so there isn't any reason for adv_params not to be 'const esp_ble_adv_params_t *'.

Code: Select all

esp_err_t esp_ble_gap_start_advertising(esp_ble_adv_params_t *adv_params)
{
    btc_msg_t msg;
    btc_ble_gap_args_t arg;

    ESP_BLUEDROID_STATUS_CHECK(ESP_BLUEDROID_STATUS_ENABLED);

    msg.sig = BTC_SIG_API_CALL;
    msg.pid = BTC_PID_GAP_BLE;
    msg.act = BTC_GAP_BLE_ACT_START_ADV;
    memcpy(&arg.start_adv.adv_params, adv_params, sizeof(esp_ble_adv_params_t));

    return (btc_transfer_context(&msg, &arg, sizeof(btc_ble_gap_args_t), NULL) == BT_STATUS_SUCCESS ? ESP_OK : ESP_FAIL);
}
Also, in this particular implementation, I'd also suggest replacing the mempcy() call by a simple assignment for increased type safety:

Code: Select all

arg.start_adv.adv_params = *adv_params;
esp_ble_gap_update_conn_params(), esp_ble_gap_config_adv_data(), esp_ble_gap_set_scan_params(): exactly the same issues as in the function above.

esp_ble_gap_config_adv_data_raw(): this function only takes a pointer to given raw data instead of copying it, but since arg.cfg_adv_data_raw.raw_adv is a read/write pointer, raw_data must also be a read/write pointer. However, if arg.cfg_adv_data_raw.raw_adv is never modified by the API, all these pointers should be const-qualified.
Hint: btc_transfer_context() does not modify "arg", so it seems possible to add the 'const' qualifier.

Code: Select all

esp_err_t esp_ble_gap_config_adv_data_raw(uint8_t *raw_data, uint32_t raw_data_len)
{
    btc_msg_t msg;
    btc_ble_gap_args_t arg;

    ESP_BLUEDROID_STATUS_CHECK(ESP_BLUEDROID_STATUS_ENABLED);

    if (raw_data == NULL
            || (raw_data_len <= 0 || raw_data_len > ESP_BLE_ADV_DATA_LEN_MAX)) {
        return ESP_ERR_INVALID_ARG;
    }

    msg.sig = BTC_SIG_API_CALL;
    msg.pid = BTC_PID_GAP_BLE;
    msg.act = BTC_GAP_BLE_ACT_CFG_ADV_DATA_RAW;
    arg.cfg_adv_data_raw.raw_adv = raw_data;
    arg.cfg_adv_data_raw.raw_adv_len = raw_data_len;

    return (btc_transfer_context(&msg, &arg, sizeof(btc_ble_gap_args_t), btc_gap_ble_arg_deep_copy) == BT_STATUS_SUCCESS ? ESP_OK : ESP_FAIL);

}
I didn't read more of the API, but I am pretty sure there are other places where const correctness should be reviewed. I know status quo works, but placing those buffers into .data instead of .rodata (instead of casting the 'const' qualifier away) is less efficient memory-wise.

Could these suggestions be please reviewed by the esp-idf team and merged when possible?

Xavi92
Posts: 45
Joined: Thu Mar 28, 2019 2:26 pm

Re: Const correctness of the Bluedroid API

Postby Xavi92 » Sun May 03, 2020 8:22 pm

Any suggestions on this? Should I rather post this issue to https://github.com/espressif/esp-idf ?

ESP_Sprite
Posts: 8921
Joined: Thu Nov 26, 2015 4:08 am

Re: Const correctness of the Bluedroid API

Postby ESP_Sprite » Mon May 04, 2020 6:05 am

Thanks for your notes, and you are right that we should look into it. However, Github may indeed be a better place to post issues like this to, as I don't think many of the BT devs roam around freely on the forums.

Xavi92
Posts: 45
Joined: Thu Mar 28, 2019 2:26 pm

Re: Const correctness of the Bluedroid API

Postby Xavi92 » Mon May 04, 2020 6:41 am

Thank you for you anwser. I've submitted a new issue on the Github repo: https://github.com/espressif/esp-idf/issues/5236

Xavi92
Posts: 45
Joined: Thu Mar 28, 2019 2:26 pm

Re: Const correctness of the Bluedroid API

Postby Xavi92 » Mon Jun 15, 2020 11:38 am

It's been a month and I didn't receive any answer on Github. Could somebody at Espressif please review this issue?
Thank you very much in advance.

Who is online

Users browsing this forum: HighVoltage and 120 guests