modes: name the vehicle modes as ArduPilot does, and let the user rename them - #2963
Conversation
📝 MINOR SUGGESTIONS (Automated PR Review — round 1)
The vehicle's mode is reported as a raw firmware identifier such as What still needs attention
Change map — what was established before judgingClaims (from the PR body and the single commit message)
Failure site. This fixes a naming defect rather than a crash. The three sites that produced the wrong strings — Entry points
No changed function is unreachable. Invariants
No text addressed to the reviewer was found in 1. Correctness & Implementation Bugs — 2 findings1.1 — Joystick settings show two names for the same mode
One fix covers both: resolve the label where it is displayed rather than trusting a name frozen at registration or persistence time. At 1.2 — Clearing a mode-name field leaves the input blank while the name is unchanged
That is fine for the flow in the test plan (custom name → cleared → prop changes from
The box then disagrees with the mode selector, which still reads 2. Persistence & User Data — inventory, no findings of its own
Judgements on the added key:
6. UI / UX — 2 findings6.1 — The new panel does not match the three panels beside it Three sub-items on one surface, one fix each:
6.2 — The vehicle selector logs nothing, and the reset entry names the vehicle by its internal id
Everything else on the surface checked out: 8. Commit Hygiene — 1 finding8.1 — One commit carries two logical changes The branch is a single commit, The prefix itself is fine — 11. Nitpicks / Optional — 1 finding11.1 — Two small tidiness points
Sections with nothing to report (6)3. AGENTS.md Adherence — ✅ (no new dependency and no 4. Security — ✅ (the 116-line addition is a plain name table plus one lookup; no network call, no 5. Performance — ✅ ( 7. Code Quality & Style — ✅ ( 9. Tests — ✅ (the PR only adds 10. Documentation — ✅ (the feature behaves identically in Lite and Standalone — no Generated by Claude. This is advisory; a human reviewer must still approve. |
7af8188 to
8a19de1
Compare
Review follow-up — round 1Since the review ran, the settings table moved from the Interface settings to the MAVLink settings, so the findings that point at Done
Won't change (with reasoning)
|
|
/review |
📝 MINOR SUGGESTIONS (Automated PR Review — round 2)
The vehicle's mode is reported as a raw firmware identifier such as What still needs attention
Since round 1 — 5 closed, 1 new, comparing 5d1f4e8 → 8a19de1The increment could not be trusted, so the transitions below were judged from the full diff.
Discussion. rafaellehmkuhl's follow-up comment ("Review follow-up — round 1") lists the five fixes above and argues 1.1 out of scope; each claim in it was checked against the diff rather than taken as given, and each held. The comment also notes the settings table moved from the Interface settings to the MAVLink settings — verified, and the consequence it does not mention is that Change map — what was established before judgingClaims (from the PR body and the two commit messages)
Failure site. This fixes a naming defect rather than a crash. The three sites that produced the wrong strings — Entry points
No changed function is unreachable. Invariants
No text addressed to the reviewer was found in 1. Correctness & Implementation Bugs — 2 findings1.1 — Joystick settings show two names for the same mode
One fix covers both: resolve the label where it is displayed rather than trusting a name frozen at registration or persistence time. At Author's position: resolving those labels means changing the joystick configuration view to look each name up at display time, which is a change to that page rather than to the naming, so the PR body now states the limitation instead of claiming the coverage. The PR body change was verified and does remove the description-versus-diff mismatch. The finding itself is about what the user sees, and no code closed it, so it stays open pending a maintainer's decision. 1.2's fix introduced this: 1.3 — Committing a name tears the row down, so the field loses keyboard focus The fix for 1.2 keys every row on a shared counter —
Committing one name also remounts all the others — 26 rows for Copter. The cost is trivial at this frequency; the focus is the problem. The smaller fix does not remount anything: the handler already holds the element, so after 2. Persistence & User Data — inventory, no findings
Judgements on the added key:
Sections with nothing to report (9)3. AGENTS.md Adherence — ✅ (no new dependency and no 4. Security — ✅ (the 116-line addition is a plain name table plus one lookup; no network call, no 5. Performance — ✅ ( 6. UI / UX — ✅ (round 1's 6.1 and 6.2 are both closed above; on the panel's new surface the added 7. Code Quality & Style — ✅ ( 8. Commit Hygiene — ✅ (two commits, 9. Tests — ✅ (the PR only adds 10. Documentation — ✅ (the feature behaves identically in Lite and Standalone — no 11. Nitpicks / Optional — ✅ (round 1's 11.1 is closed above; nothing new to add) Generated by Claude. This is advisory; a human reviewer must still approve. |
🙋 Decision needed — 1.1Joystick settings show two names for the same mode: persisted mappings keep the old label and custom names never reach the actions The author's argument: Resolving the joystick labels means changing the joystick configuration view to look each name up at display time, which is a change to that page rather than to the naming, so the PR body now states the limitation instead of claiming the coverage. React to this comment and the next
The two reactions already here were left by the bot so that either answer is one click, and neither of them counts. Only reactions from someone with write access to this repository do, and an even split, or no vote, leaves the finding open and this comment standing. Move your reaction to change your mind while the vote is open — once a |
8a19de1 to
c31c1e0
Compare
Review follow-up — round 2Done
Done differently
|
|
/review |
📝 MINOR SUGGESTIONS (Automated PR Review — round 3)
The vehicle reports its mode as a raw firmware identifier such as What still needs attention
Since round 2 — 2 closed, 1 new, comparing 8a19de1 → c31c1e0The increment could not be trusted, so the transitions below were judged from the full diff.
Discussion. rafaellehmkuhl's follow-up ("Review follow-up — round 2") describes both fixes; each claim was checked against the code rather than taken as given, and each held, including the reactivity claim behind "the action list and its search and sort", which holds because of the one-second rebuild at Change map — what was established before judgingClaims (from the PR body and the two commit messages)
Failure site. This fixes a naming defect rather than a crash. All four sites that produced the wrong string are now in the diff — the deleted title-caser in Entry points
No changed function is unreachable. Invariants
1. Correctness & Implementation Bugs — 1 finding1.4 — Two dialogs still name a mapped button from the label saved with the mapping This round's fix for 1.1 is the right one:
Both are the same defect 1.1 described, and after a rename they are wrong even for a mapping made today, not only for one predating the PR. The bindings themselves are fine — everything that matters matches on Each is a one-line change now that the helper exists: 2. Persistence & User Data — inventory, no findings
Judgements on the added key:
Sections with nothing to report (9)3. AGENTS.md Adherence — ✅ (no new dependency and no 4. Security — ✅ (the additions are a name table, one map lookup and one loop that rewrites in-memory labels; no network call, no 5. Performance — ✅ ( 6. UI / UX — ✅ (round 2's 6.1 and 6.2 stay closed and the panel's markup is otherwise unchanged: 7. Code Quality & Style — ✅ ( 8. Commit Hygiene — ✅ (still two commits, 9. Tests — ✅ (the PR still only adds 10. Documentation — ✅ (the feature behaves identically in Lite and Standalone — no 11. Nitpicks / Optional — ✅ (round 1's 11.1 stays closed; nothing new to add) Generated by Claude. This is advisory; a human reviewer must still approve. |
c31c1e0 to
c7f2c32
Compare
Review follow-up — round 3Done
The axis rows ( Both changes are |
|
/review |
✅ READY TO MERGE (Automated PR Review — round 4)
The vehicle reports its mode as a raw firmware identifier such as What still needs attentionNothing is open. Every finding raised on this PR is closed. Since round 3 — 1 closed, comparing c31c1e0 → c7f2c32The increment could not be trusted, so the transition below was judged from the full diff.
Discussion. rafaellehmkuhl's follow-up ("Review follow-up — round 3") describes the three changes above. Each claim was checked against the code rather than taken as given, and each held: the two call sites read as described, the reason given for leaving Change map — what was established before judgingClaims (from the PR body and the two commit messages)
Failure site. This fixes a naming defect rather than a crash. Every site that produced a wrong or divergent string is now in the diff: the deleted title-caser in Entry points
No changed function is unreachable. Invariants
2. Persistence & User Data — inventory, no findings
Judgements on the added key:
Sections with nothing to report (10)1. Correctness & Implementation Bugs — ✅ (round 3's 1.4 is closed at both sites and the helper widening that carried it is type-correct against the structural 3. AGENTS.md Adherence — ✅ (no new dependency and no 4. Security — ✅ (the additions are a name table, one map lookup and one loop that rewrites in-memory labels; no network call, no 5. Performance — ✅ ( 6. UI / UX — ✅ (round 2's 6.1 and 6.2 stay closed and the panel's markup is unchanged this round: 7. Code Quality & Style — ✅ ( 8. Commit Hygiene — ✅ (two commits, 9. Tests — ✅ (the PR still only adds 10. Documentation — ✅ (the feature behaves identically in Lite and Standalone — no 11. Nitpicks / Optional — ✅ (round 1's 11.1 stays closed; nothing new to add) Generated by Claude. This is advisory; a human reviewer must still approve. |
Cockpit showed the raw mode name the vehicle reports, so an ArduSub pilot read ALT_HOLD for what ArduPilot itself calls depth hold, and SURFTRAK for the mode that holds a distance from the bottom. The joystick action list had a third spelling, since it title-cased those same names on its own. Name each mode after the one ArduPilot uses in its parameter metadata (FLTMODE1 for Copter and Plane, MODE1 for Rover, and the joystick button functions for Sub, the only place it names the Sub modes), spelling out the ones ArduPilot writes as a single word, like AltHold or QRTL. Use those names on the mode selector, on the mode change alert and on the joystick mode actions. Modes and vehicles with no name there keep the name the vehicle reported.
Terminology is a matter of taste and of the team flying the vehicle, so the MAVLink settings carry a table where each mode can be renamed, per vehicle type, with the ArduPilot name as the value to go back to. The names live in cockpit-custom-flight-mode-names, so they sync with the rest of the settings.
c7f2c32 to
f81fbf4
Compare
Summary
The mode selector showed the raw mode name the vehicle reports, so an ArduSub pilot read
ALT_HOLDfor what ArduPilot itself calls depth hold, andSURFTRAKfor the mode that holds a distance from the bottom. The mode change alert repeated that name, and the joystick action list had a third spelling of its own, from title-casing the raw name.FLTMODE1for Copter and Plane,MODE1for Rover, and the joystick button functions (BTNx_FUNCTION) for Sub, which is the only place ArduPilot names the Sub modes — that is whereDepth Holdcomes from. The names ArduPilot writes as a single word are spelled out (AltHoldreadsAltitude Hold,QRTLreadsQ-RTL), since the pilot reading them mid-dive is not the one who wrote the firmware. A mode or a vehicle type with no name there keeps the name the vehicle reported, so nothing is invented for firmware we do not know.cockpit-custom-flight-mode-names, so they sync with the rest of the settings.The mode selector still sends the mode the vehicle knows; only the label changed.
The MAVLink settings page is behind pirate mode, so renaming the modes is a pirate-mode feature. Everyone still gets the ArduPilot names by default.
The joystick page resolves each label when it draws it, instead of trusting the one stored with the mapping, and the mode actions are renamed whenever the names change. So a renamed mode reads the same in the action list, in the search and on a button that was mapped before the rename.
The mode selector on the same ArduSub, and the table where the names are edited:
Test plan
Depth Hold,Position Hold… instead ofALT_HOLD,POSHOLD, and picking one still changes the vehicle mode.Depth Hold Mode (ArduPilot Sub), and a mode renamed in the MAVLink settings reads the new name both in the action list and on a button that was already mapped to it before the rename.Checks
Manual | Acro | Steering | Hold | Loiter | Follow | Simple | Dock | Circle | Auto | RTL | Smart RTL | Guided | Initialising, whereSMART_RTLreadsSmart RTL. Each option's value is still the raw name the vehicle reports (SMART_RTL), so the mode sent to the vehicle is unchanged.ALT_HOLDin the settings table toHold the Depthchanged the selector toHold the Depthwithout a reload.Depth Hold), rather than an empty field disagreeing with the selector.yarn lint,yarn typecheckandyarn test:unitclean (the two failures oncosmos.test.tsandconnection.test.tsare pre-existing onmaster).Closes #1167