Add a <FEATURE>_SUPPORTED flag

[skullydazed]

This PR will allow a keyboard to exclude hardware it does not support.

Description

Currently we use <FEATURE>_ENABLED in rules.mk to turn support for a feature on and off. We also check the status of that variable to determine whether or that feature is in use. This presents a problem when community layouts and/or userspace use rules.mk to turn this feature on, as described in Example 2 on #8422.

This addresses that problem by allowing keyboards to set <FEATURE>_SUPPORTED variables. When set to no that feature will be disabled after all rules.mk processing has completed.

Types of Changes

• Core
• Enhancement/optimization
• Documentation

Checklist

• My code follows the code style of this project.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have read the CONTRIBUTING document.
• I have tested the changes and verified that they work and don't break anything (as well as I can manage).

[mtei]

It seems to be more maintainable to use foreach as follows.

# sample Makefile

## test data
AA_SUPPORTED = yes
BB_SUPPORTED = yes
CC_SUPPORTED = no

AA_FEATURE = yes # comment
BB_FEATURE = yes # comment
CC_FEATURE = yes # comment
## end of test data

# feature list
FEATURE_NAMES :=
FEATURE_NAMES += AA
FEATURE_NAMES += BB
FEATURE_NAMES += CC

# ???_SUPPORTED apply
$(foreach AFEATURE,$(FEATURE_NAMES),\
 $(if $(filter $($(AFEATURE)_SUPPORTED),no),$(eval $(AFEATURE)_FEATURE=no)))

all:
	@echo AA_FEATURE=$(AA_FEATURE)
	@echo BB_FEATURE=$(BB_FEATURE)
	@echo CC_FEATURE=$(CC_FEATURE)

[vomindoraan]

In #2046 (comment) you wrote:

We think most of the use cases for this feature are addressed by #9058. The big open question right now is what use cases are not addressed by that PR.

The main use case for that PR, at least for me, is being able to have a set of default <FEATURE>_ENABLE flags in my userspace rules.mk, which I can then override on a per-keymap basis (just like how the config.h hierarchy works) — while at the same time having the ability to conditionally include source files from my userspace if certain feature flags are set.

These two requirements are at odds with each other with the current architecture of having just rules.mk files. This is because the former requires userspace rules to be processed before keymap rules, and the latter requires them to be processed afterwards. It's therefore not currently possible to do what I described, so the common idiom that everyone uses is to keep the conditional includes in the userspace rules.mk, and copy-paste most or all of the feature flags between keymap rules.mk files — not ideal.

This is why adding a new class of makefiles to the build pipeline makes sense to me. These new makefiles would be set to run after all rules.mk are processed (a suitable name would be post_rules.mk, for analogy with post_config.h), and userspace rules would be moved to run before keymap rules so it's consistent with how config.h files behave. This is the ideal scenario.

I apologize if any of the above is stating the obvious, I just wanted to collect all of my thoughts on this matter in one place, for my own sake as well as for posterity.

---

Now, on to the solution proposed in this PR. While it does not address the issue described above, I do believe that it could be a step in the right direction. On its own, though, it has a problem, in that it isn't entirely in line with what was described in #8422 (comment):

Features should degrade gracefully if a keymap uses a feature a keyboard can't support.

If I have RGBLIGHT_ENABLE = yes in my userspace (or keymap), and I conditionally include a source file based on this flag, even if the keyboard sets RGBLIGHT_SUPPORTED = no, the source file will still be included (and likely error out during compilation). Having post_rules.mk alongside this change would avoid the issue, as the condition would only be checked after all of the feature flags had been resolved. ¬SUPPORTED would override ENABLED, and so the source file wouldn't be included.

However, in such a case, RGBLIGHT_ENABLE = no in the keymap rules.mk file would also work, and it would also override the userspace RGBLIGHT_ENABLE = yes (since userspace rules would be processed before keymap rules in the pipeline, as per my above description). So <FEATURE>_SUPPORTED may not be required at all, at least not for this use case.

[drashna]

This should probably be moved to before the userspace code here, so that the changes are applied before the userspace checks.

[skullydazed]

I've been thinking about this but I don't see why. Can you expand on your thinking here?

[vomindoraan]

As mentioned earlier, this currently doesn't disable the feature before the userspace rules.mk file is processed. This is problematic because that file will see the feature as enabled, even though the keyboard has flagged it as not supported.

Consider the following code in a userspace rules.mk file:

ifeq ($(strip $(BACKLIGHT_ENABLE)), yes)
    SRC += my_backlight.c
endif

If BACKLIGHT_ENABLE = yes is set in this user's community layout and/or userspace, my_backlight.c will be included and compiled even if the keyboard declares BACKLIGHT_SUPPORTED = no.

Drashna's suggestion of moving this line before the userspace code fixes this problem.

[skullydazed]

Could that be solved by incorporating this into your logic?

ifneq ($(strip $(BACKLIGHT_SUPPORTED)), no)

[vomindoraan]

Yes, it works when the check is changed to:

ifneq ($(strip $(BACKLIGHT_SUPPORTED)), no)
    ifeq ($(strip $(BACKLIGHT_ENABLE)), yes)
        SRC += my_backlight.c
    endif
endif
# ifeq (,$(filter no,$(BACKLIGHT_SUPPORTED) $(BACKLIGHT_ENABLE))) would work too

Is this something that you think would be worth doing for all similar checks moving forward? Or is the idea that <FEATURE>_SUPPORTED should automatically (and correctly) work with existing code that relies on just <FEATURE>_ENABLED?

[skullydazed]

I'm not sure I'm ready to make that call yet, right now I'm gathering data. This does seem like a potential solution for at least some use cases.

[vomindoraan]

Please see #9070 (comment) for another potential solution.

[drashna]

To be honest, I'm hard pressed to find a situation WRT keymaps and userspace that isn't served by the current configuration.

But part of that, is that I feel that hardware settings should NOT be set in the userspace, for any reason. It's generic configuration, and the workflow for it isn't really equipped/set up to handle that.

You use RGBLIGHT_ENABLE as an example here... and ... that was my first goto for examples for documentation. Setting it in the userspace rules ... well, just doesn't make sense to me. On any level. That should be set on the keyboard, and keymap/layout level.

Speaking of which... I think we absolutely do need the "Supported" setting. We ... technically already have it, in LAYOUT_HAS_RGB. I think that this PR should handle most of the cases that have complex checks needed. albeit with the above suggestion applied. I tested that out, and after the move... as long as you don't set it via MAKE arguments in the command line, it gracefully handles the deactivation of features, so that they are not compiled for. (without the change, it does error out in my userspace).

[skullydazed]

It seems to be more maintainable to use foreach as follows.

@mtei I agree that what you have is easier to maintain, but I can't get it to work. I have pushed my code to this branch and am testing with make clueboard/66_hotswap/gen1:default. I have pre-configured keyboards/clueboard/66_hotswap/gen1/rules.mk with AUDIO_SUPPORTED = no.

Using the series of if statements that works as I expect. Using the foreach you describe does not, I see lines like this in my compile output:

Compiling: quantum/process_keycode/process_audio.c                                                  [OK]
Compiling: quantum/process_keycode/process_clicky.c                                                 [OK]
Compiling: quantum/audio/audio_chibios.c                                                            [OK]
Compiling: quantum/audio/voices.c                                                                   [OK]
Compiling: quantum/audio/luts.c                                                                     [OK]

Edit- @drashna spotted the mistake and now this is working perfectly.

[vomindoraan]

But part of that, is that I feel that hardware settings should NOT be set in the userspace, for any reason. It's generic configuration, and the workflow for it isn't really equipped/set up to handle that.

You use RGBLIGHT_ENABLE as an example here... and ... that was my first goto for examples for documentation. Setting it in the userspace rules ... well, just doesn't make sense to me. On any level. That should be set on the keyboard, and keymap/layout level.

You're right about hardware settings. RGBLIGHT_ENABLE was the first thing that came to mind and I must admit I didn't think twice about it. I agree that hardware-specific settings shouldn't be in userspace rules.

But what about TAP_DANCE_ENABLE, COMMAND_ENABLE, UNICODE_ENABLE and other generic features that you might want to have enabled by default for all of your keymaps (or disabled, in the case of something like SPACE_CADET_ENABLE) — except for a few keymaps, where you want to do the opposite because, e.g., the board is running out of space? This is the actual use case.

At the moment, you have no way of disabling features in the keymap rules if you've enabled them in the userspace, and so you have to copy the flags into each keymap rules.mk file. If you want to change the default for a setting, or add a new one, you have to go through all of the files to make the change.

The proposed post_keymap.mk/post_rules.mk change would fix that. It would also have the benefit of reducing perceived complexity by making rules.mk behave in the expected manner (generic to specific, keyboard→userspace→keymap), consistent with the rest of the files. In other words, it would reduce the astonishment factor of this particular part of the build system.

That being said, I believe those changes can coexist with the <FEATURE>_SUPPORTED flags.

[mtei]

Don't forget about #8422 (keyboard-level post-rules.mk)

Currently, the QMK Configurator is not able to compile the Helix keyboard. I think we need a mechanism like this post-rules.mk on the core side to solve this.
(qmk/qmk_configurator#684) (qmk/qmk_configurator#714)

If you are rejecting #8422, please tell me another way to compile the Helix keyboard using the QMK Configurator.

[skullydazed]

If you are rejecting #8422, please tell me another way to compile the Helix keyboard using the QMK Configurator.

We haven't rejected anything yet, we're still exploring the problem space. This is one of those topics where a lot of competing interests intersect.

[drashna]

It's also a very, very complicated issue/subject/etc.

The question is, which is best long term, that does everything we need, without making it unmanageable later on.

[stale]

Thank you for your contribution!
This pull request has been automatically marked as stale because it has not had activity in the last 45 days. It will be closed in 30 days if no further activity occurs. Please feel free to give a status update now, or re-open when it's ready.
For maintainers: Please label with awaiting review, breaking_change, in progress, or on hold to prevent the issue from being re-flagged.

[skullydazed]

I've rebased this to current master and have added a bunch of _SUPPORTED rules so that a large number of make all:all errors are fixed. I believe this to be ready to go in.

This still leaves an open question around #8422 but I don't think merging this actually helps or hinders the issue #1 there. @mtei please correct me if I'm wrong.

[vomindoraan]

Has the team decided on what the best way to handle use cases described above by @mtei and myself should be moving forward? (Specifically, I'm referring to this, this and this; the latter is exemplified in vomindoraan@deb080b.)

Has a decision been made regarding the addition of post_keymap.mk (or similar) files to the build system, as proposed in #9070?

[skullydazed]

Has the team decided on what the best way to handle use cases described above by @mtei and myself should be moving forward? (Specifically, I'm referring to this, this and this; the latter is exemplified in vomindoraan/qmk_firmware@deb080b.)

No, but I'm increasingly coming around to the opinion that the proposed solutions are overly complex for the problem at hand, and will hinder our data driven efforts. I think drashna is right- userspace should not be trying to enable hardware features. They are keyboard specific and that configuration belongs in the keyboard realm.

Further discussion on this is offtopic for this PR and should be taken to an appropriate issue or PR. I'd like to refocus this PR on the issue at hand- giving keyboards a way to declare what hardware they don't support- which should not conflict with the other goals described.

[mtei]

This still leaves an open question around #8422 but I don't think merging this actually helps or hinders the issue #1 there. @mtei please correct me if I'm wrong.

I agree that this is ready for a merge.