xiaoxiang781216 commented on PR #17221: URL: https://github.com/apache/nuttx/pull/17221#issuecomment-3436011593
> > > Thank you @linguini1 for the PR and @xiaoxiang781216 for the hints in #17011 :-) > > > One remark here, if we revert this commit there may be a short window of time where timers will be again inaccurate until fixes come from #17011? Or is it safe / necessary to merge in order to proceed with #17011 ? > > > > > > This change should make the timers more accurate, by adding back the accurate `ndelay`. It was less accurate after this removal because the implementation exclusively used busy-waiting, which is worse. The only difference is I imagine busy-waiting has better granularity than the accurate `ndelay` on some systems where the tick is large, assuming the busy-wait is not interrupted by other tasks, etc. I think if we want to improve the accuracy of `ndelay` then its implementation needs to change to something that's not busy waiting. We could maybe introduce some `ifdef` so if the architecture doesn't have good granularity on `ndelay`, it can default back to the busy-wait implementation? Might need a mailing list discussion after this is merged. > > Ultimately I think both ways have trade-offs, but the PR #14450 only says "Fixes many issues where up_udelay is used", not really citing any drivers/functionality that broke. However, concretely, removing this accurate `ndelay` function breaks delays entirely on `sim`, so no delays are respected. > > Did you even read the summary of PR #14450 ? It doesn't "only say" what you claim, but explain the problem that the PR fixes. The ndelay_accurate was not very accurate when it quantized the delay to _systick_ length. This caused e.g. udelay(1) to delay for close to 10000 microseconds instead of busylooping for 1 microsecond, on a system with 10ms tick. For any hardware driver initialization, where one needed to wait for a few hundred nanoseconds, the udelay(1) was commontly used. With the "accurate" ndelay everything just blew up. > The accuracy lose is introduced by https://github.com/apache/nuttx/pull/15929. Before this PR, the accuracy is same as the hardware timer. > So when you revert that, please make sure that it doesn't happen again. > > I made a fix for mpfs to use architecture specific delays, which really check the time from a timer when busylooping, and that is of course the way to go. But _not_ using the current time from oneshot_current, which calculated it from the tick! For other platforms, such as arm64, the issue will be back. it's a very bad idea to add tick variant in https://github.com/apache/nuttx/pull/7033, @Fix-Point take a long time to fix this problem and provide a better solution, which summary in this year workshop, please watch it: https://www.youtube.com/watch?v=tqWwKLCD0dU&t=19710s so, it's better to revert your change to unblock other arch(e.g. sim and many other arch/chip) which never implement tick variant. @Fix-Point is preparing the final solution which provide the same accuracy as hardware and the efficient implementation. -- 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]
