esp8266: Improved time keeping #2728
Conversation
- use more stable system clock as general time source;
- increase rtc accuracy and reduce drifts.
…e rtc accuracy and reduce drifts.
|
Problem reference: #2724 |
esp8266/machine_rtc.c
Outdated
| offset+= clk_ticks-clk_last_ticks; | ||
| } else { | ||
| // If overflow happened, assume 1 wrap-around and persist info for the new cycle | ||
| offset+= clk_ticks+~clk_last_ticks+1; |
There was a problem hiding this comment.
Did you see that there is system_time_high_word already defined elsewhere which handles overflow? See mp_hal_ticks_ms() for example. It would be good to reuse this counter, then probably you don't need esp_clk_XXX functions.
There was a problem hiding this comment.
I saw that one. However, I think that variable is for slightly different purposes.
The "system clock" is designed to be adjustable, i.e. if for any reason a program wants to adjust current system time, the "offset" will be amended accordingly;
The system_time_high_word is for tracking the time differences between two ref points, and adjusting system clock should not affect this counter, otherwise the result will be wrong after adjusting the system clock.
There was a problem hiding this comment.
I have made a new revision, which reuses system_time_high_word for time keeping, and just maintain an offset of "time between 2000 and last reboot" for system clock.
This does not eliminate the esp_clk_... functions, but made them a lot simpler. :D
esp8266/machine_rtc.c
Outdated
| return offset; | ||
| }; | ||
|
|
||
| void pyb_rtc_set_us_since_2000(uint64_t nowus) { |
There was a problem hiding this comment.
Please don't (unnecessarily) move the definition of these functions, it makes it hard to see exactly how they changed.
There was a problem hiding this comment.
I have moved the pyb_rtc... functions here because I would like to use the set function in mp_hal_rtc_init(), and I think having the definition before use make code reading a little easier. :)
The new pyb_rtc_... function are completely rewritten, so they have little relation to the old code. Their code logic are more like the esp_clk_... functions.
esp8266/machine_rtc.c
Outdated
| mp_obj_get_int(items[5]), | ||
| mp_obj_get_int(items[6])) * 1000 + mp_obj_get_int(items[7])) * 1000); | ||
| uint64_t arg_us = ((uint64_t)timeutils_seconds_since_2000( | ||
| mp_obj_get_int(items[0]), |
There was a problem hiding this comment.
Please keep original indentation of the lines that don't change.
|
Any news about integration of this PR?. |
|
This PR changes more things than needed, so it's hard even to review, so nobody did that so far (in full). Beyond that, it needs a lot of testing, because otherwise it may turn out that it fixes one thing and break others. So @jcea, if you've done an extended testing of this patch, please report details of it (what exactly was tested, how, for how long, etc.). |
|
Oh, and there're still code style issues. (Which always rings a bell - if there're errors is such a simple and visible matter as punctuation, what more complex, less visible errors the code may contain?) |
|
@pfalcon, I think by saying "punctuation" style issue, you really meant to say "indentation", as already fixed in eb0ad96? Or could you please be more specific on the "punctuation" you are referring to? Also, would you care to point out what unecessary code is involved in your opinion? Without specifics, it is hard to follow your hand-waving arguments to the conclusion you made. Or maybe your conclusion is simply infallible: |
|
Oh well, this is getting absurd. For the record, I have switched to Arduino esp8266 months ago, because I found the current uP implementation for esp8266 cannot meet my expectation of usability and stability. I decided to give it more time to mature before I venture back again. I kept my fork and PRs with the good will of at least save other people's effort and time I have spent on solving these problems. But clearly, it has become more of an eyesore to some maintainers. So, one less problem for you, one less problem for me. :P |
Update ports/nrf/common-hal/busio/UART.c to include MIDI baud rate of 32150
|
Closing this PR is very unfortunate as I'm hitting the same issue for a time critical application. @pfalcon is this PR still applicable? |