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

[Keyboard] add kbd0 Curve0 60 ANSI #24609

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

[Keyboard] add kbd0 Curve0 60 ANSI #24609

wants to merge 3 commits into from

Conversation

kbd0
Copy link

@kbd0 kbd0 commented Nov 16, 2024

Adding a keyboard

Description

Adding the Curve0 60 ANSI keyboard

Types of Changes

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

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

keyboards/kbd0/curve0/60_ansi/keyboard.json Outdated Show resolved Hide resolved
keyboards/kbd0/curve0/60_ansi/readme.md Outdated Show resolved Hide resolved
keyboards/kbd0/curve0/60_ansi/keymaps/default/keymap.json Outdated Show resolved Hide resolved
@waffle87 waffle87 requested a review from a team November 16, 2024 20:17
@dunk2k
Copy link
Contributor

dunk2k commented Nov 17, 2024

Based on reference image:

  1. Could layout information for this keyboard be altered to conform to PR Checklist regarding keyboards/PCBs with multiple layout options. (i.e. LAYOUT_all)
  2. Compatible Community Layouts are:
    • 60_ansi
    • 60_ansi_split_bs_rshift

@kbd0
Copy link
Author

kbd0 commented Nov 17, 2024

Based on reference image:

  1. Could layout information for this keyboard be altered to conform to PR Checklist regarding keyboards/PCBs with multiple layout options. (i.e. LAYOUT_all)

  2. Compatible Community Layouts are:

    • 60_ansi
    • 60_ansi_split_bs_rshift

Am unsure how best to proceed here.

The keyboard supports split backspace, split left shift, and split right shift both ways (1u 1.75u or 1.75u 1u). This results in 2 * 2 * 3 = 12 possible layouts. Having 12 layouts to choose from seems annoying for the user but maybe this is the best way?

The PCB and default keymap are designed to work for most cases by default (backspace is either the 2u key or the right 1u key; left shift is either the 2.25u key or the (left) 1.25u key, right shift is either the 2.75u key or the (left) 1.75u key). But I can see how having just the full layout can be confusing

@dunk2k
Copy link
Contributor

dunk2k commented Nov 17, 2024

Am unsure how best to proceed here.

  1. As per PR Checklist - in keyboard.json a LAYOUT_all layout will need to be provided in "layouts"
  2. Layout macro/name in default keymap changed to LAYOUT_all
  3. Add 60_ansi and 60_ansi_split_bs_rshift Community Layouts to keyboard.json ("layouts" and "community_layouts")
  4. Optional: include a matrix_diagram.md file (search repo for examples)

I'd agree that a layout entry in "layouts" (keyboard.json file) for every single combination is excessive, matrix_diagram.md will allow experienced users to tailor firmware to their chosen physical layout.

Copy link
Contributor

@dunk2k dunk2k 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 Conversation

Comment on lines +25 to +26
"layouts": {
"LAYOUT": {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"layouts": {
"LAYOUT": {
"community_layouts": ["60_ansi", "60_ansi_split_bs_rshift"],
"layouts": {
"LAYOUT_all": {

{"matrix": [4, 12], "x": 12.5, "y": 4, "w": 1.25},
{"matrix": [4, 13], "x": 13.75, "y": 4, "w": 1.25}
]
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
}
},
"LAYOUT_60_ansi": {
"layout": [
{"matrix": [0, 0], "x": 0, "y": 0},
{"matrix": [0, 1], "x": 1, "y": 0},
{"matrix": [0, 2], "x": 2, "y": 0},
{"matrix": [0, 3], "x": 3, "y": 0},
{"matrix": [0, 4], "x": 4, "y": 0},
{"matrix": [0, 5], "x": 5, "y": 0},
{"matrix": [0, 6], "x": 6, "y": 0},
{"matrix": [0, 7], "x": 7, "y": 0},
{"matrix": [0, 8], "x": 8, "y": 0},
{"matrix": [0, 9], "x": 9, "y": 0},
{"matrix": [0, 10], "x": 10, "y": 0},
{"matrix": [0, 11], "x": 11, "y": 0},
{"matrix": [0, 12], "x": 12, "y": 0},
{"matrix": [0, 14], "x": 13, "y": 0, "w": 2},
{"matrix": [1, 0], "x": 0, "y": 1, "w": 1.5},
{"matrix": [1, 1], "x": 1.5, "y": 1},
{"matrix": [1, 2], "x": 2.5, "y": 1},
{"matrix": [1, 3], "x": 3.5, "y": 1},
{"matrix": [1, 4], "x": 4.5, "y": 1},
{"matrix": [1, 5], "x": 5.5, "y": 1},
{"matrix": [1, 6], "x": 6.5, "y": 1},
{"matrix": [1, 7], "x": 7.5, "y": 1},
{"matrix": [1, 8], "x": 8.5, "y": 1},
{"matrix": [1, 9], "x": 9.5, "y": 1},
{"matrix": [1, 10], "x": 10.5, "y": 1},
{"matrix": [1, 11], "x": 11.5, "y": 1},
{"matrix": [1, 12], "x": 12.5, "y": 1},
{"matrix": [1, 13], "x": 13.5, "y": 1, "w": 1.5},
{"matrix": [2, 0], "x": 0, "y": 2, "w": 1.75},
{"matrix": [2, 1], "x": 1.75, "y": 2},
{"matrix": [2, 2], "x": 2.75, "y": 2},
{"matrix": [2, 3], "x": 3.75, "y": 2},
{"matrix": [2, 4], "x": 4.75, "y": 2},
{"matrix": [2, 5], "x": 5.75, "y": 2},
{"matrix": [2, 6], "x": 6.75, "y": 2},
{"matrix": [2, 7], "x": 7.75, "y": 2},
{"matrix": [2, 8], "x": 8.75, "y": 2},
{"matrix": [2, 9], "x": 9.75, "y": 2},
{"matrix": [2, 10], "x": 10.75, "y": 2},
{"matrix": [2, 11], "x": 11.75, "y": 2},
{"matrix": [2, 12], "x": 12.75, "y": 2, "w": 2.25},
{"matrix": [3, 0], "x": 0, "y": 3, "w": 2.25},
{"matrix": [3, 2], "x": 2.25, "y": 3},
{"matrix": [3, 3], "x": 3.25, "y": 3},
{"matrix": [3, 4], "x": 4.25, "y": 3},
{"matrix": [3, 5], "x": 5.25, "y": 3},
{"matrix": [3, 6], "x": 6.25, "y": 3},
{"matrix": [3, 7], "x": 7.25, "y": 3},
{"matrix": [3, 8], "x": 8.25, "y": 3},
{"matrix": [3, 9], "x": 9.25, "y": 3},
{"matrix": [3, 10], "x": 10.25, "y": 3},
{"matrix": [3, 11], "x": 11.25, "y": 3},
{"matrix": [3, 12], "x": 12.25, "y": 3, "w": 2.75},
{"matrix": [4, 0], "x": 0, "y": 4, "w": 1.25},
{"matrix": [4, 1], "x": 1.25, "y": 4, "w": 1.25},
{"matrix": [4, 2], "x": 2.5, "y": 4, "w": 1.25},
{"matrix": [4, 6], "x": 3.75, "y": 4, "w": 6.25},
{"matrix": [4, 10], "x": 10, "y": 4, "w": 1.25},
{"matrix": [4, 11], "x": 11.25, "y": 4, "w": 1.25},
{"matrix": [4, 12], "x": 12.5, "y": 4, "w": 1.25},
{"matrix": [4, 13], "x": 13.75, "y": 4, "w": 1.25}
]
},
"LAYOUT_60_ansi_split_bs_rshift": {
"layout": [
{"matrix": [0, 0], "x": 0, "y": 0},
{"matrix": [0, 1], "x": 1, "y": 0},
{"matrix": [0, 2], "x": 2, "y": 0},
{"matrix": [0, 3], "x": 3, "y": 0},
{"matrix": [0, 4], "x": 4, "y": 0},
{"matrix": [0, 5], "x": 5, "y": 0},
{"matrix": [0, 6], "x": 6, "y": 0},
{"matrix": [0, 7], "x": 7, "y": 0},
{"matrix": [0, 8], "x": 8, "y": 0},
{"matrix": [0, 9], "x": 9, "y": 0},
{"matrix": [0, 10], "x": 10, "y": 0},
{"matrix": [0, 11], "x": 11, "y": 0},
{"matrix": [0, 12], "x": 12, "y": 0},
{"matrix": [0, 13], "x": 13, "y": 0},
{"matrix": [0, 14], "x": 14, "y": 0},
{"matrix": [1, 0], "x": 0, "y": 1, "w": 1.5},
{"matrix": [1, 1], "x": 1.5, "y": 1},
{"matrix": [1, 2], "x": 2.5, "y": 1},
{"matrix": [1, 3], "x": 3.5, "y": 1},
{"matrix": [1, 4], "x": 4.5, "y": 1},
{"matrix": [1, 5], "x": 5.5, "y": 1},
{"matrix": [1, 6], "x": 6.5, "y": 1},
{"matrix": [1, 7], "x": 7.5, "y": 1},
{"matrix": [1, 8], "x": 8.5, "y": 1},
{"matrix": [1, 9], "x": 9.5, "y": 1},
{"matrix": [1, 10], "x": 10.5, "y": 1},
{"matrix": [1, 11], "x": 11.5, "y": 1},
{"matrix": [1, 12], "x": 12.5, "y": 1},
{"matrix": [1, 13], "x": 13.5, "y": 1, "w": 1.5},
{"matrix": [2, 0], "x": 0, "y": 2, "w": 1.75},
{"matrix": [2, 1], "x": 1.75, "y": 2},
{"matrix": [2, 2], "x": 2.75, "y": 2},
{"matrix": [2, 3], "x": 3.75, "y": 2},
{"matrix": [2, 4], "x": 4.75, "y": 2},
{"matrix": [2, 5], "x": 5.75, "y": 2},
{"matrix": [2, 6], "x": 6.75, "y": 2},
{"matrix": [2, 7], "x": 7.75, "y": 2},
{"matrix": [2, 8], "x": 8.75, "y": 2},
{"matrix": [2, 9], "x": 9.75, "y": 2},
{"matrix": [2, 10], "x": 10.75, "y": 2},
{"matrix": [2, 11], "x": 11.75, "y": 2},
{"matrix": [2, 12], "x": 12.75, "y": 2, "w": 2.25},
{"matrix": [3, 0], "x": 0, "y": 3, "w": 2.25},
{"matrix": [3, 2], "x": 2.25, "y": 3},
{"matrix": [3, 3], "x": 3.25, "y": 3},
{"matrix": [3, 4], "x": 4.25, "y": 3},
{"matrix": [3, 5], "x": 5.25, "y": 3},
{"matrix": [3, 6], "x": 6.25, "y": 3},
{"matrix": [3, 7], "x": 7.25, "y": 3},
{"matrix": [3, 8], "x": 8.25, "y": 3},
{"matrix": [3, 9], "x": 9.25, "y": 3},
{"matrix": [3, 10], "x": 10.25, "y": 3},
{"matrix": [3, 11], "x": 11.25, "y": 3},
{"matrix": [3, 12], "x": 12.25, "y": 3, "w": 1.75},
{"matrix": [3, 13], "x": 14, "y": 3},
{"matrix": [4, 0], "x": 0, "y": 4, "w": 1.25},
{"matrix": [4, 1], "x": 1.25, "y": 4, "w": 1.25},
{"matrix": [4, 2], "x": 2.5, "y": 4, "w": 1.25},
{"matrix": [4, 6], "x": 3.75, "y": 4, "w": 6.25},
{"matrix": [4, 10], "x": 10, "y": 4, "w": 1.25},
{"matrix": [4, 11], "x": 11.25, "y": 4, "w": 1.25},
{"matrix": [4, 12], "x": 12.5, "y": 4, "w": 1.25},
{"matrix": [4, 13], "x": 13.75, "y": 4, "w": 1.25}
]
}

"author": "kbd0",
"keyboard": "kbd0/curve0/60_ansi",
"keymap": "default",
"layout": "LAYOUT",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"layout": "LAYOUT",
"layout": "LAYOUT_all",

Comment on lines +8 to +19
"KC_ESC", "KC_1", "KC_2", "KC_3", "KC_4", "KC_5", "KC_6", "KC_7", "KC_8", "KC_9", "KC_0", "KC_MINS", "KC_EQL", "KC_BSLS", "KC_BSPC",
"KC_TAB", "KC_Q", "KC_W", "KC_E", "KC_R", "KC_T", "KC_Y", "KC_U", "KC_I", "KC_O", "KC_P", "KC_LBRC", "KC_RBRC", "KC_BSLS",
"KC_CAPS", "KC_A", "KC_S", "KC_D", "KC_F", "KC_G", "KC_H", "KC_J", "KC_K", "KC_L", "KC_SCLN", "KC_QUOT", "KC_ENT",
"KC_LSFT", "KC_NUBS", "KC_Z", "KC_X", "KC_C", "KC_V", "KC_B", "KC_N", "KC_M", "KC_COMM", "KC_DOT", "KC_SLSH", "KC_RSFT",
"MO(1)", "KC_LCTL", "KC_LGUI", "KC_LALT", "KC_SPC", "KC_RALT", "KC_RGUI", "KC_APP", "KC_RCTL"
],
[
"KC_GRV", "KC_F1", "KC_F2", "KC_F3", "KC_F4", "KC_F5", "KC_F6", "KC_F7", "KC_F8", "KC_F9", "KC_F10", "KC_F11", "KC_F12", "_______", "KC_DEL",
"_______", "_______", "KC_UP", "_______", "_______", "_______", "_______", "_______", "_______", "_______", "_______", "_______", "_______", "_______",
"_______", "KC_LEFT", "KC_DOWN", "KC_RGHT", "_______", "_______", "_______", "_______", "_______", "_______", "_______", "_______", "_______",
"_______", "_______", "_______", "_______", "_______", "_______", "_______", "_______", "_______", "_______", "_______", "_______", "_______",
"_______", "_______", "_______", "_______", "_______", "_______", "_______", "_______", "_______"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"KC_ESC", "KC_1", "KC_2", "KC_3", "KC_4", "KC_5", "KC_6", "KC_7", "KC_8", "KC_9", "KC_0", "KC_MINS", "KC_EQL", "KC_BSLS", "KC_BSPC",
"KC_TAB", "KC_Q", "KC_W", "KC_E", "KC_R", "KC_T", "KC_Y", "KC_U", "KC_I", "KC_O", "KC_P", "KC_LBRC", "KC_RBRC", "KC_BSLS",
"KC_CAPS", "KC_A", "KC_S", "KC_D", "KC_F", "KC_G", "KC_H", "KC_J", "KC_K", "KC_L", "KC_SCLN", "KC_QUOT", "KC_ENT",
"KC_LSFT", "KC_NUBS", "KC_Z", "KC_X", "KC_C", "KC_V", "KC_B", "KC_N", "KC_M", "KC_COMM", "KC_DOT", "KC_SLSH", "KC_RSFT",
"MO(1)", "KC_LCTL", "KC_LGUI", "KC_LALT", "KC_SPC", "KC_RALT", "KC_RGUI", "KC_APP", "KC_RCTL"
],
[
"KC_GRV", "KC_F1", "KC_F2", "KC_F3", "KC_F4", "KC_F5", "KC_F6", "KC_F7", "KC_F8", "KC_F9", "KC_F10", "KC_F11", "KC_F12", "_______", "KC_DEL",
"_______", "_______", "KC_UP", "_______", "_______", "_______", "_______", "_______", "_______", "_______", "_______", "_______", "_______", "_______",
"_______", "KC_LEFT", "KC_DOWN", "KC_RGHT", "_______", "_______", "_______", "_______", "_______", "_______", "_______", "_______", "_______",
"_______", "_______", "_______", "_______", "_______", "_______", "_______", "_______", "_______", "_______", "_______", "_______", "_______",
"_______", "_______", "_______", "_______", "_______", "_______", "_______", "_______", "_______"
"KC_ESC", "KC_1", "KC_2", "KC_3", "KC_4", "KC_5", "KC_6", "KC_7", "KC_8", "KC_9", "KC_0", "KC_MINS", "KC_EQL", "KC_BSLS", "KC_BSPC",
"KC_TAB", "KC_Q", "KC_W", "KC_E", "KC_R", "KC_T", "KC_Y", "KC_U", "KC_I", "KC_O", "KC_P", "KC_LBRC", "KC_RBRC", "KC_BSLS",
"KC_CAPS", "KC_A", "KC_S", "KC_D", "KC_F", "KC_G", "KC_H", "KC_J", "KC_K", "KC_L", "KC_SCLN", "KC_QUOT", "KC_ENT",
"KC_LSFT", "KC_NUBS", "KC_Z", "KC_X", "KC_C", "KC_V", "KC_B", "KC_N", "KC_M", "KC_COMM", "KC_DOT", "KC_SLSH", "KC_RSFT", "MO(1)",
"KC_LCTL", "KC_LGUI", "KC_LALT", "KC_SPC", "KC_RALT", "KC_RGUI", "KC_APP", "KC_RCTL"
],
[
"KC_GRV", "KC_F1", "KC_F2", "KC_F3", "KC_F4", "KC_F5", "KC_F6", "KC_F7", "KC_F8", "KC_F9", "KC_F10", "KC_F11", "KC_F12", "_______", "KC_DEL",
"_______", "_______", "KC_UP", "_______", "_______", "_______", "_______", "_______", "_______", "_______", "_______", "_______", "_______", "_______",
"_______", "KC_LEFT", "KC_DOWN", "KC_RGHT", "_______", "_______", "_______", "_______", "_______", "_______", "_______", "_______", "_______",
"_______", "_______", "_______", "_______", "_______", "_______", "_______", "_______", "_______", "_______", "_______", "_______", "_______", "_______",
"_______", "_______", "_______", "_______", "_______", "_______", "_______", "_______"

Copy link
Author

Choose a reason for hiding this comment

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

I also need to add keymaps/default_ansi_60 and keymaps/default_60_ansi_split_bs_rshift , correct?

After replacing LAYOUT with LAYOUT_ALL I can no longer build the firmware

make kbd0/curve0/60_ansi:default

Making kbd0/curve0/60_ansi with keymap default

☒ Invalid JSON keymap: C:/.../qmk_firmware/keyboards/kbd0/curve0/60_ansi/keymaps/default/keymap.json : 'LAYOUT_ALL' is not valid under any of the given schemas

Copy link
Member

Choose a reason for hiding this comment

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

You do not need to add additional keymaps for all the possible layouts, it's fine just for them to exist in keyboard.json.

You received this error because the layout naming syntax expects LAYOUT_xxx where xxx is lowercase. So it should be LAYOUT_all.

Copy link
Author

Choose a reason for hiding this comment

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

Wow haha had that wrong for the longest time

Without the additional keymaps, what happens if a user chooses that layout in configurator? Do they need to fill in the entire layout themselves or does it infer from LAYOUT_all

Copy link
Contributor

Choose a reason for hiding this comment

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

Without the additional keymaps, what happens if a user chooses that layout in configurator? Do they need to fill in the entire layout themselves or does it infer from LAYOUT_all

The former.

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.

3 participants