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

Converted RGB matrix to use last_input_activity_elapsed(). #21687

Merged
merged 4 commits into from
Nov 29, 2023

Conversation

pneisen
Copy link
Contributor

@pneisen pneisen commented Aug 3, 2023

Description

After setting RGB_MATRIX_TIMEOUT to a value on my macro pad (winry315), the timeout worked as expected, but I was frustrated that turning my encoder knobs would not turn the LEDs back on and would not prevent them from turning off.

This solution was suggested by @drashna . It removes the special timer used by RGB matrix in favor of using the last_input_activity timer. Since this timer is reset by encoder activity, it fixes the issue I was seeing and probably fixes other inputs that didn't reset the old timer.

I did quite a bit of testing on my one device (winry315) that uses RGB matrix and has encoders. RGB_MATRIX_TIMEOUT worked as advertised and all inputs I could provide with this device reset the timer and would wake the RGB matrix.

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 think the encoder activity not waking the rgb matrix was technically a bug with the RGB_MATRIX_TIMEOUT feature, but I was unable to find an issue on it.

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).

After setting RGB_MATRIX_TIMEOUT to a value on my macro pad (winry315), the timeout
worked as expected, but I was frustrated that turning my encoder knobs
would not turn the LEDs back on and would not prevent them from turning
off.

This solution was suggested by @drashna. It removes the special
timer used by RGB matrix in favor of using the last_input_activity
timer. Since this timer is reset by encoder activity, it fixes the issue
I was seeing.
@github-actions github-actions bot added the core label Aug 3, 2023
@drashna
Copy link
Member

drashna commented Aug 3, 2023

These changes should be applied to led matrix, as well (since the features are very similar, to keep parity between the two).

Also, looking closer at the code... This is all kind of a mess. Especially for splits.
Specifically, the rgb timer is called on both sides, independently. Keypresses are processed on both sides, and that is what triggers the activity.

So, for the activity timer, this isn't synced by default and would need to be enabled for any split keyboard, to properly handle the timeout.

A simple solution would be to add to quantum/split_common/post_config.h, but I'm not a fan of this. But it may be best bad option here. eg:

#ifdef RGB_MATRIX_ENABLE
#    if RGB_MATRIX_TIMEOUT > 0
#        define SPLIT_ACTIVITY_ENABLE
#    endif
#endif

The main issue here is that anyone with split rgb matrix support would need to flash both sides, after this code change. Which isn't horrible, just isn't great.

The other option is to use the local timer, but add/enable syncing of the timer, so that both sides of the split are in sync (but I think that this is "less good"/"more bad" than forcing activity sync.

I made a identical change to RGB matrix and it was pointed out that this
feature is nearly identical and should have the changes as well.
@pneisen
Copy link
Contributor Author

pneisen commented Aug 3, 2023

I made the changes on the LED matrix side, but you'll have to let me know how you want to resolve the issues with splits as I am not familiar with how splits are handled.

@drashna drashna requested a review from a team August 4, 2023 04:54
@drashna
Copy link
Member

drashna commented Aug 10, 2023

I made the changes on the LED matrix side, but you'll have to let me know how you want to resolve the issues with splits as I am not familiar with how splits are handled.

Given #21703, I'd say that updating the post_config.h file for rgb/led matrix would be a good idea.

@pneisen
Copy link
Contributor Author

pneisen commented Aug 12, 2023

Given #21703, I'd say that updating the post_config.h file for rgb/led matrix would be a good idea.

I read over that pull request, but I am not sure what you mean by updating the post_configs. Are you saying that we probably need to define RGB_MATRIX_KEYPRESSES if RGB_MATRIX_TIMEOUT > 0? I'm not quite sure why we would need to do that since RGB_MATRIX_KEYPRESSES looks to be a different thing from activity.

I'm probably just missing what you are hinting at.

@zvecr
Copy link
Member

zvecr commented Aug 15, 2023

Given #21703, I'd say that updating the post_config.h file for rgb/led matrix would be a good idea.

That is solving a completely different sort of issue. The fact that what is mostly an optimisation, has bled to over 800 references in keyboards. As this is coupling between multiple components, it doesnt infer the same solution.

For whats best in this scenario, the suggested might be best but its not something ive really looked into.

Copy link
Contributor

@lesshonor lesshonor left a comment

Choose a reason for hiding this comment

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

If you have RGB_MATRIX enabled but not RGB_MATRIX_KEYREACTIVE, deltaTime goes unused and compilation fails.

This probably merits a harder look, but:

diff --git a/quantum/rgb_matrix/rgb_matrix.c b/quantum/rgb_matrix/rgb_matrix.c
index 5d4dbd5918..293794dbae 100644
--- a/quantum/rgb_matrix/rgb_matrix.c
+++ b/quantum/rgb_matrix/rgb_matrix.c
@@ -290,13 +290,11 @@ static bool rgb_matrix_none(effect_params_t *params) {
 }
 
 static void rgb_task_timers(void) {
-#if defined(RGB_MATRIX_KEYREACTIVE_ENABLED) || RGB_MATRIX_TIMEOUT > 0
-    uint32_t deltaTime = sync_timer_elapsed32(rgb_timer_buffer);
-#endif // defined(RGB_MATRIX_KEYREACTIVE_ENABLED) || RGB_MATRIX_TIMEOUT > 0
     rgb_timer_buffer = sync_timer_read32();
 
     // Update double buffer last hit timers
 #ifdef RGB_MATRIX_KEYREACTIVE_ENABLED
+    uint32_t deltaTime = sync_timer_elapsed32(rgb_timer_buffer);
     uint8_t count = last_hit_buffer.count;
     for (uint8_t i = 0; i < count; ++i) {
         if (UINT16_MAX - deltaTime < last_hit_buffer.tick[i]) {

@pneisen
Copy link
Contributor Author

pneisen commented Aug 30, 2023

If you have RGB_MATRIX enabled but not RGB_MATRIX_KEYREACTIVE, deltaTime goes unused and compilation fails.

Good catch! I fixed this issue in both rgb_matrix and led_matrix. I also looked around for similar issues I might have caused and didn't find any.

@tzarc tzarc merged commit 7cc90c2 into qmk:develop Nov 29, 2023
4 checks passed
itsjonny96 pushed a commit to itsjonny96/qmk_firmware that referenced this pull request Jan 7, 2024
mechlovin pushed a commit to mechlovin/qmk_firmware that referenced this pull request Jan 17, 2024
mechlovin pushed a commit to mechlovin/qmk_firmware that referenced this pull request Jan 19, 2024
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
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants