Release/9.1 to master - #11945
Merged
Merged
Release/9.1 to master#11945
Conversation
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.
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
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.
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.
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.
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.
…jay-device-id Fix ESC passthrough for Bluejay/AM32 ESCs (EFM8BB51x device detection)
…slope Document rangefinder requirement for FW autoland flare phase
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.
…ion-reporting Report CI size-diff RAM delta per linker region, not combined
Contributor
|
ⓘ Qodo reviews are paused because the subscription is no longer active. Ask your workspace admin to reactivate the subscription to resume reviews. Manage billing |
Branch Targeting SuggestionYou've targeted the
If This is an automated suggestion to help route contributions to the appropriate branch. |
PR Summary by QodoImprove linker-region size reports and ESC flashing support
AI Description
Diagram
High-Level Assessment
Files changed (10)
|
Code Review by Qodo
1. The size report never shows regions
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
Comment on lines
+64
to
+67
| 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; |
There was a problem hiding this comment.
2. Changed memory layouts show false deltas 🐞 Bug ≡ Correctness
diffSizeReports unions both region-name sets and substitutes zero for any region absent from one report, despite identifying that situation as incomparable. When a target gains, loses, or renames a linker region, reviewers see its entire usage reported as growth or shrinkage and the synthetic change can also trigger the warning marker.
Agent Prompt
## Issue description
Regional comparison treats a missing region as zero usage, creating false deltas when linker-region names differ between the PR and baseline.
## Fix Focus Areas
- .github/scripts/size-diff-comment.js[58-74]
- .github/scripts/size-diff-comment.test.js[419-435]
## Recommended Fix
Only construct `regionDeltas` when both reports contain identical region-name sets. If either breakdown is absent or their key sets differ, leave `regionDeltas` undefined so rendering and notability checks fall back to the combined RAM delta; add tests covering added, removed, and renamed regions.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+73
to
+76
| map="${elf}.map" | ||
| regions='{}' | ||
| if [ -f "$map" ]; then | ||
| regions=$(python3 "${SCRIPT_DIR}/compute-region-sizes.py" "$elf" "$map" "$SIZE_TOOL") || regions='{}' |
There was a problem hiding this comment.
1. The size report never shows regions 🐞 Bug ≡ Correctness
extract-size-report.sh constructs the map path as ${elf}.map, turning a discovered
<target>.elf into <target>.elf.map even though CMake 3.15 and newer emits <target>.map from
TARGET_FILE_BASE_NAME. In CI CMake/Ninja builds, the failed existence check skips regional
extraction, omits the empty regional results from the final JSON, and silently leaves every report
with the old combined RAM figure.
Agent Prompt
## Issue description
The extraction script searches for `<target>.elf.map`, while current CMake firmware builds generate `<target>.map`. This prevents per-region size computation in CI and silently leaves reports with the old combined RAM totals.
## Fix Focus Areas
- .github/scripts/extract-size-report.sh[68-76]
- cmake/stm32.cmake[215-221]
- cmake/at32.cmake[207-213]
## Recommended Fix
Derive the primary map path by replacing the ELF file's `.elf` suffix with `.map` while preserving its containing directory, for example with `${elf%.elf}.map`. To retain compatibility with repository builds using CMake versions older than 3.15, check the legacy `${elf}.map` path if the primary path does not exist. Add an integration-level test or fixture verifying that a map generated beside `target.elf` as `target.map` is discovered.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
RAM / Flash usage vs. base commit
See RAM/flash optimization guide for techniques to reduce usage. |
|
Test firmware build ready — commit Download firmware for PR #11945 245 targets built. Find your board's
|
Member
Author
|
Followed up on Qodo's two findings here directly on
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.