Conversation
New string settings control_profile_name, battery_profile_name and mixer_profile_name (12 characters) stored in the profile structs, and MSP2_INAV_PROFILE_NAMES that returns the names of all slots in one reply so a configurator can label its profile selectors. Parameter group versions bumped for the three grown structs. Docs regenerated: Settings.md, msp_messages.json (2.1.1), README.
Three OSD elements show the user-defined name of the active control, battery and mixer profile. An unnamed slot shows the symbol and slot number instead so the element never renders blank. Builds on the profile names from the previous commit.
|
Checked with the CI-built SITL of this branch (fork run https://github.com/Raffi1202/inav/actions/runs/34445739959,
What the SITL cannot show is the character output of |
Reuse the CI fixes from iNavFlight#11885 and cover scalar, array and conditional registrations with regression fixtures.
Reuse the CI fixes from iNavFlight#11885 and cover scalar, array and conditional registrations with regression fixtures.
|
ⓘ Qodo reviews are paused because the subscription is no longer active. Ask your workspace admin to reactivate the subscription to resume reviews. Manage billing |
PR Summary by QodoAdd named profiles to settings, MSP, and OSD
AI Description
Diagram
High-Level Assessment
Files changed (19)
|
Code Review by Qodo
1.
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
| const controlConfig_t *currentControlProfile; | ||
|
|
||
| PG_REGISTER_ARRAY_WITH_RESET_FN(controlConfig_t, MAX_CONTROL_PROFILE_COUNT, controlProfiles, PG_CONTROL_PROFILES, 0); | ||
| PG_REGISTER_ARRAY_WITH_RESET_FN(controlConfig_t, MAX_CONTROL_PROFILE_COUNT, controlProfiles, PG_CONTROL_PROFILES, 1); |
There was a problem hiding this comment.
2. Firmware upgrades erase saved profiles 🐞 Bug ≡ Correctness
The PG version increments make pgLoad() reject the previously stored control, battery, and mixer profile records, because it copies EEPROM data only when the record version exactly matches the registered version. On the first boot after upgrading from the prior firmware, each affected record is reset before that failed version check, so existing tuning, battery thresholds, and mixer configuration all revert to defaults alongside the new empty name.
Agent Prompt
Issue description
The profile structs gained only a trailing name field, but incrementing their PG versions makes existing persisted records fail `pgLoad()`'s exact-version check. The loader resets the complete profile before skipping the old record, losing all pre-existing settings rather than initializing only the new name field.
Fix Focus Areas
- src/main/config/parameter_group.c[86-93]
- src/main/fc/control_profile.c[36-36]
- src/main/sensors/battery.c[119-119]
- src/main/flight/mixer_profile.c[72-77]
Recommended Fix
Add a deliberate migration path for the immediately preceding versions of these three profile PGs: retain the serialized prefix containing the old profile fields and leave the appended `name` bytes zero-initialized. Keep the version bumps so newly written records use the updated layouts, but do not reset compatible old records wholesale during this upgrade.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Replacing the shell checker with check-pg-versions.py changed the output
format but not the workflow that consumes it, and the two no longer agree.
pg-version-check.yml gates on the output carrying a "### " line:
if [ "$exit_code" -gt 1 ] || { [ "$exit_code" -eq 1 ] && ! grep -q '^### ' <<< "$output"; }; then
The Python checker printed "PG version issue: ..." with no such line, so on a
genuine finding (exit 1) the grep failed, the step exited 2 before writing
exit_code to GITHUB_OUTPUT, and "Post comment if issues found" never ran. The
check hard-failed instead of leaving the PR comment it exists to leave.
The github-script step has the same dependency from the other side: it starts
capturing at the first line containing "###", so without one the comment body
would have been empty even had the gate passed.
The checker now prints the heading the workflow is written around, splitting
each issue into "### `<name>`" and its detail, which is also what the shell
version produced. Replaying the gate condition: a finding without the heading
aborts with exit 2, with it the step continues and the renderer captures the
full text; a checker error (exit 2) still aborts, as intended.
|
Evaluated the bot review above against the sources. All four reported findings are either already handled here or do not hold — but the review missed one defect that this pull request introduces, and The one real problem: the check can no longer reach its own comment stepSwapping the shell checker for
if [ "$exit_code" -gt 1 ] || { [ "$exit_code" -eq 1 ] && ! grep -q '^### ' <<< "$output"; }; then
printf '%s\n' "$output"; exit 2
fiThe Python checker printed The Fixed by having the checker emit the heading the workflow is written around, splitting each issue into
On the four reported findings"Saved alternate layouts become corrupt" — already handled. "Firmware upgrades erase saved profiles" — the mechanism is right, the conclusion is not. Reset-on-version-mismatch is the intended design; "Generated display metadata stays stale" — already handled; "Some structure changes evade checks" — already handled, and this is the point of the rewrite. |
Those files belong to iNavFlight#11885, which replaces check-pg-versions.sh with a Python checker. Carrying a copy here only produces a conflict once either lands, and it is unrelated to this change.
1 participant
Problem
The OSD shows only the number of the active control profile:
OSD_ACTIVE_PROFILEdraws the profile symbol and the slot number, and there is no element at all for the active battery or mixer profile. Once profiles carry user-defined names (#11894), a pilot still cannot see the active profile by name. No issue is linked.Cause
src/main/io/osd.c:3301-3302(maintenance-10.x) formatsOSD_ACTIVE_PROFILEasSYM_PROFILEplusgetConfigProfile() + 1only, andosd_items_einsrc/main/io/osd.h:383-384ends atOSD_TERRAIN_AGL(171): no element exists for a profile name or for the battery and mixer profile.Change
Adds
OSD_CONTROL_PROFILE_NAME(172),OSD_BATTERY_PROFILE_NAME(173) andOSD_MIXER_PROFILE_NAME(174);OSD_ITEM_COUNTbecomes 175 and the generated enum metadata is refreshed. A newosdFormatProfileName()upper-cases the active slot'sname; an empty name prints the symbol (SYM_PROFILE,SYM_BATT_FULL,M) plus slot number so the element never renders blank. The extra items change the row stride ofosdLayoutsConfig_t::item_pos, soPG_OSD_LAYOUTS_CONFIGis bumped from 4 to 5 and stored OSD layouts reset once on upgrade.OSD_ACTIVE_PROFILEis unchanged. Thenamefields come from #11894, on which this branch is based.Test
SITL: fork run https://github.com/Raffi1202/inav/actions/runs/34445739959 (
SITL-WINat e0799bb), driven by the Configurator counterpart iNavFlight/inav-configurator#2750.MSP2_INAV_OSD_LAYOUTSreports 175 items, an enabledOSD_CONTROL_PROFILE_NAMEis stored on the FC and survives re-opening the OSD tab, and the preview showsCRUISEforcontrol_profile_name = Cruiseand the symbol plus slot number for an empty name. The latest commit builds all targets and SITL on fork CI: https://github.com/Raffi1202/inav/actions/runs/34765221823. Not run on hardware; the output on a physical display is unverified.Flash / RAM
Not measured yet. The upstream firmware CI has not been released for this PR, so no size report exists.
Docs
docs/OSD.md: rows 172-174 added to the element table (version 10.0.0).