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 bug in SNTP synchronization #22

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

cmauri
Copy link

@cmauri cmauri commented Jul 17, 2024

Correct the internal cache update to use the proper uptime reference. Previously, an incorrect reference introduced a minor error (a few milliseconds or less). In cases where NTP requests timed out, this error could increase significantly, reaching up to 10 seconds (the default timeout value).

Correct the internal cache update to use the proper uptime reference.
Previously, an incorrect reference introduced a minor error (a few
milliseconds or less). In cases where NTP requests timed out, this
error could increase significantly, reaching up to 10 seconds
(the default timeout value).
@AllanHasegawa
Copy link
Owner

Hey, thanks for the PR.

I'm completely sure I understand the issue here. Do you have an example where this was an issue? Maybe the events can help here?

A timeout will not be marked as a successful retrieval of the NTP time, therefore, it shouldn't impact or cause any side-effect in the cache.

Also, the algorithm is designed to always report a time that is usually different than the device's time as it's doing it's best to not rely fully on the device's time, nor solely on the NTP time, but it's trying to balance both.

@cmauri
Copy link
Author

cmauri commented Jul 18, 2024

Hey! You can easily reproduce the bug in the emulator like this:

  • Open the Android emulator
  • Disable WiFi leaving only cellular data connection
  • In emulator Extended controls > Cellular set "Network type" to GPRS or EDGE and "Signal strength" to "Poor"
  • Start the sample application
  • Reject localization permissions (to avoid using GPS sync)
  • Wait for a sync in which some NTP messages timed out (you may need to add some log messages if necessary to make sure)
  • Retry pressing "Stop" and "Start" if necessary.
  • The clock should display an error of ~10 seconds or more.

Rationale: the uptime used to update the cache was retrieved just before performing the update and NOT when received the sync response. When no timeouts this just introduces a small error, when timeouts and the picked response if BEFORE one timeout, the pair (systemCurrentMillis, uptime) is pointing a timeout duration (10s by default) in the past.

Hope this makes sense.

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