23 clock sometimes stalls and resets to 2000#24
Conversation
…prove network access checks
There was a problem hiding this comment.
Pull request overview
This PR aims to prevent the clock from stalling and incorrectly resetting its displayed time (notably to year 2000) by making time-source selection/logging more stable and by hardening WiFi/NTP synchronization behavior.
Changes:
- Add centralized time-source selection helper to reduce repeated “Selected time source” logs and improve fallback messaging.
- Improve WiFi readiness checks (DHCP validity, wraparound-safe tick math) and make NTP sync more resilient (multiple servers, avoid returning a 2000 default timestamp).
- Avoid display updates when brightness is “off” and prevent display exceptions from crashing the main clock loop.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
src/lib/time_source.py |
Adds select_time_source() and tracks selected source name to avoid repeated selection logging and provide clearer fallback messages. |
src/lib/networking.py |
Adds DHCP readiness checks, wraparound-safe timing, NTP concurrency guard, NTP fallback servers, and avoids returning a “2000-01-01” timestamp on failure. |
src/lib/display.py |
Skips print_text() work when brightness is off to avoid unnecessary display I/O. |
src/lib/clock.py |
Wraps display operations in try/except to prevent display failures from stopping the clock loop. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| async def async_get_timestamp_from_ntp(self): | ||
| buf_size = 48 | ||
| ntp_request_id = 0x1b | ||
| timestamp = (2000, 1, 1, 0, 0, 0, 0, 0) | ||
| server_count = len(self.ntp_servers) | ||
|
|
There was a problem hiding this comment.
async_get_timestamp_from_ntp now returns either a gmtime() tuple or None, but the signature no longer documents that (it also dropped the previous return type). Please add an explicit return annotation (e.g., optional tuple) so callers and tooling don’t assume it always returns a timestamp.
| # Default fallback to internal RTC | ||
| self.log.error("No time source available, using potentially incorrect RTC value") | ||
| if self.external_rtc.is_configured(): | ||
| self.log.info("Selected time source: RTC") | ||
| self.time_source = self.external_rtc | ||
| self.select_time_source("RTC", self.external_rtc, "No confirmed sync source active, falling back to external RTC") | ||
| return | ||
| else: | ||
| self.log.info("Selected time source: PRTC. No external RTC available.") | ||
| self.time_source = self.rtc | ||
| self.select_time_source("PRTC", self.rtc, "No confirmed sync source active and no external RTC, falling back to internal RTC") |
There was a problem hiding this comment.
The comment # Default fallback to internal RTC is misleading: the fallback path actually prefers the external RTC when it’s configured, and only falls back to the internal RTC otherwise. Updating this comment will help future debugging of time-source selection.
| if self.check_ntp_sync_needed(): | ||
| self.log.info("Performing NTP sync from network monitor") | ||
| await self.async_sync_rtc_from_ntp() | ||
| else: | ||
| self.log.info("NTP in sync, now ensuring network access") | ||
| await self.check_network_access() | ||
| if not self.ntp_sync_in_progress: | ||
| self.log.info("Scheduling NTP sync from network monitor") | ||
| create_task(self.async_sync_rtc_from_ntp()) | ||
| else: | ||
| self.log.info("NTP sync already in progress") |
There was a problem hiding this comment.
network_monitor() schedules async_sync_rtc_from_ntp() whenever check_ntp_sync_needed() is true, even when WiFi isn’t ready. When ntp_last_synced_timestamp is 0 (startup) and the network is down, this will keep spawning NTP-sync tasks every loop iteration that immediately log an error and exit. Consider only scheduling NTP sync when status == CYW43_LINK_UP and DHCP config is valid (or add backoff) to avoid task/log churn during outages.
| while (self.get_status() != 3 or not self.has_valid_network_config()) and retries <= config.WIFI_CONNECT_RETRIES: | ||
| try: | ||
| await self.connect_wifi() |
There was a problem hiding this comment.
check_network_access() compares self.get_status() to the magic number 3 even though CYW43_LINK_UP is defined and used elsewhere in this class. Using the constant here (and in the later success check) avoids drift if the status codes ever change and improves readability.
| self.ntp_servers = ("162.159.200.1", "162.159.200.123") | ||
| self.ntp_port = 123 | ||
| self.ntp_server_index = 0 | ||
| self.ntp_address = (self.ntp_servers[self.ntp_server_index], self.ntp_port) |
There was a problem hiding this comment.
self.ntp_address is written in __init__ and updated after a successful NTP response, but it is never read anywhere in the codebase. Consider removing it (and relying on ntp_servers/ntp_server_index) or using it as the single source of truth to avoid dead state.
| self.ntp_address = (self.ntp_servers[self.ntp_server_index], self.ntp_port) | |
| self.ntp_address = (self.ntp_servers[self.ntp_server_index], self.ntp_port) | |
| self.log.debug("Initial NTP server address: %s:%d", self.ntp_address[0], self.ntp_address[1]) |
…rupted by async loop hogging on core 0 for ntp etc.
No description provided.