Const correctness of the Bluedroid API
Posted: Mon Apr 27, 2020 7:43 am
Using branch release/v4.1.
From the ble_spp_server_demo.c example:
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 *'.
Also, in this particular implementation, I'd also suggest replacing the mempcy() call by a simple assignment for increased type safety:
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.
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?
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. */
}
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);
}
Code: Select all
arg.start_adv.adv_params = *adv_params;
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);
}
Could these suggestions be please reviewed by the esp-idf team and merged when possible?