xiaoxiang781216 commented on code in PR #17642:
URL: https://github.com/apache/nuttx/pull/17642#discussion_r2659586591
##########
include/nuttx/hrtimer.h:
##########
@@ -97,11 +84,11 @@ struct hrtimer_node_s
struct hrtimer_s
{
- hrtimer_node_t node; /* RB-tree node for sorted insertion */
- enum hrtimer_state_e state; /* Current timer state */
- hrtimer_cb func; /* Expiration callback function */
- FAR void *arg; /* Argument passed to callback */
- uint64_t expired; /* Absolute expiration time (ns) */
+ hrtimer_node_t node; /* RB-tree node for sorted insertion */
+ hrtimer_cb func; /* Expiration callback function */
+ FAR void *arg; /* Argument passed to callback */
Review Comment:
let's use `container_of` to restore the context and remove `arg` to save the
memory?
##########
sched/hrtimer/hrtimer_start.c:
##########
@@ -27,57 +27,9 @@
#include <nuttx/config.h>
#include <nuttx/arch.h>
#include <nuttx/clock.h>
-#include <errno.h>
Review Comment:
ditto
##########
sched/hrtimer/hrtimer_start.c:
##########
@@ -141,8 +89,18 @@ int hrtimer_start(FAR hrtimer_t *hrtimer,
/* Insert the timer into the RB-tree */
- ret = hrtimer_insert(hrtimer);
+ RB_INSERT(hrtimer_tree_s, &g_hrtimer_tree, &hrtimer->node);
+
+ /* If the inserted timer is now the earliest, start hardware timer */
+
+ if (&hrtimer->node == RB_MIN(hrtimer_tree_s, &g_hrtimer_tree))
Review Comment:
can we check RB_LEFT is null
##########
sched/hrtimer/hrtimer_cancel.c:
##########
@@ -28,8 +28,7 @@
#include <nuttx/arch.h>
#include <nuttx/clock.h>
-#include <errno.h>
Review Comment:
why remove
##########
sched/hrtimer/hrtimer_cancel.c:
##########
@@ -28,8 +28,7 @@
#include <nuttx/arch.h>
#include <nuttx/clock.h>
-#include <errno.h>
-
+#include "sched/sched.h"
Review Comment:
why need
##########
include/nuttx/hrtimer.h:
##########
@@ -79,7 +66,7 @@ typedef struct hrtimer_node_s hrtimer_node_t;
* timer context and must not block.
*/
-typedef uint32_t (*hrtimer_cb)(FAR struct hrtimer_s *hrtimer);
+typedef uint32_t (*hrtimer_cb)(FAR void *arg);
Review Comment:
why change
##########
include/nuttx/hrtimer.h:
##########
@@ -97,11 +84,11 @@ struct hrtimer_node_s
struct hrtimer_s
{
- hrtimer_node_t node; /* RB-tree node for sorted insertion */
- enum hrtimer_state_e state; /* Current timer state */
- hrtimer_cb func; /* Expiration callback function */
- FAR void *arg; /* Argument passed to callback */
- uint64_t expired; /* Absolute expiration time (ns) */
+ hrtimer_node_t node; /* RB-tree node for sorted insertion */
+ hrtimer_cb func; /* Expiration callback function */
+ FAR void *arg; /* Argument passed to callback */
+ uint64_t expired; /* Absolute expiration time (ns) */
+ uint8_t cpus; /* Number of cpus that are running the timer */
Review Comment:
it's better to save the running hrtimer to global variable like:
https://github.com/apache/nuttx/blob/master/sched/wqueue/wqueue.h#L67
https://github.com/apache/nuttx/pull/17675/changes#diff-3da39fd3cc9900688d3090fe48941b8197f3730e9a2c71d34e21e1eee1eeafa7R91
##########
include/nuttx/hrtimer.h:
##########
@@ -30,10 +30,11 @@
#include <nuttx/config.h>
#include <nuttx/clock.h>
#include <nuttx/compiler.h>
-#include <nuttx/spinlock.h>
+#include <nuttx/seqlock.h>
#include <stdint.h>
#include <sys/tree.h>
+#include <errno.h>
Review Comment:
why include the unused header file
##########
sched/hrtimer/hrtimer_cancel.c:
##########
@@ -83,57 +82,31 @@
int hrtimer_cancel(FAR hrtimer_t *hrtimer)
{
FAR hrtimer_t *first;
- irqstate_t flags;
- int ret = OK;
+ irqstate_t flags;
+ int ret = OK;
DEBUGASSERT(hrtimer != NULL);
/* Enter critical section to protect the hrtimer tree and state */
- flags = spin_lock_irqsave(&g_hrtimer_spinlock);
+ flags = write_seqlock_irqsave(&g_hrtimer_spinlock);
/* Capture the current earliest timer before any modification */
first = (FAR hrtimer_t *)RB_MIN(hrtimer_tree_s, &g_hrtimer_tree);
Review Comment:
can we check left of hrtimer to avoid RB_MIN
##########
sched/hrtimer/hrtimer_set.c:
##########
@@ -0,0 +1,90 @@
+/****************************************************************************
+ * sched/hrtimer/hrtimer_set.c
+ *
+ * SPDX-License-Identifier: Apache-2.0
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership. The
+ * ASF licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
+ * License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ *
+ ****************************************************************************/
+
+/****************************************************************************
+ * Included Files
+ ****************************************************************************/
+
+#include <nuttx/config.h>
+#include <nuttx/arch.h>
+
+#include "sched/sched.h"
+#include "hrtimer/hrtimer.h"
+
+/****************************************************************************
+ * Public Functions
+ ****************************************************************************/
+
+/****************************************************************************
+ * Name: hrtimer_set
+ *
+ * Description:
+ * Set or update the callback function and argument for a high-resolution
+ * timer.
+ *
+ * This function only updates the callback context associated with the
+ * timer. It does not arm, disarm, or otherwise modify the timer's
+ * expiration state.
+ *
+ * If the timer callback is currently executing, the updated callback
+ * function will not affect the running invocation, but will be observed
+ * by any subsequent expiration.
+ *
+ * Input Parameters:
+ * hrtimer - Pointer to the high-resolution timer instance.
+ * func - Callback function to be invoked on timer expiration.
+ * arg - Argument passed to the callback function.
+ *
+ * Returned Value:
+ * None.
+ *
+ * Assumptions/Notes:
+ * - The global hrtimer spinlock protects access to the timer state.
+ * - The caller must ensure that the timer structure remains valid while
+ * the callback may still be executing.
+ * - This function is safe to call regardless of whether the timer is
+ * currently armed or inactive.
+ *
+ ****************************************************************************/
+
+int hrtimer_set(FAR hrtimer_t *hrtimer,
Review Comment:
why need? it's seldom to initialize func with the different value in the
lifetime of hrtimer
##########
sched/hrtimer/hrtimer_process.c:
##########
@@ -94,32 +106,36 @@ void hrtimer_process(uint64_t now)
RB_REMOVE(hrtimer_tree_s, &g_hrtimer_tree, &hrtimer->node);
- /* Ensure the timer callback is valid */
+ /* Increment running reference counter */
+
+ hrtimer->cpus++;
- DEBUGASSERT(hrtimer->func != NULL);
+ /* cpus is a running reference counter and must never wrap */
- hrtimer->state = HRTIMER_STATE_RUNNING;
+ DEBUGASSERT(hrtimer->cpus != 0);
- spin_unlock_irqrestore(&g_hrtimer_spinlock, flags);
+ /* Leave critical section before invoking the callback */
+
+ write_sequnlock_irqrestore(&g_hrtimer_spinlock, flags);
/* Invoke the timer callback */
- period = hrtimer->func(hrtimer);
+ period = func(arg);
- flags = spin_lock_irqsave(&g_hrtimer_spinlock);
+ /* Re-enter critical section to update timer state */
- if ((hrtimer->state == HRTIMER_STATE_CANCELED) || (period == 0))
- {
- /* Timer is canceled or one-shot; mark it inactive */
+ flags = write_seqlock_irqsave(&g_hrtimer_spinlock);
- hrtimer->state = HRTIMER_STATE_INACTIVE;
- }
- else
+ hrtimer->cpus--;
+
+ /* Re-arm periodic timer if not canceled or re-armed concurrently */
+
+ if (period > 0 && hrtimer->expired == expired)
Review Comment:
`hrtimer->expired != UINT64_MAX && hrtimer->expired != expired`
##########
include/nuttx/hrtimer.h:
##########
@@ -79,7 +66,7 @@ typedef struct hrtimer_node_s hrtimer_node_t;
* timer context and must not block.
*/
-typedef uint32_t (*hrtimer_cb)(FAR struct hrtimer_s *hrtimer);
+typedef uint32_t (*hrtimer_cb)(FAR void *arg);
Review Comment:
return value need uint64_t
##########
sched/hrtimer/hrtimer_initialize.c:
##########
@@ -39,7 +39,7 @@
* timer tree or hrtimer state is modified.
*/
-spinlock_t g_hrtimer_spinlock = SP_UNLOCKED;
+seqcount_t g_hrtimer_spinlock = RSPINLOCK_INITIALIZER;
Review Comment:
SEQLOCK_INITIALIZER
##########
sched/hrtimer/hrtimer_process.c:
##########
@@ -94,32 +106,36 @@ void hrtimer_process(uint64_t now)
RB_REMOVE(hrtimer_tree_s, &g_hrtimer_tree, &hrtimer->node);
- /* Ensure the timer callback is valid */
+ /* Increment running reference counter */
+
+ hrtimer->cpus++;
- DEBUGASSERT(hrtimer->func != NULL);
+ /* cpus is a running reference counter and must never wrap */
- hrtimer->state = HRTIMER_STATE_RUNNING;
+ DEBUGASSERT(hrtimer->cpus != 0);
- spin_unlock_irqrestore(&g_hrtimer_spinlock, flags);
+ /* Leave critical section before invoking the callback */
+
+ write_sequnlock_irqrestore(&g_hrtimer_spinlock, flags);
/* Invoke the timer callback */
- period = hrtimer->func(hrtimer);
+ period = func(arg);
Review Comment:
expired += func(arg) and remove period
##########
include/nuttx/hrtimer.h:
##########
@@ -138,9 +125,10 @@ void hrtimer_init(FAR hrtimer_t *hrtimer,
hrtimer_cb func,
FAR void *arg)
Review Comment:
let's remove arg
##########
drivers/timers/arch_alarm.c:
##########
@@ -380,7 +380,7 @@ int weak_function up_alarm_tick_cancel(FAR clock_t *ticks)
*
****************************************************************************/
-#ifdef CONFIG_SCHED_TICKLESS
+#if defined(CONFIG_HRTIMER) || defined(CONFIG_SCHED_TICKLESS)
Review Comment:
move to patch https://github.com/apache/nuttx/pull/17573
--
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]
