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

kimiko: add encoders #521

Merged
merged 6 commits into from
Aug 18, 2023
Merged

kimiko: add encoders #521

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
cf7211d
kimiko: add encoders
Aug 9, 2023
f906951
kimiko: enable encoders in rules.mk
Aug 10, 2023
7ec67dd
kimiko: fix orientation of left-hand encoder
cstrahan Aug 10, 2023
09cb4f3
kimiko: fix encoder pin typo
cstrahan Aug 10, 2023
81c355a
kimiko: revert encoder changes in info.json
cstrahan Aug 13, 2023
8aabdd1
kimiko: enable encoders
cstrahan Aug 13, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
83 changes: 9 additions & 74 deletions keyboards/keycapsss/kimiko/keymaps/vial/keymap.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -332,77 +332,12 @@ void oled_task_user(void) {
#endif


#ifdef ENCODER_ENABLE
bool encoder_update_user(uint8_t index, bool clockwise) {
// Encoder on master side
if (index == 0) {
switch (get_highest_layer(layer_state)) {
// If the Default (QWERTY) layer is active
case _QWERTY:
// Arrow Up/Down
if (clockwise) {
tap_code(KC_DOWN);
} else {
tap_code(KC_UP);
}
break;

// If the RAISE layer is active
case _RAISE:
// Switch browser tabs
if (clockwise) {
tap_code16(LCTL(KC_TAB));
} else {
tap_code16(RCS(KC_TAB));
}
break;
// If the ADJUST layer is active
case _ADJUST:
// RGB brightness up/down
if (clockwise) {
rgblight_decrease_val(); // tap_code(RGB_VAD);
} else {
rgblight_increase_val(); // tap_code(RGB_VAI);
}
break;
}
}
// Encoder on slave side
else if (index == 1) {
switch (get_highest_layer(layer_state)) {
// If the Default (QWERTY) layer is active
case _QWERTY:
// Scroll by Word
if (clockwise) {
tap_code16(LCTL(KC_RGHT));
} else {
tap_code16(LCTL(KC_LEFT));
}
break;

// If the LOWER layer is active
case _LOWER:
// Volume up/down
if (clockwise) {
tap_code(KC_VOLU);
} else {
tap_code(KC_VOLD);
}
break;

// If the ADJUST layer is active
case _ADJUST:
// RGB hue up/down
if (clockwise) {
// tap_code(RGB_HUI);
rgblight_increase_hue();
} else {
// tap_code(RGB_HUD);
rgblight_decrease_hue();
}
break;
}
}
return true;
}
#endif // ENCODER_ENABLE
#if defined(ENCODER_MAP_ENABLE)
const uint16_t PROGMEM encoder_map[][NUM_ENCODERS][2] = {
[0] = { ENCODER_CCW_CW(KC_MS_WH_UP, KC_MS_WH_DOWN), ENCODER_CCW_CW(KC_VOLD, KC_VOLU) },
[1] = { ENCODER_CCW_CW(RGB_HUD, RGB_HUI), ENCODER_CCW_CW(RGB_SAD, RGB_SAI) },
[2] = { ENCODER_CCW_CW(RGB_VAD, RGB_VAI), ENCODER_CCW_CW(RGB_SPD, RGB_SPI) },
[3] = { ENCODER_CCW_CW(RGB_RMOD, RGB_MOD), ENCODER_CCW_CW(KC_RIGHT, KC_LEFT) },
// Encoder 1 Encoder 2
};
#endif
3 changes: 2 additions & 1 deletion keyboards/keycapsss/kimiko/keymaps/vial/rules.mk
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,4 +5,5 @@ VIAL_ENABLE = yes # Enable vial support

LTO_ENABLE = yes

QMK_SETTINGS = no
QMK_SETTINGS = no
ENCODER_MAP_ENABLE = yes
14 changes: 12 additions & 2 deletions keyboards/keycapsss/kimiko/keymaps/vial/vial.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,17 @@
},
"2,5",
{
"x": 6.5
"x": 0.25
},
"0,0\n\n\n\n\n\n\n\n\ne",
"0,1\n\n\n\n\n\n\n\n\ne",
{
"x": 2
},
"1,0\n\n\n\n\n\n\n\n\ne",
"1,1\n\n\n\n\n\n\n\n\ne",
{
"x": 0.25
},
"7,5"
],
Expand Down Expand Up @@ -277,4 +287,4 @@
]
]
}
}
}
11 changes: 9 additions & 2 deletions keyboards/keycapsss/kimiko/rev1/info.json
View file
Open in desktop
Copy link
Contributor

@lesshonor lesshonor Aug 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Please revert changes to this file. The Vial keymap submission is not an appropriate place to modify pin definitions for boards that were submitted to and still exist in upstream QMK.

To summarize comments I left on a previous PR:

  • if the encoders are wrong for everyone, you should fix them for everyone (i.e.: in QMK)
  • if your particular encoder is backwards because you sourced it yourself vs using the included/intended hardware, add #define ENCODER_DIRECTION_FLIP to your keymap's config.h
  • if the manufacturer couldn't be bothered to source consistent hardware so different runs of the keyboard use incompatible encoders...I'd probably just shrug my shoulders and let people fend for themselves. But you could consider adding additional layouts that allow end-users to flip the encoder directions in the GUI.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if the encoders are wrong for everyone, you should fix them for everyone (i.e.: in QMK)

I agree -- that was a late modification made to this PR once I discovered that you could tweak the pin definitions between left/right hand halves. But this really should go upstream.

I can revert that part for now. Is it alright if I leave the vial.json as I have it currently (wrt encoders)? Or how would you like that handled? Once (/if) the info.json fix goes upstream, the CCW/CC orientation in the UI will be as one would expect.

if the manufacturer couldn't be bothered to source consistent hardware so different runs of the keyboard use incompatible encoders...I'd probably just shrug my shoulders and let people fend for themselves. But you could consider adding additional layouts that allow end-users to flip the encoder directions in the GUI

I think this is more a matter of the keyboard reusing the same PCB for left- and right-hand halves. The encoder is seated into the same through-holes, but on opposite sides. With the way the rest of the PCB laid out, this results in the encoder pins being mirrored between the two halves.

At any rate, I'll see if I can get the pin definitions merged upstream, and also ping the designer of the keyboard.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not particularly fussed about the precise accuracy of the vial.json, as someone who does not own this board and is unlikely to investigate it thoroughly. Reverse the directions now and make a follow-up PR to Vial later, or don't. In either case the keymap is more functional than before, so it gets my nod.

Funnily enough, BenRoe is actually submitting Kimiko rev2 to QMK right now and that submission requires changes to rev1—so your timing really couldn't be better.

Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,18 @@
"diode_direction": "COL2ROW",
"encoder": {
"rotary": [
{"pin_a": "F4", "pin_b": "F5"}
{"pin_a": "F5", "pin_b": "F4"}
]
},
"split": {
"soft_serial_pin": "D2"
"soft_serial_pin": "D2",
"encoder": {
"right": {
"rotary": [
{"pin_a": "F4", "pin_b": "F5"}
]
}
}
},
"ws2812": {
"pin": "D3"
Expand Down