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

add 'include keyboard_features.mk' into build_keyboard.mk #8422

Merged
merged 5 commits into from
Aug 28, 2021

Conversation

mtei
Copy link
Contributor

@mtei mtei commented Mar 14, 2020

This is an alternative implementation of #6794 and #2046.

Description

keyboard_features.mk is a file containing keyboard post-processing rules.

Post-processing rules convert keyboard-specific shortcuts that represent combinations of standard options into QMK standard options.

keyboard_features.mk is a keyboard-local version of the functions performed by common_features.mk.

rules.mk and keyboard_features.mk are processed in the following order:

keyboards/KEYBOARD_NAME/rules.mk
keyboards/KEYBOARD_NAME/rev1/rules.mk

keyboards/KEYBOARD_NAME/keymap/KEYMAP_NAME/rules.mk
users/USER_NAME/rules.mk

keyboards/KEYBOARD_NAME/rev1/keyboard_features.mk. (this PR ADD)
keyboards/KEYBOARD_NAME/keyboard_features.mk       (this PR ADD)
common_features.mk

Example 1

The keyboard_features.mk file can interpret features of a keyboard-level before common_features.mk. For example, when your designed keyboard has the option to implement backlighting or underglow using rgblight.c, writing the following in the keyboard_features.mk makes it easier for the user to configure the rules.mk.

  • keyboards/top_folder/keymaps/a_keymap/rules.mk
    # Please set the following according to the selection of the hardware implementation option.
    RGBLED_OPTION_TYPE = backlight   ## none, backlight or underglow
  • keyboards/top_folder/keyboard_features.mk
    ifeq ($(filter $(strip $(RGBLED_OPTION_TYPE))x, nonex backlightx underglowx x),)
       $(error unknown RGBLED_OPTION_TYPE value "$(RGBLED_OPTION_TYPE)")
    endif
    
    ifeq ($(strip $(RGBLED_OPTION_TYPE)),backlight)
      RGBLIGHT_ENABLE = yes
      OPT_DEFS += -DRGBLED_NUM=30
    endif
    ifeq ($(strip $(RGBLED_OPTION_TYPE)),underglow)
      RGBLIGHT_ENABLE = yes
      OPT_DEFS += -DRGBLED_NUM=6
    endif

Example 2

Now, if you compile the following, you get an error.

make plaid:bakingpy
.......
light/backlight_driver_common.c:6:6: error: #error "Backlight pin/pins not defined. Please configure."
 #    error "Backlight pin/pins not defined. Please configure."
      ^~~~~
quantum/backlight/backlight_driver_common.c:22:36: error: 'BACKLIGHT_PIN' undeclared here (not in a function); did you mean 'BACKLIGHT_ENABLE'?
 static const pin_t backlight_pin = BACKLIGHT_PIN;
                                    ^~~~~~~~~~~~~
                                    BACKLIGHT_ENABLE    [ERRORS]
 | 
 | 
 | 
make[1]: *** [.build/obj_plaid_bakingpy/quantum/backlight/backlight_driver_common.o] Error 1
make: *** [plaid:bakingpy] Error 1
Make finished with errors

The reason for the error is that layouts/community/ortho_4x12/bakingpy/rules.mk does not have a plaid keyboard check.

ifneq ($(LAYOUTS_HAS_RGB), no)
  RGBLIGHT_ENABLE = yes
endif
AUDIO_ENABLE = no
ifeq ($(strip $(KEYBOARD)), zlant)
  BACKLIGHT_ENABLE = no
else ifeq ($(strip $(KEYBOARD)), 40percentclub/4x4)
  BACKLIGHT_ENABLE = no
else ifneq (, $(findstring lets_split, $(KEYBOARD)))
  BACKLIGHT_ENABLE = no
else
  BACKLIGHT_ENABLE = yes
endif

If you write the following in keyboards/plaid/keyboard_features.mk, the above compilation error will be resolved.

BACKLIGHT_ENABLE = no

You should write the same thing as above in the keyboard_features.mk for each keyboard of zlant, 40percentclub/4x4, and lets_split. Then layouts/community/ortho_4x12/bakingpy/rules.mk can be as simple as this:

ifneq ($(LAYOUTS_HAS_RGB), no)
  RGBLIGHT_ENABLE = yes
endif
AUDIO_ENABLE = no
BACKLIGHT_ENABLE = yes

Note

Drashna said about #6794:

I think that seeb's may be the better implementation, and the one that we should go with, long term. It supports every layer of the makefile stuff, which also allows for some pretty cool stuff.

Yes, #6794 is a bit rough implementation. However, #2046 was an implementation that was too difficult for me to understand.

So, in this PR, I tried again a straightforward and general implementation.

The Helix keyboard really need this PR feature. That's because it turned out yesterday that the QMK Configurator did not compile Helix firmware correctly since October 2019(#6952). (qmk/qmk_configurator#684) (qmk/qmk_configurator#714)

On branch https://github.com/mtei/qmk_firmware/tree/update_helix_rules_mk_using_keyboard_features_mk, I used this feature to modify rules.mk for all Helix keymaps, and confirmed that the compilation did not change.

Types of Changes

  • Core
  • Bugfix
  • New feature
  • Enhancement/optimization
  • Keyboard (addition or update)
  • Keymap/layout/userspace (addition or update)
  • Documentation

Issues Fixed or Closed by This PR

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 added tests to cover my changes.
  • I have tested the changes and verified that they work and don't break anything (as well as I can manage).

@drashna
Copy link
Member

drashna commented Mar 16, 2020

One thing I would like to add here. If we're moving stuff around, I think this needs a userspace level needs a second pass check too. And for much the same reasons that the helix style boards too, in fact.

And with the userspace check changed, so that it is consistent with specific to generic (keymap <-> user <-> keyboard). Specifically, to be more in line with the config.h handling.

Eg:

keyboards/KEYBOARD_NAME/rules.mk
keyboards/KEYBOARD_NAME/rev1/rules.mk
users/USER_NAME/rules.mk
keyboards/KEYBOARD_NAME/keymap/KEYMAP_NAME/rules.mk
keyboards/KEYBOARD_NAME/keymap/KEYMAP_NAME/keyboard_features.mk
keyboards/KEYBOARD_NAME/rev1/keyboard_features.mk
_users/USER_NAME/keyboard_features.mk_
keyboards/KEYBOARD_NAME/keyboard_features.mk
common_features.mk

Aside from this, I very much do like the change

@mtei
Copy link
Contributor Author

mtei commented Mar 16, 2020

@drashna, I understand the significance of your hope.

I looked it up.

% find users keyboards/keebio -name keyboard_features.mk
users/drashna/keyboard_features.mk
keyboards/keebio/iris/keymaps/drashna/keyboard_features.mk
keyboards/keebio/iris/keymaps/keyboard_features.mk
keyboards/keebio/iris/rev4/keyboard_features.mk
keyboards/keebio/iris/keyboard_features.mk
keyboards/keebio/keyboard_features.mk

% make keebio/iris/rev4:drashna:clean
QMK Firmware 0.8.37
==== this is keyboards/keebio/iris/rev4/rules.mk
==== this is keyboards/keebio/rules.mk
==== this is keyboards/keebio/iris/rules.mk
==== this is keyboards/keebio/iris/rev4/rules.mk
Making keebio/iris/rev4 with keymap drashna and target clean

== start build_keyboard.mk ====
==== this is keyboards/keebio/rules.mk
==== this is keyboards/keebio/iris/rules.mk
==== this is keyboards/keebio/iris/rev4/rules.mk
==== this is keyboards/keebio/iris/keymaps/drashna/rules.mk
==== this is users/drashna/rules.mk
==== this is keyboards/keebio/iris/rev4/keyboard_features.mk
==== this is keyboards/keebio/iris/keyboard_features.mk
==== this is keyboards/keebio/keyboard_features.mk
                 (((((Easy to insert here)))))
==== this is common_features.mk
== end build_keyboard.mk ====
== include tmk_core/rules.mk

It seems difficult for build_keyboard.mk to decide where to insert users/USER_NAME/keyboard_features.mk.

see https://github.com/mtei/qmk_firmware/tree/test_user_rules

@mtei
Copy link
Contributor Author

mtei commented Mar 18, 2020

@drashna

I was a bit misunderstood how build_keyboard.mk works.

In this PR, it will be processed as follows.

keyboards/KEYBOARD_NAME/rules.mk
keyboards/KEYBOARD_NAME/rev1/rules.mk
                                                   (drashna need HERE?)
keyboards/KEYBOARD_NAME/keymap/KEYMAP_NAME/rules.mk
users/USER_NAME/rules.mk

keyboards/KEYBOARD_NAME/rev1/keyboard_features.mk. (this PR ADD)
keyboards/KEYBOARD_NAME/keyboard_features.mk       (this PR ADD)
common_features.mk

see https://github.com/mtei/qmk_firmware/tree/test_user_rules if need test

@mtei
Copy link
Contributor Author

mtei commented Mar 20, 2020

@drashna , What do you think if it looks like this?

keyboards/KEYBOARD_NAME/rules.mk
keyboards/KEYBOARD_NAME/rev1/rules.mk

users/USER_NAME/user_rules.mk                      (NEW)
keyboards/KEYBOARD_NAME/keymap/KEYMAP_NAME/rules.mk
users/USER_NAME/rules.mk                           (Leave for backward compatibility)

keyboards/KEYBOARD_NAME/rev1/keyboard_features.mk. (this PR ADD)
keyboards/KEYBOARD_NAME/keyboard_features.mk       (this PR ADD)
users/USER_NAME/user_features.mk                   (NEW)

common_features.mk

@drashna
Copy link
Member

drashna commented Mar 21, 2020

I'm not a fan of leaving the rules.mk and adding a user_rules.mk, as it breaks from the normal naming convention.

If anything, I'd say just rename them, and have everything setup "correctly", but that would mean going through a breaking change. But that may be the best way to go about this.

@mtei
Copy link
Contributor Author

mtei commented Mar 21, 2020

If the change for 'users/USER_NAME/*. mk' is a breaking change, I'd like to make that change in another PR.

I now need this PR change to resolve qmk/qmk_configurator#684.

@vomindoraan
Copy link
Contributor

I agree with @drashna. Userspace rules.mk being processed after keymap rules.mk has been a major pet peeve of mine for a long time. I believe we will need a breaking change to be able to address that, though.

@mtei
Copy link
Contributor Author

mtei commented Mar 30, 2020

@vomindoraan and @drashna, I agree.

However, I would like to focus on the addition of keyboard_features.mk in this PR.

I think the improvement of rules.mk for user-level and the addition of user_features.mk should be proposed in a separate PR.

@mtei mtei requested review from skullydazed, drashna and a team and removed request for a team, drashna and skullydazed March 30, 2020 09:23
Copy link
Member

@noroadsleft noroadsleft left a comment

Choose a reason for hiding this comment

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

Don't own affected hardware, but everything seems to check out here.

@noroadsleft noroadsleft requested a review from a team April 4, 2020 05:02
Copy link
Member

@Erovia Erovia left a comment

Choose a reason for hiding this comment

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

LGTM (although consider this a weak approval)

@mtei
Copy link
Contributor Author

mtei commented Apr 12, 2020

@skullydazed @drashna, do you have any objections/opinions to this PR?

@skullydazed
Copy link
Member

I have a couple concerns here.

For example 1, that seems awfully complicated. I don't have a solution for this use case yet but I am still considering the problem.

For example 2, I think over time we have conflated the idea of enabling a feature with the idea that a feature is supported. Features should degrade gracefully if a keymap uses a feature a keyboard can't support. I have an alternative proposal to support this use case:

Add a new type of rules.mk flag to be used at the keyboard level: <FEATURE>_SUPPORTED

After processing all rules.mk files, when <FEATURE>_SUPPORTED=no is set we set <FEATURE>_ENABLED=no as well.

In your example 2, keyboards/plaid/rules.mk would add this line:

BACKLIGHT_SUPPORTED=no

After that compiling plaid:bakingpy should work without any other modifications.

An additional benefit of this proposal is that we can expose supported hardware features to the API, so Configurator could provide switches for enabling/disabling them, and possibly provide UI for configuring them.

@mtei
Copy link
Contributor Author

mtei commented Apr 21, 2020

For example 2, I think over time we have conflated the idea of enabling a feature with the idea that a feature is supported. Features should degrade gracefully if a keymap uses a feature a keyboard can't support. I have an alternative proposal to support this use case:

I agree with your suggestion in this case. It seems to be better.

@mtei
Copy link
Contributor Author

mtei commented Apr 21, 2020

For example 1, that seems awfully complicated. I don't have a solution for this use case yet but I am still considering the problem.

Ever since #2606, the Helix keyboard has been in need of this feature. In #2606, MakotoKurauchi and I aimed to give non-programmers the ability to configure in a simpler way. The standard QMK settings are too detailed and complex for them.

I've been waiting for #2046 or #6794 or this #8422.

And, with (qmk/qmk_configurator#684) and (qmk/qmk_configurator#714) reporting that Helix firmware cannot be generated, this feature is more strongly needed.

If I significantly rewrite the rules.mk and config.h under keyboards/helix/rev2/keymaps, we might be able to generate Helix firmware, even under the qmk_configurator environment. However, it is a breaking change, so I want to avoid it.

@skullydazed skullydazed mentioned this pull request May 9, 2020
8 tasks
@mtei
Copy link
Contributor Author

mtei commented Mar 4, 2021

I feel that processing.mk is ambiguous.

Since we have common_features.mk, keyboard_features.mk would be easier to understand. Is this not good enough?

Other candidates

  • prepare_build.mk
  • post_ processing.mk

@tzarc
Copy link
Member

tzarc commented Mar 4, 2021

we could also consider the following for alignment purposes:
config.h + post_config.h
rules.mk + post_rules.mk

@skullydazed
Copy link
Member

skullydazed commented Mar 4, 2021

I would prefer to avoid names that require prior knowledge to understand. A first-time keyboard maintainer should be able to reason about what the file is for based on the name alone. By that metric I don't like any of the names so far, but I do not have a better suggestion.

Of the proposed names so far post_rules.mk comes the closest to the criteria. I worry that keyboard maintainers are going to simply put everything in there, but that can be addressed at PR time for now.

@mtei mtei force-pushed the add_keyboard_features_mk branch from c7e49bb to 7418a4b Compare March 4, 2021 19:19
@mtei mtei changed the base branch from master to develop March 4, 2021 19:20
@skullydazed
Copy link
Member

I talked with @tzarc about our path forward with this PR. It should get merged in parallel with #12108 so that we can catch and properly enforce the split between rules and post_rules. Given the large impact that will have on qmk lint (and the full list of lint errors as shown by qmk generate-api) it was decided to merge this into develop first thing after the next merge.

In the interim I will work on splitting the existing rules.mk files into rules.mk and post_rules.mk so that is ready to merge at the same time, and everything will go in clean.

@mtei
Copy link
Contributor Author

mtei commented Aug 10, 2021

it was decided to merge this into develop first thing after the next merge.

Is the next merge mean the merge from develop to master on August 28?

@skullydazed
Copy link
Member

Is the next merge mean the merge from develop to master on August 28?

Yes.

@tzarc tzarc merged commit 9fe7b53 into qmk:develop Aug 28, 2021
skullydazed added a commit that referenced this pull request Aug 29, 2021
tzarc pushed a commit that referenced this pull request Aug 29, 2021
* Add check for non-assignment code in rules.mk

* fix lint check

* fix lint

* fixup to reflect the final state of #8422

* fix lint
@mtei mtei deleted the add_keyboard_features_mk branch August 29, 2021 03:09
cadusk pushed a commit to cadusk/qmk_firmware that referenced this pull request Sep 2, 2021
* qmk/develop:
  Remove bin/qmk (qmk#14231)
  ensure that the directory for sys.executable is in the user's path (qmk#14229)
  move everything from qmkfm/base_container to qmkfm/qmk_cli (qmk#14230)
  Nyquist (qmk#14202)
  Update the nix-shell environment (qmk#13316)
  [Keyboard] Add Viktus SP Mini (qmk#14069)
  [Keyboard] Corrected layout for np24 by YMDK (qmk#14096)
  [Keymap] My Prime E keymap (qmk#14117)
  [Keyboard] Add kelownaRGB64 (qmk#14141)
  [Keyboard] fix compile error `make helix/rev2/sc:all` (qmk#14214)
  fix automatic directory for qmk lint (qmk#14215)
  Add check for non-assignment code in rules.mk (qmk#12108)
  remove qmk console, which is now part of the global cli (qmk#14206)
  Fixup upstream paths for submodules. (qmk#14205)
  Bootmagic lite docs clarity. (qmk#14204)
  add 'include keyboard_features.mk' into build_keyboard.mk (qmk#8422)
  2021Q3 pre-merge `develop` changelog, keyboard aliases (qmk#14198)
ptrxyz pushed a commit to ptrxyz/qmk_firmware that referenced this pull request Apr 9, 2022
* add 'include keyboard_features.mk' into build_keyboard.mk

keyboard_features.mk is a keyboard-local version of the functions performed by common_features.mk.

* add comment into build_keyboard.mk

* added description of keyboard_features.mk in hardware_keyboard_guidelines.md.

* rename `keyboard_features.mk` to `post_rules.mk`
ptrxyz pushed a commit to ptrxyz/qmk_firmware that referenced this pull request Apr 9, 2022
* Add check for non-assignment code in rules.mk

* fix lint check

* fix lint

* fixup to reflect the final state of qmk#8422

* fix lint
BorisTestov pushed a commit to BorisTestov/qmk_firmware that referenced this pull request May 23, 2024
* add 'include keyboard_features.mk' into build_keyboard.mk

keyboard_features.mk is a keyboard-local version of the functions performed by common_features.mk.

* add comment into build_keyboard.mk

* added description of keyboard_features.mk in hardware_keyboard_guidelines.md.

* rename `keyboard_features.mk` to `post_rules.mk`
BorisTestov pushed a commit to BorisTestov/qmk_firmware that referenced this pull request May 23, 2024
* Add check for non-assignment code in rules.mk

* fix lint check

* fix lint

* fixup to reflect the final state of qmk#8422

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

Successfully merging this pull request may close these issues.

7 participants