Skip to content

zephyrCommon: improve delay* accurancy#163

Open
soburi wants to merge 1 commit intozephyrproject-rtos:nextfrom
soburi:improve_delay
Open

zephyrCommon: improve delay* accurancy#163
soburi wants to merge 1 commit intozephyrproject-rtos:nextfrom
soburi:improve_delay

Conversation

@soburi
Copy link
Member

@soburi soburi commented Feb 17, 2026

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.

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>
Copilot AI review requested due to automatic review settings February 17, 2026 09:05
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 both delay and delayMicroseconds functions to eliminate function call overhead
  • Added early return for zero-delay case in delayMicroseconds
  • Modified delayMicroseconds to call k_busy_wait(us - 1) instead of k_busy_wait(us) to compensate for overhead

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link
Member

@DhruvaG2000 DhruvaG2000 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please add some context as to why us - 1 is helping? What's the rationale behind us -1?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@soburi soburi Feb 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants