Skip to content

Fix clock colon blink regularity and AM/PM redraw heap churn#335

Open
Bullpuph123 wants to merge 2 commits into
brettdottech:devfrom
Bullpuph123:fix/clock-colon-ampm
Open

Fix clock colon blink regularity and AM/PM redraw heap churn#335
Bullpuph123 wants to merge 2 commits into
brettdottech:devfrom
Bullpuph123:fix/clock-colon-ampm

Conversation

@Bullpuph123

@Bullpuph123 Bullpuph123 commented Jul 15, 2026

Copy link
Copy Markdown

Split from #330 (part 2 of 3), rebased onto dev as requested.

Summary

  • Smooth, drift-free colon blink (NORMAL clock): the colon was driven by seconds parity from GlobalTime, so any skipped/duplicated second made the blink irregular. It's now driven by a millis()-based 500ms blinker with parity-correct catch-up (if draw is delayed by multiple periods, visibility only toggles on an odd number of elapsed periods). NIXIE/custom clocks keep the image-based 1s colon.
  • AM/PM redrawn only on change: displayAmPm() was called every second, and each call reloads TTF fonts (DSEG7↔DSEG14) plus per-loop String allocations — over days of uptime this fragments the heap and eventually corrupts glyph rendering (stray "PM" artifacts observed after long uptime). It's now redrawn only when the value changes or on force, with the old value cleared first.
  • Bounds/ownership guards: displayDigit() validates the display index; displaySeconds() only draws on the status orb and validates the seconds range — defense against a corrupted index painting tick marks on the wrong panel.
  • CLOCK_UPDATE_DELAY/CLOCK_DRAW_DELAY defaults changed from OneSecond to OneHundredMilliseconds so the 500ms colon blinker actually gets a chance to run; all per-second work is still internally gated on second changes, so this doesn't add redraw load.

Test plan

🤖 Generated with Claude Code

- The colon was driven by the parity of the displayed second, so any
  skipped/repeated second (scheduler drift, NTP resync, blocked loop)
  visibly stalled or inverted the blink. For the NORMAL clock it is now
  a dedicated millis()-based 500ms blinker (rollover-safe, drift-free,
  parity-correct catch-up after stalls). NIXIE/custom clocks keep the
  1s image cadence to avoid doubling the JPG decode load.
- AM/PM was redrawn every second, and displayAmPm() reloads the TTF
  font (DSEG7<->DSEG14 workaround) on every call: ~86k font unload/
  reload cycles per day fragmented the heap over long uptimes until
  rendering corrupted (stray "PM" on digit orbs). It is now redrawn
  only when the value changes or on a forced redraw.
- Clock update/draw timers raised from 1s to 100ms so sampling can
  never alias/skip a displayed second; both calls are cheap no-ops
  when nothing changed.
- Middle-orb ownership made explicit (SCREEN_STATUS): displaySeconds()
  rejects other orbs and out-of-range seconds, displayDigit() rejects
  invalid orb indices, so no drawing primitive can target a wrong
  screen even with corrupted state.
- setup() resets colon/AM-PM/seconds cached state so widget switches
  start clean.

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

Copilot AI left a comment

Copy link
Copy Markdown

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 improves long-uptime stability and visual consistency of the firmware ClockWidget, focusing on a drift-free colon blink for the NORMAL clock type and reducing heap churn by avoiding per-second AM/PM redraws.

Changes:

  • Implement a millis()-driven 500ms colon blinker for NORMAL clocks (with catch-up parity handling).
  • Redraw AM/PM only when it changes (or on forced redraw), avoiding repeated font reload cycles.
  • Add defensive guards so colon/seconds tick rendering is constrained to the middle/status orb and avoids invalid indices/ranges.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
firmware/src/widgets/clockwidget/ClockWidget.h Adds status-orb ownership constant, colon blink state, and changes default draw/update cadence to 100ms.
firmware/src/widgets/clockwidget/ClockWidget.cpp Implements 500ms colon blink for NORMAL, gates AM/PM redraw to changes, and adds bounds/ownership guards for digit/seconds rendering.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +95 to +115
if (m_type == (int) ClockType::NORMAL) {
// Colon blink: stable 500ms on / 500ms off, driven directly by
// millis() instead of the parity of the displayed second, so a
// skipped or repeated second (scheduler drift, NTP resync) can
// never stall the blink. Rollover-safe unsigned arithmetic.
uint32_t now = millis();
uint32_t elapsed = now - (uint32_t) m_colonBlinkPrev;
if (elapsed >= m_colonBlinkInterval) {
// Advance in whole periods so no cumulative drift builds up,
// and catch up in one step if the loop was blocked for a while.
// Toggle by period parity so a blocked loop (e.g. a blocking
// network call) can never invert the blink phase.
uint32_t periods = elapsed / m_colonBlinkInterval;
m_colonBlinkPrev += m_colonBlinkInterval * periods;
if (periods % 2 == 1) {
m_colonVisible = !m_colonVisible;
}
displayColon();
} else if (force) {
displayColon();
}

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 2b44a84: forced redraws now restart the blink phase with the colon visible (m_colonVisible = true, m_colonBlinkPrev = now) instead of running the catch-up path, so the colon can no longer reappear in the "off" state after a screen clear or widget switch.

@ericthelin ericthelin left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like to see the constant renamed then I will approve this

private:
// The middle/status orb exclusively owns the colon, the seconds tick
// and the AM/PM indicator. Digit orbs (0, 1, 3, 4) must never receive them.
static constexpr int SCREEN_STATUS = 2;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't love the variable name SCREEN_STATUS because "status" is not really what is being displayed

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point — renamed to SCREEN_MIDDLE in 2b44a84, matching the existing BUTTON_MIDDLE naming convention.

…ink phase on forced redraw

SCREEN_MIDDLE matches the existing BUTTON_MIDDLE naming convention.
Forced redraws (widget switch, clock face change) now restart the blink
phase with the colon visible instead of possibly reappearing "off".

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
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.

3 participants