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

Allow disabling RGB_MATRIX_ANIMATIONS on system76 keyboards with a pre-procesor directive #17478

Merged

Conversation

PeterFalken
Copy link
Contributor

@PeterFalken PeterFalken commented Jun 25, 2022

Description

Allow disabling RGB_MATRIX_ANIMATIONS on system76 keyboards by using a pre-procesor directive.
Not defining the animations can free up storage space (~2.2KB or more on atmega32u4) to use on other features. When DISABLE_RGB_MATRIX_ANIMATIONS is not defined, the default behavior is unchanged - It's intended to be minimally invasive and straight forward.

This change can be activated on a custom keymap with the following code:

#define DISABLE_RGB_MATRIX_ANIMATIONS
#undef ENABLE_RGB_MATRIX_CYCLE_ALL
#undef ENABLE_RGB_MATRIX_CYCLE_LEFT_RIGHT
#undef ENABLE_RGB_MATRIX_CYCLE_UP_DOWN
...

An example of how it can be used on a keymap file can be found here.

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

@drashna drashna requested a review from a team June 25, 2022 19:15
@PeterFalken PeterFalken changed the title Allow disabling RGB_MATRIX_ANIMATIONS system76 keyboards with pre-procesor directive Allow disabling RGB_MATRIX_ANIMATIONS on system76 keyboards with a pre-procesor directive Jun 28, 2022
Copy link
Contributor

@leviport leviport left a comment

Choose a reason for hiding this comment

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

(not sure if I should leave reviews on upstream QMK, but I wanted to share my findings)

I ran a few tests, building launch_1 firmware with the config.h file you linked, both with #define DISABLE_RGB_MATRIX_ANIMATIONS and the 11 #undef's below it, and with all of those lines commented out.

With animations disabled, the built firmware was 20174/28672 bytes. With animations enabled, the size was 22454/28672, meaning that disabling animations frees up 2280 bytes. My launch_1 is still working fine with animations disabled, and so does the non-RGB animation functionality of system76-keyboard-configurator. The configurator also still allows me to set solid colors per-layer, just none of the included animations. I assume users that build their own firmware and intentionally disable animations would not be confused by that feature in the configurator not working afterwards. So, I think this change looks good!

@PeterFalken
Copy link
Contributor Author

PeterFalken commented Jul 3, 2022

Thank you @leviport, for the feedback and testing - I really appreciate it.
Updated the comment for the PR to reference 2.2KB, the extra space I noticed is probably associated with other features I had disabled on my configurations.

@drashna drashna requested a review from a team July 4, 2022 06:27
@spidey3 spidey3 merged commit 4c91e5b into qmk:master Aug 14, 2022
@PeterFalken PeterFalken deleted the system76_allow_disabling_RGB_ANIMATIONS branch August 15, 2022 14:40
imhoffman pushed a commit to imhoffman/qmk_firmware that referenced this pull request Aug 20, 2022
SjB added a commit to SjB/qmk_firmware that referenced this pull request Aug 27, 2022
* upstream/master: (595 commits)
  [Keyboard] Add ibis80 (qmk#18051)
  [Keymap] toshi0383 keymaps update (qmk#18073)
  [Docs] Use layer number as example for COMBO_ONLY_FROM_LAYER (qmk#18072)
  [Keymap] jotix's community/ortho4x12 change (qmk#18069)
  Adjust the hierarchy of chapter `Deferred Execution` (qmk#18075)
  Fix WS2812 order for aurora65 and loki65 (qmk#18074)
  [Docs] update pr_checklist.md with info about data-driven (qmk#18068)
  Use correct board file in xelus/valor_frl_tkl/rev2_0 (qmk#18071)
  Migrate more F4x1 board files (qmk#18054)
  ADB to USB converter: split into rev1 and rev2 (qmk#18052)
  Move keyboard USB IDs and strings to data driven, pass 2: D-E (qmk#17956)
  Move keyboard USB IDs and strings to data driven, pass 2: F-I (qmk#17958)
  Allow disabling RGB_MATRIX_ANIMATIONS system76 keyboards (qmk#17478)
  Migrate more F4x1 board files (qmk#18046)
  Feat/port ft mars 65 (qmk#17994)
  Improve Drop Alt compatibility with VIA (qmk#18041)
  [Keyboard] Add Valor FRL TKL rev2.0 and 2.1 (qmk#17992)
  Move keyboard USB IDs and strings to data driven, pass 2: B-C (qmk#17945)
  [Keyboard] add bajjak keyboard (qmk#12377)
  Add texts for Discord Events to be created post-merge. (qmk#17944)
  ...
nolanseaton pushed a commit to nolanseaton/qmk_firmware that referenced this pull request Jan 23, 2023
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.

4 participants