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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

haslinghuis
Copy link
Member

@haslinghuis haslinghuis commented Apr 29, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
4 participants