Merge branch 'fix/pcnt_miss_accum_value_when_overflow' into 'master'

fix(pcnt): fix the accum_value missing when overflow

See merge request espressif/esp-idf!39967

Author: Kainarx

components/esp_driver_pcnt/README.md

@@ -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

@@ -475,10 +475,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_info.step_limit) / 2) {
+                temp_value += unit->step_info.step_limit;
+            }
+#endif
+        }
+    }
+
+    *value = temp_value + unit->accum_value;
     portEXIT_CRITICAL_SAFE(&unit->spinlock);
 
     return ESP_OK;
@@ -975,11 +1002,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_info.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;
 
@@ -998,11 +1041,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_info.step_limit;
-                    if (unit->flags.accum_count) {
-                        portENTER_CRITICAL_ISR(&unit->spinlock);
-                        unit->accum_value += unit->step_info.step_limit;
-                        portEXIT_CRITICAL_ISR(&unit->spinlock);
-                    }
                 }
 #else
             // step event has higher priority than pointer event
@@ -1027,20 +1065,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

docs/en/api-reference/peripherals/pcnt.rst

@@ -329,6 +329,10 @@ The internal hardware counter will be cleared to zero automatically when it reac
 
     :cpp:func:`pcnt_unit_clear_count` resets the accumulated count value as well.
 
+.. note::
+
+    When enabling the count overflow compensation, it is recommended to use as large a high/low count limit as possible, as it can avoid frequent interrupt triggering, improve system performance, and avoid compensation failure due to multiple overflows.
+
 .. _pcnt-power-management:
 
 Power Management

docs/zh_CN/api-reference/peripherals/pcnt.rst

@@ -329,6 +329,10 @@ PCNT 内部的硬件计数器会在计数达到高/低门限的时候自动清
 
     :cpp:func:`pcnt_unit_clear_count` 会复位该软件累加器。
 
+.. note::
+
+    启用计数溢出补偿时，建议使用尽可能大的高/低计数门限，因为它可以避免中断被频繁触发，提高系统性能，并且避免由于多次溢出而导致的补偿失败。
+
 .. _pcnt-power-management:
 
 电源管理