SuperHouse / esp-open-rtos

Open source FreeRTOS-based ESP8266 software framework
BSD 3-Clause "New" or "Revised" License
1.53k stars 491 forks source link

sntp_fun.c incorrecty computes fractional seconds #562

Closed jeffsf closed 6 years ago

jeffsf commented 6 years ago

Examination of the results of sntp_get_rtc_time(&us) in operation shows both values over 1,000,000 microseconds as well as negative values. When using the SNTP module, this breaks other time-related utilities as well.

The issue appears to be sntp_fun.c:84 which appears to fail to properly rescale the result by 4096. It appears that changing that line to

        *us = ((base * cal) % (1000000U<<12)) >>12;

resolved the range issue and a quick test shows an advance of ~0.10 seconds with a task delay of 100 ms.

Patch coming after I review the algorithm and confirm the correctness of the patch.

ourairquality commented 6 years ago

Fwiw the rtc clock appears to drift too much to be usage as a source for ntp. Guess would be that the rtc clock was intended for a low power clock when sleeping. The sntp code should probably be rewritten to use another clock source, something that does not change when the cpu clock speed changes, such as the system clock.

jeffsf commented 6 years ago

Agree that the "RTC" has too much short-term variability with sdk_system_rtc_clock_cali_proc() showing on the order of 1000-2000 ppm changes over even a minute to be a "good" source of time. Long-term drift, of course, is another issue. Even if it were used during deep sleep, I'd want to wait poll a server to get time on wake (or wait on a broadcast if/when it is supported).

I do need a sense of real time to be able to time-tag MQTT messages with QoS of 1 or greater to one second or better, especially when they may be queued for later delivery during connectivity losses. Remote syslog is another place that coordinated time is important to me.

At least I can fix what's there and have something, as imperfect as it may be, to use for "real" time.

_Edit: Having now found the source of the "magic" 2^12 factor, I'm revising some of the above. A sdk_system_rtc_clock_cali_proc() value of 24,576 (as an int) is actually 6 us per count._

RTC clock period (unit: microsecond), bit11∼ bit0 are decimal.

Running a Kalman filter on the values from sdk_system_rtc_clock_cali_proc() might provide some benefit, but I also agree that using a clock based on the crystal (ESP8266 spec is <15 ppm) makes more sense.

Edit: Looking at 1000 seconds of "cal" data, sampled roughly every 5 seconds shows ~9.4 count std. dev. or a little under 500 ppm (with a range of 24,504 - 24,552). I'll look at some filtering after I fix the core problem. It should help in environments where the temperature changes relatively slowly. Then again, the estimate of the RTC-to-us value is only as good as the system clock.