Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add 'Pilot name' to the Configurator UI; rename 'Display name' to 'Pilot name'; rename 'name' to 'craft_name' #2798

Merged

Conversation

krasiyan
Copy link
Contributor

@krasiyan krasiyan commented Feb 7, 2022

Fixes #1877

Related firmware changes @ betaflight/betaflight#11391


scope

This PR renames:

  • the Display name OSD / configuration element (and the internal FC.CONFIG.displayName) to Pilot name (hence FC.CONFIG.pilotName).
  • the internal FC.CONFIG.name to FC.CONFIG.craftName (with no functional impact)

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 messages for further details around the changes.


backwards compatibility

The configurator should still be backwards compatible with firmwares running MSP older than v1.45:

  • The DISPLAY_NAME and PILOT_NAME OSD elements have the same position within the elements list and are also semantically identical
  • The Display name element is still shown only on MSP versions before v1.45 (and the new Pilot name element is not visible) - e.g:
    display_name_backwards_compatibility
    • ℹ️ It uses the legacy display_name value - however it cannot be previewed nor managed through the UI.
  • The new Pilot name element is shown only on MSP versions greater or equal to v1.45 (and the old Display name element is not visible) - e.g:
    pilot_name
    • ℹ️ It uses the new pilot_name value - which can both be previewed and managed through the UI.

In addition, the craftName (ex. name) property will be retrieved set via:

  • MSP_NAME / MSP_SET_NAME for MSP versions before v1.45
  • MSP2_GET_TEXT / MSP_SET_TEXT for MSP v1.45 or greater

Testing:

  1. Get and flash the updated firmware from Rename 'display_name' to 'pilot_name'; rename 'name' to 'craft_name' ;Add the 'MSP2_GET_TEXT' and 'MSP2_SET_TEXT' MSP commands betaflight#11391
  • Alternatively you can see the UI changes by using the Virtual Mode. However that will not let you persist any changes.
  1. 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)
  2. See the changes in the Configuration and OSD tabs.

Screenshots:

image
image

@@ -4920,7 +4920,7 @@
"description": "One of the elements of the OSD"
},
"osdDescElementCraftName": {
"message": "Craft name as set in Configuration tab"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question to the reviewers:

Given that I changed some of the current en texts - should I do anything extra for all other translations?

Looking at #2791 I suspect I might have to manually remove them for now from all language files (so they can be re-translated in the future).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, this will be done automatically. Crowdin will delete the translations in the next language files update.

@krasiyan krasiyan force-pushed the 1877-display-name-configuration branch from 3473a6f to 89cf105 Compare February 7, 2022 03:59
@krasiyan krasiyan changed the title [WIP] Add Pilot name (Display name) to the Configurator UI Add Pilot name (Display name) to the Configurator UI Feb 7, 2022
@haslinghuis haslinghuis added this to the 10.9.0 milestone Feb 7, 2022
@krasiyan krasiyan force-pushed the 1877-display-name-configuration branch from 89cf105 to 26c9e75 Compare February 19, 2022 02:28
@sonarcloud
Copy link

sonarcloud bot commented Feb 19, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@github-actions

This comment has been minimized.

@ctzsnooze
Copy link
Member

I really like this

blckmn
blckmn previously approved these changes Apr 21, 2022
haslinghuis
haslinghuis previously approved these changes Apr 21, 2022
@blckmn
Copy link
Member

blckmn commented Apr 21, 2022

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 merge label NOT found -> PASS
  • at least one RN: label found -> PASS
  • Tested label found -> PASS
  • assigned to an approver -> FAIL
  • approver count at least three -> PASS

limonspb
limonspb previously approved these changes Jun 18, 2022
ctzsnooze
ctzsnooze previously approved these changes Oct 7, 2022
@krasiyan krasiyan dismissed stale reviews from ctzsnooze, limonspb, haslinghuis, and blckmn via c4d26c0 October 14, 2022 19:54
@krasiyan krasiyan force-pushed the 1877-display-name-configuration branch from 26c9e75 to c4d26c0 Compare October 14, 2022 19:54
@krasiyan krasiyan marked this pull request as draft October 14, 2022 19:54
@github-actions

This comment has been minimized.

@krasiyan krasiyan force-pushed the 1877-display-name-configuration branch from d380495 to 6389c70 Compare October 25, 2022 09:11
ctzsnooze
ctzsnooze previously approved these changes Oct 25, 2022
@github-actions

This comment has been minimized.

@ctzsnooze
Copy link
Member

ctzsnooze commented Oct 25, 2022

@krasiyan...
@haslinghuis and I have found zero backwards compatibility in configurator for firmware before 4.4.
PLEASE IGNORE - My testing was done, by mistake, with an early 4.4 build, not a 4.3 build.
Sorry.
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.

src/js/backup_restore.js Outdated Show resolved Hide resolved
src/js/main.js Outdated Show resolved Hide resolved
@krasiyan
Copy link
Contributor Author

krasiyan commented Oct 25, 2022

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:

osd_pre_msp_1_45

# 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. 👍

@ctzsnooze
Copy link
Member

ctzsnooze commented Oct 25, 2022

@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.

@ctzsnooze
Copy link
Member

@krasiyan @haslinghuis = please post a note here when you're happy it's good to go.

@krasiyan
Copy link
Contributor Author

@ctzsnooze No worries - I also confused myself at least several times while trying to set the right names everywhere 😅

@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 ^^

…lot name'; rename 'name' to 'craft_name'

- add pilot name (display_name) to the Configuration tab

  - add handling for the 'MSP2_GET_TEXT' and 'MSP2_SET_TEXT' commands

    - with support for the 'MSP2TEXT_PILOT_DISPLAY_NAME' ('displayName') config prop

  - backup handling of the 'displayName' config prop

  - add a text field to configure the pilot name in the 'Configuration/Personalization' box

    - using the 'display_name' FC config prop and the 'MSP2_GET_TEXT' / 'MSP2_SET_TEXT' MSP commands
    - add tooltips for both the 'Craft name' and 'Pilot name' fileds

  - rename the 'Display name' OSD element to 'Display name (Pilot name)'

    - expand the tooltip descriptions of 'Craft name' and 'Display name (Pilot name)'
    - change the default 'DISPLAY_NAME' OSD element preview to 'PILOT_NAME'

  - remove the default 'JOE PILOT' string value of the 'displayName' FC initial config

  - backwards compatibility handling for 'display_name' pre MSP v1.45

- rename 'display name' to 'pilot name'

  - add 'FC.CONFIG.pilotName' in place of 'FC.CONFIG.displayName'
  - add the 'PILOT_NAME' OSD element and keep backwards compatibility
    for the 'DISPLAY_NAME' OSD element (depending on the MSP version)

- rename 'FC.CONFIG.name' to 'FC.CONFIG.craftName'

  - add the 'MSP2TEXT_CRAFT_NAME' const for 'MSP2_GET_TEXT' / 'MSP2_SET_TEXT'
  - use 'MSP2_GET_TEXT' / 'MSP2_SET_TEXT' to get/set 'FC.CONFIG.craftName'
  - keep full backwards compatibility pre MSP v1.45 (using the legacy 'MSP_NAME' / 'MSP_SET_NAME')
@krasiyan krasiyan force-pushed the 1877-display-name-configuration branch from 1032f57 to 3873f82 Compare October 26, 2022 00:17
@krasiyan krasiyan requested a review from haslinghuis October 26, 2022 00:18
@sonarcloud
Copy link

sonarcloud bot commented Oct 26, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 2 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@chmelevskij
Copy link
Member

chmelevskij commented Oct 26, 2022

@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...

Copy link
Member

@chmelevskij chmelevskij left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for nice explanation @krasiyan 😊

I just have some questions about code style, but those are minor things.

Comment on lines +147 to +153
if (semver.gte(FC.CONFIG.apiVersion, API_VERSION_1_45)) {
configuration.CRAFT_NAME = FC.CONFIG.craftName;
configuration.PILOT_NAME = FC.CONFIG.pilotName;
} else {
configuration.CRAFT_NAME = FC.CONFIG.name;
configuration.DISPLAY_NAME = FC.CONFIG.displayName;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dunno, might be a stupid suggestion. But given that we have different names for fields, we could avoid version check by doing something like:

Suggested change
if (semver.gte(FC.CONFIG.apiVersion, API_VERSION_1_45)) {
configuration.CRAFT_NAME = FC.CONFIG.craftName;
configuration.PILOT_NAME = FC.CONFIG.pilotName;
} else {
configuration.CRAFT_NAME = FC.CONFIG.name;
configuration.DISPLAY_NAME = FC.CONFIG.displayName;
}
configuration.CRAFT_NAME = FC.CONFIG.craftName ?? FC.CONFIG.name;
configuration.PILOT_NAME = FC.CONFIG.pilotName ?? '';
configuration.DISPLAY_NAME = FC.CONFIG.displayName ?? '';

But this is up for discussion though.

Comment on lines +202 to +203
return semver.gte(FC.CONFIG.apiVersion, API_VERSION_1_45)
? MSP.promise(MSPCodes.MSP2_GET_TEXT, mspHelper.crunch(MSPCodes.MSP2_GET_TEXT, MSPCodes.MSP2TEXT_PILOT_NAME)) : Promise.resolve(true);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Anything returned from Promise is automatically wrapped in promise. Hence resolve is redundant.

Suggested change
return semver.gte(FC.CONFIG.apiVersion, API_VERSION_1_45)
? MSP.promise(MSPCodes.MSP2_GET_TEXT, mspHelper.crunch(MSPCodes.MSP2_GET_TEXT, MSPCodes.MSP2TEXT_PILOT_NAME)) : Promise.resolve(true);
return semver.gte(FC.CONFIG.apiVersion, API_VERSION_1_45)
? MSP.promise(MSPCodes.MSP2_GET_TEXT, mspHelper.crunch(MSPCodes.MSP2_GET_TEXT, MSPCodes.MSP2TEXT_PILOT_NAME)) : true;

Comment on lines +866 to +872
if (semver.gte(FC.CONFIG.apiVersion, API_VERSION_1_45)) {
FC.CONFIG.craftName = configuration.CRAFT_NAME;
FC.CONFIG.pilotName = configuration.PILOT_NAME;
} else {
FC.CONFIG.name = configuration.CRAFT_NAME;
FC.CONFIG.displayName = configuration.DISPLAY_NAME;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one could similary be refactored to use ?? and defaulting to things. Again up for discussion.

Comment on lines +760 to +762
const craftName = semver.gte(FC.CONFIG.apiVersion, API_VERSION_1_45)
? FC.CONFIG.craftName
: FC.CONFIG.name;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar here, technically these two are mutually exclusive

Suggested change
const craftName = semver.gte(FC.CONFIG.apiVersion, API_VERSION_1_45)
? FC.CONFIG.craftName
: FC.CONFIG.name;
const craftName = FC.CONFIG.craftName ?? FC.CONFIG.name;

Comment on lines +11 to +12
MSP_NAME: 10, // DEPRECATED IN MSP 1.45
MSP_SET_NAME: 11, // DEPRECATED IN MSP 1.45
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dunno if it will work straight away, but we could use jsdoc to add some intelissense about these.

Suggested change
MSP_NAME: 10, // DEPRECATED IN MSP 1.45
MSP_SET_NAME: 11, // DEPRECATED IN MSP 1.45
/**
* @deprecated in MSP >= 1.45
*/
MSP_NAME: 10,
/**
* @deprecated in MSP >= 1.45
*/
MSP_SET_NAME: 11,

.then(() => { return semver.gte(FC.CONFIG.apiVersion, "1.20.0") ? MSP.promise(MSPCodes.MSP_NAME) : true; })
.then(() => { return semver.gte(FC.CONFIG.apiVersion, "1.20.0") && semver.lt(FC.CONFIG.apiVersion, API_VERSION_1_45)
? MSP.promise(MSPCodes.MSP_NAME)
: Promise.resolve(true); })
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
: Promise.resolve(true); })
: true; })

: Promise.resolve(true); })
.then(() => { return semver.gte(FC.CONFIG.apiVersion, API_VERSION_1_45)
? MSP.promise(MSPCodes.MSP2_GET_TEXT, mspHelper.crunch(MSPCodes.MSP2_GET_TEXT, MSPCodes.MSP2TEXT_CRAFT_NAME))
: Promise.resolve(true); })
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
: Promise.resolve(true); })
: true; })

.then(() => { return semver.gte(FC.CONFIG.apiVersion, API_VERSION_1_31) ? MSP.promise(MSPCodes.MSP_RX_CONFIG) : true; })
.then(() => { return semver.gte(FC.CONFIG.apiVersion, API_VERSION_1_45)
? MSP.promise(MSPCodes.MSP2_GET_TEXT, mspHelper.crunch(MSPCodes.MSP2_GET_TEXT, MSPCodes.MSP2TEXT_PILOT_NAME)) : Promise.resolve(true); })
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
? MSP.promise(MSPCodes.MSP2_GET_TEXT, mspHelper.crunch(MSPCodes.MSP2_GET_TEXT, MSPCodes.MSP2TEXT_PILOT_NAME)) : Promise.resolve(true); })
? MSP.promise(MSPCodes.MSP2_GET_TEXT, mspHelper.crunch(MSPCodes.MSP2_GET_TEXT, MSPCodes.MSP2TEXT_PILOT_NAME)) : true; })

Comment on lines +378 to +384
if (semver.gte(FC.CONFIG.apiVersion, API_VERSION_1_45)) {
$('input[name="craftName"]').val(FC.CONFIG.craftName);
$('input[name="pilotName"]').val(FC.CONFIG.pilotName);
} else {
$('input[name="craftName"]').val(FC.CONFIG.name);
$('.pilotName').hide();
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (semver.gte(FC.CONFIG.apiVersion, API_VERSION_1_45)) {
$('input[name="craftName"]').val(FC.CONFIG.craftName);
$('input[name="pilotName"]').val(FC.CONFIG.pilotName);
} else {
$('input[name="craftName"]').val(FC.CONFIG.name);
$('.pilotName').hide();
}
$('input[name="craftName"]').val(FC.CONFIG.craftName ?? FC.CONFIG.name);
if (semver.gte(FC.CONFIG.apiVersion, API_VERSION_1_45)) {
$('input[name="pilotName"]').val(FC.CONFIG.pilotName);
} else {
$('.pilotName').hide();
}

? MSP.promise(MSPCodes.MSP2_SET_TEXT, mspHelper.crunch(MSPCodes.MSP2_SET_TEXT, MSPCodes.MSP2TEXT_CRAFT_NAME))
: MSP.promise(MSPCodes.MSP_SET_NAME, mspHelper.crunch(MSPCodes.MSP_SET_NAME)); })
.then(() => { return semver.gte(FC.CONFIG.apiVersion, API_VERSION_1_45) ?
MSP.promise(MSPCodes.MSP2_SET_TEXT, mspHelper.crunch(MSPCodes.MSP2_SET_TEXT, MSPCodes.MSP2TEXT_PILOT_NAME)) : Promise.resolve(true); })
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
MSP.promise(MSPCodes.MSP2_SET_TEXT, mspHelper.crunch(MSPCodes.MSP2_SET_TEXT, MSPCodes.MSP2TEXT_PILOT_NAME)) : Promise.resolve(true); })
MSP.promise(MSPCodes.MSP2_SET_TEXT, mspHelper.crunch(MSPCodes.MSP2_SET_TEXT, MSPCodes.MSP2TEXT_PILOT_NAME)) : true; })

@github-actions
Copy link
Contributor

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!

@haslinghuis haslinghuis merged commit 4f93487 into betaflight:master Oct 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Pilot Name to the Configurator UI
8 participants