From 060b63bcfef0fc57204d78bec1c3c9fb4d2ab89c Mon Sep 17 00:00:00 2001 From: Doru Gucea Date: Mon, 4 Oct 2021 00:01:50 -0700 Subject: [PATCH] Fixes after review Signed-off-by: Doru Gucea --- .../k32w/k32w0/KeyValueStoreManagerImpl.cpp | 70 +++++++++++-------- 1 file changed, 39 insertions(+), 31 deletions(-) diff --git a/src/platform/nxp/k32w/k32w0/KeyValueStoreManagerImpl.cpp b/src/platform/nxp/k32w/k32w0/KeyValueStoreManagerImpl.cpp index 8d14cf04be81e8..dbba863e366d14 100644 --- a/src/platform/nxp/k32w/k32w0/KeyValueStoreManagerImpl.cpp +++ b/src/platform/nxp/k32w/k32w0/KeyValueStoreManagerImpl.cpp @@ -37,8 +37,9 @@ namespace chip { namespace DeviceLayer { namespace PersistedStorage { -/* TODO: adjust this value */ -#define MAX_NO_OF_KEYS 20 +/* TODO: adjust these values */ +constexpr size_t kMaxNumberOfKeys = 20; +constexpr size_t kMaxKeyValueBytes = 255; KeyValueStoreManagerImpl KeyValueStoreManagerImpl::sInstance; @@ -48,22 +49,19 @@ KeyValueStoreManagerImpl KeyValueStoreManagerImpl::sInstance; */ std::unordered_map g_kvs_map; -/* list containing used PDM identifiers */ -std::list g_key_ids_list; - -/* max no of bytes for a key */ -#define MAX_KEY_VALUE 255 +/* set containing used PDM identifiers */ +std::set g_key_ids_set; /* used to check if we need to restore values from flash (e.g.: reset) */ -static bool g_restored_from_flash = FALSE; +static bool g_restored_from_flash = false; CHIP_ERROR RestoreFromFlash() { - CHIP_ERROR err = CHIP_NO_ERROR; - uint8_t key_id = 0; - char key_string_id[MAX_KEY_VALUE] = { 0 }; - size_t key_string_id_size = 0; - uint8_t pdm_id_kvs = chip::DeviceLayer::Internal::K32WConfig::kPDMId_KVS; + CHIP_ERROR err = CHIP_NO_ERROR; + uint8_t key_id = 0; + char key_string_id[kMaxKeyValueBytes] = { 0 }; + size_t key_string_id_size = 0; + uint8_t pdm_id_kvs = chip::DeviceLayer::Internal::K32WConfig::kPDMId_KVS; if (g_restored_from_flash) { @@ -71,32 +69,41 @@ CHIP_ERROR RestoreFromFlash() return err; } - for (key_id = 0; key_id < MAX_NO_OF_KEYS; key_id++) + for (key_id = 0; key_id < kMaxNumberOfKeys; key_id++) { - /* key was saved as string in flash (key_string_id) using (key_id + MAX_NO_OF_KEYS) as PDM key + /* key was saved as string in flash (key_string_id) using (key_id + kMaxNumberOfKeys) as PDM key * value corresponding to key_string_id was saved in flash using key_id as PDM key */ err = chip::DeviceLayer::Internal::K32WConfig::ReadConfigValueStr( - chip::DeviceLayer::Internal::K32WConfigKey(pdm_id_kvs, key_id + MAX_NO_OF_KEYS), key_string_id, MAX_KEY_VALUE, + chip::DeviceLayer::Internal::K32WConfigKey(pdm_id_kvs, key_id + kMaxNumberOfKeys), key_string_id, kMaxKeyValueBytes, key_string_id_size); if (err == CHIP_DEVICE_ERROR_CONFIG_NOT_FOUND) { + err = CHIP_NO_ERROR; continue; } + else if (err != CHIP_NO_ERROR) + { + break; + } if (key_string_id_size) { - g_key_ids_list.push_back(key_id); - g_kvs_map.insert(std::make_pair(std::string(key_string_id), key_id)); + g_key_ids_set.insert(key_id); + if (!g_kvs_map.insert(std::make_pair(std::string(key_string_id), key_id)).second) + { + /* key collision is not expected when restoring from flash */ + abort(); + } key_string_id_size = 0; ChipLogProgress(DeviceLayer, "KVS, restored key [%s] from flash with PDM key: %i", key_string_id, key_id); } } - g_restored_from_flash = TRUE; + g_restored_from_flash = true; return err; } @@ -131,7 +138,7 @@ CHIP_ERROR KeyValueStoreManagerImpl::_Put(const char * key, const void * value, { CHIP_ERROR err = CHIP_ERROR_INVALID_ARGUMENT; uint8_t key_id; - bool_t put_key = FALSE; + bool_t put_key = false; uint8_t pdm_id_kvs = chip::DeviceLayer::Internal::K32WConfig::kPDMId_KVS; std::unordered_map::const_iterator it; @@ -139,22 +146,23 @@ CHIP_ERROR KeyValueStoreManagerImpl::_Put(const char * key, const void * value, if ((it = g_kvs_map.find(key)) == g_kvs_map.end()) /* new key */ { - for (key_id = 0; key_id < MAX_NO_OF_KEYS; key_id++) + for (key_id = 0; key_id < kMaxNumberOfKeys; key_id++) { - std::list::iterator iter = std::find(g_key_ids_list.begin(), g_key_ids_list.end(), key_id); + std::set::iterator iter = std::find(g_key_ids_set.begin(), g_key_ids_set.end(), key_id); - if (iter == g_key_ids_list.end()) + if (iter == g_key_ids_set.end()) { - g_key_ids_list.push_back(key_id); + assert(g_key_ids_set.size() < kMaxNumberOfKeys); + g_key_ids_set.insert(key_id); - put_key = TRUE; + put_key = true; break; } } } else /* overwrite key */ { - put_key = TRUE; + put_key = true; key_id = it->second; } @@ -169,9 +177,9 @@ CHIP_ERROR KeyValueStoreManagerImpl::_Put(const char * key, const void * value, /* save the 'key' in flash such that it can be retrieved later on */ if (err == CHIP_NO_ERROR) { - ChipLogProgress(DeviceLayer, "KVS, save in flash key [%s] with PDM key: %i", key, key_id + MAX_NO_OF_KEYS); + ChipLogProgress(DeviceLayer, "KVS, save in flash key [%s] with PDM key: %i", key, key_id + kMaxNumberOfKeys); err = chip::DeviceLayer::Internal::K32WConfig::WriteConfigValueStr( - chip::DeviceLayer::Internal::K32WConfigKey(pdm_id_kvs, key_id + MAX_NO_OF_KEYS), key, strlen(key)); + chip::DeviceLayer::Internal::K32WConfigKey(pdm_id_kvs, key_id + kMaxNumberOfKeys), key, strlen(key)); } } @@ -191,7 +199,7 @@ CHIP_ERROR KeyValueStoreManagerImpl::_Delete(const char * key) if ((it = g_kvs_map.find(key)) != g_kvs_map.end()) { key_id = it->second; - g_key_ids_list.remove(key_id); + g_key_ids_set.erase(key_id); g_kvs_map.erase(it); ChipLogProgress(DeviceLayer, "KVS, delete key [%s] with PDM key: %i", key, key_id); @@ -201,9 +209,9 @@ CHIP_ERROR KeyValueStoreManagerImpl::_Delete(const char * key) /* also delete the 'key string' from flash */ if (err == CHIP_NO_ERROR) { - ChipLogProgress(DeviceLayer, "KVS, delete key [%s] with PDM key: %i", key, key_id + MAX_NO_OF_KEYS); + ChipLogProgress(DeviceLayer, "KVS, delete key [%s] with PDM key: %i", key, key_id + kMaxNumberOfKeys); err = chip::DeviceLayer::Internal::K32WConfig::ClearConfigValue( - chip::DeviceLayer::Internal::K32WConfigKey(pdm_id_kvs, key_id + MAX_NO_OF_KEYS)); + chip::DeviceLayer::Internal::K32WConfigKey(pdm_id_kvs, key_id + kMaxNumberOfKeys)); } }