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.

Sign up for GitHub

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

Jump to bottom

LFK78 refactor #7835

Merged
merged 12 commits into from
Mar 3, 2020
Merged

LFK78 refactor #7835

merged 12 commits into from
Mar 3, 2020

Conversation

fauxpark
Copy link
Member

@fauxpark fauxpark commented Jan 8, 2020

Description

Attempting to bring the lfkeyboards directory more in line with current standards, starting with the LFK78 as someone on Discord was having trouble with it recently (they needed to set the correct LFK_REV in rules.mk - lucky they were not using Configurator!).

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

@fauxpark fauxpark requested a review from a team January 8, 2020 23:42
@noroadsleft
Copy link
Member

I like this PR in concept. Just a couple issues that I'm not okay with.

  • default keymap doesn't compile on revb, due to firmware size. It's like 3kB over. Even if I do it locally in avr-gcc 8.3.0, it's 2.5kB oversize. AUDIO_ENABLE = yes for me is nearly 9kB. I'm not sure how (or if) we solve this.
  • I have a strong dislike for when revision folders are "hyphenated". I'd rather have revc_h be revc, and then note in a readme it supports revisions C through H.

drashna
drashna requested changes Jan 9, 2020
View reviewed changes
Copy link
Member

@drashna drashna left a comment •
edited
Loading

Choose a reason for hiding this comment

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

If this is using a single color backlight, I would really rather it be converted to LED Matrix, if possible.
https://docs.qmk.fm/#/feature_led_matrix

If it's RGB per key, then rgb matrix:
https://docs.qmk.fm/#/feature_rgb_matrix

The reason for this, is that we have a bunch of maintained code, including i2c/twi and the ISSI controller.

However, that may be best to put off till later

As for the size part, disabling music mode may help with the audio being enabled. Or enabling LTO (or both).

@fauxpark
Copy link
Member Author

fauxpark commented Jan 9, 2020

@noroadsleft good point on the revision folders. Changed to revc and added some stuff in the readme.
On the revB, it already compiles too large on master (Travis doesn't pick it up because LFK_REV = J). Turning off Music mode still result in 710 bytes over :/

@drashna I'll leave pulling out all the custom code for another PR, but yeah, I suspect all of it could be converted to their respective core features instead. That might help with the size...

@drashna
Copy link
Member

drashna commented Jan 12, 2020

it looks like ca178858 is looking for LAYOUT_split_rshift, and iso is looking for .... LAYOUT_iso for the lfk78 keyboard.

@fauxpark
Copy link
Member Author

Yep, neither of which are supported on the revB.

@scott-t-wilson you originally PR'ed this board, thoughts? (not sure if you are also LFKeyboards)

@stale
Copy link

stale bot commented Feb 26, 2020

Thank you for your contribution!
This pull request has been automatically marked as stale because it has not had activity in the last 45 days. It will be closed in 30 days if no further activity occurs. Please feel free to give a status update now, or re-open when it's ready.
For maintainers: Please label with awaiting review, breaking_change, in progress, or on hold to prevent the issue from being re-flagged.

@stale stale bot removed the awaiting changes label Feb 27, 2020
@drashna
Copy link
Member

drashna commented Mar 1, 2020

Rebase needed

@fauxpark
Copy link
Member Author

fauxpark commented Mar 1, 2020

All done

noroadsleft
noroadsleft approved these changes Mar 3, 2020
View reviewed changes
Copy link
Member

@noroadsleft noroadsleft 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.

@noroadsleft noroadsleft requested a review from drashna March 3, 2020 21:44
@drashna drashna merged commit 8c3ff3f into qmk:master Mar 3, 2020
@noroadsleft noroadsleft mentioned this pull request Mar 4, 2020
Merged
@fauxpark fauxpark deleted the lfk78-refactor branch March 4, 2020 04:51
HokieGeek pushed a commit to HokieGeek/qmk_firmware that referenced this pull request Mar 5, 2020
@fauxpark @HokieGeek
[Keyboard] LFK78 refactor (qmk#7835)
57f689a 
* Change include guards to pragma once

* Clean up default keymaps

* Remove some magic numbers and use GPIO macros

* Clean up keyboard.[ch]

* Tidy up info.json and readme

* Align config.h with template

* Split up revision code into subfolders

* rev C-H has no audio, apparently

* Change revc_h to revc and document differences

* Turn off Audio on revb for now, for Travis' sake

* Split info.json into revision folders

* Clean up default keymaps some more
nesth pushed a commit to nesth/qmk_firmware that referenced this pull request Mar 5, 2020
Merge remote-tracking branch 'upstream/master'
4aeb119 
* 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)
  ...
c0psrul3 pushed a commit to c0psrul3/qmk_firmware that referenced this pull request Mar 23, 2020
@fauxpark @c0psrul3
[Keyboard] LFK78 refactor (qmk#7835)
567a50b 
* Change include guards to pragma once

* Clean up default keymaps

* Remove some magic numbers and use GPIO macros

* Clean up keyboard.[ch]

* Tidy up info.json and readme

* Align config.h with template

* Split up revision code into subfolders

* rev C-H has no audio, apparently

* Change revc_h to revc and document differences

* Turn off Audio on revb for now, for Travis' sake

* Split info.json into revision folders

* Clean up default keymaps some more
sowbug pushed a commit to sowbug/qmk_firmware that referenced this pull request Apr 2, 2020
@fauxpark @sowbug
[Keyboard] LFK78 refactor (qmk#7835)
7059ea6 
* Change include guards to pragma once

* Clean up default keymaps

* Remove some magic numbers and use GPIO macros

* Clean up keyboard.[ch]

* Tidy up info.json and readme

* Align config.h with template

* Split up revision code into subfolders

* rev C-H has no audio, apparently

* Change revc_h to revc and document differences

* Turn off Audio on revb for now, for Travis' sake

* Split info.json into revision folders

* Clean up default keymaps some more
kylekuj pushed a commit to kylekuj/qmk_firmware that referenced this pull request Apr 21, 2020
@fauxpark @kujawakk
[Keyboard] LFK78 refactor (qmk#7835)
07a0c63 
* Change include guards to pragma once

* Clean up default keymaps

* Remove some magic numbers and use GPIO macros

* Clean up keyboard.[ch]

* Tidy up info.json and readme

* Align config.h with template

* Split up revision code into subfolders

* rev C-H has no audio, apparently

* Change revc_h to revc and document differences

* Turn off Audio on revb for now, for Travis' sake

* Split info.json into revision folders

* Clean up default keymaps some more
jakeisnt pushed a commit to jakeisnt/qmk_firmware that referenced this pull request Aug 20, 2020
@fauxpark @jakeisnt
[Keyboard] LFK78 refactor (qmk#7835)
f1eb370 
* Change include guards to pragma once

* Clean up default keymaps

* Remove some magic numbers and use GPIO macros

* Clean up keyboard.[ch]

* Tidy up info.json and readme

* Align config.h with template

* Split up revision code into subfolders

* rev C-H has no audio, apparently

* Change revc_h to revc and document differences

* Turn off Audio on revb for now, for Travis' sake

* Split info.json into revision folders

* Clean up default keymaps some more
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@noroadsleft noroadsleft noroadsleft approved these changes

@drashna drashna drashna approved these changes

Assignees
No one assigned
Labels
awaiting review enhancement keyboard keymap
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@fauxpark @noroadsleft @drashna