Add PG validation tool for release preparation #11283
Open
Conversation
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
Implement automated validation of parameter group struct sizes during firmware builds to prevent settings corruption from struct changes without version increments. Features: - Validates all PG struct sizes against database after each build - Fails build if size changed without PG version increment - Auto-updates database when version IS properly incremented - No extra dependencies (uses standard nm tool) - Works on SITL and all hardware targets Implementation: - check-pg-struct-sizes.sh: Build-time validation script - extract-pg-sizes-nm.sh: Extract sizes from binaries using nm - generate-pg-database.sh: Generate database with sizes + versions - pg_struct_sizes.db: Database of 44 PG structs with current sizes/versions - Integrated into cmake/main.cmake as POST_BUILD step Testing verified: - Passes when no changes made - Fails when size changed without version increment - Passes and auto-updates when size changed with version increment - Database unchanged when validation fails Complements existing GitHub Action PR check with compile-time validation.
Add auto-addition of new PG structs to database: - cmake/check-pg-struct-sizes.sh - New structs with PG_REGISTER are automatically added to database - Tracks additions in update report Improve architecture detection robustness: - cmake/extract-pg-sizes-nm.sh - Check for arm-none-eabi-nm existence before use - Use more robust file type checking Note: Keeping automatic database updates when version IS incremented as per original design requirement.
Removed files not needed in public release: - cmake/PG-SIZE-EXTRACTION-NM.md (internal process documentation) - cmake/PG-SIZE-EXTRACTION-PAHOLE.md (alternative approach documentation) - cmake/PG-STRUCT-VALIDATION.md (internal design documentation) - cmake/generate-pg-database.sh (replaced by auto-add feature in check script) - cmake/compare-pg-sizes-nm.sh (developer utility, not needed for builds) Remaining files provide core build-time validation: - cmake/check-pg-struct-sizes.sh (main validation script) - cmake/extract-pg-sizes-nm.sh (size extraction utility) - cmake/pg_struct_sizes.db (struct size database)
Contributor
PR Compliance Guide 🔍All compliance sections have been disabled in the configurations. |
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
PROBLEM: - Single database used sizes from x86_64 SITL build - ARM targets have different struct sizes due to alignment/packing - Builds failed validation: batteryMetersConfig_t 36B→24B, etc. SOLUTION: 1. Architecture-specific databases: - pg_struct_sizes.arm.db for ARM targets - pg_struct_sizes.x86_64.db for x86_64 SITL 2. Auto-detect architecture from ELF file 3. Select appropriate database at build time 4. Extract PG versions in extract-pg-sizes-nm.sh IMPROVEMENTS: - Add argument validation to check-pg-struct-sizes.sh - Improve sed robustness with [[:space:]] patterns - Invoke scripts with bash explicitly for Windows compatibility Both SITL and hardware targets now validate correctly against architecture-appropriate struct sizes.
The extract-pg-sizes-nm.sh script was updated to output three fields (struct_type, size, version) but the validation loop was only reading two fields, causing the version to be appended to the size field. This resulted in error messages like: size changed 4B → 4 0B but version not incremented (still v0) Fixed by reading all three fields in the while loop and removing the now-redundant get_pg_version function.
The script had two echo statements for the validation start message: - Line 53: with () - correct - Line 55: without () - duplicate/leftover This caused confusing output in CI logs showing both: 🔍 Checking PG struct sizes against database (x86_64)... 🔍 Checking PG struct sizes against database... Removed the duplicate at line 55.
…order Move tag creation to final step after testing, add PG struct validation to pre-release checklist, and remove master branch references from instructions. Changes: - Add PG validation step to pre-release checklist - Reorder workflow: tags created after testing, not before - Add PG validation section with concise instructions - Remove 'master' from branch-specific instructions - Clarify SITL binary update timing
Replace architecture-specific databases with single reference database based on SPEEDYBEEF745AIO. Add release validation script for release managers. Changes: - Replace pg_struct_sizes.arm.db with pg_struct_sizes.reference.db - Remove pg_struct_sizes.x86_64.db (no longer needed) - Update check-pg-struct-sizes.sh to use reference database - Add cmake/validate-pg-for-release.sh for release preparation The reference-target approach validates only against SPEEDYBEEF745AIO configuration, avoiding the fundamental issue that struct sizes vary by build configuration, not just architecture.
sensei-hacker
changed the title
Merge check-pg-struct-sizes.sh and extract-pg-sizes-nm.sh into validate-pg-for-release.sh for simpler maintenance. Remove POST_BUILD hook since this is release-only validation, not per-build. Changes: - Consolidate three scripts into one validate-pg-for-release.sh - Remove cmake/check-pg-struct-sizes.sh (merged) - Remove cmake/extract-pg-sizes-nm.sh (merged) - Remove POST_BUILD hook from cmake/main.cmake - All functionality preserved in single 222-line script This is a release tool, not a build-time check, so consolidation makes sense.
Use mktemp for build log instead of hardcoded /tmp path to work with sandbox restrictions.
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.
Summary
Add PG struct validation tool for release preparation and update release documentation.
What This Does
cmake/validate-pg-for-release.sh) - Run before creating release tags to detect PG struct changes without version incrementsdocs/development/release-create.md) - Add PG validation to pre-release checklistWhy
Parameter Group structs store flight controller settings in EEPROM. If a developer changes a struct layout without incrementing the PG version, loading old settings causes memory corruption and crashes. This tool catches that mistake before releases.
Usage
Release managers run before tagging:
✅ Pass → proceed with release
❌ Fail → create hotfix PR to increment PG version, then retry
Implementation
cmake/validate-pg-for-release.sh- Release validation scriptcmake/pg_struct_sizes.reference.db- Reference sizes from SPEEDYBEEF745AIOdocs/development/release-create.md- Updated with PG validation stepLimitations
This is intentional - struct sizes vary by build configuration (#ifdef directives), making per-target or CI automation impractical. The reference-target approach catches most issues with zero impact on normal builds.