ppisa commented on code in PR #16491:
URL: https://github.com/apache/nuttx/pull/16491#discussion_r2155614642
##########
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:
> Then, we shouldn't deal with the accumulator in different contexts: this
is solely a task for the ISR, and that's why I proposed to
https://github.com/apache/nuttx/pull/16491#discussion_r2150694039 at
esp_pcnt_unit_get_count during the critical section: if we have an interrupt to
be serviced (that will update the accumulator value), then we should exit the
critical section and let it to run.
We do not propose to manipulate with accumulator anywhere else than in the
interrupt
> The problem could only happen if we have a condition where the overflow
happens too frequently
No, the problem is caused even by single increment and overflow after
critical section is `esp_pcnt_unit_get_count` is entered and before PCNT count
register is entered.
> Another (complementary) approach would be to save the last position read
(counter + accumulator) and if nothing has changed in hardware (operation mode,
high/low limits etc), check for consistency of the read value during the
critical section (if the PCNT is configured to increase the counter and the
read value is lower than the last one and nothing changed, it means it
overflowed and we need to wait for the ISR to updated the accumulator value).
Would not solve problem for incremental encoder, because it requires to
count up and down, so two consecutive reads could have any bigger, less
relation.
There is alternative, which would work on single core system, it is the
loading and remembering the accumulator, then read position without interrupt
disabled and then read the accumulator again. If there is interrupt in between
then interrupt will update the accumulated and we see the change in accumulator
and if the previously read counter values is between `priv->config.low_limit /
2` and `priv->config.high_limit / 2` then we know that we should use the second
read of accumulator and if it is outside (bigger in absolute value) then the
first read should be added.
But that would not work on SMP system, because there is not guaranteed that
pending and accepted interrupt preempts driver context code execution. So
again, the code which reads `pcnt_ll_get_intr_status` is much better solution.
I have only some doubts about event register read behavior because there is
missing (in the manual) specification if there is or is not side effect of the
read and if the event are somehow accumulated in the bits or cleared
immediately.
Anyway, there seems to be serious/fatal problem even in ESP-IDF. I plan to
talk with people in Brino anyway, so we can discuss that with @igrr and other
people and I hope that they can help to check/clarify chip design logic for my
consideration of corner cases.
I apologize to @Vajnar , that it takes so much of his time. I consider that
as really unnecessary and it is shame, that we have working solution in NuttX
mainline half year ago and it is broken for long time unnecessarily now. I need
to check even another student group who got the platform for experiments with
two wheel platform what is they state and if they suffer for months
unnecessarily...
--
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
