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

Do not enable features by default #13608

Merged
merged 9 commits into from
May 21, 2024

Conversation

@haslinghuis haslinghuis self-assigned this Apr 29, 2024
Copy link

Do you want to test this code? You can flash it directly from Betaflight Configurator:

  • Simply put #13608 (this pull request number) in the Select commit field of the Configurator firmware flasher tab (you need to Enable expert mode, Show release candidates and Development).

WARNING: It may be unstable. Use only for testing!

Copy link
Contributor

@ledvinap ledvinap left a 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.

#ifdef USE_LED_STRIP
featureEnableImmediate(FEATURE_LED_STRIP);
#endif
#ifdef USE_OSD
Copy link
Contributor

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

Copy link
Member Author

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)

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated and tested.

@haslinghuis haslinghuis linked an issue Apr 29, 2024 that may be closed by this pull request
@ledvinap
Copy link
Contributor

@haslinghuis problem is that you want osd only on targets with integrated osd. the same with other features...

@haslinghuis
Copy link
Member Author

@ledvinap OSD feature is also used when using MSP instead of MAX7456

@hydra
Copy link
Contributor

hydra commented Apr 29, 2024

@haslinghuis problem is that you want osd only on targets with integrated osd. the same with other features...

I think the situations where you want it enabled by default are as follows:

  1. FC has OSD on-board. (MAX7456/AT7456, FrSkyOSD, SPRacingPixelOSD, etc)
  2. FC has HD/DJI OSD connector or is always paired with an MSP DisplayPort device (daughter board, stack, etc) and one serial port is configured by default to use MSP DisplayPort.

@haslinghuis
Copy link
Member Author

Currently

  • OSD_SD and OSD_HD is a default build option. User should choose which one or neither to use.
  • Including OSD_HD defaults to MSP displayport as switching between analog or hd video system does not work in respect to positioning the elements.

@haslinghuis haslinghuis added this to the 4.6 milestone Apr 29, 2024
src/main/target/common_pre.h Outdated Show resolved Hide resolved
@@ -477,6 +477,10 @@
#endif
#endif

#ifndef DEFAULT_FEATURES
#define DEFAULT_FEATURES (FEATURE_OSD)
Copy link
Member Author

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed + rebased on master

@haslinghuis
Copy link
Member Author

Closing in favor of #13494

@haslinghuis haslinghuis deleted the fix-features branch May 18, 2024 20:47
@hydra
Copy link
Contributor

hydra commented May 19, 2024

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

@haslinghuis haslinghuis restored the fix-features branch May 19, 2024 15:34
@haslinghuis
Copy link
Member Author

haslinghuis commented May 19, 2024

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.

@haslinghuis haslinghuis reopened this May 19, 2024
src/main/config/config.c Outdated Show resolved Hide resolved
@haslinghuis haslinghuis requested review from ledvinap and a team May 20, 2024 20:22
@haslinghuis haslinghuis merged commit 4ae4a06 into betaflight:master May 21, 2024
23 checks passed
@haslinghuis haslinghuis deleted the fix-features branch May 21, 2024 13:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
5 participants