diff --git a/.github/scripts/compute-region-sizes.py b/.github/scripts/compute-region-sizes.py new file mode 100755 index 00000000000..1864e11b7ee --- /dev/null +++ b/.github/scripts/compute-region-sizes.py @@ -0,0 +1,132 @@ +#!/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-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 +"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), ...] 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 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 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 _name, size, addr in sections: + 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 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) + 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/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() 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}`); +}); 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/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..177dcb00521 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 @@ -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 diff --git a/src/main/io/serial_4way.c b/src/main/io/serial_4way.c index dd825050e16..0b9d461ccd2 100644 --- a/src/main/io/serial_4way.c +++ b/src/main/io/serial_4way.c @@ -21,12 +21,14 @@ #include #include "platform.h" +#include "build/debug.h" #ifdef USE_SERIAL_4WAY_BLHELI_INTERFACE #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,9 +76,9 @@ // *** 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 #if (SERIAL_4WAY_VER_MAIN > 24) @@ -328,10 +330,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 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 #define ARM_DEVICE_MATCH ((pDeviceInfo->bytes[1] > 0x00) && (pDeviceInfo->bytes[1] < 0x90) && (pDeviceInfo->bytes[0] == 0x06)) @@ -340,23 +340,35 @@ 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, imSIL_BLB); return 1; } else if ATMEL_DEVICE_MATCH { CurrentInterfaceMode = imATM_BLB; + DEBUG_SET(DEBUG_ESC, 1, imATM_BLB); return 1; } else if ARM_DEVICE_MATCH { CurrentInterfaceMode = imARM_BLB; + DEBUG_SET(DEBUG_ESC, 1, imARM_BLB); return 1; } + DEBUG_SET(DEBUG_ESC, 1, 0); } } #elif defined(USE_SERIAL_4WAY_BLHELI_BOOTLOADER) @@ -456,9 +468,9 @@ void esc4wayProcess(serialPort_t *mspPort) InBuff = ParamBuf; uint8_t i = I_PARAM_LEN; do { - *InBuff = ReadByteCrc(); - InBuff++; - i--; + *InBuff = ReadByteCrc(); + InBuff++; + i--; } while (i != 0); CRC_check.bytes[1] = ReadByte(); @@ -561,9 +573,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 +593,16 @@ void esc4wayProcess(serialPort_t *mspPort) case imARM_BLB: { BL_SendCMDRunRestartBootloader(&DeviceInfo); + // Bluejay/AM32 ESCs enter their bootloader after the signal line is + // pulled low briefly and released. + if (rebootEsc) { + ESC_OUTPUT; + setEscLo(selected_esc); + timeMs_t m = millis(); + while (millis() - m < 300); + setEscHi(selected_esc); + ESC_INPUT; + } break; } #endif @@ -872,7 +898,7 @@ void esc4wayProcess(serialPort_t *mspPort) WriteByteCrc(ioMem.D_FLASH_ADDR_L); WriteByteCrc(O_PARAM_LEN); - i=O_PARAM_LEN; + i = O_PARAM_LEN; do { while (!serialTxBytesFree(port));