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

Change lib8tion library to be usable in user keymaps #5598

Merged
merged 5 commits into from
Apr 30, 2019

Conversation

alecgeatches
Copy link
Contributor

This PR moves a header-defined constant in the lib8tion library to an implementation file so that FastLED library functions can be included and used in user code. Using the FastLED functions in code outside of rgb_matrix.c was previously not possible due to the header-defined b_m16_interleave array. See #5597:

[@alecgeatches: ] I'm having an issue using any of the lib8tion functions in my custom code. If I try adding #include "lib/lib8tion/lib8tion.h" to my keymap file, I get a linker error:

$ make ergodox_ez:alecg
QMK Firmware 0.6.328
Making ergodox_ez with keymap alecg

# ...

 [OK]
Linking: .build/ergodox_ez_alecg.elf                                                                [ERRORS]
 |
 | .build/obj_ergodox_ez_alecg/quantum/rgb_matrix.o:(.rodata.b_m16_interleave+0x0): multiple definition of `b_m16_interleave'
 | .build/obj_ergodox_ez_alecg/keyboards/ergodox_ez/keymaps/alecg/keymap.o:(.rodata.b_m16_interleave+0x0): first defined here
 | collect2: error: ld returned 1 exit status
 |
tmk_core/rules.mk:290: recipe for target '.build/ergodox_ez_alecg.elf' failed
make[1]: *** [.build/ergodox_ez_alecg.elf] Error 1
Make finished with errors
Makefile:535: recipe for target 'ergodox_ez:alecg' failed
make: *** [ergodox_ez:alecg] Error 1

From what I understand, this is due to the b_m16_interleave constant being defined in the header file lib/lib8tion/trig8.h included from lib8tion.h. Since the constant is defined in the header file, including the same header from anywhere else causes a linker error becuase it's already included in quantum/rgb_matrix.c.

[@pelrun: ] Simplest fix is to add 'static' to the definition of b_m16_interleave, which will remove any external visibility. A more complete fix will be to move the definition to a C file and include it in the build, but it's not necessary unless you're desperate to save a dozen bytes of ram or flash.

This PR takes the second approach since it seems a little cleaner and will save a handful of bytes for RGB_MATRIX_ENABLEd keyboards.

This is my first time contributing here, so please let me know if you'd prefer the static approach or would rather this be accomplished in a different way. Thank you!

lib/lib8tion/trig8.c Outdated Show resolved Hide resolved
common_features.mk Outdated Show resolved Hide resolved
common_features.mk Outdated Show resolved Hide resolved
common_features.mk Outdated Show resolved Hide resolved
@alecgeatches
Copy link
Contributor Author

@pelrun Thank you for the notes! I have addressed them.

@XScorpion2
Copy link
Contributor

XScorpion2 commented Apr 10, 2019

With the custom animation pr coming up, you will be able to use the functions directly without any changes to lib8tion fyi #5338

This is because this method pulls you animation code into the rgb matrix compile unit after the math lib, so you will be able to access it without doing any includes on your end.

@pelrun
Copy link
Contributor

pelrun commented Apr 10, 2019

Okay, looks like the build is having a tough time generating the lib8tion.a file, it's broken lots of keyboards.

@alecgeatches
Copy link
Contributor Author

alecgeatches commented Apr 10, 2019

With the custom animation pr coming up, you will be able to use the functions directly without any changes to lib8tion fyi #5338

This is because this method pulls you animation code into the rgb matrix compile unit after the math lib, so you will be able to access it without doing any includes on your end.

@XScorpion2 That will be awesome! Thank you for all of your work on improving the animation support in QMK, it's a huge help for new people like me getting into the ecosystem. Do you think it's worth making these changes in case a user wants to use these functions in custom code? I understand it'll be making changes to the underlying lib8tion library, but I think there still might be use in cleaning it up so it can be included from multiple sources if necessary.

Okay, looks like the build is having a tough time generating the lib8tion.a file, it's broken lots of keyboards.

@pelrun Thank you for your help here. I can take a look at the build breaks to see if I can figure out why they're happening.

@XScorpion2
Copy link
Contributor

I think it's useful as you said to allow others to use these faster math functions. As for your change, I think you can get the same effect with a simpler change by just marking the array static.

My main thoughts are around this point: how likely are we to take future updates to this lib?

Right now most of this lib is inlined for performance costing ~150 bytes (with lto) of firmware space. So I do like the idea of ripping apart this lib into reusable c/h files and reduce in line code duplication at the cost of a bit of performance, but that definitely will prevent future lib updates easily. Tbh I'm really leaning towards this approach but just want to make 100% sure before pulling the trigger.

@drashna
Copy link
Member

drashna commented Apr 11, 2019

Okay, looks like the build is having a tough time generating the lib8tion.a file, it's broken lots of keyboards.

Specifically, ARM boards are having this issue.

@pelrun
Copy link
Contributor

pelrun commented Apr 11, 2019

Ah, there aren't any functions in there on an ARM build, just the variables. Libraries mustn't like that.

@alecgeatches
Copy link
Contributor Author

Build is still failing, but from what I can tell the keyboards/zeal60/rgb_backlight.c file causing the errors is also failing for other builds like this unrelated one from 2 days ago. All of the other keyboards are compiling, so I think this change is passing otherwise.

[@XScorpion2: ] As for your change, I think you can get the same effect with a simpler change by just marking the array static.
...
Right now most of this lib is inlined for performance costing ~150 bytes (with lto) of firmware space. So I do like the idea of ripping apart this lib into reusable c/h files and reduce in line code duplication at the cost of a bit of performance, but that definitely will prevent future lib updates easily. Tbh I'm really leaning towards this approach but just want to make 100% sure before pulling the trigger.

@pelrun @XScorpion2 The code has been changed to match your suggestions to just convert the constant to static. @XScorpion2's suggestion for a full header rewrite to the lib8tion library sounds like a good idea, but tackling this is a little beyond my expertise.

I'm hoping this is a good compromise as it changes as little about the library as possible (in case of rewrite or wanting parity with lib8tion's main repo), and doesn't break ARM builds. Does anyone have issues with this workaround for the time being? Thanks!

Copy link
Contributor

@XScorpion2 XScorpion2 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, failures in CI look unrelated to this change.

@drashna drashna merged commit 75d72c2 into qmk:master Apr 30, 2019
akrob pushed a commit to akrob/qmk_firmware that referenced this pull request Apr 30, 2019
* upstream/master: (779 commits)
  [Keyboard] Signum3.0 remove sortedcontainers (qmk#5679)
  Simple extended space cadet (qmk#5277)
  Removed forced in lining for lib8tion functions (qmk#5670)
  Change lib8tion library to be usable in user keymaps (qmk#5598)
  [Keyboard] Fixing drag-and-drop (qmk#5728)
  [Keyboard] Adding ortho_4x12 & planck_mit layouts for KBD4X (qmk#5729)
  [Keyboard] Minor fixes for Baguette (qmk#5737)
  Updated rgb_led struct field modifier to flags (qmk#5619)
  RGB Matrix: Custom effects on a kb/user level (qmk#5338)
  Fix Planck and Preonic builds (qmk#5658)
  [Keymap] dz60 keymap w/ hhkb-esque default layer (qmk#5708)
  [Keymap] Added compatibility for Planck rev6 (qmk#5706)
  [Keyboard] Satisfaction75 i2c fix and VIA layout (qmk#5726)
  A better new_project.sh (qmk#5191)
  Fix sendstring "#" producing "£" instead (qmk#5724)
  [Keyboard] Added WT69-A PCB (qmk#5721)
  [Keymap] Fix typo and function layer image for Quefrency (qmk#5719)
  [Keymap] Initial keyboard layout for KBD67 (qmk#5720)
  [Keymap] New keymap for Quefrency 65% with split backspace, RGB, media keys, mouse keys (qmk#5717)
  [Keyboard] Update Gergo to use newer Ergodox Matrix code (qmk#5703)
  ...
drashna pushed a commit to zsa/qmk_firmware that referenced this pull request Apr 30, 2019
* Move lib8tion header-defined constant into implementation file, add to build

* Move b_m16_interleave initializtion to lib8tion.c, change build to include lib8tion.c in QUANTUM_LIB_SRC

* Remove left-over whitespace

* Move lib8tion include by RGB_MATRIX_ENABLE code in makefile

* Revert build changes and change lib8tion b_m16_interleave constant to static
foosinn pushed a commit to foosinn/qmk_firmware that referenced this pull request May 6, 2019
* Move lib8tion header-defined constant into implementation file, add to build

* Move b_m16_interleave initializtion to lib8tion.c, change build to include lib8tion.c in QUANTUM_LIB_SRC

* Remove left-over whitespace

* Move lib8tion include by RGB_MATRIX_ENABLE code in makefile

* Revert build changes and change lib8tion b_m16_interleave constant to static
Timbus pushed a commit to Timbus/qmk_firmware that referenced this pull request Jun 23, 2019
* Move lib8tion header-defined constant into implementation file, add to build

* Move b_m16_interleave initializtion to lib8tion.c, change build to include lib8tion.c in QUANTUM_LIB_SRC

* Remove left-over whitespace

* Move lib8tion include by RGB_MATRIX_ENABLE code in makefile

* Revert build changes and change lib8tion b_m16_interleave constant to static
ridingqwerty pushed a commit to ridingqwerty/qmk_firmware that referenced this pull request Jan 10, 2020
* Move lib8tion header-defined constant into implementation file, add to build

* Move b_m16_interleave initializtion to lib8tion.c, change build to include lib8tion.c in QUANTUM_LIB_SRC

* Remove left-over whitespace

* Move lib8tion include by RGB_MATRIX_ENABLE code in makefile

* Revert build changes and change lib8tion b_m16_interleave constant to static
JeffreyPalmer pushed a commit to JeffreyPalmer/qmk_firmware that referenced this pull request Feb 27, 2020
* Move lib8tion header-defined constant into implementation file, add to build

* Move b_m16_interleave initializtion to lib8tion.c, change build to include lib8tion.c in QUANTUM_LIB_SRC

* Remove left-over whitespace

* Move lib8tion include by RGB_MATRIX_ENABLE code in makefile

* Revert build changes and change lib8tion b_m16_interleave constant to static
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