betaflight-configurator
betaflight-configurator copied to clipboard
betaflight
Add 'Pilot name' to the Configurator UI (rename 'Display name' to 'Pilot name')
Fixes #1877
Related firmware changes @ https://github.com/betaflight/betaflight/pull/11391
scope
This PR renames the Display name OSD / configuration element (and the internal FC.CONFIG.displayName) to Pilot name (hence FC.CONFIG.pilotName).
It also adds the option to set the pilot name through the configurator UI. The value can be show in the OSD (independently of the current craft name which can already be managed through the configurator UI).
See the commit message for further details around the changes.
OSD backwards compatibility
The configurator should still be backwards compatible with firmwares running MSP older than v1.45:
- The
DISPLAY_NAMEandPILOT_NAMEOSD elements have the same position within the elements list and are also semantically identical - The
Display nameelement is still shown only on MSP versions beforev1.45(and the newPilot nameelement is not visible) - e.g:
- :information_source: It uses the legacy
display_namevalue - however it cannot be previewed nor managed through the UI.
- :information_source: It uses the legacy
- The new
Pilot nameelement is shown only on MSP versions greater or equal tov1.45(and the oldDisplay nameelement is not visible) - e.g:
- :information_source: It uses the new
pilot_namevalue - which can both be previewed and managed through the UI.
- :information_source: It uses the new
Testing:
- Get and flash the updated firmware from https://github.com/betaflight/betaflight/pull/11391
- Alternatively you can see the UI changes by using the
Virtual Mode. However that will not let you persist any changes.
- Run the configurator locally via
yarn start(I can also try to provide a build for a specific platform but I might not be able to test it myself) - See the changes in the
ConfigurationandOSDtabs.
Screenshots:
Kudos, SonarCloud Quality Gate passed! 
0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells
No Coverage information
0.0% Duplication
![sonarqubecloud[bot] avatar](https://avatars.githubusercontent.com/in/12526?v=4 "Published by a pub.dev verified publisher"){kind=small}
Do you want to test this code? Here you have an automated build: Betaflight-Configurator-Debug-Android Betaflight-Configurator-Debug-Linux Betaflight-Configurator-Debug-macOS Betaflight-Configurator-Debug-Windows WARNING: It may be unstable and result in corrupted configurations or data loss. Use only for testing!
![github-actions[bot] avatar](https://avatars.githubusercontent.com/in/15368?v=4 "Published by a pub.dev verified publisher"){kind=small}
AUTOMERGE: (FAIL)
- github identifies PR as mergeable -> PASS
- assigned to a milestone -> PASS
- cooling off period lapsed -> PASS
- commit count less or equal to three -> PASS
-
Don't mergelabel NOT found -> PASS - at least one
RN:label found -> PASS -
Testedlabel found -> PASS - assigned to an approver -> FAIL
- approver count at least three -> PASS
Do you want to test this code? Here you have an automated build: Betaflight-Configurator-Android Betaflight-Configurator-Linux Betaflight-Configurator-macOS Betaflight-Configurator-Windows WARNING: It may be unstable and result in corrupted configurations or data loss. Use only for testing!
Tested firmware 7ab4b93 today with Configurator build from commit 52c8de6.
Configurator interface in Configuration tab looks great!
This is a Long Overdue fix - THANK YOU!
I found one issue:
- change my 'Pilot Name' from
OLD_NAMEtoNEW_NAMEin the Configuration tab, - hit
Save and Reboot, - return to the OSD tab to check the outcome
-
PILOT_NAMEis shown.
Then:
- Return to OSD tab, or power cycle
- Open OSD tab, it shows
NEW_NAMEcorrectly
Same thing happens when changing the display_name value in CLI. Save the change, wait for the reboot, and go direct to OSD, and you will see PILOT_NAME, not your newly saved name. Briefly go to some other tab and return and OSD is correct.
This seems to be some internal updating issue, it's minor. The change is saved properly.
Apart from this small glitch, it works great!
Could I suggest that we change display_name in CLI and Configurator to pilot_name, and within the code itself. It's more work but we just have to 'bite the bullet' I think. From the end-user perspective, not only pilot name, but craft name, profile name and all such names are 'display names'. Calling just one of them 'display_name' doesn't make sense to me.
People will need to reconfigure their display_name value when updating, but I think that's OK, because it wasn't shown on the CLI previously anyway. I think 'pilot name' is much more likely to be found when searching CLI or OSD setup than 'display_name. Might as well tidy this up properly, at this point in time, that's my feeling. Only going to be harder to make the change in future.
Really looking forward to getting this merged once that small UI update issue is resolved.
@krasiyan as you are working on it please also solve the sonar code smells.
Testing with updated firmware assets (Matek F722 Mini bare board) the update is done after save and reboot without the need for a second refresh?
Please @ctzsnooze confirm as only the code smells need to addressed but shouldn't change functionality.
Do you want to test this code? Here you have an automated build: Betaflight-Configurator-Android Betaflight-Configurator-Linux Betaflight-Configurator-macOS Betaflight-Configurator-Windows WARNING: It may be unstable and result in corrupted configurations or data loss. Use only for testing!
Do you want to test this code? Here you have an automated build: Betaflight-Configurator-Android Betaflight-Configurator-Linux Betaflight-Configurator-macOS Betaflight-Configurator-Windows WARNING: It may be unstable and result in corrupted configurations or data loss. Use only for testing!
@ctzsnooze Thank you very much for the detailed review and reproduction steps! :bow: Indeed there was an issue with the refresh - we were missing an MSP call to retrieve the pilot name in src/js/serial_backend.js's processName() (this method seems to be called upon connecting to the FC). Even though it's an async "fire and forget" call it tends to load fast enough so the previews can be displayed when the user navigates to the OSD tab. Without that MSP call (as you discovered) one would have to navigate to the configuration tab in order to get FC.CONFIG.pilotName populated (and thus the OSD preview).
As for the display_name to pilot_name renaming - I fully support that idea as well! :grin: (Initially I was tempted to do it but I thought it might be somewhat out-of-scope). I did the changes here and in the firmware - at a glance they seem rather safe. Nevertheless I put them in separate commits in both PRs so if we have any concerns I won't mind taking them out to a separate PR.
P.S. There are now small notes on the OSD backwards compatibility in the PR descriptions. I tested with two live FCs (debug and one running 4.2) and couldn't spot any obvious regressions when manipulating the OSD elements.
@haslinghuis Thanks for the reminder for the Sonar code smells! I tackled a few of them but it seems that there are still 3 left which might require a broader refactoring of the existing code (or at least so far I haven't found a way to work around them more elegantly). E.g.:
-
MspHelper.prototype.crunch- already has inconsistent return types (which shouldn't be too hard to fix though)
- has a really high cognitive complexity which will likely only grow unless we break it down into separate methods
-
OSD.loadDisplayFieldsis already above the 15 complexity points threshold so it will likely need to be broken down as well.
Would you prefer if I did the refactoring now (as this MR has already gotten somewhat big and would anyway require testing) or should we postpone these for another round?
Only this one might be a problem:
MspHelper.prototype.crunch - already has inconsistent return types (which shouldn't be too hard to fix though).
Works good !!!
Kudos, SonarCloud Quality Gate passed!
0 Bugs
0 Vulnerabilities
0 Security Hotspots
2 Code Smells
No Coverage information
0.0% Duplication
Sorry for multiple comments.
My feeling is that we should make it clean, and consistently refer to pilot_name and pilotName, completely taking out displayName from Configurator.
And the old value name, which in truth means craftName, should be referred to as craftName. Much easier to find in the code like that.
In firmware we should make matching changes.
Gosh sorry there is so much complexity here. Can't wait to use it though.
Very grateful for all the time and effort in this PR. Thank you!
Do you want to test this code? Here you have an automated build: Betaflight-Configurator-Android Betaflight-Configurator-Linux Betaflight-Configurator-macOS Betaflight-Configurator-Windows WARNING: It may be unstable and result in corrupted configurations or data loss. Use only for testing!
This is really, really good. Tested and approved :-)
Personally I think it is ready for merge consideration.
@chmelevskij is there a way to mark these sonar issues as informational instead of failing the analysis (seems sonar is tightening the screws)
@krasiyan Please squash the commits and rebase to master, thanks. I can confirm that after doing this here, and in firmware, PilotName works perfectly.
@haslinghuis hey Mark, what should be done about these 'complexity' SonarCloud warnings?
Do you want to test this code? Here you have an automated build: Betaflight-Configurator-Android Betaflight-Configurator-Linux Betaflight-Configurator-macOS Betaflight-Configurator-Windows WARNING: It may be unstable and result in corrupted configurations or data loss. Use only for testing!
@krasiyan...
@haslinghuis and <STRIKE>I have found zero backwards compatibility in configurator for firmware before 4.4</STRIKE>.
<EDIT> PLEASE IGNORE - My testing was done, by mistake, with an early 4.4 build, not a 4.3 build.
Sorry.
<STRIKE>The UI won't show display_name in either the configuration or the OSD tabs, only CLI can be used.
Unfortunately, this problem will need to be solved before merging.</STRIKE>
Thank you very much for testing @ctzsnooze @haslinghuis ! And sorry for the omissions on my side!
As for:
The UI won't show display_name in either the configuration or the OSD tabs, only CLI can be used.
Before the current MR (thus MSP v1.45) we didn't have an input field for display_name in the Configuration tab. I am not sure if it can be added for MSP versions before 1.45 as we don't have any MSP command to retrieve it or update it. So in this case - are we OK to keep hiding the input field for MSP versions before v1.45?
As for it missing in the OSD tab - I will try to reproduce this issue since I can see it on an older 4.2.6 firmware running MSP v1.43:
# version
# Betaflight / STM32F411 (S411) 4.2.6 Jan 5 2021 / 19:07:43 (a4b6db1e7) MSP API: 1.43
# config: manufacturer_id: GEPR, board_name: GEPRCF411, version: 8758488d, date: 2019-12-29T10:31:32Z
# board: manufacturer_id: GEPR, board_name: GEPRCF411
Do you recall which MSP/Betaflight version you used?
@haslinghuis As for the backwards compatibility around FC.CONFIG.name / FC.CONFIG.craftName - thank you for the samples - I will update the rest of the occurrences accordingly. My initial idea was to permanently rename FC.CONFIG.name to FC.CONFIG.craftName (since it's completely internal to the configurator) and to use the appropriate MSP command depending on the MSP version. Thus this should have theoretically retained full backwards compatibility as the only change is the name of the internal variable in which we store the craft name.
However I agree that if we keep FC.CONFIG.name (similarly to how we kept FC.CONFIG.displayName alongside .pilotName) the deprecation will be a bit more explicit and easy to understand. :+1:
@krasiyan @haslinghuis You know what? I'm stupid.
I thought I was testing 4.3 firmware, but by mistake the quad had an early 4.4 flashed.
When testing with 'real' 4.3 firmware, it seems perfectly OK; I see 'Craft Name' in OSD and Configuration tab, the correct name appears, and it all works properly.
My bad. I can't find a problem now I'm testing properly. It seems to me that nothing needs to be changed, other than for readability or ease of maintenance. Apologies.
@krasiyan @haslinghuis = please post a note here when you're happy it's good to go.
@ctzsnooze No worries - I also confused myself at least several times while trying to set the right names everywhere :sweat_smile:
@haslinghuis I pushed the latest changes as per your recommendations and re-tested locally with a current and an older firmware - so far I also couldn't find any regressions :)
Nevertless - don't hesitate to let me know if you notice anything off ^^
Kudos, SonarCloud Quality Gate passed!
0 Bugs
0 Vulnerabilities
0 Security Hotspots
2 Code Smells
No Coverage information
0.0% Duplication
@chmelevskij is there a way to mark these sonar issues as informational instead of failing the analysis (seems sonar is tightening the screws)
@haslinghuis I loooked into this. Dunno much about sonar, but when I looked into these kind of things, often they are caused by having quite a few helper functions declared in the scope of the parent function. So solution would be to move them out to make them pure functions.
When doing massive changes these are pain in the bum...
Do you want to test this code? Here you have an automated build: Betaflight-Configurator-Android Betaflight-Configurator-Linux Betaflight-Configurator-macOS Betaflight-Configurator-Windows WARNING: It may be unstable and result in corrupted configurations or data loss. Use only for testing!