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

Add "wk84" keybaord #24731

Open
wants to merge 125 commits into
base: master
Choose a base branch
from
Open

Add "wk84" keybaord #24731

wants to merge 125 commits into from

Conversation

gskygithub
Copy link
Contributor

Description

Add wk84 keybaord

Types of Changes

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

Issues Fixed or Closed by This PR

Checklist

  • My code follows the code style of this project: C, Python
  • I have read the PR Checklist document and have made the appropriate changes.
  • 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).

Copy link
Member

@waffle87 waffle87 left a comment

Choose a reason for hiding this comment

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

As per the PR checklist:

  • PRs should contain the smallest amount of modifications required for a single change to the codebase
    - multiple keyboards at the same time is not acceptable
    - the smaller the PR, the higher likelihood of a quicker review, higher likelihood of quicker merge, and less chance of conflicts

@drashna
Copy link
Member

drashna commented Dec 22, 2024

Also, it looks like you have keyboard files outside of the keyboard folder. These would need to be moved/removed.

keyboards/glencreag/wk84/readme.md Outdated Show resolved Hide resolved
keyboards/glencreag/wk84/keyboard.json Outdated Show resolved Hide resolved
keyboards/glencreag/wk84/keyboard.json Outdated Show resolved Hide resolved
keyboards/glencreag/wk84/keyboard.json Outdated Show resolved Hide resolved
keyboards/glencreag/wk84/keyboard.json Outdated Show resolved Hide resolved
keyboards/glencreag/wk84/keymaps/default/keymap.c Outdated Show resolved Hide resolved
keyboards/glencreag/wk84/mcuconf.h Outdated Show resolved Hide resolved
keyboards/glencreag/wk84/config.h Outdated Show resolved Hide resolved
@dunk2k
Copy link
Contributor

dunk2k commented Dec 23, 2024

@gskygithub
please could files relating to keyboard projectd/65/projectd_65_iso be removed from this PR? as said keyboard is under PR #21941

@gskygithub gskygithub requested a review from dunk2k December 24, 2024 06:00
Comment on lines +28 to +32
/* RGB Matrix */
#define RGB_MATRIX_FRAMEBUFFER_EFFECTS
#define RGB_MATRIX_STARTUP_MODE 13
#define RGB_TRIGGER_ON_KEYDOWN

Copy link
Member

Choose a reason for hiding this comment

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

These are handled in the keyboard.json.

Suggested change
/* RGB Matrix */
#define RGB_MATRIX_FRAMEBUFFER_EFFECTS
#define RGB_MATRIX_STARTUP_MODE 13
#define RGB_TRIGGER_ON_KEYDOWN

Copy link
Member

Choose a reason for hiding this comment

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

Per PR checklist:

  • default keymaps should be "pristine"
    • bare minimum to be used as a "clean slate" for another user to develop their own user-specific keymap
    • what does pristine mean? no custom keycodes. no advanced features like tap dance or macros. basic mod taps and home row mods would be acceptable where their use is necessary
    • standard layouts preferred in these keymaps, if possible
    • should use encoder map feature, rather than encoder_update_user()
    • default keymap should not enable VIA -- keymaps targeting VIA support should be submitted to the VIA QMK Userspace repository


#include "spi_master.h"

void spi_init(void) {
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason for this?

#define RGB_TRIGGER_ON_KEYDOWN

/* WS2812 */
#define WS2812_SPI_DRIVER SPIDM2
Copy link
Member

Choose a reason for hiding this comment

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

Why is this using a different driver than the one above?


#endif

void keyboard_pre_init_kb(void) {
Copy link
Member

Choose a reason for hiding this comment

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

Is there is a reason for this? This pin doesn't appear to be used, at all.


/* WS2812 */
#define WS2812_SPI_DRIVER SPIDM2
#define WS2812_SPI_DIVISOR 16
Copy link
Member

Choose a reason for hiding this comment

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

Default value.

Suggested change
#define WS2812_SPI_DIVISOR 16

Comment on lines +8 to +10
"dynamic_keymap": {
"layer_count": 4
},
Copy link
Member

Choose a reason for hiding this comment

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

Default value.

Suggested change
"dynamic_keymap": {
"layer_count": 4
},

"layer_count": 4
},
"eeprom": {
"driver": "wear_leveling",
Copy link
Member

Choose a reason for hiding this comment

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

Default value.

Suggested change
"driver": "wear_leveling",

Comment on lines +21 to +25
"console": false,
"extrakey": true,
"mousekey": true,
"nkro": true,
"rgb_matrix": true,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"console": false,
"extrakey": true,
"mousekey": true,
"nkro": true,
"rgb_matrix": true,
"console": false,
"extrakey": true,
"mousekey": true,
"nkro": true,
"rgb_matrix": true

{"matrix": [0, 15], "x": 150, "y": 0, "flags": 4}
],
"react_on_keyup": true,
"led_count": 84,
Copy link
Member

Choose a reason for hiding this comment

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

This is inferred.

Suggested change
"led_count": 84,

Comment on lines +181 to +182
"val_steps": 40,
"speed_steps": 60,
Copy link
Member

Choose a reason for hiding this comment

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

These seem like rather large values to set as the default. Users should configure this themselves if they'd like.

Suggested change
"val_steps": 40,
"speed_steps": 60,

A keyboard, Equipped with the WestBerry Q95 microcontroller.

* Keyboard Maintainer: [GSKY](https://github.com/gksygithub)
* Hardware Supported: WK84
Copy link
Member

Choose a reason for hiding this comment

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

Please provide a link to where this keyboard can be purchased, or at least where more information can be found.

Suggested change
* Hardware Supported: WK84
* Hardware Supported: WK84
* Hardware Availability: *Links to where you can find this hardware*

"center_point": [77, 30],
"driver": "ws2812",
"layout": [
{"matrix": [5, 4], "x": 37, "y": 50, "flags": 4},
Copy link
Member

Choose a reason for hiding this comment

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

These coordinates do not look right.

The expected range of values for { x, y } is the inclusive range { 0..224, 0..64 }.

"splash": true,
"typing_heatmap": true
},
"center_point": [77, 30],
Copy link
Member

Choose a reason for hiding this comment

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

Should not be needed with correct coordinates.

Suggested change
"center_point": [77, 30],

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