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

Fix RGB heatmap to use XY positions and use correct led limits. #17184

Merged
merged 7 commits into from
Jun 21, 2022

Conversation

daskygit
Copy link
Member

@daskygit daskygit commented May 22, 2022

Description

Tested on a split keyboard and board with a duplex matrix and it behaves much better, no more trigger incorrect locations. The effect is slightly different visually compared with the old one. I've not tested on AVR, hopefully not too much impact.

There's a define RGB_MATRIX_TYPING_HEATMAP_SPREAD that can be changed to increase the area of effect.

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

@github-actions github-actions bot added the core label May 22, 2022
@daskygit daskygit marked this pull request as draft May 22, 2022 01:58
@filterpaper
Copy link
Contributor

Tested this on boardsource/technik_o and crkbd/rev1 running AVR controllers. There were no impact on typing performance and heatmap spread displayed correctly on the split matrix without any "bleeding" into incorrect rows or columns.

Copy link
Member

@drashna drashna left a comment

Choose a reason for hiding this comment

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

Tested and works better.

@drashna drashna requested a review from a team May 23, 2022 05:07
@daskygit daskygit marked this pull request as ready for review May 24, 2022 11:39
@daskygit daskygit requested a review from drashna May 24, 2022 21:44
@daskygit
Copy link
Member Author

Did a couple of small updates, noticed the effect was triggering on both key down and key up.

@drashna drashna requested a review from a team June 20, 2022 20:02
@daskygit
Copy link
Member Author

Thanks, I'll update the docs with the added configuration options before it's merged.

@filterpaper
Copy link
Contributor

Fixes #14041

@drashna drashna merged commit be42c5f into qmk:develop Jun 21, 2022
@daskygit daskygit deleted the fix/rgb_heatmap branch June 21, 2022 18:05
@RagingCactus
Copy link
Contributor

RagingCactus commented Jun 27, 2022

@daskygit @drashna I'm trying to experiment with this in my firmware. I hope you don't mind me asking as to me it seems related to this change and looks like a potential issue to me: How does this treat keys with no LED ? Wouldn't NO_LED entries in matrix_co mess up the dereferencing in led_point_t? I'm by no means versed in QMK, but it looks to me like NO_LED keys would trigger access to g_led_config.point[255], which would be nonsense.

To me it looks like we would need to do one of two things:

  1. Be be able to define LED indexes that are not really LEDs (and get skipped for the driver code) andthus get proper distance calculation even for keys without LEDs (would be cool, but needs changes for all effects and in the core LED matrix system)
  2. Skip keys that are defined as NO_LED or have an invalid index altogether for this effect.

@daskygit
Copy link
Member Author

Thanks for spotting this, I think option 2 is likely going to be the way forward.

It could revert to the old behaviour if there's no led at the position but I'm now wondering if this should just be a separate effect entirely.

@daskygit daskygit mentioned this pull request Jun 27, 2022
14 tasks
@daskygit
Copy link
Member Author

@RagingCactus I've opened PR #17488 to address this, thanks again.

0xcharly pushed a commit to Bastardkb/bastardkb-qmk that referenced this pull request Jul 4, 2022
…7184)

* Fix RGB heatmap to use XY positions

* lower effect area limit and make configurable

* tidy up macro

* Fix triggering in both directions.

* add docs

* fix bug when decreasing value

* performance tweak
nolanseaton pushed a commit to nolanseaton/qmk_firmware that referenced this pull request Jan 23, 2023
…7184)

* Fix RGB heatmap to use XY positions

* lower effect area limit and make configurable

* tidy up macro

* Fix triggering in both directions.

* add docs

* fix bug when decreasing value

* performance tweak
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