Skip to content

fix(ci): report flash and static RAM usage separately - #28789

Merged
dakejahl merged 9 commits into
mainfrom
dakejahl/flash-ram-analysis
Sep 22, 2026
Merged

dakejahl merged 9 commits into
mainfrom
dakejahl/flash-ram-analysis

Conversation

@dakejahl

@dakejahl dakejahl commented Sep 22, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Report separate flash and static RAM deltas while retaining Bloaty's section and symbol breakdown.

Problem

The current headline treats total VM growth as flash growth. For #28760, FMUv5x shows +7,608 B even though flash grows by +3,128 B and static RAM by +4,480 B.

Solution

Measure the flash load image and RAM allocations separately from ELF section headers, with no per-target constants: sections with VMA == LMA execute from flash, and everything else allocated counts as RAM. Initialized data and RAM code count in both totals, flash includes padding exactly as objcopy -O binary does, and debug information is excluded. Program headers aren't used because, depending on ld's page size, the first LOAD segment can include the ELF header below the flash origin. Growth over 100 B is marked yellow and over 1000 B red; savings over 100 B are marked green. The Bloaty breakdown is limited to VM sizes, which drops the debug and symbol table rows. A new analysis comment is posted only when flash or static RAM changes on a target. Existing comments are updated even when every delta returns to zero.

PRs are diffed against the first parent of the merge commit instead of the base branch tip. If the PR merged while the job was queued, the tip already contains it, which caused the near-zero diffs seen after merge.

Validated with eight ARM ELF tests, flash totals matching the .bin for ark_fmu-v6x_default and px4_fmu-v6x_flash-analysis, and the CI bloaty-action image with --domain=vm.

Comment preview: #28760 linker totals; breakdown from a local fmu-v6x change adding a 4 KiB buffer

🔎 Flash and RAM Analysis

Target Flash change Static RAM change
px4_fmu-v5x 🔴 +3,128 B (+0.15%) 🔴 +4,480 B (+3.82%)
px4_fmu-v6x 🔴 +3,120 B (+0.16%) 🔴 +4,480 B (+3.95%)
px4_fmu-v6x — Bloaty section/symbol breakdown
     VM SIZE    
 -------------- 
  +4.0% +4.00Ki    .bss
    [NEW] +4.00Ki    probe_bss
  +0.0%    +360    .text
    [NEW]    +256    probe_data
     +11%    +108    ver_main
     +44%      +4    g_nullstring
   +10.0%      +2    DEFAULT_NETMAN_CONFIG
   -33.3%      -2    sz_ver_hw_str
    -0.0%      -4    [section .text]
    -0.5%      -4    up_dshot_init
  +0.2% +4.35Ki    TOTAL

Bloaty's VM total includes zero-initialized RAM and understates the combined cost of data and code copied from flash into RAM. Report the flash load image and static RAM allocations independently so the PR headline reflects each resource's actual change.

Assisted-by: Codex
@github-actions github-actions Bot added kind:bug Something is broken or behaving incorrectly. scope:infrastructure CI workflows, checks, automation, or build infrastructure. kind:test Adds or improves tests. scope:tools Scripts, developer tools, packaging, or setup helpers. scope:testing Unit, integration, fuzzing, or test data. labels Sep 22, 2026
@JonasPerolini

Copy link
Copy Markdown
Contributor

Thanks for this PR @dakejahl!

Claude review on behalf of @JonasPerolini

Tools/ci/firmware_size.py:29 — the flash total is derived from PT_LOAD segments, which makes it depend on ld's page-size heuristic. When the first section leaves room in its page, ld puts the ELF header into the first LOAD segment, so the segment LMA starts below the flash origin. With binutils 2.34 (GCC 9 2020-q2) and the fmu-v5x flash-analysis scripts (origin 0x08008000, 64 KB max page size) the first LOAD is at 0x08000000, the region check raises "load image outside the configured flash region", and 5 of 7 tests fail. It passes on 2.42 (px4-dev container) only because the default max page size there is 4 KB; relinking on 2.34 with -z max-page-size=0x1000 makes the failure disappear. Suggest measuring like objcopy does (ALLOC+LOAD sections via objdump -h, or just stat the .bin the build already writes) or clamping the segment start at the flash origin, plus a test that links the fixture with -z max-page-size=0x10000 to cover this layout.

Tools/ci/test_firmware_size.py:117 — the CLI test runs python3 from PATH rather than sys.executable. The script needs 3.9+ for dict[str, int], so this test fails on machines whose default python3 is older while the rest of the suite passes under a newer interpreter.

@mbjd

mbjd commented Sep 22, 2026

Copy link
Copy Markdown
Contributor

Thanks! While we're at it:

  • Often after a PR is merged the flash diff is updated to basically zero, such as here. Any idea why that is?
  • WDYT about making it yellow if >100B, red if >1000B, green if <-100B or similar?
  • We could also suggest an AI review with a prompt focused on identifying added flash bloat, or provide a link to directly provide that review, e.g. with https://claude.ai/new?q=<prompt> et al.

The base branch tip is fetched when the job runs, so a PR merged while its job was queued was compared against itself and reported a near-zero change, and commits landed in the meantime showed up as reverted.
ld maps the ELF header into the first LOAD segment when its max page size exceeds the flash origin's alignment (binutils 2.34 defaults to 64 KiB), so segment-based accounting rejected valid firmware as outside the flash region.
Growth over 100 B and 1000 B gets yellow and red markers, savings over 100 B green, so reviewers can spot size regressions without reading the numbers.
The matrix duplicated each board's flash-analysis linker region, which would drift silently. Section headers alone distinguish code executing in place (VMA == LMA) from data copied to or reserved in RAM, and the image span matches objcopy -O binary.
Debug, symbol and string table rows only change the file size and made up most of the comment.
@dakejahl

Copy link
Copy Markdown
Contributor Author

We could also suggest an AI review with a prompt focused on identifying added flash bloat, or provide a link to directly provide that review, e.g. with https://claude.ai/new?q= et al.

This would be cool. Deferring until we setup the infrastructure for automated AI reviews.

@dakejahl
dakejahl requested a review from mbjd September 22, 2026 19:10
A push that puts flash and static RAM back to zero was still rewriting the sticky comment, so a PR with no size change carried a zero report. Remove that comment instead. Fork tokens cannot delete it, so those PRs hand a delete artifact to the poster.

Assisted-by: Grok:grok-4.7
Signed-off-by: Jacob Dahl <dahl.jakejacob@gmail.com>
@PX4 PX4 deleted a comment from github-actions Bot Sep 22, 2026
Existing reports can be updated when size deltas return to zero, avoiding a separate privileged deletion path.

Signed-off-by: Jacob Dahl <dahl.jakejacob@gmail.com>
Assisted-by: Codex:gpt-6
@dakejahl
dakejahl requested a review from julianoes September 22, 2026 20:21

@julianoes julianoes 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.

Nice!

@dakejahl
dakejahl merged commit 1d761cd into main Sep 22, 2026
73 of 77 checks passed
@dakejahl
dakejahl deleted the dakejahl/flash-ram-analysis branch September 22, 2026 22:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind:bug Something is broken or behaving incorrectly. kind:test Adds or improves tests. scope:infrastructure CI workflows, checks, automation, or build infrastructure. scope:testing Unit, integration, fuzzing, or test data. scope:tools Scripts, developer tools, packaging, or setup helpers.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants