From e314f42b0c71fd96913e0f4f8b79d4e16e69982a Mon Sep 17 00:00:00 2001 From: Ivan Grokhotkov Date: Thu, 3 Nov 2016 20:18:30 +0800 Subject: [PATCH] nvs: fix Page::findItem and Storage::findItem regression When read caching was added, Page::findItem started modifying itemIndex reference argument even if item wasn't found. Incidentally, Storage::findItem reused itemIndex when starting search at next page. So, - if the first page had a cached index (findItem was called for that page), and it pointed to a non-zero index, - first page has a few empty items at the end (but is marked full), - next search looked up the item on the second page, - index of the item on the second page was less than the cached index on the first page, then the search would fail because cached starting index was reused. This change fixes both sides of the problem: - Page::findItem shouldn't modify itemIndex argument if item is not found - Storage::findItem should not reuse itemIndex between pages Two tests have been added. --- components/nvs_flash/src/nvs_page.cpp | 11 ++--- components/nvs_flash/src/nvs_storage.cpp | 2 +- components/nvs_flash/test/test_nvs.cpp | 54 ++++++++++++++++++++++++ 3 files changed, 61 insertions(+), 6 deletions(-) diff --git a/components/nvs_flash/src/nvs_page.cpp b/components/nvs_flash/src/nvs_page.cpp index 23cefd1aad..d2ca225352 100644 --- a/components/nvs_flash/src/nvs_page.cpp +++ b/components/nvs_flash/src/nvs_page.cpp @@ -656,19 +656,20 @@ esp_err_t Page::findItem(uint8_t nsIndex, ItemType datatype, const char* key, si if (mState == PageState::CORRUPT || mState == PageState::INVALID || mState == PageState::UNINITIALIZED) { return ESP_ERR_NVS_NOT_FOUND; } - - if (itemIndex >= ENTRY_COUNT) { + + size_t findBeginIndex = itemIndex; + if (findBeginIndex >= ENTRY_COUNT) { return ESP_ERR_NVS_NOT_FOUND; } CachedFindInfo findInfo(nsIndex, datatype, key); if (mFindInfo == findInfo) { - itemIndex = mFindInfo.itemIndex(); + findBeginIndex = mFindInfo.itemIndex(); } size_t start = mFirstUsedEntry; - if (itemIndex > mFirstUsedEntry && itemIndex < ENTRY_COUNT) { - start = itemIndex; + if (findBeginIndex > mFirstUsedEntry && findBeginIndex < ENTRY_COUNT) { + start = findBeginIndex; } size_t end = mNextFreeEntry; diff --git a/components/nvs_flash/src/nvs_storage.cpp b/components/nvs_flash/src/nvs_storage.cpp index eb90cac5bc..cacfbd4022 100644 --- a/components/nvs_flash/src/nvs_storage.cpp +++ b/components/nvs_flash/src/nvs_storage.cpp @@ -71,8 +71,8 @@ esp_err_t Storage::init(uint32_t baseSector, uint32_t sectorCount) esp_err_t Storage::findItem(uint8_t nsIndex, ItemType datatype, const char* key, Page* &page, Item& item) { - size_t itemIndex = 0; for (auto it = std::begin(mPageManager); it != std::end(mPageManager); ++it) { + size_t itemIndex = 0; auto err = it->findItem(nsIndex, datatype, key, itemIndex, item); if (err == ESP_OK) { page = it; diff --git a/components/nvs_flash/test/test_nvs.cpp b/components/nvs_flash/test/test_nvs.cpp index 528c9df686..81bf7fd216 100644 --- a/components/nvs_flash/test/test_nvs.cpp +++ b/components/nvs_flash/test/test_nvs.cpp @@ -300,6 +300,27 @@ TEST_CASE("storage doesn't add duplicates within multiple pages", "[nvs]") CHECK(page.findItem(1, itemTypeOf(), "bar") == ESP_OK); } +TEST_CASE("storage can find items on second page if first is not fully written and has cached search data", "[nvs]") +{ + SpiFlashEmulator emu(3); + Storage storage; + CHECK(storage.init(0, 3) == ESP_OK); + int bar = 0; + uint8_t bigdata[100 * 32] = {0}; + // write one big chunk of data + ESP_ERROR_CHECK(storage.writeItem(0, ItemType::BLOB, "first", bigdata, sizeof(bigdata))); + + // write second one; it will not fit into the first page + ESP_ERROR_CHECK(storage.writeItem(0, ItemType::BLOB, "second", bigdata, sizeof(bigdata))); + + size_t size; + ESP_ERROR_CHECK(storage.getItemDataSize(0, ItemType::BLOB, "first", size)); + CHECK(size == sizeof(bigdata)); + ESP_ERROR_CHECK(storage.getItemDataSize(0, ItemType::BLOB, "second", size)); + CHECK(size == sizeof(bigdata)); +} + + TEST_CASE("can write and read variable length data lots of times", "[nvs]") { SpiFlashEmulator emu(8); @@ -1055,6 +1076,39 @@ TEST_CASE("crc error in variable length item is handled", "[nvs]") } +TEST_CASE("read/write failure (TW8406)", "[nvs]") +{ + SpiFlashEmulator emu(3); + nvs_flash_init_custom(0, 3); + for (int attempts = 0; attempts < 3; ++attempts) { + int i = 0; + nvs_handle light_handle = 0; + char key[15] = {0}; + char data[76] = {12, 13, 14, 15, 16}; + uint8_t number = 20; + size_t data_len = sizeof(data); + + ESP_ERROR_CHECK(nvs_open("LIGHT", NVS_READWRITE, &light_handle)); + ESP_ERROR_CHECK(nvs_set_u8(light_handle, "RecordNum", number)); + for (i = 0; i < number; ++i) { + sprintf(key, "light%d", i); + ESP_ERROR_CHECK(nvs_set_blob(light_handle, key, data, sizeof(data))); + } + nvs_commit(light_handle); + + uint8_t get_number = 0; + ESP_ERROR_CHECK(nvs_get_u8(light_handle, "RecordNum", &get_number)); + REQUIRE(number == get_number); + for (i = 0; i < number; ++i) { + char data[76] = {0}; + sprintf(key, "light%d", i); + ESP_ERROR_CHECK(nvs_get_blob(light_handle, key, data, &data_len)); + } + nvs_close(light_handle); + } +} + + TEST_CASE("dump all performance data", "[nvs]") { std::cout << "====================" << std::endl << "Dumping benchmarks" << std::endl;