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

Reduce PROGMEM usage for sendstring LUT #8109

Merged
merged 6 commits into from
Mar 2, 2020

Conversation

kitlaan
Copy link
Contributor

@kitlaan kitlaan commented Feb 6, 2020

Description

Bit-pack the keycode bool array to gain back a small amount of flash space.
The trade-off is an increase in runtime instructions when running macros.

It does make the code a bit harder to read, as well as maintain.

For configs that use send_string() et al, it saves ~100 bytes.

I'm really unsure if this is even worth it given the annoyance...
I did NOT test this, other than
compiling a few random user keyboard configs to see for size differences. For example, kyria:drashna moved from a failed build to just back under the line.

Also note: When I run clang-format on quantum.c, it decides to do some weird reformatting. So I didn't run it here.

Types of Changes

  • Core
  • Bugfix
  • New feature
  • Enhancement/optimization
  • Keyboard (addition or update)
  • Keymap/layout/userspace (addition or update)
  • Documentation

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

Bit-pack the keycode bool array to gain back a small amount of flash space.
The trade-off is an increase in runtime instructions when running macros.

It does make the code a bit harder to read, as well as maintain.

For configs that use send_string() et al, it saves ~100 bytes.
@tzarc
Copy link
Member

tzarc commented Feb 21, 2020

Could we perhaps leverage something like the following, to simplify the conversion?

#define LUT_BYTE(a, b, c, d, e, f, g, h) \
  ( ((a) ? 1 : 0) << 7 \
  | ((b) ? 1 : 0) << 6 \
  | ((c) ? 1 : 0) << 5 \
  | ((d) ? 1 : 0) << 4 \
  | ((e) ? 1 : 0) << 3 \
  | ((f) ? 1 : 0) << 2 \
  | ((g) ? 1 : 0) << 1 \
  | ((h) ? 1 : 0) << 0 )

We'd then be able to do this, which is a lot closer to what we had before:

const uint8_t ascii_to_shift_lut[16] PROGMEM = {
    LUT_BYTE(0, 0, 0, 0, 0, 0, 0, 0),
    LUT_BYTE(0, 0, 0, 0, 0, 0, 0, 0),
    LUT_BYTE(0, 0, 0, 0, 0, 0, 0, 0),
    LUT_BYTE(0, 0, 0, 0, 0, 0, 0, 0),
    LUT_BYTE(0, 0, 0, 0, 0, 1, 0, 0),
    LUT_BYTE(0, 0, 1, 1, 0, 0, 1, 1),
    LUT_BYTE(1, 1, 1, 1, 1, 1, 1, 1),
    LUT_BYTE(1, 1, 0, 0, 0, 0, 1, 1),
    LUT_BYTE(0, 1, 1, 1, 1, 1, 1, 1),
    LUT_BYTE(1, 1, 1, 1, 1, 1, 1, 1),
    LUT_BYTE(1, 1, 1, 1, 1, 1, 1, 1),
    LUT_BYTE(1, 1, 1, 0, 0, 0, 0, 1),
    LUT_BYTE(0, 0, 0, 0, 0, 0, 0, 0),
    LUT_BYTE(0, 0, 0, 0, 0, 0, 0, 0),
    LUT_BYTE(0, 0, 0, 0, 0, 0, 0, 0),
    LUT_BYTE(0, 0, 0, 0, 0, 0, 0, 0)
};

@kitlaan
Copy link
Contributor Author

kitlaan commented Feb 21, 2020

I had considered it but didn't really think it through. Actually seeing it typed out, I should have tried it!

I'll update the branch and see if I can clean it up even further.

@kitlaan kitlaan changed the title Reduce PROGMEM usage for keycode map Reduce PROGMEM usage for sendstring LUT Feb 21, 2020
@kitlaan
Copy link
Contributor Author

kitlaan commented Feb 21, 2020

Did some changes, and things are looking promising. Much cleaner and "easier" to maintain. I'll finish it up shortly and push, and see if folks like it better.

Now that I have my board running properly, I can finally test and see if there are any real performance differences. Even if there aren't, keeping both flows "for a while" still seems wise.

Reminder: resync master and make the same edits to the new sendstring LUTs.

Rewrite the array declarations so both the unpacked (original) and
packed LUT arrays can use the same value definitions. This is done by
defining a macro that "knows what to do".

This makes the code much easier to read and maintain.
quantum/quantum.c Outdated Show resolved Hide resolved
quantum/quantum.c Outdated Show resolved Hide resolved
@kitlaan
Copy link
Contributor Author

kitlaan commented Feb 21, 2020

A question I was pondering is if there are any folks that have their own LUT definitions. If there are, then changing to packed-always would hopefully compile error (type redefinition?) if we kept the old variable name. Or they'd silently go back to the default LUT without warning if we changed variable names.

Is that reasonable to do? Or is it a breaking change? Or an I just being over-cautious?

Pack the bits in a more efficient order for extraction.
And also fix the copy/paste error in the macro...
@tzarc
Copy link
Member

tzarc commented Feb 22, 2020

Discussed with the others, the general feeling is that we'll go with the bitmasks and drop the full-size arrays. Makes no sense to have it configurable.

If we can get the other languages sorted as well, I think we'll be much closer to merge!

@tzarc tzarc requested a review from a team February 22, 2020 03:06
Some minor reformatting.
Compile tested all sendstring_xyz.h to make sure they were converted
properly. Also checked that an unconverted version would generate a
compile error.
@kitlaan
Copy link
Contributor Author

kitlaan commented Feb 22, 2020

All updated, and gave all the headers a quick compile-smoketest.

I believe there' still the Hungarian pull request pending, so once that's merged, I'll update my branch. And hopefully that's it.

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.

I'm really unsure if this is even worth it given the annoyance...
I did NOT test this, other than compiling a few random user keyboard configs to see for size differences. For example, kyria:drashna moved from a failed build to just back under the line.

lol. Yeah, My keymaps tend to run very close to the line, and depending on which compiler version you're using, may actually go over it. :D

For the most part, as long as the default keymap compiles, and nothing is broken, that's fine. And in the case of my keymaps, as long as it compiles, even if it's over, that's fine.

And in this case, testing locally, looks good! No issues from what I am seeing (namely, my make and version commands)

Copy link
Member

@fauxpark fauxpark left a comment

Choose a reason for hiding this comment

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

A spot of prettifying

quantum/quantum.c Outdated Show resolved Hide resolved
quantum/quantum.c Outdated Show resolved Hide resolved
quantum/quantum.c Outdated Show resolved Hide resolved
quantum/quantum.h Outdated Show resolved Hide resolved
quantum/quantum.c Outdated Show resolved Hide resolved
@tzarc
Copy link
Member

tzarc commented Mar 2, 2020

Hungarian can get sorted out later. Merging.

@tzarc tzarc merged commit 552f8d8 into qmk:master Mar 2, 2020
@kitlaan kitlaan deleted the flashsize_packed_keycode_map branch March 3, 2020 00:18
Shinichi-Ohki added a commit to Shinichi-Ohki/qmk_firmware that referenced this pull request Mar 3, 2020
* 'master' of https://github.com/qmk/qmk_firmware: (37 commits)
  Update Hungarian keymap and add sendstring LUT (qmk#8220)
  Remove "ugly hack in usb_main.c" comments (qmk#8296)
  Update encoder functions for Iris VIA keymap (qmk#8295)
  Reduce PROGMEM usage for sendstring LUT (qmk#8109)
  [Docs] Update ISP Flashing guide (qmk#8149)
  Rewrite the Bathroom Epiphanies Frosty Flake matrix and LED handling (qmk#8243)
  Add onekey keymap for testing reset to bootloader. (qmk#8288)
  Get the direction right on the S75 encoder (qmk#8287)
  Prune out pure software pwm && custom driver && remove wrapping BACKLIGHT_PIN (qmk#8041)
  Make a fix to savage65 and tmov2 for via (qmk#8286)
  format code according to conventions [skip ci]
  Short term fix for conflicting types for 'tfp_printf' (qmk#8157)
  Fix recent clang-format breaking quantum.c (qmk#8282)
  format code according to conventions [skip ci]
  Remove duplicate BRTG case (qmk#8277)
  Clean up includes for glcdfont headers (qmk#7745)
  Fix the Breaking Changes doc again
  [Docs] translated 'feature_tap_dance.md' to japanese. (qmk#8137)
  PWM DMA based RGB Underglow for STM32 (qmk#7928)
  Add VIA support to Prime_M. Clean up all files (qmk#8247)
  ...
HokieGeek pushed a commit to HokieGeek/qmk_firmware that referenced this pull request Mar 5, 2020
* Reduce PROGMEM usage for keycode map

Bit-pack the keycode bool array to gain back a small amount of flash space.
The trade-off is an increase in runtime instructions when running macros.

It does make the code a bit harder to read, as well as maintain.

For configs that use send_string() et al, it saves ~100 bytes.

* Switch to macro and common definition

Rewrite the array declarations so both the unpacked (original) and
packed LUT arrays can use the same value definitions. This is done by
defining a macro that "knows what to do".

This makes the code much easier to read and maintain.

* Fix macro typos and improve perf

Pack the bits in a more efficient order for extraction.
And also fix the copy/paste error in the macro...

* Switch fully to packed LUT

Some minor reformatting.
Compile tested all sendstring_xyz.h to make sure they were converted
properly. Also checked that an unconverted version would generate a
compile error.

* Apply whitespace suggestions from code review

Co-Authored-By: Ryan <[email protected]>

Co-authored-by: Ryan <[email protected]>
nesth pushed a commit to nesth/qmk_firmware that referenced this pull request Mar 5, 2020
* upstream/master: (37 commits)
  Add RGB lighting through one of the free pins (qmk#8294)
  [Keymap] Adding the 4sStylZ xd75 (qmk#8285)
  YD60MQ refactor and Configurator layout support (qmk#8313)
  [Keyboard] Forget to ifdef Super16 led config (qmk#8314)
  [Keyboard] Switch to RGB Matrix for Super16 (qmk#8305)
  [Keymap] Keymap Update (qmk#8309)
  New Keyboard: SiddersKB Majbritt (Pronounced My Brit) (qmk#8260)
  [Keyboard] VIA Support: Tada68 (qmk#8289)
  [Keyboard] LFK78 refactor (qmk#7835)
  [Keymap] new userspace for ibnuda (qmk#8221)
  [Keymap] Add crd's equinox keymap (qmk#8251)
  [Keymap] Feature/alfrdmalr/keymap update (qmk#8174)
  Fix bootloader definition for namecard2x4 (qmk#8301)
  Update Hungarian keymap and add sendstring LUT (qmk#8220)
  Remove "ugly hack in usb_main.c" comments (qmk#8296)
  Update encoder functions for Iris VIA keymap (qmk#8295)
  Reduce PROGMEM usage for sendstring LUT (qmk#8109)
  [Docs] Update ISP Flashing guide (qmk#8149)
  Rewrite the Bathroom Epiphanies Frosty Flake matrix and LED handling (qmk#8243)
  Add onekey keymap for testing reset to bootloader. (qmk#8288)
  ...
drashna pushed a commit to zsa/qmk_firmware that referenced this pull request Mar 19, 2020
* Reduce PROGMEM usage for keycode map

Bit-pack the keycode bool array to gain back a small amount of flash space.
The trade-off is an increase in runtime instructions when running macros.

It does make the code a bit harder to read, as well as maintain.

For configs that use send_string() et al, it saves ~100 bytes.

* Switch to macro and common definition

Rewrite the array declarations so both the unpacked (original) and
packed LUT arrays can use the same value definitions. This is done by
defining a macro that "knows what to do".

This makes the code much easier to read and maintain.

* Fix macro typos and improve perf

Pack the bits in a more efficient order for extraction.
And also fix the copy/paste error in the macro...

* Switch fully to packed LUT

Some minor reformatting.
Compile tested all sendstring_xyz.h to make sure they were converted
properly. Also checked that an unconverted version would generate a
compile error.

* Apply whitespace suggestions from code review

Co-Authored-By: Ryan <[email protected]>

Co-authored-by: Ryan <[email protected]>
c0psrul3 pushed a commit to c0psrul3/qmk_firmware that referenced this pull request Mar 23, 2020
* Reduce PROGMEM usage for keycode map

Bit-pack the keycode bool array to gain back a small amount of flash space.
The trade-off is an increase in runtime instructions when running macros.

It does make the code a bit harder to read, as well as maintain.

For configs that use send_string() et al, it saves ~100 bytes.

* Switch to macro and common definition

Rewrite the array declarations so both the unpacked (original) and
packed LUT arrays can use the same value definitions. This is done by
defining a macro that "knows what to do".

This makes the code much easier to read and maintain.

* Fix macro typos and improve perf

Pack the bits in a more efficient order for extraction.
And also fix the copy/paste error in the macro...

* Switch fully to packed LUT

Some minor reformatting.
Compile tested all sendstring_xyz.h to make sure they were converted
properly. Also checked that an unconverted version would generate a
compile error.

* Apply whitespace suggestions from code review

Co-Authored-By: Ryan <[email protected]>

Co-authored-by: Ryan <[email protected]>
fdidron pushed a commit to zsa/qmk_firmware that referenced this pull request Mar 24, 2020
* Reduce PROGMEM usage for keycode map

Bit-pack the keycode bool array to gain back a small amount of flash space.
The trade-off is an increase in runtime instructions when running macros.

It does make the code a bit harder to read, as well as maintain.

For configs that use send_string() et al, it saves ~100 bytes.

* Switch to macro and common definition

Rewrite the array declarations so both the unpacked (original) and
packed LUT arrays can use the same value definitions. This is done by
defining a macro that "knows what to do".

This makes the code much easier to read and maintain.

* Fix macro typos and improve perf

Pack the bits in a more efficient order for extraction.
And also fix the copy/paste error in the macro...

* Switch fully to packed LUT

Some minor reformatting.
Compile tested all sendstring_xyz.h to make sure they were converted
properly. Also checked that an unconverted version would generate a
compile error.

* Apply whitespace suggestions from code review

Co-Authored-By: Ryan <[email protected]>

Co-authored-by: Ryan <[email protected]>
sowbug pushed a commit to sowbug/qmk_firmware that referenced this pull request Apr 2, 2020
* Reduce PROGMEM usage for keycode map

Bit-pack the keycode bool array to gain back a small amount of flash space.
The trade-off is an increase in runtime instructions when running macros.

It does make the code a bit harder to read, as well as maintain.

For configs that use send_string() et al, it saves ~100 bytes.

* Switch to macro and common definition

Rewrite the array declarations so both the unpacked (original) and
packed LUT arrays can use the same value definitions. This is done by
defining a macro that "knows what to do".

This makes the code much easier to read and maintain.

* Fix macro typos and improve perf

Pack the bits in a more efficient order for extraction.
And also fix the copy/paste error in the macro...

* Switch fully to packed LUT

Some minor reformatting.
Compile tested all sendstring_xyz.h to make sure they were converted
properly. Also checked that an unconverted version would generate a
compile error.

* Apply whitespace suggestions from code review

Co-Authored-By: Ryan <[email protected]>

Co-authored-by: Ryan <[email protected]>
kylekuj pushed a commit to kylekuj/qmk_firmware that referenced this pull request Apr 21, 2020
* Reduce PROGMEM usage for keycode map

Bit-pack the keycode bool array to gain back a small amount of flash space.
The trade-off is an increase in runtime instructions when running macros.

It does make the code a bit harder to read, as well as maintain.

For configs that use send_string() et al, it saves ~100 bytes.

* Switch to macro and common definition

Rewrite the array declarations so both the unpacked (original) and
packed LUT arrays can use the same value definitions. This is done by
defining a macro that "knows what to do".

This makes the code much easier to read and maintain.

* Fix macro typos and improve perf

Pack the bits in a more efficient order for extraction.
And also fix the copy/paste error in the macro...

* Switch fully to packed LUT

Some minor reformatting.
Compile tested all sendstring_xyz.h to make sure they were converted
properly. Also checked that an unconverted version would generate a
compile error.

* Apply whitespace suggestions from code review

Co-Authored-By: Ryan <[email protected]>

Co-authored-by: Ryan <[email protected]>
jakeisnt pushed a commit to jakeisnt/qmk_firmware that referenced this pull request Aug 20, 2020
* Reduce PROGMEM usage for keycode map

Bit-pack the keycode bool array to gain back a small amount of flash space.
The trade-off is an increase in runtime instructions when running macros.

It does make the code a bit harder to read, as well as maintain.

For configs that use send_string() et al, it saves ~100 bytes.

* Switch to macro and common definition

Rewrite the array declarations so both the unpacked (original) and
packed LUT arrays can use the same value definitions. This is done by
defining a macro that "knows what to do".

This makes the code much easier to read and maintain.

* Fix macro typos and improve perf

Pack the bits in a more efficient order for extraction.
And also fix the copy/paste error in the macro...

* Switch fully to packed LUT

Some minor reformatting.
Compile tested all sendstring_xyz.h to make sure they were converted
properly. Also checked that an unconverted version would generate a
compile error.

* Apply whitespace suggestions from code review

Co-Authored-By: Ryan <[email protected]>

Co-authored-by: Ryan <[email protected]>
BorisTestov pushed a commit to BorisTestov/qmk_firmware that referenced this pull request May 23, 2024
* Reduce PROGMEM usage for keycode map

Bit-pack the keycode bool array to gain back a small amount of flash space.
The trade-off is an increase in runtime instructions when running macros.

It does make the code a bit harder to read, as well as maintain.

For configs that use send_string() et al, it saves ~100 bytes.

* Switch to macro and common definition

Rewrite the array declarations so both the unpacked (original) and
packed LUT arrays can use the same value definitions. This is done by
defining a macro that "knows what to do".

This makes the code much easier to read and maintain.

* Fix macro typos and improve perf

Pack the bits in a more efficient order for extraction.
And also fix the copy/paste error in the macro...

* Switch fully to packed LUT

Some minor reformatting.
Compile tested all sendstring_xyz.h to make sure they were converted
properly. Also checked that an unconverted version would generate a
compile error.

* Apply whitespace suggestions from code review

Co-Authored-By: Ryan <[email protected]>

Co-authored-by: Ryan <[email protected]>
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.

6 participants