jlaitine commented on code in PR #15929: URL: https://github.com/apache/nuttx/pull/15929#discussion_r1982029384
########## arch/risc-v/src/common/riscv_mtimer.c: ########## @@ -222,8 +221,11 @@ static int riscv_mtimer_start(struct oneshot_lowerhalf_s *lower, flags = up_irq_save(); mtime = riscv_mtimer_get_mtime(priv); - alarm = mtime + ts->tv_sec * priv->freq + - ts->tv_nsec * priv->freq / NSEC_PER_SEC; + + /* Align each tick start to cycle_per_tick boundary to avoid clock drift */ + + alarm = mtime / priv->cycle_per_tick * priv->cycle_per_tick + Review Comment: This is wrong, and would add drift to the timer. The "mtime" here is not the value where the compare was set previously. It is some arbitrary time after that. The time between is unknown. The drift will eventually cause random errors in wdog functionality (e.g. sleeping for 1 tick may randomly timeout in less than 1 tick time), which then again breaks all the HW drivers using the tick based sleep as a timeout timer. This is the main reason why it is smart idea to only allow tick times which are even multiples of the underlying hw timer; you know the previous tick start by always aligning the current mtime to the tick time boundary. There is a way around this by using priv->freq as well, by specifically storing the previously set compare time, and always calculate the needed amount of ticks to progress by using the current mtime and previously set compare value.... I didn't do all of this, as I still don't understand why you can't just allow the inaccuracy in a single tick cycle time. What is the practical problem you are trying to solve?? I can, however, make a suggestion tomorrow following that path if you insist. I see that there is again changes to arm64 tick timer. I hope this time drift was not added *again* there. It is painful to debug because it causes very random failures in HW drivers due to timeout timers firing too early randomly. But again, adding this drift to the timer is a horrible mistake, which is repeated over and over again with these timers. -- 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
