Skip to content

Add Pomodoro Timer plugin with MQTT + Home Assistant control - #227

Open
ChuckBuilds wants to merge 7 commits into
mainfrom
claude/pomodoro-timer-mqtt-plugin-naxswg
Open

Add Pomodoro Timer plugin with MQTT + Home Assistant control#227
ChuckBuilds wants to merge 7 commits into
mainfrom
claude/pomodoro-timer-mqtt-plugin-naxswg

Conversation

@ChuckBuilds

@ChuckBuilds ChuckBuilds commented Jul 28, 2026

Copy link
Copy Markdown
Owner

Summary

Adds pomodoro-timer (v1.3.1), a configurable focus/break timer for the matrix driven over MQTT with Home Assistant auto-discovery — the same integration approach as the on-air plugin, extended so the durations and the current task are settable from HA. Work/short-break/long-break lengths and sessions-per-set are all configurable, and the timer can be started, paused, resumed, skipped, stopped, or reset remotely.

Note: this description was rewritten at 1.3.1. The display has changed substantially since the first commit — the countdown is now seven-segment geometry rather than font-rendered text, and the fill-up progress bar has been replaced by a burndown ring. The per-version detail is in manifest.json's versions array.

Type of change

  • Bug fix in an existing plugin
  • New plugin (also fill out the SUBMISSION.md checklist below)
  • New feature for an existing plugin
  • Documentation only
  • Repo-wide change (registry script, hook, top-level docs)

Plugin(s) affected

pomodoro-timer (new). plugins.json gains its registry entry.

Related issues

N/A

Test plan

  • Loaded the plugin in LEDMatrix on real hardware
  • Loaded the plugin in LEDMatrix emulator mode (EMULATOR=true python3 run.py)
  • Rendered the plugin in the dev preview server
  • Verified the web UI configuration form against the schema
  • N/A — repo-wide / docs-only change

Safety harness — passes on all eight sampled panel sizes. Goldens committed for the four canonical sizes with a test/harness.json pinning every render-affecting setting.

=== pomodoro-timer ===
  [PASS]   64x32  pomodoro (golden ✓)      [PASS]  128x64  pomodoro (golden ✓)
  [PASS]  128x32  pomodoro (golden ✓)      [PASS]  256x32  pomodoro (golden ✓)
  [PASS]   64x64  pomodoro                 [PASS]  128x96  pomodoro
  [PASS]   96x48  pomodoro                 [PASS] 256x128  pomodoro

Panel-shape sweep — the harness samples eight shapes, but a matrix build can be any size, so I swept 256 shapes from 32×8 to 384×128: no crashes, no overflow, no blank frames. This caught 18 shapes (all under 16px tall) where the countdown was laid out past the bottom edge — fixed in 1.3.1 and pinned by a test.

Pixel-exactness — a rendered frame contains only its palette colours: background, accent, the 70% label, the 14% ring track, the 28% upcoming pip, and the lightened ring head. Six values, no antialiasing. PIL antialiases TrueType text by default, which had been putting a grey halo around every glyph (26–36 distinct colours per frame); the label now renders through a thresholded mask. Pinned by a test.

Unit teststest_pomodoro_timer.py ships with the plugin: 39 cases covering the state machine (full 4-session cycle into the long break and back, pause/resume/toggle, stop-vs-reset counter semantics, skipped work not earning a long break), payload parsing, the display-pin compare-and-set, MQTT connect/subscribe/dispatch/teardown, discovery validity, topic-abandonment cleanup, digit legibility across panel shapes, and the burndown ring draining in proportion. It stubs the core, so it runs from a plain checkout with only Pillow installed:

python -m pytest plugins/pomodoro-timer/test_pomodoro_timer.py -q   # 39 passed
python plugins/pomodoro-timer/test_pomodoro_timer.py                # no pytest needed

End-to-end MQTT — run against a live mosquitto broker: connects, publishes all discovery configs, responds to every command and to a work_minutes/set write, publishes state/status/phase/remaining/session/attributes/event on each transition, and publishes offline before disconnecting.

Schema cross-check — every config key the manager reads exists in config_schema.json and every schema default is mirrored in the code's config.get(...); x-propertyOrder covers all 46 properties; the schema validates as Draft-07 and its own defaults validate against it. check_module_collisions.py passes — the plugin ships only manager.py, so there's no bare-name collision surface. Bandit reports zero findings.

Not tested on real hardware — I don't have a panel. Layout and bounds are covered by the sweep, the MQTT path by a real broker.

Required for plugin changes

  • Bumped version in plugins/<id>/manifest.json (1.3.1)
  • class_name matches the actual class in manager.py
  • entry_point matches the real file
  • Updated the plugin's README.md
  • config_schema.json is the source of truth — every option has a default, description, and constraints
  • Pre-commit hook ran successfully (auto-syncs plugins.json)

SUBMISSION checklist (new plugins only)

  • Plugin id matches the directory name and is unique
  • manifest.json has all required fields and validates against the core's schema/manifest_schema.json
  • manager.py inherits from BasePlugin and implements update() and display()
  • config_schema.json exists and validates as JSON Schema Draft-7
  • requirements.txt lists all Python dependencies (paho-mqtt>=1.6.0)
  • README.md documents what the plugin does, how to install, and the configuration options
  • LICENSE is GPL-3.0
  • No hardcoded API keys or secrets — the broker password is an x-sensitive config field
  • Tested on a real LEDMatrix setup (harness + live MQTT broker only; no hardware available)

What it does

Timer. Configurable work / short break / long break lengths and sessions per set, with independent auto-start for breaks and for the next work session. It runs on its own thread, so it keeps counting and publishing whether or not it's the screen on the panel. By default it holds the display while a session is active and releases it when idle; when a phase ends it grabs the panel and flashes.

Display. The countdown is drawn as seven-segment digits sized to the panel — geometry, not a font — so it scales to whatever space is left instead of snapping to font sizes. The digits refuse to drop below a readable width-to-height ratio and step down a size instead. Above them is the phase label, or a task name if one is set; below, one pip per session in the set (solid = done, hollow = upcoming, picked out = the one you're in). The burndown ring lights the whole border at the start of a phase and drains it clockwise behind a brighter head pixel — it costs no interior space, which is what pays for the larger digits. bar, segments and none are the alternatives.

Layout adapts: stacked on 64×32, 128×32 and 128×64; label and pips beside an oversized countdown on long panels like 256×32. On panels under 40px tall the pips tuck in beside the label rather than taking a row. Phase colour carries the state (work / short break / long break / paused / idle), with a calm palette preset and a desaturate option for paused.

MQTT. The command topic accepts START, PAUSE, RESUME, TOGGLE, SKIP, STOP, RESET and the direct phase commands, plus JSON for one-off durations and labels ({"command":"start","duration_minutes":50,"label":"DEEP WORK"}) and for changing the configured durations. Unrecognised payloads are ignored, so a stray message can't be mistaken for a stop. It publishes state, status, phase, remaining, remaining_seconds, session, task, a JSON attributes snapshot, transition events, and available (with LWT).

Auto-discovery announces 18 entities: a switch, five control buttons, four duration number boxes that write back, a text entity for the current task, five sensors, an event entity for automations, and connectivity.

Notes for reviewer

  • Rendering into the display manager's buffer. Unlike on-air and clock-simple, which assign a fresh Image.new(...), this plugin reuses the existing buffer when it's at least panel-sized and fills the panel region with a background rectangle. That keeps the harness's bounds check meaningful — a fresh canvas makes overflow undetectable — while avoiding the clear() flash clock-simple documents.
  • Runtime vs. saved durations. Changes from the HA number boxes and the task field apply immediately but aren't written back to saved config, so they're lost on restart. Documented in the README and schema. Happy to change it if there's an existing pattern for a plugin persisting its own config.
  • Session counter semantics. STOP goes idle and keeps the completed count; RESET zeroes it and clears the task. A completed long break resets the pips. Skipping a work session deliberately doesn't count toward the set, so it can't earn a long break.
  • No circular progress ring. Requested during review, but not viable here: enclosing the readout in a circle takes the digits from 16px wide to 1px on 128×32, and 27 to 8 on 128×64. The rectangular perimeter is that same gauge fitted to the panel's aspect ratio.
  • Goldens are committed for the four canonical sizes only, not all eight sampled, to limit exposure to Pillow antialiasing drift across versions.
  • Docstring pre-merge warning is stale — it reports 3.23% from the second commit and hasn't re-measured since. manager.py is at 76%; I stopped short of the 80% threshold rather than pad trivial helpers. For context, on-air sits at 22% and countdown at 15%.

A configurable focus/break timer rendered on the matrix and driven over
MQTT, modelled on the on-air plugin's MQTT + HA auto-discovery approach.

Timer:
- Configurable work, short break, and long break lengths, plus sessions
  per set; auto-start breaks and/or the next work session independently.
- Runs on its own thread so it keeps counting and publishing whether or
  not it is the screen currently on the panel.
- Holds the display while a session is active (optional) and grabs the
  panel with a flash when a phase ends.

Display:
- Phase label, MM:SS countdown, session dots, and a phase progress bar,
  each individually toggleable and colourable.
- Countdown font is fitted to the panel; layout is stacked on smaller
  panels and side-by-side on long ones (256x32). Labels that still do
  not fit are truncated rather than drawn past the panel edge.

MQTT:
- Command topic accepts START/PAUSE/RESUME/TOGGLE/SKIP/STOP/RESET and
  the phase commands, plus JSON for one-off durations and labels and
  for changing the configured durations.
- Publishes state, status, phase, remaining, remaining_seconds, session
  count, a JSON attributes snapshot, transition events, and availability
  (with LWT).
- HA auto-discovery announces 17 entities: switch, five control buttons,
  four duration number boxes, five sensors, a timer-event entity, and
  connectivity. Duration boxes write back so the timer can be set from
  Home Assistant.

Verified with the core safety harness across all eight sampled panel
sizes (goldens committed for 64x32, 128x32, 128x64, 256x32), a 21-case
self-contained test suite, and an end-to-end run against mosquitto.

Co-Authored-By: Claude Opus 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_01AtZDGDrzbWdK4JxUeH4hk6
@coderabbitai

coderabbitai Bot commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

Adds a Pomodoro Timer plugin with configurable work and break phases, LED matrix rendering, MQTT commands and state publishing, Home Assistant discovery, plugin metadata, documentation, and automated tests.

Changes

Pomodoro Timer Plugin

Layer / File(s) Summary
Plugin contract and registration
plugins/pomodoro-timer/config_schema.json, plugins/pomodoro-timer/manifest.json, plugins.json, plugins/pomodoro-timer/requirements.txt, plugins/pomodoro-timer/LICENSE
Defines configuration validation, plugin metadata, registry information, the MQTT dependency, and GPLv3 licensing.
Timer state machine and lifecycle
plugins/pomodoro-timer/manager.py
Implements timer phases, commands, duration updates, state snapshots, lifecycle hooks, and threaded timer execution.
Display rendering and pinning
plugins/pomodoro-timer/manager.py
Adds responsive layouts, font fitting, labels, session indicators, progress bars, alerts, and display pin synchronization.
MQTT and Home Assistant integration
plugins/pomodoro-timer/manager.py, plugins/pomodoro-timer/README.md
Adds topic handling, state/event/availability publishing, MQTT reconnect behavior, Home Assistant discovery entities, and manual MQTT/Home Assistant configuration.
Validation, fixtures, and documentation
plugins/pomodoro-timer/test_pomodoro_timer.py, plugins/pomodoro-timer/test/harness.json, plugins/pomodoro-timer/README.md
Adds timer, rendering, MQTT, discovery, configuration, and lifecycle tests plus usage, setup, testing, and troubleshooting documentation.

Estimated code review effort: 4 (Complex) | ~60 minutes

Sequence Diagram(s)

sequenceDiagram
  participant HomeAssistant
  participant MQTTBroker
  participant PomodoroTimerPlugin
  participant LEDMatrix
  HomeAssistant->>MQTTBroker: Send timer command
  MQTTBroker->>PomodoroTimerPlugin: Deliver subscribed message
  PomodoroTimerPlugin->>PomodoroTimerPlugin: Update timer state
  PomodoroTimerPlugin->>MQTTBroker: Publish state and events
  PomodoroTimerPlugin->>LEDMatrix: Render current phase and remaining time
  MQTTBroker->>HomeAssistant: Deliver state and discovery messages
Loading
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 3.23% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main change: adding the Pomodoro Timer plugin with MQTT and Home Assistant integration.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch claude/pomodoro-timer-mqtt-plugin-naxswg

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@codacy-production

codacy-production Bot commented Jul 28, 2026

Copy link
Copy Markdown

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Metrics 540 complexity

Metric Results
Complexity 540

View in Codacy

NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 5

🧹 Nitpick comments (2)
plugins/pomodoro-timer/LICENSE (1)

1-17: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

LICENSE contains only the GPL-3.0 notice, not the license text.

GPL-3.0 asks that a copy of the full license accompany the work; this file is just the short notice. Also worth confirming the copyright holder (LEDMatrix Team) matches the manifest author (ChuckBuilds).

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@plugins/pomodoro-timer/LICENSE` around lines 1 - 17, Replace the short notice
in LICENSE with the complete GPL-3.0 license text, preserving the applicable
copyright attribution. Also verify the copyright holder against the manifest
author and update the attribution if they do not match.
plugins/pomodoro-timer/manager.py (1)

1112-1116: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Call _remove_discovery() when discovery is being torn down.

_remove_discovery() is only defined, so HA discovery topics remain retained when the plugin is disabled, ha_discovery is turned off, or config changes restart MQTT. Add the removal call before the MQTT clients are shut down in on_disable(), _graceful_shutdown(), and the discovery-disabled path in on_config_change().

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@plugins/pomodoro-timer/manager.py` around lines 1112 - 1116, Invoke
_remove_discovery() before MQTT clients are shut down in on_disable(),
_graceful_shutdown(), and the discovery-disabled branch of on_config_change().
Ensure each teardown or configuration path removes retained Home Assistant
discovery topics before disabling or replacing the MQTT connection.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@plugins/pomodoro-timer/manager.py`:
- Around line 396-407: The START/ON/PLAY handling in the command-processing
method emits a “started” event even when an already-running timer has no phase
or duration override. Track whether one of the state-changing branches in this
block executes, and append the event only when the timer was begun, resumed, or
otherwise updated; preserve the existing event payload for actual state changes.
- Around line 1190-1211: Centralize MQTT cleanup in one idempotent teardown
helper that stops the network loop, disconnects, and clears self.mqtt_client. In
_mqtt_loop, call it before every _connect_mqtt attempt and clear mqtt_connecting
when no CONNACK arrives during the 5-second wait; in
plugins/pomodoro-timer/manager.py lines 1239-1256, set mqtt_stop_event
unconditionally and invoke the same helper after joining the thread so shutdown
always releases the client.
- Around line 1239-1256: Update _graceful_shutdown to unconditionally set
mqtt_stop_event and clean up any existing mqtt_client, regardless of whether
mqtt_thread is alive. Keep loop_stop() and disconnect() protected against
cleanup exceptions, then join the thread only when it exists and is alive before
clearing the MQTT state flags.
- Around line 934-954: Update _sync_pin so the comparison and update of
self._pinned, together with the corresponding cache_manager.set request, execute
under state_lock. Preserve the existing want calculation and early return, using
the lock’s RLock behavior to serialize concurrent callers and prevent requests
from being written out of order.

In `@plugins/pomodoro-timer/test_pomodoro_timer.py`:
- Around line 53-64: Update _load_module to save the existing src,
src.plugin_system, and src.plugin_system.base_plugin entries before installing
stubs, then restore those original entries in a finally block after importing
manager.py, removing only entries that were previously absent. Keep
_install_core_stub unchanged and ensure later tests see the installed core
modules.

---

Nitpick comments:
In `@plugins/pomodoro-timer/LICENSE`:
- Around line 1-17: Replace the short notice in LICENSE with the complete
GPL-3.0 license text, preserving the applicable copyright attribution. Also
verify the copyright holder against the manifest author and update the
attribution if they do not match.

In `@plugins/pomodoro-timer/manager.py`:
- Around line 1112-1116: Invoke _remove_discovery() before MQTT clients are shut
down in on_disable(), _graceful_shutdown(), and the discovery-disabled branch of
on_config_change(). Ensure each teardown or configuration path removes retained
Home Assistant discovery topics before disabling or replacing the MQTT
connection.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 1611a9f0-217c-4242-a941-26d440b53b64

📥 Commits

Reviewing files that changed from the base of the PR and between 3f7c90e and 8050b27.

⛔ Files ignored due to path filters (4)
  • plugins/pomodoro-timer/test/golden/128x32/pomodoro.png is excluded by !**/*.png
  • plugins/pomodoro-timer/test/golden/128x64/pomodoro.png is excluded by !**/*.png
  • plugins/pomodoro-timer/test/golden/256x32/pomodoro.png is excluded by !**/*.png
  • plugins/pomodoro-timer/test/golden/64x32/pomodoro.png is excluded by !**/*.png
📒 Files selected for processing (9)
  • plugins.json
  • plugins/pomodoro-timer/LICENSE
  • plugins/pomodoro-timer/README.md
  • plugins/pomodoro-timer/config_schema.json
  • plugins/pomodoro-timer/manager.py
  • plugins/pomodoro-timer/manifest.json
  • plugins/pomodoro-timer/requirements.txt
  • plugins/pomodoro-timer/test/harness.json
  • plugins/pomodoro-timer/test_pomodoro_timer.py

Comment thread plugins/pomodoro-timer/manager.py Outdated
Comment thread plugins/pomodoro-timer/manager.py Outdated
Comment thread plugins/pomodoro-timer/manager.py
Comment thread plugins/pomodoro-timer/manager.py Outdated
Comment thread plugins/pomodoro-timer/test_pomodoro_timer.py
Review fixes, all in the plugin's threading/MQTT plumbing:

- Centralise paho client teardown in _teardown_client(). The reconnect
  path, the loop's error handler, and shutdown now all funnel through
  it, so a refused CONNACK or a half-open attempt can no longer leave a
  live client and its network thread behind for the next attempt to
  overwrite.
- Retry when no CONNACK arrives within the 5s window. Previously
  mqtt_connecting stayed set, which parked the supervisor loop in its
  1s wait forever and never reconnected.
- _graceful_shutdown now sets the stop event and tears the client down
  unconditionally, so a loop that already died (or never started) with
  a live client still releases it.
- Publish the offline availability state, and wait for it, before
  disconnecting. It was racing the loop thread's own offline publish
  against our disconnect, so HA could sit on a stale 'online' until the
  keepalive lapsed.
- Serialise the whole _sync_pin compare-and-set under state_lock. It is
  called from the timer, MQTT, and display threads; interleaving could
  publish a stop after a start and, because the method early-returns
  once _pinned matches, strand the panel until the next transition. A
  failed request now restores _pinned so the next tick retries.
- START on an already-running timer no longer emits a 'started' event,
  so pressing the HA Start button mid-session can't re-trigger
  automations. Settings sent in the same payload still apply.
- Clear retained discovery topics when ha_discovery is switched off,
  otherwise the entities linger in HA forever. A plain restart still
  leaves them retained -- that is what lets HA keep the device across
  reboots, with the LWT marking it unavailable.
- Restore sys.modules in the test loader so its stub 'src' package
  can't shadow the real core for other plugins' tests.

Removes the three try/except/pass blocks flagged as a security issue by
Codacy and Ruff (S110); bandit is now clean on manager.py. Adds six
regression tests (26 total) and re-verified the harness across all
sampled panel sizes and end-to-end against mosquitto.

Co-Authored-By: Claude Opus 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_01AtZDGDrzbWdK4JxUeH4hk6

Copy link
Copy Markdown
Owner Author

Addressed the review in 42cb7b0 (version bumped to 1.0.1). Six of the seven findings were valid; one I've skipped with a reason.

Fixed

  • _sync_pin unsynchronized compare-and-set — real, and the worst of the batch. Folded the whole compare-and-set, including the cache_manager.set, into state_lock. A failed request now restores _pinned so the next tick retries instead of the early-return stranding the panel.
  • MQTT teardown not centralized — real. Added _teardown_client(), and the reconnect path, the loop's error handler, and shutdown all funnel through it. Chasing this also turned up a worse adjacent bug: when no CONNACK arrived within the 5s window, mqtt_connecting stayed set, which parked the supervisor loop in its 1s wait forever — the plugin would never reconnect. It now tears the client down and backs off.
  • _graceful_shutdown skips cleanup when the thread is dead — real. Stop event and teardown are now unconditional. Verifying this surfaced a regression my own first fix introduced: publishing offline from the loop thread's exit path was racing our disconnect(), so the availability message never left paho's queue and HA would sit on a stale online until the keepalive lapsed. Shutdown now publishes offline at QoS 1 and waits for it before disconnecting. Caught it in the mosquitto run, not in review.
  • Spurious started event — real. A redundant START emits no event now. It still applies any settings in the same payload and publishes them, so {"command":"start","work_minutes":30} mid-session isn't silently dropped.
  • Test loader leaves stubs in sys.modules — real. Restores the originals in a finally, removing only the entries that were absent before. manager.py binds BasePlugin at class-creation time, so the stub only needs to survive the exec.
  • _remove_discovery() never called — real, but adopted narrowly. It now fires only when ha_discovery goes true→false, which is the case where entities would otherwise linger in HA forever. I deliberately did not call it on disable/shutdown: retained discovery is what lets HA keep the device across a reboot, and the LWT already marks it unavailable in the meantime. Removing it on every restart would churn the device instead.

Skipped

  • LICENSE is the short notice, not the full GPL text — this file is byte-identical to the one every other plugin in the repo ships (plugins/*/LICENSE), including the LEDMatrix Team attribution. Changing it for this one plugin would make it the odd one out; if the convention should change it's a repo-wide call, not something to fix here.

Side effect worth noting: this removed the three try/except/pass blocks that Ruff flagged as S110, which I believe is also the "1 medium security issue" the Codacy gate is failing on — bandit now reports zero findings on manager.py.

Verification after the changes: 26 tests pass (six new regression cases covering teardown idempotency, a client that raises during teardown, shutdown with a dead thread, the offline-before-disconnect publish, the redundant START, and pin restoration on a failed request); the safety harness passes on all eight sampled panel sizes with the four goldens still matching; and a fresh end-to-end run against mosquitto confirms discovery, every command, the number-box write, phase expiry, and the offline publish on cleanup.


Generated by Claude Code

Redraws the panel. The countdown was set in the display's pixel font,
which is a good label face and a poor timer face; the burndown was a bar
that filled up along the bottom.

- The countdown is now drawn as geometric seven-segment digits sized to
  the panel rather than picked from a font, so it scales to whatever
  space is left instead of snapping to font sizes. Unlit segments stay
  faintly lit the way a real LED clock shows its dark segments, which
  also stops the digits appearing to shift as the numbers change.
  digit_style: pixel keeps the old look.
- The digits refuse to go below a 0.64 width-to-height ratio and step
  down a size instead. Without that guard a tall box drove them to a
  sliver and a 0 stopped reading as a digit at all -- it became two
  stacked boxes on 128x64.
- New burndown indicator. perimeter (default) lights the whole border at
  the start of a phase and drains it clockwise from the top-left behind
  a brighter head pixel. It costs no interior space, which is what pays
  for the larger digits. bar and segments sit along the bottom edge and
  both now empty rather than fill; none hides it.
- On panels under 40px tall the session dots tuck in beside the phase
  label when the label leaves room, instead of taking a row of their
  own. That is a quarter of the height back for the digits on a 32px
  panel.

Config: show_progress_bar is replaced by progress_style; adds
digit_style and show_ghost_segments. Goldens regenerated for all four
canonical sizes. Five new tests (31 total) cover the ring draining in
proportion, digit legibility across panel shapes, the fallback when
there is no room for segments, and every style combination rendering.

Co-Authored-By: Claude Opus 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_01AtZDGDrzbWdK4JxUeH4hk6

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🧹 Nitpick comments (3)
plugins/pomodoro-timer/manager.py (2)

1132-1140: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Don't mutate self.progress_style from the render path.

_draw_segments permanently rewrites the configured style on a transient width computation, from the display thread and outside state_lock. Draw the bar fallback locally instead of persisting the change.

♻️ Proposed fix
         block = (width - (count - 1) * gap) // count
         if block < 1:
-            self.progress_style = "bar"
+            # Too narrow for discrete blocks — render this frame as a plain bar.
+            draw.rectangle([x, y, x + width - 1, y + height - 1], fill=_dim(accent, 0.18))
+            lit_w = int(round(max(0.0, min(1.0, remaining)) * width))
+            if lit_w > 0:
+                draw.rectangle([x, y, x + lit_w - 1, y + height - 1], fill=accent)
             return
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@plugins/pomodoro-timer/manager.py` around lines 1132 - 1140, Update
_draw_segments so an insufficient block width renders the bar fallback locally
without assigning to self.progress_style. Keep the configured style unchanged
and ensure the fallback uses the existing bar-rendering path.

643-649: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win

Retained discovery configs are orphaned when command_topic/discovery_prefix change.

The cleanup only triggers on discovery being turned off. Renaming command_topic (which derives every state topic) or discovery_prefix republishes under new topics while the old retained configs linger, leaving permanently-unavailable HA entities. Consider also calling _remove_discovery() when those two inputs change.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@plugins/pomodoro-timer/manager.py` around lines 643 - 649, Update the
configuration-change handling around self._remove_discovery() to also clear
retained discovery topics when command_topic or discovery_prefix changes, not
only when ha_discovery is disabled. Compare the previous and new values of both
inputs, invoke cleanup using the existing discovery configuration before
publishing replacements, and preserve the current restart behavior when none of
these settings change.
plugins/pomodoro-timer/test_pomodoro_timer.py (1)

341-352: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Unused unpacked variables gap/colon_w.

Only h, digit_w, stroke, and total are used in the assertions; Ruff flags gap and colon_w as unused (RUF059).

🧹 Proposed fix
-            h, digit_w, stroke, gap, colon_w, total = metrics
+            h, digit_w, stroke, _gap, _colon_w, total = metrics
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@plugins/pomodoro-timer/test_pomodoro_timer.py` around lines 341 - 352, Update
test_seven_segment_digits_keep_a_readable_shape to avoid binding the unused gap
and colon_w values when unpacking _seven_segment_metrics; retain the existing h,
digit_w, stroke, and total values used by the assertions.

Source: Linters/SAST tools

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@plugins/pomodoro-timer/manager.py`:
- Around line 1132-1140: Update _draw_segments so an insufficient block width
renders the bar fallback locally without assigning to self.progress_style. Keep
the configured style unchanged and ensure the fallback uses the existing
bar-rendering path.
- Around line 643-649: Update the configuration-change handling around
self._remove_discovery() to also clear retained discovery topics when
command_topic or discovery_prefix changes, not only when ha_discovery is
disabled. Compare the previous and new values of both inputs, invoke cleanup
using the existing discovery configuration before publishing replacements, and
preserve the current restart behavior when none of these settings change.

In `@plugins/pomodoro-timer/test_pomodoro_timer.py`:
- Around line 341-352: Update test_seven_segment_digits_keep_a_readable_shape to
avoid binding the unused gap and colon_w values when unpacking
_seven_segment_metrics; retain the existing h, digit_w, stroke, and total values
used by the assertions.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 83c553c8-fe18-4a3f-ab88-66b75e13f770

📥 Commits

Reviewing files that changed from the base of the PR and between 8050b27 and bb26b7f.

⛔ Files ignored due to path filters (4)
  • plugins/pomodoro-timer/test/golden/128x32/pomodoro.png is excluded by !**/*.png
  • plugins/pomodoro-timer/test/golden/128x64/pomodoro.png is excluded by !**/*.png
  • plugins/pomodoro-timer/test/golden/256x32/pomodoro.png is excluded by !**/*.png
  • plugins/pomodoro-timer/test/golden/64x32/pomodoro.png is excluded by !**/*.png
📒 Files selected for processing (7)
  • plugins.json
  • plugins/pomodoro-timer/README.md
  • plugins/pomodoro-timer/config_schema.json
  • plugins/pomodoro-timer/manager.py
  • plugins/pomodoro-timer/manifest.json
  • plugins/pomodoro-timer/test/harness.json
  • plugins/pomodoro-timer/test_pomodoro_timer.py
🚧 Files skipped from review as they are similar to previous changes (5)
  • plugins/pomodoro-timer/test/harness.json
  • plugins.json
  • plugins/pomodoro-timer/config_schema.json
  • plugins/pomodoro-timer/manifest.json
  • plugins/pomodoro-timer/README.md

… topics

Review fixes.

- _draw_segments assigned self.progress_style = "bar" when a panel was
  too narrow for discrete blocks, permanently overwriting the user's
  setting from the render thread on a transient width computation. The
  bar is now its own method that both the strip dispatcher and the
  segments fallback call, so the fallback lasts one frame.
- Changing the HA discovery prefix republishes the configs under the new
  prefix and never touches the old ones, leaving permanently-unavailable
  entities in Home Assistant. Those retained configs are now cleared
  before the settings are reloaded, while the old prefix is still known.
- Renaming the command topic is a different case: the discovery path is
  keyed on the prefix and plugin id, so republishing overwrites it in
  place and HA follows. What it does strand is the retained payloads on
  the old state topics, so those are cleared instead.
- Dropped two unused names from a test's tuple unpack (RUF059).
- Added docstrings across the state machine, render path, and MQTT
  plumbing, taking manager.py from 38% to 76% coverage.

34 tests (three new: the render path leaving progress_style alone, and
each of the two topic-orphaning cases). Harness green on all sampled
sizes with the goldens unchanged.

Co-Authored-By: Claude Opus 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_01AtZDGDrzbWdK4JxUeH4hk6

Copy link
Copy Markdown
Owner Author

Addressed in 9237514 (1.1.1). All three findings were valid.

  • _draw_segments rewriting self.progress_style — a real bug, and the worst of the three: a transient width computation on the render thread was permanently overwriting the user's configured setting. The bar is now its own method that both the strip dispatcher and the segments fallback call, so a too-narrow panel falls back for one frame instead of editing the config.

  • Orphaned discovery configs — valid for discovery_prefix, and the two inputs turn out to fail differently, so they're handled differently:

    • Prefix change republishes the configs under the new prefix and never touches the old ones, leaving permanently-unavailable entities in HA. Those are now cleared before the settings reload, while the old prefix is still known.
    • Command topic change is not orphaning in the same way — the discovery path is keyed on the prefix and plugin id, so republishing overwrites it in place and HA follows the entity to its new topics. What it does strand is the retained payloads on the abandoned state topics, so those get cleared instead.
  • Unused gap/colon_w — fixed.

On the docstring coverage check: I measured before acting, because 3.23% didn't match the code. On manager.py it was 38%; the low figure looks like it's averaging in the test file, where 31 self-describing test_* functions have no docstrings by design. I've taken manager.py to 76% by documenting the state machine's locking contract, the render path, and the MQTT supervisor loop — the places where a docstring actually tells you something. I've deliberately not chased the remaining 4 points, which would mean writing """Return the dot size.""" over _dot_size; that's noise, not documentation. For context, the comparable plugins in this repo sit at 22% (on-air) and 15% (countdown).

34 tests now, three new ones covering the render path leaving progress_style alone and each of the two topic-abandonment cases. Harness green on all eight sampled sizes with the goldens unchanged, and bandit still clean.


Generated by Claude Code

ChuckBuilds and others added 3 commits July 28, 2026 22:59
The ghost segments were the problem. They are authentic -- real LED
clocks show their dark segments -- but they put a faint 8 behind every
digit, so 15:00 read as 85:00 from across a room. My own comparison
sheet showed the no-ghost row was the clearest of the lot and I shipped
them on anyway. They are now off by default, in the schema as well as
the code, and still available for anyone who wants the clock-radio look.

Also, to push the digits further:

- Heavier segment stroke (0.15 -> 0.17 of digit height). More lit pixels
  is what legibility comes down to at a distance.
- Digits may spend spare panel width (cap 0.72 -> 0.78 of height).
  Height is usually the binding constraint, so there was width going
  unused; longer horizontal segments read further away.
- The phase label drops to 70% brightness. It was competing with the
  countdown at equal weight; it is still plainly the phase colour.
- A 1 is centred in its cell instead of hanging off the right, so
  "11:11" no longer reads as four scattered bars. Cell widths are
  unchanged, so the digits never jitter as the time changes. With ghosts
  on it stays right-hung, matching real hardware.

Verified across 11:11, 01:01, 10:00, 179:59, 09:08 and 00:08 on 64x32
and 128x32, and on every phase colour. Goldens regenerated.

Co-Authored-By: Claude Opus 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_01AtZDGDrzbWdK4JxUeH4hk6
Picks up the parts of the requested UI spec that a display-only panel can
actually carry.

- Session pips gain a third state. Completed are solid, upcoming are
  hollow outlines, and the session you are in is picked out and slowly
  pulses, so the row reads as "two done, on the third, one to go"
  instead of just a count. Hollow needs 3px, so smaller pips fall back
  to a dim fill.
- Task label. Set it from Home Assistant via a new MQTT text entity, or
  publish to <base>/task/set (32 chars, empty clears). It takes the
  label row rather than fighting for a second one on a 32px panel --
  the phase is already carried by the colour. Survives phase changes,
  since it is what you are working on rather than which phase you are
  in; a per-session label from a start command still wins, and RESET
  clears it. Discovery goes from 17 entities to 18.
- Calm colour theme (warm terracotta, sage, soft indigo) as a preset
  alongside the punchy defaults, and paused_style: desaturate, which
  drains the phase's own hue rather than switching to amber, so a held
  timer reads as halted without changing which phase you are in.
- A label sent with START to an already-running timer is now applied
  rather than silently dropped. It still emits no "started" event.

Not built, and why: a circular progress ring around the readout is not
viable on this hardware -- enclosing the digits in a circle takes them
from 16px wide to 1px on 128x32, and from 27 to 8 on 128x64. The
existing rectangular perimeter is that same gauge fitted to the panel's
aspect ratio. Hero buttons, tabs, task drawers and settings icons need
an input device the panel does not have; those controls already exist as
Home Assistant entities.

37 tests. Goldens regenerated for the hollow pips.

Co-Authored-By: Claude Opus 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_01AtZDGDrzbWdK4JxUeH4hk6
…rflow

Two things turned up when measuring rather than eyeballing.

Antialiasing. PIL antialiases TrueType text, so the phase label carried a
grey halo of partial-brightness pixels -- 26 to 36 distinct colours in a
frame that should have six. The label face is a pixel font rendered at
integer sizes, so every one of those intermediate values was artefact,
not design. The label now goes through a thresholded mask and is painted
one flat colour. A frame is back to exactly its palette: background,
accent, the 70% label, the 14% ring track, the 28% upcoming pip, and the
lightened ring head.

Overflow below ~16px tall. _render_stacked floored the countdown box at
6px, so once the label had taken its row on a short panel the digits were
laid out past the bottom edge -- 18 of 168 sampled shapes drew off-panel,
all at heights 8 to 14. The countdown is the point, so the label is now
dropped when both cannot fit, and the box is clamped to the space that
actually exists rather than floored. The harness only samples down to
32px tall, so nothing in CI would have caught this.

Swept 256 shapes from 32x8 to 384x128: no overflow, no crashes, no blank
frames. Two regression tests pin both properties, and the goldens are
regenerated for the crisper label.

Co-Authored-By: Claude Opus 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_01AtZDGDrzbWdK4JxUeH4hk6
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.

1 participant