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

refactor(Kconfig): Extracted designer defaults out into new files #2537

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

Nick-Munnich
Copy link
Contributor

@Nick-Munnich Nick-Munnich commented Oct 6, 2024

This is an alternative approach to #1886, with the same end goal. All of the "designer configurable" defaults are extracted out to separate files named Kconfig.defaults. The definitions stay in the same Kconfig file, only the defaults move. The defaults are imported after the keyboard config options, allowing them to be overridden by designers.

I used my own judgement to decide whether an option should be treated as "designer configurable". In general:

  • Anything related to behaviors or how the keyboard would be used is a user config option and the designer has no business touching it
  • Anything listed under our docs that isn't strictly a user config option is made available to designers
  • Specific additional options are made available to designers, such as USB vendor ID -- companies who have their own VID may want to override this one.

There are also some options that are listed in the docs, which I don't think should be listed in the docs as I don't understand why either designers or users should have any business touching them. For example:

  • CONFIG_ZMK_USB_INIT_PRIORITY
  • CONFIG_ZMK_BLE_INIT_PRIORITY

I have included such flags where I was uncertain as "designer configurable" out of caution.

While at it, I added a name to the EC11 trigger mode choice, and updated some values listed in the docs to be accurate.

Hopefully this more explicit approach is more "palatable" than #1886 and can get merged easier...

app/Kconfig.defaults Outdated Show resolved Hide resolved
@Nick-Munnich
Copy link
Contributor Author

Moved the USB PID/VID stuff back to the standard Kconfig file. I realised that the Kconfig.defaults files should only be used for stuff that both the user and the designer may want to configure. For designer-only and user-only, they have their respective .conf files.

Copy link
Contributor

@petejohanson petejohanson left a comment

Choose a reason for hiding this comment

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

As a whole, happy with this, just needs some reformatting funkiness fixed.

app/src/split/bluetooth/Kconfig Outdated Show resolved Hide resolved
app/src/split/bluetooth/Kconfig Outdated Show resolved Hide resolved
app/src/split/bluetooth/Kconfig Outdated Show resolved Hide resolved
app/src/split/bluetooth/Kconfig Outdated Show resolved Hide resolved
app/src/studio/Kconfig Outdated Show resolved Hide resolved
app/src/studio/Kconfig Outdated Show resolved Hide resolved
@Nick-Munnich
Copy link
Contributor Author

Fixed formatting of comments as requested, did a pass to catch any that were previously misformatted (bunch of wrong spacings). While I was at it, I did a pass over the if blocks, moving lonely ones into depends on, and moving another one into a menu (bottom of this).

I also got rid of the if blocks in the Kconfig.defaults, as on reading the page again if blocks get turned into depends on internally, making the if statements in the defaults pages redundant.

I left those last changes as a separate commit, to let it be removed easily if you'd prefer them not to change like this. Let me know if you want me to squash that or remove it.

Copy link
Contributor

@petejohanson petejohanson left a comment

Choose a reason for hiding this comment

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

One main item from reviewing the code. Will test locally a bit once that's fixed. Thanks!

app/Kconfig.defaults Show resolved Hide resolved
@Nick-Munnich Nick-Munnich force-pushed the kconfig-refactor branch 3 times, most recently from b2e4db4 to 6dfe385 Compare November 16, 2024 00:32
@Nick-Munnich
Copy link
Contributor Author

Nick-Munnich commented Nov 16, 2024

Did some local testing, ended up popping the if-refactor. Turns out they are not equivalent, Zephyr docs are wrong there.

I built both halves of corne with n!nv2, nice60, and bt60_v2, and diff'd the resulting .config files against each other. For each of those, there is only one difference with the current version of the refactor: The location of the CONFIG_SPI flag in the resulting output file, be it a comment that it was unset or it being set to y.

Currently it is being grouped under the RGB_UNDERGLOW section, with this change it gets moved under the advanced section alongside I2C etc.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants