tmedicci commented on code in PR #16491:
URL: https://github.com/apache/nuttx/pull/16491#discussion_r2135722484
##########
arch/risc-v/src/common/espressif/esp_pcnt.c:
##########
Review Comment:
One step back: let me try to understand the issue first. If I understood
correctly, the issue we are trying to solve here is "overflow interrupts not
being handled due to interrupts being disabled (either because we are in a
critical section or because we are on ISR)", right?
The second question is: 1) are we taking too long on current PCNT's ISR? (in
such a way that we can't even treat other events) or 2) is the overall system
(other interrupt sources) overloaded?
If `1)`, which function is being registered with `CAPIOC_HANDLER`? (This
function runs during ISR, so it should not take too long to execute). Still
considering this case, wouldn't setting the thresholds with a higher range
solve the issue?
##########
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:
I totally agree with protecting the whole loop on `esp_pcnt_isr_default`
with the spinlock, but I'm not sure if this approach is correct: if
`pcnt_ll_get_intr_status` has any corresponding bits set, it should be treated
on ISR (should have been triggered there), messing with it outside the ISR may
lead to unknown behavior (I'm checking how those bits behave when the interrupt
is not triggered because it's disabled).
--
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
