xiaoxiang781216 commented on code in PR #17642:
URL: https://github.com/apache/nuttx/pull/17642#discussion_r2664648489
##########
sched/hrtimer/hrtimer_process.c:
##########
@@ -69,63 +70,76 @@
void hrtimer_process(uint64_t now)
{
FAR hrtimer_t *hrtimer;
- uint64_t expired;
- uint32_t period = 0;
irqstate_t flags;
+ hrtimer_cb func;
+ uint64_t expired;
+ uint64_t period = 0;
+ int cpu = this_cpu();
/* Lock the hrtimer RB-tree to protect access */
flags = spin_lock_irqsave(&g_hrtimer_spinlock);
/* Fetch the earliest active timer */
- hrtimer = (FAR hrtimer_t *)RB_MIN(hrtimer_tree_s, &g_hrtimer_tree);
+ hrtimer = hrtimer_get_first();
while (hrtimer != NULL)
{
+ func = hrtimer->func;
+
+ /* Ensure the timer callback is valid */
+
+ DEBUGASSERT(func != NULL);
+
+ expired = hrtimer->expired;
+
/* Check if the timer has expired */
- if (!clock_compare(hrtimer->expired, now))
+ if (!clock_compare(expired, now))
{
break;
}
/* Remove the expired timer from the active tree */
- RB_REMOVE(hrtimer_tree_s, &g_hrtimer_tree, &hrtimer->node);
+ hrtimer_remove(hrtimer);
- /* Ensure the timer callback is valid */
-
- DEBUGASSERT(hrtimer->func != NULL);
+ g_hrtimer_running[cpu] = hrtimer;
- hrtimer->state = HRTIMER_STATE_RUNNING;
+ /* Leave critical section before invoking the callback */
spin_unlock_irqrestore(&g_hrtimer_spinlock, flags);
/* Invoke the timer callback */
- period = hrtimer->func(hrtimer);
+ period = func(hrtimer, expired);
+
+ /* Re-enter critical section to update timer state */
flags = spin_lock_irqsave(&g_hrtimer_spinlock);
- if ((hrtimer->state == HRTIMER_STATE_CANCELED) || (period == 0))
- {
- /* Timer is canceled or one-shot; mark it inactive */
+ g_hrtimer_running[cpu] = NULL;
- hrtimer->state = HRTIMER_STATE_INACTIVE;
- }
- else
+ /* If the timer is periodic and has not been rearmed or
+ * cancelled concurrently,
+ * compute next expiration and reinsert into RB-tree
+ */
+
+ if (period > 0 && hrtimer->expired == expired)
{
- /* Restart the periodic timer */
+ hrtimer->expired = expired + period;
+
+ /* Ensure no overflow occurs */
+
+ DEBUGASSERT(hrtimer->expired != UINT64_MAX);
Review Comment:
add DEBUGASSERT(hrtimer->expired > period);
##########
sched/hrtimer/hrtimer_start.c:
##########
@@ -139,10 +88,24 @@ int hrtimer_start(FAR hrtimer_t *hrtimer,
hrtimer->expired = hrtimer_gettime() + ns;
}
+ /* Ensure expiration time does not overflow */
+
+ DEBUGASSERT(hrtimer->expired != UINT64_MAX);
Review Comment:
let's add `DEBUGASSERT(hrtimer->expired >= ns);` to catch up the wrap issue
##########
include/nuttx/hrtimer.h:
##########
Review Comment:
```suggestion
uint64_t expired,
```
##########
sched/hrtimer/hrtimer_process.c:
##########
@@ -69,63 +70,76 @@
void hrtimer_process(uint64_t now)
{
FAR hrtimer_t *hrtimer;
- uint64_t expired;
- uint32_t period = 0;
irqstate_t flags;
+ hrtimer_cb func;
+ uint64_t expired;
+ uint64_t period = 0;
+ int cpu = this_cpu();
/* Lock the hrtimer RB-tree to protect access */
flags = spin_lock_irqsave(&g_hrtimer_spinlock);
/* Fetch the earliest active timer */
- hrtimer = (FAR hrtimer_t *)RB_MIN(hrtimer_tree_s, &g_hrtimer_tree);
+ hrtimer = hrtimer_get_first();
while (hrtimer != NULL)
{
+ func = hrtimer->func;
+
+ /* Ensure the timer callback is valid */
+
+ DEBUGASSERT(func != NULL);
+
+ expired = hrtimer->expired;
+
/* Check if the timer has expired */
- if (!clock_compare(hrtimer->expired, now))
+ if (!clock_compare(expired, now))
{
break;
}
/* Remove the expired timer from the active tree */
- RB_REMOVE(hrtimer_tree_s, &g_hrtimer_tree, &hrtimer->node);
+ hrtimer_remove(hrtimer);
- /* Ensure the timer callback is valid */
-
- DEBUGASSERT(hrtimer->func != NULL);
+ g_hrtimer_running[cpu] = hrtimer;
- hrtimer->state = HRTIMER_STATE_RUNNING;
+ /* Leave critical section before invoking the callback */
spin_unlock_irqrestore(&g_hrtimer_spinlock, flags);
/* Invoke the timer callback */
- period = hrtimer->func(hrtimer);
+ period = func(hrtimer, expired);
+
+ /* Re-enter critical section to update timer state */
flags = spin_lock_irqsave(&g_hrtimer_spinlock);
- if ((hrtimer->state == HRTIMER_STATE_CANCELED) || (period == 0))
- {
- /* Timer is canceled or one-shot; mark it inactive */
+ g_hrtimer_running[cpu] = NULL;
- hrtimer->state = HRTIMER_STATE_INACTIVE;
- }
- else
+ /* If the timer is periodic and has not been rearmed or
+ * cancelled concurrently,
+ * compute next expiration and reinsert into RB-tree
+ */
+
+ if (period > 0 && hrtimer->expired == expired)
{
- /* Restart the periodic timer */
+ hrtimer->expired = expired + period;
Review Comment:
hrtimer->expired += period
##########
sched/hrtimer/hrtimer_start.c:
##########
Review Comment:
change ns to expired
##########
include/nuttx/hrtimer.h:
##########
Review Comment:
expired
##########
sched/hrtimer/hrtimer_process.c:
##########
@@ -69,63 +70,76 @@
void hrtimer_process(uint64_t now)
{
FAR hrtimer_t *hrtimer;
- uint64_t expired;
- uint32_t period = 0;
irqstate_t flags;
+ hrtimer_cb func;
+ uint64_t expired;
+ uint64_t period = 0;
Review Comment:
```suggestion
uint64_t period;
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
