consolidate(flights): map width, debug images, log volume, committed artifacts - #314
Conversation
Noticed on a live rig: the flight map came back at two different sizes.
Vegas asks a plugin for a narrower render so a layout built for the full panel
does not read as sparse in the ticker. It normally delivers that by narrowing
the shared canvas for the duration of the call, which a plugin sizing itself
from matrix.width picks up with no changes of its own. But it cannot narrow
the canvas in offscreen mode -- _render_at swaps the shared canvas, which is
unsafe there -- so it only sets the hint, and notes that a plugin reading
get_vegas_render_width() still gets the narrow size while one that reads only
matrix.width "renders full width and is trimmed instead".
This plugin relied on the canvas being narrowed; its own docstring said the
projection scales "without any extra plumbing". Measured over two hours on the
rig, that assumption held 3 times out of 12:
Native: requesting 256px instead of 512px
Native: SUCCESS - 1 images, 512px total width <- cropped afterwards
Native: SUCCESS - 1 images, 256px total width
so which width came back depended on a path the plugin cannot see, and the
full-width renders composed a map twice the needed width to have most of it
thrown away.
The width property now reads the hint. That covers both paths and falls back
to the panel width outside a Vegas request, so the rotation is unaffected. The
composite cache is already keyed by (width, height) and its comment already
says "Rotation and Vegas render the same map at different widths", so this
adds a cache entry rather than invalidating one.
No plugin in the repo read get_vegas_render_width() before this, despite core
providing and documenting it. Nine other plugins currently render full-width
frames that the adapter then trims by 56-83%; they are candidates for the same
treatment but each needs its own layout thought, so this fixes the one with a
measured problem.
test_vegas_render_width.py asserts the width follows the request, returns to
the panel width afterwards, and falls back on a nonsense hint, plus that the
cache still keys on size. Mutation-checked: reverting to matrix.width fails it.
The harness sets both display_manager and _display_manager_ref deliberately:
this plugin keeps the private name while BasePlugin stores the same object
under the public one, and get_vegas_render_width()'s fallback reads the public
one. A stub with only the private name makes the fallback miss the matrix and
return its hard-coded 128, which looks exactly like the property being broken.
_get_map_background() saved two PNGs every time it composed a map, with
nothing gating it -- no debug flag, no config check -- under a comment reading
"Debug: Save composite image to see what's happening". Left-over debugging
shipped as production behaviour.
Measured on a live rig:
debug_composite.png 5.36 MB
debug_cropped.png 0.04 MB
composites in 6h 5
That is roughly 108 MB a day written to an SD card for files nothing reads, on
a device whose cards have already failed twice with unreadable-block-device
symptoms. It also gets worse, not better, with the change that makes this
plugin honour the ticker's narrower render request: composing at two widths
instead of one roughly doubles the count.
They also landed in the process's working directory, which is the install
root, where a pre-commit hook has previously swept them into a commit.
Now written only when debug logging is enabled, and into the plugin's own tile
cache directory rather than wherever the process happens to be running.
For comparison, the health-record churn fixed in the core repo was ~6,300
small writes a day; this is 108 MB of large ones from a single plugin.
test_no_debug_image_writes.py walks the module's AST and asserts every save()
sits under an isEnabledFor(DEBUG) guard and that no bare working-directory
path remains. Mutation-checked: ungating the writes fails two checks, and
restoring the bare CWD path fails another. All 10 flights suites pass.
The flight tracker logged two unconditional INFO lines on every poll -- one naming the aircraft count it was about to process, one summarising the result. Polls run about every five seconds, so on a live rig that was 686 lines per half hour, roughly 86% of the device's entire log volume, and a steady trickle of journal writes to the SD card for a line that mostly repeated itself. The "Processing N aircraft" line is trace: the summary immediately below reports the same total. Demoted to debug, with lazy %-args so a disabled level costs nothing to skip. The summary is worth keeping, so it is now reported when it changes. Which fields to key on mattered more than expected. Total and With-position jitter on almost every poll as distant traffic drifts through the receiver's edge, and keying on the whole line collapsed 343 samples to 210 -- barely worth doing. Keying on what the plugin actually displays, aircraft in range and aircraft tracked, collapses the same samples to 67. The jittery counts still ride along in the message, where they cost nothing. A 300-second heartbeat keeps a quiet sky from looking like a stalled tracker. Measured by replaying 343 real polls captured from a running rig through the committed logic: 686 lines become 67, a 90% reduction, with the heartbeat never needing to fire. Safety harness passes at all eight sizes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01STMbQE4YctTacQXfbYqKuW
debug_composite.png and debug_cropped.png are output, not assets. The map
composer writes them with Path("debug_composite.png") -- a relative path, so
on an install they land in the process working directory, which is the core
checkout. A running rig accordingly carries two untracked files in its
LEDMatrix repo, and copies of them were committed here as well.
Untracked files in a checkout are harmless until upstream adds a file at the
same path, at which point the pull refuses. There is no reason to leave that
waiting.
Removed and added to .gitignore so they cannot come back. #305 is what stops
them being written at all unless debug logging is on, and puts them under the
tile cache directory rather than the CWD; this only clears what was already
committed.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01STMbQE4YctTacQXfbYqKuW
…huckBuilds/ledmatrix-plugins into consolidate/flights # Conflicts: # plugins/ledmatrix-flights/manifest.json
…uilds/ledmatrix-plugins into consolidate/flights # Conflicts: # plugins.json # plugins/ledmatrix-flights/manifest.json
….com/ChuckBuilds/ledmatrix-plugins into consolidate/flights # Conflicts: # plugins.json # plugins/ledmatrix-flights/manifest.json
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 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. Comment |
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 26 |
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.
ChuckBuilds
left a comment
ChuckBuilds
left a comment
There was a problem hiding this comment.
Reviewed this because I'd duplicated part of it (#321, now closed) and wanted to be sure I understood what it already covers. One finding, and it turns out to be systemic rather than specific to this PR.
The manifest version has no changelog entry
main: version 1.12.13 versions[0] = 1.12.13 ✓
#314: version 1.12.17 versions[0] = 1.12.13 ✗
The version jumps four patches — reasonable for a consolidation of #304, #305, #307 and #312 — but the versions array is untouched, so the store will offer 1.12.17 with the release notes of 1.12.13. Four PRs' worth of changes ship undescribed.
CLAUDE.md asks for the two to stay in step: "Add the new entry at the top of the versions array; keep version in sync with that top entry."
Why CI didn't catch it
check_manifest_version_fields.py passes on this branch:
OK: 1 plugin(s) checked, newest version entries are current.
It validates that versions[0] uses ledmatrix_min_version rather than the deprecated spelling — it never compares version against versions[0].version. So this class of drift is invisible to the gate.
It's already happened eight times
On main today:
| plugin | version | top changelog entry |
|---|---|---|
| afl-scoreboard | 1.9.5 | 1.9.2 |
| baseball-scoreboard | 1.28.3 | 1.28.1 |
| basketball-scoreboard | 1.16.6 | 1.16.2 |
| football-scoreboard | 2.19.1 | 2.18.1 |
| hockey-scoreboard | 1.13.3 | 1.13.1 |
| lacrosse-scoreboard | 1.12.5 | 1.12.1 |
| nrl-scoreboard | 1.9.5 | 1.9.2 |
| soccer-scoreboard | 2.12.6 | 2.12.2 |
So this PR isn't doing anything unusual — it's hitting a gap that's been quietly accumulating. Users on those eight plugins see updates with stale or missing notes.
Suggestion
Adding version == versions[0].version to the gate would close it, but be aware of the consequence: the gate only inspects changed plugins, so it would start failing the next PR that touches any of those eight until the missing entries are backfilled. That may well be what you want — it forces the debt to be paid where it was incurred — but it's a CI-behaviour change affecting other people's PRs, so I haven't made it unilaterally. Say the word and I'll add it, either as a hard failure or as a warning first.
For this PR specifically, a 1.12.17 entry summarising the four consolidated changes would fix the immediate case.
Everything else here checks out — and it supersedes my #321, which I've closed: your debug-image fix is the better one, since it also relocates the files out of the process working directory rather than only gating them.
2 participants
Consolidates #304, #305, #307 and #312 into one PR. All four touch only
ledmatrix-flights, and merging them separately meant resolving the samemanifest.json/plugins.jsonconflict four times for one plugin. Here it is one version bump.No code conflicts — every conflict when combining them was version bookkeeping. The four changes are in different parts of the file and compose cleanly.
Verified on the combined branch, not assumed:
Final version 1.12.17.
Closes #304, closes #305, closes #307, closes #312.