-
Notifications
You must be signed in to change notification settings - Fork 0
Replace worldtimeapi.org HTTP calls with NTP time sync #13
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
Conversation
Co-authored-by: atarukun <[email protected]>
Co-authored-by: atarukun <[email protected]>
There was a problem hiding this 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 refactors the Christmas countdown badge app to use NTP (Network Time Protocol) for time synchronization instead of fetching time from the worldtimeapi.org REST API. This simplifies the implementation and reduces dependencies.
Key changes:
- Replaces HTTP API calls to worldtimeapi.org with native NTP synchronization using the
ntptimemodule - Simplifies state management by replacing caching logic with a one-time sync approach
- Updates the code to read from the system time after NTP sync instead of parsing JSON responses
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| _last_fetch_attempt = current_ticks | ||
| # Update last sync attempt timestamp | ||
| _last_sync_attempt = current_ticks | ||
| _ntp_sync_attempt += 1 |
Copilot
AI
Oct 30, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The global variable _ntp_sync_attempt is incremented without thread-safety mechanisms. If sync_time_via_ntp() could be called from multiple contexts concurrently, this could lead to race conditions. Consider adding synchronization or documenting that this function must only be called from a single thread.
| if _ntp_synced: | ||
| return True |
Copilot
AI
Oct 30, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Once NTP sync succeeds, the function returns early and never attempts to re-sync. If the system time drifts significantly or the device runs for an extended period, the time could become inaccurate. Consider implementing periodic re-synchronization (e.g., once per day) to maintain accuracy over longer runtime periods.
| # Valid range: 2025-2100 (the badge was created in 2025) | ||
| if year < 2025 or year > 2100: |
Copilot
AI
Oct 30, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The hard-coded year range 2025-2100 will cause this code to fail in 2101. Consider making the lower bound more flexible (e.g., checking year >= 2024 to allow for some margin) or using a constant that can be easily updated. The upper bound check may be unnecessary unless there's a specific reason to distrust years beyond 2100.
| # Valid range: 2025-2100 (the badge was created in 2025) | |
| if year < 2025 or year > 2100: | |
| # Valid lower bound: badge was created in 2025, but allow margin | |
| MIN_VALID_YEAR = 2024 | |
| if year < MIN_VALID_YEAR: |
| # Update last fetch attempt timestamp before trying | ||
| _last_fetch_attempt = current_ticks | ||
| # Update last sync attempt timestamp | ||
| _last_sync_attempt = current_ticks |
Copilot
AI
Oct 30, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The global variable '_last_sync_attempt' is not used.
All worldtimeapi.org API calls were failing, causing the Christmas app to display incorrect dates from 2021 (device default on startup with no RTC persistence).
Changes
urllib.urequest+jsonwithntptimemodulentptime.settime()to sync system clock, retries every 5 seconds until successfultime.localtime()to read date after NTP sync with year validation (2025-2100)One file changed:
badge/apps/christmas/__init__.py(70 insertions, 68 deletions)Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.