Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix time drift - fix esp_timer_get_time - exclude some unused from compile (GIT8266O-650) #1072

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

dsptech
Copy link

@dsptech dsptech commented Mar 30, 2021

a portion of xPortSysTickHandle is rewritten to solve the time drift issue in:
#1068 (comment)

Note:

  • The main flow of the handler (performed when there is no occurred latency) runs with the better performance of (average) 150 clock cycles saved on each call.
  • When recovering, the handler further checks the evaluated values ​​(see loop) to make sure they work. This ensure against further latencies inside the handler due any nmi shot (of unknown lenght)
  • Some parts are only required by a freertos compile option. The same build condition has been added to the handler to further save resources and cpu. The purpose of g_esp_os_cpu_clk is preserved.
  • g_esp_os_ticks: seems no longer used by this sdk. Applied a conditional exclusion to save resources.
  • esp_timer_get_time: an own interrupt protection has been added with an internally stored interrupt status. This also allows you to call the function from anywhere (even inside critical sections or interrupt handlers).
  • xPortSysTickHandle contains a large comment in order to explain how it works to the reviewer. If so, feel free to take it off.

This is my first pull request, so I would be grateful for any feedback.
Thank you.

@github-actions github-actions bot changed the title fix time drift - fix esp_timer_get_time - exclude some unused from compile fix time drift - fix esp_timer_get_time - exclude some unused from compile (GIT8266O-650) Mar 30, 2021
@dsptech dsptech force-pushed the fix_timerHandler branch from f8fb767 to 39447b0 Compare April 1, 2021 18:58
@dsptech
Copy link
Author

dsptech commented Apr 3, 2021

The esp_timer_get_time previously proposed has an issue due a possible underflow in case of a frequency switch condition.
The PR has been updated to fix it. Now the maximum detectable latency is reduced to 13 secs @160MHz or 26 secs @80Mhz (the maximum I experienced: 6 secs @80 Mhz in a old version of this sdk) and the performance of the handler are further increased due the simpler check (few cpu cycles).
Please note that esp_timer_get_time is still not monotonic in case of frequency switch (like the original one from this sdk). I'm already working on it, but the solution is a work upon the above fix. So, I will wait first for a your feedback.
Thank you.
Regards.

@dsptech
Copy link
Author

dsptech commented Apr 6, 2021

As I mentioned, esp_set_cpu_freq() may cause error in the values returned from esp_timer_get_time(). These returned values may not be monotonic (dangerous).
I also fixed this. Now the time error is very small (in our tests : <1us for each esp_set_cpu_freq() call - see note in the code) and esp_timer_get_time() is now guaranteed to be monotonic.
The interrupt handler has been a bit changed (against the previous fix) to avoid a code duplication, so I would be grateful if you could ignore the previous versions.
Regards.

Closes #1068

@CLAassistant
Copy link

CLAassistant commented Jun 29, 2024

CLA assistant check
All committers have signed the CLA.

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.

2 participants