From dd44195481659bb0c526dcc3cca626728a388c19 Mon Sep 17 00:00:00 2001 From: Tiago Medicci Date: Wed, 15 Jul 2026 14:02:27 -0300 Subject: [PATCH 1/2] fix(ana_cmpr): Fix swapped POS/NEG cross interrupt masks on ESP32-C5/P4/C61 In components/soc/esp32c5/register/soc/gpio_ext_struct.h (ESP32-C5), components/soc/esp32c61/register/soc/gpio_ext_struct.h (ESP32-C61), and components/soc/esp32p4/register/hw_ver3/soc/gpio_struct.h (ESP32-P4), the analog comparator raw/status/enable/clear register fields are named comp_neg_0_*/comp0_neg_* for bit 0 and comp_pos_0_*/comp0_pos_* for bit 1, but each field's own comment says the opposite: bit 0 is documented as "analog comparator pos edge interrupt raw/status/enable/clear" and bit 1 as the "neg" counterpart. The LL masks were defined from the field names rather than from this documented behavior, so ANALOG_CMPR_LL_POS_CROSS_INTR_MASK() ended up selecting bit 1 and ANALOG_CMPR_LL_NEG_CROSS_INTR_MASK() bit 0. A new test case, added in the following commit, arms only one cross direction at a time and checks that a matching transition fires the callback while the opposite, never-armed direction does not; without this fix it reproducibly fails on ESP32-C5, ESP32-P4, and ESP32-C61. Signed-off-by: Tiago Medicci --- .../esp_hal_ana_cmpr/esp32c5/include/hal/ana_cmpr_ll.h | 6 +++--- .../esp_hal_ana_cmpr/esp32c61/include/hal/ana_cmpr_ll.h | 6 +++--- .../esp_hal_ana_cmpr/esp32p4/include/hal/ana_cmpr_ll.h | 6 +++--- 3 files changed, 9 insertions(+), 9 deletions(-) diff --git a/components/esp_hal_ana_cmpr/esp32c5/include/hal/ana_cmpr_ll.h b/components/esp_hal_ana_cmpr/esp32c5/include/hal/ana_cmpr_ll.h index e98fe461c0d..7553db94eb3 100644 --- a/components/esp_hal_ana_cmpr/esp32c5/include/hal/ana_cmpr_ll.h +++ b/components/esp_hal_ana_cmpr/esp32c5/include/hal/ana_cmpr_ll.h @@ -1,5 +1,5 @@ /* - * SPDX-FileCopyrightText: 2024 Espressif Systems (Shanghai) CO LTD + * SPDX-FileCopyrightText: 2024-2026 Espressif Systems (Shanghai) CO LTD * * SPDX-License-Identifier: Apache-2.0 */ @@ -37,8 +37,8 @@ extern "C" { #define ANALOG_CMPR_LL_GET_HW(unit) (&ANALOG_CMPR[unit]) -#define ANALOG_CMPR_LL_NEG_CROSS_INTR_MASK(unit, src_chan) 0x01 -#define ANALOG_CMPR_LL_POS_CROSS_INTR_MASK(unit, src_chan) 0x02 +#define ANALOG_CMPR_LL_NEG_CROSS_INTR_MASK(unit, src_chan) 0x02 +#define ANALOG_CMPR_LL_POS_CROSS_INTR_MASK(unit, src_chan) 0x01 #define ANALOG_CMPR_LL_ANY_CROSS_INTR_MASK(unit, src_chan) (ANALOG_CMPR_LL_NEG_CROSS_INTR_MASK(unit, src_chan) | ANALOG_CMPR_LL_POS_CROSS_INTR_MASK(unit, src_chan)) #define ANALOG_CMPR_LL_ALL_INTR_MASK(unit) 0x07 diff --git a/components/esp_hal_ana_cmpr/esp32c61/include/hal/ana_cmpr_ll.h b/components/esp_hal_ana_cmpr/esp32c61/include/hal/ana_cmpr_ll.h index 8db001d82fb..b952bcd0890 100644 --- a/components/esp_hal_ana_cmpr/esp32c61/include/hal/ana_cmpr_ll.h +++ b/components/esp_hal_ana_cmpr/esp32c61/include/hal/ana_cmpr_ll.h @@ -1,5 +1,5 @@ /* - * SPDX-FileCopyrightText: 2024 Espressif Systems (Shanghai) CO LTD + * SPDX-FileCopyrightText: 2024-2026 Espressif Systems (Shanghai) CO LTD * * SPDX-License-Identifier: Apache-2.0 */ @@ -37,8 +37,8 @@ extern "C" { #define ANALOG_CMPR_LL_GET_HW(unit) (&ANALOG_CMPR[unit]) -#define ANALOG_CMPR_LL_NEG_CROSS_INTR_MASK(unit, src_chan) 0x01 -#define ANALOG_CMPR_LL_POS_CROSS_INTR_MASK(unit, src_chan) 0x02 +#define ANALOG_CMPR_LL_NEG_CROSS_INTR_MASK(unit, src_chan) 0x02 +#define ANALOG_CMPR_LL_POS_CROSS_INTR_MASK(unit, src_chan) 0x01 #define ANALOG_CMPR_LL_ANY_CROSS_INTR_MASK(unit, src_chan) (ANALOG_CMPR_LL_NEG_CROSS_INTR_MASK(unit, src_chan) | ANALOG_CMPR_LL_POS_CROSS_INTR_MASK(unit, src_chan)) #define ANALOG_CMPR_LL_ALL_INTR_MASK(unit) 0x07 diff --git a/components/esp_hal_ana_cmpr/esp32p4/include/hal/ana_cmpr_ll.h b/components/esp_hal_ana_cmpr/esp32p4/include/hal/ana_cmpr_ll.h index 5d8d2733c0f..138ba8ac0d8 100644 --- a/components/esp_hal_ana_cmpr/esp32p4/include/hal/ana_cmpr_ll.h +++ b/components/esp_hal_ana_cmpr/esp32p4/include/hal/ana_cmpr_ll.h @@ -1,5 +1,5 @@ /* - * SPDX-FileCopyrightText: 2023-2024 Espressif Systems (Shanghai) CO LTD + * SPDX-FileCopyrightText: 2023-2026 Espressif Systems (Shanghai) CO LTD * * SPDX-License-Identifier: Apache-2.0 */ @@ -38,8 +38,8 @@ extern "C" { #define ANALOG_CMPR_LL_GET_HW(unit) (&ANALOG_CMPR[unit]) #define ANALOG_CMPR_LL_GET_UNIT(hw) ((hw) == (&ANALOG_CMPR[0]) ? 0 : 1) -#define ANALOG_CMPR_LL_NEG_CROSS_INTR_MASK(unit, src_chan) (1UL << ((unit) * 3 + 0)) -#define ANALOG_CMPR_LL_POS_CROSS_INTR_MASK(unit, src_chan) (1UL << ((unit) * 3 + 1)) +#define ANALOG_CMPR_LL_NEG_CROSS_INTR_MASK(unit, src_chan) (1UL << ((unit) * 3 + 1)) +#define ANALOG_CMPR_LL_POS_CROSS_INTR_MASK(unit, src_chan) (1UL << ((unit) * 3 + 0)) #define ANALOG_CMPR_LL_ANY_CROSS_INTR_MASK(unit, src_chan) (ANALOG_CMPR_LL_NEG_CROSS_INTR_MASK(unit, src_chan) | ANALOG_CMPR_LL_POS_CROSS_INTR_MASK(unit, src_chan)) #define ANALOG_CMPR_LL_ALL_INTR_MASK(unit) (0x07 << ((unit) * 3)) From a8382517ca5bd8abc2a3cd7311507f732ad43153 Mon Sep 17 00:00:00 2001 From: Tiago Medicci Date: Wed, 15 Jul 2026 14:05:15 -0300 Subject: [PATCH 2/2] test(ana_cmpr): Add edge-specific interrupt direction test case Add a Unity test case that arms only ANA_CMPR_CROSS_POS (resp. only ANA_CMPR_CROSS_NEG) on a unit and asserts that a real transition of the matching direction fires the callback exactly once, while a transition of the opposite (never-armed) direction does not fire at all. This closes a gap in the existing test_apps: none of the current cases isolate cross direction, so a swapped POS/NEG interrupt mask in the LL layer (fixed in the previous commit) previously went undetected. On the scan-based comparator IP (ESP32-H4/S31), a crossing is only sampled/latched when a scan is explicitly triggered, so the new test case also triggers a scan after each level change on that IP, plus one extra priming scan right after enabling the unit so the internal compare state starts in sync with the already-set initial GPIO level. Signed-off-by: Tiago Medicci --- .../analog_comparator/main/test_ana_cmpr.cpp | 143 ++++++++++++++++++ .../analog_comparator/main/test_ana_cmpr.h | 22 ++- .../main/test_ana_cmpr_utils.cpp | 13 +- 3 files changed, 176 insertions(+), 2 deletions(-) diff --git a/components/esp_driver_ana_cmpr/test_apps/analog_comparator/main/test_ana_cmpr.cpp b/components/esp_driver_ana_cmpr/test_apps/analog_comparator/main/test_ana_cmpr.cpp index a285ebee404..78f4ab3e845 100644 --- a/components/esp_driver_ana_cmpr/test_apps/analog_comparator/main/test_ana_cmpr.cpp +++ b/components/esp_driver_ana_cmpr/test_apps/analog_comparator/main/test_ana_cmpr.cpp @@ -63,6 +63,149 @@ TEST_CASE("ana_cmpr unit install/uninstall", "[ana_cmpr]") TEST_ESP_OK(ana_cmpr_del_unit(cmpr)); } +TEST_CASE("ana_cmpr edge-specific interrupt reports correct cross direction", "[ana_cmpr]") +{ +#if !ANALOG_CMPR_LL_SUPPORT(EDGE_SPECIFIC_INTR_MASK) + TEST_IGNORE_MESSAGE("target cannot distinguish cross direction"); +#else + test_ana_cmpr_edge_cnt_t cnt = {}; + ana_cmpr_event_callbacks_t cbs = { + .on_cross = test_ana_cmpr_edge_cnt_callback, + }; + ana_cmpr_internal_ref_config_t ref_cfg = {}; + ref_cfg.ref_volt = ANA_CMPR_REF_VOLT_50_PCT_VDD; + ana_cmpr_debounce_config_t dbc_cfg = { + .wait_us = 10, + }; + + /* Arm only the rising (POS) interrupt: a real rising transition must be + * reported as POS, and a following falling transition must not fire at + * all, since NEG was never armed. */ + { + ana_cmpr_handle_t cmpr = NULL; + ana_cmpr_config_t config = {}; + config.unit = TEST_ANA_CMPR_UNIT_ID; + config.clk_src = ANA_CMPR_CLK_SRC_DEFAULT; + config.ref_src = ANA_CMPR_REF_SRC_INTERNAL; + config.cross_type = ANA_CMPR_CROSS_POS; +#if ANALOG_CMPR_LL_GET(IP_VERSION) > 1 + config.src_chan0_gpio = test_pad_gpio_num(ana_cmpr_periph[TEST_ANA_CMPR_UNIT_ID].pad_gpios[0]); + config.resample_limit = 3; +#else + config.src_chan0_gpio = GPIO_NUM_NC; + config.resample_limit = 0; +#endif + config.ext_ref_gpio = GPIO_NUM_NC; + TEST_ESP_OK(ana_cmpr_new_unit(&config, &cmpr)); + + gpio_num_t src_chan_io = test_init_src_chan_gpio(cmpr, 0, 0); + TEST_ESP_OK(ana_cmpr_set_internal_reference(cmpr, &ref_cfg)); + TEST_ESP_OK(ana_cmpr_set_debounce(cmpr, &dbc_cfg)); + TEST_ESP_OK(ana_cmpr_register_event_callbacks(cmpr, &cbs, &cnt)); +#if ANALOG_CMPR_LL_GET(IP_VERSION) > 1 + // this IP version only samples/latches a crossing when a scan is triggered + ana_cmpr_scan_config_t scan_cfg = { + .scan_mode = ANA_CMPR_SCAN_MODE_FULL, + .poll_period_us = 2, + }; + TEST_ESP_OK(ana_cmpr_set_scan_config(cmpr, &scan_cfg)); +#endif + TEST_ESP_OK(ana_cmpr_enable(cmpr)); + esp_rom_delay_us(1000); // allow the comparator analog block to settle after power-up +#if ANALOG_CMPR_LL_GET(IP_VERSION) > 1 + // prime the internal compare state so it reflects the already-set init level, + // otherwise the first real level change below may not be seen as a transition + TEST_ESP_OK(ana_cmpr_trigger_scan(cmpr)); + esp_rom_delay_us(1000); +#endif + + gpio_set_level(src_chan_io, 1); // rising: armed as POS, must fire as POS + esp_rom_delay_us(1000); +#if ANALOG_CMPR_LL_GET(IP_VERSION) > 1 + TEST_ESP_OK(ana_cmpr_trigger_scan(cmpr)); + esp_rom_delay_us(1000); +#endif + TEST_ASSERT_EQUAL_UINT32(1, cnt.pos_cnt); + TEST_ASSERT_EQUAL_UINT32(0, cnt.neg_cnt); + + gpio_set_level(src_chan_io, 0); // falling: NEG never armed, must not fire + esp_rom_delay_us(1000); +#if ANALOG_CMPR_LL_GET(IP_VERSION) > 1 + TEST_ESP_OK(ana_cmpr_trigger_scan(cmpr)); + esp_rom_delay_us(1000); +#endif + TEST_ASSERT_EQUAL_UINT32(1, cnt.pos_cnt); + TEST_ASSERT_EQUAL_UINT32(0, cnt.neg_cnt); + + TEST_ESP_OK(ana_cmpr_disable(cmpr)); + TEST_ESP_OK(ana_cmpr_del_unit(cmpr)); + } + + /* Same as above, mirrored: arm only the falling (NEG) interrupt. */ + { + cnt.pos_cnt = 0; + cnt.neg_cnt = 0; + ana_cmpr_handle_t cmpr = NULL; + ana_cmpr_config_t config = {}; + config.unit = TEST_ANA_CMPR_UNIT_ID; + config.clk_src = ANA_CMPR_CLK_SRC_DEFAULT; + config.ref_src = ANA_CMPR_REF_SRC_INTERNAL; + config.cross_type = ANA_CMPR_CROSS_NEG; +#if ANALOG_CMPR_LL_GET(IP_VERSION) > 1 + config.src_chan0_gpio = test_pad_gpio_num(ana_cmpr_periph[TEST_ANA_CMPR_UNIT_ID].pad_gpios[0]); + config.resample_limit = 3; +#else + config.src_chan0_gpio = GPIO_NUM_NC; + config.resample_limit = 0; +#endif + config.ext_ref_gpio = GPIO_NUM_NC; + TEST_ESP_OK(ana_cmpr_new_unit(&config, &cmpr)); + + gpio_num_t src_chan_io = test_init_src_chan_gpio(cmpr, 0, 1); + TEST_ESP_OK(ana_cmpr_set_internal_reference(cmpr, &ref_cfg)); + TEST_ESP_OK(ana_cmpr_set_debounce(cmpr, &dbc_cfg)); + TEST_ESP_OK(ana_cmpr_register_event_callbacks(cmpr, &cbs, &cnt)); +#if ANALOG_CMPR_LL_GET(IP_VERSION) > 1 + // this IP version only samples/latches a crossing when a scan is triggered + ana_cmpr_scan_config_t scan_cfg = { + .scan_mode = ANA_CMPR_SCAN_MODE_FULL, + .poll_period_us = 2, + }; + TEST_ESP_OK(ana_cmpr_set_scan_config(cmpr, &scan_cfg)); +#endif + TEST_ESP_OK(ana_cmpr_enable(cmpr)); + esp_rom_delay_us(1000); // allow the comparator analog block to settle after power-up +#if ANALOG_CMPR_LL_GET(IP_VERSION) > 1 + // prime the internal compare state so it reflects the already-set init level, + // otherwise the first real level change below may not be seen as a transition + TEST_ESP_OK(ana_cmpr_trigger_scan(cmpr)); + esp_rom_delay_us(1000); +#endif + + gpio_set_level(src_chan_io, 0); // falling: armed as NEG, must fire as NEG + esp_rom_delay_us(1000); +#if ANALOG_CMPR_LL_GET(IP_VERSION) > 1 + TEST_ESP_OK(ana_cmpr_trigger_scan(cmpr)); + esp_rom_delay_us(1000); +#endif + TEST_ASSERT_EQUAL_UINT32(0, cnt.pos_cnt); + TEST_ASSERT_EQUAL_UINT32(1, cnt.neg_cnt); + + gpio_set_level(src_chan_io, 1); // rising: POS never armed, must not fire + esp_rom_delay_us(1000); +#if ANALOG_CMPR_LL_GET(IP_VERSION) > 1 + TEST_ESP_OK(ana_cmpr_trigger_scan(cmpr)); + esp_rom_delay_us(1000); +#endif + TEST_ASSERT_EQUAL_UINT32(0, cnt.pos_cnt); + TEST_ASSERT_EQUAL_UINT32(1, cnt.neg_cnt); + + TEST_ESP_OK(ana_cmpr_disable(cmpr)); + TEST_ESP_OK(ana_cmpr_del_unit(cmpr)); + } +#endif +} + TEST_CASE("ana_cmpr event callback", "[ana_cmpr]") { uint32_t cnt = 0; diff --git a/components/esp_driver_ana_cmpr/test_apps/analog_comparator/main/test_ana_cmpr.h b/components/esp_driver_ana_cmpr/test_apps/analog_comparator/main/test_ana_cmpr.h index 41c6f185716..12aaa00f68e 100644 --- a/components/esp_driver_ana_cmpr/test_apps/analog_comparator/main/test_ana_cmpr.h +++ b/components/esp_driver_ana_cmpr/test_apps/analog_comparator/main/test_ana_cmpr.h @@ -1,5 +1,5 @@ /* - * SPDX-FileCopyrightText: 2023-2025 Espressif Systems (Shanghai) CO LTD + * SPDX-FileCopyrightText: 2023-2026 Espressif Systems (Shanghai) CO LTD * * SPDX-License-Identifier: Apache-2.0 */ @@ -42,6 +42,26 @@ extern "C" { */ bool test_ana_cmpr_on_cross_callback(ana_cmpr_handle_t cmpr, const ana_cmpr_cross_event_data_t *edata, void *user_ctx); +/** + * @brief Test context to count how many POS/NEG cross events were reported + */ +typedef struct { + uint32_t pos_cnt; + uint32_t neg_cnt; +} test_ana_cmpr_edge_cnt_t; + +/** + * @brief Test on cross callback that tallies observed cross direction + * + * @param cmpr Analog Comparator handle + * @param edata Event data + * @param user_ctx User context, need to input a `test_ana_cmpr_edge_cnt_t *` + * @return + * - true Need to yield + * - false Don't need yield + */ +bool test_ana_cmpr_edge_cnt_callback(ana_cmpr_handle_t cmpr, const ana_cmpr_cross_event_data_t *edata, void *user_ctx); + /** * @brief Initialize Analog Comparator source channel GPIO * diff --git a/components/esp_driver_ana_cmpr/test_apps/analog_comparator/main/test_ana_cmpr_utils.cpp b/components/esp_driver_ana_cmpr/test_apps/analog_comparator/main/test_ana_cmpr_utils.cpp index d11727bae3c..54366999d1c 100644 --- a/components/esp_driver_ana_cmpr/test_apps/analog_comparator/main/test_ana_cmpr_utils.cpp +++ b/components/esp_driver_ana_cmpr/test_apps/analog_comparator/main/test_ana_cmpr_utils.cpp @@ -1,5 +1,5 @@ /* - * SPDX-FileCopyrightText: 2023-2025 Espressif Systems (Shanghai) CO LTD + * SPDX-FileCopyrightText: 2023-2026 Espressif Systems (Shanghai) CO LTD * * SPDX-License-Identifier: Apache-2.0 */ @@ -15,6 +15,17 @@ bool IRAM_ATTR test_ana_cmpr_on_cross_callback(ana_cmpr_handle_t cmpr, const ana return false; } +bool IRAM_ATTR test_ana_cmpr_edge_cnt_callback(ana_cmpr_handle_t cmpr, const ana_cmpr_cross_event_data_t *edata, void *user_ctx) +{ + test_ana_cmpr_edge_cnt_t *cnt = (test_ana_cmpr_edge_cnt_t *)user_ctx; + if (edata->cross_type == ANA_CMPR_CROSS_POS) { + cnt->pos_cnt++; + } else if (edata->cross_type == ANA_CMPR_CROSS_NEG) { + cnt->neg_cnt++; + } + return false; +} + gpio_num_t test_init_src_chan_gpio(ana_cmpr_handle_t cmpr, int src_chan_id, int init_level) { gpio_num_t src_chan_num = GPIO_NUM_NC;