Skip to content

Fix long-uptime clock display bugs (colon stalls, stray PM/seconds dot)#330

Closed
Bullpuph123 wants to merge 3 commits into
brettdottech:devfrom
Bullpuph123:main
Closed

Fix long-uptime clock display bugs (colon stalls, stray PM/seconds dot)#330
Bullpuph123 wants to merge 3 commits into
brettdottech:devfrom
Bullpuph123:main

Conversation

@Bullpuph123

@Bullpuph123 Bullpuph123 commented Jul 5, 2026

Copy link
Copy Markdown

Summary

  • Fixes three ClockWidget symptoms that only appear after long uptime and clear on reboot: irregular colon blinking, the PM indicator showing up on a digit orb, and a stray seconds dot on a digit orb.
  • GlobalTime: the 1s scheduler reset itself with m_updateTimer = millis() after a > 1000ms check, so its period drifted a few ms per cycle relative to the NTP epoch's exact 1000ms grid (same millis() clock). Each time the drift crossed a second boundary, a second was skipped — and since the colon toggled on seconds parity, it froze for 2+s at irregular intervals. Now advances by whole periods (rollover-safe, drift-free) so seconds are never skipped.
  • ClockWidget: colon now blinks on a dedicated 500ms millis()-based timer (stable 1s cycle) instead of seconds parity; AM/PM is only redrawn when it changes, eliminating the per-second TTF font unload/reload cycle (~86k FreeType heap alloc cycles/day) that fragmented the heap over multi-day uptimes; middle/status orb ownership of colon/seconds tick/AM-PM is explicit with bounds guards on orb indexes and seconds values; update() uses rollover-safe unsigned elapsed-time math and avoids per-loop String allocations.
  • ScreenManager: selectScreen() deselects all CS lines before asserting the target and validates the index, so two panels can never be briefly selected during a transition (stray pixels latched into adjacent orbs).
  • Adds a host-side simulation (firmware/test/host_sim_long_uptime.py) that replicates the exact uint32 arithmetic: the old scheduler skips 53 seconds in 3 simulated hours; the new logic shows 0 skips including across the 49.7-day millis() rollover, holds a 500ms colon cadence with 2ms cumulative drift over 24h, and keeps all seconds-dot draws bounded to the middle orb.

Test plan

  • Host simulation: python firmware/test/host_sim_long_uptime.py (all checks pass, incl. rollover boundary and blocked-loop catch-up)
  • pio run -e esp32doit-devkit-v1 builds clean (no new warnings)
  • Flashed to ESP32 hardware; verified normal boot, WiFi reconnect, and stable operation
  • Long-run soak on hardware across multiple days

🤖 Generated with Claude Code

Bullpuph123 and others added 2 commits July 4, 2026 23:17
- GlobalTime: drift-free rollover-safe 1s scheduler (advance by whole
  periods instead of resetting to millis()), so displayed seconds are
  never skipped due to accumulated sampling drift
- ClockWidget: colon now blinks on a dedicated 500ms millis()-based
  timer instead of seconds parity; AM/PM only redrawn on change to stop
  the per-second TTF font reload cycle that fragmented the heap over
  days; middle/status orb ownership of colon/ticks/AM-PM made explicit
  with bounds guards on orb indexes and seconds values
- ScreenManager: deselect all CS lines before selecting the target
  screen and validate the screen index, so draws can never land on an
  adjacent orb
- Add host-side simulation (firmware/test/host_sim_long_uptime.py)
  covering millis() rollover, blocked loops and 24h blink drift

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
After hours of uptime the colon degraded to 1s on / 1s off: when the
NTP server stops responding, NTPClient::forceUpdate() blocks ~1010ms
and never advances _lastUpdate on failure, so NTPClient::update()
re-attempts the blocking call on every invocation. With GlobalTime
ticking once per second, the whole render loop degraded to ~1
iteration/second, and the colon (one toggle max per draw) blinked at
half rate. A reboot picked a responsive server and masked the bug.

- GlobalTime: gate NTP attempts to once per 61s (5s until first sync),
  tracked regardless of success, and log failed attempts; timekeeping
  continues via millis() between syncs
- ClockWidget: colon catch-up now toggles by period parity so loop
  stalls cannot invert the blink phase
- Host sim: new check reproduces the old failure (median toggle
  ~1010ms) and verifies zero phase errors with the fix

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@Bullpuph123

Copy link
Copy Markdown
Author

Follow-up fix pushed (b8becdf): during on-hardware soak testing, the colon blink degraded from 500ms to 1s on / 1s off after several hours of uptime — a new reproduction of the original "works after reboot, degrades over time" pattern, this time with a different root cause.

Root cause: when the NTP server stops responding (pool rotation, router NAT timeout, etc.), NTPClient::forceUpdate() blocks for ~1010ms waiting on the UDP reply and returns without advancing _lastUpdate on failure. NTPClient::update() therefore re-attempts the blocking call on every invocation once a refresh is due — and GlobalTime::updateTime() calls it once per second. The entire render loop degrades to ~1 iteration/second, so the colon (max one toggle per draw) blinks at exactly half rate. Timekeeping continues via millis(), so the digits and seconds dot stay correct — only the colon shows the fault. A reboot picks a responsive server via fresh DNS and masks the bug.

Changes:

  • GlobalTime: NTP attempts are now gated to once per 61s (5s until the first successful sync), with the attempt timestamp recorded regardless of outcome. A dead NTP server now costs at most one ~1s hiccup per minute instead of one per second, and failures are logged to serial.
  • ClockWidget: the colon catch-up toggles by elapsed-period parity, so a stalled loop can never invert the blink phase — it stays locked to the 500ms grid.
  • Host simulation: new check reproduces the failure with the old code (median toggle interval ~1010ms) and verifies zero phase errors over 2 simulated hours with the fix.

Verified on hardware: builds clean, flashed, normal boot with WiFi/timezone intact, stable 500ms blink.

…ISRs

- Gate getTimeZoneOffsetFromAPI() attempts (30s until first success, 5min
  after a failed refresh): it is a blocking HTTP call and timezonedb
  rate-limits to ~1 req/s, so retrying every 1s tick stalled the render
  loop and guaranteed rate-limit failures at boot and DST changeover.
- Initialize the loop index in WidgetSet::updateAll() (was reading an
  uninitialized int8_t - undefined behavior on every boot).
- Add IRAM_ATTR to the button ISR wrappers and Button::isrButtonChange()
  so a button edge during a flash write cannot crash with "Cache disabled
  but cached memory region accessed".

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@Bullpuph123

Copy link
Copy Markdown
Author

Follow-up commit 0c4fbf1 fixes three more interaction bugs found while reviewing how the event loops interact:

1. Timezone API retry storm (same class as the NTP bug above)
GlobalTime::updateTime() called getTimeZoneOffsetFromAPI() on every 1-second tick whenever m_timeZoneOffset == -1 (boot, until the first successful fetch) or when the epoch passed m_nextTimeZoneUpdate (DST changeover). The call is a blocking HTTP request (up to ~5s on an unreachable server), and timezonedb rate-limits free keys to ~1 req/s — so hammering it once per second both stalls the render loop and makes the requests likely to fail. Attempts are now gated: retry every 30s until the first success, then every 5 minutes if a scheduled refresh fails. A successful refresh still reschedules via zoneEnd as before.

2. Uninitialized loop index in WidgetSet::updateAll()
for (int8_t i; i < m_widgetCount; i++) read an uninitialized i — undefined behavior executed on every boot via initializeAllWidgetsData(). If the stack garbage was ever non-zero, widgets would be silently skipped (or worse, m_widgets[i] indexed out of bounds). Now int8_t i = 0.

3. Button ISRs not placed in IRAM
The three ISR wrappers in main.cpp and Button::isrButtonChange() lacked IRAM_ATTR. On the ESP32, an interrupt that executes from flash-cached code while the flash cache is disabled (e.g. during an NVS/config write) crashes with "Cache disabled but cached memory region accessed" — i.e. pressing a button at the wrong moment could reboot the device. All four functions are now IRAM_ATTR (millis() and digitalRead() are IRAM-safe in the ESP32 Arduino core).

Build verified (RAM 16.2%, Flash 77.2%) and the host-side timing simulation in firmware/test/host_sim_long_uptime.py still passes all checks.

@ericthelin
ericthelin changed the base branch from main to dev July 14, 2026 18:04
@ericthelin

ericthelin commented Jul 14, 2026

Copy link
Copy Markdown
Collaborator

I updated the target branch to be dev instead of main because I think that is still where we are doing development then merging dev into main at release points. (Unless I missed something in the time I have been in active). But unfortunately that triggered merge conflicts. That said please do fix the conflicts and update because I fully support your change and want to see it merged. I chased the tick delay issues for a long time and didn't find a solution. So I really want this merged. That said it also would be a good thing to split this into a few separate PRs as it is now doing multiple things at once.

@Bullpuph123

Copy link
Copy Markdown
Author

Thanks for the support and for retargeting to dev! As requested, I've split this into three focused PRs, each re-implemented directly on top of dev (rather than resolving conflicts here, since dev's ConfigManager/WidgetTimer architecture diverged quite a bit from main):

One note: the uninitialized loop index fix in WidgetSet::updateAll from this PR was dropped — it's already fixed on dev.

Closing this in favor of the three above.

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