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

Small round of rgb matrix optimizations #5170

Closed
wants to merge 3 commits into from

Conversation

XScorpion2
Copy link
Contributor

@XScorpion2 XScorpion2 commented Feb 18, 2019

Description

Small around of changes to the rgb matrix animation code hoisting a few more chunks of calculations out of the loops.

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

@mechmerlin
Copy link
Contributor

I wish we had some unit tests to test these.

@drashna
Copy link
Member

drashna commented Feb 18, 2019

@mechmerlin I mean, there is Drashna CI, just need to kick it. :D

@XScorpion2
Copy link
Contributor Author

XScorpion2 commented Feb 18, 2019

I wanted to keep this pr small (unlike my other one) for this round. I'm thinking about the changes to make this giant file more modular / testable especially within the context of the animation functions themselves.

Current thinking is move each animation function into a separate code file, and update the function to take pointers to the necessary global values instead of accessing them via extern field. This way the function can be run directly with a set of test values and the results can then be validated without needing anything but just the animation code file itself.

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.

Looks good to me, Works on both my planck light, and ergodox ez glow, without any issues

@XScorpion2 XScorpion2 mentioned this pull request Feb 23, 2019
13 tasks
Copy link
Contributor

@yanfali yanfali left a comment

Choose a reason for hiding this comment

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

Hoisting work out of loop bodies seems worthwhile. +1

@XScorpion2
Copy link
Contributor Author

Latest version: #5304
Works with the test suite being developed for rgb matrix animations.

@XScorpion2 XScorpion2 closed this Mar 14, 2019
@XScorpion2 XScorpion2 deleted the matrix_anim_perf branch March 23, 2019 22:01
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.

5 participants