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

Updating KLOR in QMK Tree #24426

Closed
wants to merge 6 commits into from
Closed

Updating KLOR in QMK Tree #24426

wants to merge 6 commits into from

Conversation

t4corun
Copy link

@t4corun t4corun commented Sep 22, 2024

Submitting my modernization take of @GEIGEIGEIST official firmware. Made it compatible with modern QMK while maintaining the features in the original firmware (haptics, audio, oled, encoders, etc)

Description

  • Configuration is now in .json files whenever possible to align with future QMK objectives
  • The four layout variations are now different boards versus being combined into one file
  • now Pro Micro RP2040 native

Types of Changes

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

@t4corun
Updated KLOR in QMK Tree
97f0a72 
- updated geigeigeist official firmware for modern qmk
- put as much config as possible into JSON files
- split up the layouts as different boards
- now promicro RP2040 native
@tzarc tzarc added invalid pr_checklist_pending Needs changes as per the PR checklist labels Sep 22, 2024
@t4corun
Updated files per linter
f390182 
- added copyright information for the files the linter called out
- updated info.json to remove outdated configuration
@t4corun
Missed copyright in one file. added
2d3e9b9
@t4corun
Copy link
Author

t4corun commented Sep 24, 2024

Linter failed because the geigeigeist/klor folder only has common configuration for each of the KLOR variants yubitsume, konrad, saegewerk, and polydactyl. The layouts are in each variant's keyboard.json file. Please let me know if there are further changes that need to be made

@t4corun
Simplification of default keymap options
69f3644 
- moved keymap config.h options to the kb config.h
- added rgblight to the json files
- added tapping term and tapping term per key to json
@t4corun
Re-adding license information
b289341
@t4corun
Forget these files for the license header
5928dd5
@zvecr
Copy link
Member

zvecr commented Oct 3, 2024

Configuration is now in .json files whenever possible to align with future QMK objectives

Existing configuration was already aligned. Worse, this PR introduces changes which actually deviate further from current standards.

The four layout variations are now different boards versus being combined into one file

From what I can tell this is only for the changes to the RGB Matrix config? (Outside this, its actually preferred to have a single build target but fine if actually required to workaround limitations in the lighting framework.)

now Pro Micro RP2040 native

The more fundamental issue is this change. What are users who built with an 32u4 pro micro, or something like a blok controller expected to use? I cant see us accepting this change.

@t4corun
Copy link
Author

t4corun commented Oct 3, 2024

@zvecr Appreciate your feedback and I'm okay taking this back. So I can learn, would you please elaborate on this PR introduces changes which actually deviate further from current standards?

@zvecr
Copy link
Member

zvecr commented Oct 3, 2024

From a quick skim,

  • Unnecessary "empty" .h and .c files
  • Changes to default behaviour at keyboard level that should be avoided
    • tapping term
  • Setting values that are inferred
    • rgb_matrix.led_count
  • Duplication of defaults
    • MASTER_LEFT
    • DYNAMIC_MACRO_ENABLE
    • split.transport.protocol
  • Removal of keyboard level feature handling
    • OLED rendering
      • previously intentionally implemented for configurator
  • Implementation of _kb callbacks at keymap level
    • oled_task_kb
  • Duplicacation of core behaviours
    • suspend_power_down_kb/suspend_wakeup_init_kb
  • Files not containing licence headers

Plus i'm sure a lot more if I were to be doing a more formal review of the changes.

@t4corun
Copy link
Author

t4corun commented Oct 3, 2024

Those make sense. Thank you again for your feedback. I'll close the PR

@t4corun t4corun closed this Oct 3, 2024
@t4corun t4corun deleted the klor_update branch October 4, 2024 21:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
invalid keyboard keymap pr_checklist_pending Needs changes as per the PR checklist
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@t4corun @zvecr @tzarc