From 0122ef0803afb7f861bfe9b44e8b335a9f05e863 Mon Sep 17 00:00:00 2001 From: ismail Date: Tue, 2 Sep 2025 17:02:05 +0300 Subject: [PATCH] Fix NVS key bug and optimize for single mode - Fixed a critical NVS key generation bug that would cause overwriting the values for all channels. - Optimized the code for single channel mode since no formatting required. - Improved multi-channel test coverage to cover that each value for each channel stored correctly. Refs #1096, #1098 and fixes #1100 --- src/relay_chn_nvs.c | 77 +++++++++++++++---- test_apps/main/test_relay_chn_nvs_multi.c | 91 ++++++++++++++++------- 2 files changed, 124 insertions(+), 44 deletions(-) diff --git a/src/relay_chn_nvs.c b/src/relay_chn_nvs.c index 5ba4fe0..7ddd5ae 100644 --- a/src/relay_chn_nvs.c +++ b/src/relay_chn_nvs.c @@ -5,15 +5,25 @@ */ #include "esp_check.h" +#include "nvs.h" #include "relay_chn_nvs.h" #define RELAY_CHN_KEY_DIR "dir" /*!< Direction key */ #if CONFIG_RELAY_CHN_ENABLE_RUN_LIMIT -#define RELAY_CHN_KEY_RLIM(ch) "rlim_%d" /*!< Run limit key */ +#if CONFIG_RELAY_CHN_COUNT > 1 +#define RELAY_CHN_KEY_RLIM_FMT "rlim_%d" /*!< Run limit key format for multi-channel */ +#else +#define RELAY_CHN_KEY_RLIM "rlim_0" /*!< Run limit key for single-channel */ +#endif #endif #if CONFIG_RELAY_CHN_ENABLE_TILTING -#define RELAY_CHN_KEY_TSENS(ch) "tsens_%d" /*!< Tilt sensitivity key */ -#define RELAY_CHN_KEY_TCNT(ch) "tcnt_%d" /*!< Tilt count key */ +#if CONFIG_RELAY_CHN_COUNT > 1 +#define RELAY_CHN_KEY_TSENS_FMT "tsens_%d" /*!< Tilt sensitivity key format for multi-channel */ +#define RELAY_CHN_KEY_TCNT_FMT "tcnt_%d" /*!< Tilt count key format for multi-channel */ +#else +#define RELAY_CHN_KEY_TSENS "tsens_0" /*!< Tilt sensitivity key for single-channel */ +#define RELAY_CHN_KEY_TCNT "tcnt_0" /*!< Tilt count key for single-channel */ +#endif #endif static const char *TAG = "RELAY_CHN_NVS"; @@ -44,12 +54,9 @@ esp_err_t relay_chn_nvs_init() esp_err_t relay_chn_nvs_set_direction(uint8_t ch, relay_chn_direction_t direction) { - uint8_t direction_val; + uint8_t direction_val = 0; esp_err_t ret = nvs_get_u8(relay_chn_nvs, RELAY_CHN_KEY_DIR, &direction_val); - if (ret == ESP_ERR_NVS_NOT_FOUND) { - // The key does not exist yet, set it to zero which is the default direction - direction_val = RELAY_CHN_DIRECTION_DEFAULT; - } else if (ret != ESP_OK) { + if (ret != ESP_OK && ret != ESP_ERR_NVS_NOT_FOUND) { ESP_RETURN_ON_ERROR(ret, TAG, "Failed to get direction from NVS with error: %s", esp_err_to_name(ret)); } direction_val &= ~(1 << ch); // Clear the bit for the channel @@ -75,7 +82,14 @@ esp_err_t relay_chn_nvs_get_direction(uint8_t ch, relay_chn_direction_t *directi #if CONFIG_RELAY_CHN_ENABLE_RUN_LIMIT esp_err_t relay_chn_nvs_set_run_limit(uint8_t ch, uint16_t limit_sec) { - esp_err_t ret = nvs_set_u16(relay_chn_nvs, RELAY_CHN_KEY_RLIM(ch), limit_sec); + esp_err_t ret; +#if CONFIG_RELAY_CHN_COUNT > 1 + char key[NVS_KEY_NAME_MAX_SIZE]; + snprintf(key, sizeof(key), RELAY_CHN_KEY_RLIM_FMT, ch); + ret = nvs_set_u16(relay_chn_nvs, key, limit_sec); +#else + ret = nvs_set_u16(relay_chn_nvs, RELAY_CHN_KEY_RLIM, limit_sec); +#endif ESP_RETURN_ON_ERROR(ret, TAG, "Failed to set run limit for channel %d", ch); return nvs_commit(relay_chn_nvs); } @@ -84,14 +98,27 @@ esp_err_t relay_chn_nvs_get_run_limit(uint8_t ch, uint16_t *limit_sec) { ESP_RETURN_ON_FALSE(limit_sec != NULL, ESP_ERR_INVALID_ARG, TAG, "Run limit value pointer is NULL"); - return nvs_get_u16(relay_chn_nvs, RELAY_CHN_KEY_RLIM(ch), limit_sec); +#if CONFIG_RELAY_CHN_COUNT > 1 + char key[NVS_KEY_NAME_MAX_SIZE]; + snprintf(key, sizeof(key), RELAY_CHN_KEY_RLIM_FMT, ch); + return nvs_get_u16(relay_chn_nvs, key, limit_sec); +#else + return nvs_get_u16(relay_chn_nvs, RELAY_CHN_KEY_RLIM, limit_sec); +#endif } #endif // CONFIG_RELAY_CHN_ENABLE_RUN_LIMIT == 1 #if CONFIG_RELAY_CHN_ENABLE_TILTING esp_err_t relay_chn_nvs_set_tilt_sensitivity(uint8_t ch, uint8_t sensitivity) { - esp_err_t ret = nvs_set_u8(relay_chn_nvs, RELAY_CHN_KEY_TSENS(ch), sensitivity); + esp_err_t ret; +#if CONFIG_RELAY_CHN_COUNT > 1 + char key[NVS_KEY_NAME_MAX_SIZE]; + snprintf(key, sizeof(key), RELAY_CHN_KEY_TSENS_FMT, ch); + ret = nvs_set_u8(relay_chn_nvs, key, sensitivity); +#else + ret = nvs_set_u8(relay_chn_nvs, RELAY_CHN_KEY_TSENS, sensitivity); +#endif ESP_RETURN_ON_ERROR(ret, TAG, "Failed to set tilt sensitivity for channel %d", ch); return nvs_commit(relay_chn_nvs); } @@ -100,22 +127,40 @@ esp_err_t relay_chn_nvs_get_tilt_sensitivity(uint8_t ch, uint8_t *sensitivity) { ESP_RETURN_ON_FALSE(sensitivity != NULL, ESP_ERR_INVALID_ARG, TAG, "Sensitivity pointer is NULL"); - return nvs_get_u8(relay_chn_nvs, RELAY_CHN_KEY_TSENS(ch), sensitivity); +#if CONFIG_RELAY_CHN_COUNT > 1 + char key[NVS_KEY_NAME_MAX_SIZE]; + snprintf(key, sizeof(key), RELAY_CHN_KEY_TSENS_FMT, ch); + return nvs_get_u8(relay_chn_nvs, key, sensitivity); +#else + return nvs_get_u8(relay_chn_nvs, RELAY_CHN_KEY_TSENS, sensitivity); +#endif } esp_err_t relay_chn_nvs_set_tilt_count(uint8_t ch, uint16_t tilt_count) { esp_err_t ret; - ret = nvs_set_u16(relay_chn_nvs, RELAY_CHN_KEY_TCNT(ch), tilt_count); +#if CONFIG_RELAY_CHN_COUNT > 1 + char key[NVS_KEY_NAME_MAX_SIZE]; + snprintf(key, sizeof(key), RELAY_CHN_KEY_TCNT_FMT, ch); + ret = nvs_set_u16(relay_chn_nvs, key, tilt_count); +#else + ret = nvs_set_u16(relay_chn_nvs, RELAY_CHN_KEY_TCNT, tilt_count); +#endif ESP_RETURN_ON_ERROR(ret, TAG, "Failed to save tilt_count tilt counter"); return nvs_commit(relay_chn_nvs); } esp_err_t relay_chn_nvs_get_tilt_count(uint8_t ch, uint16_t *tilt_count) { - ESP_RETURN_ON_FALSE(tilt_count != NULL, - ESP_ERR_INVALID_ARG, TAG, "Counter pointers are NULL"); - return nvs_get_u16(relay_chn_nvs, RELAY_CHN_KEY_TCNT(ch), tilt_count); + ESP_RETURN_ON_FALSE(tilt_count != NULL, ESP_ERR_INVALID_ARG, TAG, "Counter pointers are NULL"); + +#if CONFIG_RELAY_CHN_COUNT > 1 + char key[NVS_KEY_NAME_MAX_SIZE]; + snprintf(key, sizeof(key), RELAY_CHN_KEY_TCNT_FMT, ch); + return nvs_get_u16(relay_chn_nvs, key, tilt_count); +#else + return nvs_get_u16(relay_chn_nvs, RELAY_CHN_KEY_TCNT, tilt_count); +#endif } #endif // CONFIG_RELAY_CHN_ENABLE_TILTING diff --git a/test_apps/main/test_relay_chn_nvs_multi.c b/test_apps/main/test_relay_chn_nvs_multi.c index 1c9b243..4270d7d 100644 --- a/test_apps/main/test_relay_chn_nvs_multi.c +++ b/test_apps/main/test_relay_chn_nvs_multi.c @@ -54,15 +54,15 @@ TEST_CASE("Test relay_chn_nvs_erase_all", "[relay_chn][nvs]") // Store some test data first relay_chn_direction_t direction = RELAY_CHN_DIRECTION_FLIPPED; - for (int channel = 0; channel < CONFIG_RELAY_CHN_COUNT; channel++) { - TEST_ESP_OK(relay_chn_nvs_set_direction(0, direction)); - } + // Set direction for all channels + TEST_ESP_OK(relay_chn_nvs_set_direction(0, direction)); + TEST_ESP_OK(relay_chn_nvs_set_direction(1, direction)); #if CONFIG_RELAY_CHN_ENABLE_TILTING uint8_t sensitivity = 50; for (int channel = 0; channel < CONFIG_RELAY_CHN_COUNT; channel++) { - TEST_ESP_OK(relay_chn_nvs_set_tilt_sensitivity(0, sensitivity)); - TEST_ESP_OK(relay_chn_nvs_set_tilt_count(0, 100)); + TEST_ESP_OK(relay_chn_nvs_set_tilt_sensitivity(channel, sensitivity)); + TEST_ESP_OK(relay_chn_nvs_set_tilt_count(channel, 100 + channel)); } #endif @@ -74,11 +74,13 @@ TEST_CASE("Test relay_chn_nvs_erase_all", "[relay_chn][nvs]") TEST_ASSERT_EQUAL(ESP_ERR_NVS_NOT_FOUND, relay_chn_nvs_get_direction(0, &read_direction)); #if CONFIG_RELAY_CHN_ENABLE_TILTING - uint8_t read_sensitivity; - TEST_ASSERT_EQUAL(ESP_ERR_NVS_NOT_FOUND, relay_chn_nvs_get_tilt_sensitivity(0, &read_sensitivity)); + for (int channel = 0; channel < CONFIG_RELAY_CHN_COUNT; channel++) { + uint8_t read_sensitivity; + TEST_ASSERT_EQUAL(ESP_ERR_NVS_NOT_FOUND, relay_chn_nvs_get_tilt_sensitivity(channel, &read_sensitivity)); - uint16_t tilt_count; - TEST_ASSERT_EQUAL(ESP_ERR_NVS_NOT_FOUND, relay_chn_nvs_get_tilt_count(0, &tilt_count)); + uint16_t tilt_count; + TEST_ASSERT_EQUAL(ESP_ERR_NVS_NOT_FOUND, relay_chn_nvs_get_tilt_count(channel, &tilt_count)); + } #endif TEST_ESP_OK(relay_chn_nvs_deinit()); @@ -89,14 +91,34 @@ TEST_CASE("Test run limit setting and getting", "[relay_chn][nvs][run_limit]") { TEST_ESP_OK(relay_chn_nvs_init()); - const uint16_t run_limit_sec = 32; + // Use different values for each channel to detect overwrites + uint16_t test_limits[CONFIG_RELAY_CHN_COUNT]; + for (uint8_t i = 0; i < CONFIG_RELAY_CHN_COUNT; i++) { + test_limits[i] = 30 + i; // e.g., 30, 31, 32... + } + + // 1. Set all values first + for (uint8_t i = 0; i < CONFIG_RELAY_CHN_COUNT; i++) { + TEST_ESP_OK(relay_chn_nvs_set_run_limit(i, test_limits[i])); + } + + // 2. Then, read them all back and verify for (uint8_t i = 0; i < CONFIG_RELAY_CHN_COUNT; i++) { - TEST_ESP_OK(relay_chn_nvs_set_run_limit(i, run_limit_sec)); - uint16_t run_limit_read; TEST_ESP_OK(relay_chn_nvs_get_run_limit(i, &run_limit_read)); - TEST_ASSERT_EQUAL(run_limit_sec, run_limit_read); + TEST_ASSERT_EQUAL_UINT16(test_limits[i], run_limit_read); } + + // 3. Verify that changing one channel doesn't affect another + uint16_t new_limit_ch0 = 99; + TEST_ESP_OK(relay_chn_nvs_set_run_limit(0, new_limit_ch0)); + + uint16_t read_val_ch0, read_val_ch1; + TEST_ESP_OK(relay_chn_nvs_get_run_limit(0, &read_val_ch0)); + TEST_ESP_OK(relay_chn_nvs_get_run_limit(1, &read_val_ch1)); + TEST_ASSERT_EQUAL_UINT16(new_limit_ch0, read_val_ch0); + TEST_ASSERT_EQUAL_UINT16(test_limits[1], read_val_ch1); // Should still be the old value + TEST_ESP_OK(relay_chn_nvs_deinit()); } #endif @@ -106,14 +128,21 @@ TEST_CASE("Test sensitivity setting and getting", "[relay_chn][nvs][tilt]") { TEST_ESP_OK(relay_chn_nvs_init()); - const uint8_t test_sensitivity = 75; - uint8_t sensitivity; + uint8_t test_sensitivities[CONFIG_RELAY_CHN_COUNT]; + for (uint8_t i = 0; i < CONFIG_RELAY_CHN_COUNT; i++) { + test_sensitivities[i] = 70 + i; // e.g., 70, 71, 72... + } - // Test all channels - for (int channel = 0; channel < CONFIG_RELAY_CHN_COUNT; channel++) { - TEST_ESP_OK(relay_chn_nvs_set_tilt_sensitivity(channel, test_sensitivity)); - TEST_ESP_OK(relay_chn_nvs_get_tilt_sensitivity(channel, &sensitivity)); - TEST_ASSERT_EQUAL(test_sensitivity, sensitivity); + // 1. Set all values first + for (uint8_t i = 0; i < CONFIG_RELAY_CHN_COUNT; i++) { + TEST_ESP_OK(relay_chn_nvs_set_tilt_sensitivity(i, test_sensitivities[i])); + } + + // 2. Then, read them all back and verify + for (uint8_t i = 0; i < CONFIG_RELAY_CHN_COUNT; i++) { + uint8_t sensitivity_read; + TEST_ESP_OK(relay_chn_nvs_get_tilt_sensitivity(i, &sensitivity_read)); + TEST_ASSERT_EQUAL_UINT8(test_sensitivities[i], sensitivity_read); } TEST_ESP_OK(relay_chn_nvs_deinit()); @@ -123,15 +152,21 @@ TEST_CASE("Test tilt counter operations", "[relay_chn][nvs][tilt]") { TEST_ESP_OK(relay_chn_nvs_init()); - const uint16_t tilt_count = 100; - uint16_t tilt_count_read; + uint16_t test_counts[CONFIG_RELAY_CHN_COUNT]; + for (uint8_t i = 0; i < CONFIG_RELAY_CHN_COUNT; i++) { + test_counts[i] = 100 + i; // e.g., 100, 101, 102... + } - // Test all channels - for (int channel = 0; channel < CONFIG_RELAY_CHN_COUNT; channel++) { - // Test setting counters - TEST_ESP_OK(relay_chn_nvs_set_tilt_count(channel, tilt_count)); - TEST_ESP_OK(relay_chn_nvs_get_tilt_count(channel, &tilt_count_read)); - TEST_ASSERT_EQUAL(tilt_count, tilt_count_read); + // 1. Set all values first + for (uint8_t i = 0; i < CONFIG_RELAY_CHN_COUNT; i++) { + TEST_ESP_OK(relay_chn_nvs_set_tilt_count(i, test_counts[i])); + } + + // 2. Then, read them all back and verify + for (uint8_t i = 0; i < CONFIG_RELAY_CHN_COUNT; i++) { + uint16_t count_read; + TEST_ESP_OK(relay_chn_nvs_get_tilt_count(i, &count_read)); + TEST_ASSERT_EQUAL_UINT16(test_counts[i], count_read); } TEST_ESP_OK(relay_chn_nvs_deinit());