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

Merged
merged 3 commits into from
Dec 4, 2024

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

BEGIN_COMMIT_OVERRIDE
feat(Kconfig): Allow overriding ZMK Kconfig defaults (#2537)

docs: Fix incorrect kconfig default values

Updated docs to match actual Kconfig defaults.

fix(Kconfig): Added a name to EC11's trigger mode choice

Kconfig choices need names to be defaulted elsewhere.

refactor(Kconfig): Moved designer defaults out into new files

Allow overriding defaults by setting ZMK defaults after board/shield defconfigs.

END_COMMIT_OVERRIDE

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 Nick-Munnich force-pushed the kconfig-refactor branch 2 times, most recently from 38e9a65 to d21eb6a Compare November 16, 2024 11:22
@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.

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.

Looks really good! One minor set of nitpicks left. Thanks!

app/src/split/bluetooth/Kconfig Outdated Show resolved Hide resolved
app/src/studio/Kconfig Outdated Show resolved Hide resolved
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.

Thanks!

@petejohanson petejohanson merged commit 40925d4 into zmkfirmware:main Dec 4, 2024
49 checks passed
@Nick-Munnich Nick-Munnich deleted the kconfig-refactor branch December 4, 2024 21:20
@caksoylar
Copy link
Contributor

Does this require any docs changes? Does it affect the warning note at https://zmk.dev/docs/development/hardware-integration/new-shield#user-configuration-files?

autoferrit added a commit to SpaceRockMedia/zmk that referenced this pull request Dec 7, 2024
…johanson/zmk into petejohanson-feat/pointers-with-input-processors

* 'feat/pointers-with-input-processors' of github.com:petejohanson/zmk: (186 commits)
  docs: Add initial pointer docs.
  feat: Add input split support.
  feat(mouse): Add mouse move and scroll support
  feat(boards): Update for mikoto board definition (zmkfirmware#1946)
  refactor(Kconfig): Extracted designer defaults out into new files (zmkfirmware#2537)
  chore(main): release 0.1.0 (zmkfirmware#2657)
  docs: Create a Hardware Integration index page (zmkfirmware#2634)
  docs: Mention combos in reset behaviors (zmkfirmware#2677)
  fix(boards): Disable uart serial node in Xiao BLE by default (zmkfirmware#2672)
  chore(deps): bump the prod-other-minor-patch group across 1 directory with 2 updates
  feat(boards): Add glove80 nexus node for extension GPIO. (zmkfirmware#2594)
  docs: Move defines to the end in layer behaviors (zmkfirmware#2639)
  docs: Update new-shield.mdx (zmkfirmware#2664)
  docs(ci): Netlify ignore command to check branch (zmkfirmware#2659)
  chore(deps): bump cross-spawn from 7.0.3 to 7.0.6 in /docs
  feat(ci): Add release-please automation with VERSION (zmkfirmware#2622)
  docs: Remove dangling 0 in sticky keys docs
  fix: include a header file for RC macros (zmkfirmware#2649)
  feat(drivers): Support init high/low in 595 driver
  docs: touchups on the soft off information for improved clarity. (zmkfirmware#2647)
  ...
autoferrit added a commit to SpaceRockMedia/zmk that referenced this pull request Dec 7, 2024
…fingerpunch

* petejohanson-feat/pointers-with-input-processors: (186 commits)
  docs: Add initial pointer docs.
  feat: Add input split support.
  feat(mouse): Add mouse move and scroll support
  feat(boards): Update for mikoto board definition (zmkfirmware#1946)
  refactor(Kconfig): Extracted designer defaults out into new files (zmkfirmware#2537)
  chore(main): release 0.1.0 (zmkfirmware#2657)
  docs: Create a Hardware Integration index page (zmkfirmware#2634)
  docs: Mention combos in reset behaviors (zmkfirmware#2677)
  fix(boards): Disable uart serial node in Xiao BLE by default (zmkfirmware#2672)
  chore(deps): bump the prod-other-minor-patch group across 1 directory with 2 updates
  feat(boards): Add glove80 nexus node for extension GPIO. (zmkfirmware#2594)
  docs: Move defines to the end in layer behaviors (zmkfirmware#2639)
  docs: Update new-shield.mdx (zmkfirmware#2664)
  docs(ci): Netlify ignore command to check branch (zmkfirmware#2659)
  chore(deps): bump cross-spawn from 7.0.3 to 7.0.6 in /docs
  feat(ci): Add release-please automation with VERSION (zmkfirmware#2622)
  docs: Remove dangling 0 in sticky keys docs
  fix: include a header file for RC macros (zmkfirmware#2649)
  feat(drivers): Support init high/low in 595 driver
  docs: touchups on the soft off information for improved clarity. (zmkfirmware#2647)
  ...
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.

3 participants