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.
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.
There was a problem hiding this comment.
It's not a matter of not trusting their work. It's about maintaining this upstream project with as much adherence to zephyr's contribution guidelines as possible.
This is a report I generated with gemini's help: (take numbers with a pinch of salt)
- 450 commits (~80%) are missing the Signed-off-by: tag, which is a critical violation of the Zephyr Project's Developer Certificate of Origin (DCO) requirement.
- 323 commits (~57%) have completely empty bodies, failing to provide any detailed description of the changes made.
We will very much land into legal / licensing issues down the line we we just cherrypick code from author's without their DCO's. Projects like Linux and Zephyr take the DCO very seriously. IANAL so I don't know the extent of the legal repurcussions of this, so I will just stick to following clear guidelines from here: https://docs.zephyrproject.org/latest/contribute/guidelines.html#dco-sign-off
We will also find it incredibly hard to bisect on most of those commits if we ever want to be able to debug and fix bugs in future that may have come from these newly merged commits.
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.
How do you think we can integrate this project with upstream zephyr if we don't adhere to some standards and quality here itself? I know it's the most long pending action item to actually be able to get this arduino core into zephyr RTOS repo, and I totally understand the urgency to do that as well. I don't understand what that has to do with us blindly pulling in 700+ commits into this repo for that?
I am not saying no to reuniting with Arduino , I am just saying that we do it in small logical chunks, be selective about what we pull in and do it all systematically. There exist programs like GSOC too which I think we should leverage for such purposes so we can share both our bandwidth accordingly. It's definitely hard for just 2 people to be able to reunite with a 700+ commit delta, so it can be done with the help of community and in multiple stages.
This and upstreaming of this arduino core into zephyr are 2 unrelated tasks that can happen parallely IMO? Let's plan this out better and do it the right way.
There was a problem hiding this comment.
OK, if you're willing to join the task, there's no reason to object.
One question: When do you plan to finish it?
There was a problem hiding this comment.
Yes indeed, I am keen on seeing this project grow too. I know that development had really slowed down for a year in between, but over the last few months I can see things picking up. I am carving out time to review and submit as well.
On the timeline question.
Since there's such a big delta between arduino fork and this repo, for just 2 people to clean up things and bring feature parity can take up to a year or more too, depends.
Meanwhile, I think the plan for beginning upstream merge into zephyr RTOS is already in progress I saw the PR from your side: zephyrproject-rtos/zephyr#96641 . Let me know how I can be of help! The next major milestone is to first start converting the non Apache stuff to rust right?
Or what plan do you have in mind?
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.