espressif / esp-idf

Espressif IoT Development Framework. Official development framework for Espressif SoCs.
Apache License 2.0
12.56k stars 7.01k forks source link

nvs::NVSHandle::set_item() API change request (IDFGH-12670) #13662

Open yzazik opened 3 weeks ago

yzazik commented 3 weeks ago

Is your feature request related to a problem?

In components/nvs_flash/include/nvs_handle.hpp(283). Unlike in nvs::NVSHandle::get_item(), the value parameter in nvs::NVSHandle::set_item() is passed "by-value" (by copy).

// Template Implementations
template<typename T>
esp_err_t NVSHandle::set_item(const char *key, T value) {
    return set_typed_item(itemTypeOf(value), key, &value, sizeof(value));
}

template<typename T>
esp_err_t NVSHandle::get_item(const char *key, T &value) {
    return get_typed_item(itemTypeOf(value), key, &value, sizeof(value));
}

In addition to a possible performance hit, the sizeof(T) in set_item() is also different when T is an array. This leads to assymetric behaviour between set and get item:

#include <iostream>
template<typename T> size_t sizeof_T_in_set(T value) {return sizeof(value);}
template<typename T> size_t sizeof_T_in_get(T& value) {return sizeof(value);}
int main()
{
    using ssid_t = uint8_t[32];
    using mac_t = uint8_t[6];
    ssid_t ssid {};
    mac_t mac {};
    std::cout << "sizeof_T_in_set(ssid) = " << sizeof_T_in_set(ssid) << "\n";
    std::cout << "sizeof_T_in_set(mac)  = " << sizeof_T_in_set(mac) << "\n";
    std::cout << "sizeof_T_in_get(ssid) = " << sizeof_T_in_get(ssid) << "\n";
    std::cout << "sizeof_T_in_get(mac)  = " << sizeof_T_in_get(mac) << "\n";
    return 0;
}

output:

sizeof_T_in_set(ssid) = 4
sizeof_T_in_set(mac)  = 4
sizeof_T_in_get(ssid) = 32
sizeof_T_in_get(mac)  = 6

Describe the solution you'd like.

Pass by reference:

template<typename T>
esp_err_t NVSHandle::set_item(const char *key, const T& value) {
    return set_typed_item(itemTypeOf(value), key, &value, sizeof(value));
}

Describe alternatives you've considered.

Well, the "alternative" would be set_blob() but it's beside the point as it is meant for variable size raw data.

Additional context.

No response