From e3a717fd15d67dda4826d0626f42521594d3d7d2 Mon Sep 17 00:00:00 2001 From: gaoxu Date: Fri, 2 Aug 2024 16:52:35 +0800 Subject: [PATCH] feat(isp): fix potentiual AF/AE/AWB continuous and oneshot conflict and add test --- components/esp_driver_isp/src/isp_ae.c | 39 +++---- components/esp_driver_isp/src/isp_af.c | 104 +++++++++--------- components/esp_driver_isp/src/isp_awb.c | 64 +++++------ .../test_apps/isp/main/test_isp_driver.c | 87 ++++++++++++++- 4 files changed, 182 insertions(+), 112 deletions(-) diff --git a/components/esp_driver_isp/src/isp_ae.c b/components/esp_driver_isp/src/isp_ae.c index bfac4a330c..f6d421b450 100644 --- a/components/esp_driver_isp/src/isp_ae.c +++ b/components/esp_driver_isp/src/isp_ae.c @@ -171,24 +171,22 @@ esp_err_t esp_isp_ae_controller_get_oneshot_statistics(isp_ae_ctlr_t ae_ctlr, in TickType_t ticks = timeout_ms < 0 ? portMAX_DELAY : pdMS_TO_TICKS(timeout_ms); isp_fsm_t expected_fsm = ISP_FSM_ENABLE; - if (atomic_compare_exchange_strong(&ae_ctlr->fsm, &expected_fsm, ISP_FSM_ONESHOT)) { - // Reset the queue in case receiving the legacy data in the queue - xQueueReset(ae_ctlr->evt_que); + ESP_RETURN_ON_FALSE_ISR(atomic_compare_exchange_strong(&ae_ctlr->fsm, &expected_fsm, ISP_FSM_ONESHOT), ESP_ERR_INVALID_STATE, TAG, "controller isn't enabled or continuous statistics has started"); - // Disable the env detector when manual statistics. - // Otherwise, the env detector results may overwrite the manual statistics results when the statistics results are not read out in time - isp_ll_ae_env_detector_set_thresh(ae_ctlr->isp_proc->hal.hw, 0, 0); - // Trigger the AE statistics manually - isp_ll_ae_manual_update(ae_ctlr->isp_proc->hal.hw); - // Wait the statistics to finish and receive the result from the queue - if ((ticks > 0) && xQueueReceive(ae_ctlr->evt_que, out_res, ticks) != pdTRUE) { - ret = ESP_ERR_TIMEOUT; - } - // Re-enable the env detector after manual statistics. - isp_ll_ae_env_detector_set_thresh(ae_ctlr->isp_proc->hal.hw, ae_ctlr->low_thresh, ae_ctlr->high_thresh); - } else { - ESP_RETURN_ON_FALSE_ISR(false, ESP_ERR_INVALID_STATE, TAG, "controller is not enabled yet"); + // Reset the queue in case receiving the legacy data in the queue + xQueueReset(ae_ctlr->evt_que); + + // Disable the env detector when manual statistics. + // Otherwise, the env detector results may overwrite the manual statistics results when the statistics results are not read out in time + isp_ll_ae_env_detector_set_thresh(ae_ctlr->isp_proc->hal.hw, 0, 0); + // Trigger the AE statistics manually + isp_ll_ae_manual_update(ae_ctlr->isp_proc->hal.hw); + // Wait the statistics to finish and receive the result from the queue + if ((ticks > 0) && xQueueReceive(ae_ctlr->evt_que, out_res, ticks) != pdTRUE) { + ret = ESP_ERR_TIMEOUT; } + // Re-enable the env detector after manual statistics. + isp_ll_ae_env_detector_set_thresh(ae_ctlr->isp_proc->hal.hw, ae_ctlr->low_thresh, ae_ctlr->high_thresh); atomic_store(&ae_ctlr->fsm, ISP_FSM_ENABLE); return ret; @@ -199,11 +197,8 @@ esp_err_t esp_isp_ae_controller_start_continuous_statistics(isp_ae_ctlr_t ae_ctl ESP_RETURN_ON_FALSE_ISR(ae_ctlr, ESP_ERR_INVALID_ARG, TAG, "invalid argument: null pointer"); isp_fsm_t expected_fsm = ISP_FSM_ENABLE; - if (atomic_compare_exchange_strong(&ae_ctlr->fsm, &expected_fsm, ISP_FSM_CONTINUOUS)) { - isp_ll_ae_manual_update(ae_ctlr->isp_proc->hal.hw); - } else { - ESP_RETURN_ON_FALSE_ISR(false, ESP_ERR_INVALID_STATE, TAG, "controller is not enabled yet"); - } + ESP_RETURN_ON_FALSE_ISR(atomic_compare_exchange_strong(&ae_ctlr->fsm, &expected_fsm, ISP_FSM_CONTINUOUS), ESP_ERR_INVALID_STATE, TAG, "controller is not enabled yet"); + isp_ll_ae_manual_update(ae_ctlr->isp_proc->hal.hw); return ESP_OK; } @@ -297,7 +292,7 @@ bool IRAM_ATTR esp_isp_ae_isr(isp_proc_handle_t proc, uint32_t ae_events) need_yield |= high_task_awake == pdTRUE; /* If started continuous sampling, then trigger the next AE sample */ - if (atomic_load(&ae_ctlr->fsm) == ISP_FSM_START) { + if (atomic_load(&ae_ctlr->fsm) == ISP_FSM_CONTINUOUS) { isp_ll_ae_manual_update(ae_ctlr->isp_proc->hal.hw); } } diff --git a/components/esp_driver_isp/src/isp_af.c b/components/esp_driver_isp/src/isp_af.c index 9cef3208c1..28c2055328 100644 --- a/components/esp_driver_isp/src/isp_af.c +++ b/components/esp_driver_isp/src/isp_af.c @@ -6,6 +6,7 @@ #include #include +#include #include "sdkconfig.h" #include "esp_log.h" #include "esp_check.h" @@ -18,7 +19,7 @@ static const char *TAG = "ISP_AF"; typedef struct isp_af_controller_t { int id; - isp_fsm_t fsm; + _Atomic isp_fsm_t fsm; portMUX_TYPE spinlock; intr_handle_t intr_handle; isp_proc_handle_t isp_proc; @@ -104,7 +105,7 @@ esp_err_t esp_isp_new_af_controller(isp_proc_handle_t isp_proc, const esp_isp_af ESP_RETURN_ON_FALSE(af_ctlr, ESP_ERR_NO_MEM, TAG, "no mem"); af_ctlr->evt_que = xQueueCreateWithCaps(1, sizeof(isp_af_result_t), ISP_MEM_ALLOC_CAPS); ESP_GOTO_ON_FALSE(af_ctlr->evt_que, ESP_ERR_NO_MEM, err1, TAG, "no mem for af event queue"); - af_ctlr->fsm = ISP_FSM_INIT; + atomic_init(&af_ctlr->fsm, ISP_FSM_INIT); af_ctlr->spinlock = (portMUX_TYPE)portMUX_INITIALIZER_UNLOCKED; af_ctlr->isp_proc = isp_proc; @@ -141,7 +142,7 @@ err1: esp_err_t esp_isp_del_af_controller(isp_af_ctlr_t af_ctlr) { ESP_RETURN_ON_FALSE(af_ctlr && af_ctlr->isp_proc, ESP_ERR_INVALID_ARG, TAG, "invalid argument: null pointer"); - ESP_RETURN_ON_FALSE(af_ctlr->fsm == ISP_FSM_INIT, ESP_ERR_INVALID_STATE, TAG, "controller isn't in init state"); + ESP_RETURN_ON_FALSE(atomic_load(&af_ctlr->fsm) == ISP_FSM_INIT, ESP_ERR_INVALID_STATE, TAG, "controller not in init state"); bool exist = false; for (int i = 0; i < SOC_ISP_AF_CTLR_NUMS; i++) { if (af_ctlr->isp_proc->af_ctlr[i] == af_ctlr) { @@ -163,12 +164,13 @@ esp_err_t esp_isp_del_af_controller(isp_af_ctlr_t af_ctlr) esp_err_t esp_isp_af_controller_enable(isp_af_ctlr_t af_ctlr) { ESP_RETURN_ON_FALSE(af_ctlr && af_ctlr->isp_proc, ESP_ERR_INVALID_ARG, TAG, "invalid argument: null pointer"); - ESP_RETURN_ON_FALSE(af_ctlr->fsm == ISP_FSM_INIT, ESP_ERR_INVALID_STATE, TAG, "controller isn't in init state"); + isp_fsm_t expected_fsm = ISP_FSM_INIT; + ESP_RETURN_ON_FALSE(atomic_compare_exchange_strong(&af_ctlr->fsm, &expected_fsm, ISP_FSM_ENABLE), + ESP_ERR_INVALID_STATE, TAG, "controller not in init state"); isp_ll_af_clk_enable(af_ctlr->isp_proc->hal.hw, true); isp_ll_enable_intr(af_ctlr->isp_proc->hal.hw, ISP_LL_EVENT_AF_MASK, true); isp_ll_af_enable(af_ctlr->isp_proc->hal.hw, true); - af_ctlr->fsm = ISP_FSM_ENABLE; return ESP_OK; } @@ -176,54 +178,56 @@ esp_err_t esp_isp_af_controller_enable(isp_af_ctlr_t af_ctlr) esp_err_t esp_isp_af_controller_disable(isp_af_ctlr_t af_ctlr) { ESP_RETURN_ON_FALSE(af_ctlr && af_ctlr->isp_proc, ESP_ERR_INVALID_ARG, TAG, "invalid argument: null pointer"); - ESP_RETURN_ON_FALSE(af_ctlr->fsm == ISP_FSM_ENABLE, ESP_ERR_INVALID_STATE, TAG, "controller isn't in enable state"); + isp_fsm_t expected_fsm = ISP_FSM_ENABLE; + ESP_RETURN_ON_FALSE(atomic_compare_exchange_strong(&af_ctlr->fsm, &expected_fsm, ISP_FSM_INIT), + ESP_ERR_INVALID_STATE, TAG, "controller not in enable state"); isp_ll_af_clk_enable(af_ctlr->isp_proc->hal.hw, false); isp_ll_enable_intr(af_ctlr->isp_proc->hal.hw, ISP_LL_EVENT_AF_MASK, false); isp_ll_af_enable(af_ctlr->isp_proc->hal.hw, false); esp_intr_disable(af_ctlr->intr_handle); - af_ctlr->fsm = ISP_FSM_INIT; return ESP_OK; } -esp_err_t esp_isp_af_controller_get_oneshot_statistics(isp_af_ctlr_t af_ctrlr, int timeout_ms, isp_af_result_t *out_res) +esp_err_t esp_isp_af_controller_get_oneshot_statistics(isp_af_ctlr_t af_ctlr, int timeout_ms, isp_af_result_t *out_res) { - ESP_RETURN_ON_FALSE_ISR(af_ctrlr && (out_res || timeout_ms == 0), ESP_ERR_INVALID_ARG, TAG, "invalid argument: null pointer"); - ESP_RETURN_ON_FALSE_ISR(af_ctrlr->fsm == ISP_FSM_ENABLE, ESP_ERR_INVALID_STATE, TAG, "controller isn't enabled or continuous statistics has started"); + ESP_RETURN_ON_FALSE_ISR(af_ctlr && (out_res || timeout_ms == 0), ESP_ERR_INVALID_ARG, TAG, "invalid argument: null pointer"); esp_err_t ret = ESP_OK; TickType_t ticks = timeout_ms < 0 ? portMAX_DELAY : pdMS_TO_TICKS(timeout_ms); + isp_fsm_t expected_fsm = ISP_FSM_ENABLE; + ESP_RETURN_ON_FALSE_ISR(atomic_compare_exchange_strong(&af_ctlr->fsm, &expected_fsm, ISP_FSM_ONESHOT), ESP_ERR_INVALID_STATE, TAG, "controller isn't enabled or continuous statistics has starte"); + // Reset the queue in case receiving the legacy data in the queue - xQueueReset(af_ctrlr->evt_que); + xQueueReset(af_ctlr->evt_que); // Trigger the AF statistics manually - isp_ll_af_manual_update(af_ctrlr->isp_proc->hal.hw); + isp_ll_af_manual_update(af_ctlr->isp_proc->hal.hw); // Wait the statistics to finish and receive the result from the queue - if ((ticks > 0) && xQueueReceive(af_ctrlr->evt_que, out_res, ticks) != pdTRUE) { + if ((ticks > 0) && xQueueReceive(af_ctlr->evt_que, out_res, ticks) != pdTRUE) { ret = ESP_ERR_TIMEOUT; } + atomic_store(&af_ctlr->fsm, ISP_FSM_ENABLE); return ret; } -esp_err_t esp_isp_af_controller_start_continuous_statistics(isp_af_ctlr_t af_ctrlr) +esp_err_t esp_isp_af_controller_start_continuous_statistics(isp_af_ctlr_t af_ctlr) { - ESP_RETURN_ON_FALSE_ISR(af_ctrlr, ESP_ERR_INVALID_ARG, TAG, "invalid argument: null pointer"); - ESP_RETURN_ON_FALSE_ISR(af_ctrlr->fsm == ISP_FSM_ENABLE, ESP_ERR_INVALID_STATE, TAG, "controller isn't in enable state"); - - af_ctrlr->fsm = ISP_FSM_START; - isp_ll_af_enable_auto_update(af_ctrlr->isp_proc->hal.hw, true); + ESP_RETURN_ON_FALSE_ISR(af_ctlr, ESP_ERR_INVALID_ARG, TAG, "invalid argument: null pointer"); + isp_fsm_t expected_fsm = ISP_FSM_ENABLE; + ESP_RETURN_ON_FALSE_ISR(atomic_compare_exchange_strong(&af_ctlr->fsm, &expected_fsm, ISP_FSM_CONTINUOUS), ESP_ERR_INVALID_STATE, TAG, "controller is not enabled yet"); + isp_ll_af_enable_auto_update(af_ctlr->isp_proc->hal.hw, true); return ESP_OK; } -esp_err_t esp_isp_af_controller_stop_continuous_statistics(isp_af_ctlr_t af_ctrlr) +esp_err_t esp_isp_af_controller_stop_continuous_statistics(isp_af_ctlr_t af_ctlr) { - ESP_RETURN_ON_FALSE_ISR(af_ctrlr, ESP_ERR_INVALID_ARG, TAG, "invalid argument: null pointer"); - ESP_RETURN_ON_FALSE_ISR(af_ctrlr->fsm == ISP_FSM_START, ESP_ERR_INVALID_STATE, TAG, "controller isn't in continuous state"); - - isp_ll_af_enable_auto_update(af_ctrlr->isp_proc->hal.hw, false); - af_ctrlr->fsm = ISP_FSM_ENABLE; + ESP_RETURN_ON_FALSE_ISR(af_ctlr, ESP_ERR_INVALID_ARG, TAG, "invalid argument: null pointer"); + isp_fsm_t expected_fsm = ISP_FSM_CONTINUOUS; + ESP_RETURN_ON_FALSE_ISR(atomic_compare_exchange_strong(&af_ctlr->fsm, &expected_fsm, ISP_FSM_ENABLE), ESP_ERR_INVALID_STATE, TAG, "controller is not enabled yet"); + isp_ll_af_enable_auto_update(af_ctlr->isp_proc->hal.hw, false); return ESP_OK; } @@ -231,27 +235,27 @@ esp_err_t esp_isp_af_controller_stop_continuous_statistics(isp_af_ctlr_t af_ctrl /*--------------------------------------------- AF Env Detector ----------------------------------------------*/ -esp_err_t esp_isp_af_controller_set_env_detector(isp_af_ctlr_t af_ctrlr, const esp_isp_af_env_config_t *env_config) +esp_err_t esp_isp_af_controller_set_env_detector(isp_af_ctlr_t af_ctlr, const esp_isp_af_env_config_t *env_config) { - ESP_RETURN_ON_FALSE(af_ctrlr && env_config, ESP_ERR_INVALID_ARG, TAG, "invalid argument: null pointer"); - ESP_RETURN_ON_FALSE(af_ctrlr->fsm == ISP_FSM_INIT, ESP_ERR_INVALID_STATE, TAG, "invalid fsm, should be called when in init state"); + ESP_RETURN_ON_FALSE(af_ctlr && env_config, ESP_ERR_INVALID_ARG, TAG, "invalid argument: null pointer"); + ESP_RETURN_ON_FALSE(atomic_load(&af_ctlr->fsm) == ISP_FSM_INIT, ESP_ERR_INVALID_STATE, TAG, "controller not in init state"); - af_ctrlr->config.interval = env_config->interval; + af_ctlr->config.interval = env_config->interval; - isp_ll_af_env_detector_set_period(af_ctrlr->isp_proc->hal.hw, 0); - isp_ll_clear_intr(af_ctrlr->isp_proc->hal.hw, ISP_LL_EVENT_AF_ENV); + isp_ll_af_env_detector_set_period(af_ctlr->isp_proc->hal.hw, 0); + isp_ll_clear_intr(af_ctlr->isp_proc->hal.hw, ISP_LL_EVENT_AF_ENV); - isp_ll_af_env_detector_set_mode(af_ctrlr->isp_proc->hal.hw, ISP_LL_AF_ENV_DETECTOR_MODE_ABS); - isp_ll_af_env_detector_set_period(af_ctrlr->isp_proc->hal.hw, af_ctrlr->config.interval); - isp_ll_enable_intr(af_ctrlr->isp_proc->hal.hw, ISP_LL_EVENT_AF_ENV, true); + isp_ll_af_env_detector_set_mode(af_ctlr->isp_proc->hal.hw, ISP_LL_AF_ENV_DETECTOR_MODE_ABS); + isp_ll_af_env_detector_set_period(af_ctlr->isp_proc->hal.hw, af_ctlr->config.interval); + isp_ll_enable_intr(af_ctlr->isp_proc->hal.hw, ISP_LL_EVENT_AF_ENV, true); return ESP_OK; } -esp_err_t esp_isp_af_env_detector_register_event_callbacks(isp_af_ctlr_t af_ctrlr, const esp_isp_af_env_detector_evt_cbs_t *cbs, void *user_data) +esp_err_t esp_isp_af_env_detector_register_event_callbacks(isp_af_ctlr_t af_ctlr, const esp_isp_af_env_detector_evt_cbs_t *cbs, void *user_data) { - ESP_RETURN_ON_FALSE(af_ctrlr && cbs, ESP_ERR_INVALID_ARG, TAG, "invalid argument"); - ESP_RETURN_ON_FALSE(af_ctrlr->fsm == ISP_FSM_INIT, ESP_ERR_INVALID_STATE, TAG, "detector isn't in the init state"); + ESP_RETURN_ON_FALSE(af_ctlr && cbs, ESP_ERR_INVALID_ARG, TAG, "invalid argument"); + ESP_RETURN_ON_FALSE(atomic_load(&af_ctlr->fsm) == ISP_FSM_INIT, ESP_ERR_INVALID_STATE, TAG, "controller not in init state"); #if CONFIG_ISP_ISR_IRAM_SAFE if (cbs->on_env_statistics_done) { @@ -264,19 +268,19 @@ esp_err_t esp_isp_af_env_detector_register_event_callbacks(isp_af_ctlr_t af_ctrl ESP_RETURN_ON_FALSE(esp_ptr_internal(user_data), ESP_ERR_INVALID_ARG, TAG, "user context not in internal RAM"); } #endif - af_ctrlr->cbs.on_env_statistics_done = cbs->on_env_statistics_done; - af_ctrlr->cbs.on_env_change = cbs->on_env_change; - af_ctrlr->user_data = user_data; + af_ctlr->cbs.on_env_statistics_done = cbs->on_env_statistics_done; + af_ctlr->cbs.on_env_change = cbs->on_env_change; + af_ctlr->user_data = user_data; return ESP_OK; } -esp_err_t esp_isp_af_controller_set_env_detector_threshold(isp_af_ctlr_t af_ctrlr, int definition_thresh, int luminance_thresh) +esp_err_t esp_isp_af_controller_set_env_detector_threshold(isp_af_ctlr_t af_ctlr, int definition_thresh, int luminance_thresh) { - ESP_RETURN_ON_FALSE_ISR(af_ctrlr, ESP_ERR_INVALID_ARG, TAG, "invalid argument"); - ESP_RETURN_ON_FALSE_ISR(af_ctrlr->fsm == ISP_FSM_ENABLE, ESP_ERR_INVALID_STATE, TAG, "detector isn't in enable state"); + ESP_RETURN_ON_FALSE_ISR(af_ctlr, ESP_ERR_INVALID_ARG, TAG, "invalid argument"); + ESP_RETURN_ON_FALSE(atomic_load(&af_ctlr->fsm) == ISP_FSM_ENABLE, ESP_ERR_INVALID_STATE, TAG, "controller not in enable state"); - isp_ll_af_env_detector_set_thresh(af_ctrlr->isp_proc->hal.hw, definition_thresh, luminance_thresh); + isp_ll_af_env_detector_set_thresh(af_ctlr->isp_proc->hal.hw, definition_thresh, luminance_thresh); return ESP_OK; } @@ -286,7 +290,7 @@ esp_err_t esp_isp_af_controller_set_env_detector_threshold(isp_af_ctlr_t af_ctrl ---------------------------------------------------------------*/ bool IRAM_ATTR esp_isp_af_isr(isp_proc_handle_t proc, uint32_t af_events) { - isp_af_ctlr_t af_ctrlr = proc->af_ctlr[0]; + isp_af_ctlr_t af_ctlr = proc->af_ctlr[0]; bool need_yield = false; esp_isp_af_env_detector_evt_data_t edata = {}; @@ -307,17 +311,17 @@ bool IRAM_ATTR esp_isp_af_isr(isp_proc_handle_t proc, uint32_t af_events) if (af_events & ISP_LL_EVENT_AF_FDONE) { BaseType_t high_task_awake = false; // Send the event data to the queue, overwrite the legacy one if exist - xQueueOverwriteFromISR(af_ctrlr->evt_que, &edata.af_result, &high_task_awake); + xQueueOverwriteFromISR(af_ctlr->evt_que, &edata.af_result, &high_task_awake); // Invoke the callback if the callback is registered need_yield |= high_task_awake == pdTRUE; - if (af_ctrlr->cbs.on_env_statistics_done) { - need_yield |= af_ctrlr->cbs.on_env_statistics_done(af_ctrlr, &edata, af_ctrlr->user_data); + if (af_ctlr->cbs.on_env_statistics_done) { + need_yield |= af_ctlr->cbs.on_env_statistics_done(af_ctlr, &edata, af_ctlr->user_data); } } if (af_events & ISP_LL_EVENT_AF_ENV) { // Invoke the callback if the callback is registered - if (af_ctrlr->cbs.on_env_change) { - need_yield |= af_ctrlr->cbs.on_env_change(af_ctrlr, &edata, af_ctrlr->user_data); + if (af_ctlr->cbs.on_env_change) { + need_yield |= af_ctlr->cbs.on_env_change(af_ctlr, &edata, af_ctlr->user_data); } } return need_yield; diff --git a/components/esp_driver_isp/src/isp_awb.c b/components/esp_driver_isp/src/isp_awb.c index fbc22d959c..eb8abbb7d7 100644 --- a/components/esp_driver_isp/src/isp_awb.c +++ b/components/esp_driver_isp/src/isp_awb.c @@ -6,6 +6,7 @@ #include #include +#include #include "freertos/FreeRTOS.h" #include "sdkconfig.h" #include "esp_log.h" @@ -15,12 +16,11 @@ #include "esp_private/isp_private.h" typedef struct isp_awb_controller_t { - isp_fsm_t fsm; + _Atomic isp_fsm_t fsm; portMUX_TYPE spinlock; intr_handle_t intr_handle; isp_proc_handle_t isp_proc; QueueHandle_t evt_que; - SemaphoreHandle_t stat_lock; esp_isp_awb_cbs_t cbs; void *user_data; } isp_awb_controller_t; @@ -63,9 +63,6 @@ static void s_isp_awb_free_controller(isp_awb_ctlr_t awb_ctlr) if (awb_ctlr->evt_que) { vQueueDeleteWithCaps(awb_ctlr->evt_que); } - if (awb_ctlr->stat_lock) { - vSemaphoreDeleteWithCaps(awb_ctlr->stat_lock); - } free(awb_ctlr); } } @@ -98,9 +95,7 @@ esp_err_t esp_isp_new_awb_controller(isp_proc_handle_t isp_proc, const esp_isp_a ESP_RETURN_ON_FALSE(awb_ctlr, ESP_ERR_NO_MEM, TAG, "no mem for awb controller"); awb_ctlr->evt_que = xQueueCreateWithCaps(1, sizeof(isp_awb_stat_result_t), ISP_MEM_ALLOC_CAPS); ESP_GOTO_ON_FALSE(awb_ctlr->evt_que, ESP_ERR_NO_MEM, err1, TAG, "no mem for awb event queue"); - awb_ctlr->stat_lock = xSemaphoreCreateBinaryWithCaps(ISP_MEM_ALLOC_CAPS); - ESP_GOTO_ON_FALSE(awb_ctlr->stat_lock, ESP_ERR_NO_MEM, err1, TAG, "no mem for awb semaphore"); - awb_ctlr->fsm = ISP_FSM_INIT; + atomic_init(&awb_ctlr->fsm, ISP_FSM_INIT); awb_ctlr->spinlock = (portMUX_TYPE)portMUX_INITIALIZER_UNLOCKED; awb_ctlr->isp_proc = isp_proc; @@ -133,7 +128,7 @@ esp_err_t esp_isp_del_awb_controller(isp_awb_ctlr_t awb_ctlr) { ESP_RETURN_ON_FALSE(awb_ctlr && awb_ctlr->isp_proc, ESP_ERR_INVALID_ARG, TAG, "invalid argument: null pointer"); ESP_RETURN_ON_FALSE(awb_ctlr->isp_proc->awb_ctlr == awb_ctlr, ESP_ERR_INVALID_ARG, TAG, "controller isn't in use"); - ESP_RETURN_ON_FALSE(awb_ctlr->fsm == ISP_FSM_INIT, ESP_ERR_INVALID_STATE, TAG, "controller isn't in init state"); + ESP_RETURN_ON_FALSE(atomic_load(&awb_ctlr->fsm) == ISP_FSM_INIT, ESP_ERR_INVALID_STATE, TAG, "controller not in init state"); ESP_RETURN_ON_FALSE(esp_isp_deregister_isr(awb_ctlr->isp_proc, ISP_SUBMODULE_AWB) == ESP_OK, ESP_FAIL, TAG, "fail to deregister ISR"); s_isp_declaim_awb_controller(awb_ctlr); @@ -147,19 +142,21 @@ esp_err_t esp_isp_del_awb_controller(isp_awb_ctlr_t awb_ctlr) esp_err_t esp_isp_awb_controller_reconfig(isp_awb_ctlr_t awb_ctlr, const esp_isp_awb_config_t *awb_cfg) { ESP_RETURN_ON_FALSE(awb_ctlr && awb_cfg, ESP_ERR_INVALID_ARG, TAG, "invalid argument: null pointer"); + ESP_RETURN_ON_FALSE(atomic_load(&awb_ctlr->fsm) == ISP_FSM_ENABLE, ESP_ERR_INVALID_STATE, TAG, "controller not in enable state"); + return s_esp_isp_awb_config_hardware(awb_ctlr->isp_proc, awb_cfg); } esp_err_t esp_isp_awb_controller_enable(isp_awb_ctlr_t awb_ctlr) { ESP_RETURN_ON_FALSE(awb_ctlr && awb_ctlr->isp_proc, ESP_ERR_INVALID_ARG, TAG, "invalid argument: null pointer"); - ESP_RETURN_ON_FALSE(awb_ctlr->fsm == ISP_FSM_INIT, ESP_ERR_INVALID_STATE, TAG, "controller isn't in init state"); - awb_ctlr->fsm = ISP_FSM_ENABLE; + isp_fsm_t expected_fsm = ISP_FSM_INIT; + ESP_RETURN_ON_FALSE(atomic_compare_exchange_strong(&awb_ctlr->fsm, &expected_fsm, ISP_FSM_ENABLE), + ESP_ERR_INVALID_STATE, TAG, "controller not in init state"); esp_err_t ret = ESP_OK; isp_ll_awb_clk_enable(awb_ctlr->isp_proc->hal.hw, true); isp_ll_enable_intr(awb_ctlr->isp_proc->hal.hw, ISP_LL_EVENT_AWB_MASK, true); - xSemaphoreGive(awb_ctlr->stat_lock); return ret; } @@ -167,9 +164,9 @@ esp_err_t esp_isp_awb_controller_enable(isp_awb_ctlr_t awb_ctlr) esp_err_t esp_isp_awb_controller_disable(isp_awb_ctlr_t awb_ctlr) { ESP_RETURN_ON_FALSE(awb_ctlr && awb_ctlr->isp_proc, ESP_ERR_INVALID_ARG, TAG, "invalid argument: null pointer"); - ESP_RETURN_ON_FALSE(awb_ctlr->fsm == ISP_FSM_ENABLE, ESP_ERR_INVALID_STATE, TAG, "controller isn't in enable state"); - xSemaphoreTake(awb_ctlr->stat_lock, 0); - awb_ctlr->fsm = ISP_FSM_INIT; + isp_fsm_t expected_fsm = ISP_FSM_ENABLE; + ESP_RETURN_ON_FALSE(atomic_compare_exchange_strong(&awb_ctlr->fsm, &expected_fsm, ISP_FSM_INIT), + ESP_ERR_INVALID_STATE, TAG, "controller not in enable state"); isp_ll_enable_intr(awb_ctlr->isp_proc->hal.hw, ISP_LL_EVENT_AWB_MASK, false); isp_ll_awb_clk_enable(awb_ctlr->isp_proc->hal.hw, false); @@ -183,10 +180,10 @@ esp_err_t esp_isp_awb_controller_get_oneshot_statistics(isp_awb_ctlr_t awb_ctlr, ESP_RETURN_ON_FALSE(awb_ctlr && (out_res || timeout_ms == 0), ESP_ERR_INVALID_ARG, TAG, "invalid argument: null pointer"); esp_err_t ret = ESP_OK; TickType_t ticks = timeout_ms < 0 ? portMAX_DELAY : pdMS_TO_TICKS(timeout_ms); - xSemaphoreTake(awb_ctlr->stat_lock, ticks); - ESP_GOTO_ON_FALSE(awb_ctlr->fsm == ISP_FSM_ENABLE, ESP_ERR_INVALID_STATE, err, TAG, "controller isn't in enable state"); - // Update state to avoid race condition - awb_ctlr->fsm = ISP_FSM_START; + + isp_fsm_t expected_fsm = ISP_FSM_ENABLE; + ESP_RETURN_ON_FALSE_ISR(atomic_compare_exchange_strong(&awb_ctlr->fsm, &expected_fsm, ISP_FSM_ONESHOT), ESP_ERR_INVALID_STATE, TAG, "controller isn't enabled or continuous statistics has starte"); + // Reset the queue in case receiving the legacy data in the queue xQueueReset(awb_ctlr->evt_que); // Start the AWB white patch statistics and waiting it done @@ -197,9 +194,8 @@ esp_err_t esp_isp_awb_controller_get_oneshot_statistics(isp_awb_ctlr_t awb_ctlr, } // Stop the AWB white patch statistics isp_ll_awb_enable(awb_ctlr->isp_proc->hal.hw, false); - awb_ctlr->fsm = ISP_FSM_ENABLE; -err: - xSemaphoreGive(awb_ctlr->stat_lock); + + atomic_store(&awb_ctlr->fsm, ISP_FSM_ENABLE); return ret; } @@ -207,29 +203,21 @@ esp_err_t esp_isp_awb_controller_start_continuous_statistics(isp_awb_ctlr_t awb_ { ESP_RETURN_ON_FALSE(awb_ctlr, ESP_ERR_INVALID_ARG, TAG, "invalid argument: null pointer"); ESP_RETURN_ON_FALSE(awb_ctlr->cbs.on_statistics_done, ESP_ERR_INVALID_STATE, TAG, "invalid state: on_statistics_done callback not registered"); - esp_err_t ret = ESP_OK; - if (xSemaphoreTake(awb_ctlr->stat_lock, 0) == pdFALSE) { - ESP_LOGW(TAG, "statistics lock is not acquired, controller is busy"); - return ESP_ERR_INVALID_STATE; - } - ESP_GOTO_ON_FALSE(awb_ctlr->fsm == ISP_FSM_ENABLE, ESP_ERR_INVALID_STATE, err, TAG, "controller isn't in enable state"); - awb_ctlr->fsm = ISP_FSM_START; + + isp_fsm_t expected_fsm = ISP_FSM_ENABLE; + ESP_RETURN_ON_FALSE_ISR(atomic_compare_exchange_strong(&awb_ctlr->fsm, &expected_fsm, ISP_FSM_CONTINUOUS), ESP_ERR_INVALID_STATE, TAG, "controller is not enabled yet"); isp_ll_awb_enable(awb_ctlr->isp_proc->hal.hw, true); - return ret; -err: - xSemaphoreGive(awb_ctlr->stat_lock); - return ret; + return ESP_OK; } esp_err_t esp_isp_awb_controller_stop_continuous_statistics(isp_awb_ctlr_t awb_ctlr) { ESP_RETURN_ON_FALSE(awb_ctlr, ESP_ERR_INVALID_ARG, TAG, "invalid argument: null pointer"); - ESP_RETURN_ON_FALSE(awb_ctlr->fsm == ISP_FSM_START, ESP_ERR_INVALID_STATE, TAG, "controller isn't in continuous state"); + isp_fsm_t expected_fsm = ISP_FSM_CONTINUOUS; + ESP_RETURN_ON_FALSE_ISR(atomic_compare_exchange_strong(&awb_ctlr->fsm, &expected_fsm, ISP_FSM_ENABLE), ESP_ERR_INVALID_STATE, TAG, "controller is not enabled yet"); isp_ll_awb_enable(awb_ctlr->isp_proc->hal.hw, false); - awb_ctlr->fsm = ISP_FSM_ENABLE; - xSemaphoreGive(awb_ctlr->stat_lock); return ESP_OK; } @@ -261,7 +249,7 @@ bool IRAM_ATTR esp_isp_awb_isr(isp_proc_handle_t proc, uint32_t awb_events) xQueueOverwriteFromISR(awb_ctlr->evt_que, &edata.awb_result, &high_task_awake); need_yield |= high_task_awake == pdTRUE; /* If started continuous sampling, then trigger the next AWB sample */ - if (awb_ctlr->fsm == ISP_FSM_START) { + if (atomic_load(&awb_ctlr->fsm) == ISP_FSM_CONTINUOUS) { isp_ll_awb_enable(awb_ctlr->isp_proc->hal.hw, false); isp_ll_awb_enable(awb_ctlr->isp_proc->hal.hw, true); } @@ -272,7 +260,7 @@ bool IRAM_ATTR esp_isp_awb_isr(isp_proc_handle_t proc, uint32_t awb_events) esp_err_t esp_isp_awb_register_event_callbacks(isp_awb_ctlr_t awb_ctlr, const esp_isp_awb_cbs_t *cbs, void *user_data) { ESP_RETURN_ON_FALSE(awb_ctlr && cbs, ESP_ERR_INVALID_ARG, TAG, "invalid argument"); - ESP_RETURN_ON_FALSE(awb_ctlr->fsm == ISP_FSM_INIT, ESP_ERR_INVALID_STATE, TAG, "detector isn't in the init state"); + ESP_RETURN_ON_FALSE(atomic_load(&awb_ctlr->fsm) == ISP_FSM_INIT, ESP_ERR_INVALID_STATE, TAG, "controller not in init state"); #if CONFIG_ISP_ISR_IRAM_SAFE if (cbs->on_statistics_done) { ESP_RETURN_ON_FALSE(esp_ptr_in_iram(cbs->on_env_change), ESP_ERR_INVALID_ARG, TAG, "on_env_change callback not in IRAM"); diff --git a/components/esp_driver_isp/test_apps/isp/main/test_isp_driver.c b/components/esp_driver_isp/test_apps/isp/main/test_isp_driver.c index 5a583c13bc..ffcaeba7f9 100644 --- a/components/esp_driver_isp/test_apps/isp/main/test_isp_driver.c +++ b/components/esp_driver_isp/test_apps/isp/main/test_isp_driver.c @@ -9,6 +9,10 @@ #include "driver/isp.h" #include "soc/soc_caps.h" +/*--------------------------------------------------------------- + ISP +---------------------------------------------------------------*/ + TEST_CASE("ISP processor exhausted allocation", "[isp]") { esp_isp_processor_cfg_t isp_config = { @@ -30,6 +34,75 @@ TEST_CASE("ISP processor exhausted allocation", "[isp]") } } +/*--------------------------------------------------------------- + AF +---------------------------------------------------------------*/ + +static bool test_isp_af_default_on_statistics_done_cb(isp_af_ctlr_t af_ctlr, const esp_isp_af_env_detector_evt_data_t *edata, void *user_data) +{ + (void) af_ctlr; + (void) edata; + (void) user_data; + // Do nothing + return false; +} + +static bool test_isp_af_default_on_continuous_done_cb(isp_af_ctlr_t af_ctlr, const esp_isp_af_env_detector_evt_data_t *edata, void *user_data) +{ + (void) af_ctlr; + (void) edata; + (void) user_data; + // Do nothing + return false; +} + +TEST_CASE("ISP AF driver oneshot vs continuous test", "[isp]") +{ + esp_isp_processor_cfg_t isp_config = { + .clk_hz = 80 * 1000 * 1000, + .input_data_source = ISP_INPUT_DATA_SOURCE_CSI, + .input_data_color_type = ISP_COLOR_RAW8, + .output_data_color_type = ISP_COLOR_RGB565, + }; + isp_proc_handle_t isp_proc = NULL; + TEST_ESP_OK(esp_isp_new_processor(&isp_config, &isp_proc)); + TEST_ESP_OK(esp_isp_enable(isp_proc)); + + esp_isp_af_config_t af_config = { + .edge_thresh = 128, + }; + isp_af_ctlr_t af_ctlr = NULL; + isp_af_result_t af_res = {}; + /* Create the af controller */ + TEST_ESP_OK(esp_isp_new_af_controller(isp_proc, &af_config, &af_ctlr)); + /* Register af callback */ + esp_isp_af_env_detector_evt_cbs_t af_cb = { + .on_env_statistics_done = test_isp_af_default_on_statistics_done_cb, + .on_env_change = test_isp_af_default_on_continuous_done_cb, + }; + TEST_ESP_OK(esp_isp_af_env_detector_register_event_callbacks(af_ctlr, &af_cb, NULL)); + /* Enabled the af controller */ + TEST_ESP_OK(esp_isp_af_controller_enable(af_ctlr)); + /* Start continuous af statistics */ + TEST_ESP_OK(esp_isp_af_controller_start_continuous_statistics(af_ctlr)); + + /* Test do oneshot mode when continuous mode start, should return error */ + TEST_ESP_ERR(ESP_ERR_INVALID_STATE, esp_isp_af_controller_get_oneshot_statistics(af_ctlr, 0, &af_res)); + + /* Stop continuous af statistics */ + TEST_ESP_OK(esp_isp_af_controller_stop_continuous_statistics(af_ctlr)); + TEST_ESP_ERR(ESP_ERR_TIMEOUT, esp_isp_af_controller_get_oneshot_statistics(af_ctlr, 1, &af_res)); + + /* Disable the af controller */ + TEST_ESP_OK(esp_isp_af_controller_disable(af_ctlr)); + + /* Delete the af controller and free the resources */ + TEST_ESP_OK(esp_isp_del_af_controller(af_ctlr)); + + TEST_ESP_OK(esp_isp_disable(isp_proc)); + TEST_ESP_OK(esp_isp_del_processor(isp_proc)); +} + TEST_CASE("ISP AF controller exhausted allocation", "[isp]") { esp_isp_processor_cfg_t isp_config = { @@ -57,6 +130,10 @@ TEST_CASE("ISP AF controller exhausted allocation", "[isp]") TEST_ESP_OK(esp_isp_del_processor(isp_proc)); } +/*--------------------------------------------------------------- + AWB +---------------------------------------------------------------*/ + static bool test_isp_awb_default_on_statistics_done_cb(isp_awb_ctlr_t awb_ctlr, const esp_isp_awb_evt_data_t *edata, void *user_data) { (void) awb_ctlr; @@ -66,7 +143,7 @@ static bool test_isp_awb_default_on_statistics_done_cb(isp_awb_ctlr_t awb_ctlr, return false; } -TEST_CASE("ISP AWB driver basic function", "[isp]") +TEST_CASE("ISP AWB driver oneshot vs continuous test", "[isp]") { esp_isp_processor_cfg_t isp_config = { .clk_hz = 80 * 1000 * 1000, @@ -154,6 +231,10 @@ TEST_CASE("ISP CCM basic function", "[isp]") TEST_ESP_OK(esp_isp_del_processor(isp_proc)); } +/*--------------------------------------------------------------- + AE +---------------------------------------------------------------*/ + static bool test_isp_ae_default_on_statistics_done_cb(isp_ae_ctlr_t ae_ctlr, const esp_isp_ae_env_detector_evt_data_t *edata, void *user_data) { (void) ae_ctlr; @@ -172,7 +253,7 @@ static bool test_isp_ae_default_on_continuous_done_cb(isp_ae_ctlr_t ae_ctlr, con return false; } -TEST_CASE("ISP AE driver basic function", "[isp]") +TEST_CASE("ISP AE driver oneshot vs continuous test", "[isp]") { esp_isp_processor_cfg_t isp_config = { .clk_hz = 80 * 1000 * 1000, @@ -202,6 +283,8 @@ TEST_CASE("ISP AE driver basic function", "[isp]") TEST_ESP_OK(esp_isp_ae_controller_enable(ae_ctlr)); /* Start continuous AE statistics */ TEST_ESP_OK(esp_isp_ae_controller_start_continuous_statistics(ae_ctlr)); + + /* Test do oneshot mode when continuous mode start, should return error */ TEST_ESP_ERR(ESP_ERR_INVALID_STATE, esp_isp_ae_controller_get_oneshot_statistics(ae_ctlr, 0, &ae_res)); /* Stop continuous AE statistics */