Mirrors/esp-idf
1
0
Fork 0
mirror of https://github.com/espressif/esp-idf.git synced 2026-10-01 18:50:34 +03:00
Code Issues Packages Projects Releases Wiki Activity

fix(pcnt): fix the accum_value missing when overflow

Browse Source
This commit is contained in:
Chen Jichang
2025-07-02 20:02:23 +08:00
parent 3e5d2c7d58
commit 2dcfc31c05
4 changed files with 93 additions and 18 deletions
Download Patch File Download Diff File Expand all files Collapse all files
components/esp_driver_pcnt/README.md
+39
View File
@@ -0,0 +1,39 @@
# PCNT Driver Design
## Concurrency
The count value and the overflow state of the count value are located in *different* registers, resulting in the software being unable to obtain information from both of them in the same read instruction.
The race condition case is as follow:
```mermaid
sequenceDiagram
participant HW as PCNT Hardware
participant CPU0_ISR as CPU0_ISR
participant CPU1_Task as CPU1_Task (pcnt_unit_get_count)
participant REG as Reg and Soft accum counter State
CPU1_Task->>CPU1_Task: Call pcnt_unit_get_count()
Note over REG: intr_status = 0<br/>cnt_reg = cnt_value<br/>accum_value = old_value
CPU1_Task->>CPU1_Task: portENTER_CRITICAL_SAFE()
CPU1_Task->>REG: Read intr_status
Note over CPU1_Task: intr_status=0, no need to do compensation
HW->>REG: Overflow interrupt triggered
Note over REG: intr_status = 1<br/>cnt_reg = 0<br/>accum_value = old_value
REG->>CPU0_ISR: ISR is called
CPU0_ISR->>CPU0_ISR: try portENTER_CRITICAL_SAFE() but spin
CPU1_Task->>REG: Read cnt_reg(0) + accum_value(old)
CPU1_Task->>CPU1_Task: portEXIT_CRITICAL_SAFE()
CPU0_ISR->>CPU0_ISR: portENTER_CRITICAL_SAFE()
CPU0_ISR->>REG: Clear interrupt status and update accum_value
Note over REG: intr_status = 0<br/>accum_value = new_value
CPU0_ISR->>CPU0_ISR: portEXIT_CRITICAL_SAFE()
Note over CPU0_ISR: Process events
Note over CPU1_Task: Return incorrect count ❌
```
In the software, we determine whether to perform compensation by checking whether the count value exceeds half of the limit. This can prevent counting errors when the overflow frequency is not high.
components/esp_driver_pcnt/src/pulse_cnt.c
+46 -18
View File
@@ -497,10 +497,37 @@ esp_err_t pcnt_unit_get_count(pcnt_unit_handle_t unit, int *value)
pcnt_group_t *group = NULL;
ESP_RETURN_ON_FALSE_ISR(unit && value, ESP_ERR_INVALID_ARG, TAG, "invalid argument");
group = unit->group;
int temp_value = 0;
// the accum_value is also accessed by the ISR, so adding a critical section
portENTER_CRITICAL_SAFE(&unit->spinlock);
*value = pcnt_ll_get_count(group->hal.dev, unit->unit_id) + unit->accum_value;
temp_value = pcnt_ll_get_count(group->hal.dev, unit->unit_id) ;
// Check for pending overflow interrupts that haven't been processed yet
// Add compensation to get accurate count
if (unit->flags.accum_count) {
uint32_t intr_status = pcnt_ll_get_intr_status(group->hal.dev);
if (intr_status & PCNT_LL_UNIT_WATCH_EVENT(unit->unit_id)) {
uint32_t event_status = pcnt_ll_get_event_status(group->hal.dev, unit->unit_id);
// TODO: DIG-683
// Note, the overflow may be triggered between `pcnt_ll_get_count` and `pcnt_ll_get_event_status`
// In this case, we don't want to do the compensation.
// so we should check the count value is greater(less) than the low(high) limit / 2 to filter this case.
// This workaround is only valid for the case that the counter won't overflow twice between `pcnt_ll_get_count()` and `pcnt_ll_get_intr_status()`
if (event_status & BIT(PCNT_LL_WATCH_EVENT_LOW_LIMIT) && temp_value >= unit->low_limit / 2) {
temp_value += unit->low_limit;
} else if (event_status & BIT(PCNT_LL_WATCH_EVENT_HIGH_LIMIT) && temp_value <= unit->high_limit / 2) {
temp_value += unit->high_limit;
}
#if SOC_PCNT_SUPPORT_STEP_NOTIFY && PCNT_LL_STEP_NOTIFY_DIR_LIMIT
else if (event_status & BIT(PCNT_LL_STEP_EVENT_REACH_LIMIT) && abs(temp_value) <= abs(unit->step_limit) / 2) {
temp_value += unit->step_limit;
}
#endif
}
}
*value = temp_value + unit->accum_value;
portEXIT_CRITICAL_SAFE(&unit->spinlock);
return ESP_OK;
@@ -951,11 +978,27 @@ IRAM_ATTR static void pcnt_default_isr(void *args)
uint32_t intr_status = pcnt_ll_get_intr_status(group->hal.dev);
if (intr_status & PCNT_LL_UNIT_WATCH_EVENT(unit_id)) {
pcnt_ll_clear_intr_status(group->hal.dev, PCNT_LL_UNIT_WATCH_EVENT(unit_id));
// event status word contains information about the real watch event type
uint32_t event_status = pcnt_ll_get_event_status(group->hal.dev, unit_id);
// clear interrupt status and update accum_value atomically
portENTER_CRITICAL_ISR(&unit->spinlock);
pcnt_ll_clear_intr_status(group->hal.dev, PCNT_LL_UNIT_WATCH_EVENT(unit_id));
if (unit->flags.accum_count) {
if (event_status & BIT(PCNT_LL_WATCH_EVENT_LOW_LIMIT)) {
unit->accum_value += unit->low_limit;
} else if (event_status & BIT(PCNT_LL_WATCH_EVENT_HIGH_LIMIT)) {
unit->accum_value += unit->high_limit;
}
#if SOC_PCNT_SUPPORT_STEP_NOTIFY && PCNT_LL_STEP_NOTIFY_DIR_LIMIT
else if (event_status & BIT(PCNT_LL_STEP_EVENT_REACH_LIMIT)) {
unit->accum_value += unit->step_limit;
}
#endif
}
portEXIT_CRITICAL_ISR(&unit->spinlock);
int count_value = 0;
pcnt_unit_zero_cross_mode_t zc_mode = PCNT_UNIT_ZERO_CROSS_INVALID;
@@ -973,11 +1016,6 @@ IRAM_ATTR static void pcnt_default_isr(void *args)
event_status &= ~BIT(PCNT_LL_STEP_EVENT_REACH_LIMIT);
// adjust current count value to the step limit
count_value = unit->step_limit;
if (unit->flags.accum_count) {
portENTER_CRITICAL_ISR(&unit->spinlock);
unit->accum_value += unit->step_limit;
portEXIT_CRITICAL_ISR(&unit->spinlock);
}
}
// step event may happen with other pointer event at the same time, we don't need to process them again
event_status &= ~(BIT(PCNT_LL_WATCH_EVENT_LOW_LIMIT) | BIT(PCNT_LL_WATCH_EVENT_HIGH_LIMIT) |
@@ -988,20 +1026,10 @@ IRAM_ATTR static void pcnt_default_isr(void *args)
event_status &= ~(BIT(PCNT_LL_WATCH_EVENT_LOW_LIMIT));
// adjust the count value to reflect the actual watch point value
count_value = unit->low_limit;
if (unit->flags.accum_count) {
portENTER_CRITICAL_ISR(&unit->spinlock);
unit->accum_value += unit->low_limit;
portEXIT_CRITICAL_ISR(&unit->spinlock);
}
} else if (event_status & BIT(PCNT_LL_WATCH_EVENT_HIGH_LIMIT)) {
event_status &= ~(BIT(PCNT_LL_WATCH_EVENT_HIGH_LIMIT));
// adjust the count value to reflect the actual watch point value
count_value = unit->high_limit;
if (unit->flags.accum_count) {
portENTER_CRITICAL_ISR(&unit->spinlock);
unit->accum_value += unit->high_limit;
portEXIT_CRITICAL_ISR(&unit->spinlock);
}
} else if (event_status & BIT(PCNT_LL_WATCH_EVENT_THRES0)) {
event_status &= ~(BIT(PCNT_LL_WATCH_EVENT_THRES0));
// adjust the count value to reflect the actual watch point value