refactor: Use Kconfig to conditionally compile behaviors #2106
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
caksoylar
reviewed
Jan 10, 2024
bryanforbes
force-pushed
the
refactor/behaviors-kconfig
branch
from
b3e50a9
to
01a8a9d
Compare
bryanforbes
force-pushed
the
refactor/behaviors-kconfig
branch
from
01a8a9d
to
347cd10
Compare
petejohanson
requested changes
Feb 23, 2024
app/Kconfig.behaviors
Outdated
Comment on lines
75
to
78
| config ZMK_BEHAVIOR_RESET | ||
| bool | ||
| default y | ||
| depends on DT_HAS_ZMK_BEHAVIOR_RESET_ENABLED |
There was a problem hiding this comment.
So this one is a bit tricky... This is needs to be enables a bit more specifically for the right side of splits, even if it's not referenced in a keymap. There's a few of those, like the RBG behavior, that are either "local" or "global" so need this special handling.
There was a problem hiding this comment.
This makes sense. I've removed the Kconfig options for the behaviors we talked about in Discord and reverted them back to using the main Kconfig option where available. This means the reset behaviors are always compiled in for both sides.
app/Kconfig.behaviors
Outdated
| default y | ||
| depends on DT_HAS_ZMK_BEHAVIOR_RESET_ENABLED | ||
|
|
||
| config ZMK_BEHAVIOR_RGB_UNDERGLOW |
app/dts/behaviors/ext_power.dtsi
Outdated
| @@ -7,7 +7,7 @@ | |||
| / { | |||
| behaviors { | |||
| // Behavior can be invoked on peripherals, so name must be <= 8 characters. | |||
| ext_power: extpower { | |||
| /omit-if-no-ref/ ext_power: extpower { | |||
There was a problem hiding this comment.
This was intentional, so probably needs an added comment, instead of adding the omit-if-no-ref.
app/dts/behaviors/reset.dtsi
Outdated
| @@ -9,13 +9,13 @@ | |||
| / { | |||
| behaviors { | |||
| // Behavior can be invoked on peripherals, so name must be <= 8 characters. | |||
| sys_reset: sysreset { | |||
| /omit-if-no-ref/ sys_reset: sysreset { | |||
Because global behaviors have to exist on both the central and split regardless if the user references them in their keymap, we can't omit the behavior declaration if the user doen't reference it and decide to compile it later based on the existence of the behavior declaration. The best option seems to be to decide whether to compile those behaviors based on the feature Kconfig flag. This also means that the two reset behaviors will always be compiled into both sides.
bryanforbes
force-pushed
the
refactor/behaviors-kconfig
branch
from
347cd10
to
72f8c50
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This pull request updates the current behaviors to use
Kconfig.behaviorsandtarget_sources_ifdef()withinapp/CMakeLists.txtto conditionally compile behaviors based on the behaviors used in the user's keymap