[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #1859: arch/sim: Add host timer and improve the oneshot timer logic

[GitBox <gi...@apache.org>]

xiaoxiang781216 commented on a change in pull request #1859:
URL: https://github.com/apache/incubator-nuttx/pull/1859#discussion_r492772747



##########
File path: arch/sim/src/sim/up_oneshot.c
##########
@@ -384,6 +450,19 @@ void up_timer_update(void)
 
   FAR sq_entry_t *entry;
 
+#ifdef CONFIG_SIM_ONESHOT_HOST_TIMER
+
+  /* If we are simulating the 'tick' events from the Idle Task
+   * we return here as we are currently using the oneshot host
+   * timer.
+   */
+
+  if (up_interrupt_context() == false)
+    {
+      return;
+    }
+#endif
+
   clock_timespec_add(&g_current, &tick, &g_current);

Review comment:
       It's better to add a new static funtion like this:
   ```
   clock_timespec_add(&g_current, &tick, &g_current);
   for (entry = sq_peek(&g_oneshot_list); entry; entry = sq_next(entry))
       {
         sim_process_tick(entry);
       }
   ```
   and let's up_timer_update and host_sigalrm_handler call it.

##########
File path: arch/sim/src/sim/up_internal.h
##########
@@ -232,6 +232,8 @@ void  host_free_shmem(void *mem);
 uint64_t host_gettime(bool rtc);
 void host_sleep(uint64_t nsec);
 void host_sleepuntil(uint64_t nsec);
+int host_setup_timer(void);

Review comment:
       How about we let host_setup_timer return(or add a pointer) the irq number?
   ```
   int host_setup_timer(int *irq);
   ```
   so we don't need host_get_alarm_signal.

##########
File path: arch/sim/src/sim/up_hosttime.c
##########
@@ -89,3 +91,42 @@ void host_sleepuntil(uint64_t nsec)
       usleep((nsec - now) / 1000);
     }
 }
+
+/****************************************************************************
+ * Name: host_setup_timer
+ *
+ * Description:
+ *   Set up a timer to send periodic signals.
+ *
+ ****************************************************************************/
+
+int host_setup_timer(void)

Review comment:
       Change to host_setimer to align with other function naming?

##########
File path: arch/sim/Kconfig
##########
@@ -509,4 +509,9 @@ config SIM_UART3_NAME
 	default "/dev/ttySIM3"
 	depends on SIM_UART_NUMBER >= 4
 
+config SIM_ONESHOT_HOST_TIMER
+	bool "Use a host timer to deliver oneshot events"
+	default n
+	depends on ONESHOT

Review comment:
       should we change to depend on SIM_WALLTIME? if SIM_WALLTIME equals n, the simulator should run as fast as possible. BTW, is it better to name this option to SIM_WALLTIME_SIGNAL and move after SIM_WALLTIME?




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org