-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Do not enable features by default #13608
Conversation
haslinghuis
commented
Apr 29, 2024
•
edited
edited
- Fixes Dashboard build option impedes Betaflight initialization and creates I2C errors. #13590
- Fixes Unnecessary features automatically enabled by default. #13602
- See also Improve features (+ cli) #13494 which should be merged first
- Remember Fix arming when GPS included in build but no active GNSS device attached + revert msp request for SatInfo #13244
Do you want to test this code? You can flash it directly from Betaflight Configurator:
WARNING: It may be unstable. Use only for testing! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be accompanied with documentation change.
Also, some features should be enabled - for example with OSD integrated FC, you want FEATURE_OSD almost universally.
src/main/config/config.c
Outdated
#ifdef USE_LED_STRIP | ||
featureEnableImmediate(FEATURE_LED_STRIP); | ||
#endif | ||
#ifdef USE_OSD |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For consideration:
#if defined(USE_OSD) && USE_OSD >= 2
It will allow config.h to #define USE_OSD 2
and make <target> OPTIONS='USE_OSD=2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was thinking about missing default features from unified_targets and adding in config.h something like:
#define DEFAULT_FEATURES (FEATURES_OSD)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@haslinghuis : https://github.com/betaflight/betaflight/pull/13494/files#diff-384c3b23c60da197eb7ecccd9bbfd19d68d96245f45181ec4db541e9c1014b92R418-R432
All that is necessary is to define autoFeatures by target
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated and tested.
@haslinghuis problem is that you want osd only on targets with integrated osd. the same with other features... |
@ledvinap OSD feature is also used when using MSP instead of MAX7456 |
I think the situations where you want it enabled by default are as follows:
|
Currently
|
src/main/target/common_post.h
Outdated
@@ -477,6 +477,10 @@ | |||
#endif | |||
#endif | |||
|
|||
#ifndef DEFAULT_FEATURES | |||
#define DEFAULT_FEATURES (FEATURE_OSD) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be depend on USE_OSD included.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed + rebased on master
Closing in favor of #13494 |
#13494 still enables some features by default, this issue isn't resolved and won't be until another PR to change the features is created, see https://github.com/betaflight/betaflight/pull/13494/files#r1605977635 |
Okay, will wait for #13494 to be merged and work from there. EDIT: rebased on master, removed some features which perhaps should be enabled manually. |