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 CRKBD firmware size by reducing layer numbers #5471

Merged
merged 5 commits into from
Apr 18, 2019

Conversation

drashna
Copy link
Member

@drashna drashna commented Mar 23, 2019

Description

Change the layer values to be sequential. This will reduce the firmware size, at it was using 16 layers, but now uses 4.

Unfortunately, the requires changing both the default keymaps, but other keymaps using the layer_state_reader code.

So this necesitate the changes of other keymaps.

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

@Lenbok
Copy link
Contributor

Lenbok commented Mar 28, 2019

Can this be merged? I tested on my new board and it's a nice boost in available space.

@drashna
Copy link
Member Author

drashna commented Mar 28, 2019

@foostan any issues with these changes?

@foostan
Copy link
Contributor

foostan commented Mar 29, 2019

Thanks! LGTM 👍

@Lenbok
Copy link
Contributor

Lenbok commented Mar 31, 2019

Question: I just converted my keymaps to userspace (in Lenbok@74e7e5a), so now _RAISE, _LOWER and _ADJUST are defined in my users/lenbok/lenbok.h. When I I test this PR (regardless of whether I update the #defines in my lenbok.h to correspond with the new layer indices in this PR) now I get:

Compiling: keyboards/crkbd/lib/layer_state_reader.c                                                keyboards/crkbd/lib/layer_state_reader.c: In function ‘read_layer_state’:
keyboards/crkbd/lib/layer_state_reader.c:8:21: error: ‘_RAISE’ undeclared (first use in this function)
 #define L_RAISE (1<<_RAISE)
                     ^
keyboards/crkbd/lib/layer_state_reader.c:20:8: note: in expansion of macro ‘L_RAISE’
   case L_RAISE:
        ^
keyboards/crkbd/lib/layer_state_reader.c:8:21: note: each undeclared identifier is reported only once for each function it appears in
 #define L_RAISE (1<<_RAISE)
                     ^
keyboards/crkbd/lib/layer_state_reader.c:20:8: note: in expansion of macro ‘L_RAISE’
   case L_RAISE:
        ^
keyboards/crkbd/lib/layer_state_reader.c:7:21: error: ‘_LOWER’ undeclared (first use in this function)
 #define L_LOWER (1<<_LOWER)
                     ^
keyboards/crkbd/lib/layer_state_reader.c:23:8: note: in expansion of macro ‘L_LOWER’
   case L_LOWER:
        ^
keyboards/crkbd/lib/layer_state_reader.c:9:22: error: ‘_ADJUST’ undeclared (first use in this function)
 #define L_ADJUST (1<<_ADJUST)
                      ^
keyboards/crkbd/lib/layer_state_reader.c:26:8: note: in expansion of macro ‘L_ADJUST’
   case L_ADJUST:
        ^
 [ERRORS]
 |
 |
 |
tmk_core/rules.mk:369: recipe for target '.build/obj_crkbd_rev1_lenbok/./lib/layer_state_reader.o' failed

Is there something I need to do so that layer_state_reader.c gets my userspace defines?

@drashna
Copy link
Member Author

drashna commented Apr 1, 2019

Ah, yes, this is bugged. :(

This reverts commit 036d347.

Unfortunately, because this is NOT in the keymap itself, the layer macros aren't accessible and will error on commit
@drashna
Copy link
Member Author

drashna commented Apr 2, 2019

@Lenbok I reverted the change, so the code uses hard coded values. That should fix it for now.

However, the proper fix is to move as much of the OLED code into the keymap.c file (or into the folder).

There are some other possible solutions here, but I think the keymap is the best option, long term. However, this PR's goal isn't to fix that. Right now, it's to reduce the compiled size, to avoid feature creep, and unintentional bloat.
I'll open another PR that brings the Corne Keyboard more in line with the Helix code, if that's okay with @foostan.

@foostan
Copy link
Contributor

foostan commented Apr 16, 2019

Sorry for the late reply.

I'll open another PR that brings the Corne Keyboard more in line with the Helix code, if that's okay with @foostan.

Sounds good for me. Thanks!

@drashna drashna mentioned this pull request Apr 16, 2019
6 tasks
@XScorpion2 XScorpion2 mentioned this pull request Apr 17, 2019
13 tasks
Copy link
Contributor

@mechmerlin mechmerlin 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, thanks!

@mechmerlin mechmerlin merged commit a58c66d into qmk:master Apr 18, 2019
@drashna drashna deleted the fix/crkbd_feature_creep branch April 18, 2019 21:20
foosinn pushed a commit to foosinn/qmk_firmware that referenced this pull request May 6, 2019
* Reduce CRKBD firmware size by reducing layer numbers

* Update layer output code based on mtei's suggestion/code

* Fix spacing

* Revert "Update layer output code based on mtei's suggestion/code"

This reverts commit 036d347.

Unfortunately, because this is NOT in the keymap itself, the layer macros aren't accessible and will error on commit

* Add comment for future person
shimesaba-type0 pushed a commit to shimesaba-type0/qmk_firmware that referenced this pull request Jun 22, 2019
* Reduce CRKBD firmware size by reducing layer numbers

* Update layer output code based on mtei's suggestion/code

* Fix spacing

* Revert "Update layer output code based on mtei's suggestion/code"

This reverts commit 036d347.

Unfortunately, because this is NOT in the keymap itself, the layer macros aren't accessible and will error on commit

* Add comment for future person
Timbus pushed a commit to Timbus/qmk_firmware that referenced this pull request Jun 23, 2019
* Reduce CRKBD firmware size by reducing layer numbers

* Update layer output code based on mtei's suggestion/code

* Fix spacing

* Revert "Update layer output code based on mtei's suggestion/code"

This reverts commit 036d347.

Unfortunately, because this is NOT in the keymap itself, the layer macros aren't accessible and will error on commit

* Add comment for future person
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