From 27e6322c2cb5896fc38ac828e94ee0b62c81134d Mon Sep 17 00:00:00 2001 From: Ray Morris Date: Sun, 16 Aug 2026 13:00:23 -0500 Subject: [PATCH 1/6] docs: seed RAM/flash optimization guide with PR #11718 technique Stub only -- captures the buffer-decoupling and shrink-audit technique from the MSP tunnel reply buffer fix before it's lost, ahead of the tracked document-ram-flash-optimization-practices project writing the full guide and linking it from Development.md. --- .../development/ram-and-flash-optimization.md | 72 +++++++++++++++++++ 1 file changed, 72 insertions(+) create mode 100644 docs/development/ram-and-flash-optimization.md diff --git a/docs/development/ram-and-flash-optimization.md b/docs/development/ram-and-flash-optimization.md new file mode 100644 index 00000000000..f13eae8bef3 --- /dev/null +++ b/docs/development/ram-and-flash-optimization.md @@ -0,0 +1,72 @@ +# RAM and Flash Optimization + +> **Status: seed draft.** This is a stub started while fixing a RAM overflow +> on PR #11718, so the technique wouldn't get lost before the full guide is +> written. It is not yet linked from `Development.md` and is not a complete +> guide — see the tracked project for the rest of this doc's scope. + +Static RAM headroom is a binding constraint on several boards (128KB-RAM +F4 targets in particular). Treat RAM and flash reduction as an active goal +when writing or reviewing code that adds static buffers, not just something +to check for overflow after the fact. + +## Don't size a buffer for a different subsystem's worst case + +If a buffer is reused across features, check what its size actually derives +from before assuming a new consumer needs the same size. On PR #11718 (the +MSP-over-MAVLink tunnel), a reply buffer was sized off `MSP_PORT_OUTBUF_SIZE` +(4112 bytes under `USE_FLASHFS`) simply because that was the constant already +in scope for MSP replies. But that size exists to let `MSP_DATAFLASH_READ` +return a full 4096-byte flash page in one shot for fast blackbox downloads +over USB/serial — the tunnel's own use case (MSP replies fragmented into +128-byte MAVLink `TUNNEL` payload chunks regardless of source buffer size) +never benefited from that size at all. Giving the tunnel its own, +independently-sized constant (512 bytes, matching the size every +non-FLASHFS board already uses for ordinary MSP replies) cut ~3.6KB with no +functional loss — data that used to arrive in one large read now arrives in +more, smaller reads, which costs nothing extra when it's already being +fragmented for the wire. + +**Before reusing an existing size constant for a new buffer, ask what +specifically drove that number** — it may be tied to an unrelated worst +case that doesn't apply to the new use. + +## When shrinking a shared buffer, audit what else writes into it + +A buffer's "extra" size can be silently load-bearing for a bug elsewhere, +not just wasted space. While sizing the tunnel buffer above, auditing every +MSP command reachable through it surfaced `serializeDataflashReadReply()` +computing its available-space clamp *before* writing a 4-byte address +header, then writing `address + clamped_data` — an unconditional 4-byte +overflow whenever a request is large enough to hit the clamp. On the +original 4112-byte buffer this was practically unreachable (16 bytes of +incidental headroom absorbed it for a standard 4096-byte request); shrinking +to 512 bytes with no equivalent headroom made it trivially reachable by any +normal-sized request. + +**When shrinking a buffer other code paths also write into, check whether +any of those paths were relying on the buffer being larger than their own +worst-case write** — not just whether the new size covers the new +consumer's own worst case. Fix the root cause (the clamp math, here) rather +than padding the buffer back up to paper over it. + +## Survey actual message/data sizes before assuming a buffer needs to be large + +"How big does this actually need to be" is often answerable by grep, not +guesswork — but match the audit's scope to everything that can actually +reach the buffer. A first pass on the tunnel buffer above checked only MSP2 +command handlers and found 432 bytes as the largest reply; widening to MSP1 +(legacy) handlers — reachable through the same buffer — found 512 bytes +exactly (`MSP_LED_STRIP_CONFIG`, no bounds check, no margin). Scoping an +audit to "the obviously relevant" category of code instead of everything +that can reach the buffer produces a confident-looking wrong answer. + +## Related + +- Full strategy pattern library (more entries, other PRs): + `claude/projects/active/ram-reduction-program/ram-flash-strategies.md` in + the `inav-claude` tooling repo (not part of this firmware repo). +- Tracked follow-up: `document-ram-flash-optimization-practices` project — + expands this into a full guide, links it from `Development.md`, and + updates review checklists to treat RAM/flash reduction as an active + review category. From 75aec881e7c327dc8e26ecbb601f5cb1f96de618 Mon Sep 17 00:00:00 2001 From: Ray Morris Date: Mon, 17 Aug 2026 15:20:25 -0500 Subject: [PATCH 2/6] Add CI PR comment for RAM/flash usage delta vs. base branch Surfaces RAM/flash regressions and creep on every PR instead of only via a hard CI failure (motivated by PR #11718's terrain-cache RAM overflow, which was only caught because CI happened to fail on that specific board). Extracts flash/RAM usage from each build's .elf right after compiling (no second build of the base branch), persists a baseline per branch as a release asset in the pr-test-builds companion repo, and diffs 4 representative targets (one per MCU family) against it on PR builds. --- .github/scripts/extract-size-report.sh | 49 ++++ .github/scripts/merge-size-reports.sh | 19 ++ .github/scripts/size-diff-comment.js | 101 ++++++++ .github/scripts/size-diff-comment.test.js | 294 ++++++++++++++++++++++ .github/workflows/README.md | 27 ++ .github/workflows/ci-size-report.yml | 193 ++++++++++++++ .github/workflows/ci.yml | 49 ++++ 7 files changed, 732 insertions(+) create mode 100755 .github/scripts/extract-size-report.sh create mode 100755 .github/scripts/merge-size-reports.sh create mode 100644 .github/scripts/size-diff-comment.js create mode 100644 .github/scripts/size-diff-comment.test.js create mode 100644 .github/workflows/ci-size-report.yml diff --git a/.github/scripts/extract-size-report.sh b/.github/scripts/extract-size-report.sh new file mode 100755 index 00000000000..87f810154d1 --- /dev/null +++ b/.github/scripts/extract-size-report.sh @@ -0,0 +1,49 @@ +#!/bin/bash +# +# Run the size tool on every built .elf and write a small JSON report of +# flash/RAM usage per target, consumed by ci-size-report.yml. +# +# Usage: extract-size-report.sh [size-tool] +# +# flash = .text + .data (what's programmed into flash) +# ram = .data + .bss (what's reserved in RAM at runtime) + +set -euo pipefail + +BUILD_DIR=${1:?usage: extract-size-report.sh [size-tool]} +OUTPUT_JSON=${2:?usage: extract-size-report.sh [size-tool]} +SIZE_TOOL=${3:-arm-none-eabi-size} + +# CMake's RUNTIME_OUTPUT_DIRECTORY puts built executables under +# /bin/ (see cmake/main.cmake), not directly — search +# instead of assuming a fixed depth, in case that ever changes. +mapfile -t ELFS < <(find "$BUILD_DIR" -maxdepth 3 -name '*.elf' | sort) +if [ "${#ELFS[@]}" -eq 0 ]; then + echo "::warning::No .elf files found under $BUILD_DIR, writing empty size report" + echo '{}' > "$OUTPUT_JSON" + exit 0 +fi + +ENTRIES=() +for elf in "${ELFS[@]}"; do + target=$(basename "$elf" .elf) + + # Berkeley format: " text data bss dec hex filename" + read -r text data bss _dec _hex _name < <("$SIZE_TOOL" -B "$elf" | tail -n1) + + flash=$((text + data)) + ram=$((data + bss)) + + ENTRIES+=("\"$target\":{\"flash\":$flash,\"ram\":$ram}") +done + +{ + printf '{' + printf '%s' "${ENTRIES[0]}" + for entry in "${ENTRIES[@]:1}"; do + printf ',%s' "$entry" + done + printf '}' +} > "$OUTPUT_JSON" + +echo "Wrote size report for ${#ENTRIES[@]} target(s) to $OUTPUT_JSON" diff --git a/.github/scripts/merge-size-reports.sh b/.github/scripts/merge-size-reports.sh new file mode 100755 index 00000000000..fcb79457bc9 --- /dev/null +++ b/.github/scripts/merge-size-reports.sh @@ -0,0 +1,19 @@ +#!/bin/bash +# +# Merge multiple per-shard size-report.json files (see extract-size-report.sh) +# into one aggregate JSON, used by the "upload-artifacts" job in ci.yml. +# +# Usage: merge-size-reports.sh ... + +set -euo pipefail + +OUTPUT_JSON=${1:?usage: merge-size-reports.sh ...} +shift + +if [ "$#" -eq 0 ]; then + echo '{}' > "$OUTPUT_JSON" + exit 0 +fi + +jq -s 'add' "$@" > "$OUTPUT_JSON" +echo "Merged $# size report(s) into $OUTPUT_JSON" diff --git a/.github/scripts/size-diff-comment.js b/.github/scripts/size-diff-comment.js new file mode 100644 index 00000000000..0d66311cf13 --- /dev/null +++ b/.github/scripts/size-diff-comment.js @@ -0,0 +1,101 @@ +// Pure logic for the "PR RAM/Flash usage delta" comment: diffs a PR's +// size-report.json against a base-branch baseline and renders the markdown +// comment body. Deliberately has no network/filesystem/GitHub Actions +// dependency so it can be unit tested directly and reused (via require) +// from the actions/github-script step in ci-size-report.yml. +// +// Report shape: { "": { "flash": , "ram": }, ... } + +'use strict'; + +const REPRESENTATIVE_TARGETS = ['MATEKF405', 'MATEKF722', 'MATEKF765', 'MATEKH743']; + +// Below this magnitude a delta is noise (rounding/toolchain jitter), not a +// real change worth calling out. +const NOISE_THRESHOLD_BYTES = 32; + +function formatDelta(deltaBytes, baseBytes) { + const sign = deltaBytes > 0 ? '+' : deltaBytes < 0 ? '' : '±'; + const pct = baseBytes > 0 ? (deltaBytes / baseBytes) * 100 : 0; + const pctStr = baseBytes > 0 ? ` (${sign}${pct.toFixed(2)}%)` : ''; + return `${sign}${deltaBytes} B${pctStr}`; +} + +// Returns an array of row objects, one per representative target, each +// either a comparison row or a status row (missing from PR/baseline). +function diffSizeReports(prReport, baselineReport) { + return REPRESENTATIVE_TARGETS.map((target) => { + const pr = prReport[target]; + const base = baselineReport ? baselineReport[target] : undefined; + + if (!pr && !base) { + return { target, status: 'not-built' }; + } + if (!pr) { + return { target, status: 'missing-from-pr' }; + } + if (!base) { + return { target, status: 'no-baseline', flash: pr.flash, ram: pr.ram }; + } + + const flashDelta = pr.flash - base.flash; + const ramDelta = pr.ram - base.ram; + return { + target, + status: 'compared', + flash: pr.flash, + ram: pr.ram, + flashDelta, + ramDelta, + notable: Math.abs(flashDelta) >= NOISE_THRESHOLD_BYTES || Math.abs(ramDelta) >= NOISE_THRESHOLD_BYTES, + }; + }); +} + +// docLink: string URL to link, or null/undefined to omit the doc-link line. +function renderComment({ prReport, baselineReport, shortSha, docLink, marker }) { + const rows = diffSizeReports(prReport, baselineReport); + const lines = [marker, '**RAM / Flash usage vs. base branch** — commit `' + shortSha + '`', '']; + + if (!baselineReport) { + lines.push( + '> No size baseline is available yet for this PR\'s base branch ' + + '(first run after this feature shipped, or a new branch). ' + + 'This comment will show deltas once a baseline exists.', + '' + ); + } + + const anyComparable = rows.some((r) => r.status === 'compared' || r.status === 'no-baseline'); + if (anyComparable) { + lines.push('| Target | Flash Δ | RAM Δ |', '|---|---|---|'); + for (const row of rows) { + if (row.status === 'compared') { + const flashCell = formatDelta(row.flashDelta, row.flash - row.flashDelta); + const ramCell = formatDelta(row.ramDelta, row.ram - row.ramDelta); + const notableMark = row.notable ? ' ⚠️' : ''; + lines.push(`| ${row.target}${notableMark} | ${flashCell} | ${ramCell} |`); + } else if (row.status === 'no-baseline') { + lines.push(`| ${row.target} | ${row.flash} B (no baseline) | ${row.ram} B (no baseline) |`); + } else if (row.status === 'missing-from-pr') { + lines.push(`| ${row.target} | not built by this PR | not built by this PR |`); + } + // 'not-built': omit entirely, nothing meaningful to say + } + lines.push(''); + } else { + lines.push( + '_None of the representative targets (' + REPRESENTATIVE_TARGETS.join(', ') + ') ' + + 'were built by this PR — no size comparison to show._', + '' + ); + } + + if (docLink) { + lines.push(`See [RAM/flash optimization guide](${docLink}) for techniques to reduce usage.`, ''); + } + + return lines.join('\n').trimEnd() + '\n'; +} + +module.exports = { REPRESENTATIVE_TARGETS, NOISE_THRESHOLD_BYTES, diffSizeReports, renderComment, formatDelta }; diff --git a/.github/scripts/size-diff-comment.test.js b/.github/scripts/size-diff-comment.test.js new file mode 100644 index 00000000000..bf005ed012d --- /dev/null +++ b/.github/scripts/size-diff-comment.test.js @@ -0,0 +1,294 @@ +// Unit tests for size-diff-comment.js (RAM/Flash usage delta PR comment). +// +// Run with: node --test .github/scripts/size-diff-comment.test.js +// +// Pure-logic tests only — no filesystem/network access, matching the module +// under test. Uses Node's built-in test runner (node:test / node:assert), +// no dependencies. + +'use strict'; + +const test = require('node:test'); +const assert = require('node:assert/strict'); + +const { + REPRESENTATIVE_TARGETS, + NOISE_THRESHOLD_BYTES, + diffSizeReports, + renderComment, + formatDelta, +} = require('./size-diff-comment.js'); + +// Sanity on the fixed constants the rest of the tests assume. +test('REPRESENTATIVE_TARGETS is the expected 4 targets, NOISE_THRESHOLD_BYTES is 32', () => { + assert.deepEqual(REPRESENTATIVE_TARGETS, ['MATEKF405', 'MATEKF722', 'MATEKF765', 'MATEKH743']); + assert.equal(NOISE_THRESHOLD_BYTES, 32); +}); + +// --------------------------------------------------------------------------- +// formatDelta +// --------------------------------------------------------------------------- + +test('formatDelta: positive delta gets a + sign and correct percentage', () => { + const result = formatDelta(100, 10000); + assert.equal(result, '+100 B (+1.00%)'); +}); + +test('formatDelta: negative delta gets no extra sign (native minus) and correct percentage', () => { + const result = formatDelta(-100, 10000); + assert.equal(result, '-100 B (-1.00%)'); + // Make sure there's no double-minus like "--100" or "(-+1.00%)". + assert.ok(!result.includes('--')); +}); + +test('formatDelta: zero delta does not show a + sign', () => { + const result = formatDelta(0, 10000); + assert.ok(!result.includes('+'), `expected no "+" in zero-delta output, got: ${result}`); + assert.equal(result, '±0 B (±0.00%)'); +}); + +test('formatDelta: baseBytes of 0 omits the percentage entirely (no divide-by-zero artifact)', () => { + const result = formatDelta(50, 0); + assert.equal(result, '+50 B'); + assert.ok(!result.includes('NaN')); + assert.ok(!result.includes('Infinity')); +}); + +// --------------------------------------------------------------------------- +// diffSizeReports +// --------------------------------------------------------------------------- + +test('diffSizeReports: target present in both PR and baseline with a real delta -> compared, correct deltas', () => { + const pr = { MATEKF405: { flash: 500100, ram: 60000 } }; + const base = { MATEKF405: { flash: 500000, ram: 60200 } }; + const rows = diffSizeReports(pr, base); + const row = rows.find((r) => r.target === 'MATEKF405'); + + assert.equal(row.status, 'compared'); + assert.equal(row.flash, 500100); + assert.equal(row.ram, 60000); + assert.equal(row.flashDelta, 100); + assert.equal(row.ramDelta, -200); +}); + +test('diffSizeReports: notable is gated at exactly NOISE_THRESHOLD_BYTES (32 notable, 31 not)', () => { + const baseSizes = { flash: 100000, ram: 50000 }; + + const prAt32 = { MATEKF405: { flash: baseSizes.flash + 32, ram: baseSizes.ram } }; + const prAt31 = { MATEKF405: { flash: baseSizes.flash + 31, ram: baseSizes.ram } }; + const base = { MATEKF405: baseSizes }; + + const rowAt32 = diffSizeReports(prAt32, base).find((r) => r.target === 'MATEKF405'); + const rowAt31 = diffSizeReports(prAt31, base).find((r) => r.target === 'MATEKF405'); + + assert.equal(rowAt32.flashDelta, 32); + assert.equal(rowAt32.notable, true, 'delta of exactly 32 (the threshold) should be notable'); + + assert.equal(rowAt31.flashDelta, 31); + assert.equal(rowAt31.notable, false, 'delta of 31 (below the threshold) should not be notable'); +}); + +test('diffSizeReports: notable also triggers from ramDelta alone, and honors negative deltas via Math.abs', () => { + const base = { MATEKF405: { flash: 100000, ram: 50000 } }; + const pr = { MATEKF405: { flash: 100000, ram: 50000 - 40 } }; // flash unchanged, ram shrank by 40 + const row = diffSizeReports(pr, base).find((r) => r.target === 'MATEKF405'); + + assert.equal(row.flashDelta, 0); + assert.equal(row.ramDelta, -40); + assert.equal(row.notable, true, 'a -40 ram delta exceeds the 32-byte threshold in magnitude'); +}); + +test('diffSizeReports: target missing from PR report but present in baseline -> missing-from-pr', () => { + const pr = {}; // MATEKF405 not built this run + const base = { MATEKF405: { flash: 100000, ram: 50000 } }; + const row = diffSizeReports(pr, base).find((r) => r.target === 'MATEKF405'); + + assert.equal(row.status, 'missing-from-pr'); + assert.equal(row.flash, undefined); + assert.equal(row.ram, undefined); +}); + +test('diffSizeReports: target missing from both PR and baseline -> not-built', () => { + const pr = {}; + const base = {}; + const row = diffSizeReports(pr, base).find((r) => r.target === 'MATEKF405'); + + assert.equal(row.status, 'not-built'); +}); + +test('diffSizeReports: baselineReport undefined but target present in PR -> no-baseline', () => { + const pr = { MATEKF405: { flash: 100000, ram: 50000 } }; + const row = diffSizeReports(pr, undefined).find((r) => r.target === 'MATEKF405'); + + assert.equal(row.status, 'no-baseline'); + assert.equal(row.flash, 100000); + assert.equal(row.ram, 50000); +}); + +test('diffSizeReports: baselineReport null (not just undefined) behaves the same as undefined', () => { + const pr = { MATEKF405: { flash: 100000, ram: 50000 } }; + const row = diffSizeReports(pr, null).find((r) => r.target === 'MATEKF405'); + + assert.equal(row.status, 'no-baseline'); +}); + +test('diffSizeReports: returns exactly one row per representative target, in order', () => { + const rows = diffSizeReports({}, {}); + assert.equal(rows.length, REPRESENTATIVE_TARGETS.length); + assert.deepEqual(rows.map((r) => r.target), REPRESENTATIVE_TARGETS); +}); + +// --------------------------------------------------------------------------- +// renderComment +// --------------------------------------------------------------------------- + +function fullReport(sizes) { + // Helper: build a { target: {flash, ram} } report for all 4 representative + // targets from a flat { target: [flash, ram] } shorthand. + const report = {}; + for (const [target, [flash, ram]] of Object.entries(sizes)) { + report[target] = { flash, ram }; + } + return report; +} + +test('renderComment: baseline present, all 4 targets compared, mix of notable/non-notable', () => { + const marker = ''; + const baselineReport = fullReport({ + MATEKF405: [500000, 60000], + MATEKF722: [510000, 61000], + MATEKF765: [520000, 62000], + MATEKH743: [530000, 63000], + }); + const prReport = fullReport({ + MATEKF405: [500100, 60000], // +100 flash -> notable + MATEKF722: [510010, 61000], // +10 flash -> not notable + MATEKF765: [519960, 62000], // -40 flash -> notable + MATEKH743: [530000, 63000], // no change -> not notable + }); + + const body = renderComment({ prReport, baselineReport, shortSha: 'abc1234', docLink: null, marker }); + + // Marker present (and first line, as GitHub comment-identification markers + // are conventionally expected to be). + assert.ok(body.includes(marker)); + assert.equal(body.split('\n')[0], marker); + + // Markdown table present. + assert.ok(body.includes('| Target | Flash Δ | RAM Δ |')); + assert.ok(body.includes('|---|---|---|')); + + // Notable rows flagged with the warning emoji, non-notable rows are not. + const lines = body.split('\n'); + const matekf405Line = lines.find((l) => l.startsWith('| MATEKF405')); + const matekf722Line = lines.find((l) => l.startsWith('| MATEKF722')); + const matekf765Line = lines.find((l) => l.startsWith('| MATEKF765')); + const matekh743Line = lines.find((l) => l.startsWith('| MATEKH743')); + + assert.ok(matekf405Line.includes('⚠️'), 'MATEKF405 (+100 flash) should be flagged notable'); + assert.ok(!matekf722Line.includes('⚠️'), 'MATEKF722 (+10 flash) should NOT be flagged notable'); + assert.ok(matekf765Line.includes('⚠️'), 'MATEKF765 (-40 flash) should be flagged notable'); + assert.ok(!matekh743Line.includes('⚠️'), 'MATEKH743 (no change) should NOT be flagged notable'); + + // No "no baseline available" note when a baseline was supplied. + assert.ok(!body.includes('No size baseline is available yet')); +}); + +test('renderComment: baselineReport falsy -> includes the "no baseline available yet" note', () => { + const prReport = fullReport({ + MATEKF405: [500000, 60000], + MATEKF722: [510000, 61000], + MATEKF765: [520000, 62000], + MATEKH743: [530000, 63000], + }); + + const body = renderComment({ + prReport, + baselineReport: undefined, + shortSha: 'def5678', + docLink: null, + marker: '', + }); + + assert.ok(body.includes('No size baseline is available yet')); +}); + +test('renderComment: docLink set -> link line present', () => { + const prReport = fullReport({ MATEKF405: [500000, 60000] }); + const baselineReport = fullReport({ MATEKF405: [499000, 60000] }); + + const body = renderComment({ + prReport, + baselineReport, + shortSha: 'abc1234', + docLink: 'https://example.com/optimization-guide', + marker: '', + }); + + assert.ok(body.includes('[RAM/flash optimization guide](https://example.com/optimization-guide)')); +}); + +test('renderComment: docLink omitted -> link line absent entirely', () => { + const prReport = fullReport({ MATEKF405: [500000, 60000] }); + const baselineReport = fullReport({ MATEKF405: [499000, 60000] }); + + const body = renderComment({ + prReport, + baselineReport, + shortSha: 'abc1234', + docLink: null, + marker: '', + }); + + assert.ok(!body.includes('optimization guide')); + assert.ok(!body.includes('See [')); +}); + +test('renderComment: docLink undefined (key omitted from options) -> link line absent entirely', () => { + const prReport = fullReport({ MATEKF405: [500000, 60000] }); + const baselineReport = fullReport({ MATEKF405: [499000, 60000] }); + + const body = renderComment({ + prReport, + baselineReport, + shortSha: 'abc1234', + marker: '', + }); + + assert.ok(!body.includes('optimization guide')); +}); + +test('renderComment: none of the 4 representative targets built -> fallback message, not an empty/broken table', () => { + // Neither PR nor baseline built any representative target -> all rows 'not-built'. + const body = renderComment({ + prReport: {}, + baselineReport: {}, + shortSha: 'abc1234', + docLink: null, + marker: '', + }); + + assert.ok( + body.includes('None of the representative targets') && body.includes('were built by this PR'), + `expected fallback message, got:\n${body}` + ); + assert.ok( + REPRESENTATIVE_TARGETS.every((t) => body.includes(t)), + 'fallback message should still name all 4 representative targets' + ); + // No markdown table header should be emitted since there's nothing to show. + assert.ok(!body.includes('| Target | Flash Δ | RAM Δ |')); +}); + +test('renderComment: result always ends with exactly one trailing newline', () => { + const body = renderComment({ + prReport: fullReport({ MATEKF405: [500000, 60000] }), + baselineReport: fullReport({ MATEKF405: [499000, 60000] }), + shortSha: 'abc1234', + docLink: null, + marker: '', + }); + + assert.ok(body.endsWith('\n')); + assert.ok(!body.endsWith('\n\n')); +}); diff --git a/.github/workflows/README.md b/.github/workflows/README.md index 967080a66fb..07205b1ecc8 100644 --- a/.github/workflows/README.md +++ b/.github/workflows/README.md @@ -48,6 +48,33 @@ This directory contains automated CI/CD workflows for the INAV project. ### Pull Request Helpers +#### `ci-size-report.yml` - RAM/Flash Usage Delta PR Comment +**Triggers:** `workflow_run` after "Build firmware" (`ci.yml`) completes +**Purpose:** Posts/updates a PR comment showing flash and RAM usage delta vs. +the PR's base branch, for 4 representative targets (one per MCU family: +MATEKF405, MATEKF722, MATEKF765, MATEKH743). Surfaces RAM/flash regressions +and creep before they become an overflow, on every PR. + +**How it works:** +1. `ci.yml` extracts a small per-target size report (`arm-none-eabi-size` + on each built `.elf`) right after each build and uploads it as an + artifact — no second build anywhere in this flow. +2. On pushes to a branch, `ci-size-report.yml` persists that size report as + a release asset (`size-baseline-`) in the companion + `iNavFlight/pr-test-builds` repo — the "known good" baseline for that + branch, overwritten on every push. +3. On PR builds, it fetches the PR's base branch's persisted baseline (no + rebuild), diffs it against the PR's own size report, and posts/updates a + comment (marker ``). + +**Script:** `.github/scripts/extract-size-report.sh` (size extraction), +`.github/scripts/merge-size-reports.sh` (merges per-shard reports), +`.github/scripts/size-diff-comment.js` (pure diff + markdown rendering, +unit tested in `.github/scripts/size-diff-comment.test.js`) + +**Uses the same `PR_BUILDS_TOKEN` secret and `workflow_run` trigger pattern +as `pr-test-builds.yml`** (secrets available even for fork PRs). + #### `pr-branch-suggestion.yml` - Branch Targeting Suggestion **Triggers:** PRs targeting master branch **Purpose:** Suggests using maintenance-9.x or maintenance-10.x instead diff --git a/.github/workflows/ci-size-report.yml b/.github/workflows/ci-size-report.yml new file mode 100644 index 00000000000..e25cb092916 --- /dev/null +++ b/.github/workflows/ci-size-report.yml @@ -0,0 +1,193 @@ +name: CI Size Report + +# Runs after "Build firmware" completes. Uses workflow_run (rather than +# pull_request/push directly) so that secrets are available even for PRs +# from forks — same reasoning as pr-test-builds.yml. +# +# Two jobs: +# - publish-baseline: on branch pushes (maintenance-*), persists the size +# report as a release asset in the companion iNavFlight/pr-test-builds +# repo, so PR runs never need to rebuild the base branch to get a +# comparison point. +# - pr-comment: on PR builds, fetches that persisted baseline, diffs the +# 4 representative targets, and posts/updates a PR comment. +# +# Requires the same repository secret PR_BUILDS_TOKEN as pr-test-builds.yml +# (Contents: write access to iNavFlight/pr-test-builds). +on: + workflow_run: + workflows: ["Build firmware"] + types: [completed] + +jobs: + publish-baseline: + runs-on: ubuntu-latest + if: > + github.event.workflow_run.event == 'push' && + github.event.workflow_run.conclusion == 'success' + concurrency: + group: size-baseline-${{ github.event.workflow_run.head_branch }} + cancel-in-progress: true + permissions: + actions: read # to download artifacts from the triggering workflow run + steps: + - name: Download size report + uses: actions/download-artifact@v4 + with: + name: size-report + run-id: ${{ github.event.workflow_run.id }} + github-token: ${{ secrets.GITHUB_TOKEN }} + + - name: Download branch name + uses: actions/download-artifact@v4 + with: + name: branch-name + run-id: ${{ github.event.workflow_run.id }} + github-token: ${{ secrets.GITHUB_TOKEN }} + + - name: Publish baseline + env: + GH_TOKEN: ${{ secrets.PR_BUILDS_TOKEN }} + run: | + BRANCH=$(cat branch.txt) + TAG="size-baseline-${BRANCH}" + gh release delete "$TAG" --repo iNavFlight/pr-test-builds --cleanup-tag --yes 2>/dev/null || true + gh release create "$TAG" size-report.json \ + --repo iNavFlight/pr-test-builds \ + --prerelease \ + --title "Size baseline: ${BRANCH}" \ + --notes "Latest per-target flash/RAM size report for ${BRANCH}. Auto-updated on every push. Not for human consumption." + + pr-comment: + runs-on: ubuntu-latest + if: > + github.event.workflow_run.event == 'pull_request' && + github.event.workflow_run.conclusion == 'success' + concurrency: + group: pr-size-report-${{ github.event.workflow_run.head_repository.full_name }}-${{ github.event.workflow_run.head_branch }} + cancel-in-progress: true + permissions: + actions: read + issues: write + pull-requests: write + contents: read + steps: + # Checks out this workflow's own ref (the default branch — workflow_run + # always runs the workflow file from the default branch), NOT the PR's + # head. We only need our own trusted .github/scripts/ here; the PR's + # untrusted code is never checked out in this privileged context. + - uses: actions/checkout@v4 + + - name: Download PR number + uses: actions/download-artifact@v4 + with: + name: pr-number + run-id: ${{ github.event.workflow_run.id }} + github-token: ${{ secrets.GITHUB_TOKEN }} + + - name: Download base ref + uses: actions/download-artifact@v4 + with: + name: base-ref + run-id: ${{ github.event.workflow_run.id }} + github-token: ${{ secrets.GITHUB_TOKEN }} + + - name: Download PR size report + uses: actions/download-artifact@v4 + with: + name: size-report + run-id: ${{ github.event.workflow_run.id }} + github-token: ${{ secrets.GITHUB_TOKEN }} + + - name: Read PR number and base ref + id: pr + run: | + PR_NUM=$(tr -dc '0-9' < pr_number.txt) + if [ -z "$PR_NUM" ]; then + echo "::error::Invalid PR number in artifact" + exit 1 + fi + echo "number=${PR_NUM}" >> $GITHUB_OUTPUT + echo "base_ref=$(cat base_ref.txt)" >> $GITHUB_OUTPUT + echo "short_sha=$(echo '${{ github.event.workflow_run.head_sha }}' | cut -c1-7)" >> $GITHUB_OUTPUT + + - name: Fetch base branch baseline + id: baseline + env: + GH_TOKEN: ${{ secrets.PR_BUILDS_TOKEN }} + run: | + TAG="size-baseline-${{ steps.pr.outputs.base_ref }}" + mkdir -p baseline + if gh release download "$TAG" --repo iNavFlight/pr-test-builds --pattern size-report.json --dir baseline 2>/dev/null; then + echo "found=true" >> $GITHUB_OUTPUT + else + echo "found=false" >> $GITHUB_OUTPUT + fi + + - name: Check for doc on PR head commit + id: doc + env: + GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} + run: | + HEAD_SHA="${{ github.event.workflow_run.head_sha }}" + HEAD_REPO="${{ github.event.workflow_run.head_repository.full_name }}" + if gh api "repos/${HEAD_REPO}/contents/docs/development/ram-and-flash-optimization.md?ref=${HEAD_SHA}" >/dev/null 2>&1; then + echo "link=https://github.com/${HEAD_REPO}/blob/${HEAD_SHA}/docs/development/ram-and-flash-optimization.md" >> $GITHUB_OUTPUT + else + echo "link=" >> $GITHUB_OUTPUT + fi + + - name: Post or update PR comment + uses: actions/github-script@v7 + env: + PR_NUMBER: ${{ steps.pr.outputs.number }} + SHORT_SHA: ${{ steps.pr.outputs.short_sha }} + BASELINE_FOUND: ${{ steps.baseline.outputs.found }} + DOC_LINK: ${{ steps.doc.outputs.link }} + with: + script: | + const fs = require('fs'); + const path = require('path'); + const { renderComment } = require( + path.join(process.env.GITHUB_WORKSPACE, '.github', 'scripts', 'size-diff-comment.js') + ); + + const prNumber = parseInt(process.env.PR_NUMBER, 10); + if (isNaN(prNumber)) throw new Error(`Invalid PR number: ${process.env.PR_NUMBER}`); + + const prReport = JSON.parse(fs.readFileSync('size-report.json', 'utf8')); + const baselineReport = process.env.BASELINE_FOUND === 'true' + ? JSON.parse(fs.readFileSync('baseline/size-report.json', 'utf8')) + : null; + + const marker = ''; + const body = renderComment({ + prReport, + baselineReport, + shortSha: process.env.SHORT_SHA, + docLink: process.env.DOC_LINK || null, + marker, + }); + + const comments = await github.paginate( + github.rest.issues.listComments, + { owner: context.repo.owner, repo: context.repo.repo, issue_number: prNumber } + ); + + const existing = comments.find(c => c.user.type === 'Bot' && c.body.includes(marker)); + + if (existing) { + await github.rest.issues.updateComment({ + owner: context.repo.owner, + repo: context.repo.repo, + comment_id: existing.id, + body, + }); + } else { + await github.rest.issues.createComment({ + owner: context.repo.owner, + repo: context.repo.repo, + issue_number: prNumber, + body, + }); + } diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index fb48b235904..31064c8084a 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -121,12 +121,20 @@ jobs: key: ${{ runner.os }}-downloads-${{ hashFiles('CMakeLists.txt') }}-${{ hashFiles('**/cmake/*')}} - name: Build targets (${{ matrix.id }}) run: mkdir -p build && cd build && cmake -DWARNINGS_AS_ERRORS=ON -DCI_JOB_INDEX=${{ matrix.id }} -DCI_JOB_COUNT=${{ strategy.job-total }} -DBUILD_SUFFIX=${{ env.BUILD_SUFFIX }} -DMAIN_COMPILE_OPTIONS=-pipe -G Ninja .. && ninja -j${{ env.NUM_CORES }} ci + - name: Extract size report + run: .github/scripts/extract-size-report.sh build build/size-report.json - name: Upload artifacts uses: actions/upload-artifact@v4 with: name: matrix-${{ env.BUILD_NAME }}.${{ matrix.id }} path: ./build/*.hex retention-days: 1 + - name: Upload size report + uses: actions/upload-artifact@v4 + with: + name: size-report-${{ matrix.id }} + path: ./build/size-report.json + retention-days: 1 build-single-target: needs: [detect] @@ -153,12 +161,20 @@ jobs: key: ${{ runner.os }}-downloads-${{ hashFiles('CMakeLists.txt') }}-${{ hashFiles('**/cmake/*')}} - name: Build targets (${{ needs.detect.outputs.target_names }}) run: mkdir -p build && cd build && cmake -DWARNINGS_AS_ERRORS=ON -DBUILD_SUFFIX=${{ env.BUILD_SUFFIX }} -DMAIN_COMPILE_OPTIONS=-pipe -G Ninja .. && ninja -j${{ env.NUM_CORES }} ${{ needs.detect.outputs.target_names }} + - name: Extract size report + run: .github/scripts/extract-size-report.sh build build/size-report.json - name: Upload artifacts uses: actions/upload-artifact@v4 with: name: matrix-${{ env.BUILD_NAME }}.single path: ./build/*.hex retention-days: 1 + - name: Upload size report + uses: actions/upload-artifact@v4 + with: + name: size-report-single + path: ./build/size-report.json + retention-days: 1 upload-artifacts: runs-on: ubuntu-latest @@ -212,6 +228,39 @@ jobs: name: pr-number path: pr_number.txt retention-days: 1 + - name: Save base ref + if: github.event_name == 'pull_request' + run: echo "${{ github.base_ref }}" > base_ref.txt + - name: Upload base ref + if: github.event_name == 'pull_request' + uses: actions/upload-artifact@v4 + with: + name: base-ref + path: base_ref.txt + retention-days: 1 + - name: Save branch name + if: github.event_name == 'push' + run: echo "${{ github.ref_name }}" > branch.txt + - name: Upload branch name + if: github.event_name == 'push' + uses: actions/upload-artifact@v4 + with: + name: branch-name + path: branch.txt + retention-days: 1 + - name: Download size reports + uses: actions/download-artifact@v4 + with: + pattern: size-report-* + path: size-reports + - name: Merge size reports + run: .github/scripts/merge-size-reports.sh size-report.json size-reports/*/size-report.json + - name: Upload merged size report + uses: actions/upload-artifact@v4 + with: + name: size-report + path: size-report.json + retention-days: 1 build-SITL-Linux-arm64: runs-on: ubuntu-22.04-arm From dc1dcb8b1b68f5913d36ee3b45795e127db78360 Mon Sep 17 00:00:00 2001 From: Ray Morris Date: Mon, 17 Aug 2026 15:24:31 -0500 Subject: [PATCH 3/6] Fix GitHub Actions script-injection risk in size-report workflows MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Several steps spliced \${{ }} expressions directly into run: script text instead of passing them through env: — including one value (base_ref) that round-trips through an artifact from a less-trusted PR build before being used to construct a release tag. Route all of them through env: and validate base_ref against safe git-ref characters before use. --- .github/workflows/ci-size-report.yml | 34 ++++++++++++++++++++-------- .github/workflows/ci.yml | 8 +++++-- 2 files changed, 30 insertions(+), 12 deletions(-) diff --git a/.github/workflows/ci-size-report.yml b/.github/workflows/ci-size-report.yml index e25cb092916..380fd4acd8b 100644 --- a/.github/workflows/ci-size-report.yml +++ b/.github/workflows/ci-size-report.yml @@ -101,40 +101,54 @@ jobs: - name: Read PR number and base ref id: pr + env: + HEAD_SHA: ${{ github.event.workflow_run.head_sha }} run: | PR_NUM=$(tr -dc '0-9' < pr_number.txt) if [ -z "$PR_NUM" ]; then echo "::error::Invalid PR number in artifact" exit 1 fi - echo "number=${PR_NUM}" >> $GITHUB_OUTPUT - echo "base_ref=$(cat base_ref.txt)" >> $GITHUB_OUTPUT - echo "short_sha=$(echo '${{ github.event.workflow_run.head_sha }}' | cut -c1-7)" >> $GITHUB_OUTPUT + + # base_ref.txt round-trips through an artifact produced by the + # (less-trusted) PR build, so validate it against safe git-ref + # characters before it's ever used to build a shell command or + # release tag downstream — never trust artifact content blindly. + BASE_REF=$(head -n1 base_ref.txt | tr -d '\r\n') + if ! [[ "$BASE_REF" =~ ^[A-Za-z0-9._/-]{1,100}$ ]]; then + echo "::error::Invalid base ref in artifact: $BASE_REF" + exit 1 + fi + + echo "number=${PR_NUM}" >> "$GITHUB_OUTPUT" + echo "base_ref=${BASE_REF}" >> "$GITHUB_OUTPUT" + echo "short_sha=${HEAD_SHA:0:7}" >> "$GITHUB_OUTPUT" - name: Fetch base branch baseline id: baseline env: GH_TOKEN: ${{ secrets.PR_BUILDS_TOKEN }} + BASE_REF: ${{ steps.pr.outputs.base_ref }} run: | - TAG="size-baseline-${{ steps.pr.outputs.base_ref }}" + TAG="size-baseline-${BASE_REF}" mkdir -p baseline if gh release download "$TAG" --repo iNavFlight/pr-test-builds --pattern size-report.json --dir baseline 2>/dev/null; then - echo "found=true" >> $GITHUB_OUTPUT + echo "found=true" >> "$GITHUB_OUTPUT" else - echo "found=false" >> $GITHUB_OUTPUT + echo "found=false" >> "$GITHUB_OUTPUT" fi - name: Check for doc on PR head commit id: doc env: GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} + HEAD_SHA: ${{ github.event.workflow_run.head_sha }} + HEAD_REPO: ${{ github.event.workflow_run.head_repository.full_name }} run: | - HEAD_SHA="${{ github.event.workflow_run.head_sha }}" - HEAD_REPO="${{ github.event.workflow_run.head_repository.full_name }}" if gh api "repos/${HEAD_REPO}/contents/docs/development/ram-and-flash-optimization.md?ref=${HEAD_SHA}" >/dev/null 2>&1; then - echo "link=https://github.com/${HEAD_REPO}/blob/${HEAD_SHA}/docs/development/ram-and-flash-optimization.md" >> $GITHUB_OUTPUT + echo "link=https://github.com/${HEAD_REPO}/blob/${HEAD_SHA}/docs/development/ram-and-flash-optimization.md" >> "$GITHUB_OUTPUT" else - echo "link=" >> $GITHUB_OUTPUT + echo "link=" >> "$GITHUB_OUTPUT" fi - name: Post or update PR comment diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 31064c8084a..615a55b9e7b 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -230,7 +230,9 @@ jobs: retention-days: 1 - name: Save base ref if: github.event_name == 'pull_request' - run: echo "${{ github.base_ref }}" > base_ref.txt + env: + BASE_REF: ${{ github.base_ref }} + run: echo "$BASE_REF" > base_ref.txt - name: Upload base ref if: github.event_name == 'pull_request' uses: actions/upload-artifact@v4 @@ -240,7 +242,9 @@ jobs: retention-days: 1 - name: Save branch name if: github.event_name == 'push' - run: echo "${{ github.ref_name }}" > branch.txt + env: + REF_NAME: ${{ github.ref_name }} + run: echo "$REF_NAME" > branch.txt - name: Upload branch name if: github.event_name == 'push' uses: actions/upload-artifact@v4 From bb4fa3f4663d07045afc5d044e6676efd776c09a Mon Sep 17 00:00:00 2001 From: Ray Morris Date: Mon, 17 Aug 2026 15:51:59 -0500 Subject: [PATCH 4/6] Address code review: target-coverage wording, JSON escaping, test hygiene - Clarify that the 4 representative targets are size tiers, not one-per- silicon-family (two are STM32F7, none are AT32); flag the AT32 gap in code comments/README rather than silently changing the manager-approved target list. - Build size-report.json via jq instead of manual string concatenation, so target names are JSON-escaped rather than assumed safe. - Carry baseFlash/baseRam through diffSizeReports instead of reconstructing them by subtracting the delta back out in renderComment. - Drop a test that only re-asserted a literal copy of a source constant (no behavior exercised); derive the threshold-boundary test from NOISE_THRESHOLD_BYTES instead of hardcoding 32/31. - Note the accepted tradeoff that this (non-gating) script runs on the PR's own checkout, so its numbers are informational only. --- .github/scripts/extract-size-report.sh | 24 ++++++++++++----------- .github/scripts/size-diff-comment.js | 11 +++++++++-- .github/scripts/size-diff-comment.test.js | 24 +++++++++-------------- .github/workflows/README.md | 8 +++++--- .github/workflows/ci-size-report.yml | 11 +++++++---- 5 files changed, 43 insertions(+), 35 deletions(-) diff --git a/.github/scripts/extract-size-report.sh b/.github/scripts/extract-size-report.sh index 87f810154d1..0cb9a5ea6c7 100755 --- a/.github/scripts/extract-size-report.sh +++ b/.github/scripts/extract-size-report.sh @@ -7,6 +7,11 @@ # # flash = .text + .data (what's programmed into flash) # ram = .data + .bss (what's reserved in RAM at runtime) +# +# Runs inside the (unprivileged) build job on the PR's own checkout, so a +# PR could in principle modify this script to misreport its own numbers. +# Accepted tradeoff: this feature is informational/non-gating, and a real +# overflow still fails the link step regardless of what this script says. set -euo pipefail @@ -24,7 +29,7 @@ if [ "${#ELFS[@]}" -eq 0 ]; then exit 0 fi -ENTRIES=() +JQ_ARGS=() for elf in "${ELFS[@]}"; do target=$(basename "$elf" .elf) @@ -34,16 +39,13 @@ for elf in "${ELFS[@]}"; do flash=$((text + data)) ram=$((data + bss)) - ENTRIES+=("\"$target\":{\"flash\":$flash,\"ram\":$ram}") + JQ_ARGS+=(--argjson "entry_${#JQ_ARGS[@]}" "{\"target\":\"${target}\",\"flash\":${flash},\"ram\":${ram}}") done -{ - printf '{' - printf '%s' "${ENTRIES[0]}" - for entry in "${ENTRIES[@]:1}"; do - printf ',%s' "$entry" - done - printf '}' -} > "$OUTPUT_JSON" +# Build via jq rather than manual string concatenation, so the target name +# (an .elf basename, not otherwise validated) is JSON-escaped properly +# instead of relying on it never containing a special character. +jq -n "${JQ_ARGS[@]}" 'reduce $ARGS.named[] as $e ({}; .[$e.target] = {flash: $e.flash, ram: $e.ram})' \ + > "$OUTPUT_JSON" -echo "Wrote size report for ${#ENTRIES[@]} target(s) to $OUTPUT_JSON" +echo "Wrote size report for ${#ELFS[@]} target(s) to $OUTPUT_JSON" diff --git a/.github/scripts/size-diff-comment.js b/.github/scripts/size-diff-comment.js index 0d66311cf13..0ec79554ecf 100644 --- a/.github/scripts/size-diff-comment.js +++ b/.github/scripts/size-diff-comment.js @@ -8,6 +8,11 @@ 'use strict'; +// 4 representative targets spanning flash/RAM size tiers (manager-approved +// set, 2026-08-17). Note MATEKF722 and MATEKF765 are both STM32F7 parts — +// "one per family" in the loose sense of distinct flash/RAM budgets, not +// one per silicon line. No AT32 target is covered; flagged back to the +// manager as a possible coverage gap, not decided unilaterally here. const REPRESENTATIVE_TARGETS = ['MATEKF405', 'MATEKF722', 'MATEKF765', 'MATEKH743']; // Below this magnitude a delta is noise (rounding/toolchain jitter), not a @@ -45,6 +50,8 @@ function diffSizeReports(prReport, baselineReport) { status: 'compared', flash: pr.flash, ram: pr.ram, + baseFlash: base.flash, + baseRam: base.ram, flashDelta, ramDelta, notable: Math.abs(flashDelta) >= NOISE_THRESHOLD_BYTES || Math.abs(ramDelta) >= NOISE_THRESHOLD_BYTES, @@ -71,8 +78,8 @@ function renderComment({ prReport, baselineReport, shortSha, docLink, marker }) lines.push('| Target | Flash Δ | RAM Δ |', '|---|---|---|'); for (const row of rows) { if (row.status === 'compared') { - const flashCell = formatDelta(row.flashDelta, row.flash - row.flashDelta); - const ramCell = formatDelta(row.ramDelta, row.ram - row.ramDelta); + const flashCell = formatDelta(row.flashDelta, row.baseFlash); + const ramCell = formatDelta(row.ramDelta, row.baseRam); const notableMark = row.notable ? ' ⚠️' : ''; lines.push(`| ${row.target}${notableMark} | ${flashCell} | ${ramCell} |`); } else if (row.status === 'no-baseline') { diff --git a/.github/scripts/size-diff-comment.test.js b/.github/scripts/size-diff-comment.test.js index bf005ed012d..d235e0132d6 100644 --- a/.github/scripts/size-diff-comment.test.js +++ b/.github/scripts/size-diff-comment.test.js @@ -19,12 +19,6 @@ const { formatDelta, } = require('./size-diff-comment.js'); -// Sanity on the fixed constants the rest of the tests assume. -test('REPRESENTATIVE_TARGETS is the expected 4 targets, NOISE_THRESHOLD_BYTES is 32', () => { - assert.deepEqual(REPRESENTATIVE_TARGETS, ['MATEKF405', 'MATEKF722', 'MATEKF765', 'MATEKH743']); - assert.equal(NOISE_THRESHOLD_BYTES, 32); -}); - // --------------------------------------------------------------------------- // formatDelta // --------------------------------------------------------------------------- @@ -71,21 +65,21 @@ test('diffSizeReports: target present in both PR and baseline with a real delta assert.equal(row.ramDelta, -200); }); -test('diffSizeReports: notable is gated at exactly NOISE_THRESHOLD_BYTES (32 notable, 31 not)', () => { +test('diffSizeReports: notable is gated at exactly NOISE_THRESHOLD_BYTES', () => { const baseSizes = { flash: 100000, ram: 50000 }; - const prAt32 = { MATEKF405: { flash: baseSizes.flash + 32, ram: baseSizes.ram } }; - const prAt31 = { MATEKF405: { flash: baseSizes.flash + 31, ram: baseSizes.ram } }; + const atThreshold = { MATEKF405: { flash: baseSizes.flash + NOISE_THRESHOLD_BYTES, ram: baseSizes.ram } }; + const belowThreshold = { MATEKF405: { flash: baseSizes.flash + NOISE_THRESHOLD_BYTES - 1, ram: baseSizes.ram } }; const base = { MATEKF405: baseSizes }; - const rowAt32 = diffSizeReports(prAt32, base).find((r) => r.target === 'MATEKF405'); - const rowAt31 = diffSizeReports(prAt31, base).find((r) => r.target === 'MATEKF405'); + const rowAtThreshold = diffSizeReports(atThreshold, base).find((r) => r.target === 'MATEKF405'); + const rowBelowThreshold = diffSizeReports(belowThreshold, base).find((r) => r.target === 'MATEKF405'); - assert.equal(rowAt32.flashDelta, 32); - assert.equal(rowAt32.notable, true, 'delta of exactly 32 (the threshold) should be notable'); + assert.equal(rowAtThreshold.flashDelta, NOISE_THRESHOLD_BYTES); + assert.equal(rowAtThreshold.notable, true, 'a delta of exactly NOISE_THRESHOLD_BYTES should be notable'); - assert.equal(rowAt31.flashDelta, 31); - assert.equal(rowAt31.notable, false, 'delta of 31 (below the threshold) should not be notable'); + assert.equal(rowBelowThreshold.flashDelta, NOISE_THRESHOLD_BYTES - 1); + assert.equal(rowBelowThreshold.notable, false, 'a delta one byte below NOISE_THRESHOLD_BYTES should not be notable'); }); test('diffSizeReports: notable also triggers from ramDelta alone, and honors negative deltas via Math.abs', () => { diff --git a/.github/workflows/README.md b/.github/workflows/README.md index 07205b1ecc8..c4cd07979e0 100644 --- a/.github/workflows/README.md +++ b/.github/workflows/README.md @@ -51,9 +51,11 @@ This directory contains automated CI/CD workflows for the INAV project. #### `ci-size-report.yml` - RAM/Flash Usage Delta PR Comment **Triggers:** `workflow_run` after "Build firmware" (`ci.yml`) completes **Purpose:** Posts/updates a PR comment showing flash and RAM usage delta vs. -the PR's base branch, for 4 representative targets (one per MCU family: -MATEKF405, MATEKF722, MATEKF765, MATEKH743). Surfaces RAM/flash regressions -and creep before they become an overflow, on every PR. +the PR's base branch, for 4 representative targets spanning flash/RAM size +tiers (MATEKF405, MATEKF722, MATEKF765, MATEKH743 — note MATEKF722 and +MATEKF765 are both STM32F7 parts; no AT32 target is currently covered). +Surfaces RAM/flash regressions and creep before they become an overflow, on +every PR. **How it works:** 1. `ci.yml` extracts a small per-target size report (`arm-none-eabi-size` diff --git a/.github/workflows/ci-size-report.yml b/.github/workflows/ci-size-report.yml index 380fd4acd8b..fdf25724268 100644 --- a/.github/workflows/ci-size-report.yml +++ b/.github/workflows/ci-size-report.yml @@ -5,10 +5,13 @@ name: CI Size Report # from forks — same reasoning as pr-test-builds.yml. # # Two jobs: -# - publish-baseline: on branch pushes (maintenance-*), persists the size -# report as a release asset in the companion iNavFlight/pr-test-builds -# repo, so PR runs never need to rebuild the base branch to get a -# comparison point. +# - publish-baseline: on any branch push that triggers ci.yml (in practice, +# almost always maintenance-9.x/maintenance-10.x — see ci.yml's own push +# trigger for the exact filter), persists the size report as a release +# asset in the companion iNavFlight/pr-test-builds repo, so PR runs never +# need to rebuild the base branch to get a comparison point. Stale +# baselines for since-deleted branches aren't cleaned up automatically +# (same known limitation pr-test-builds has for old PR releases). # - pr-comment: on PR builds, fetches that persisted baseline, diffs the # 4 representative targets, and posts/updates a PR comment. # From b09680f88959ba6ea4b01fefb354d718f9eb7d35 Mon Sep 17 00:00:00 2001 From: Ray Morris Date: Mon, 17 Aug 2026 16:04:10 -0500 Subject: [PATCH 5/6] Fix baseline-publish race flagged by Qodo review on PR #11790 Delete-then-recreate left a window where the release didn't exist at all, which a concurrent PR's baseline-fetch could hit and misreport as "no baseline available yet" for a baseline that actually existed. Create the release once, then only ever replace the asset in place on later pushes. Also add a short retry on the fetch side, since an individual asset replace still involves a brief delete-then-upload under the hood. --- .github/workflows/ci-size-report.yml | 42 ++++++++++++++++++++-------- 1 file changed, 31 insertions(+), 11 deletions(-) diff --git a/.github/workflows/ci-size-report.yml b/.github/workflows/ci-size-report.yml index fdf25724268..403461ac9be 100644 --- a/.github/workflows/ci-size-report.yml +++ b/.github/workflows/ci-size-report.yml @@ -54,12 +54,21 @@ jobs: run: | BRANCH=$(cat branch.txt) TAG="size-baseline-${BRANCH}" - gh release delete "$TAG" --repo iNavFlight/pr-test-builds --cleanup-tag --yes 2>/dev/null || true - gh release create "$TAG" size-report.json \ - --repo iNavFlight/pr-test-builds \ - --prerelease \ - --title "Size baseline: ${BRANCH}" \ - --notes "Latest per-target flash/RAM size report for ${BRANCH}. Auto-updated on every push. Not for human consumption." + # Never delete+recreate the release: that leaves a window where + # the release doesn't exist at all, which a concurrent PR's + # "Fetch base branch baseline" step could hit and misreport as + # "no baseline available yet". Create it once, then only ever + # replace the asset in place (--clobber) on later pushes — the + # release/tag itself stays continuously resolvable. + if gh release view "$TAG" --repo iNavFlight/pr-test-builds >/dev/null 2>&1; then + gh release upload "$TAG" size-report.json --repo iNavFlight/pr-test-builds --clobber + else + gh release create "$TAG" size-report.json \ + --repo iNavFlight/pr-test-builds \ + --prerelease \ + --title "Size baseline: ${BRANCH}" \ + --notes "Latest per-target flash/RAM size report for ${BRANCH}. Auto-updated on every push. Not for human consumption." + fi pr-comment: runs-on: ubuntu-latest @@ -135,11 +144,22 @@ jobs: run: | TAG="size-baseline-${BASE_REF}" mkdir -p baseline - if gh release download "$TAG" --repo iNavFlight/pr-test-builds --pattern size-report.json --dir baseline 2>/dev/null; then - echo "found=true" >> "$GITHUB_OUTPUT" - else - echo "found=false" >> "$GITHUB_OUTPUT" - fi + + # publish-baseline replaces the asset in place (--clobber) rather + # than deleting/recreating the release, but an individual asset + # replace still briefly deletes-then-uploads under the hood. A + # few short retries absorb that narrow window instead of a + # concurrent run misreporting "no baseline available yet" for a + # baseline that actually exists. + FOUND=false + for attempt in 1 2 3; do + if gh release download "$TAG" --repo iNavFlight/pr-test-builds --pattern size-report.json --dir baseline 2>/dev/null; then + FOUND=true + break + fi + sleep 3 + done + echo "found=${FOUND}" >> "$GITHUB_OUTPUT" - name: Check for doc on PR head commit id: doc From 4029d9848335e56426d97671b45479f155ff2d66 Mon Sep 17 00:00:00 2001 From: Ray Morris Date: Mon, 17 Aug 2026 18:08:56 -0500 Subject: [PATCH 6/6] Fix arm-none-eabi-size not found on real CI runners MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit cmake/arm-none-eabi-checks.cmake downloads its own toolchain into tools/ and adds it to PATH, but only inside that cmake process via set(ENV{PATH} ...) — that doesn't persist to a later CI step's shell. extract-size-report.sh defaulted to bare `arm-none-eabi-size` on PATH, which happened to work in local dev sandboxes with a system copy installed but fails on actual GitHub Actions runners, which have only the project's downloaded toolchain. Fall back to the same tools/ location CMake uses when nothing is found on PATH. --- .github/scripts/extract-size-report.sh | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/.github/scripts/extract-size-report.sh b/.github/scripts/extract-size-report.sh index 0cb9a5ea6c7..24170ac952b 100755 --- a/.github/scripts/extract-size-report.sh +++ b/.github/scripts/extract-size-report.sh @@ -17,7 +17,24 @@ set -euo pipefail BUILD_DIR=${1:?usage: extract-size-report.sh [size-tool]} OUTPUT_JSON=${2:?usage: extract-size-report.sh [size-tool]} -SIZE_TOOL=${3:-arm-none-eabi-size} +SIZE_TOOL=${3:-} + +if [ -z "$SIZE_TOOL" ]; then + if command -v arm-none-eabi-size >/dev/null 2>&1; then + SIZE_TOOL=arm-none-eabi-size + else + # cmake/arm-none-eabi-checks.cmake downloads its own toolchain into + # tools/ and adds it to PATH — but only inside that cmake process + # via set(ENV{PATH} ...), which doesn't persist to a later CI step's + # shell. Fall back to the same location CMake would have used, + # relative to the repo root (this script must be run from there). + SIZE_TOOL=$(compgen -G 'tools/arm-gnu-toolchain-*/bin/arm-none-eabi-size' 2>/dev/null | head -n1 || true) + if [ -z "$SIZE_TOOL" ]; then + echo "::error::arm-none-eabi-size not found on PATH or under tools/arm-gnu-toolchain-*/bin/" >&2 + exit 1 + fi + fi +fi # CMake's RUNTIME_OUTPUT_DIRECTORY puts built executables under # /bin/ (see cmake/main.cmake), not directly — search