From c5b4446f7c5c0d31eeb4007603dfb91683756dad Mon Sep 17 00:00:00 2001 From: Ray Morris Date: Wed, 9 Sep 2026 10:30:56 -0500 Subject: [PATCH 1/7] Report CI size-diff RAM delta per linker region instead of combined MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The size-diff bot's "RAM Δ" summed every writable memory region into one figure, so a PR that only grew CCM (or DTCM) usage looked identical to one that grew main RAM by the same amount - misleading on parts where those regions have very different remaining headroom (e.g. reported +3,740 B for MATEKF405 when the RAM region itself only grew 608 B, the rest was CCM). extract-size-report.sh now also computes each target's per-region usage from its .map file (matching arm-none-eabi-size -A section addresses against the linker's own memory map, no per-family section-name table needed) and size-diff-comment.js renders regions separately when both the PR and baseline reports have them, falling back to the old combined figure otherwise so existing stored baselines keep working. Also split the shared 256 B notability threshold into separate flash (4096 B) and RAM (1024 B, applied per-region) thresholds - the old single threshold was too tight for flash's much larger budget and flagged nearly every PR. --- .github/scripts/compute-region-sizes.py | 123 ++++++++++++++++++++++ .github/scripts/extract-size-report.sh | 35 +++++- .github/scripts/size-diff-comment.js | 52 +++++++-- .github/scripts/size-diff-comment.test.js | 108 ++++++++++++++++--- 4 files changed, 291 insertions(+), 27 deletions(-) create mode 100755 .github/scripts/compute-region-sizes.py diff --git a/.github/scripts/compute-region-sizes.py b/.github/scripts/compute-region-sizes.py new file mode 100755 index 00000000000..f77be880302 --- /dev/null +++ b/.github/scripts/compute-region-sizes.py @@ -0,0 +1,123 @@ +#!/usr/bin/env python3 +"""Compute per-linker-region byte usage for one .elf, from its companion +.map file (Memory Configuration table) and `arm-none-eabi-size -A` output. + +Why: `arm-none-eabi-size -B` (Berkeley format, used for the flat flash/ram +totals elsewhere in this pipeline) sums ALL writable sections into one +"data"/"bss" pair regardless of which physical memory region they're linked +into (e.g. F4/F7 parts split writable memory across RAM and CCM; H7 across +RAM and DTCM). That single combined number is what CI historically reported +as "RAM Delta", which misattributes growth that actually landed in a +different, smaller, often more memory-pressured region. This script instead +matches each section's load address against the target's own linker-defined +memory regions (from its .map file), so the breakdown is exact and requires +no per-MCU-family section-name table to maintain. + +Usage: compute-region-sizes.py [size-tool] +Prints a JSON object of {"": , ...} to stdout, one entry per +writable region (attributes containing "w") that has nonzero used bytes. +This is informational/non-gating, so any failure (missing/unparsable map, +`size` tool error) prints "{}" and exits 0 rather than aborting the +caller's build-artifact pipeline over a region breakdown it can live +without - callers degrade gracefully to the flat flash/ram figures. +""" +import json +import re +import subprocess +import sys + + +def parse_memory_regions(map_path): + """Returns [(name, origin, end), ...] for writable, nonzero-length + regions, parsed from the map file's "Memory Configuration" table.""" + try: + with open(map_path, encoding='utf-8', errors='replace') as f: + text = f.read() + except OSError: + return [] + + m = re.search(r'^Memory Configuration\s*\n\s*\n[^\n]*\n(.*?)\n\s*\nLinker script and memory map', + text, re.MULTILINE | re.DOTALL) + if not m: + return [] + + regions = [] + for line in m.group(1).splitlines(): + parts = line.split() + if len(parts) < 3 or parts[0] == '*default*': + continue + name, origin_str, length_str = parts[0], parts[1], parts[2] + attrs = parts[3] if len(parts) > 3 else '' + try: + origin = int(origin_str, 16) + length = int(length_str, 16) + except ValueError: + continue + if length <= 0 or 'w' not in attrs: + continue + regions.append((name, origin, origin + length)) + return regions + + +def parse_section_sizes(elf_path, size_tool): + """Returns [(section_name, size, addr), ...] via `size -A` (sysv), the + one format that reports per-section addresses needed for region + matching (Berkeley's -B only gives family totals, no addresses).""" + out = subprocess.run([size_tool, '-A', elf_path], capture_output=True, text=True, check=True).stdout + sections = [] + for line in out.splitlines(): + parts = line.split() + if len(parts) != 3: + continue + name, size_str, addr_str = parts + if name in ('section', 'Total'): + continue + try: + size, addr = int(size_str), int(addr_str) + except ValueError: + continue + sections.append((name, size, addr)) + return sections + + +def compute(elf_path, map_path, size_tool): + regions = parse_memory_regions(map_path) + if not regions: + return {} + + usage = {name: 0 for name, _, _ in regions} + for _section_name, size, addr in parse_section_sizes(elf_path, size_tool): + # Non-allocated sections (debug info, symbol/string tables, comments) + # report addr 0 - they're never actually placed in memory, so they + # must be excluded explicitly rather than relying on address-range + # matching alone: a region whose own origin is 0x0 (e.g. some parts' + # ITCM alias) would otherwise false-match every one of them and + # report several megabytes of phantom "usage". + if size <= 0 or addr == 0: + continue + for name, start, end in regions: + if start <= addr < end: + usage[name] += size + break + + return {name: bytes_ for name, bytes_ in usage.items() if bytes_ > 0} + + +def main(): + if len(sys.argv) not in (3, 4): + print('usage: compute-region-sizes.py [size-tool]', file=sys.stderr) + sys.exit(1) + elf_path, map_path = sys.argv[1], sys.argv[2] + size_tool = sys.argv[3] if len(sys.argv) == 4 else 'arm-none-eabi-size' + + try: + result = compute(elf_path, map_path, size_tool) + except (OSError, subprocess.CalledProcessError) as e: + print(f'compute-region-sizes.py: {e} - falling back to no region breakdown', file=sys.stderr) + result = {} + + print(json.dumps(result)) + + +if __name__ == '__main__': + main() diff --git a/.github/scripts/extract-size-report.sh b/.github/scripts/extract-size-report.sh index 24170ac952b..d9883040fe7 100755 --- a/.github/scripts/extract-size-report.sh +++ b/.github/scripts/extract-size-report.sh @@ -6,7 +6,14 @@ # 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) +# ram = .data + .bss (what's reserved in RAM at runtime, summed across +# EVERY writable memory region the target has — e.g. RAM+CCM on +# F4/F7 parts, RAM+DTCM on H7. It's a total, not one region.) +# regions = { "": , ... } — the same total broken out per +# linker memory region (RAM, CCM, DTCM, ...), computed from the +# build's own .map file via compute-region-sizes.py. Omitted for a +# target whose .map file is missing, so consumers must treat it as +# optional. # # 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. @@ -46,6 +53,8 @@ if [ "${#ELFS[@]}" -eq 0 ]; then exit 0 fi +SCRIPT_DIR=$(dirname "${BASH_SOURCE[0]}") + JQ_ARGS=() for elf in "${ELFS[@]}"; do target=$(basename "$elf" .elf) @@ -56,13 +65,29 @@ for elf in "${ELFS[@]}"; do flash=$((text + data)) ram=$((data + bss)) - JQ_ARGS+=(--argjson "entry_${#JQ_ARGS[@]}" "{\"target\":\"${target}\",\"flash\":${flash},\"ram\":${ram}}") + # cmake's stm32.cmake/at32.cmake link every target with -Wl,-Map,.map + # (alongside -Wl,--print-memory-usage), so the per-region breakdown this + # script computes here matches exactly what the linker itself reported at + # build time. A missing map (e.g. a toolchain change that stops emitting + # one) degrades to just the flat flash/ram totals above, not a hard error. + map="${elf}.map" + regions='{}' + if [ -f "$map" ]; then + regions=$(python3 "${SCRIPT_DIR}/compute-region-sizes.py" "$elf" "$map" "$SIZE_TOOL") || regions='{}' + fi + + JQ_ARGS+=(--argjson "entry_${#JQ_ARGS[@]}" "{\"target\":\"${target}\",\"flash\":${flash},\"ram\":${ram},\"regions\":${regions}}") done # 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" +# instead of relying on it never containing a special character. Omit +# "regions" entirely when empty rather than storing a misleading {} that +# would read as "this target has no writable memory regions". +jq -n "${JQ_ARGS[@]}" ' + reduce $ARGS.named[] as $e ({}; + .[$e.target] = {flash: $e.flash, ram: $e.ram} + + (if ($e.regions | length) > 0 then {regions: $e.regions} else {} end) + )' > "$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 278036aa970..11d47200727 100644 --- a/.github/scripts/size-diff-comment.js +++ b/.github/scripts/size-diff-comment.js @@ -4,7 +4,16 @@ // 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": }, ... } +// Report shape: { "": { "flash": , "ram": , +// "regions": { "": , ... } }, ... } +// +// "regions" is optional and, when present, breaks "ram" down by linker +// memory region (e.g. RAM/CCM on F4/F7 parts, RAM/DTCM on H7) — "ram" alone +// is the sum of those regions and stays purely additive/informational once +// a regional breakdown exists. A region delta is only rendered when BOTH +// the PR and baseline entries carry "regions" for that target; if either +// side predates the field (e.g. an old stored baseline), the row falls +// back to the combined "ram" figure instead of a partial breakdown. 'use strict'; @@ -15,9 +24,9 @@ // 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 -// real change worth calling out. -const NOISE_THRESHOLD_BYTES = 256; +// Below these magnitudes a delta is noise, not worth flagging. +const FLASH_NOISE_THRESHOLD_BYTES = 4096; +const RAM_NOISE_THRESHOLD_BYTES = 1024; function formatDelta(deltaBytes, baseBytes) { const sign = deltaBytes > 0 ? '+' : deltaBytes < 0 ? '' : '±'; @@ -45,6 +54,25 @@ function diffSizeReports(prReport, baselineReport) { const flashDelta = pr.flash - base.flash; const ramDelta = pr.ram - base.ram; + + // Only trust a per-region breakdown when both sides have one - a + // region missing from just one side (schema drift, or a region a + // target gained/lost) would otherwise render a misleading partial + // delta for that region. + let regionDeltas; + if (pr.regions && base.regions) { + const names = Array.from(new Set([...Object.keys(pr.regions), ...Object.keys(base.regions)])).sort(); + regionDeltas = names.map((name) => { + const prBytes = pr.regions[name] || 0; + const baseBytes = base.regions[name] || 0; + return { name, delta: prBytes - baseBytes, baseBytes }; + }); + } + + const notable = regionDeltas + ? Math.abs(flashDelta) >= FLASH_NOISE_THRESHOLD_BYTES || regionDeltas.some((r) => Math.abs(r.delta) >= RAM_NOISE_THRESHOLD_BYTES) + : Math.abs(flashDelta) >= FLASH_NOISE_THRESHOLD_BYTES || Math.abs(ramDelta) >= RAM_NOISE_THRESHOLD_BYTES; + return { target, status: 'compared', @@ -54,7 +82,8 @@ function diffSizeReports(prReport, baselineReport) { baseRam: base.ram, flashDelta, ramDelta, - notable: Math.abs(flashDelta) >= NOISE_THRESHOLD_BYTES || Math.abs(ramDelta) >= NOISE_THRESHOLD_BYTES, + regionDeltas, + notable, }; }); } @@ -95,7 +124,9 @@ function renderComment({ prReport, baselineReport, shortSha, baselineCommit, bas for (const row of rows) { if (row.status === 'compared') { const flashCell = formatDelta(row.flashDelta, row.baseFlash); - const ramCell = formatDelta(row.ramDelta, row.baseRam); + const ramCell = row.regionDeltas + ? row.regionDeltas.map((r) => `${r.name}: ${formatDelta(r.delta, r.baseBytes)}`).join('
') + : formatDelta(row.ramDelta, row.baseRam); const notableMark = row.notable ? ' ⚠️' : ''; lines.push(`| ${row.target}${notableMark} | ${flashCell} | ${ramCell} |`); } else if (row.status === 'no-baseline') { @@ -121,4 +152,11 @@ function renderComment({ prReport, baselineReport, shortSha, baselineCommit, bas return lines.join('\n').trimEnd() + '\n'; } -module.exports = { REPRESENTATIVE_TARGETS, NOISE_THRESHOLD_BYTES, diffSizeReports, renderComment, formatDelta }; +module.exports = { + REPRESENTATIVE_TARGETS, + FLASH_NOISE_THRESHOLD_BYTES, + RAM_NOISE_THRESHOLD_BYTES, + diffSizeReports, + renderComment, + formatDelta, +}; diff --git a/.github/scripts/size-diff-comment.test.js b/.github/scripts/size-diff-comment.test.js index 5b10836e47d..0e422648673 100644 --- a/.github/scripts/size-diff-comment.test.js +++ b/.github/scripts/size-diff-comment.test.js @@ -13,7 +13,8 @@ const assert = require('node:assert/strict'); const { REPRESENTATIVE_TARGETS, - NOISE_THRESHOLD_BYTES, + FLASH_NOISE_THRESHOLD_BYTES, + RAM_NOISE_THRESHOLD_BYTES, diffSizeReports, renderComment, formatDelta, @@ -65,31 +66,49 @@ 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', () => { +test('diffSizeReports: notable (flash) is gated at exactly FLASH_NOISE_THRESHOLD_BYTES', () => { const baseSizes = { flash: 100000, ram: 50000 }; - 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 atThreshold = { MATEKF405: { flash: baseSizes.flash + FLASH_NOISE_THRESHOLD_BYTES, ram: baseSizes.ram } }; + const belowThreshold = { MATEKF405: { flash: baseSizes.flash + FLASH_NOISE_THRESHOLD_BYTES - 1, ram: baseSizes.ram } }; const base = { MATEKF405: baseSizes }; const rowAtThreshold = diffSizeReports(atThreshold, base).find((r) => r.target === 'MATEKF405'); const rowBelowThreshold = diffSizeReports(belowThreshold, base).find((r) => r.target === 'MATEKF405'); - assert.equal(rowAtThreshold.flashDelta, NOISE_THRESHOLD_BYTES); - assert.equal(rowAtThreshold.notable, true, 'a delta of exactly NOISE_THRESHOLD_BYTES should be notable'); + assert.equal(rowAtThreshold.flashDelta, FLASH_NOISE_THRESHOLD_BYTES); + assert.equal(rowAtThreshold.notable, true, 'a delta of exactly FLASH_NOISE_THRESHOLD_BYTES should 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'); + assert.equal(rowBelowThreshold.flashDelta, FLASH_NOISE_THRESHOLD_BYTES - 1); + assert.equal(rowBelowThreshold.notable, false, 'a delta one byte below FLASH_NOISE_THRESHOLD_BYTES should not be notable'); +}); + +test('diffSizeReports: notable (ram) is gated at exactly RAM_NOISE_THRESHOLD_BYTES, independently of flash', () => { + const baseSizes = { flash: 100000, ram: 50000 }; + + const atThreshold = { MATEKF405: { flash: baseSizes.flash, ram: baseSizes.ram + RAM_NOISE_THRESHOLD_BYTES } }; + const belowThreshold = { MATEKF405: { flash: baseSizes.flash, ram: baseSizes.ram + RAM_NOISE_THRESHOLD_BYTES - 1 } }; + const base = { MATEKF405: baseSizes }; + + const rowAtThreshold = diffSizeReports(atThreshold, base).find((r) => r.target === 'MATEKF405'); + const rowBelowThreshold = diffSizeReports(belowThreshold, base).find((r) => r.target === 'MATEKF405'); + + assert.equal(rowAtThreshold.ramDelta, RAM_NOISE_THRESHOLD_BYTES); + assert.equal(rowAtThreshold.notable, true, 'a ram delta of exactly RAM_NOISE_THRESHOLD_BYTES should be notable'); + + assert.equal(rowBelowThreshold.ramDelta, RAM_NOISE_THRESHOLD_BYTES - 1); + assert.equal(rowBelowThreshold.notable, false, 'a ram delta one byte below RAM_NOISE_THRESHOLD_BYTES 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 - 300 } }; // flash unchanged, ram shrank by 300 + const ramShrink = RAM_NOISE_THRESHOLD_BYTES + 300; + const pr = { MATEKF405: { flash: 100000, ram: 50000 - ramShrink } }; // flash unchanged, ram shrank const row = diffSizeReports(pr, base).find((r) => r.target === 'MATEKF405'); assert.equal(row.flashDelta, 0); - assert.equal(row.ramDelta, -300); - assert.equal(row.notable, true, 'a -300 ram delta exceeds NOISE_THRESHOLD_BYTES in magnitude'); + assert.equal(row.ramDelta, -ramShrink); + assert.equal(row.notable, true, 'a ram delta exceeding RAM_NOISE_THRESHOLD_BYTES in magnitude is notable'); }); test('diffSizeReports: target missing from PR report but present in baseline -> missing-from-pr', () => { @@ -155,9 +174,9 @@ test('renderComment: baseline present, all 4 targets compared, mix of notable/no MATEKH743: [530000, 63000], }); const prReport = fullReport({ - MATEKF405: [500300, 60000], // +300 flash -> notable + MATEKF405: [504100, 60000], // +4100 flash -> notable MATEKF722: [510010, 61000], // +10 flash -> not notable - MATEKF765: [519700, 62000], // -300 flash -> notable + MATEKF765: [515700, 62000], // -4300 flash -> notable MATEKH743: [530000, 63000], // no change -> not notable }); @@ -179,9 +198,9 @@ test('renderComment: baseline present, all 4 targets compared, mix of notable/no const matekf765Line = lines.find((l) => l.startsWith('| MATEKF765')); const matekh743Line = lines.find((l) => l.startsWith('| MATEKH743')); - assert.ok(matekf405Line.includes('⚠️'), 'MATEKF405 (+300 flash) should be flagged notable'); + assert.ok(matekf405Line.includes('⚠️'), 'MATEKF405 (+4100 flash) should be flagged notable'); assert.ok(!matekf722Line.includes('⚠️'), 'MATEKF722 (+10 flash) should NOT be flagged notable'); - assert.ok(matekf765Line.includes('⚠️'), 'MATEKF765 (-300 flash) should be flagged notable'); + assert.ok(matekf765Line.includes('⚠️'), 'MATEKF765 (-4300 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. @@ -355,3 +374,62 @@ test('renderComment: baselineCommit supplied but no baseline report -> graceful assert.ok(!body.includes('vs. base commit `9e932ba`'), 'header must not name a baseline commit when no baseline exists'); assert.ok(body.includes('vs. base branch'), 'header should fall back to the generic wording'); }); + +// --------------------------------------------------------------------------- +// renderComment: per-region (RAM vs CCM/DTCM) breakdown +// --------------------------------------------------------------------------- +// +// When both sides of a comparison carry a `regions` breakdown, renderComment +// reports each region's own delta instead of one combined "RAM Δ" figure +// that silently mixes growth across separate, non-fungible memory regions +// (e.g. RAM vs. CCM on F4/F7, RAM vs. DTCM on H7). +test('renderComment: RAM and CCM region deltas are reported separately, not combined into one RAM Δ', () => { + const baselineReport = { + MATEKF405: { flash: 500000, ram: 60000, regions: { RAM: 50000, CCM: 10000 } }, + }; + const prReport = { + // Combined ram grew by 3740 B (60000 -> 63740), matching the real CI + // report, but the regional breakdown shows RAM grew by only 608 B + // while CCM grew by 3132 B (608 + 3132 = 3740). + MATEKF405: { flash: 500000, ram: 63740, regions: { RAM: 50608, CCM: 13132 } }, + }; + + const body = renderComment({ + prReport, + baselineReport, + shortSha: 'abc1234', + docLink: null, + marker: '', + }); + + assert.ok( + body.includes('RAM: +608 B'), + `expected the RAM region's own delta ("RAM: +608 B") to be reported separately, got:\n${body}` + ); + assert.ok( + body.includes('CCM: +3132 B'), + `expected the CCM region's own delta ("CCM: +3132 B") to be reported separately, got:\n${body}` + ); + assert.ok( + !body.includes('+3740 B'), + `RAM and CCM deltas must not be silently combined into one "+3740 B" figure, got:\n${body}` + ); +}); + +test('renderComment: falls back to the combined RAM Δ when only one side has a regions breakdown', () => { + // A baseline captured before this field existed (or a target that lost + // its region info) must not produce a partial/misleading breakdown. + const baselineReport = { MATEKF405: { flash: 500000, ram: 60000 } }; + const prReport = { MATEKF405: { flash: 500000, ram: 63740, regions: { RAM: 50608, CCM: 13132 } } }; + + const body = renderComment({ + prReport, + baselineReport, + shortSha: 'abc1234', + docLink: null, + marker: '', + }); + + assert.ok(body.includes('+3740 B'), `expected the combined RAM Δ fallback, got:\n${body}`); + assert.ok(!body.includes('RAM: +608 B'), `must not render a partial region breakdown, got:\n${body}`); +}); From dbce11e8b5ab11f1397d5030978453256901e54e Mon Sep 17 00:00:00 2001 From: Ray Morris Date: Wed, 9 Sep 2026 21:09:48 -0500 Subject: [PATCH 2/7] Document rangefinder requirement for FW autoland flare phase Flare only activates when a healthy rangefinder is present (see getHwRangefinderStatus() gate in navigation.c); on GPS-only aircraft the landing silently stays in the glide phase all the way to touchdown, producing a consistent overshoot in height and distance that no other autoland tuning parameter can fix. This wasn't documented anywhere users would see it while tuning nav_fw_land_flare_alt/pitch, only in a separate wiki-style doc, so add it to the settings descriptions directly. Fixes #11751 --- docs/Settings.md | 4 ++-- src/main/fc/settings.yaml | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/docs/Settings.md b/docs/Settings.md index 3d7351dc371..16161b870f1 100644 --- a/docs/Settings.md +++ b/docs/Settings.md @@ -3304,7 +3304,7 @@ Modifier for pitch to throttle ratio at final approach. In Percent. ### nav_fw_land_flare_alt -Initial altitude of the flare phase +Initial altitude of the flare phase. Requires a healthy rangefinder; without one the aircraft stays in the glide phase (see nav_fw_land_glide_alt/nav_fw_land_glide_pitch) all the way to touchdown. | Default | Min | Max | | --- | --- | --- | @@ -3314,7 +3314,7 @@ Initial altitude of the flare phase ### nav_fw_land_flare_pitch -Pitch value for flare phase. In degrees +Pitch value for flare phase. In degrees. Only applies with a healthy rangefinder; the flare phase never activates without one. | Default | Min | Max | | --- | --- | --- | diff --git a/src/main/fc/settings.yaml b/src/main/fc/settings.yaml index 4e8affb0221..bcfa3ef0968 100644 --- a/src/main/fc/settings.yaml +++ b/src/main/fc/settings.yaml @@ -4341,7 +4341,7 @@ groups: min: 100 max: 5000 - name: nav_fw_land_flare_alt - description: "Initial altitude of the flare phase" + description: "Initial altitude of the flare phase. Requires a healthy rangefinder; without one the aircraft stays in the glide phase (see nav_fw_land_glide_alt/nav_fw_land_glide_pitch) all the way to touchdown." default_value: 150 field: flareAltitude min: 0 @@ -4353,7 +4353,7 @@ groups: min: -15 max: 45 - name: nav_fw_land_flare_pitch - description: "Pitch value for flare phase. In degrees" + description: "Pitch value for flare phase. In degrees. Only applies with a healthy rangefinder; the flare phase never activates without one." default_value: 8 field: flarePitch min: -15 From 2bad6bef8eeb8c65da1fb73e7bc232a8eb65def2 Mon Sep 17 00:00:00 2001 From: Ray Morris Date: Sat, 10 Jan 2026 11:35:55 -0600 Subject: [PATCH 3/7] Port Betaflight ESC passthrough fixes for Bluejay/AM32 compatibility Changes from Betaflight PRs #13287 and #14214: - Add timeout handling to ReadByte() and ReadByteCrc() to prevent indefinite blocking during 4-way interface communication - Update SILABS_DEVICE_MATCH to range-based detection (0xE800-0xF900) for broader ESC firmware compatibility - Add ESC reboot logic in cmd_DeviceReset for Bluejay/AM32 ESCs - Update protocol version to 108 and interface version to 20.0.06 Note: These changes could not be verified due to hardware limitations (4-way interface did not respond on test hardware with either INAV or Betaflight). Community testing requested. --- src/main/io/serial_4way.c | 90 ++++++++++++++++++++++++++------------- 1 file changed, 60 insertions(+), 30 deletions(-) diff --git a/src/main/io/serial_4way.c b/src/main/io/serial_4way.c index dd825050e16..446e089ae24 100644 --- a/src/main/io/serial_4way.c +++ b/src/main/io/serial_4way.c @@ -27,6 +27,7 @@ #include "drivers/buf_writer.h" #include "drivers/io.h" #include "drivers/serial.h" +#include "drivers/time.h" #include "drivers/timer.h" #include "drivers/pwm_mapping.h" #include "drivers/pwm_output.h" @@ -74,11 +75,17 @@ // *** change to adapt Revision #define SERIAL_4WAY_VER_MAIN 20 #define SERIAL_4WAY_VER_SUB_1 (uint8_t) 0 -#define SERIAL_4WAY_VER_SUB_2 (uint8_t) 05 +#define SERIAL_4WAY_VER_SUB_2 (uint8_t) 06 -#define SERIAL_4WAY_PROTOCOL_VER 107 +#define SERIAL_4WAY_PROTOCOL_VER 108 // *** end +// Timeout values for 4-way interface communication (from Betaflight PR #13287) +#define CMD_TIMEOUT_US 50000 +#define ARG_TIMEOUT_US 25000 +#define DAT_TIMEOUT_US 10000 +#define CRC_TIMEOUT_US 10000 + #if (SERIAL_4WAY_VER_MAIN > 24) #error "beware of SERIAL_4WAY_VER_SUB_1 is uint8_t" #endif @@ -328,10 +335,8 @@ uint16_t _crc_xmodem_update (uint16_t crc, uint8_t data) { #define ATMEL_DEVICE_MATCH ((pDeviceInfo->words[0] == 0x9307) || (pDeviceInfo->words[0] == 0x930A) || \ (pDeviceInfo->words[0] == 0x930F) || (pDeviceInfo->words[0] == 0x940B)) -#define SILABS_DEVICE_MATCH ((pDeviceInfo->words[0] == 0xF310)||(pDeviceInfo->words[0] == 0xF330) || \ - (pDeviceInfo->words[0] == 0xF410) || (pDeviceInfo->words[0] == 0xF390) || \ - (pDeviceInfo->words[0] == 0xF850) || (pDeviceInfo->words[0] == 0xE8B1) || \ - (pDeviceInfo->words[0] == 0xE8B2)) +// Range-based detection for Bluejay/AM32 compatibility (from Betaflight PR #13287) +#define SILABS_DEVICE_MATCH ((pDeviceInfo->words[0] > 0xE800) && (pDeviceInfo->words[0] < 0xF900)) // BLHeli_32 MCU ID hi > 0x00 and < 0x90 / lo always = 0x06 #define ARM_DEVICE_MATCH ((pDeviceInfo->bytes[1] > 0x00) && (pDeviceInfo->bytes[1] < 0x90) && (pDeviceInfo->bytes[0] == 0x06)) @@ -384,19 +389,26 @@ static uint8_t Connect(uint8_32_u *pDeviceInfo) static serialPort_t *port; -static uint8_t ReadByte(void) +static bool ReadByte(uint8_t *data, timeDelta_t timeoutUs) { - // need timeout? - while (!serialRxBytesWaiting(port)); - return serialRead(port); + timeUs_t startTime = micros(); + while (!serialRxBytesWaiting(port)) { + if (timeoutUs && (cmpTimeUs(micros(), startTime) > timeoutUs)) { + return true; // timeout occurred + } + } + *data = serialRead(port); + return false; // success } static uint8_16_u CRC_in; -static uint8_t ReadByteCrc(void) +static bool ReadByteCrc(uint8_t *data, timeDelta_t timeoutUs) { - uint8_t b = ReadByte(); - CRC_in.word = _crc_xmodem_update(CRC_in.word, b); - return b; + bool timedOut = ReadByte(data, timeoutUs); + if (!timedOut) { + CRC_in.word = _crc_xmodem_update(CRC_in.word, *data); + } + return timedOut; } static void WriteByte(uint8_t b) @@ -437,10 +449,13 @@ void esc4wayProcess(serialPort_t *mspPort) bool isExitScheduled = false; while (1) { + bool timedOut = false; + // restart looking for new sequence from host do { CRC_in.word = 0; - ESC = ReadByteCrc(); + // No timeout - BLHeliSuite32 waits indefinitely for input + ReadByteCrc(&ESC, 0); } while (ESC != cmd_Local_Escape); RX_LED_ON; @@ -448,23 +463,25 @@ void esc4wayProcess(serialPort_t *mspPort) Dummy.word = 0; O_PARAM = &Dummy.bytes[0]; O_PARAM_LEN = 1; - CMD = ReadByteCrc(); - ioMem.D_FLASH_ADDR_H = ReadByteCrc(); - ioMem.D_FLASH_ADDR_L = ReadByteCrc(); - I_PARAM_LEN = ReadByteCrc(); - InBuff = ParamBuf; - uint8_t i = I_PARAM_LEN; - do { - *InBuff = ReadByteCrc(); - InBuff++; - i--; - } while (i != 0); + timedOut = ReadByteCrc(&CMD, CMD_TIMEOUT_US) || + ReadByteCrc(&ioMem.D_FLASH_ADDR_H, ARG_TIMEOUT_US) || + ReadByteCrc(&ioMem.D_FLASH_ADDR_L, ARG_TIMEOUT_US) || + ReadByteCrc(&I_PARAM_LEN, ARG_TIMEOUT_US); - CRC_check.bytes[1] = ReadByte(); - CRC_check.bytes[0] = ReadByte(); + if (!timedOut) { + uint8_t i = I_PARAM_LEN; + InBuff = ParamBuf; + do { + timedOut = ReadByteCrc(InBuff++, DAT_TIMEOUT_US); + } while ((--i > 0) && !timedOut); - if (CRC_check.word == CRC_in.word) { + for (int8_t j = 1; (j >= 0) && !timedOut; j--) { + timedOut = ReadByte(&CRC_check.bytes[j], CRC_TIMEOUT_US); + } + } + + if ((CRC_check.word == CRC_in.word) && !timedOut) { ACK_OUT = ACK_OK; } else { ACK_OUT = ACK_I_INVALID_CRC; @@ -561,9 +578,13 @@ void esc4wayProcess(serialPort_t *mspPort) case cmd_DeviceReset: { + bool rebootEsc = false; if (ParamBuf[0] < escCount) { // Channel may change here selected_esc = ParamBuf[0]; + if (ioMem.D_FLASH_ADDR_L == 1) { + rebootEsc = true; + } } else { ACK_OUT = ACK_I_INVALID_CHANNEL; @@ -577,6 +598,15 @@ void esc4wayProcess(serialPort_t *mspPort) case imARM_BLB: { BL_SendCMDRunRestartBootloader(&DeviceInfo); + // ESC reboot logic for Bluejay/AM32 (from Betaflight PR #14214) + if (rebootEsc) { + ESC_OUTPUT; + setEscLo(selected_esc); + timeMs_t m = millis(); + while (millis() - m < 300); + setEscHi(selected_esc); + ESC_INPUT; + } break; } #endif @@ -872,7 +902,7 @@ void esc4wayProcess(serialPort_t *mspPort) WriteByteCrc(ioMem.D_FLASH_ADDR_L); WriteByteCrc(O_PARAM_LEN); - i=O_PARAM_LEN; + uint8_t i = O_PARAM_LEN; do { while (!serialTxBytesFree(port)); From aad80f2dc5a7ba35b2e3e18765c93a6853cdb8f5 Mon Sep 17 00:00:00 2001 From: Ray Morris Date: Thu, 10 Sep 2026 00:26:18 -0500 Subject: [PATCH 4/7] Add DEBUG_ESC mode to expose raw ESC bootloader signature The 4-way passthrough Connect() records the raw device signature and detected family (SiLabs/Atmel/ARM) so ESC firmware can be identified during passthrough, e.g. EFM8BB51x (0xE8B5) which the fixed whitelist rejects. --- src/main/build/debug.h | 1 + src/main/fc/cli.c | 3 ++- src/main/fc/settings.yaml | 2 +- src/main/io/serial_4way.c | 6 ++++++ 4 files changed, 10 insertions(+), 2 deletions(-) diff --git a/src/main/build/debug.h b/src/main/build/debug.h index 0bb74bac1ac..1faced7dbd6 100644 --- a/src/main/build/debug.h +++ b/src/main/build/debug.h @@ -79,6 +79,7 @@ typedef enum { DEBUG_GPS, DEBUG_LULU, DEBUG_SBUS2, + DEBUG_ESC, DEBUG_COUNT // also update debugModeNames in cli.c } debugType_e; diff --git a/src/main/fc/cli.c b/src/main/fc/cli.c index 9bb2c776883..cd3f64696d3 100644 --- a/src/main/fc/cli.c +++ b/src/main/fc/cli.c @@ -222,7 +222,8 @@ static const char *debugModeNames[DEBUG_COUNT] = { "HEADTRACKER", "GPS", "LULU", - "SBUS2" + "SBUS2", + "ESC" }; /* Sensor names (used in lookup tables for *_hardware settings and in status diff --git a/src/main/fc/settings.yaml b/src/main/fc/settings.yaml index 4e8affb0221..dccd8a22c8f 100644 --- a/src/main/fc/settings.yaml +++ b/src/main/fc/settings.yaml @@ -84,7 +84,7 @@ tables: "VIBE", "CRUISE", "REM_FLIGHT_TIME", "SMARTAUDIO", "ACC", "NAV_YAW", "PCF8574", "DYN_GYRO_LPF", "AUTOLEVEL", "ALTITUDE", "AUTOTRIM", "AUTOTUNE", "RATE_DYNAMICS", "LANDING", "POS_EST", - "ADAPTIVE_FILTER", "HEADTRACKER", "GPS", "LULU", "SBUS2"] + "ADAPTIVE_FILTER", "HEADTRACKER", "GPS", "LULU", "SBUS2", "ESC"] - name: aux_operator values: ["OR", "AND"] enum: modeActivationOperator_e diff --git a/src/main/io/serial_4way.c b/src/main/io/serial_4way.c index 446e089ae24..459e3a5d888 100644 --- a/src/main/io/serial_4way.c +++ b/src/main/io/serial_4way.c @@ -33,6 +33,7 @@ #include "drivers/pwm_output.h" #include "drivers/light_led.h" #include "drivers/system.h" +#include "build/debug.h" #include "flight/mixer.h" @@ -352,16 +353,21 @@ static uint8_t Connect(uint8_32_u *pDeviceInfo) return 1; } else { if (BL_ConnectEx(pDeviceInfo)) { + DEBUG_SET(DEBUG_ESC, 0, pDeviceInfo->words[0]); if SILABS_DEVICE_MATCH { CurrentInterfaceMode = imSIL_BLB; + DEBUG_SET(DEBUG_ESC, 1, 1); return 1; } else if ATMEL_DEVICE_MATCH { CurrentInterfaceMode = imATM_BLB; + DEBUG_SET(DEBUG_ESC, 1, 2); return 1; } else if ARM_DEVICE_MATCH { CurrentInterfaceMode = imARM_BLB; + DEBUG_SET(DEBUG_ESC, 1, 3); return 1; } + DEBUG_SET(DEBUG_ESC, 1, 0); } } #elif defined(USE_SERIAL_4WAY_BLHELI_BOOTLOADER) From de2280427b711b95039790b16967dd2c3b6e0083 Mon Sep 17 00:00:00 2001 From: Ray Morris Date: Sun, 13 Sep 2026 02:07:33 -0500 Subject: [PATCH 5/7] Address code review feedback on ESC passthrough fix Drop the unverified 4-way read timeout (Betaflight gates it behind USE_TIMEOUT_4WAYIF, which nothing defines upstream) while keeping the range-based SiLabs detection, reboot sequence and version bump that actually fix EFM8BB51x detection. Also clear DEBUG_ESC slots before each probe, record the interface mode on the STK path, and correct the mis-attributed provenance comments. --- src/main/io/serial_4way.c | 84 +++++++++++++++++---------------------- 1 file changed, 37 insertions(+), 47 deletions(-) diff --git a/src/main/io/serial_4way.c b/src/main/io/serial_4way.c index 459e3a5d888..7d87ed1c33b 100644 --- a/src/main/io/serial_4way.c +++ b/src/main/io/serial_4way.c @@ -21,6 +21,7 @@ #include #include "platform.h" +#include "build/debug.h" #ifdef USE_SERIAL_4WAY_BLHELI_INTERFACE @@ -33,7 +34,6 @@ #include "drivers/pwm_output.h" #include "drivers/light_led.h" #include "drivers/system.h" -#include "build/debug.h" #include "flight/mixer.h" @@ -81,12 +81,6 @@ #define SERIAL_4WAY_PROTOCOL_VER 108 // *** end -// Timeout values for 4-way interface communication (from Betaflight PR #13287) -#define CMD_TIMEOUT_US 50000 -#define ARG_TIMEOUT_US 25000 -#define DAT_TIMEOUT_US 10000 -#define CRC_TIMEOUT_US 10000 - #if (SERIAL_4WAY_VER_MAIN > 24) #error "beware of SERIAL_4WAY_VER_SUB_1 is uint8_t" #endif @@ -336,7 +330,7 @@ uint16_t _crc_xmodem_update (uint16_t crc, uint8_t data) { #define ATMEL_DEVICE_MATCH ((pDeviceInfo->words[0] == 0x9307) || (pDeviceInfo->words[0] == 0x930A) || \ (pDeviceInfo->words[0] == 0x930F) || (pDeviceInfo->words[0] == 0x940B)) -// Range-based detection for Bluejay/AM32 compatibility (from Betaflight PR #13287) +// Range-based detection for newer SiLabs BLHeli_S MCUs (EFM8BB51x reports 0xE8B5) #define SILABS_DEVICE_MATCH ((pDeviceInfo->words[0] > 0xE800) && (pDeviceInfo->words[0] < 0xF900)) // BLHeli_32 MCU ID hi > 0x00 and < 0x90 / lo always = 0x06 @@ -346,25 +340,32 @@ static uint8_t CurrentInterfaceMode; static uint8_t Connect(uint8_32_u *pDeviceInfo) { + // DEBUG_ESC: [0] = raw bootloader signature (words[0]), [1] = interface mode + // (imSIL_BLB=1 SiLabs/BLHeli_S-Bluejay, imATM_BLB=2 Atmel, imSK=3 SimonK, imARM_BLB=4 ARM/BLHeli32-AM32). + DEBUG_SET(DEBUG_ESC, 0, 0); + DEBUG_SET(DEBUG_ESC, 1, 0); + for (uint8_t I = 0; I < 3; ++I) { #if (defined(USE_SERIAL_4WAY_BLHELI_BOOTLOADER) && defined(USE_SERIAL_4WAY_SK_BOOTLOADER)) if ((CurrentInterfaceMode != imARM_BLB) && Stk_ConnectEx(pDeviceInfo) && ATMEL_DEVICE_MATCH) { CurrentInterfaceMode = imSK; + DEBUG_SET(DEBUG_ESC, 0, pDeviceInfo->words[0]); + DEBUG_SET(DEBUG_ESC, 1, imSK); return 1; } else { if (BL_ConnectEx(pDeviceInfo)) { DEBUG_SET(DEBUG_ESC, 0, pDeviceInfo->words[0]); if SILABS_DEVICE_MATCH { CurrentInterfaceMode = imSIL_BLB; - DEBUG_SET(DEBUG_ESC, 1, 1); + DEBUG_SET(DEBUG_ESC, 1, imSIL_BLB); return 1; } else if ATMEL_DEVICE_MATCH { CurrentInterfaceMode = imATM_BLB; - DEBUG_SET(DEBUG_ESC, 1, 2); + DEBUG_SET(DEBUG_ESC, 1, imATM_BLB); return 1; } else if ARM_DEVICE_MATCH { CurrentInterfaceMode = imARM_BLB; - DEBUG_SET(DEBUG_ESC, 1, 3); + DEBUG_SET(DEBUG_ESC, 1, imARM_BLB); return 1; } DEBUG_SET(DEBUG_ESC, 1, 0); @@ -395,26 +396,19 @@ static uint8_t Connect(uint8_32_u *pDeviceInfo) static serialPort_t *port; -static bool ReadByte(uint8_t *data, timeDelta_t timeoutUs) +static uint8_t ReadByte(void) { - timeUs_t startTime = micros(); - while (!serialRxBytesWaiting(port)) { - if (timeoutUs && (cmpTimeUs(micros(), startTime) > timeoutUs)) { - return true; // timeout occurred - } - } - *data = serialRead(port); - return false; // success + // need timeout? + while (!serialRxBytesWaiting(port)); + return serialRead(port); } static uint8_16_u CRC_in; -static bool ReadByteCrc(uint8_t *data, timeDelta_t timeoutUs) +static uint8_t ReadByteCrc(void) { - bool timedOut = ReadByte(data, timeoutUs); - if (!timedOut) { - CRC_in.word = _crc_xmodem_update(CRC_in.word, *data); - } - return timedOut; + uint8_t b = ReadByte(); + CRC_in.word = _crc_xmodem_update(CRC_in.word, b); + return b; } static void WriteByte(uint8_t b) @@ -455,13 +449,10 @@ void esc4wayProcess(serialPort_t *mspPort) bool isExitScheduled = false; while (1) { - bool timedOut = false; - // restart looking for new sequence from host do { CRC_in.word = 0; - // No timeout - BLHeliSuite32 waits indefinitely for input - ReadByteCrc(&ESC, 0); + ESC = ReadByteCrc(); } while (ESC != cmd_Local_Escape); RX_LED_ON; @@ -469,25 +460,23 @@ void esc4wayProcess(serialPort_t *mspPort) Dummy.word = 0; O_PARAM = &Dummy.bytes[0]; O_PARAM_LEN = 1; + CMD = ReadByteCrc(); + ioMem.D_FLASH_ADDR_H = ReadByteCrc(); + ioMem.D_FLASH_ADDR_L = ReadByteCrc(); + I_PARAM_LEN = ReadByteCrc(); - timedOut = ReadByteCrc(&CMD, CMD_TIMEOUT_US) || - ReadByteCrc(&ioMem.D_FLASH_ADDR_H, ARG_TIMEOUT_US) || - ReadByteCrc(&ioMem.D_FLASH_ADDR_L, ARG_TIMEOUT_US) || - ReadByteCrc(&I_PARAM_LEN, ARG_TIMEOUT_US); - - if (!timedOut) { - uint8_t i = I_PARAM_LEN; - InBuff = ParamBuf; - do { - timedOut = ReadByteCrc(InBuff++, DAT_TIMEOUT_US); - } while ((--i > 0) && !timedOut); + InBuff = ParamBuf; + uint8_t i = I_PARAM_LEN; + do { + *InBuff = ReadByteCrc(); + InBuff++; + i--; + } while (i != 0); - for (int8_t j = 1; (j >= 0) && !timedOut; j--) { - timedOut = ReadByte(&CRC_check.bytes[j], CRC_TIMEOUT_US); - } - } + CRC_check.bytes[1] = ReadByte(); + CRC_check.bytes[0] = ReadByte(); - if ((CRC_check.word == CRC_in.word) && !timedOut) { + if (CRC_check.word == CRC_in.word) { ACK_OUT = ACK_OK; } else { ACK_OUT = ACK_I_INVALID_CRC; @@ -604,7 +593,8 @@ void esc4wayProcess(serialPort_t *mspPort) case imARM_BLB: { BL_SendCMDRunRestartBootloader(&DeviceInfo); - // ESC reboot logic for Bluejay/AM32 (from Betaflight PR #14214) + // Bluejay/AM32 ESCs enter their bootloader after the signal line is + // pulled low briefly and released. if (rebootEsc) { ESC_OUTPUT; setEscLo(selected_esc); From c07bf73affdba9ef301ec295162b6a6f68182332 Mon Sep 17 00:00:00 2001 From: Ray Morris Date: Sun, 13 Sep 2026 02:10:31 -0500 Subject: [PATCH 6/7] Fix redefinition of loop variable in esc4wayProcess Reverting the timed reads hoisted the parameter-loop counter back into the esc4wayProcess loop body, colliding with the response-echo counter of the same name in the same scope. --- src/main/io/serial_4way.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/io/serial_4way.c b/src/main/io/serial_4way.c index 7d87ed1c33b..0b9d461ccd2 100644 --- a/src/main/io/serial_4way.c +++ b/src/main/io/serial_4way.c @@ -898,7 +898,7 @@ void esc4wayProcess(serialPort_t *mspPort) WriteByteCrc(ioMem.D_FLASH_ADDR_L); WriteByteCrc(O_PARAM_LEN); - uint8_t i = O_PARAM_LEN; + i = O_PARAM_LEN; do { while (!serialTxBytesFree(port)); From 5645e1f46bde6d93922ea90097ca700622a7468a Mon Sep 17 00:00:00 2001 From: Ray Morris Date: Mon, 14 Sep 2026 00:49:58 -0500 Subject: [PATCH 7/7] Fix compute-region-sizes.py dropping FAST_CODE from region reports The addr == 0 filter meant to skip unallocated debug/symbol metadata also caught .tcm_code, since F7/H7 place ITCM_RAM at ORIGIN 0x0 and link that FAST_CODE section there. Detect allocation via objdump -h's ALLOC flag instead of address, which is the only way to tell a real zero-origin section apart from metadata that's never actually placed. Verified against a real AOCODARCF722AIO build: the old filter silently dropped its 11192-byte ITCM_RAM usage from the region breakdown entirely; the fix reports it correctly alongside TCM and RAM. --- .github/scripts/compute-region-sizes.py | 63 ++++++----- .github/scripts/compute-region-sizes.test.py | 107 +++++++++++++++++++ 2 files changed, 143 insertions(+), 27 deletions(-) create mode 100644 .github/scripts/compute-region-sizes.test.py diff --git a/.github/scripts/compute-region-sizes.py b/.github/scripts/compute-region-sizes.py index f77be880302..1864e11b7ee 100755 --- a/.github/scripts/compute-region-sizes.py +++ b/.github/scripts/compute-region-sizes.py @@ -1,6 +1,6 @@ #!/usr/bin/env python3 """Compute per-linker-region byte usage for one .elf, from its companion -.map file (Memory Configuration table) and `arm-none-eabi-size -A` output. +.map file (Memory Configuration table) and `arm-none-eabi-objdump -h` output. Why: `arm-none-eabi-size -B` (Berkeley format, used for the flat flash/ram totals elsewhere in this pipeline) sums ALL writable sections into one @@ -60,41 +60,42 @@ def parse_memory_regions(map_path): def parse_section_sizes(elf_path, size_tool): - """Returns [(section_name, size, addr), ...] via `size -A` (sysv), the - one format that reports per-section addresses needed for region - matching (Berkeley's -B only gives family totals, no addresses).""" - out = subprocess.run([size_tool, '-A', elf_path], capture_output=True, text=True, check=True).stdout + """Returns [(section_name, size, addr), ...] for allocated sections only + (the SHF_ALLOC ELF flag - i.e. sections that actually occupy memory at + runtime), via `objdump -h`'s two-line-per-section format. Filtering on + that flag, rather than on address, is what correctly tells apart + never-placed debug/symbol metadata (addr 0) from a real, zero-origin + section like F7/H7's ITCM-resident .tcm_code (FAST_CODE). + + Derives the objdump binary from `size_tool` (same toolchain bin dir, + "-size" -> "-objdump") rather than taking a separate CLI argument, since + both are always installed side by side.""" + objdump_tool = re.sub(r'-size$', '-objdump', size_tool) + out = subprocess.run([objdump_tool, '-h', elf_path], capture_output=True, text=True, check=True).stdout + lines = out.splitlines() sections = [] - for line in out.splitlines(): - parts = line.split() - if len(parts) != 3: + for i, line in enumerate(lines): + m = re.match(r'\s*\d+\s+(\S+)\s+([0-9a-fA-F]+)\s+([0-9a-fA-F]+)\s+[0-9a-fA-F]+\s+[0-9a-fA-F]+', line) + if not m: continue - name, size_str, addr_str = parts - if name in ('section', 'Total'): + name, size_str, addr_str = m.groups() + flags_line = lines[i + 1] if i + 1 < len(lines) else '' + if 'ALLOC' not in flags_line: continue - try: - size, addr = int(size_str), int(addr_str) - except ValueError: + size, addr = int(size_str, 16), int(addr_str, 16) + if size <= 0: continue sections.append((name, size, addr)) return sections -def compute(elf_path, map_path, size_tool): - regions = parse_memory_regions(map_path) - if not regions: - return {} - +def assign_sections(sections, regions): + """Sums (name, size, addr) `sections` into whichever of `regions` + [(name, start, end), ...] each falls within. Split out from `compute()` + so the matching logic can be unit tested without touching the + filesystem or spawning a subprocess.""" usage = {name: 0 for name, _, _ in regions} - for _section_name, size, addr in parse_section_sizes(elf_path, size_tool): - # Non-allocated sections (debug info, symbol/string tables, comments) - # report addr 0 - they're never actually placed in memory, so they - # must be excluded explicitly rather than relying on address-range - # matching alone: a region whose own origin is 0x0 (e.g. some parts' - # ITCM alias) would otherwise false-match every one of them and - # report several megabytes of phantom "usage". - if size <= 0 or addr == 0: - continue + for _name, size, addr in sections: for name, start, end in regions: if start <= addr < end: usage[name] += size @@ -103,6 +104,14 @@ def compute(elf_path, map_path, size_tool): return {name: bytes_ for name, bytes_ in usage.items() if bytes_ > 0} +def compute(elf_path, map_path, size_tool): + regions = parse_memory_regions(map_path) + if not regions: + return {} + + return assign_sections(parse_section_sizes(elf_path, size_tool), regions) + + def main(): if len(sys.argv) not in (3, 4): print('usage: compute-region-sizes.py [size-tool]', file=sys.stderr) diff --git a/.github/scripts/compute-region-sizes.test.py b/.github/scripts/compute-region-sizes.test.py new file mode 100644 index 00000000000..017482129f8 --- /dev/null +++ b/.github/scripts/compute-region-sizes.test.py @@ -0,0 +1,107 @@ +#!/usr/bin/env python3 +"""Unit tests for compute-region-sizes.py (per-linker-region size breakdown). + +Run with: python3 .github/scripts/compute-region-sizes.test.py + +Pure-logic tests only - no filesystem/subprocess access, matching the +convention in size-diff-comment.test.js. Exercises assign_sections() +directly with synthetic (name, size, addr) sections rather than building +real .elf/.map files. +""" +import importlib.util +import pathlib +import unittest + +_SPEC = importlib.util.spec_from_file_location( + 'compute_region_sizes', + pathlib.Path(__file__).parent / 'compute-region-sizes.py', +) +crs = importlib.util.module_from_spec(_SPEC) +_SPEC.loader.exec_module(crs) + + +class AssignSectionsTest(unittest.TestCase): + def test_zero_address_allocated_section_is_counted(self): + # F7/H7's ITCM_RAM originates at 0x0 and holds real, allocated + # .tcm_code (FAST_CODE) - this must not be dropped as if it were + # unallocated metadata. + regions = [('ITCM_RAM', 0x00000000, 0x00004000)] + sections = [('.tcm_code', 22, 0x00000000)] + self.assertEqual(crs.assign_sections(sections, regions), {'ITCM_RAM': 22}) + + def test_non_allocated_sections_are_never_passed_in(self): + # parse_section_sizes() is responsible for excluding non-ALLOC + # sections (debug info, .comment, .ARM.attributes, symtab/strtab) + # before assign_sections() ever sees them - simulate that here by + # simply not including any such section in the input. + regions = [('ITCM_RAM', 0x00000000, 0x00004000)] + sections = [('.tcm_code', 22, 0x00000000)] + result = crs.assign_sections(sections, regions) + self.assertEqual(sum(result.values()), 22) + + def test_normal_ram_region_matching_unaffected(self): + regions = [('RAM', 0x20000000, 0x20020000), ('CCM', 0x10000000, 0x10010000)] + sections = [ + ('.data', 100, 0x20000000), + ('.bss', 200, 0x20000100), + ('.ccm_bss', 50, 0x10000000), + ] + self.assertEqual(crs.assign_sections(sections, regions), {'RAM': 300, 'CCM': 50}) + + def test_section_outside_any_region_is_dropped(self): + regions = [('RAM', 0x20000000, 0x20020000)] + sections = [('.some_other', 100, 0x90000000)] + self.assertEqual(crs.assign_sections(sections, regions), {}) + + def test_empty_regions_yields_empty_result(self): + self.assertEqual(crs.assign_sections([('.data', 10, 0x20000000)], []), {}) + + +class ParseSectionSizesObjdumpParsingTest(unittest.TestCase): + """Exercises the objdump -h text parsing in isolation by feeding it + through the same regex/flag logic parse_section_sizes() uses, without + invoking a real toolchain.""" + + def _parse(self, objdump_output): + import re + lines = objdump_output.splitlines() + sections = [] + for i, line in enumerate(lines): + m = re.match(r'\s*\d+\s+(\S+)\s+([0-9a-fA-F]+)\s+([0-9a-fA-F]+)\s+[0-9a-fA-F]+\s+[0-9a-fA-F]+', line) + if not m: + continue + name, size_str, addr_str = m.groups() + flags_line = lines[i + 1] if i + 1 < len(lines) else '' + if 'ALLOC' not in flags_line: + continue + size, addr = int(size_str, 16), int(addr_str, 16) + if size <= 0: + continue + sections.append((name, size, addr)) + return sections + + def test_itcm_section_at_zero_address_is_allocated(self): + out = ( + "t2.elf: file format elf32-littlearm\n\n" + "Sections:\n" + "Idx Name Size VMA LMA File off Algn\n" + " 0 .text 00000018 08000000 08000000 00010000 2**3\n" + " CONTENTS, ALLOC, LOAD, READONLY, CODE\n" + " 1 .tcm_code 00000016 00000000 08000018 00020000 2**1\n" + " CONTENTS, ALLOC, LOAD, READONLY, CODE\n" + " 2 .comment 00000033 00000000 00000000 00020016 2**0\n" + " CONTENTS, READONLY\n" + " 3 .ARM.attributes 0000002e 00000000 00000000 00020049 2**0\n" + " CONTENTS, READONLY\n" + ) + sections = self._parse(out) + names = {name for name, _, _ in sections} + self.assertIn('.tcm_code', names) + self.assertNotIn('.comment', names) + self.assertNotIn('.ARM.attributes', names) + tcm = next(s for s in sections if s[0] == '.tcm_code') + self.assertEqual(tcm, ('.tcm_code', 0x16, 0x00000000)) + + +if __name__ == '__main__': + unittest.main()