From 3567b1d0ce110aa17aec897fc4cbbbeda749b397 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 c8f8ba0141..19960a6656 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 1e1450bf5dd0bbdeb74d084f5af79cffc9259fa3 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 | 43 +++++++++++++++++++++++++++-------- tools/ci/test_build_system.sh | 27 ++++++++++++++++++++++ 2 files changed, 61 insertions(+), 9 deletions(-) diff --git a/make/project.mk b/make/project.mk index 10556b191f..e19ba38bc7 100644 --- a/make/project.mk +++ b/make/project.mk @@ -131,6 +131,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 @@ -138,33 +141,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 ?= @@ -260,7 +285,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) diff --git a/tools/ci/test_build_system.sh b/tools/ci/test_build_system.sh index efc3703b18..136045987e 100755 --- a/tools/ci/test_build_system.sh +++ b/tools/ci/test_build_system.sh @@ -237,7 +237,34 @@ function run_tests() make defconfig || failure "Failed to reconfigure with smaller flash" ( 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 + touch sdkconfig # to update mtime + 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 echo "Some failures were detected:" From 450118fdf806a6411f9454424915f662fd3f6e88 Mon Sep 17 00:00:00 2001 From: Ivan Grokhotkov Date: Wed, 27 Feb 2019 20:29:25 +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 07fc1c5d60..5fe05464c4 100644 --- a/components/soc/esp32/rtc_clk.c +++ b/components/soc/esp32/rtc_clk.c @@ -400,7 +400,6 @@ static void rtc_clk_cpu_freq_to_xtal() 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_XTL); - DPORT_REG_WRITE(DPORT_CPU_PER_CONF_REG, 0); // clear DPORT_CPUPERIOD_SEL rtc_clk_apb_freq_update(xtal_freq * MHZ); s_cur_freq = RTC_CPU_FREQ_XTAL; @@ -472,7 +471,6 @@ void rtc_clk_cpu_freq_set(rtc_cpu_freq_t cpu_freq) */ rtc_clk_wait_for_slow_cycle(); - DPORT_REG_SET_FIELD(DPORT_CPU_PER_CONF_REG, DPORT_CPUPERIOD_SEL, 0); SET_PERI_REG_MASK(RTC_CNTL_OPTIONS0_REG, RTC_CNTL_BB_I2C_FORCE_PD | RTC_CNTL_BBPLL_FORCE_PD | RTC_CNTL_BBPLL_I2C_FORCE_PD); From beb8347faa45e3189fedda30b399e95e5b145878 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 64ef9c2333..0ba723f897 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. */ rtc_cpu_freq_t cpu_freq = RTC_CPU_FREQ_80M; - /* 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 = RTC_CPU_FREQ_240M; } From 48416c38c8ce011b14b95f727190ba62e02599c2 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 --- .../bootloader_support/src/bootloader_clock.c | 2 +- components/soc/esp32/include/soc/dport_reg.h | 3 +++ components/soc/esp32/rtc_clk.c | 18 +++++++++--------- 3 files changed, 13 insertions(+), 10 deletions(-) diff --git a/components/bootloader_support/src/bootloader_clock.c b/components/bootloader_support/src/bootloader_clock.c index 0ba723f897..e5e3a6c559 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 = RTC_CPU_FREQ_240M; } diff --git a/components/soc/esp32/include/soc/dport_reg.h b/components/soc/esp32/include/soc/dport_reg.h index 6c23dfe639..9cc3320b39 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 5fe05464c4..329ea169c4 100644 --- a/components/soc/esp32/rtc_clk.c +++ b/components/soc/esp32/rtc_clk.c @@ -425,15 +425,15 @@ static void rtc_clk_cpu_freq_to_pll(rtc_cpu_freq_t cpu_freq) if (cpu_freq == RTC_CPU_FREQ_80M) { REG_SET_FIELD(RTC_CNTL_REG, RTC_CNTL_DIG_DBIAS_WAK, DIG_DBIAS_80M_160M); - DPORT_REG_WRITE(DPORT_CPU_PER_CONF_REG, 0); + DPORT_REG_WRITE(DPORT_CPU_PER_CONF_REG, DPORT_CPUPERIOD_SEL_80); freq = 80; } else if (cpu_freq == RTC_CPU_FREQ_160M) { REG_SET_FIELD(RTC_CNTL_REG, RTC_CNTL_DIG_DBIAS_WAK, DIG_DBIAS_80M_160M); - DPORT_REG_WRITE(DPORT_CPU_PER_CONF_REG, 1); + DPORT_REG_WRITE(DPORT_CPU_PER_CONF_REG, DPORT_CPUPERIOD_SEL_160); freq = 160; } else if (cpu_freq == RTC_CPU_FREQ_240M) { REG_SET_FIELD(RTC_CNTL_REG, RTC_CNTL_DIG_DBIAS_WAK, DIG_DBIAS_240M); - DPORT_REG_WRITE(DPORT_CPU_PER_CONF_REG, 2); + DPORT_REG_WRITE(DPORT_CPU_PER_CONF_REG, DPORT_CPUPERIOD_SEL_240); freq = 240; } REG_SET_FIELD(RTC_CNTL_CLK_CONF_REG, RTC_CNTL_SOC_CLK_SEL, RTC_CNTL_SOC_CLK_SEL_PLL); @@ -500,15 +500,15 @@ void rtc_clk_cpu_freq_set(rtc_cpu_freq_t cpu_freq) RTC_CNTL_BBPLL_FORCE_PD | RTC_CNTL_BBPLL_I2C_FORCE_PD); rtc_clk_bbpll_set(xtal_freq, cpu_freq); if (cpu_freq == RTC_CPU_FREQ_80M) { - DPORT_REG_SET_FIELD(DPORT_CPU_PER_CONF_REG, DPORT_CPUPERIOD_SEL, 0); + DPORT_REG_SET_FIELD(DPORT_CPU_PER_CONF_REG, DPORT_CPUPERIOD_SEL, DPORT_CPUPERIOD_SEL_80); ets_update_cpu_frequency(80); s_cur_pll = RTC_PLL_320M; } else if (cpu_freq == RTC_CPU_FREQ_160M) { - DPORT_REG_SET_FIELD(DPORT_CPU_PER_CONF_REG, DPORT_CPUPERIOD_SEL, 1); + DPORT_REG_SET_FIELD(DPORT_CPU_PER_CONF_REG, DPORT_CPUPERIOD_SEL, DPORT_CPUPERIOD_SEL_160); ets_update_cpu_frequency(160); s_cur_pll = RTC_PLL_320M; } else if (cpu_freq == RTC_CPU_FREQ_240M) { - DPORT_REG_SET_FIELD(DPORT_CPU_PER_CONF_REG, DPORT_CPUPERIOD_SEL, 2); + DPORT_REG_SET_FIELD(DPORT_CPU_PER_CONF_REG, DPORT_CPUPERIOD_SEL, DPORT_CPUPERIOD_SEL_240); ets_update_cpu_frequency(240); s_cur_pll = RTC_PLL_480M; } @@ -536,11 +536,11 @@ rtc_cpu_freq_t rtc_clk_cpu_freq_get() } case RTC_CNTL_SOC_CLK_SEL_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) { return RTC_CPU_FREQ_80M; - } else if (cpuperiod_sel == 1) { + } else if (cpuperiod_sel == DPORT_CPUPERIOD_SEL_160) { return RTC_CPU_FREQ_160M; - } else if (cpuperiod_sel == 2) { + } else if (cpuperiod_sel == DPORT_CPUPERIOD_SEL_240) { return RTC_CPU_FREQ_240M; } else { assert(false && "unsupported frequency"); From 9cfd94a9d6d8d79b4baa27d56f0d3cc28c9f3ce5 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 3b6f3b40d6..7d87d50182 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 70a9e72e06ea493c221e8c2bd058374362841fc8 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 3d63cf2bd8..b6315ca760 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 7d87d50182..510bed4618 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 94afe01bd7..52d4f1ca3e 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 7ee102a9f658898ac07c4b89923aa7ab2d1b949e 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 845dd09106..a6cfdac1e2 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 272205d6fd..632d2ef013 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 4e55490602d319a7d5075ab746b643a61fe5d587 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 632d2ef013..cc5c2f5228 100644 --- a/components/nvs_flash/test_nvs_host/test_nvs.cpp +++ b/components/nvs_flash/test_nvs_host/test_nvs.cpp @@ -1807,6 +1807,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 04ed861eb8887ff85fbba08d9e6e0eb9b918a373 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 0071874503..3cd0f49217 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -596,6 +596,7 @@ check_submodule_sync: variables: GIT_STRATEGY: clone before_script: *do_nothing_before + retry: 2 script: # check if all submodules are correctly synced to public repostory - git submodule update --init --recursive From fb4b035105e51fd7a3b33d3219fb407acd0c202b 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 3cd0f49217..65cac82296 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -593,6 +593,8 @@ check_submodule_sync: - /^release\/v/ - /^v\d+\.\d+(\.\d+)?($|-)/ dependencies: [] + tags: + - github_sync variables: GIT_STRATEGY: clone before_script: *do_nothing_before