-
-
Notifications
You must be signed in to change notification settings - Fork 39k
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
RGB Matrix Lighting implementation for Novelkeys NK87 #19422
Conversation
The RGB Matrix Lighting can't be configured with VIA, so VIA keymap has RGB Matrix Lighting disabled and uses RGB Backlight. Default keymap has RGB Matrix Lighting enabled.
The g_led_config should have correct map of led's physical position in so effects like heat map will work properly. The backspace led position in matrix on the right from enter key (like in iso) but physically on the top from enter (like ansi).
Code from the config_led.* files should be moved to nk87.c or config.h files.
Minor update to make layer_state_set_user function more readable
Switching to the rgb_matrix_indicators_kb() since no parameters of the rgb_matrix_indicators_advanced_kb() are not used.
Making preprocessor directives indentation style compliant with the qmk coding conventions.
The SRC build option from the via keymap to keyboard level post_rules.mk file to avoid potential duplication.
Thank you for your contribution! |
Co-authored-by: Ryan <fauxpark@gmail.com>
Thank you for your contribution! |
ifeq ($(strip $(RGB_BACKLIGHT)),yes) | ||
|
||
# Disable RGB Matrix | ||
RGB_MATRIX_ENABLE = no |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Honestly, I'd opt for this:
RGB_MATRIX_ENABLE = no | |
RGB_MATRIX_SUPPORTED = no |
This actually disables the feature, even when enabling the feature from command line. It prevents it from being used at all. Which is more appropriate for what this is trying to do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@drashna, I don't follow it. Is it about to introduce introduce different detective (RGB_MATRIX_SUPPORTED) that will control what RGB should be used (RGB_MATRIX or RGB_BACKLIGHT)?
I am thinking to implement something like:
ifeq ($(filter $(strip $(RGBLED_OPTION_TYPE))x, nonex backlightx matrixx x),)
$(error unknown RGBLED_OPTION_TYPE value "$(RGBLED_OPTION_TYPE)")
endif
ifeq ($(strip $(RGBLED_OPTION_TYPE)),backlight)
RGB_MATRIX_ENABLE = no
RGB_BACKLIGHT = yes
.......
endif
ifeq ($(strip $(RGBLED_OPTION_TYPE)),matrix)
RGB_BACKLIGHT = no
RGB_MATRIX_ENABLE = yes
RGB_MATRIX_DRIVER = IS31FL3733
..........
endif
So, both RGB options can be supported and configured at keymap level by setting the RGBLED_OPTION_TYPE.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RGB_MATRIX_SUPPORTED = no
This actually won't work, because disable_features.mk
is included before any of the post_rules.mk
files.
|
||
Default layer is normal ANSI TKL | ||
|
||
This keymap uses [RGB Matrix Lighting](https://docs.qmk.fm/#/feature_rgb_matrix) | ||
by default. VIA is not supporting configuration of RGB Matrix Lighting effects but there are still options to do the effects configurations: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This statement is obsolete — VIA V3 (#18222) has support for RGB Matrix configuration (the builtin support is limited to setting mode/HSV/speed though, and you may need to customize the list of effects in the VIA JSON if you are not enabling all existing effects in the firmware for some reason).
Looks like it's too complicated to have two different variants for RGB lighting (RGB Matrix for the default keymap and RGB Backlight for via), code become too messy and complex. So, I think it will be better to unify everything and use RGB Matrix everywhere, which was implemented with commit f4c561c. I've tested VIA using Design Tab and novelkeys_nk87.json file. PR description was updated as well. |
Thank you for your contribution! |
Thank you for your contribution! |
Description
This PR adds RGB Matrix Lighting effects for Novelkeys NK87 keyboard.
Summary:
RGB_MATRIX_ENABLE
is enabled by default and used by thedefault
andvia
keymapRGB_MATRIX_ENABLE = no
at therules.mk
filedefault
keymap has new key codes at FN layer to change RGB Matrix Lightning settingsvia
keymap to make sure that everything is working VIA configurator including lightning effectsTagging @yiancar as the original creator of the NK87 keyboard.
Types of Changes
Issues Fixed or Closed by This PR
Checklist