From 5dcdef0639849b82af1d275fd381beb90acf5b28 Mon Sep 17 00:00:00 2001 From: Ivan Grokhotkov Date: Thu, 14 Feb 2019 11:17:48 +0800 Subject: [PATCH 01/11] kconfig: fix compatibility with very old versions of flex See https://github.com/crosstool-ng/crosstool-ng/commit/4e762e4918d8755e762db1db328760dfa5fc7a14 Closes https://github.com/espressif/esp-idf/issues/2703 --- tools/kconfig/Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/kconfig/Makefile b/tools/kconfig/Makefile index cd0064043..106ffb6b4 100644 --- a/tools/kconfig/Makefile +++ b/tools/kconfig/Makefile @@ -334,7 +334,7 @@ conf-idf: $(conf-objs) zconf.tab.c: zconf.lex.c zconf.lex.c: $(SRCDIR)/zconf.l - flex -L -P zconf -o zconf.lex.c $< + flex -L -Pzconf -ozconf.lex.c $< zconf.hash.c: $(SRCDIR)/zconf.gperf # strip CRs on Windows systems where gperf will otherwise barf on them From 8bbd99ad9e8028cc1811ce0af1800cc9d4ef80e5 Mon Sep 17 00:00:00 2001 From: Ivan Grokhotkov Date: Wed, 13 Feb 2019 11:40:48 +0800 Subject: [PATCH 02/11] make: fix issues related to EXTRA_COMPONENT_DIRS 1. When one of the COMPONENT_DIRS points to a component directory (i.e. a directory containing component.mk, not a directory of multiple components), and there is a subdirectory in it which also contains a component, the subdirectory was mistakenly added to the list of components and compiled. For example: main/ component.mk main.c test/ component.mk test_main.c Would compile test_main.c and link libtest.a. 2. When one of the COMPONENT_DIRS points to a component directory, and the parent directory contained a directory with the same name as another component, that directory would be mistakenly added to the COMPONENT_PATHS. For example: esp/ esp-idf/ esp32/ (random stuff) mycomponent/ component.mk mycomponent.c myproject/ main/ Makefile and Makefile sets EXTRA_COMPONENT_DIRS=$(realpath ../mycomponent), then "esp32" directory which is at the same level as mycomponent was added to COMPONENT_PATHS. 3. If EXTRA_COMPONENT_DIRS pointed to a directory with a list of components, and one of the subdirectories was not a component, but had the same name as another component, than that directory would be mistakenly added to COMPONENT_PATHS instead of the real esp32 component directory. For example: my_components/ my_component/ component.mk my_component.c esp32/ (some random stuff) and EXTRA_COMPONENT_DIRS would point to my_components/, then "esp32" directory would be added to COMPONENT_PATHS instead of the real esp32 component directory. --- make/project.mk | 45 +++++++++++++++++++++++++++-------- tools/ci/test_build_system.sh | 31 +++++++++++++++++++++++- 2 files changed, 65 insertions(+), 11 deletions(-) diff --git a/make/project.mk b/make/project.mk index 7b9bd9b1d..f5d29aa25 100644 --- a/make/project.mk +++ b/make/project.mk @@ -133,6 +133,9 @@ ifndef COMPONENT_DIRS EXTRA_COMPONENT_DIRS ?= COMPONENT_DIRS := $(PROJECT_PATH)/components $(EXTRA_COMPONENT_DIRS) $(IDF_PATH)/components $(PROJECT_PATH)/main endif +# Make sure that every directory in the list is an absolute path without trailing slash. +# This is necessary to split COMPONENT_DIRS into SINGLE_COMPONENT_DIRS and MULTI_COMPONENT_DIRS below. +COMPONENT_DIRS := $(foreach cd,$(COMPONENT_DIRS),$(abspath $(cd))) export COMPONENT_DIRS ifdef SRCDIRS @@ -140,33 +143,55 @@ $(warning SRCDIRS variable is deprecated. These paths can be added to EXTRA_COMP COMPONENT_DIRS += $(abspath $(SRCDIRS)) endif -# The project Makefile can define a list of components, but if it does not do this we just take all available components -# in the component dirs. A component is COMPONENT_DIRS directory, or immediate subdirectory, +# List of component directories, i.e. directories which contain a component.mk file +SINGLE_COMPONENT_DIRS := $(abspath $(dir $(dir $(foreach cd,$(COMPONENT_DIRS),\ + $(wildcard $(cd)/component.mk))))) + +# List of components directories, i.e. directories which may contain components +MULTI_COMPONENT_DIRS := $(filter-out $(SINGLE_COMPONENT_DIRS),$(COMPONENT_DIRS)) + +# The project Makefile can define a list of components, but if it does not do this +# we just take all available components in the component dirs. +# A component is COMPONENT_DIRS directory, or immediate subdirectory, # which contains a component.mk file. # # Use the "make list-components" target to debug this step. ifndef COMPONENTS # Find all component names. The component names are the same as the # directories they're in, so /bla/components/mycomponent/component.mk -> mycomponent. -COMPONENTS := $(dir $(foreach cd,$(COMPONENT_DIRS), \ - $(wildcard $(cd)/*/component.mk) $(wildcard $(cd)/component.mk) \ - )) +# We need to do this for MULTI_COMPONENT_DIRS only, since SINGLE_COMPONENT_DIRS +# are already known to contain component.mk. +COMPONENTS := $(dir $(foreach cd,$(MULTI_COMPONENT_DIRS),$(wildcard $(cd)/*/component.mk))) \ + $(SINGLE_COMPONENT_DIRS) COMPONENTS := $(sort $(foreach comp,$(COMPONENTS),$(lastword $(subst /, ,$(comp))))) endif -# After a full manifest of component names is determined, subtract the ones explicitly omitted by the project Makefile. +# After a full manifest of component names is determined, subtract the ones explicitly +# omitted by the project Makefile. +EXCLUDE_COMPONENTS ?= ifdef EXCLUDE_COMPONENTS -COMPONENTS := $(filter-out $(subst ",,$(EXCLUDE_COMPONENTS)), $(COMPONENTS)) +COMPONENTS := $(filter-out $(subst ",,$(EXCLUDE_COMPONENTS)), $(COMPONENTS)) # to keep syntax highlighters happy: ")) endif export COMPONENTS # Resolve all of COMPONENTS into absolute paths in COMPONENT_PATHS. +# For each entry in COMPONENT_DIRS: +# - either this is directory with multiple components, in which case check that +# a subdirectory with component name exists, and it contains a component.mk file. +# - or, this is a directory of a single component, in which case the name of this +# directory has to match the component name # # If a component name exists in multiple COMPONENT_DIRS, we take the first match. # # NOTE: These paths must be generated WITHOUT a trailing / so we # can use $(notdir x) to get the component name. -COMPONENT_PATHS := $(foreach comp,$(COMPONENTS),$(firstword $(foreach cd,$(COMPONENT_DIRS),$(wildcard $(dir $(cd))$(comp) $(cd)/$(comp))))) +COMPONENT_PATHS := $(foreach comp,$(COMPONENTS),\ + $(firstword $(foreach cd,$(COMPONENT_DIRS),\ + $(if $(findstring $(cd),$(MULTI_COMPONENT_DIRS)),\ + $(abspath $(dir $(wildcard $(cd)/$(comp)/component.mk))),)\ + $(if $(findstring $(cd),$(SINGLE_COMPONENT_DIRS)),\ + $(if $(filter $(comp),$(notdir $(cd))),$(cd),),)\ + ))) export COMPONENT_PATHS TEST_COMPONENTS ?= @@ -263,7 +288,7 @@ LDFLAGS ?= -nostdlib \ # before including project.mk. Default flags will be added before the ones provided in application Makefile. # CPPFLAGS used by C preprocessor -# If any flags are defined in application Makefile, add them at the end. +# If any flags are defined in application Makefile, add them at the end. CPPFLAGS ?= EXTRA_CPPFLAGS ?= CPPFLAGS := -DESP_PLATFORM -D IDF_VER=\"$(IDF_VER)\" -MMD -MP $(CPPFLAGS) $(EXTRA_CPPFLAGS) @@ -575,7 +600,7 @@ list-components: $(info $(TEST_COMPONENTS_LIST)) $(info $(call dequote,$(SEPARATOR))) $(info TEST_EXCLUDE_COMPONENTS (list of test excluded names)) - $(info $(if $(EXCLUDE_COMPONENTS) || $(TEST_EXCLUDE_COMPONENTS),$(EXCLUDE_COMPONENTS) $(TEST_EXCLUDE_COMPONENTS),(none provided))) + $(info $(if $(EXCLUDE_COMPONENTS) || $(TEST_EXCLUDE_COMPONENTS),$(EXCLUDE_COMPONENTS) $(TEST_EXCLUDE_COMPONENTS),(none provided))) $(info $(call dequote,$(SEPARATOR))) $(info COMPONENT_PATHS (paths to all components):) $(foreach cp,$(COMPONENT_PATHS),$(info $(cp))) diff --git a/tools/ci/test_build_system.sh b/tools/ci/test_build_system.sh index f3fb2b6b1..02ffd7f6f 100755 --- a/tools/ci/test_build_system.sh +++ b/tools/ci/test_build_system.sh @@ -238,7 +238,7 @@ function run_tests() ( make 2>&1 | grep "does not fit in configured flash size 1MB" ) || failure "Build didn't fail with expected flash size failure message" mv sdkconfig.bak sdkconfig - print_status "sdkconfig should have contents both files: sdkconfig and sdkconfig.defaults" + print_status "sdkconfig should have contents of both files: sdkconfig and sdkconfig.defaults" make clean > /dev/null; rm -f sdkconfig.defaults; rm -f sdkconfig; @@ -247,6 +247,35 @@ function run_tests() make defconfig > /dev/null; grep "CONFIG_PARTITION_TABLE_OFFSET=0x10000" sdkconfig || failure "The define from sdkconfig.defaults should be into sdkconfig" grep "CONFIG_PARTITION_TABLE_TWO_OTA=y" sdkconfig || failure "The define from sdkconfig should be into sdkconfig" + rm sdkconfig sdkconfig.defaults + make defconfig + + print_status "Empty directory not treated as a component" + mkdir -p components/esp32 + make || failure "Failed to build with empty esp32 directory in components" + rm -rf components + + print_status "If a component directory is added to COMPONENT_DIRS, its subdirectories are not added" + mkdir -p main/test + touch main/test/component.mk + echo "#error This should not be built" > main/test/test.c + make || failure "COMPONENT_DIRS has added component subdirectory to the build" + rm -rf main/test + + print_status "If a component directory is added to COMPONENT_DIRS, its sibling directories are not added" + mkdir -p mycomponents/mycomponent + touch mycomponents/mycomponent/component.mk + # first test by adding single component directory to EXTRA_COMPONENT_DIRS + mkdir -p mycomponents/esp32 + touch mycomponents/esp32/component.mk + make EXTRA_COMPONENT_DIRS=$PWD/mycomponents/mycomponent || failure "EXTRA_COMPONENT_DIRS has added a sibling directory" + rm -rf mycomponents/esp32 + # now the same thing, but add a components directory + mkdir -p esp32 + touch esp32/component.mk + make EXTRA_COMPONENT_DIRS=$PWD/mycomponents || failure "EXTRA_COMPONENT_DIRS has added a sibling directory" + rm -rf esp32 + rm -rf mycomponents print_status "All tests completed" if [ -n "${FAILURES}" ]; then From b4727a87651a26736b8eb046a2a9ea06b94b35d6 Mon Sep 17 00:00:00 2001 From: Ivan Grokhotkov Date: Thu, 29 Nov 2018 15:15:21 +0800 Subject: [PATCH 03/11] =?UTF-8?q?soc/rtc=5Fclk:=20don=E2=80=99t=20clear=20?= =?UTF-8?q?DPORT=5FCPUPERIOD=5FSEL=20when=20switching=20to=20XTAL?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is not necessary since RTC_CNTL_SOC_CLK_SEL is set before this. --- components/soc/esp32/rtc_clk.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/components/soc/esp32/rtc_clk.c b/components/soc/esp32/rtc_clk.c index 8c2948553..3a96c75e2 100644 --- a/components/soc/esp32/rtc_clk.c +++ b/components/soc/esp32/rtc_clk.c @@ -395,7 +395,6 @@ void rtc_clk_cpu_freq_to_xtal(int freq, int div) REG_WRITE(APB_CTRL_XTAL_TICK_CONF_REG, freq * MHZ / REF_CLK_FREQ - 1); /* switch clock source */ REG_SET_FIELD(RTC_CNTL_CLK_CONF_REG, RTC_CNTL_SOC_CLK_SEL, RTC_CNTL_SOC_CLK_SEL_XTL); - DPORT_REG_WRITE(DPORT_CPU_PER_CONF_REG, 0); /* clear DPORT_CPUPERIOD_SEL */ rtc_clk_apb_freq_update(freq * MHZ); /* lower the voltage */ if (freq <= 2) { @@ -411,7 +410,6 @@ static void rtc_clk_cpu_freq_to_8m() REG_SET_FIELD(RTC_CNTL_REG, RTC_CNTL_DIG_DBIAS_WAK, DIG_DBIAS_XTAL); REG_SET_FIELD(APB_CTRL_SYSCLK_CONF_REG, APB_CTRL_PRE_DIV_CNT, 0); REG_SET_FIELD(RTC_CNTL_CLK_CONF_REG, RTC_CNTL_SOC_CLK_SEL, RTC_CNTL_SOC_CLK_SEL_8M); - DPORT_REG_WRITE(DPORT_CPU_PER_CONF_REG, 0); // clear DPORT_CPUPERIOD_SEL rtc_clk_apb_freq_update(RTC_FAST_CLK_FREQ_8M); } From f78c96a3d719a8708ece7cbac0cac587cefc290f Mon Sep 17 00:00:00 2001 From: Ivan Grokhotkov Date: Thu, 29 Nov 2018 15:18:11 +0800 Subject: [PATCH 04/11] bootloader: check previously used clock frequency at run time In the situation when bootloader was compiled for 240MHz, and app was compiled for 160MHz, and the chip is a revision 0 chip, the bootloader will assume that the application has also been running at 240MHz. This will cause the chip to lock up later. Modify this to use a run time check of DPORT_CPUPERIOD_SEL, which indicates which of the PLL frequencies was used. Closes https://github.com/espressif/esp-idf/issues/2731. --- components/bootloader_support/src/bootloader_clock.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/components/bootloader_support/src/bootloader_clock.c b/components/bootloader_support/src/bootloader_clock.c index d5a1e52ab..50c84cfb0 100644 --- a/components/bootloader_support/src/bootloader_clock.c +++ b/components/bootloader_support/src/bootloader_clock.c @@ -31,14 +31,14 @@ void bootloader_clock_configure() /* Set CPU to 80MHz. Keep other clocks unmodified. */ int cpu_freq_mhz = 80; - /* On ESP32 rev 0, switching to 80MHz if clock was previously set to + /* On ESP32 rev 0, switching to 80/160 MHz if clock was previously set to * 240 MHz may cause the chip to lock up (see section 3.5 of the errata - * document). For rev. 0, switch to 240 instead if it was chosen in - * menuconfig. + * document). For rev. 0, switch to 240 instead if it has been enabled + * previously. */ uint32_t chip_ver_reg = REG_READ(EFUSE_BLK0_RDATA3_REG); if ((chip_ver_reg & EFUSE_RD_CHIP_VER_REV1_M) == 0 && - CONFIG_ESP32_DEFAULT_CPU_FREQ_MHZ == 240) { + DPORT_REG_GET_FIELD(DPORT_CPU_PER_CONF_REG, DPORT_CPUPERIOD_SEL) == 2) { cpu_freq_mhz = 240; } From 22dc4898d990195d526074d2c78351ef61b97b68 Mon Sep 17 00:00:00 2001 From: Ivan Grokhotkov Date: Tue, 26 Feb 2019 17:07:59 +0800 Subject: [PATCH 05/11] soc: define named constants for DPORT_CPUPERIOD_SEL values --- components/bootloader_support/src/bootloader_clock.c | 2 +- components/soc/esp32/include/soc/dport_reg.h | 3 +++ components/soc/esp32/rtc_clk.c | 12 ++++++------ 3 files changed, 10 insertions(+), 7 deletions(-) diff --git a/components/bootloader_support/src/bootloader_clock.c b/components/bootloader_support/src/bootloader_clock.c index 50c84cfb0..24267cc32 100644 --- a/components/bootloader_support/src/bootloader_clock.c +++ b/components/bootloader_support/src/bootloader_clock.c @@ -38,7 +38,7 @@ void bootloader_clock_configure() */ uint32_t chip_ver_reg = REG_READ(EFUSE_BLK0_RDATA3_REG); if ((chip_ver_reg & EFUSE_RD_CHIP_VER_REV1_M) == 0 && - DPORT_REG_GET_FIELD(DPORT_CPU_PER_CONF_REG, DPORT_CPUPERIOD_SEL) == 2) { + DPORT_REG_GET_FIELD(DPORT_CPU_PER_CONF_REG, DPORT_CPUPERIOD_SEL) == DPORT_CPUPERIOD_SEL_240) { cpu_freq_mhz = 240; } diff --git a/components/soc/esp32/include/soc/dport_reg.h b/components/soc/esp32/include/soc/dport_reg.h index 6c23dfe63..9cc3320b3 100644 --- a/components/soc/esp32/include/soc/dport_reg.h +++ b/components/soc/esp32/include/soc/dport_reg.h @@ -179,6 +179,9 @@ #define DPORT_CPUPERIOD_SEL_M ((DPORT_CPUPERIOD_SEL_V)<<(DPORT_CPUPERIOD_SEL_S)) #define DPORT_CPUPERIOD_SEL_V 0x3 #define DPORT_CPUPERIOD_SEL_S 0 +#define DPORT_CPUPERIOD_SEL_80 0 +#define DPORT_CPUPERIOD_SEL_160 1 +#define DPORT_CPUPERIOD_SEL_240 2 #define DPORT_PRO_CACHE_CTRL_REG (DR_REG_DPORT_BASE + 0x040) /* DPORT_PRO_DRAM_HL : R/W ;bitpos:[16] ;default: 1'b0 ; */ diff --git a/components/soc/esp32/rtc_clk.c b/components/soc/esp32/rtc_clk.c index 3a96c75e2..62a97866a 100644 --- a/components/soc/esp32/rtc_clk.c +++ b/components/soc/esp32/rtc_clk.c @@ -450,14 +450,14 @@ static void rtc_clk_bbpll_enable() static void rtc_clk_cpu_freq_to_pll_mhz(int cpu_freq_mhz) { int dbias = DIG_DBIAS_80M_160M; - int per_conf = 0; + int per_conf = DPORT_CPUPERIOD_SEL_80; if (cpu_freq_mhz == 80) { /* nothing to do */ } else if (cpu_freq_mhz == 160) { - per_conf = 1; + per_conf = DPORT_CPUPERIOD_SEL_160; } else if (cpu_freq_mhz == 240) { dbias = DIG_DBIAS_240M; - per_conf = 2; + per_conf = DPORT_CPUPERIOD_SEL_240; } else { SOC_LOGE(TAG, "invalid frequency"); abort(); @@ -685,15 +685,15 @@ void rtc_clk_cpu_freq_get_config(rtc_cpu_freq_config_t* out_config) case RTC_CNTL_SOC_CLK_SEL_PLL: { source = RTC_CPU_FREQ_SRC_PLL; uint32_t cpuperiod_sel = DPORT_REG_GET_FIELD(DPORT_CPU_PER_CONF_REG, DPORT_CPUPERIOD_SEL); - if (cpuperiod_sel == 0) { + if (cpuperiod_sel == DPORT_CPUPERIOD_SEL_80) { source_freq_mhz = RTC_PLL_FREQ_320M; div = 4; freq_mhz = 80; - } else if (cpuperiod_sel == 1) { + } else if (cpuperiod_sel == DPORT_CPUPERIOD_SEL_160) { source_freq_mhz = RTC_PLL_FREQ_320M; div = 2; freq_mhz = 160; - } else if (cpuperiod_sel == 2) { + } else if (cpuperiod_sel == DPORT_CPUPERIOD_SEL_240) { source_freq_mhz = RTC_PLL_FREQ_480M; div = 2; freq_mhz = 240; From 7e16a79cf74222a8a815afe2973ff20b96d324d0 Mon Sep 17 00:00:00 2001 From: Ivan Grokhotkov Date: Thu, 15 Nov 2018 20:03:13 +0800 Subject: [PATCH 06/11] esp_timer: improve unit test robustness 1. call esp_timer_get_time and ref_clock_get in the same order on start and in the loop 2. disable interrupts when calculating delta between ref_clock and esp_timer 3. ensure both functions are in cache before calculating the delta --- components/esp32/test/test_esp_timer.c | 29 +++++++++++++++++++++----- 1 file changed, 24 insertions(+), 5 deletions(-) diff --git a/components/esp32/test/test_esp_timer.c b/components/esp32/test/test_esp_timer.c index 3b6f3b40d..7d87d5018 100644 --- a/components/esp32/test/test_esp_timer.c +++ b/components/esp32/test/test_esp_timer.c @@ -413,22 +413,38 @@ TEST_CASE("esp_timer_get_time returns monotonic values", "[esp_timer]") int error_cnt; int64_t total_sq_error; int64_t max_error; + int64_t avg_diff; + int64_t dummy; } test_state_t; void timer_test_task(void* arg) { test_state_t* state = (test_state_t*) arg; state->pass = true; - int64_t start_time = ref_clock_get(); - int64_t delta = esp_timer_get_time() - start_time; + /* make sure both functions are in cache */ + state->dummy = esp_timer_get_time(); + state->dummy += ref_clock_get(); + + /* calculate the difference between the two clocks */ + portDISABLE_INTERRUPTS(); + int64_t hs_start_time = esp_timer_get_time(); + int64_t start_time = ref_clock_get(); + portENABLE_INTERRUPTS(); + int64_t delta = hs_start_time - start_time; + int64_t now = start_time; int error_repeat_cnt = 0; while (now - start_time < 10000000) { /* 10 seconds */ + /* Get values of both clocks again, and check that they are close to 'delta'. + * We don't disable interrupts here, because esp_timer_get_time doesn't lock + * interrupts internally, so we check if it can get "broken" by a well placed + * interrupt. + */ int64_t hs_now = esp_timer_get_time(); now = ref_clock_get(); int64_t diff = hs_now - (now + delta); /* Allow some difference due to rtos tick interrupting task between - * getting 'now' and 'ref_now'. + * getting 'hs_now' and 'now'. */ if (abs(diff) > 100) { error_repeat_cnt++; @@ -440,10 +456,12 @@ TEST_CASE("esp_timer_get_time returns monotonic values", "[esp_timer]") printf("diff=%lld\n", diff); state->pass = false; } + state->avg_diff += diff; state->max_error = MAX(state->max_error, abs(diff)); state->test_cnt++; state->total_sq_error += diff * diff; } + state->avg_diff /= state->test_cnt; xSemaphoreGive(state->done); vTaskDelete(NULL); } @@ -460,10 +478,11 @@ TEST_CASE("esp_timer_get_time returns monotonic values", "[esp_timer]") for (int i = 0; i < portNUM_PROCESSORS; ++i) { TEST_ASSERT_TRUE( xSemaphoreTake(done, portMAX_DELAY) ); - printf("CPU%d: %s test_cnt=%d error_cnt=%d std_error=%d |max_error|=%d\n", + printf("CPU%d: %s test_cnt=%d error_cnt=%d std_error=%d avg_diff=%d |max_error|=%d\n", i, states[i].pass ? "PASS" : "FAIL", states[i].test_cnt, states[i].error_cnt, - (int) sqrt(states[i].total_sq_error / states[i].test_cnt), (int) states[i].max_error); + (int) sqrt(states[i].total_sq_error / states[i].test_cnt), + (int) states[i].avg_diff, (int) states[i].max_error); } vSemaphoreDelete(done); From 742f8e7f8ac820a4f5c1f9b16a8a4c88873c588d Mon Sep 17 00:00:00 2001 From: Ivan Grokhotkov Date: Fri, 22 Feb 2019 21:27:43 +0800 Subject: [PATCH 07/11] esp_timer: fix occasional failures in "monotonic values" test 1. ref_clock used in unit tests occasionally produces time off by ~100 microseconds shortly after being started. Add a delay to let ref_clock stabilise, until the cause is found. 2. Reduce roundoff error accumulation which would occasionally cause the test to fail, by choosing an overflow value which can be divided by APB frequency. 3. Move time sampling part of the test into an IRAM function to reduce variations due to cache behavior. 4. Remove calculation of "standard deviation" in the test, as what was calculated was not actually standard deviation, and it did not add any useful information. --- components/esp32/esp_timer_esp32.c | 4 +-- components/esp32/test/test_esp_timer.c | 35 ++++++++++--------- .../components/unity/ref_clock.c | 2 ++ 3 files changed, 23 insertions(+), 18 deletions(-) diff --git a/components/esp32/esp_timer_esp32.c b/components/esp32/esp_timer_esp32.c index d42c8cfdc..75e3a58fa 100644 --- a/components/esp32/esp_timer_esp32.c +++ b/components/esp32/esp_timer_esp32.c @@ -97,7 +97,7 @@ static uint32_t s_alarm_overflow_val = DEFAULT_ALARM_OVERFLOW_VAL; static const char* TAG = "esp_timer_impl"; -// Interrupt handle retuned by the interrupt allocator +// Interrupt handle returned by the interrupt allocator static intr_handle_t s_timer_interrupt_handle; // Function from the upper layer to be called when the interrupt happens. @@ -123,7 +123,7 @@ static uint32_t s_timer_us_per_overflow; // value than the one which caused an interrupt. This can cause interrupt handler // to consider that the interrupt has happened due to timer overflow, incrementing // s_time_base_us. To avoid this, frequency switch hook sets this flag if -// it needs to set timer alarm value to ALARM_OVERFLOW_VAL. Interrupt hanler +// it needs to set timer alarm value to ALARM_OVERFLOW_VAL. Interrupt handler // will not increment s_time_base_us if this flag is set. static bool s_mask_overflow; diff --git a/components/esp32/test/test_esp_timer.c b/components/esp32/test/test_esp_timer.c index 7d87d5018..510bed461 100644 --- a/components/esp32/test/test_esp_timer.c +++ b/components/esp32/test/test_esp_timer.c @@ -24,7 +24,12 @@ static uint32_t s_old_overflow_val; static void setup_overflow() { s_old_overflow_val = esp_timer_impl_get_overflow_val(); - esp_timer_impl_set_overflow_val(0x7fffff); /* overflow every ~0.1 sec */} + /* Overflow every 0.1 sec. + * Chosen so that it is 0 modulo s_timer_ticks_per_us (which is 80), + * to prevent roundoff error on each overflow. + */ + esp_timer_impl_set_overflow_val(8000000); +} static void teardown_overflow() { @@ -404,6 +409,13 @@ TEST_CASE("esp_timer_get_time call takes less than 1us", "[esp_timer]") TEST_PERFORMANCE_LESS_THAN(ESP_TIMER_GET_TIME_PER_CALL, "%dns", ns_per_call); } +static int64_t IRAM_ATTR __attribute__((noinline)) get_clock_diff() +{ + uint64_t hs_time = esp_timer_get_time(); + uint64_t ref_time = ref_clock_get(); + return hs_time - ref_time; +} + TEST_CASE("esp_timer_get_time returns monotonic values", "[esp_timer]") { typedef struct { @@ -411,7 +423,6 @@ TEST_CASE("esp_timer_get_time returns monotonic values", "[esp_timer]") bool pass; int test_cnt; int error_cnt; - int64_t total_sq_error; int64_t max_error; int64_t avg_diff; int64_t dummy; @@ -422,27 +433,21 @@ TEST_CASE("esp_timer_get_time returns monotonic values", "[esp_timer]") state->pass = true; /* make sure both functions are in cache */ - state->dummy = esp_timer_get_time(); - state->dummy += ref_clock_get(); + state->dummy = get_clock_diff(); /* calculate the difference between the two clocks */ portDISABLE_INTERRUPTS(); - int64_t hs_start_time = esp_timer_get_time(); - int64_t start_time = ref_clock_get(); + int64_t delta = get_clock_diff(); portENABLE_INTERRUPTS(); - int64_t delta = hs_start_time - start_time; - - int64_t now = start_time; + int64_t start_time = ref_clock_get(); int error_repeat_cnt = 0; - while (now - start_time < 10000000) { /* 10 seconds */ + while (ref_clock_get() - start_time < 10000000) { /* 10 seconds */ /* Get values of both clocks again, and check that they are close to 'delta'. * We don't disable interrupts here, because esp_timer_get_time doesn't lock * interrupts internally, so we check if it can get "broken" by a well placed * interrupt. */ - int64_t hs_now = esp_timer_get_time(); - now = ref_clock_get(); - int64_t diff = hs_now - (now + delta); + int64_t diff = get_clock_diff() - delta; /* Allow some difference due to rtos tick interrupting task between * getting 'hs_now' and 'now'. */ @@ -459,7 +464,6 @@ TEST_CASE("esp_timer_get_time returns monotonic values", "[esp_timer]") state->avg_diff += diff; state->max_error = MAX(state->max_error, abs(diff)); state->test_cnt++; - state->total_sq_error += diff * diff; } state->avg_diff /= state->test_cnt; xSemaphoreGive(state->done); @@ -478,10 +482,9 @@ TEST_CASE("esp_timer_get_time returns monotonic values", "[esp_timer]") for (int i = 0; i < portNUM_PROCESSORS; ++i) { TEST_ASSERT_TRUE( xSemaphoreTake(done, portMAX_DELAY) ); - printf("CPU%d: %s test_cnt=%d error_cnt=%d std_error=%d avg_diff=%d |max_error|=%d\n", + printf("CPU%d: %s test_cnt=%d error_cnt=%d avg_diff=%d |max_error|=%d\n", i, states[i].pass ? "PASS" : "FAIL", states[i].test_cnt, states[i].error_cnt, - (int) sqrt(states[i].total_sq_error / states[i].test_cnt), (int) states[i].avg_diff, (int) states[i].max_error); } diff --git a/tools/unit-test-app/components/unity/ref_clock.c b/tools/unit-test-app/components/unity/ref_clock.c index 94afe01bd..52d4f1ca3 100644 --- a/tools/unit-test-app/components/unity/ref_clock.c +++ b/tools/unit-test-app/components/unity/ref_clock.c @@ -119,6 +119,8 @@ void ref_clock_init() PCNT.ctrl.val |= BIT(REF_CLOCK_PCNT_UNIT * 2); PCNT.ctrl.val &= ~BIT(REF_CLOCK_PCNT_UNIT * 2); + ets_delay_us(10000); + // Enable interrupt s_milliseconds = 0; ESP_ERROR_CHECK(esp_intr_alloc(ETS_PCNT_INTR_SOURCE, ESP_INTR_FLAG_IRAM, pcnt_isr, NULL, &s_intr_handle)); From 203f75223eb2f85ab1e23985c206997328d052f6 Mon Sep 17 00:00:00 2001 From: Ivan Grokhotkov Date: Fri, 22 Feb 2019 17:28:43 +0800 Subject: [PATCH 08/11] nvs: do eager cleanup of HashListBlocks Previously when HashList was removing items, HashListBlocks were removed lazily. This resulted in empty HashListBlocks dangling around in full pages, even when all items have been erased. These blocks would only be deleted when NVS was re-initialized (nvs_flash_deinit/nvs_flash_init). This change does eager cleanup instead, based on the code from @negativekelvin offered in https://github.com/espressif/esp-idf/issues/1642#issuecomment-367227994. Closes https://github.com/espressif/esp-idf/issues/1642. --- .../nvs_flash/src/nvs_item_hash_list.cpp | 13 +++++- .../nvs_flash/test_nvs_host/test_nvs.cpp | 41 +++++++++++++++++++ 2 files changed, 53 insertions(+), 1 deletion(-) diff --git a/components/nvs_flash/src/nvs_item_hash_list.cpp b/components/nvs_flash/src/nvs_item_hash_list.cpp index 845dd0910..a6cfdac1e 100644 --- a/components/nvs_flash/src/nvs_item_hash_list.cpp +++ b/components/nvs_flash/src/nvs_item_hash_list.cpp @@ -64,15 +64,22 @@ void HashList::erase(size_t index, bool itemShouldExist) { for (auto it = mBlockList.begin(); it != mBlockList.end();) { bool haveEntries = false; + bool foundIndex = false; for (size_t i = 0; i < it->mCount; ++i) { if (it->mNodes[i].mIndex == index) { it->mNodes[i].mIndex = 0xff; - return; + foundIndex = true; + /* found the item and removed it */ } if (it->mNodes[i].mIndex != 0xff) { haveEntries = true; } + if (haveEntries && foundIndex) { + /* item was found, and HashListBlock still has some items */ + return; + } } + /* no items left in HashListBlock, can remove */ if (!haveEntries) { auto tmp = it; ++it; @@ -81,6 +88,10 @@ void HashList::erase(size_t index, bool itemShouldExist) } else { ++it; } + if (foundIndex) { + /* item was found and empty HashListBlock was removed */ + return; + } } if (itemShouldExist) { assert(false && "item should have been present in cache"); diff --git a/components/nvs_flash/test_nvs_host/test_nvs.cpp b/components/nvs_flash/test_nvs_host/test_nvs.cpp index 05560353a..f98dd4ada 100644 --- a/components/nvs_flash/test_nvs_host/test_nvs.cpp +++ b/components/nvs_flash/test_nvs_host/test_nvs.cpp @@ -284,6 +284,47 @@ TEST_CASE("Page handles invalid CRC of variable length items", "[nvs][cur]") } } +class HashListTestHelper : public HashList +{ + public: + size_t getBlockCount() + { + return mBlockList.size(); + } +}; + +TEST_CASE("HashList is cleaned up as soon as items are erased", "[nvs]") +{ + HashListTestHelper hashlist; + // Add items + const size_t count = 128; + for (size_t i = 0; i < count; ++i) { + char key[16]; + snprintf(key, sizeof(key), "i%ld", (long int)i); + Item item(1, ItemType::U32, 1, key); + hashlist.insert(item, i); + } + INFO("Added " << count << " items, " << hashlist.getBlockCount() << " blocks"); + // Remove them in reverse order + for (size_t i = count; i > 0; --i) { + hashlist.erase(i - 1, true); + } + CHECK(hashlist.getBlockCount() == 0); + // Add again + for (size_t i = 0; i < count; ++i) { + char key[16]; + snprintf(key, sizeof(key), "i%ld", (long int)i); + Item item(1, ItemType::U32, 1, key); + hashlist.insert(item, i); + } + INFO("Added " << count << " items, " << hashlist.getBlockCount() << " blocks"); + // Remove them in the same order + for (size_t i = 0; i < count; ++i) { + hashlist.erase(i, true); + } + CHECK(hashlist.getBlockCount() == 0); +} + TEST_CASE("can init PageManager in empty flash", "[nvs]") { SpiFlashEmulator emu(4); From c0d32f6e486f8bb4ccfd57b9b6bd8dfbc8910fac Mon Sep 17 00:00:00 2001 From: Ivan Grokhotkov Date: Fri, 22 Feb 2019 18:14:48 +0800 Subject: [PATCH 09/11] nvs: add a blob fragmentation test case Ref. TW12937 --- .../nvs_flash/test_nvs_host/test_nvs.cpp | 22 +++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/components/nvs_flash/test_nvs_host/test_nvs.cpp b/components/nvs_flash/test_nvs_host/test_nvs.cpp index f98dd4ada..8f3819c53 100644 --- a/components/nvs_flash/test_nvs_host/test_nvs.cpp +++ b/components/nvs_flash/test_nvs_host/test_nvs.cpp @@ -1757,6 +1757,28 @@ TEST_CASE("Check that orphaned blobs are erased during init", "[nvs]") TEST_ESP_OK(storage.writeItem(1, ItemType::BLOB, "key3", blob, sizeof(blob))); } +TEST_CASE("nvs blob fragmentation test", "[nvs]") +{ + SpiFlashEmulator emu(4); + TEST_ESP_OK(nvs_flash_init_custom(NVS_DEFAULT_PART_NAME, 0, 4) ); + const size_t BLOB_SIZE = 3500; + uint8_t *blob = (uint8_t*) malloc(BLOB_SIZE); + CHECK(blob != NULL); + memset(blob, 0xEE, BLOB_SIZE); + const uint32_t magic = 0xff33eaeb; + nvs_handle h; + TEST_ESP_OK( nvs_open("blob_tests", NVS_READWRITE, &h) ); + for (int i = 0; i < 128; i++) { + INFO("Iteration " << i << "...\n"); + TEST_ESP_OK( nvs_set_u32(h, "magic", magic) ); + TEST_ESP_OK( nvs_set_blob(h, "blob", blob, BLOB_SIZE) ); + char seq_buf[16]; + sprintf(seq_buf, "seq%d", i); + TEST_ESP_OK( nvs_set_u32(h, seq_buf, i) ); + } + free(blob); +} + TEST_CASE("nvs code handles errors properly when partition is near to full", "[nvs]") { const size_t blob_size = Page::CHUNK_MAX_SIZE * 0.3 ; From 226c790766f85a356ec6182f3532666b73121ef6 Mon Sep 17 00:00:00 2001 From: Angus Gratton Date: Thu, 21 Feb 2019 10:48:55 +1100 Subject: [PATCH 10/11] ci: Retry submodule sync 2 more times before failing --- .gitlab-ci.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index 57c66024b..95411aaa1 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -731,6 +731,7 @@ check_submodule_sync: <<: *check_job_template variables: GIT_STRATEGY: clone + retry: 2 script: # check if all submodules are correctly synced to public repostory - git submodule update --init --recursive From 35e491856ee5626086c4aba4842675f4d7205e2c Mon Sep 17 00:00:00 2001 From: Angus Gratton Date: Mon, 25 Feb 2019 10:41:39 +1100 Subject: [PATCH 11/11] ci: Only use "github_sync" tagged runners to talk to GitHub --- .gitlab-ci.yml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index 95411aaa1..bb9b79d54 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -729,6 +729,8 @@ check_ut_cmake_make: check_submodule_sync: <<: *check_job_template + tags: + - github_sync variables: GIT_STRATEGY: clone retry: 2