From c3df56b53e90ec37b23b9601848d1bece59fd57a Mon Sep 17 00:00:00 2001 From: wanckl Date: Fri, 25 Oct 2024 15:06:46 +0800 Subject: [PATCH] fix(esp_adc): fixed adc continue monitor don't work issue Closes https://github.com/espressif/esp-idf/issues/14769 Closes https://github.com/espressif/esp-idf/issues/14814 --- components/esp_adc/adc_continuous.c | 16 +++++ components/esp_adc/adc_monitor.c | 43 ++++------- .../esp_adc/include/esp_adc/adc_monitor.h | 3 +- .../esp_adc/test_apps/adc/main/test_adc.c | 72 ++++++++----------- components/hal/adc_hal.c | 11 +++ components/hal/include/hal/adc_hal.h | 13 +++- 6 files changed, 82 insertions(+), 76 deletions(-) diff --git a/components/esp_adc/adc_continuous.c b/components/esp_adc/adc_continuous.c index 747ecb15ef..f85aa702b0 100644 --- a/components/esp_adc/adc_continuous.c +++ b/components/esp_adc/adc_continuous.c @@ -381,6 +381,22 @@ esp_err_t adc_continuous_start(adc_continuous_handle_t handle) } #endif //#if SOC_ADC_ARBITER_SUPPORTED +#if SOC_ADC_MONITOR_SUPPORTED + adc_ll_digi_monitor_clear_intr(); + for (int i = 0; i < SOC_ADC_DIGI_MONITOR_NUM; i++) { + adc_monitor_t *monitor_ctx = handle->adc_monitor[i]; + if (monitor_ctx) { + // config monitor hardware + adc_hal_digi_monitor_set_thres(monitor_ctx->monitor_id, monitor_ctx->config.adc_unit, monitor_ctx->config.channel, monitor_ctx->config.h_threshold, monitor_ctx->config.l_threshold); + // if monitor not enabled now, just using monitor api later + if (monitor_ctx->fsm == ADC_MONITOR_FSM_ENABLED) { + // restore the started FSM + adc_ll_digi_monitor_user_start(monitor_ctx->monitor_id, ((monitor_ctx->config.h_threshold >= 0) || (monitor_ctx->config.l_threshold >= 0))); + } + } + } +#endif //#if SOC_ADC_MONITOR_SUPPORTED + if (handle->use_adc1) { adc_hal_set_controller(ADC_UNIT_1, ADC_HAL_CONTINUOUS_READ_MODE); } diff --git a/components/esp_adc/adc_monitor.c b/components/esp_adc/adc_monitor.c index 8bdeeddd85..025f13e979 100644 --- a/components/esp_adc/adc_monitor.c +++ b/components/esp_adc/adc_monitor.c @@ -1,5 +1,5 @@ /* - * SPDX-FileCopyrightText: 2023 Espressif Systems (Shanghai) CO LTD + * SPDX-FileCopyrightText: 2023-2024 Espressif Systems (Shanghai) CO LTD * * SPDX-License-Identifier: Apache-2.0 */ @@ -173,10 +173,6 @@ esp_err_t adc_new_continuous_monitor(adc_continuous_handle_t handle, const adc_m ESP_GOTO_ON_ERROR(adc_monitor_intr_alloc(), intr_err, MNTOR_TAG, "esp intr alloc failed"); } - // config hardware - adc_ll_digi_monitor_clear_intr(); - adc_ll_digi_monitor_set_thres(monitor_ctx->monitor_id, monitor_ctx->config.adc_unit, monitor_ctx->config.channel, monitor_ctx->config.h_threshold, monitor_ctx->config.l_threshold); - *ret_handle = monitor_ctx; return ESP_OK; @@ -191,7 +187,6 @@ esp_err_t adc_continuous_monitor_register_event_callbacks(adc_monitor_handle_t m { ESP_RETURN_ON_FALSE(monitor_handle && cbs, ESP_ERR_INVALID_ARG, MNTOR_TAG, "invalid argument: null pointer"); ESP_RETURN_ON_FALSE(monitor_handle->fsm == ADC_MONITOR_FSM_INIT, ESP_ERR_INVALID_STATE, MNTOR_TAG, "monitor should be in init state"); - ESP_RETURN_ON_FALSE(!(monitor_handle->cbs.on_over_high_thresh || monitor_handle->cbs.on_below_low_thresh), ESP_ERR_INVALID_STATE, MNTOR_TAG, "callbacks had beed registered"); #if CONFIG_IDF_TARGET_ESP32S2 ESP_RETURN_ON_FALSE(!(cbs->on_below_low_thresh && cbs->on_over_high_thresh), ESP_ERR_NOT_SUPPORTED, MNTOR_TAG, "ESP32S2 support only one threshold"); #endif @@ -216,45 +211,31 @@ esp_err_t adc_continuous_monitor_register_event_callbacks(adc_monitor_handle_t m esp_err_t adc_continuous_monitor_enable(adc_monitor_handle_t monitor_handle) { - ESP_RETURN_ON_FALSE(monitor_handle, ESP_ERR_INVALID_ARG, MNTOR_TAG, "invalid argument: null pointer"); - ESP_RETURN_ON_FALSE(monitor_handle->fsm == ADC_MONITOR_FSM_INIT, ESP_ERR_INVALID_STATE, MNTOR_TAG, "monitor should be in init state"); + ESP_RETURN_ON_FALSE_ISR(monitor_handle, ESP_ERR_INVALID_ARG, MNTOR_TAG, "invalid argument: null pointer"); + ESP_RETURN_ON_FALSE_ISR(monitor_handle->fsm == ADC_MONITOR_FSM_INIT, ESP_ERR_INVALID_STATE, MNTOR_TAG, "monitor should be in init state"); - // enable peripheral intr_ena - if ((monitor_handle->config.h_threshold >= 0)) { - adc_ll_digi_monitor_enable_intr(monitor_handle->monitor_id, ADC_MONITOR_MODE_HIGH, true); - } - if ((monitor_handle->config.l_threshold >= 0)) { - adc_ll_digi_monitor_enable_intr(monitor_handle->monitor_id, ADC_MONITOR_MODE_LOW, true); - } - - adc_ll_digi_monitor_user_start(monitor_handle->monitor_id, true); + // start monitor + adc_ll_digi_monitor_user_start(monitor_handle->monitor_id, ((monitor_handle->config.h_threshold >= 0) || (monitor_handle->config.l_threshold >= 0))); monitor_handle->fsm = ADC_MONITOR_FSM_ENABLED; - return esp_intr_enable(s_adc_monitor_platform.monitor_intr_handle); + return ESP_OK; } esp_err_t adc_continuous_monitor_disable(adc_monitor_handle_t monitor_handle) { - ESP_RETURN_ON_FALSE(monitor_handle, ESP_ERR_INVALID_ARG, MNTOR_TAG, "invalid argument: null pointer"); - ESP_RETURN_ON_FALSE(monitor_handle->fsm == ADC_MONITOR_FSM_ENABLED, ESP_ERR_INVALID_STATE, MNTOR_TAG, "monitor not in running"); - - // disable peripheral intr_ena - if ((monitor_handle->config.h_threshold >= 0)) { - adc_ll_digi_monitor_enable_intr(monitor_handle->monitor_id, ADC_MONITOR_MODE_HIGH, false); - } - if ((monitor_handle->config.l_threshold >= 0)) { - adc_ll_digi_monitor_enable_intr(monitor_handle->monitor_id, ADC_MONITOR_MODE_LOW, false); - } + ESP_RETURN_ON_FALSE_ISR(monitor_handle, ESP_ERR_INVALID_ARG, MNTOR_TAG, "invalid argument: null pointer"); + ESP_RETURN_ON_FALSE_ISR(monitor_handle->fsm == ADC_MONITOR_FSM_ENABLED, ESP_ERR_INVALID_STATE, MNTOR_TAG, "monitor not in running"); + // stop monitor adc_ll_digi_monitor_user_start(monitor_handle->monitor_id, false); monitor_handle->fsm = ADC_MONITOR_FSM_INIT; - return esp_intr_disable(s_adc_monitor_platform.monitor_intr_handle); + return ESP_OK; } esp_err_t adc_del_continuous_monitor(adc_monitor_handle_t monitor_handle) { ESP_RETURN_ON_FALSE(monitor_handle, ESP_ERR_INVALID_ARG, MNTOR_TAG, "invalid argument: null pointer"); ESP_RETURN_ON_FALSE((monitor_handle->fsm == ADC_MONITOR_FSM_INIT) && (s_adc_monitor_platform.continuous_ctx->fsm == ADC_FSM_INIT), \ - ESP_ERR_INVALID_STATE, MNTOR_TAG, "monitor and ADC continuous driver should all be in init state"); + ESP_ERR_INVALID_STATE, MNTOR_TAG, "monitor and ADC continuous driver should all stopped"); ESP_RETURN_ON_ERROR(s_adc_monitor_release(monitor_handle), MNTOR_TAG, "monitor not find or isn't in use"); for (int i = 0; i < SOC_ADC_DIGI_MONITOR_NUM; i++) { @@ -265,7 +246,7 @@ esp_err_t adc_del_continuous_monitor(adc_monitor_handle_t monitor_handle) } } - // If no monitor is using, the release intr handle as well + // If no monitor is using, then release intr handle as well ESP_RETURN_ON_ERROR(esp_intr_free(s_adc_monitor_platform.monitor_intr_handle), MNTOR_TAG, "esp intr release failed\n"); s_adc_monitor_platform.monitor_intr_handle = NULL; free(monitor_handle); diff --git a/components/esp_adc/include/esp_adc/adc_monitor.h b/components/esp_adc/include/esp_adc/adc_monitor.h index 8066771a2e..946967e271 100644 --- a/components/esp_adc/include/esp_adc/adc_monitor.h +++ b/components/esp_adc/include/esp_adc/adc_monitor.h @@ -65,7 +65,8 @@ typedef struct { esp_err_t adc_new_continuous_monitor(adc_continuous_handle_t handle, const adc_monitor_config_t *monitor_cfg, adc_monitor_handle_t *ret_handle); /** - * @brief Register threshold interrupt callbacks for allocated monitor. + * @brief Register/Unregister threshold interrupt callbacks for allocated monitor. + * Passing `cbs` contain the NULL `over_high/below_low` will unregister relative callbacks. * * @param[in] monitor_handle Monitor handle * @param[in] cbs Pointer to a adc_monitor_evt_cbs_t struct diff --git a/components/esp_adc/test_apps/adc/main/test_adc.c b/components/esp_adc/test_apps/adc/main/test_adc.c index d1d2789e4c..f3cb8cb74c 100644 --- a/components/esp_adc/test_apps/adc/main/test_adc.c +++ b/components/esp_adc/test_apps/adc/main/test_adc.c @@ -382,9 +382,6 @@ TEST_CASE("ADC continuous monitor init_deinit", "[adc]") TEST_ESP_OK(adc_del_continuous_monitor(monitor_handle_2)); TEST_ESP_ERR(ESP_ERR_INVALID_ARG, adc_del_continuous_monitor(monitor_handle_3)); - //try register cbs again - TEST_ESP_ERR(ESP_ERR_INVALID_STATE, adc_continuous_monitor_register_event_callbacks(monitor_handle, &monitor_cb, &monitor_cb)); - //try delete it when adc is running but monitor not running TEST_ESP_OK(adc_continuous_start(handle)); TEST_ESP_ERR(ESP_ERR_INVALID_STATE, adc_del_continuous_monitor(monitor_handle)); @@ -400,23 +397,7 @@ TEST_CASE("ADC continuous monitor init_deinit", "[adc]") TEST_ESP_OK(adc_continuous_deinit(handle)); } -/** - * NOTE: To run this special feature test case, you need wire ADC channel pin you want to monit - * to a wave output pin defined below. - * - * +---------+ - * | | - * | (adc)|------------+ - * | | | - * | (wave)|------------+ - * | | - * | ESP32 | - * +---------+ - * - * or you can connect your signals from signal generator to ESP32 pin which you monitoring - **/ -#define TEST_ADC_CHANNEL ADC_CHANNEL_0 //GPIO_1 -#define TEST_WAVE_OUT_PIN GPIO_NUM_2 //GPIO_2 +#define TEST_ADC_CHANNEL ADC_CHANNEL_0 static uint32_t m1h_cnt, m1l_cnt; bool IRAM_ATTR m1h_cb(adc_monitor_handle_t monitor_handle, const adc_monitor_evt_data_t *event_data, void *user_data) @@ -429,7 +410,7 @@ bool IRAM_ATTR m1l_cb(adc_monitor_handle_t monitor_handle, const adc_monitor_evt m1l_cnt ++; return false; } -TEST_CASE("ADC continuous monitor functionary", "[adc][manual][ignore]") +TEST_CASE("ADC continuous monitor functionary", "[adc]") { adc_continuous_handle_t handle = NULL; adc_continuous_handle_cfg_t adc_config = { @@ -474,35 +455,40 @@ TEST_CASE("ADC continuous monitor functionary", "[adc][manual][ignore]") }; TEST_ESP_OK(adc_new_continuous_monitor(handle, &adc_monitor_cfg, &monitor_handle)); TEST_ESP_OK(adc_continuous_monitor_register_event_callbacks(monitor_handle, &monitor_cb, NULL)); - - //config a pin to generate wave - gpio_config_t gpio_cfg = { - .pin_bit_mask = (1ULL << TEST_WAVE_OUT_PIN), - .mode = GPIO_MODE_INPUT_OUTPUT, - .pull_up_en = GPIO_PULLDOWN_ENABLE, - }; - TEST_ESP_OK(gpio_config(&gpio_cfg)); - TEST_ESP_OK(adc_continuous_monitor_enable(monitor_handle)); TEST_ESP_OK(adc_continuous_start(handle)); - for (uint8_t i = 0; i < 8; i++) { - vTaskDelay(1000); + int adc_io; + adc_continuous_channel_to_io(ADC_UNIT_1, TEST_ADC_CHANNEL, &adc_io); + printf("Using ADC_CHANNEL_%d on GPIO%d\n", TEST_ADC_CHANNEL, adc_io); - // check monitor cb - printf("%d\t high_cnt %4ld\tlow_cnt %4ld\n", i, m1h_cnt, m1l_cnt); - if (gpio_get_level(TEST_WAVE_OUT_PIN)) { + // Test with internal gpio pull up/down, detail and order refer to `gpio_pull_mode_t` + // pull_up + pull_down to get half ADC convert + for (uint8_t i = 0; i < 8; i++) { + int pull_mode = i % GPIO_FLOATING; + gpio_set_pull_mode(adc_io, pull_mode); + vTaskDelay(10); // wait some time for GPIO level be stable + m1h_cnt = 0; + m1l_cnt = 0; + vTaskDelay(1000); //time to count monitor interrupt + printf("%d\t %s\t high_cnt %4ld\tlow_cnt %4ld\n", i, (pull_mode == 0) ? "up " : (pull_mode == 1) ? "down" : "mid ", m1h_cnt, m1l_cnt); + + switch (pull_mode) { + case GPIO_PULLUP_ONLY: #if !CONFIG_IDF_TARGET_ESP32S2 - // TEST_ASSERT_UINT32_WITHIN(SOC_ADC_SAMPLE_FREQ_THRES_LOW*0.1, SOC_ADC_SAMPLE_FREQ_THRES_LOW, m1h_cnt); - // TEST_ASSERT_LESS_THAN_UINT32(5, m1l_cnt); //Actually, it will still encountered 1~2 times because hardware run very quickly + TEST_ASSERT_UINT32_WITHIN(SOC_ADC_SAMPLE_FREQ_THRES_LOW * 0.1, SOC_ADC_SAMPLE_FREQ_THRES_LOW, m1h_cnt); + TEST_ASSERT_EQUAL(0, m1l_cnt); //low limit should NOT triggrted when pull_up #endif - m1h_cnt = 0; - gpio_set_level(TEST_WAVE_OUT_PIN, 0); - } else { + break; + case GPIO_PULLDOWN_ONLY: TEST_ASSERT_UINT32_WITHIN(SOC_ADC_SAMPLE_FREQ_THRES_LOW * 0.1, SOC_ADC_SAMPLE_FREQ_THRES_LOW, m1l_cnt); - TEST_ASSERT_LESS_THAN_UINT32(5, m1h_cnt); //Actually, it will still encountered 1~2 times because hardware run very quickly - m1l_cnt = 0; - gpio_set_level(TEST_WAVE_OUT_PIN, 1); + TEST_ASSERT_EQUAL(0, m1h_cnt); + break; + case GPIO_PULLUP_PULLDOWN: + TEST_ASSERT_EQUAL(0, m1h_cnt); //half votage, both limit should NOT triggered + TEST_ASSERT_EQUAL(0, m1l_cnt); + break; + default: printf("unknown gpio pull mode !!!\n"); } } TEST_ESP_OK(adc_continuous_stop(handle)); diff --git a/components/hal/adc_hal.c b/components/hal/adc_hal.c index c24c33bc9b..42c04ac7ca 100644 --- a/components/hal/adc_hal.c +++ b/components/hal/adc_hal.c @@ -365,3 +365,14 @@ void adc_hal_digi_clr_eof(void) adc_ll_digi_dma_clr_eof(); } #endif + +#if SOC_ADC_MONITOR_SUPPORTED +void adc_hal_digi_monitor_set_thres(adc_monitor_id_t monitor_id, adc_unit_t adc_n, uint8_t adc_ch, int32_t h_thres, int32_t l_thres) +{ + adc_ll_digi_monitor_set_thres(monitor_id, adc_n, adc_ch, h_thres, l_thres); + + // enable peripheral intr_ena accordingly + adc_ll_digi_monitor_enable_intr(monitor_id, ADC_MONITOR_MODE_HIGH, (h_thres >= 0)); + adc_ll_digi_monitor_enable_intr(monitor_id, ADC_MONITOR_MODE_LOW, (l_thres >= 0)); +} +#endif //SOC_ADC_MONITOR_SUPPORTED diff --git a/components/hal/include/hal/adc_hal.h b/components/hal/include/hal/adc_hal.h index c085bb177e..b22247eff9 100644 --- a/components/hal/include/hal/adc_hal.h +++ b/components/hal/include/hal/adc_hal.h @@ -139,7 +139,7 @@ void adc_hal_dma_ctx_config(adc_hal_dma_ctx_t *hal, const adc_hal_dma_config_t * * Setting the digital controller. * * @param hal Context of the HAL - * @param cfg Pointer to digital controller paramter. + * @param cfg Pointer to digital controller parameter. */ void adc_hal_digi_controller_config(adc_hal_dma_ctx_t *hal, const adc_hal_digi_ctrlr_cfg_t *cfg); @@ -214,6 +214,17 @@ void adc_hal_digi_stop(adc_hal_dma_ctx_t *hal); void adc_hal_digi_clr_eof(void); #endif +/** + * @brief Set ADC monitor with high and low thresholds, and will enable the interrupts accordingly + * + * @param monitor_id Monitor to configure + * @param adc_n Which ADC unit will be monitored + * @param adc_ch Which ADC channel will be monitored + * @param h_thres High threshold (disable if < 0) + * @param l_thres Low threshold (disable if < 0) + */ +void adc_hal_digi_monitor_set_thres(adc_monitor_id_t monitor_id, adc_unit_t adc_n, uint8_t adc_ch, int32_t h_thres, int32_t l_thres); + #ifdef __cplusplus } #endif