ppisa commented on code in PR #16491:
URL: https://github.com/apache/nuttx/pull/16491#discussion_r2150077162
##########
arch/risc-v/src/common/espressif/esp_pcnt.c:
##########
@@ -642,8 +646,34 @@ static int esp_pcnt_unit_get_count(struct cap_lowerhalf_s
*dev, int *ret)
}
flags = spin_lock_irqsave(&priv->lock);
- *ret = pcnt_ll_get_count(ctx.dev, priv->unit_id) +
- priv->accum_value;
+ tmp_count = pcnt_ll_get_count(ctx.dev, priv->unit_id);
+
+ if (priv->config.accum_count)
+ {
+ intr_status = pcnt_ll_get_intr_status(ctx.dev);
+ if (intr_status & PCNT_LL_UNIT_WATCH_EVENT(priv->unit_id))
+ {
+ event_status = pcnt_ll_get_event_status(ctx.dev, priv->unit_id);
+ while (event_status)
+ {
+ int event_id = __builtin_ffs(event_status) - 1;
+ event_status &= (event_status - 1);
+
+ if ((event_id == PCNT_LL_WATCH_EVENT_LOW_LIMIT) && \
+ (tmp_count >= (priv->config.low_limit / 2)))
+ {
+ tmp_count += priv->config.low_limit;
+ }
+ else if ((event_id == PCNT_LL_WATCH_EVENT_HIGH_LIMIT) && \
+ (tmp_count <= (priv->config.high_limit / 2)))
+ {
+ tmp_count += priv->config.high_limit;
+ }
+ }
+ }
+ }
+
+ *ret = tmp_count + priv->accum_value;
Review Comment:
@tmedicci or somebody else at Espressif, please, confirm that the function
`pcnt_ll_get_intr_status` realized by code `return hw->int_st.val` has no side
effect. What are the event status bit cleaning and setting conditions in the
hardware? It seems that they are set to match event state at the (first???)
occurrence of the interrupt. But I am not sure if the cleat is not
conditionalized by clearing of the interrupt bit or reading of the register. If
the bits are directly cleared by next event then HW is unusable when two events
are close each to other. So I expect that there could be some more complex
rule. But manual is not specific enough.
It seems that somebody reports same kind of issue as caused by ESP HAL in
NuttX even at ESP IDF
https://github.com/espressif/esp-idf/issues/10167
When I look at actual ESP IDF PCNT driver then I consider it as fatally
broken as well.
https://github.com/espressif/esp-idf/blob/master/components/esp_driver_pcnt/src/pulse_cnt.c#L473
There is time window where it is off by `low_limit` or `high_limit`. This
can be fatal for some precise control and there is no way to correct that by
calling of the ISR or another handler from the `pcnt_unit_get_count` code.
There can be race in each case, you read position and after that status and
position is before overflow and status after so the adjustment is abundant or
you read status then count and you miss overflow between status and count reads.
I see only solution to account limit of the frequency of changes and use
proposed solution or get rid of all the over-complexity and return to the
original solution with proper binary counter extensions ideally with limits
disabled (if that is possible in hardware) or with limits set to -0x4000 and
+0x4000 and guaranteed read at least twice per binary counter overflow. Such
solution has no conditionals, no branch prediction misprediction etc. But for
the actual code which offers limits which are not power of two the proposed
solution is probably only one possible.
It would worth to check if ESP PCNT support is broken in Zephyr as well.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
