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

Enable or disable features according to port configuration #2840

Merged

Conversation

haslinghuis
Copy link
Member

Fixes: #329

@haslinghuis haslinghuis added this to the 10.9.0 milestone Feb 28, 2022
@haslinghuis haslinghuis self-assigned this Feb 28, 2022
@haslinghuis haslinghuis force-pushed the enable_features_from_portsettings branch 2 times, most recently from b8dfb74 to 6dbfcf3 Compare February 28, 2022 19:52
@github-actions

This comment has been minimized.

@haslinghuis haslinghuis force-pushed the enable_features_from_portsettings branch from 6dbfcf3 to aee70af Compare February 28, 2022 20:13
@github-actions

This comment has been minimized.

@haslinghuis haslinghuis force-pushed the enable_features_from_portsettings branch 2 times, most recently from 6071317 to 7d234f2 Compare February 28, 2022 20:37
@github-actions

This comment has been minimized.

@McGiverGim
Copy link
Member

The idea is great, but has some sense to define the port and disable the feature?

If not, to me, the correct way is to remove completely the "feature" in the firmware, and make it enable it simply when defined the port.

@blckmn
Copy link
Member

blckmn commented Mar 1, 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 -> FAIL
  • assigned to an approver -> PASS
  • approver count at least three -> FAIL

Comment on lines 409 to 429
serialPortConfig.functions.forEach(e => {
if (e === 'RX_SERIAL') {
enableRxSerial = true;
}

if (e.startsWith("TELEMETRY")) {
enableTelemetry = true;
}
});

if (serialPortConfig.functions.indexOf('BLACKBOX') >= 0) {
enableBlackbox = true;
}

if (serialPortConfig.functions.indexOf('ESC_SENSOR') >= 0) {
enableEsc = true;
}

if (serialPortConfig.functions.indexOf('GPS') >= 0) {
enableGps = true;
}
Copy link
Member

Choose a reason for hiding this comment

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

Something like that woudl work as well

Suggested change
serialPortConfig.functions.forEach(e => {
if (e === 'RX_SERIAL') {
enableRxSerial = true;
}
if (e.startsWith("TELEMETRY")) {
enableTelemetry = true;
}
});
if (serialPortConfig.functions.indexOf('BLACKBOX') >= 0) {
enableBlackbox = true;
}
if (serialPortConfig.functions.indexOf('ESC_SENSOR') >= 0) {
enableEsc = true;
}
if (serialPortConfig.functions.indexOf('GPS') >= 0) {
enableGps = true;
}
let functions = serialPortConfig.functions
enableRxSerial = functions.includes('RX_SERIAL');
enableTelemetry = functions.some(e => e.startsWith("TELEMETRY"));
enableBlackbox = functions.includes('BLACKBOX');
enableEsc = functions.includes('ESC_SENSOR');
enableGps = functions.includes('GPS');

Copy link
Member Author

@haslinghuis haslinghuis Apr 19, 2022

Choose a reason for hiding this comment

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

Thanks. Much easier to read.

@haslinghuis haslinghuis force-pushed the enable_features_from_portsettings branch from 7d234f2 to bdffebd Compare April 19, 2022 21:16
@github-actions

This comment has been minimized.

@haslinghuis haslinghuis force-pushed the enable_features_from_portsettings branch from bdffebd to bfc9daf Compare April 19, 2022 21:31
@github-actions

This comment has been minimized.

@haslinghuis haslinghuis force-pushed the enable_features_from_portsettings branch 2 times, most recently from dff7a04 to 84670ef Compare April 19, 2022 21:46
@github-actions

This comment has been minimized.

chmelevskij
chmelevskij previously approved these changes Apr 20, 2022
@sonarcloud
Copy link

sonarcloud bot commented Apr 20, 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.

@haslinghuis haslinghuis requested a review from McGiverGim June 23, 2022 23:48
Copy link
Member

@McGiverGim McGiverGim left a comment

Choose a reason for hiding this comment

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

As requested, I can approve it if you want, but as I said, I think this is not the correct approach.

This is a workaround more that we need to maintain in the Configurator.

I think that if we want that something is enabled or disabled automatically, is better to remove the feature in the firmware and simply make it enabled by default. Or if we want to maintain the feature, enable it in the firmware not in the Configurator when it receives the MSP command.

The problem with the Configurator is that we need to maintain backward compatibility, if the things change between versions, we need to maintain old and new code. If we do this in the firmware part and it changes in a future, the old code is simply removed and new code replaces it.

@limonspb
Copy link
Member

limonspb commented Jun 24, 2022

As requested, I can approve it if you want, but as I said, I think this is not the correct approach.

This is a workaround more that we need to maintain in the Configurator.

I think that if we want that something is enabled or disabled automatically, is better to remove the feature in the firmware and simply make it enabled by default. Or if we want to maintain the feature, enable it in the firmware not in the Configurator when it receives the MSP command.

The problem with the Configurator is that we need to maintain backward compatibility, if the things change between versions, we need to maintain old and new code. If we do this in the firmware part and it changes in a future, the old code is simply removed and new code replaces it.

i would agree, sounds reasonable

@limonspb limonspb self-requested a review June 24, 2022 06:25
@mituritsyn
Copy link
Contributor

mituritsyn commented Jun 24, 2022

Could VTX tab be hided with the same logic in this PR? Discussed it with @limonspb a week ago and was going to open own PR.

P.S. I like @McGiverGim proposion

@haslinghuis haslinghuis marked this pull request as draft September 18, 2022 12:21
@haslinghuis haslinghuis marked this pull request as ready for review October 14, 2022 01:00
@haslinghuis haslinghuis marked this pull request as draft October 14, 2022 01:01
@haslinghuis haslinghuis force-pushed the enable_features_from_portsettings branch from c9de010 to ee3b53c Compare November 14, 2022 15:27
@sonarcloud
Copy link

sonarcloud bot commented Nov 14, 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

@haslinghuis haslinghuis marked this pull request as ready for review November 14, 2022 15:32
@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 4df7f5b into betaflight:master Nov 14, 2022
@haslinghuis haslinghuis deleted the enable_features_from_portsettings branch November 14, 2022 21:30
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.

Use serial port settings to preselect features
6 participants