zephyrCommon: improve delay* accurancy#163
zephyrCommon: improve delay* accurancy#163soburi wants to merge 1 commit intozephyrproject-rtos:nextfrom
Conversation
experimental results here arduino#332 (comment) On boards with different clock speed, function call overhead etc. the results may vary but will likely be consistent, given that all the boards share CONFIG_SYS_CLOCK_TICKS_PER_SEC Co-authored-by: Martino Facchin <m.facchin@arduino.cc> Signed-off-by: TOKITA Hiroshi <tokita.hiroshi@gmail.com>
There was a problem hiding this comment.
Pull request overview
This PR aims to improve the timing accuracy of the delay and delayMicroseconds functions in the Zephyr Arduino core by reducing function call overhead and compensating for inherent delays in the timing functions.
Changes:
- Added
__attribute__((always_inline))to bothdelayanddelayMicrosecondsfunctions to eliminate function call overhead - Added early return for zero-delay case in
delayMicroseconds - Modified
delayMicrosecondsto callk_busy_wait(us - 1)instead ofk_busy_wait(us)to compensate for overhead
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
DhruvaG2000
left a comment
There was a problem hiding this comment.
I think we could use some summary of the thread here: arduino#332
I am seeing that this is helping but still isn't a rootcause fix. Should we label this as a HACK just to make sure we don't mislead people into thinking the issue has been fixed for good?
I see the accuracy is still lacking from the results mentioned?
| if (us == 0) { | ||
| return; | ||
| } | ||
| k_busy_wait(us - 1); |
There was a problem hiding this comment.
Could you please add some context as to why us - 1 is helping? What's the rationale behind us -1?
There was a problem hiding this comment.
It's empirically known that with very short duration, the call overhead can exceed the waiting time, so a value of -1 is a reasonable response in the sense that it won't get worse. This is a problem that will require hacking no matter what, so it's also reasonable in that sense, but it would be ideal to set it as a Kconfig value, I think.
There was a problem hiding this comment.
it would be ideal to set it as a Kconfig value, I think.
I agree, similar to how we do kconfig for CONFIG_SYS_CLOCK_TICKS_PER_SEC let's make this into a kconfig option
There was a problem hiding this comment.
Making additional modifications will make it difficult to integrate the trees,
so I would like to ask you to merge them in their current state first.
There was a problem hiding this comment.
I disagree that we need to merge anything from forks as is. The large merge PR will also need to be cleaned up meticulously. So if you have something against adding kconfig fundamentally, we can discuss, otherwise I suggest to go ahead and make the changes that we both agree on.
There was a problem hiding this comment.
I don't disagree with adding it to kconfig, but if you think so, we should better include it in arduino/ArduinoCore-zephyr first.
As I've already said, it won't make things worse, so I don't think there's any problem with merge it.
The large merge PR will also need to be meticulously cleaned up.
My view is different. They ship it in production, and unlike us, who stop committing for one year, they maintain the development speed to support it. They'd better catch up quickly.
As someone who maintains multiple submodules, I think it would be easier to maintain if we minimized the gaps by merging and managed Zephyr-specific patches.
There was a problem hiding this comment.
My worry is how can anyone possibly review a PR as big as https://github.com/zephyrproject-rtos/ArduinoCore-zephyr/pull/164/commits
This PR is small and sizeable and it's possible to make improvements in it anyway while you're at it.
They ship it in production, and unlike us, who stop committing for one year, they maintain the development speed to support it. They'd better catch up quickly.
I agree that they have the development speed, ofcourse vendor forks move much faster in terms of features than upstream. That's the standard story across open source. Things move slowly, and take their own time.
I'd brought this up on discord too, and this is quoting from @pillo79 :
I do agree with your analysis, it's the unfortunate truth that sometimes the first thing that gets in is the 'quick and dirty' solution, with the proper one being promised "soon". We try to minimize that but... it happens. If there's the will, I think you should definitely take the time to improve the commits... everyone would benefit in the end.
This is not only about commits but also the fact that code reviews can be done upstream which is here, in a more neutral way.
If we really want to bridge gaps, we might as well have long back just pushed the arduino fork's branch here, easy and done. In fact then why even maintain 2 different forks?
As someone who maintains multiple submodules, I think it would be easier to maintain if we minimized the gaps by merging and managed Zephyr-specific patches.
I totally respect your views, and really do appreciate all the contributions you've been making all along. However I still think it's best if we are choosy about exactly what we want to bring in from the Arduino fork and what we don't want. There's no point in doing a mass reunion. It's just completely unfamiliar and unreviewed code for maintainers of this repo at that point. I am not sure how that would make things easy for us to maintain atall?
There was a problem hiding this comment.
I don't understand why we can't trust their work.
They run Zephyr-based CI instead of Arduino, just like we do, and on top of that they test their Arduino platform, so to be honest, I think their work is more reliable than ours.
In reality, we've only been able to muster the maintenance effort of "doing nothing for a year," so we think it's best to trust and accept their work so we can focus on the work we need to do, such as integrating with Zephyr itself.
experimental results here arduino#332 (comment)
On boards with different clock speed, function call overhead etc. the results may vary but will likely be consistent, given that all the boards share CONFIG_SYS_CLOCK_TICKS_PER_SEC
You may be unhappy with the format of the commit,
but please bear with us until the merge is complete.