espressif/esp-idf
1
0
Fork 1
mirror of https://github.com/espressif/esp-idf.git synced 2025-08-05 21:54:33 +02:00
Code Issues Packages Projects Releases Wiki Activity

driver: fix dead code in error handling path

Browse Source 
... for gptimer and pulse_cnt driver, reported by Coverity Scan
This commit is contained in:
morris
2022-04-08 13:31:29 +08:00
parent 483149e41b
commit 5732e2a4be
6 changed files with 164 additions and 96 deletions
Download Patch File Download Diff File Expand all files Collapse all files

1
components/driver/deprecated/driver/pcnt.h
View File

@@ -6,6 +6,7 @@
#pragma once
#include <stdint.h>
#include "sdkconfig.h"
#include "esp_err.h"
#include "driver/pcnt_types_legacy.h"

118
components/driver/gptimer.c
View File

@@ -100,27 +100,17 @@ static void gptimer_release_group_handle(gptimer_group_t *group);
static esp_err_t gptimer_select_periph_clock(gptimer_t *timer, gptimer_clock_source_t src_clk, uint32_t resolution_hz);
static void gptimer_default_isr(void *args);
esp_err_t gptimer_new_timer(const gptimer_config_t *config, gptimer_handle_t *ret_timer)
static esp_err_t gptimer_register_to_group(gptimer_t *timer)
{
esp_err_t ret = ESP_OK;
gptimer_group_t *group = NULL;
gptimer_t *timer = NULL;
int group_id = -1;
int timer_id = -1;
ESP_GOTO_ON_FALSE(config && ret_timer, ESP_ERR_INVALID_ARG, err, TAG, "invalid argument");
ESP_GOTO_ON_FALSE(config->resolution_hz, ESP_ERR_INVALID_ARG, err, TAG, "invalid timer resolution:%d", config->resolution_hz);
timer = heap_caps_calloc(1, sizeof(gptimer_t), GPTIMER_MEM_ALLOC_CAPS);
ESP_GOTO_ON_FALSE(timer, ESP_ERR_NO_MEM, err, TAG, "no mem for gptimer");
for (int i = 0; (i < SOC_TIMER_GROUPS) && (timer_id < 0); i++) {
for (int i = 0; i < SOC_TIMER_GROUPS; i++) {
group = gptimer_acquire_group_handle(i);
ESP_GOTO_ON_FALSE(group, ESP_ERR_NO_MEM, err, TAG, "no mem for group (%d)", i);
ESP_RETURN_ON_FALSE(group, ESP_ERR_NO_MEM, TAG, "no mem for group (%d)", i);
// loop to search free timer in the group
portENTER_CRITICAL(&group->spinlock);
for (int j = 0; j < SOC_TIMER_GROUP_TIMERS_PER_GROUP; j++) {
if (!group->timers[j]) {
group_id = i;
timer_id = j;
group->timers[j] = timer;
break;
@@ -130,12 +120,60 @@ esp_err_t gptimer_new_timer(const gptimer_config_t *config, gptimer_handle_t *re
if (timer_id < 0) {
gptimer_release_group_handle(group);
group = NULL;
}
}
ESP_GOTO_ON_FALSE(timer_id != -1, ESP_ERR_NOT_FOUND, err, TAG, "no free timer");
} else {
timer->timer_id = timer_id;
timer->group = group;
break;;
}
}
ESP_RETURN_ON_FALSE(timer_id != -1, ESP_ERR_NOT_FOUND, TAG, "no free timer");
return ESP_OK;
}
static void gptimer_unregister_from_group(gptimer_t *timer)
{
gptimer_group_t *group = timer->group;
int timer_id = timer->timer_id;
portENTER_CRITICAL(&group->spinlock);
group->timers[timer_id] = NULL;
portEXIT_CRITICAL(&group->spinlock);
// timer has a reference on group, release it now
gptimer_release_group_handle(group);
}
static esp_err_t gptimer_destory(gptimer_t *timer)
{
if (timer->pm_lock) {
ESP_RETURN_ON_ERROR(esp_pm_lock_delete(timer->pm_lock), TAG, "delete pm_lock failed");
}
if (timer->intr) {
ESP_RETURN_ON_ERROR(esp_intr_free(timer->intr), TAG, "delete interrupt service failed");
}
if (timer->group) {
gptimer_unregister_from_group(timer);
}
free(timer);
return ESP_OK;
}
esp_err_t gptimer_new_timer(const gptimer_config_t *config, gptimer_handle_t *ret_timer)
{
#if CONFIG_GPTIMER_ENABLE_DEBUG_LOG
esp_log_level_set(TAG, ESP_LOG_DEBUG);
#endif
esp_err_t ret = ESP_OK;
gptimer_t *timer = NULL;
ESP_GOTO_ON_FALSE(config && ret_timer, ESP_ERR_INVALID_ARG, err, TAG, "invalid argument");
ESP_GOTO_ON_FALSE(config->resolution_hz, ESP_ERR_INVALID_ARG, err, TAG, "invalid timer resolution:%d", config->resolution_hz);
timer = heap_caps_calloc(1, sizeof(gptimer_t), GPTIMER_MEM_ALLOC_CAPS);
ESP_GOTO_ON_FALSE(timer, ESP_ERR_NO_MEM, err, TAG, "no mem for gptimer");
// register timer to the group (because one group can have several timers)
ESP_GOTO_ON_ERROR(gptimer_register_to_group(timer), err, TAG, "register timer failed");
gptimer_group_t *group = timer->group;
int group_id = group->group_id;
int timer_id = timer->timer_id;
// initialize HAL layer
timer_hal_init(&timer->hal, group_id, timer_id);
// stop counter, alarm, auto-reload
@@ -165,49 +203,27 @@ esp_err_t gptimer_new_timer(const gptimer_config_t *config, gptimer_handle_t *re
err:
if (timer) {
if (timer->pm_lock) {
esp_pm_lock_delete(timer->pm_lock);
}
free(timer);
}
if (group) {
gptimer_release_group_handle(group);
gptimer_destory(timer);
}
return ret;
}
esp_err_t gptimer_del_timer(gptimer_handle_t timer)
{
gptimer_group_t *group = NULL;
bool valid_state = true;
ESP_RETURN_ON_FALSE(timer, ESP_ERR_INVALID_ARG, TAG, "invalid argument");
portENTER_CRITICAL(&timer->spinlock);
if (timer->fsm != GPTIMER_FSM_STOP) {
valid_state = false;
}
portEXIT_CRITICAL(&timer->spinlock);
ESP_RETURN_ON_FALSE(valid_state, ESP_ERR_INVALID_STATE, TAG, "can't delete timer as it's not in stop state");
group = timer->group;
gptimer_group_t *group = timer->group;
int group_id = group->group_id;
int timer_id = timer->timer_id;
if (timer->intr) {
esp_intr_free(timer->intr);
ESP_LOGD(TAG, "uninstall interrupt service for timer (%d,%d)", group_id, timer_id);
}
if (timer->pm_lock) {
esp_pm_lock_delete(timer->pm_lock);
ESP_LOGD(TAG, "uninstall APB_FREQ_MAX lock for timer (%d,%d)", group_id, timer_id);
}
free(timer);
bool valid_state = true;
portENTER_CRITICAL(&timer->spinlock);
valid_state = timer->fsm == GPTIMER_FSM_STOP;
portEXIT_CRITICAL(&timer->spinlock);
ESP_RETURN_ON_FALSE(valid_state, ESP_ERR_INVALID_STATE, TAG, "can't delete timer as it's not stop yet");
ESP_LOGD(TAG, "del timer (%d,%d)", group_id, timer_id);
portENTER_CRITICAL(&group->spinlock);
group->timers[timer_id] = NULL;
portEXIT_CRITICAL(&group->spinlock);
// timer has a reference on group, release it now
gptimer_release_group_handle(group);
// recycle memory resource
ESP_RETURN_ON_ERROR(gptimer_destory(timer), TAG, "destory gptimer failed");
return ESP_OK;
}
@@ -275,18 +291,20 @@ esp_err_t gptimer_set_alarm_action(gptimer_handle_t timer, const gptimer_alarm_c
bool valid_auto_reload = !config->flags.auto_reload_on_alarm || config->alarm_count != config->reload_count;
ESP_RETURN_ON_FALSE_ISR(valid_auto_reload, ESP_ERR_INVALID_ARG, TAG, "reload count can't equal to alarm count");
portENTER_CRITICAL_SAFE(&timer->spinlock);
timer->reload_count = config->reload_count;
timer->alarm_count = config->alarm_count;
timer->flags.auto_reload_on_alarm = config->flags.auto_reload_on_alarm;
timer->flags.alarm_en = true;
portENTER_CRITICAL_SAFE(&timer->spinlock);
timer_ll_set_reload_value(timer->hal.dev, timer->timer_id, config->reload_count);
timer_ll_set_alarm_value(timer->hal.dev, timer->timer_id, config->alarm_count);
portEXIT_CRITICAL_SAFE(&timer->spinlock);
} else {
portENTER_CRITICAL_SAFE(&timer->spinlock);
timer->flags.auto_reload_on_alarm = false;
timer->flags.alarm_en = false;
portEXIT_CRITICAL_SAFE(&timer->spinlock);
}
portENTER_CRITICAL_SAFE(&timer->spinlock);

4
components/driver/include/driver/pulse_cnt.h
View File

@@ -134,6 +134,7 @@ esp_err_t pcnt_unit_set_glitch_filter(pcnt_unit_handle_t unit, const pcnt_glitch
*
* @note This function will acquire the PM lock when power management is enabled. Also see `pcnt_unit_set_glitch_filter()` for the condition of PM lock installation.
* @note The number of calls to this function should be equal to the number of calls to `pcnt_unit_stop()`.
* @note This function is allowed to run within ISR context
* @note This function will be placed into IRAM if `CONFIG_PCNT_CTRL_FUNC_IN_IRAM`, so that it's allowed to be executed when Cache is disabled
*
* @param[in] unit PCNT unit handle created by `pcnt_new_unit()`
@@ -151,6 +152,7 @@ esp_err_t pcnt_unit_start(pcnt_unit_handle_t unit);
* Also see `pcnt_unit_set_glitch_filter()` for the condition of PM lock installation.
* @note The stop operation won't clear the counter. Also see `pcnt_unit_clear_count()` for how to clear pulse count value.
* @note The number of calls to this function should be equal to the number of calls to `pcnt_unit_start()`.
* @note This function is allowed to run within ISR context
* @note This function will be placed into IRAM if `CONFIG_PCNT_CTRL_FUNC_IN_IRAM`, so that it is allowed to be executed when Cache is disabled
*
* @param[in] unit PCNT unit handle created by `pcnt_new_unit()`
@@ -165,6 +167,7 @@ esp_err_t pcnt_unit_stop(pcnt_unit_handle_t unit);
* @brief Clear PCNT pulse count value to zero
*
* @note It's recommended to call this function after adding a watch point by `pcnt_unit_add_watch_point()`, so that the newly added watch point is effective immediately.
* @note This function is allowed to run within ISR context
* @note This function will be placed into IRAM if `CONFIG_PCNT_CTRL_FUNC_IN_IRAM`, so that it's allowed to be executed when Cache is disabled
*
* @param[in] unit PCNT unit handle created by `pcnt_new_unit()`
@@ -178,6 +181,7 @@ esp_err_t pcnt_unit_clear_count(pcnt_unit_handle_t unit);
/**
* @brief Get PCNT count value
*
* @note This function is allowed to run within ISR context
* @note This function will be placed into IRAM if `CONFIG_PCNT_CTRL_FUNC_IN_IRAM`, so that it's allowed to be executed when Cache is disabled
*
* @param[in] unit PCNT unit handle created by `pcnt_new_unit()`

114
components/driver/pulse_cnt.c
View File

@@ -108,28 +108,17 @@ static pcnt_group_t *pcnt_acquire_group_handle(int group_id);
static void pcnt_release_group_handle(pcnt_group_t *group);
static void pcnt_default_isr(void *args);
esp_err_t pcnt_new_unit(const pcnt_unit_config_t *config, pcnt_unit_handle_t *ret_unit)
static esp_err_t pcnt_register_to_group(pcnt_unit_t *unit)
{
esp_err_t ret = ESP_OK;
pcnt_group_t *group = NULL;
pcnt_unit_t *unit = NULL;
int group_id = -1;
int unit_id = -1;
ESP_GOTO_ON_FALSE(config && ret_unit, ESP_ERR_INVALID_ARG, err, TAG, "invalid argument");
ESP_GOTO_ON_FALSE(config->low_limit < 0 && config->high_limit > 0 && config->low_limit >= PCNT_LL_MIN_LIN && config->high_limit <= PCNT_LL_MAX_LIM,
ESP_ERR_INVALID_ARG, err, TAG, "invalid limit range:[%d,%d]", config->low_limit, config->high_limit);
unit = heap_caps_calloc(1, sizeof(pcnt_unit_t), PCNT_MEM_ALLOC_CAPS);
ESP_GOTO_ON_FALSE(unit, ESP_ERR_NO_MEM, err, TAG, "no mem for unit");
for (int i = 0; (i < SOC_PCNT_GROUPS) && (unit_id < 0); i++) {
for (int i = 0; i < SOC_PCNT_GROUPS; i++) {
group = pcnt_acquire_group_handle(i);
ESP_GOTO_ON_FALSE(group, ESP_ERR_NO_MEM, err, TAG, "no mem for group (%d)", i);
ESP_RETURN_ON_FALSE(group, ESP_ERR_NO_MEM, TAG, "no mem for group (%d)", i);
// loop to search free unit in the group
portENTER_CRITICAL(&group->spinlock);
for (int j = 0; j < SOC_PCNT_UNITS_PER_GROUP; j++) {
if (!group->units[j]) {
group_id = i;
unit_id = j;
group->units[j] = unit;
break;
@@ -139,12 +128,63 @@ esp_err_t pcnt_new_unit(const pcnt_unit_config_t *config, pcnt_unit_handle_t *re
if (unit_id < 0) {
pcnt_release_group_handle(group);
group = NULL;
}
}
ESP_GOTO_ON_FALSE(unit_id != -1, ESP_ERR_NOT_FOUND, err, TAG, "no free unit");
} else {
unit->group = group;
unit->unit_id = unit_id;
break;
}
}
ESP_RETURN_ON_FALSE(unit_id != -1, ESP_ERR_NOT_FOUND, TAG, "no free unit");
return ESP_OK;
}
static void pcnt_unregister_from_group(pcnt_unit_t *unit)
{
pcnt_group_t *group = unit->group;
int unit_id = unit->unit_id;
portENTER_CRITICAL(&group->spinlock);
group->units[unit_id] = NULL;
portEXIT_CRITICAL(&group->spinlock);
// unit has a reference on group, release it now
pcnt_release_group_handle(group);
}
static esp_err_t pcnt_destory(pcnt_unit_t *unit)
{
if (unit->pm_lock) {
// if pcnt_unit_start and pcnt_unit_stop are not called the same number of times, deleting pm_lock will return invalid state
// which in return also reflects an invalid state of PCNT driver
ESP_RETURN_ON_ERROR(esp_pm_lock_delete(unit->pm_lock), TAG, "delete pm lock failed");
}
if (unit->intr) {
ESP_RETURN_ON_ERROR(esp_intr_free(unit->intr), TAG, "delete interrupt service failed");
}
if (unit->group) {
pcnt_unregister_from_group(unit);
}
free(unit);
return ESP_OK;
}
esp_err_t pcnt_new_unit(const pcnt_unit_config_t *config, pcnt_unit_handle_t *ret_unit)
{
#if CONFIG_PCNT_ENABLE_DEBUG_LOG
esp_log_level_set(TAG, ESP_LOG_DEBUG);
#endif
esp_err_t ret = ESP_OK;
pcnt_unit_t *unit = NULL;
ESP_GOTO_ON_FALSE(config && ret_unit, ESP_ERR_INVALID_ARG, err, TAG, "invalid argument");
ESP_GOTO_ON_FALSE(config->low_limit < 0 && config->high_limit > 0 && config->low_limit >= PCNT_LL_MIN_LIN &&
config->high_limit <= PCNT_LL_MAX_LIM, ESP_ERR_INVALID_ARG, err, TAG,
"invalid limit range:[%d,%d]", config->low_limit, config->high_limit);
unit = heap_caps_calloc(1, sizeof(pcnt_unit_t), PCNT_MEM_ALLOC_CAPS);
ESP_GOTO_ON_FALSE(unit, ESP_ERR_NO_MEM, err, TAG, "no mem for unit");
// register unit to the group (because one group can have several units)
ESP_GOTO_ON_ERROR(pcnt_register_to_group(unit), err, TAG, "register unit failed");
pcnt_group_t *group = unit->group;
int group_id = group->group_id;
int unit_id = unit->unit_id;
// some events are enabled by default, disable them all
pcnt_ll_disable_all_events(group->hal.dev, unit_id);
@@ -176,19 +216,24 @@ esp_err_t pcnt_new_unit(const pcnt_unit_config_t *config, pcnt_unit_handle_t *re
err:
if (unit) {
free(unit);
}
if (group) {
pcnt_release_group_handle(group);
pcnt_destory(unit);
}
return ret;
}
esp_err_t pcnt_del_unit(pcnt_unit_handle_t unit)
{
pcnt_group_t *group = NULL;
ESP_RETURN_ON_FALSE(unit, ESP_ERR_INVALID_ARG, TAG, "invalid argument");
ESP_RETURN_ON_FALSE(unit->fsm == PCNT_FSM_STOP, ESP_ERR_INVALID_STATE, TAG, "can't delete unit as it's not in stop state");
pcnt_group_t *group = unit->group;
int group_id = group->group_id;
int unit_id = unit->unit_id;
bool valid_state = true;
portENTER_CRITICAL(&unit->spinlock);
valid_state = unit->fsm == PCNT_FSM_STOP;
portEXIT_CRITICAL(&unit->spinlock);
ESP_RETURN_ON_FALSE(valid_state, ESP_ERR_INVALID_STATE, TAG, "can't delete unit as it's not stop yet");
for (int i = 0; i < SOC_PCNT_CHANNELS_PER_UNIT; i++) {
ESP_RETURN_ON_FALSE(!unit->channels[i], ESP_ERR_INVALID_STATE, TAG, "channel %d still in working", i);
}
@@ -196,27 +241,10 @@ esp_err_t pcnt_del_unit(pcnt_unit_handle_t unit)
ESP_RETURN_ON_FALSE(unit->watchers[i].event_id == PCNT_LL_WATCH_EVENT_INVALID, ESP_ERR_INVALID_STATE, TAG,
"watch point %d still in working", unit->watchers[i].watch_point_value);
}
group = unit->group;
int group_id = group->group_id;
int unit_id = unit->unit_id;
portENTER_CRITICAL(&group->spinlock);
group->units[unit_id] = NULL;
portEXIT_CRITICAL(&group->spinlock);
if (unit->intr) {
esp_intr_free(unit->intr);
ESP_LOGD(TAG, "uninstall interrupt service for unit (%d,%d)", group_id, unit_id);
}
if (unit->pm_lock) {
esp_pm_lock_delete(unit->pm_lock);
ESP_LOGD(TAG, "uninstall APB_FREQ_MAX lock for unit (%d,%d)", group_id, unit_id);
}
free(unit);
ESP_LOGD(TAG, "del unit (%d,%d)", group_id, unit_id);
// unit has a reference on group, release it now
pcnt_release_group_handle(group);
// recycle memory resource
ESP_RETURN_ON_ERROR(pcnt_destory(unit), TAG, "destory pcnt unit failed");
return ESP_OK;
}

14
docs/en/api-reference/peripherals/gptimer.rst
View File

@@ -237,6 +237,8 @@ When power management is enabled (i.e. :ref:`CONFIG_PM_ENABLE` is on), the syste
However, the driver can prevent the system from changing APB frequency by acquiring a power management lock of type :cpp:enumerator:`ESP_PM_APB_FREQ_MAX`. Whenever the driver creates a GPTimer instance that has selected :cpp:enumerator:`GPTIMER_CLK_SRC_APB` as its clock source, the driver will guarantee that the power management lock is acquired when the timer is started by :cpp:func:`gptimer_start`. Likewise, the driver releases the lock when :cpp:func:`gptimer_stop` is called for that timer. This requires that the :cpp:func:`gptimer_start` and :cpp:func:`gptimer_stop` should appear in pairs.
If the gptimer clock source is selected to others like :cpp:enumerator:`GPTIMER_CLK_SRC_XTAL`, then the driver won't install power management lock for it, which is more suitable for a low power application as long as the source clock can still provide sufficient resolution.
IRAM Safe
^^^^^^^^^
@@ -248,7 +250,7 @@ There's a Kconfig option :ref:`CONFIG_GPTIMER_ISR_IRAM_SAFE` that will:
2. Place all functions that used by the ISR into IRAM [2]_
3. Place driver object into DRAM (in case it's linked to PSRAM by accident)
3. Place driver object into DRAM (in case it's mapped to PSRAM by accident)
This will allow the interrupt to run while the cache is disabled but will come at the cost of increased IRAM consumption.
@@ -264,7 +266,15 @@ Thread Safety
^^^^^^^^^^^^^
The factory function :cpp:func:`gptimer_new_timer` is guaranteed to be thread safe by the driver, which means, user can call it from different RTOS tasks without protection by extra locks.
Other functions that take the :cpp:type:`gptimer_handle_t` as the first positional parameter, are not thread safe. The lifecycle of the gptimer handle is maintained by the user. So user should avoid calling them concurrently. If it has to, then one should introduce another mutex to prevent the gptimer handle being accessed concurrently.
The following functions are allowed to run under ISR context, the driver uses a critical section to prevent them being called concurrently in both task and ISR.
- :cpp:func:`gptimer_start`
- :cpp:func:`gptimer_stop`
- :cpp:func:`gptimer_get_raw_count`
- :cpp:func:`gptimer_set_raw_count`
- :cpp:func:`gptimer_set_alarm_action`
Other functions that take the :cpp:type:`gptimer_handle_t` as the first positional parameter, are not treated as thread safe. Which means the user should avoid calling them from multiple tasks.
Kconfig Options
^^^^^^^^^^^^^^^

13
docs/en/api-reference/peripherals/pcnt.rst
View File

@@ -18,7 +18,7 @@ Typically, a PCNT module can be used in scenarios like:
Functional Overview
-------------------
Description of the PCNT functionality is broken down into the following sections:
Description of the PCNT functionality is divided into into the following sections:
- `Resource Allocation <#resource-allocation>`__ - covers how to allocate PCNT units and channels with properly set of configurations. It also covers how to recycle the resources when they finished working.
@@ -215,7 +215,7 @@ There's a Kconfig option :ref:`CONFIG_PCNT_ISR_IRAM_SAFE` that will:
2. Place all functions that used by the ISR into IRAM [2]_
3. Place driver object into DRAM (in case it's linked to PSRAM by accident)
3. Place driver object into DRAM (in case it's mapped to PSRAM by accident)
This will allow the interrupt to run while the cache is disabled but will come at the cost of increased IRAM consumption.
@@ -230,7 +230,14 @@ Thread Safety
^^^^^^^^^^^^^
The factory function :cpp:func:`pcnt_new_unit` and :cpp:func:`pcnt_new_channel` are guaranteed to be thread safe by the driver, which means, user can call them from different RTOS tasks without protection by extra locks.
Other functions that take the :cpp:type:`pcnt_unit_handle_t` and :cpp:type:`pcnt_channel_handle_t` as the first positional parameter, are not thread safe. The lifecycle of the PCNT unit and channel handle is maintained by the user. So user should avoid calling them concurrently. If it has to, another mutex should be added to prevent them being accessed concurrently.
The following functions are allowed to run under ISR context, the driver uses a critical section to prevent them being called concurrently in both task and ISR.
- :cpp:func:`pcnt_unit_start`
- :cpp:func:`pcnt_unit_stop`
- :cpp:func:`pcnt_unit_clear_count`
- :cpp:func:`pcnt_unit_get_count`
Other functions that take the :cpp:type:`pcnt_unit_handle_t` and :cpp:type:`pcnt_channel_handle_t` as the first positional parameter, are not treated as thread safe. Which means the user should avoid calling them from multiple tasks.
Kconfig Options
^^^^^^^^^^^^^^^