linguini1 commented on PR #17221: URL: https://github.com/apache/nuttx/pull/17221#issuecomment-3436717187
I don't think we should revert #15929 based on what @jlaitine has said. Sleeping for a tick too long is one thing, but delays should never return *before* the delay is over. I would be willing to do an architecture-specific implementation of `up_delay` for `sim`, but I guess I'm still concerned that if `sim` has been broken by this change for all this time, I wonder if anything else broke and has been unnoticed. Anyways, to give context on the bigger picture for why I'm reverting this: 1. CONFIG_BOARD_LOOPSPERMSEC controls all the busy wait delays, which before #17204 were pretty much everywhere in the code 2. CONFIG_BOARD_LOOPSPERMSEC had some small default value, so users who forgot to calibrate their board and pick a sane value (or users who didn't know the option existed) would have very incorrect delay times 3. I introduced PR #17011 to have an invalid default value that would get caught at compile time, so the user is always notified to calibrate their board 4. Some boards *shouldn't* need this option at all (like sim setting it to 0) since they have timer-based delay implementations which are better 5. Therefore, I want to put back the timer-based delay in `arch_alarm.c` since those architectures should be able to have a delay which isn't busy-wait based (since busy-waiting is bad performance-wise and accuracy-wise). This narrows the amount of architectures relying on this bad busy-loop, without needing lots of unique implementations per-architecture. So ideally, there would be a way to modify the delay in `arch_alarm.c` so that it's timer-based and is better than the flawed old version of `ndelay_accurate` you removed before. I believe this is the new method that Xiang Xiao is talking about, but it's not upstreamed just yet. In the meantime, I put back the old way to fix sim (and possibly other architectures) to get closer to the goal of removing reliance on the busy-wait. I think drivers using `up_delay` functions should not be sensitive to the delay being longer than anticipated (within reason), but having an implementation that is more accurate would improve performance. That's I guess on hold until this new fix Xiang mentioned is upstreamed. TLDR; the busy-wait is bad and prone to user-error. The old method sleeps longer than it should and isn't great, but it fixes things and a new method is coming soon. Delay functions should never wake up early, but can wake up late. -- 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]
