Add build-time PG struct size validation #11281
Closed
+238
−314
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
PROBLEM: PR iNavFlight#11276 modified blackboxConfig_s struct in blackbox.h without incrementing version in blackbox.c, and the workflow didn't catch it. ROOT CAUSE: Script processed files independently: - Checked blackbox.c → found PG_REGISTER but struct not in this file - Checked blackbox.h → found struct change but no PG_REGISTER - Never connected the two pieces together SOLUTION: 1. Expand file list to include companions that contain PG_REGISTER (if blackbox.h changed, also check blackbox.c for PG_REGISTER) 2. Search ALL changed files for struct definitions (when checking PG_REGISTER in blackbox.c, search all changed files including blackbox.h for the struct) 3. Detect when struct changed but PG_REGISTER wasn't touched (empty old_version and new_version → version not incremented) This correctly catches the most common bug: developer modifies struct in header file but forgets to increment version in source file.
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.
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}
1. Fix bash runtime error: Remove 'local' keyword from top-level variable - .github/scripts/check-pg-versions.sh:190 - Variable 'companion' was declared with 'local' outside of a function 2. 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 3. Improve architecture detection robustness - cmake/extract-pg-sizes-nm.sh:26 - 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)
Removed: - .github/scripts/check-pg-versions.sh (GitHub Action script) - .github/workflows/pg-version-check.yml (GitHub Action workflow) The build-time validation in cmake/check-pg-struct-sizes.sh provides superior coverage: - Catches issues during every build (not just PRs) - Works for local development builds - Auto-updates database when versions are incremented - Auto-adds new PG structs to database - Provides immediate developer feedback The GitHub Action approach is redundant with build-time validation.
Member
Author
|
Superseded by #11283 with clean commit history. The merge conflict was caused by commits that added GitHub Action files and then later removed them. The new PR contains only the build-time validation commits without the GitHub Action approach. |
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.
User description
Summary
Implements automated validation of parameter group (PG) struct sizes during firmware builds to prevent settings corruption from struct layout changes without proper PG version increments.
This complements the existing GitHub Action PR check (#11278) with compile-time validation that catches issues during local development builds.
Changes
Core Validation System
check-pg-struct-sizes.sh: Build-time validation script that:
extract-pg-sizes-nm.sh: Extracts PG struct sizes using
nm(no extra dependencies)pg_struct_sizes.db: Database of 44 PG structs with current sizes and versions
Build Integration
cmake/main.cmaketo run validation as POST_BUILD step on all targetscmake -E chdirto ensure correct working directoryTesting
Verified all scenarios work correctly:
Test procedure:
barometerConfig_tstruct by adding a fieldRelated
#11236
Limitations