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

Kumo Keyboard added with 3 layouts #5770

Closed
wants to merge 14 commits into from
Closed

Kumo Keyboard added with 3 layouts #5770

wants to merge 14 commits into from

Conversation

LEdiodes
Copy link
Contributor

@LEdiodes LEdiodes commented May 3, 2019

Description

I added a keyboard, the Minivan Kumo which was a kickstarter keyboard

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

LEdiodes added 2 commits May 3, 2019 12:14
added readme.md files for the layouts and the Kumo keyboard
Copy link
Contributor

@mechmerlin mechmerlin left a comment

Choose a reason for hiding this comment

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

Quite a few general comments here

  1. rules.mk, and config.h are included as part of every keymap here. I would take the common parts of each and just stick it into the parent kumo rules.mk and config.h. Only specific things to that Keymap need to be in there.

  2. Kumo should be in thevankeyboards directory

  3. There doesn't seem to be a QMK Configurator info.json file

  4. What you have here are actually 3 different keymaps not layouts. A layout refers to the physical layout of a board, what keys go where and what keys are what size. A keymap is how each key in that specific layout is programmed.

  5. Also this seems to share the exact same matrix/pins as the minivan. The physical layout itself is the LAYOUT_arrow as you can see in https://config.qmk.fm/#/thevankeyboards/minivan/LAYOUT_arrow

Perhaps simply updating the minivan readme and throwing in these keymaps would suffice?

keyboards/kumo/config.h Outdated Show resolved Hide resolved
keyboards/kumo/config.h Outdated Show resolved Hide resolved
keyboards/kumo/config.h Outdated Show resolved Hide resolved
#define VENDOR_ID 0xFEAE
#define PRODUCT_ID 0x8847
#define DEVICE_VER 0x0001
#define MANUFACTURER TheVan Keyboards
Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably put this in thevankeyboards directory

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahh I did not know this existed, thanks!

keyboards/kumo/keymaps/LEdiodes/config.h Outdated Show resolved Hide resolved
keyboards/kumo/keymaps/LEdiodes/keymap.c Outdated Show resolved Hide resolved
* | RESET | | | | HOME+END | |VOL- |MUTE |VOL+ |
* `----------------------------------------------------------------------------'
*/
[L1] = KEYMAP(LT(KC_F1, KC_GRV), KC_F2, KC_F3, KC_F4, KC_F5, KC_F6, KC_F7, KC_F8, KC_F9, KC_F10, KC_F11, KC_F12, \
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
[L1] = KEYMAP(LT(KC_F1, KC_GRV), KC_F2, KC_F3, KC_F4, KC_F5, KC_F6, KC_F7, KC_F8, KC_F9, KC_F10, KC_F11, KC_F12, \
[L1] = LAYOUT(LT(KC_F1, KC_GRV), KC_F2, KC_F3, KC_F4, KC_F5, KC_F6, KC_F7, KC_F8, KC_F9, KC_F10, KC_F11, KC_F12, \

keyboards/kumo/keymaps/LEdiodes/keymap.c Outdated Show resolved Hide resolved
keyboards/kumo/keymaps/LEdiodes/keymap.c Outdated Show resolved Hide resolved
keyboards/kumo/keymaps/LEdiodes/keymap.c Outdated Show resolved Hide resolved
#define DESCRIPTION Hotswap MiniVan 40% (Kumo)
#define MATRIX_ROWS 4
#define MATRIX_COLS 12
#define MATRIX_ROW_PINS { D7, B5, F7, D4 }
Copy link
Contributor

Choose a reason for hiding this comment

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

Do the rows and col pins used change per keymap?

@drashna drashna requested review from drashna, noroadsleft and yanfali May 4, 2019 00:44
#define TAPPING_TERM 175
#define LOCKING_SUPPORT_ENABLE
#define LOCKING_RESYNC_ENABLE
#define IS_COMMAND() ( \
Copy link
Member

Choose a reason for hiding this comment

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

Lines 23-25 should be commented out. This definition is only necessary if you're not using the default value.

Reference: #4301

Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above, I recommend deleting them.

keyboards/kumo/keymaps/LEdiodes/config.h Outdated Show resolved Hide resolved
#define LOCKING_RESYNC_ENABLE

/* key combination for command */
#define IS_COMMAND() ( \
Copy link
Member

Choose a reason for hiding this comment

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

Lines 25-28 should be commented out. This definition is only necessary if you're not using the default value.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd recommend deleting them, actually.

// #define AUTO_SHIFT_TIMEOUT 100000
#define NO_AUTO_SHIFT_SPECIAL

#define IS_COMMAND() ( \
Copy link
Member

Choose a reason for hiding this comment

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

Lines 37-39 should be deleted.

)

/* prevent stuck modifiers */
#define PREVENT_STUCK_MODIFIERS
Copy link
Member

Choose a reason for hiding this comment

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

This setting is now the default behavior for QMK. Lines 30-31 should be deleted.

Reference: #3107

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sweet!

#define LOCKING_RESYNC_ENABLE

/* key combination for command */
#define IS_COMMAND() ( \
Copy link
Member

Choose a reason for hiding this comment

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

Lines 25-28 should be commented out.

)

/* prevent stuck modifiers */
#define PREVENT_STUCK_MODIFIERS
Copy link
Member

Choose a reason for hiding this comment

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

This setting is now the default behavior for QMK. Lines 30-31 should be deleted.

// Southpaw + arrow layout for MiniVan Kumo
// Created by LEdiodes on 5-2-19

#include "kumo.h"
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
#include "kumo.h"
#include QMK_KEYBOARD_H

// Created by LEdiodes on 5-2-19

#include "kumo.h"
#include "action_layer.h"
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
#include "action_layer.h"

// MiniVan Kumo deafult keymap
// Created by LEdiodes on 5-2-19

#include "Kumo.h"
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
#include "Kumo.h"
#include QMK_KEYBOARD_H

mechmerlin and others added 12 commits May 6, 2019 07:34
Co-Authored-By: LEdiodes <14025111+LEdiodes@users.noreply.github.com>
Co-Authored-By: LEdiodes <14025111+LEdiodes@users.noreply.github.com>
Co-Authored-By: LEdiodes <14025111+LEdiodes@users.noreply.github.com>
Co-Authored-By: LEdiodes <14025111+LEdiodes@users.noreply.github.com>
Co-Authored-By: LEdiodes <14025111+LEdiodes@users.noreply.github.com>
Co-Authored-By: LEdiodes <14025111+LEdiodes@users.noreply.github.com>
Co-Authored-By: LEdiodes <14025111+LEdiodes@users.noreply.github.com>
Co-Authored-By: LEdiodes <14025111+LEdiodes@users.noreply.github.com>
renaming keymap to layout

Co-Authored-By: LEdiodes <14025111+LEdiodes@users.noreply.github.com>
ahh sweet! this must be newish.. good to know, thanks!

Co-Authored-By: LEdiodes <14025111+LEdiodes@users.noreply.github.com>
Co-Authored-By: LEdiodes <14025111+LEdiodes@users.noreply.github.com>
removing #endif after getting advice on this.

Co-Authored-By: LEdiodes <14025111+LEdiodes@users.noreply.github.com>
RGBLIGHT_ENABLE = yes # Enable keyboard underlight functionality (+4870)
BACKLIGHT_ENABLE = yes # Enable keyboard backlight functionality (+1150)
TAP_DANCE_ENABLE = yes #
AUTO_SHIFT_ENABLE = yes # Enables autoshift
Copy link
Contributor

@vomindoraan vomindoraan May 6, 2019

Choose a reason for hiding this comment

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

It appears that this is a rules.mk file for a specific keymap. You actually don't need most of this stuff in here, only the lines that are different from the keyboard's base rules.mk file (keyboards/kumo/rules.mk).

Actually, it looks like you've got the base and keymap rules.mk files reversed. This should be the core keyboards/kumo/rules.mk file since it has all the MCU, ARCH etc. definitions; keyboards/kumo/keymaps/SeaSpider/rules.mk should only contain the few things that are different, namely:

CONSOLE_ENABLE = no
COMMAND_ENABLE = no

(by the way, I don't recommend disabling Command unless you're low on space)

Similarly, only the main config.h file (keyboards/kumo/config.h) should contain the VENDOR_ID, PRODUCT_ID, etc.; the config.h files in the keymaps don't need any of this.

Finally, you should probably make a default keymap for the keyboard (keyboards/kumo/keymaps/default).

Take a look at how some of the other keyboards organize their base files and keymap-specific files.

Copy link
Member

@drashna drashna left a comment

Choose a reason for hiding this comment

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

Any update on this?

@@ -0,0 +1,41 @@
#pragma once
#include "config_common.h"
#define VENDOR_ID 0xFEAE
Copy link
Member

Choose a reason for hiding this comment

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

Actually, most of this isn't needed. Only things changed from the base keyboard should be in here.

),
};
// Macros
const macro_t *action_get_macro(keyrecord_t *record, uint8_t id, uint8_t opt) {
Copy link
Member

Choose a reason for hiding this comment

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

Could you remove the action_get_macro function? It's been deprecated in favor of process_record_user.

@@ -0,0 +1,70 @@
# MCU name
MCU = atmega32u4
Copy link
Member

Choose a reason for hiding this comment

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

All of this stuff isn't needed.

Just the stuff in needed to be changed from default are needed here.

@mechmerlin
Copy link
Contributor

mechmerlin commented May 10, 2019

@LEdiodes hello just wanted to know if you had looked into my point 5. Can this just be part of the original minivan port or is there something I'm missing here.

@drashna
Copy link
Member

drashna commented Jul 30, 2019

Any update on this?

@drashna
Copy link
Member

drashna commented Nov 16, 2019

Given the amount of time that this has been inactive, and hasn't had a response, i'm going to close this.

if you wish to update this and get it merged in, feel free to re-open this PR, or resubmit it as a new PR.

@drashna drashna closed this Nov 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants