alex/esp-idf
1
0
Fork 0
mirror of https://github.com/espressif/esp-idf.git synced 2025-10-17 09:14:04 +00:00
Code Issues Packages Projects Releases Wiki Activity

Merge branch 'fix/pcnt_miss_accum_value_when_overflow_v5.1' into 'release/v5.1'

Browse Source 
fix(pcnt): fix the accum_value missing when overflow (v5.1)

See merge request espressif/esp-idf!40318
This commit is contained in:
morris
2025-07-08 11:14:07 +08:00
parent 517b018dfa ec5a0c0097
commit 4460844155
4 changed files with 87 additions and 14 deletions
Download Patch File Download Diff File Expand all files Collapse all files

39
components/driver/pcnt/README.md Normal file
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.

54
components/driver/pcnt/pulse_cnt.c
View File

@@ -1,5 +1,5 @@
/*
* SPDX-FileCopyrightText: 2021-2023 Espressif Systems (Shanghai) CO LTD
* SPDX-FileCopyrightText: 2021-2025 Espressif Systems (Shanghai) CO LTD
*
* SPDX-License-Identifier: Apache-2.0
*/
@@ -404,10 +404,32 @@ 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;
}
}
}
*value = temp_value + unit->accum_value;
portEXIT_CRITICAL_SAFE(&unit->spinlock);
return ESP_OK;
@@ -773,23 +795,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);
// iter on each event_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;
}
}
portEXIT_CRITICAL_ISR(&unit->spinlock);
// using while loop so that we don't miss any event
while (event_status) {
int event_id = __builtin_ffs(event_status) - 1;
event_status &= (event_status - 1); // clear the right most bit
portENTER_CRITICAL_ISR(&unit->spinlock);
if (unit->flags.accum_count) {
if (event_id == PCNT_LL_WATCH_EVENT_LOW_LIMIT) {
unit->accum_value += unit->low_limit;
} else if (event_id == PCNT_LL_WATCH_EVENT_HIGH_LIMIT) {
unit->accum_value += unit->high_limit;
}
}
portEXIT_CRITICAL_ISR(&unit->spinlock);
// invoked user registered callback
if (on_reach) {
pcnt_watch_event_data_t edata = {