chore: add firmware size history script#1235
chore: add firmware size history script#1235znelson merged 5 commits intocrosspoint-reader:masterfrom
Conversation
📝 WalkthroughWalkthroughAdds a new CLI Python script that builds firmware across selected git commits (range or explicit list), collects per-commit flash usage by running PlatformIO builds, computes deltas, and emits results as an aligned table or CSV while preserving and restoring working state. Changes
Sequence Diagram(s)sequenceDiagram
participant Script as firmware_size_history.py
participant Git as Git
participant FS as Filesystem
participant PIO as PlatformIO
participant Out as OutputHandler
Script->>Git: Resolve refs & build commit list
Script->>Git: Record current ref
Script->>FS: Stash uncommitted changes (if any)
loop For each commit
Script->>Git: Checkout commit (detached HEAD)
Script->>PIO: Run build (pio run -e <env>)
PIO->>Script: Return build output
Script->>Script: Parse flash usage (regex) & record success/failure
Script->>Script: Compute delta vs previous commit
end
Script->>Git: Checkout original ref
Script->>FS: Restore stashed changes (if any)
Script->>Out: Write results (table or CSV)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
|
Sample output: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
scripts/firmware_size_history.py (1)
161-161: Consider list unpacking for cleaner syntax.Using list unpacking is more idiomatic than concatenation.
♻️ Suggested change
- all_commits = [(start_sha, start_title)] + commits + all_commits = [(start_sha, start_title), *commits]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/firmware_size_history.py` at line 161, The current construction all_commits = [(start_sha, start_title)] + commits uses concatenation; replace it with list unpacking to be more idiomatic by creating all_commits using a single list literal that unpacks commits, e.g., start the list with (start_sha, start_title) followed by *commits—update the assignment where all_commits is defined to use this unpacking pattern and keep the rest of the code that consumes all_commits unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/firmware_size_history.py`:
- Around line 133-136: The f-string uses a backslash escape inside the
expression which is invalid on Python <3.12; define a module-level constant like
BOX_CHAR = "─" (or BOX_CHAR = "\u2500") and update the formatting in
format_table() to build sep using BOX_CHAR * COL_COMMIT, BOX_CHAR * COL_FLASH,
BOX_CHAR * COL_DELTA and BOX_CHAR * 40 (e.g. f"{BOX_CHAR * COL_COMMIT}
{BOX_CHAR * COL_FLASH} {BOX_CHAR * COL_DELTA} {BOX_CHAR * 40}") so no
backslash appears inside f-string expressions and compatibility with Python 3.8+
is preserved.
---
Nitpick comments:
In `@scripts/firmware_size_history.py`:
- Line 161: The current construction all_commits = [(start_sha, start_title)] +
commits uses concatenation; replace it with list unpacking to be more idiomatic
by creating all_commits using a single list literal that unpacks commits, e.g.,
start the list with (start_sha, start_title) followed by *commits—update the
assignment where all_commits is defined to use this unpacking pattern and keep
the rest of the code that consumes all_commits unchanged.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
scripts/firmware_size_history.py
📜 Review details
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-02-23T19:55:03.054Z
Learnt from: martinbrook
Repo: crosspoint-reader/crosspoint-reader PR: 1136
File: scripts/patch_jpegdec.py:20-23
Timestamp: 2026-02-23T19:55:03.054Z
Learning: In the crosspoint-reader project, enable Python linters (e.g., Ruff or Flake8) in CI for all Python scripts. Run linting as part of the CI workflow and fix reported issues to maintain code quality. Do not skip linting for any Python files.
Applied to files:
scripts/firmware_size_history.py
🪛 Ruff (0.15.2)
scripts/firmware_size_history.py
[error] 42-42: subprocess call: check for execution of untrusted input
(S603)
[error] 88-88: subprocess call: check for execution of untrusted input
(S603)
[error] 89-89: Starting a process with a partial executable path
(S607)
[warning] 161-161: Consider [(start_sha, start_title), *commits] instead of concatenation
Replace with [(start_sha, start_title), *commits]
(RUF005)
🔇 Additional comments (6)
scripts/firmware_size_history.py (6)
1-38: LGTM!Clear documentation with usage examples, and standard library imports are appropriate. The regex pattern correctly captures PlatformIO's flash usage output format.
41-64: LGTM!The helper functions are well-structured. The
resolve_reffunction properly validates refs before proceeding and provides clear error messages. The fallback fromsymbolic-reftorev-parseingit_current_refcorrectly handles both branch and detached HEAD states.Regarding the static analysis hint (S603) about subprocess with untrusted input: this is acceptable for a developer tool that deliberately invokes git commands with user-provided refs.
67-92: LGTM!Good implementation choices:
--detachprevents unintentional branch movement, and combining stdout/stderr captures all PlatformIO output for parsing. Usingpioas a partial executable path (S607) is appropriate since PlatformIO is expected to be in the developer's PATH.
95-107: LGTM!Clean implementation. Returning
Nonefor missing flash data allows the caller to handle failures gracefully, andcsv.DictWriterensures proper CSV formatting.
173-256: LGTM!Excellent defensive design:
- Refs are validated before any working tree modifications
- Auto-stash prevents losing uncommitted work
try/finallyensures restoration regardless of success/failureKeyboardInterruptis handled gracefully with partial resultscheck=Falseon restore operations avoids masking the real issue
258-290: LGTM!The delta calculation correctly skips failed builds while preserving the last valid measurement for subsequent comparisons. Output handling cleanly supports both file and stdout CSV modes alongside the default table format.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
scripts/firmware_size_history.py (2)
250-257: Excellent interrupt handling; consider warning on stash pop failure.The
KeyboardInterrupthandling andfinallycleanup are well implemented. However, ifgit stash popfails (e.g., due to conflicts), the user receives no indication that their changes remain stashed.✨ Optional improvement to warn on stash restoration failure
if stash_needed: print("[info] Restoring stashed changes...", file=sys.stderr) - run(["git", "stash", "pop"], check=False) + result = run(["git", "stash", "pop"], check=False) + if result.returncode != 0: + print("[warn] Failed to restore stash. Run 'git stash list' to recover.", file=sys.stderr)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/firmware_size_history.py` around lines 250 - 257, When restoring stashed changes in the finally block (use of stash_needed and run(["git","stash","pop"], check=False)), capture the CompletedProcess result and check result.returncode; if non‑zero, print a clear warning to stderr (including that stash pop failed and local changes may still be stashed or there may be conflicts) so users know to inspect the stash manually. Ensure the message references the operation ("git stash pop") and suggests next steps (e.g., run "git stash list" / "git stash pop" manually).
162-162: Consider list unpacking for cleaner syntax.Per Ruff's suggestion, list unpacking is more idiomatic than concatenation.
✨ Optional style improvement
- all_commits = [(start_sha, start_title)] + commits + all_commits = [(start_sha, start_title), *commits]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/firmware_size_history.py` at line 162, Replace the concatenation used to build all_commits with list unpacking for clearer, more idiomatic syntax: instead of building all_commits via (start_sha, start_title) concatenated with commits, create a new list that begins with (start_sha, start_title) and expands commits using the splat operator (refer to the variables all_commits, start_sha, start_title, and commits to locate the code).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@scripts/firmware_size_history.py`:
- Around line 250-257: When restoring stashed changes in the finally block (use
of stash_needed and run(["git","stash","pop"], check=False)), capture the
CompletedProcess result and check result.returncode; if non‑zero, print a clear
warning to stderr (including that stash pop failed and local changes may still
be stashed or there may be conflicts) so users know to inspect the stash
manually. Ensure the message references the operation ("git stash pop") and
suggests next steps (e.g., run "git stash list" / "git stash pop" manually).
- Line 162: Replace the concatenation used to build all_commits with list
unpacking for clearer, more idiomatic syntax: instead of building all_commits
via (start_sha, start_title) concatenated with commits, create a new list that
begins with (start_sha, start_title) and expands commits using the splat
operator (refer to the variables all_commits, start_sha, start_title, and
commits to locate the code).
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
scripts/firmware_size_history.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build
- GitHub Check: cppcheck
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-02-23T19:55:03.054Z
Learnt from: martinbrook
Repo: crosspoint-reader/crosspoint-reader PR: 1136
File: scripts/patch_jpegdec.py:20-23
Timestamp: 2026-02-23T19:55:03.054Z
Learning: In the crosspoint-reader project, enable Python linters (e.g., Ruff or Flake8) in CI for all Python scripts. Run linting as part of the CI workflow and fix reported issues to maintain code quality. Do not skip linting for any Python files.
Applied to files:
scripts/firmware_size_history.py
🪛 Ruff (0.15.2)
scripts/firmware_size_history.py
[error] 43-43: subprocess call: check for execution of untrusted input
(S603)
[error] 89-89: subprocess call: check for execution of untrusted input
(S603)
[error] 90-90: Starting a process with a partial executable path
(S607)
[warning] 162-162: Consider [(start_sha, start_title), *commits] instead of concatenation
Replace with [(start_sha, start_title), *commits]
(RUF005)
🔇 Additional comments (12)
scripts/firmware_size_history.py (12)
1-39: LGTM!The docstring is comprehensive with clear usage examples. The
BOX_CHARconstant properly addresses Python 3.8+ compatibility by keeping backslash escapes outside f-string expressions.
42-46: LGTM!Clean subprocess wrapper. The static analysis warning (S603) about untrusted input is a false positive here—this is a local developer tool where the user explicitly provides refs via command line.
49-57: LGTM!Proper error handling with user-friendly messages when ref resolution fails.
60-65: LGTM!Correctly handles both attached branch and detached HEAD states for proper restoration.
68-80: LGTM!Clean parsing of git log output with proper handling of commit titles containing spaces.
83-93: LGTM!Using
--detachfor checkout is appropriate for arbitrary commit navigation. The partial path forpio(S607) is acceptable—developers using this tool will have PlatformIO in their PATH.
96-108: LGTM!Clean implementations for regex parsing and CSV output.
111-149: LGTM!Good table formatting with proper handling of FAILED builds and empty deltas. The
BOX_CHARconstant usage maintains Python 3.8+ compatibility.
167-171: LGTM!Clean implementation with proper pluralization in the description.
174-219: LGTM!Excellent defensive design—validating refs before touching the working tree prevents stranding stashed changes on bad input.
259-274: LGTM!The delta calculation correctly handles failed builds by preserving the last successful size for comparison, which provides meaningful deltas even when some commits fail to build.
276-291: LGTM!Clean output handling for both CSV and table formats with proper file handling.
…actors) Merges upstream commits: - fix: fixed-point x-advance and kerning for text layout (crosspoint-reader#1168) - chore: add firmware size history script (crosspoint-reader#1235) - refactor: binary search for font lookups (crosspoint-reader#1202) - perf: remove unused ConfirmationActivity member (crosspoint-reader#1234) Resolved conflicts in ubuntu_10/12 bold/regular font headers by accepting upstream regenerated values (local had no changes to these files).
3 participants
Summary
What is the goal of this PR?
scripts/firmware_size_history.py, a developer tool that builds firmware at selected git commits and reports flash usage with deltas between them.--range START ENDto walk every commit in a range, or--commits REF [REF ...]to compare specific refs (which can span branches).--csvfor machine-readable output to stdout or--csv FILEto write to a file.AI Usage
While CrossPoint doesn't have restrictions on AI tools in contributing, please be transparent about their usage as it
helps set the right context for reviewers.
Did you use AI tools to help write this code? YES, fully written by AI