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 missing rgb matrix default parameters #22281

Merged
merged 3 commits into from
Jan 30, 2024

Conversation

FabienFellay
Copy link
Contributor

@FabienFellay FabienFellay commented Oct 15, 2023

This PR proposes to add the missing configurable rgb matrix default parameters in quantum/rgb_matrix/rgb_matrix.c.

Description

quantum/rgb_matrix/rgb_matrix.c offers the possibility of configuring the following rgb matrix parameters in a config.h file:

  • RGB_MATRIX_DEFAULT_MODE
  • RGB_MATRIX_DEFAULT_HUE
  • RGB_MATRIX_DEFAULT_SAT
  • RGB_MATRIX_DEFAULT_VAL
  • RGB_MATRIX_DEFAULT_SPD

However, there is no way of configuring the default rgb_matrix_config flags nor the default rgb_matrix_config enable-disable status. Therefore, the following config.h options have been added:

  • RGB_MATRIX_DEFAULT_STATUS
  • RGB_MATRIX_DEFAULT_FLAG

For example, this will allow to directly configure the following default rgb matrix option (after an EEPROM reset) in a config.h file:

/* Select the default RGB matrix flag (disable RGB effect by default) */
#define RGB_MATRIX_DEFAULT_FLAG LED_FLAG_NONE

Currently, I have to rely on the use of keyboard_post_init_user() in a keymap.c file:

void keyboard_post_init_user(void) {
    // Use non noeeprom versions to write to EEPROM as well
    rgb_matrix_set_flags(LED_FLAG_NONE);  // Select the default RGB matrix flag (disable RGB effect by default)
}

However, this applies at each boot (and not only at each EEPROM reset, as wished). Additionally, it unnecessarily causes multiple write to the EEPROM. This could be avoided by directly specifying the desired (not supported yet) default options in a config.h. Then, eeconfig_update_rgb_matrix_default() will directly take care of them.

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

I am not aware of an issue opened for that.

Checklist

  • My code follows the code style of this project: C, Python
  • I have read the PR Checklist document and have made the appropriate changes.
  • 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, but not sure of all the consequences of the changes, though).

@lesshonor
Copy link
Contributor

lesshonor commented Oct 15, 2023

However, this applies at each boot (and not only at each EEPROM reset, as wished).

Consider void eeconfig_init_user(void).

Additionally, it unnecessarily causes multiple write to the EEPROM.

The desire to have this happen on EEPROM reset and the desire to avoid EEPROM writes seems to be at odds, but avoiding writes is why the _noeeprom-suffixed version exists:
https://github.com/qmk/qmk_firmware/blob/bd815f0fdb12bb56a3999c5e9a14352910f32484/quantum/rgb_matrix/rgb_matrix.c#L758-L760

All that aside:

  1. Core changes must target develop.
    2. QMK is moving toward data-driven configuration. In accordance with that page, it would be a good idea to add the requisite entries for these settings to the JSON schema files as part of your proposal. Edit: This part isnt yet covered - 21825

@zvecr zvecr changed the base branch from master to develop October 16, 2023 00:05
@FabienFellay
Copy link
Contributor Author

@lesshonor Thank you for your suggestion. Using void eeconfig_init_user(void) was actually my first attempt, but when I do the following in my keymap.c:

void eeconfig_init_user(void) {
    // Use non noeeprom versions to write to EEPROM as well
    rgb_matrix_set_flags(LED_FLAG_NONE);  // Select the default RGB matrix flag (disable RGB effect by default)
}

It just does not work: all the default parameters set by quantum/rgb_matrix/rgb_matrix.c:

void eeconfig_update_rgb_matrix_default(void) {
    dprintf("eeconfig_update_rgb_matrix_default\n");
    rgb_matrix_config.enable = 1;
    rgb_matrix_config.mode   = RGB_MATRIX_DEFAULT_MODE;
    rgb_matrix_config.hsv    = (HSV){RGB_MATRIX_DEFAULT_HUE, RGB_MATRIX_DEFAULT_SAT, RGB_MATRIX_DEFAULT_VAL};
    rgb_matrix_config.speed  = RGB_MATRIX_DEFAULT_SPD;
    rgb_matrix_config.flags  = LED_FLAG_ALL;
    eeconfig_flush_rgb_matrix(true);
}

seems to override what I do in void eeconfig_init_user(void) (and thus the rgb_matrix part of the example of https://docs.qmk.fm/#/feature_eeprom?id=eeconfig-function-documentation is not working for me). This is the only thing that motivated this PR, actually.

However, if you have ideas of why void eeconfig_init_user(void) is not working when setting rgb_matrix default parameters, I will agree this PR is not needed.

Finally, what I meant by avoiding multiple write to the EEPROM is that, by using the proposed new preprocessor #define directive, the rgb matrix defaults are only written once to the EEPROM by quantum/rgb_matrix/rgb_matrix.c. The example of https://docs.qmk.fm/#/feature_eeprom?id=eeconfig-function-documentation, if it was working, seems to cause multiple write. The goal was just to minimize the number of write to the EEPROM (but honestly, I admit that is too much care and not necessary).

Can you confirm that

void eeconfig_init_user(void) {
    // Use non noeeprom versions to write to EEPROM as well
    rgb_matrix_set_flags(LED_FLAG_NONE);  // Select the default RGB matrix flag (disable RGB effect by default)
}

has no effect? Or am I missing something? It does have an effect in void keyboard_post_init_user(void) but I want to retain my setting on reboot. I only want to set custom defaults on EEPROM reset.

(sorry for the master targeting instead of develop)

@FabienFellay
Copy link
Contributor Author

FabienFellay commented Oct 16, 2023

OK: on the develop branch, it seems they somehow introduced something similar to the missing default parameters I proposed in this PR. So, the latter may definitely not be needed anymore. However, you could maybe still help me to figure out what is wrong with void eeconfig_init_user(void)?

@FabienFellay
Copy link
Contributor Author

Note that the example regarding void eeconfig_init_user(void) from https://docs.qmk.fm/#/feature_eeprom?id=eeconfig-function-documentation showcases rgblight functions (underglow) but in my case, I am dealing with a whole rgb matrix. It could be that void eeconfig_init_user(void) is working with rgblight but not with rgb matrix. However, I have no way to test with rgb light and thus, I cannot confirm this.

@lesshonor
Copy link
Contributor

lesshonor commented Oct 18, 2023

However, you could maybe still help me to figure out what is wrong with void eeconfig_init_user(void)?

Truthfully, I'm not sure you actually need this function if all you want to do is make sure the lights are off when you reset the EEPROM. Have you tried setting #define RGB_MATRIX_DEFAULT_VAL 0? I'm not sure exactly what the goal of modifying the default flags is.

I wasn't aware of (...or perhaps had just forgotten about) zvecr's PR when I left that comment, so you probably want to review that. My point was that new configuration settings require additions to QMK's data-driven JSON schema—they cannot exist in C alone.

@FabienFellay
Copy link
Contributor Author

I am customizing a keychron q6 keyboard based on what Keychron released here: qmk_firmware/keyboards/keychron/q6/iso_encoder/keymaps/keychron/. When host indicator #define are present (numlock or caps locks), which is my case, they somehow use the flag integer instead of rgb_matrix_enable(), rgb_matrix_disable() and rgb_matrix_toggle() in order to turn RGB effect on and off while preserving the indicator state.

You can look at what they did in keyboards/keychron/q6/q6.c. I want to stick with this logic (which works well) and the ability to set the flag state after an EEPROM reset would do exactly what I expect (want). This is a typical use case where modifying the default flags would be useful.

@FabienFellay FabienFellay force-pushed the add_rgb_matrix_default branch from bd815f0 to df3c412 Compare November 23, 2023 23:41
@FabienFellay
Copy link
Contributor Author

I resolved the conflicts that occurred by changing the target branch. In short:

  • Now, the only new proposed default parameter that can be specified in a config.h is RGB_MATRIX_DEFAULT_FLAG because the develop branch already implemented RGB_MATRIX_DEFAULT_ON (that I initially proposed as well under the less clear name RGB_MATRIX_DEFAULT_STATUS).
  • The documentation docs/feature_rgb_matrix.md has been updated accordingly (and the order of options has been fixed to be consistent with the order in the code in quantum/rgb_matrix/rgb_matrix.c and quantum/rgb_matrix/rgb_matrix.h).

@drashna drashna requested a review from a team December 6, 2023 04:06
@FabienFellay FabienFellay force-pushed the add_rgb_matrix_default branch from df3c412 to 5e88a74 Compare December 13, 2023 00:51
@FabienFellay
Copy link
Contributor Author

I just tested this on a quite recent state of the develop branch and I can confirm that it is working as expected and that it does not break anything.

It would allow one to now put the following in a config.h file:

#pragma once

#define RGB_MATRIX_DEFAULT_ON true // Sets the default enabled state, if none has been set
#define RGB_MATRIX_DEFAULT_FLAG LED_FLAG_NONE // Sets the default LED flags, if none has been set

which is useful to initially (after a EEPROM reset only) power off the rgb effect while preserving the host indicators LEDs (caps lock, num lock) when using a Keychron q6 keyboard (see keyboards/keychron/q6/q6.c).

Copy link

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 bug, awaiting review, breaking_change, in progress, or on hold to prevent the issue from being re-flagged.

@github-actions github-actions bot added the stale Issues or pull requests that have become inactive without resolution. label Jan 27, 2024
@FabienFellay
Copy link
Contributor Author

Hello guys, the github-actions bot wants to close this PR as there has been no feedback for 45 days, apparently. As a reminder, this PR is ready, and its status should be awaiting review.

@github-actions github-actions bot removed the stale Issues or pull requests that have become inactive without resolution. label Jan 28, 2024
docs/feature_rgb_matrix.md Outdated Show resolved Hide resolved
quantum/rgb_matrix/rgb_matrix.c Outdated Show resolved Hide resolved
quantum/rgb_matrix/rgb_matrix.h Outdated Show resolved Hide resolved
@FabienFellay
Copy link
Contributor Author

Thank you for your review, the suggestions have been applied.

@zvecr zvecr requested a review from fauxpark January 28, 2024 21:06
@fauxpark fauxpark merged commit 734c7af into qmk:develop Jan 30, 2024
3 of 4 checks passed
@FabienFellay FabienFellay deleted the add_rgb_matrix_default branch March 4, 2024 21:54
nuess0r pushed a commit to nuess0r/qmk_firmware that referenced this pull request Sep 8, 2024
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.

4 participants