Merge branch 'bugfix/nvs_set_datatype' into 'master'

bugfix(nvs_flash) : fixed nvs_set functions behaviour when called sequentially with same key and different data type(s)

Closes IDFGH-9727

See merge request espressif/esp-idf!25581

Author: pacucha42

components/nvs_flash/.build-test-rules.yml

@@ -0,0 +1,3 @@
+components/nvs_flash/host_test:
+  enable:
+    - if: IDF_TARGET == "linux"

components/nvs_flash/Kconfig

@@ -25,4 +25,14 @@ menu "NVS"
         default n
         help
             This option switches error checking type between assertions (y) or return codes (n).
+
+    config NVS_LEGACY_DUP_KEYS_COMPATIBILITY
+        bool "Enable legacy nvs_set function behavior when same key is reused with different data types"
+        default n
+        help
+            Enabling this option will switch the nvs_set() family of functions to the legacy mode:
+            when called repeatedly with the same key but different data type, the existing value
+            in the NVS remains active and the new value is just stored, actually not accessible through
+            corresponding nvs_get() call for the key given. Use this option only when your application
+            relies on such NVS API behaviour.
 endmenu

components/nvs_flash/host_test/nvs_host_test/main/test_nvs.cpp

@@ -3255,6 +3255,71 @@ TEST_CASE("check and read data from partition generated via manufacturing utilit
     }
 }
 
+TEST_CASE("nvs multiple write with same key but different types", "[nvs][xxx]")
+{
+    PartitionEmulationFixture f(0, 10);
+
+    nvs_handle_t handle_1;
+    const uint32_t NVS_FLASH_SECTOR = 6;
+    const uint32_t NVS_FLASH_SECTOR_COUNT_MIN = 3;
+    TEMPORARILY_DISABLED(f.emu.setBounds(NVS_FLASH_SECTOR, NVS_FLASH_SECTOR + NVS_FLASH_SECTOR_COUNT_MIN);)
+
+    for (uint16_t j = NVS_FLASH_SECTOR; j < NVS_FLASH_SECTOR + NVS_FLASH_SECTOR_COUNT_MIN; ++j) {
+        f.erase(j);
+    }
+    TEST_ESP_OK(nvs::NVSPartitionManager::get_instance()->init_custom(f.part(),
+                NVS_FLASH_SECTOR,
+                NVS_FLASH_SECTOR_COUNT_MIN));
+
+    TEST_ESP_OK(nvs_open("namespace1", NVS_READWRITE, &handle_1));
+
+    nvs_erase_all(handle_1);
+
+    int32_t v32;
+    int8_t v8;
+
+    TEST_ESP_OK(nvs_set_i32(handle_1, "foo", (int32_t)12345678));
+    TEST_ESP_OK(nvs_set_i8(handle_1, "foo", (int8_t)12));
+    TEST_ESP_OK(nvs_set_i8(handle_1, "foo", (int8_t)34));
+
+#ifdef CONFIG_NVS_LEGACY_DUP_KEYS_COMPATIBILITY
+    // Legacy behavior
+    // First use of key hooks data type until removed by nvs_erase_key. Alternative re-use of same key with different
+    // data type is written to the storage as hidden active value. It is returned by nvs_get function after nvs_erase_key is called.
+    // Mixing more than 2 data types brings undefined behavior. It is not tested here.
+
+    TEST_ESP_ERR(nvs_get_i8(handle_1, "foo", &v8), ESP_ERR_NVS_NOT_FOUND);
+    TEST_ESP_OK(nvs_get_i32(handle_1, "foo", &v32));
+    TEST_ESP_OK(nvs_erase_key(handle_1, "foo"));
+
+    TEST_ESP_OK(nvs_get_i8(handle_1, "foo", &v8));
+    TEST_ESP_ERR(nvs_get_i32(handle_1, "foo", &v32), ESP_ERR_NVS_NOT_FOUND);
+    TEST_ESP_OK(nvs_erase_key(handle_1, "foo"));
+
+    TEST_ESP_OK(nvs_get_i8(handle_1, "foo", &v8));
+    TEST_ESP_ERR(nvs_get_i32(handle_1, "foo", &v32), ESP_ERR_NVS_NOT_FOUND);
+    TEST_ESP_OK(nvs_erase_key(handle_1, "foo"));
+
+    TEST_ESP_ERR(nvs_erase_key(handle_1, "foo"), ESP_ERR_NVS_NOT_FOUND);
+#else
+    // New behavior
+    // Latest nvs_set call replaces any existing value. Only one active value under the key exists in storage
+
+    TEST_ESP_OK(nvs_get_i8(handle_1, "foo", &v8));
+    TEST_ESP_ERR(nvs_get_i32(handle_1, "foo", &v32), ESP_ERR_NVS_NOT_FOUND);
+    TEST_ESP_OK(nvs_erase_key(handle_1, "foo"));
+
+    TEST_ESP_ERR(nvs_get_i8(handle_1, "foo", &v8), ESP_ERR_NVS_NOT_FOUND);
+    TEST_ESP_ERR(nvs_get_i32(handle_1, "foo", &v32), ESP_ERR_NVS_NOT_FOUND);
+    TEST_ESP_ERR(nvs_erase_key(handle_1, "foo"), ESP_ERR_NVS_NOT_FOUND);
+#endif
+
+    nvs_close(handle_1);
+
+    TEST_ESP_OK(nvs_flash_deinit_partition(NVS_DEFAULT_PART_NAME));
+}
+
+
 /* Add new tests above */
 /* This test has to be the final one */
 

components/nvs_flash/host_test/nvs_host_test/sdkconfig.ci.default_set_key

@@ -0,0 +1 @@
+# CONFIG_NVS_LEGACY_DUP_KEYS_COMPATIBILITY=n

components/nvs_flash/host_test/nvs_host_test/sdkconfig.ci.legacy_set_key

@@ -0,0 +1 @@
+CONFIG_NVS_LEGACY_DUP_KEYS_COMPATIBILITY=y

components/nvs_flash/host_test/nvs_page_test/main/nvs_page_test.cpp

@@ -1,16 +1,8 @@
-// Copyright 2015-2016 Espressif Systems (Shanghai) PTE LTD
-//
-// Licensed under the Apache License, Version 2.0 (the "License");
-// you may not use this file except in compliance with the License.
-// You may obtain a copy of the License at
-
-//     http://www.apache.org/licenses/LICENSE-2.0
-//
-// Unless required by applicable law or agreed to in writing, software
-// distributed under the License is distributed on an "AS IS" BASIS,
-// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-// See the License for the specific language governing permissions and
-// limitations under the License.
+/*
+ * SPDX-FileCopyrightText: 2015-2023 Espressif Systems (Shanghai) CO LTD
+ *
+ * SPDX-License-Identifier: Apache-2.0
+ */
 #include <stdio.h>
 #include "unity.h"
 #include "test_fixtures.hpp"
@@ -863,7 +855,7 @@ void test_Page_calcEntries__uninit()
     NVSPageFixture fix;
     TEST_ASSERT_EQUAL(Page::PageState::UNINITIALIZED, fix.page.state());
 
-    nvs_stats_t nvsStats = {0, 0, 0, 0};
+    nvs_stats_t nvsStats = {0, 0, 0, 0, 0};
 
     TEST_ASSERT_EQUAL(ESP_OK, fix.page.calcEntries(nvsStats));
     TEST_ASSERT_EQUAL(0, nvsStats.used_entries);
@@ -888,7 +880,7 @@ void test_Page_calcEntries__corrupt()
 
     TEST_ASSERT_EQUAL(Page::PageState::CORRUPT, page.state());
 
-    nvs_stats_t nvsStats = {0, 0, 0, 0};
+    nvs_stats_t nvsStats = {0, 0, 0, 0, 0};
 
     TEST_ASSERT_EQUAL(ESP_OK, page.calcEntries(nvsStats));
     TEST_ASSERT_EQUAL(0, nvsStats.used_entries);
@@ -901,7 +893,7 @@ void test_Page_calcEntries__active_wo_blob()
 {
     NVSValidPageFixture fix;
 
-    nvs_stats_t nvsStats = {0, 0, 0, 0};
+    nvs_stats_t nvsStats = {0, 0, 0, 0, 0};
 
     TEST_ASSERT_EQUAL(ESP_OK, fix.page.calcEntries(nvsStats));
     TEST_ASSERT_EQUAL(2, nvsStats.used_entries);
@@ -914,7 +906,7 @@ void test_Page_calcEntries__active_with_blob()
 {
     NVSValidBlobPageFixture fix;
 
-    nvs_stats_t nvsStats = {0, 0, 0, 0};
+    nvs_stats_t nvsStats = {0, 0, 0, 0, 0};
 
     TEST_ASSERT_EQUAL(ESP_OK, fix.page.calcEntries(nvsStats));
     TEST_ASSERT_EQUAL(4, nvsStats.used_entries);
@@ -927,7 +919,7 @@ void test_Page_calcEntries__invalid()
 {
     Page page;
 
-    nvs_stats_t nvsStats = {0, 0, 0, 0};
+    nvs_stats_t nvsStats = {0, 0, 0, 0, 0};
 
     TEST_ASSERT_EQUAL(Page::PageState::INVALID, page.state());
 

components/nvs_flash/src/nvs_page.cpp

@@ -878,7 +878,7 @@ esp_err_t Page::findItem(uint8_t nsIndex, ItemType datatype, const char* key, si
         end = ENTRY_COUNT;
     }
 
-    if (nsIndex != NS_ANY && datatype != ItemType::ANY && key != NULL) {
+    if (nsIndex != NS_ANY && key != NULL) {
         size_t cachedIndex = mHashList.find(start, Item(nsIndex, datatype, 0, key, chunkIdx));
         if (cachedIndex < ENTRY_COUNT) {
             start = cachedIndex;

components/nvs_flash/src/nvs_storage.cpp

@@ -1,5 +1,5 @@
 /*
- * SPDX-FileCopyrightText: 2015-2022 Espressif Systems (Shanghai) CO LTD
+ * SPDX-FileCopyrightText: 2015-2023 Espressif Systems (Shanghai) CO LTD
  *
  * SPDX-License-Identifier: Apache-2.0
  */
@@ -285,13 +285,27 @@ esp_err_t Storage::writeItem(uint8_t nsIndex, ItemType datatype, const char* key
     }
 
     Page* findPage = nullptr;
+    bool matchedTypePageFound = false;
     Item item;
 
     esp_err_t err;
     if (datatype == ItemType::BLOB) {
         err = findItem(nsIndex, ItemType::BLOB_IDX, key, findPage, item);
+        if(err == ESP_OK) {
+            matchedTypePageFound = true;
+        }
     } else {
+#ifdef CONFIG_NVS_LEGACY_DUP_KEYS_COMPATIBILITY
         err = findItem(nsIndex, datatype, key, findPage, item);
+        if(err == ESP_OK && findPage != nullptr) {
+            matchedTypePageFound = true;
+        }
+#else
+        err = findItem(nsIndex, ItemType::ANY, key, findPage, item);
+        if(err == ESP_OK && datatype == item.datatype) {
+            matchedTypePageFound = true;
+        }
+#endif
     }
 
     if (err != ESP_OK && err != ESP_ERR_NVS_NOT_FOUND) {
@@ -301,7 +315,7 @@ esp_err_t Storage::writeItem(uint8_t nsIndex, ItemType datatype, const char* key
     if (datatype == ItemType::BLOB) {
         VerOffset prevStart,  nextStart;
         prevStart = nextStart = VerOffset::VER_0_OFFSET;
-        if (findPage) {
+        if (matchedTypePageFound) {
             // Do a sanity check that the item in question is actually being modified.
             // If it isn't, it is cheaper to purposefully not write out new data.
             // since it may invoke an erasure of flash.
@@ -335,7 +349,7 @@ esp_err_t Storage::writeItem(uint8_t nsIndex, ItemType datatype, const char* key
             return err;
         }
 
-        if (findPage) {
+        if (matchedTypePageFound) {
             /* Erase the blob with earlier version*/
             err = eraseMultiPageBlob(nsIndex, key, prevStart);
 
@@ -358,7 +372,7 @@ esp_err_t Storage::writeItem(uint8_t nsIndex, ItemType datatype, const char* key
         // Do a sanity check that the item in question is actually being modified.
         // If it isn't, it is cheaper to purposefully not write out new data.
         // since it may invoke an erasure of flash.
-        if (findPage != nullptr &&
+        if (matchedTypePageFound &&
                 findPage->cmpItem(nsIndex, datatype, key, data, dataSize) == ESP_OK) {
             return ESP_OK;
         }
@@ -392,12 +406,20 @@ esp_err_t Storage::writeItem(uint8_t nsIndex, ItemType datatype, const char* key
     if (findPage) {
         if (findPage->state() == Page::PageState::UNINITIALIZED ||
                 findPage->state() == Page::PageState::INVALID) {
+#ifdef CONFIG_NVS_LEGACY_DUP_KEYS_COMPATIBILITY
             err = findItem(nsIndex, datatype, key, findPage, item);
+#else
+            err = findItem(nsIndex, ItemType::ANY, key, findPage, item);
+#endif
             if (err != ESP_OK) {
                 return err;
             }
         }
+#ifdef CONFIG_NVS_LEGACY_DUP_KEYS_COMPATIBILITY
         err = findPage->eraseItem(nsIndex, datatype, key);
+#else
+        err = findPage->eraseItem(nsIndex, ItemType::ANY, key);
+#endif
         if (err == ESP_ERR_FLASH_OP_FAIL) {
             return ESP_ERR_NVS_REMOVE_FAILED;
         }

docs/en/api-reference/storage/nvs_flash.rst

@@ -35,12 +35,9 @@ NVS operates on key-value pairs. Keys are ASCII strings; the maximum key length
 
 Additional types, such as ``float`` and ``double`` might be added later.
 
-Keys are required to be unique. Assigning a new value to an existing key works as follows:
+Keys are required to be unique. Assigning a new value to an existing key replaces the old value and data type with the value and data type specified by a write operation.
 
--  If the new value is of the same type as the old one, value is updated.
--  If the new value has a different data type, an error is returned.
-
-Data type check is also performed when reading a value. An error is returned if the data type of the read operation does not match the data type of the value.
+A data type check is performed when reading a value. An error is returned if the data type expected by read operation does not match the data type of entry found for the key provided.
 
 
 Namespaces

docs/zh_CN/api-reference/storage/nvs_flash.rst

@@ -35,12 +35,9 @@ NVS 的操作对象为键值对，其中键是 ASCII 字符串，当前支持的
 
 后续可能会增加对 ``float`` 和 ``double`` 等其他类型数据的支持。
 
-键必须唯一。为现有的键写入新的值可能产生如下结果：
+键必须唯一。为现有的键写入新值时，会将旧的值及数据类型更新为写入操作指定的值和数据类型。
 
--  如果新旧值数据类型相同，则更新值；
--  如果新旧值数据类型不同，则返回错误。
-
-读取值时也会执行数据类型检查。如果读取操作的数据类型与该值的数据类型不匹配，则返回错误。
+读取值时会执行数据类型检查。如果读取操作预期的数据类型与对应键的数据类型不匹配，则返回错误。
 
 
 命名空间

tools/ci/check_copyright_ignore.txt

@@ -586,7 +586,6 @@ components/mbedtls/port/include/sha512_alt.h
 components/mbedtls/port/sha/dma/include/esp_sha_dma_priv.h
 components/mbedtls/port/sha/dma/sha.c
 components/mbedtls/port/sha/parallel_engine/sha.c
-components/nvs_flash/host_test/nvs_page_test/main/nvs_page_test.cpp
 components/nvs_flash/include/nvs_handle.hpp
 components/nvs_flash/src/nvs_cxx_api.cpp
 components/nvs_flash/src/nvs_encrypted_partition.hpp